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@xxxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
