Re: [[RFC] 1/5] API to consolidate PWM devices behind a common user and kernel interface

[Bill Gatliff <bgat@billgatliff.com>]

Mike Frysinger wrote:
> On Mon, Oct 19, 2009 at 16:32, Bill Gatliff wrote:
>   
>> +A generic PWM device framework must accomodate the substantial
>>     
>
> accommodate
>   

Heh, and to think I sometimes get paid to write!  :)

Similar and redundant feedback [snipped]


>> +synchronize, unsynchronize -- (optional) Callbacks to
>> +synchronize/unsynchronize channels.
>>     
>
> perhaps use desynchronize instead ?
>   

I like that idea.

>   
>> --- /dev/null
>> +++ b/drivers/pwm/pwm.c
>> @@ -0,0 +1,692 @@
>> +#define DEBUG 99
>>     
>
> whoops again
>   

Grr.  ADHD guys shouldn't code without their caffeine!  :)


>> +int pwm_register(struct pwm_device *pwm)
>> +{
>> +       p = kcalloc(pwm->nchan, sizeof(struct pwm_channel), GFP_KERNEL);
>>     
>
> sizeof(*p)
>   

Good catch.


>> +       pr_info("%s: %d channel%s\n", pwm->bus_id, pwm->nchan,
>> +               pwm->nchan > 1 ? "s" : "");
>>     
>
> dev_info(pwm->dev, ....)
>   

Yep.  I wonder what I was thinking at the time?  :)

