lkml.org 
[lkml]   [2011]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PWM v6 1/3] PWM: Implement a generic PWM framework
On 06/03/2011 04:17, Bill Gatliff wrote:
> +The Generic PWM Device API framework is implemented in
> +include/linux/pwm/pwm.h and drivers/pwm/pwm.c. The functions therein
> +use information from pwm_device and pwm__config structures to invoke

Could you fix that to be pwm_config please.

[snip]

> --- /dev/null
> +++ b/drivers/pwm/Kconfig
> @@ -0,0 +1,10 @@
> +#
> +# PWM infrastructure and devices
> +#
> +
> +menuconfig GENERIC_PWM
> + tristate "PWM Support"
> + help
> + Enables PWM device support implemented via a generic
> + framework. If unsure, say N.
> +

Does this need help text? Can't we just select GENERIC_PWM for the
drivers / users that need it?

> diff --git a/drivers/pwm/pwm.c b/drivers/pwm/pwm.c

[snip]

> +static int __pwm_request(struct pwm_device *p, const char *label)
> +{
> + int ret;
> +
> + ret = test_and_set_bit(FLAG_REQUESTED, &p->flags);
> + if (ret) {
> + ret = -EBUSY;
> + goto done;
> + }
> +
> + p->label = label;
> +
> + if (p->ops->request) {
> + ret = p->ops->request(p);
> + if (ret) {
> + clear_bit(FLAG_REQUESTED, &p->flags);
> + goto done;

You don't need this goto here.

> + }
> + }
> +
> +done:
> + return ret;
> +}
> +

[snip]

> +struct pwm_device *pwm_request(const char *bus_id, int id, const char *label)
> +{
> + char name[256];
> + int ret;
> +
> + if (id == -1)
> + ret = scnprintf(name, sizeof name, "%s", bus_id);

Kernel doc comment for the function to describe the id == -1 case?

> + else
> + ret = scnprintf(name, sizeof name, "%s:%d", bus_id, id);
> + if (ret <= 0 || ret >= sizeof name)
> + return ERR_PTR(-EINVAL);
> +
> + return pwm_request_byname(name, label);
> +}
> +EXPORT_SYMBOL(pwm_request);

[snip]

> +int pwm_config(struct pwm_device *p, struct pwm_config *c)
> +{
> + int ret = 0;

might_sleep() ?

[snip]

> +static void pwm_handler(struct work_struct *w)
> +{
> + struct pwm_device *p = container_of(w, struct pwm_device,
> + handler_work);

You could just not queue the work if !p->handler

> + if (p->handler)
> + p->handler(p, p->handler_data);
> +}
> +
> +void pwm_callback(struct pwm_device *p)
> +{
> + queue_work(pwm_handler_workqueue, &p->handler_work);
> +}
> +EXPORT_SYMBOL(pwm_callback);
> +

[snip]

> +int pwm_register(struct pwm_device *p, struct device *parent, int id)
> +{
> + int ret;
> + char name[256];
> +
> + if (IS_ERR_OR_NULL(parent))
> + return -EINVAL;
> +
> + if (id == -1)
> + ret = scnprintf(name, sizeof name, "%s", dev_name(parent));

A kernel doc comment to explain id == -1 would be nice.

> + else
> + ret = scnprintf(name, sizeof name, "%s:%d", dev_name(parent), id);
> + if (ret <= 0 || ret >= sizeof name)
> + return -EINVAL;
> +
> + return pwm_register_byname(p, parent, name);
> +}
> +EXPORT_SYMBOL(pwm_register);
> +

Very nice framework!

Would be nice to have kernel doc comments for the functions but that can
follow on later.

Thanks,

Jack


\
 
 \ /
  Last update: 2011-03-07 05:23    [W:0.139 / U:2.348 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site