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

[Mike Frysinger <vapier.adi@gmail.com>]

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

accommodate

> +be accomodated by the Generic PWM Device API framework.

accommodated

> +bus_id -- the plaintext name of the device.  Users will bind to a

plain text

> +synchronize, unsynchronize -- (optional) Callbacks to
> +synchronize/unsynchronize channels.

perhaps use desynchronize instead ?

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

whoops again

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

sizeof(*p)

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

dev_info(pwm->dev, ....)

> +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

> +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, ....);

> +void pwm_free(struct pwm_channel *p)
> +{
> +       pr_debug("%s: %s:%d free\n",
> +                __func__, p->pwm->bus_id, p->chan);

dev_dbg(p->dev, ....);

> +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 ?

> +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

> +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

> +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.

> +       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, ....);

> +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

> +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, ....);

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

spurious new line ?

> --- 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

> +       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 ?

> +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 ...
-mike