[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-embedded
Subject:    Re: [PWM PATCH 2/7] Emulates PWM hardware using a high-resolution
From:       Bill Gatliff <bgat () billgatliff ! com>
Date:       2010-02-10 13:42:57
Message-ID: 4B72B7E1.40907 () billgatliff ! com
[Download RAW message or body]

Stanislav O. Bezzubtsev wrote:
> > +
> > +struct gpio_pwm {
> > +	struct pwm_device pwm;
> > +	struct hrtimer t;
> > 
> 
> Wouldn't a little bit longer name "timer" instead of simple "t" increase \
> readability? 

Couldn't hurt.  Done.

> > +static void
> > +gpio_pwm_work (struct work_struct *work)
> > +{
> > +	struct gpio_pwm *gp = container_of(work, struct gpio_pwm, work);
> > +
> > +	if (gp->active)
> > +		gpio_direction_output(gp->gpio, gp->polarity ? 1 : 0);
> > +	else
> > +		gpio_direction_output(gp->gpio, gp->polarity ? 0 : 1);
> > 
> 
> Maybe the following would be better:
> gpio_direction_output(gp->gpio, gp->polarity ^ gp->active)
> Instead of doing several comparisons.
> 

... except that I'm trying to guarantee that only the values '1' or '0'
get sent to gpio_direction_output.  There's nothing in the spec that
says other values are legal, although I'll admit that any nonzero value
is unlikely to cause problems.  Should I be pedantic here?

> > +
> > +	if (gp->active)
> > +		hrtimer_start(&gp->t,
> > +			      ktime_set(0, gp->pwm.channels[0].duty_ticks),
> > +			      HRTIMER_MODE_REL);
> > +	else
> > +		hrtimer_start(&gp->t,
> > +			      ktime_set(0,gp->pwm.channels[0].period_ticks
> > +					- gp->pwm.channels[0].duty_ticks),
> > +			      HRTIMER_MODE_REL);
> > 
> 
> if (gp->active)
> 	t =  ktime_set(0, gp->pwm.channels[0].duty_ticks));
> else
> 	t = ktime_set(0, gp->pwm.channels[0].period_ticks - \
> gp->pwm.channels[0].duty_ticks)); 
> htimer_start(&gp->t, t, HRTIMER_MODE_REL);
> 

Excellent.


> > +
> > +	ret = pwm_register(&gp->pwm);
> > +	if (ret)
> > +		goto err_pwm_register;
> > +
> > +	return 0;
> > +
> > +err_pwm_register:
> > 
> 
> platform_set_drvdata(pdev, 0);
> 

Good catch!


> > +static int __devexit
> > +gpio_pwm_remove(struct platform_device *pdev)
> > +{
> > +	struct gpio_pwm *gp = platform_get_drvdata(pdev);
> > +	int ret;
> > +
> > +	ret = pwm_unregister(&gp->pwm);
> > +	hrtimer_cancel(&gp->t);
> > +	cancel_work_sync(&gp->work);
> > 
> 
> platform_set_drvdata(pdev, 0);
> 

Ditto.

> And there are too much pr_debug & dev_dbg calls. Several of them are inside \
> critical sections or in functions called from critical sections (inside \
> spin_lock_irqsave - spin_lock_irqrestore block I mean). Don't think it is good. 

Ok.  Now that the code is relatively mature, they're unnecessary anyway.


b.g.

-- 
Bill Gatliff
Embedded systems training and consulting
http://billgatliff.com
bgat@billgatliff.com

--
To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[prev in list] [next in list] [prev in thread] [next in thread]