Re: [PWM v5 3/3] PWM: Atmel PWMC driver

[Mike Frysinger <vapier.adi@xxxxxxxxx>]

On Sat, Feb 19, 2011 at 23:10, Bill Gatliff wrote:
> +static inline void pwmc_writel(const struct atmel_pwmc *p, unsigned offset, u32 val)
> +static inline u32 pwmc_readl(const struct atmel_pwmc *p, unsigned offset)
> +static inline void pwmc_chan_writel(const struct pwm_device *p,
> +                                   u32 offset, u32 val)
> +static inline u32 pwmc_chan_readl(const struct pwm_device *p, u32 offset)
> +static inline int __atmel_pwmc_is_on(struct pwm_device *p)
> +static inline void __atmel_pwmc_stop(struct pwm_device *p)
> +static inline void __atmel_pwmc_start(struct pwm_device *p)
> +static inline int __atmel_pwmc_config_polarity(struct pwm_device *p,
> +                                             struct pwm_config *c)
> +static inline int __atmel_pwmc_config_duty_ticks(struct pwm_device *p,
> +                                               struct pwm_config *c)
> +static inline int __atmel_pwmc_config_period_ticks(struct pwm_device *p,
> +                                                 struct pwm_config *c)

seems like a lot of unnecessary inlines.  while the first two might
make sense since they're really just redirecting to the read/write i/o
api, the rest are quite a bit bigger.

> +static inline int __atmel_pwmc_config_polarity(struct pwm_device *p,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct pwm_config *c)
> +static inline int __atmel_pwmc_config_duty_ticks(struct pwm_device *p,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct pwm_config *c)
> +static inline int __atmel_pwmc_config_period_ticks(struct pwm_device *p,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct pwm_config *c)

these funcs always return 0 and the callers never check the return
value, so i guess these should return void instead

> +static int atmel_pwmc_stop_sync(struct pwm_device *p)
> +{
> + Â Â Â struct atmel_pwmc *ap = pwm_get_drvdata(p);
> + Â Â Â int was_on = __atmel_pwmc_is_on(p);
> + Â Â Â int chan = p - &ap->p[0];
> + Â Â Â int ret;
> +
> + Â Â Â if (was_on) {
> + Â Â Â Â Â Â Â do {
> + Â Â Â Â Â Â Â Â Â Â Â init_completion(&ap->complete);
> + Â Â Â Â Â Â Â Â Â Â Â set_bit(FLAG_STOP, &p->flags);
> + Â Â Â Â Â Â Â Â Â Â Â pwmc_writel(ap, PWMC_IER, BIT(chan));
> +
> + Â Â Â Â Â Â Â Â Â Â Â dev_dbg(p->dev, "waiting on stop_sync completion...\n");
> +
> + Â Â Â Â Â Â Â Â Â Â Â ret = wait_for_completion_interruptible(&ap->complete);
> +
> + Â Â Â Â Â Â Â Â Â Â Â dev_dbg(p->dev, "stop_sync complete (%d)\n", ret);
> +
> + Â Â Â Â Â Â Â Â Â Â Â if (ret)
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â return ret;
> + Â Â Â Â Â Â Â } while (test_bit(FLAG_STOP, &p->flags));
> + Â Â Â }
> +
> + Â Â Â return was_on;
> +}

if you changed this to return immediately when !was_on, then you
wouldnt need to indent the entire block.

> + Â Â Â ap->iobase = ioremap_nocache(r->start, r->end - r->start + 1);

isnt there a resource_size helper ?

> + Â Â Â if (IS_ERR_OR_NULL(ap->iobase)) {
> + Â Â Â Â Â Â Â ret = -ENODEV;
> + Â Â Â Â Â Â Â goto err_ioremap;
> + Â Â Â }

i dont think any of the io funcs return PTR_ERR.  they all return NULL
or a valid address.

> + Â Â Â for (chan = 0; chan < NCHAN; chan++) {
> + Â Â Â Â Â Â Â ap->p[chan].ops = &ap->ops;
> + Â Â Â Â Â Â Â pwm_set_drvdata(&ap->p[chan], ap);
> + Â Â Â Â Â Â Â ret = pwm_register(&ap->p[chan], &pdev->dev, chan);
> + Â Â Â Â Â Â Â if (ret)
> + Â Â Â Â Â Â Â Â Â Â Â goto err_pwm_register;
> + Â Â Â }
> +
> +err_pwm_register:
> + Â Â Â for (chan = 0; chan < chan; chan++) {
> + Â Â Â Â Â Â Â if (pwm_is_registered(&ap->p[chan]))
> + Â Â Â Â Â Â Â Â Â Â Â pwm_unregister(&ap->p[chan]);
> + Â Â Â }

if you wanted to be tricky, you could just have the unwind not change
the value of "chan".
while (--chan > 0)
    pwm_unregister(&ap->p[chan]);

otherwise, the "chan < chan" test makes no sense in the for loop.

> +static int __devexit atmel_pwmc_remove(struct platform_device *pdev)
> +{
> + Â Â Â struct atmel_pwmc *ap = platform_get_drvdata(pdev);
> + Â Â Â int chan;
> +
> + Â Â Â for (chan = 0; chan < NCHAN; chan++)
> + Â Â Â Â Â Â Â if (pwm_is_registered(&ap->p[chan]))
> + Â Â Â Â Â Â Â Â Â Â Â pwm_unregister(&ap->p[chan]);

why do you test if it's registered ?  the probe function will abort if
any do not register properly.
-mike
--
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