Re: [PWM v5 1/3] PWM: Implement a generic PWM framework

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


On Sat, Feb 19, 2011 at 23:10, Bill Gatliff wrote:
> +void pwm_release(struct pwm_device *p)
> +{
> + Â Â Â mutex_lock(&device_list_mutex);
> +
> + Â Â Â if (!test_and_clear_bit(FLAG_REQUESTED, &p->flags)) {
> + Â Â Â Â Â Â Â BUG();
> + Â Â Â Â Â Â Â goto done;
> + Â Â Â }

shouldnt that BUG be a WARN ?

> +int pwm_set(struct pwm_device *p, unsigned long period_ns,
> + Â Â Â Â Â unsigned long duty_ns, int active_high)
> +{
> + Â Â Â struct pwm_config c = {
> + Â Â Â Â Â Â Â .config_mask = (BIT(PWM_CONFIG_PERIOD_TICKS)
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â | BIT(PWM_CONFIG_DUTY_TICKS)
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â | BIT(PWM_CONFIG_POLARITY)),
> + Â Â Â Â Â Â Â .period_ticks = pwm_ns_to_ticks(p, period_ns),
> + Â Â Â Â Â Â Â .duty_ticks = pwm_ns_to_ticks(p, duty_ns),
> + Â Â Â Â Â Â Â .polarity = active_high};

i think that brace needs to be uncuddled

> +static const struct attribute *pwm_attrs[] =
> +{
> + Â Â Â &dev_attr_tick_hz.attr,

cuddle up that brace

> +static const struct attribute_group pwm_device_attr_group = {
> + Â Â Â .attrs = (struct attribute **) pwm_attrs,
> +};

should attribute_group have its attrs member constified ?

> +static void __exit pwm_exit(void)
> +{
> + Â Â Â destroy_workqueue(pwm_handler_workqueue);
> + Â Â Â class_unregister(&pwm_class);
> +}
> +
> +#ifdef MODULE
> +module_init(pwm_init);
> +module_exit(pwm_exit);
> +MODULE_LICENSE("GPL");
> +#else
> +postcore_initcall(pwm_init);
> +#endif

i dont think you need this MODULE trickery.  common code already takes
care of this for you, and it'd let you avoid a warning about pwm_exit
being unused.
postcore_initcall(pwm_init);
module_exit(pwm_exit);
MODULE_LICENSE("GPL");

also, no MODULE_{INFO,AUTHOR} ?

> +struct pwm_device {
> + Â Â Â struct pwm_device_ops *ops;

const ?

> +void pwm_callback(struct pwm_device *pwm);
> +int gpio_pwm_destroy(struct pwm_device *p);

seems a bit inconsistent ... sometimes you use "p", sometimes you use "pwm" ...
-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


[Gstreamer Embedded]     [Linux MMC Devel]     [U-Boot V2]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]    [Yosemite Photos]    [Free Online Dating]     [Linux Kernel]     [Linux OMAP]     [Linux SCSI]     [XFree86]

Add to Google Powered by Linux