lkml.org 
[lkml]   [2010]   [Feb]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PWM PATCH 2/7] Emulates PWM hardware using a high-resolution timer and a GPIO pin
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



\
 
 \ /
  Last update: 2010-02-10 14:53    [from the cache]
©2003-2011 Jasper Spaans