On Mon, Oct 19, 2009 at 16:32, Bill Gatliff wrote:
> +A generic PWM device framework must accomodate the substantial
accommodate
> +be accomodated by the Generic PWM Device API framework.
accommodated
> +bus_id -- the plaintext name of the device. Users will bind to a
plain text
> +synchronize, unsynchronize -- (optional) Callbacks to
> +synchronize/unsynchronize channels.
perhaps use desynchronize instead ?
> --- /dev/null
> +++ b/drivers/pwm/pwm.c
> @@ -0,0 +1,692 @@
> +#define DEBUG 99
whoops again
> +int pwm_register(struct pwm_device *pwm)
> +{
> + p = kcalloc(pwm->nchan, sizeof(struct pwm_channel), GFP_KERNEL);
sizeof(*p)
> + pr_info("%s: %d channel%s\n", pwm->bus_id, pwm->nchan,
> + pwm->nchan > 1 ? "s" : "");
dev_info(pwm->dev, ....)
> +static struct pwm_device *
> +__pwm_find_device(const char *bus_id)
> +{
> + struct pwm_device *p;
> +
> + list_for_each_entry(p, &pwm_device_list, list)
> + {
cuddle that brace
> +struct pwm_channel *
> +pwm_request(const char *bus_id,
> + int chan,
> + const char *requester)
> +{
> + struct pwm_device *p;
> +
> + pr_debug("%s: %s:%d returns %p\n", __func__,
> + bus_id, chan, &p->channels[chan]);
> + pr_debug("%s: %s:%d returns NULL\n",
> + __func__, bus_id, chan);
dev_dbg(p->dev, ....);
> +void pwm_free(struct pwm_channel *p)
> +{
> + pr_debug("%s: %s:%d free\n",
> + __func__, p->pwm->bus_id, p->chan);
dev_dbg(p->dev, ....);
> +unsigned long pwm_ns_to_ticks(struct pwm_channel *p,
> + unsigned long nsecs)
> +{
> + unsigned long long ticks;
> +
> + ticks = nsecs;
> + ticks *= p->tick_hz;
> + do_div(ticks, 1000000000);
> + return ticks;
> +}
> +EXPORT_SYMBOL(pwm_ns_to_ticks);
perhaps better as a static inline in the header ?
> +unsigned long pwm_ticks_to_ns(struct pwm_channel *p,
> + unsigned long ticks)
> +{
> + unsigned long long ns;
> +
> + if (!p->tick_hz)
> + return 0;
> +
> + ns = ticks;
> + ns *= 1000000000UL;
> + do_div(ns, p->tick_hz);
> + return ns;
> +}
> +EXPORT_SYMBOL(pwm_ticks_to_ns);
this one too
> +static void
> +pwm_config_percent_to_ticks(struct pwm_channel *p,
> + struct pwm_channel_config *c)
> +{
> + if (c->config_mask & PWM_CONFIG_DUTY_PERCENT) {
> + if (c->config_mask & PWM_CONFIG_PERIOD_TICKS)
> + c->duty_ticks = c->period_ticks;
> + else
> + c->duty_ticks = p->period_ticks;
> +
> + c->duty_ticks *= c->duty_percent;
> + c->duty_ticks /= 100;
> + c->config_mask &= ~PWM_CONFIG_DUTY_PERCENT;
> + c->config_mask |= PWM_CONFIG_DUTY_TICKS;
> + }
> +}
if you returned when the mask doesnt match, then you wouldnt need to
indent the rest of the func
> +int pwm_config(struct pwm_channel *p,
> + struct pwm_channel_config *c)
> +{
> + int ret = 0;
> +
> + if (unlikely(!p->pwm->config)) {
> + pr_debug("%s: %s:%d has no config handler (-EINVAL)\n",
> + __func__, p->pwm->bus_id, p->chan);
dev_dbg(p->dev, ....);
> + switch (c->config_mask & (PWM_CONFIG_PERIOD_TICKS
> + | PWM_CONFIG_DUTY_TICKS)) {
> + case PWM_CONFIG_PERIOD_TICKS:
> + if (p->duty_ticks > c->period_ticks) {
> + ret = -EINVAL;
> + goto err;
> + }
> + break;
> + case PWM_CONFIG_DUTY_TICKS:
> + if (p->period_ticks < c->duty_ticks) {
> + ret = -EINVAL;
> + goto err;
> + }
> + break;
> + case PWM_CONFIG_DUTY_TICKS | PWM_CONFIG_PERIOD_TICKS:
> + if (c->duty_ticks > c->period_ticks) {
> + ret = -EINVAL;
> + goto err;
> + }
> + break;
unless i missed something, the first and third case is the same. so
probably better (compiled code wise) to write an if statement.
> + pr_debug("%s: config_mask %d period_ticks %lu duty_ticks %lu"
> + " polarity %d duty_ns %lu period_ns %lu duty_percent %d\n",
> + __func__, c->config_mask, c->period_ticks, c->duty_ticks,
> + c->polarity, c->duty_ns, c->period_ns, c->duty_percent);
dev_dbg(p->dev, ....);
> +int pwm_set_period_ns(struct pwm_channel *p,
> + unsigned long period_ns)
> +unsigned long pwm_get_period_ns(struct pwm_channel *p)
> +int pwm_set_duty_ns(struct pwm_channel *p,
> + unsigned long duty_ns)
> +unsigned long pwm_get_duty_ns(struct pwm_channel *p)
> +int pwm_set_duty_percent(struct pwm_channel *p,
> + int percent)
> +int pwm_set_polarity(struct pwm_channel *p,
> + int active_high)
> +int pwm_start(struct pwm_channel *p)
> +int pwm_stop(struct pwm_channel *p)
these all look like they should static inlines
> +static void __pwm_callback(struct pwm_channel *p)
> +{
> + queue_work(pwm_handler_workqueue, &p->handler_work);
> + pr_debug("%s:%d handler %p scheduled with data %p\n",
> + p->pwm->bus_id, p->chan, p->handler, p->handler_data);
dev_dbg(p->dev, ....);
> +static struct class pwm_class = {
> + .name = "pwm",
> + .owner = THIS_MODULE,
> +
> + .class_attrs = pwm_class_attrs,
> +};
spurious new line ?
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -1,31 +1,170 @@
> #ifndef __LINUX_PWM_H
> #define __LINUX_PWM_H
>
> -struct pwm_device;
> -
> /*
> - * pwm_request - request a PWM device
> + * include/linux/pwm.h
header #ifdef should really be after the comment blob
> + PWM_CONFIG_DUTY_TICKS = BIT(0),
> + PWM_CONFIG_PERIOD_TICKS = BIT(1),
> + PWM_CONFIG_POLARITY = BIT(2),
> + PWM_CONFIG_START = BIT(3),
> + PWM_CONFIG_STOP = BIT(4),
>
> + PWM_CONFIG_HANDLER = BIT(5),
> +
> + PWM_CONFIG_DUTY_NS = BIT(6),
spurious new lines ?
> +enum {
> + FLAG_REQUESTED = 0,
> + FLAG_STOP = 1,
> +};
this is weird ... you're doing things like:
if (pwm->channels[wchan].flags & FLAG_REQUESTED) {
if (test_and_set_bit(FLAG_REQUESTED, &p->flags))
but FLAG_REQUESTED is 0 ...
-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
