Re: [PWM v4 2/3] PWM: GPIO+hrtimer device emulation

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

On Sat, Feb 12, 2011 at 21:15, Bill Gatliff wrote:
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> +config GPIO_PWM
> +       tristate "GPIO+hrtimer PWM device emulation"
> +       depends on GENERIC_PWM
> +       help
> +         This option enables code that emulates a single-channel
> +         PWM device using a high-resolution timer and a GPIO
> +         pin.  The PWM framework allows you to create as many
> +         of these devices as desired, subject to CPU overhead
> +         and GPIO pin availability.  If unsure, say N.

document the module name in the help ?

> --- /dev/null
> +++ b/drivers/pwm/gpio-pwm.c
> +#define DRIVER_NAME "gpio-pwm"

use KBUILD_MODNAME instead ?

> +static void gpio_pwm_work (struct work_struct *work)

no space before the "("

> +static struct pwm_device_ops gpio_pwm_device_ops = {
> +       .config         = gpio_pwm_config,
> +       .config_nosleep = gpio_pwm_config_nosleep,
> +       .request        = gpio_pwm_request,
> +};

is this struct not constified ?  same for some of the other structs in
this file ...

> +       gp = kzalloc(sizeof(*gp), GFP_KERNEL);
> +       if (IS_ERR_OR_NULL(gp))

i dont think the kmalloc funcs return a pointer error.  it's either
NULL or it's a valid pointer.  this comes up a few times ...

> +EXPORT_SYMBOL(gpio_pwm_create);
> +EXPORT_SYMBOL(gpio_pwm_destroy);

are people expected to call these directly ?  is that useful if they
cant call any of the config funcs ?
-mike