>> +static struct pwm_device *
>> +__pwm_find_device(const char *bus_id)
>> +{
>> +       struct pwm_device *p;
>> +
>> +       list_for_each_entry(p, &pwm_device_list, list)
>> +       {
>>     
>
> cuddle that brace
>   

Yep.

>> +struct pwm_channel *
>> +pwm_request(const char *bus_id,
>> +           int chan,
>> +           const char *requester)
>> +{
>> +       struct pwm_device *p;
>> +
>> +       pr_debug("%s: %s:%d returns %p\n", __func__,
>> +                bus_id, chan, &p->channels[chan]);
>> +       pr_debug("%s: %s:%d returns NULL\n",
>> +                __func__, bus_id, chan);
>>     
>
> dev_dbg(p->dev, ....);
>   

Yep again, and again...

>   
>> +unsigned long pwm_ns_to_ticks(struct pwm_channel *p,
>> +                             unsigned long nsecs)
>> +{
>> +       unsigned long long ticks;
>> +
>> +       ticks = nsecs;
>> +       ticks *= p->tick_hz;
>> +       do_div(ticks, 1000000000);
>> +       return ticks;
>> +}
>> +EXPORT_SYMBOL(pwm_ns_to_ticks);
>>     
>
> perhaps better as a static inline in the header ?
>   

I guess that wouldn't negatively affect modules using the function, so I 
think I agree.


>> +unsigned long pwm_ticks_to_ns(struct pwm_channel *p,
>> +                             unsigned long ticks)
>> +{
>> +       unsigned long long ns;
>> +
>> +       if (!p->tick_hz)
>> +               return 0;
>> +
>> +       ns = ticks;
>> +       ns *= 1000000000UL;
>> +       do_div(ns, p->tick_hz);
>> +       return ns;
>> +}
>> +EXPORT_SYMBOL(pwm_ticks_to_ns);
>>     
>
> this one too
>   

Yep.

>> +static void
>> +pwm_config_percent_to_ticks(struct pwm_channel *p,
>> +                           struct pwm_channel_config *c)
>> +{
>> +       if (c->config_mask & PWM_CONFIG_DUTY_PERCENT) {
>> +               if (c->config_mask & PWM_CONFIG_PERIOD_TICKS)
>> +                       c->duty_ticks = c->period_ticks;
>> +               else
>> +                       c->duty_ticks = p->period_ticks;
>> +
>> +               c->duty_ticks *= c->duty_percent;
>> +               c->duty_ticks /= 100;
>> +               c->config_mask &= ~PWM_CONFIG_DUTY_PERCENT;
>> +               c->config_mask |= PWM_CONFIG_DUTY_TICKS;
>> +       }
>> +}
>>     
>
> if you returned when the mask doesnt match, then you wouldnt need to
> indent the rest of the func
>   

That's a habit I learned in my early days when I had an emulator that 
really didn't like multiple exit points from functions.  And it's been a 
hard habit to break!

>> +int pwm_config(struct pwm_channel *p,
>> +              struct pwm_channel_config *c)
>> +{
>> +       int ret = 0;
>> +
>> +       if (unlikely(!p->pwm->config)) {
>> +               pr_debug("%s: %s:%d has no config handler (-EINVAL)\n",
>> +                        __func__, p->pwm->bus_id, p->chan);
>>     
>
> dev_dbg(p->dev, ....);
>
>   
>> +       switch (c->config_mask & (PWM_CONFIG_PERIOD_TICKS
>> +                                 | PWM_CONFIG_DUTY_TICKS)) {
>> +       case PWM_CONFIG_PERIOD_TICKS:
>> +               if (p->duty_ticks > c->period_ticks) {
>> +                       ret = -EINVAL;
>> +                       goto err;
>> +               }
>> +               break;
>> +       case PWM_CONFIG_DUTY_TICKS:
>> +               if (p->period_ticks < c->duty_ticks) {
>> +                       ret = -EINVAL;
>> +                       goto err;
>> +               }
>> +               break;
>> +       case PWM_CONFIG_DUTY_TICKS | PWM_CONFIG_PERIOD_TICKS:
>> +               if (c->duty_ticks > c->period_ticks) {
>> +                       ret = -EINVAL;
>> +                       goto err;
>> +               }
>> +               break;
>>     
>
> unless i missed something, the first and third case is the same.  so
> probably better (compiled code wise) to write an if statement.
>   

Yea.  Can you tell that code has been "refactored"?  :)

>> +       pr_debug("%s: config_mask %d period_ticks %lu duty_ticks %lu"
>> +                " polarity %d duty_ns %lu period_ns %lu duty_percent %d\n",
>> +                __func__, c->config_mask, c->period_ticks, c->duty_ticks,
>> +                c->polarity, c->duty_ns, c->period_ns, c->duty_percent);
>>     
>
> dev_dbg(p->dev, ....);
>   

Yep.

>> +int pwm_set_period_ns(struct pwm_channel *p,
>> +                     unsigned long period_ns)
>> +unsigned long pwm_get_period_ns(struct pwm_channel *p)
>> +int pwm_set_duty_ns(struct pwm_channel *p,
>> +                   unsigned long duty_ns)
>> +unsigned long pwm_get_duty_ns(struct pwm_channel *p)
>> +int pwm_set_duty_percent(struct pwm_channel *p,
>> +                        int percent)
>> +int pwm_set_polarity(struct pwm_channel *p,
>> +                    int active_high)
>> +int pwm_start(struct pwm_channel *p)
>> +int pwm_stop(struct pwm_channel *p)
>>     
>
> these all look like they should static inlines
>   

I'll double-check each one, but I think you might be right.

>> +static void __pwm_callback(struct pwm_channel *p)
>> +{
>> +       queue_work(pwm_handler_workqueue, &p->handler_work);
>> +       pr_debug("%s:%d handler %p scheduled with data %p\n",
>> +                p->pwm->bus_id, p->chan, p->handler, p->handler_data);
>>     
>
> dev_dbg(p->dev, ....);
>   

Yep.


>> +static struct class pwm_class = {
>> +       .name = "pwm",
>> +       .owner = THIS_MODULE,
>> +
>> +       .class_attrs = pwm_class_attrs,
>> +};
>>     
>
> spurious new line ?
>   

Mentally, I separate the two activities and I guess my code reflects 
that.  :)


>> --- a/include/linux/pwm.h
>> +++ b/include/linux/pwm.h
>> @@ -1,31 +1,170 @@
>>  #ifndef __LINUX_PWM_H
>>  #define __LINUX_PWM_H
>>
>> -struct pwm_device;
>> -
>>  /*
>> - * pwm_request - request a PWM device
>> + * include/linux/pwm.h
>>     
>
> header #ifdef should really be after the comment blob
>   

Agreed.


>> +       PWM_CONFIG_DUTY_TICKS = BIT(0),
>> +       PWM_CONFIG_PERIOD_TICKS = BIT(1),
>> +       PWM_CONFIG_POLARITY = BIT(2),
>> +       PWM_CONFIG_START = BIT(3),
>> +       PWM_CONFIG_STOP = BIT(4),
>>
>> +       PWM_CONFIG_HANDLER = BIT(5),
>> +
>> +       PWM_CONFIG_DUTY_NS = BIT(6),
>>     
>
> spurious new lines ?
>   

More subtle mental "compartmentalizing" that needs to go away now that 
the code has stabilized.

>> +enum {
>> +       FLAG_REQUESTED = 0,
>> +       FLAG_STOP = 1,
>> +};
>>     
>
> this is weird ... you're doing things like:
>     if (pwm->channels[wchan].flags & FLAG_REQUESTED) {
>     if (test_and_set_bit(FLAG_REQUESTED, &p->flags))
>
> but FLAG_REQUESTED is 0 ...
>   

Yea, that first example is obviously very, very wrong.  The enumerations 
are supposed to be bit positions.


b.g.

-- 
Bill Gatliff
bgat@billgatliff.com