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    [W:0.044 / U:29.780 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site