Re: [[RFC] 2/5] Emulates PWM hardware using a high-resolution timer and a GPIO pin

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

On Mon, Oct 19, 2009 at 16:32, Bill Gatliff wrote:
> --- /dev/null
> +++ b/drivers/pwm/gpio.c
> @@ -0,0 +1,318 @@
> +#define DEBUG 99

whoops

> +       pr_debug("%s:%d start, %lu ticks\n",
> +                dev_name(p->pwm->dev), p->chan, p->duty_ticks);

you already have a struct device, so this is just odd.  use dev_dbg()
instead and let it worry about dev_name() stuff.  plus you're already
using dev_dbg() in the rest of the code, so i guess you just forgot
about this.

> +       case PWM_CONFIG_START:
> +               if (!hrtimer_active(&gp->t)) {
> +                       gpio_pwm_start(p);
> +               }

dont really need those braces

> +       struct gpio_pwm *gp = container_of(p->pwm, struct gpio_pwm, pwm);

helper macro for this ?

> +static int
> +gpio_pwm_config(struct pwm_channel *p,
> +               struct pwm_channel_config *c)
> +{
> +       struct gpio_pwm *gp = container_of(p->pwm, struct gpio_pwm, pwm);
> +       int was_on = 0;
> +
> +       if (p->pwm->config_nosleep) {
> +               if (!p->pwm->config_nosleep(p, c))
> +                       return 0;
> +       }
> +
> +       might_sleep();

shouldnt this be above the config_n

> +static int __init
> +gpio_pwm_probe(struct platform_device *pdev)

__devinit

> +static struct platform_driver gpio_pwm_driver = {
> +       .driver = {
> +               .name = "gpio_pwm",
> +               .owner = THIS_MODULE,
> +       },

dont need the explicit .owner ?

> +static void gpio_pwm_exit(void)

__exit

> +MODULE_DESCRIPTION("PWM output using GPIO and an itimer");

itimer -> hrtimer ?
-mike