Re: [PWM PATCH 2/7] Emulates PWM hardware using a high-resolution timer and a GPIO pin

[Bill Gatliff <bgat@billgatliff.com>]

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