Re: [[RFC] 4/5] An LED "dimmer" trigger, which uses the PWM API to vary the brightness of an LED according to system load

2009-10-19 Thread Mike Frysinger
On Mon, Oct 19, 2009 at 21:42, Bill Gatliff wrote:
> Mike Frysinger wrote:
>> On Mon, Oct 19, 2009 at 16:32, Bill Gatliff wrote:
>>>  #include 
>>> +#include 
>>
>> if there's going to be a bunch of new pwm related headers, perhaps a
>> subdir makes more sense: linux/pwm/xxx
>
> In general I don't have a problem with this.  However, in this specific case
> would it make more sense to just fold pwm-led.h into pwm.h?  There really
> isn't much to pwm-led.h.

since pwm-led.h is meant for platform data and for boards to include
it, keeping it separate makes sense to me.  and once we start getting
more pwm based drivers that want to do headers of their own, having
linux/pwm/ for them would be nice.

as for the core pwm header, either linux/pwm.h or linux/pwm/pwm.h
would work i think.

>>> +static void
>>> +led_pwm_brightness_set(struct led_classdev *c,
>>> +                      enum led_brightness b)
>>> +{
>>> +       struct led_pwm *led;
>>> +       int percent;
>>> +
>>> +       percent = (b * 100) / (LED_FULL - LED_OFF);
>>> +       led = container_of(c, struct led_pwm, led);
>>
>> instead of using container_of() everywhere, why not add a helper macro
>> that turns a led_classev into a led_pwm
>
> That's just a personal style thing.  As soon as I write a helper macro, I
> tend to forget how it works and then start mis-using it everywhere.  But I
> don't have a problem with doing a helper.
>
> For better type-safety, would a helper inline-function be a better choice
> than a helper macro?  Or would it make any difference?

an inline function might be better.  it would also allow you to pass
in void* pointers though ...

>>> +       if (!try_module_get(d->driver->owner))
>>> +               return -ENODEV;
>>
>> is this really needed ?
>
> Not sure.

after looking at the other patches, i dont think it is.  the core pwm
framework does the module_get for you when you register the driver.
-mike
--
To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [[RFC] 3/5] Expunge old Atmel PWMC driver, replacing it with one that conforms to the PWM API

2009-10-19 Thread Bill Gatliff

Mike Frysinger wrote:

+   ap->clk = clk_get(&pdev->dev, "pwm_clk");
+   if (IS_ERR(ap->clk)) {
+   pr_info("%s: clk_get error %ld\n",
+   ap->pwm.bus_id, PTR_ERR(ap->clk));
+   ret = -ENODEV;
+   goto err_clk_get;



shouldnt it be:
  ret = PTR_ERR(ap->clk);
  


Probably, because it's preferable to return the actual error code when 
it's known, rather than translating all error codes to -ENODEV.  Good catch.



b.g.

--
Bill Gatliff
b...@billgatliff.com

--
To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [[RFC] 1/5] API to consolidate PWM devices behind a common user and kernel interface

2009-10-19 Thread Bill Gatliff

Mike Frysinger wrote:

On Mon, Oct 19, 2009 at 16:32, Bill Gatliff wrote:
  

+A generic PWM device framework must accomodate the substantial



accommodate
  


Heh, and to think I sometimes get paid to write!  :)

Similar and redundant feedback [snipped]



+synchronize, unsynchronize -- (optional) Callbacks to
+synchronize/unsynchronize channels.



perhaps use desynchronize instead ?
  


I like that idea.

  

--- /dev/null
+++ b/drivers/pwm/pwm.c
@@ -0,0 +1,692 @@
+#define DEBUG 99



whoops again
  


Grr.  ADHD guys shouldn't code without their caffeine!  :)



+int pwm_register(struct pwm_device *pwm)
+{
+   p = kcalloc(pwm->nchan, sizeof(struct pwm_channel), GFP_KERNEL);



sizeof(*p)
  


Good catch.



+   pr_info("%s: %d channel%s\n", pwm->bus_id, pwm->nchan,
+   pwm->nchan > 1 ? "s" : "");



dev_info(pwm->dev, )
  


Yep.  I wonder what I was thinking at the time?  :)


+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
  


Yep.


+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, );
  


Yep again, and again...

  

+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, 10);
+   return ticks;
+}
+EXPORT_SYMBOL(pwm_ns_to_ticks);



perhaps better as a static inline in the header ?
  


I guess that wouldn't negatively affect modules using the function, so I 
think I agree.




+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 *= 10UL;
+   do_div(ns, p->tick_hz);
+   return ns;
+}
+EXPORT_SYMBOL(pwm_ticks_to_ns);



this one too
  


Yep.


+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
  


That's a habit I learned in my early days when I had an emulator that 
really didn't like multiple exit points from functions.  And it's been a 
hard habit to break!



+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.
  


Yea.  Can you tell that code has been "refactored"?  :)


+   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, );
  


Yep.


+int pwm_set_period_ns(struct pwm_channel *p,
+ unsigned long period_ns)
+unsigned long pwm_get_period_ns(struct pwm_channel 

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

2009-10-19 Thread Bill Gatliff

Mike Frysinger wrote:

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

  


Indeed!



+   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.
  


Yea, I think so.


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



dont really need those braces
  


Agreed.


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



helper macro for this ?
  


Ditto.


+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
  


Actually, not.  In this example I'm hoping that the requested action can 
be handled by the config_nosleep code, and if it can then I just 
return.  Otherwise, I go through the might_sleep and then into the code 
that might actually sleep.


But, I think the upper-level code might already do this same thing, so 
the above might be entirely redundant.  I'll have to check on that.




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



__devinit
  


Yep.


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



dont need the explicit .owner ?
  


Again, not sure.


  

+static void gpio_pwm_exit(void)



__exit
  


I think so, yes.



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



itimer -> hrtimer ?
  


Yep.


b.g.

--
Bill Gatliff
b...@billgatliff.com

--
To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [[RFC] 4/5] An LED "dimmer" trigger, which uses the PWM API to vary the brightness of an LED according to system load

2009-10-19 Thread Bill Gatliff

Mike Frysinger wrote:

On Mon, Oct 19, 2009 at 16:32, Bill Gatliff wrote:
  

--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -1,153 +1,167 @@
-/*
- * linux/drivers/leds-pwm.c
- *
- * simple PWM based LED control
- *
- * Copyright 2009 Luotao Fu @ Pengutronix (l...@pengutronix.de)
- *
- * based on leds-gpio.c by Raphael Assenat 
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */



this should not be removed.  if you wanted to add your copyright line,
then that's fine, but the rest needs to stay.
  


For the record, the reason the file looks like it does is because I 
wrote an original one that replaced the previous leds-pwm.c--- but 
obviously git didn't see it that way when it produced the diff.


I certainly wasn't trying to write-out anyone's copyright!  I don't have 
a problem with their names appearing in the file, regardless, so I'll 
put them back in.  If they have problems with their names appearing 
therein, they can let me know.  :)



 #include 
+#include 



if there's going to be a bunch of new pwm related headers, perhaps a
subdir makes more sense: linux/pwm/xxx
  


In general I don't have a problem with this.  However, in this specific 
case would it make more sense to just fold pwm-led.h into pwm.h?  There 
really isn't much to pwm-led.h.



+static void
+led_pwm_brightness_set(struct led_classdev *c,
+  enum led_brightness b)
+{
+   struct led_pwm *led;
+   int percent;
+
+   percent = (b * 100) / (LED_FULL - LED_OFF);
+   led = container_of(c, struct led_pwm, led);



instead of using container_of() everywhere, why not add a helper macro
that turns a led_classev into a led_pwm
  


That's just a personal style thing.  As soon as I write a helper macro, 
I tend to forget how it works and then start mis-using it everywhere.  
But I don't have a problem with doing a helper.


For better type-safety, would a helper inline-function be a better 
choice than a helper macro?  Or would it make any difference?



+static int __init
+led_pwm_probe(struct platform_device *pdev)



probe functions are typical __devinit
  


Yep, my bad.


+   if (!try_module_get(d->driver->owner))
+   return -ENODEV;



is this really needed ?
  


Not sure.


-static int __devexit led_pwm_remove(struct platform_device *pdev)
+static int
+led_pwm_remove(struct platform_device *pdev)



looks like you lost the __devexit markings
  


Yep.


 static struct platform_driver led_pwm_driver = {
+   .driver = {
+   .name = "leds-pwm",
+   .owner =THIS_MODULE,
   },



i dont think platform_drivers need to explicitly set their owner.  i
thought that was all handled for you.
  


There are examples for both cases.  I don't see anything assigning to 
.owner in drivers/base/platform.c, though.



--- /dev/null
+++ b/include/linux/pwm-led.h
@@ -0,0 +1,34 @@
+#ifndef __LINUX_PWM_LED_H
+#define __LINUX_PWM_LED_H
+
+/*
+ * include/linux/pwm-led.h



the ifdef protection typically goes after the header comment blob, not before
  


Yep, my bad.


Thanks for the feedback!


b.g.

--
Bill Gatliff
b...@billgatliff.com

--
To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [[RFC] 3/5] Expunge old Atmel PWMC driver, replacing it with one that conforms to the PWM API

2009-10-19 Thread Mike Frysinger
On Mon, Oct 19, 2009 at 16:32, Bill Gatliff wrote:
> --- /dev/null
> +++ b/drivers/pwm/atmel-pwm.c
> +       const struct atmel_pwm *ap
> +               = container_of(p->pwm, struct atmel_pwm, pwm);

make a helper ?

> +       pr_debug("%s:%d sync_mask %x\n",
> +                p->pwm->bus_id, p->chan, ap->sync_mask[p->chan]);

all of the pr_* funcs in this driver should be dev_* funcs

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

__devinit

> +       ap->clk = clk_get(&pdev->dev, "pwm_clk");
> +       if (IS_ERR(ap->clk)) {
> +               pr_info("%s: clk_get error %ld\n",
> +                       ap->pwm.bus_id, PTR_ERR(ap->clk));
> +               ret = -ENODEV;
> +               goto err_clk_get;

shouldnt it be:
  ret = PTR_ERR(ap->clk);

> +static struct platform_driver atmel_pwm_driver = {
> +       .driver = {
> +               .name = "atmel_pwmc",
> +               .owner = THIS_MODULE,
> +       },

no need for .owner ?

> +static void atmel_pwm_exit(void)

__exit
-mike
--
To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [[RFC] 5/5] Incorporate PWM API code into KBuild

2009-10-19 Thread Mike Frysinger
On Mon, Oct 19, 2009 at 16:32, Bill Gatliff wrote:
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -6,6 +6,8 @@
>  #
>
>  obj-y                          += gpio/
> +obj-$(CONFIG_GENERIC_PWM)      += pwm/
> +
>  obj-$(CONFIG_PCI)              += pci/

spurious new line

> --- /dev/null
> +++ b/drivers/pwm/Kconfig
> +         software controlled power-efficent backlights on LCD

efficient

> --- /dev/null
> +++ b/drivers/pwm/Makefile
> @@ -0,0 +1,7 @@
> +#
> +# Makefile for pwm devices
> +#
> +obj-y := pwm.o
> +
> +obj-$(CONFIG_ATMEL_PWM)                += atmel-pwm.o
> +obj-$(CONFIG_GPIO_PWM)         += gpio.o
> \ No newline at end of file

there should be a newline at the end of the file
-mike
--
To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [[RFC] 1/5] API to consolidate PWM devices behind a common user and kernel interface

2009-10-19 Thread Mike Frysinger
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, 10);
> +       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 *= 10UL;
> +       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

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

2009-10-19 Thread Mike Frysinger
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
--
To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [[RFC] 4/5] An LED "dimmer" trigger, which uses the PWM API to vary the brightness of an LED according to system load

2009-10-19 Thread Mike Frysinger
On Mon, Oct 19, 2009 at 16:32, Bill Gatliff wrote:
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -1,153 +1,167 @@
> -/*
> - * linux/drivers/leds-pwm.c
> - *
> - * simple PWM based LED control
> - *
> - * Copyright 2009 Luotao Fu @ Pengutronix (l...@pengutronix.de)
> - *
> - * based on leds-gpio.c by Raphael Assenat 
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - */

this should not be removed.  if you wanted to add your copyright line,
then that's fine, but the rest needs to stay.

>  #include 
> +#include 

if there's going to be a bunch of new pwm related headers, perhaps a
subdir makes more sense: linux/pwm/xxx

> +static void
> +led_pwm_brightness_set(struct led_classdev *c,
> +                      enum led_brightness b)
> +{
> +       struct led_pwm *led;
> +       int percent;
> +
> +       percent = (b * 100) / (LED_FULL - LED_OFF);
> +       led = container_of(c, struct led_pwm, led);

instead of using container_of() everywhere, why not add a helper macro
that turns a led_classev into a led_pwm

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

probe functions are typical __devinit

> +       if (!try_module_get(d->driver->owner))
> +               return -ENODEV;

is this really needed ?

> -static int __devexit led_pwm_remove(struct platform_device *pdev)
> +static int
> +led_pwm_remove(struct platform_device *pdev)

looks like you lost the __devexit markings

>  static struct platform_driver led_pwm_driver = {
> +       .driver = {
> +               .name =         "leds-pwm",
> +               .owner =        THIS_MODULE,
>        },

i dont think platform_drivers need to explicitly set their owner.  i
thought that was all handled for you.

> --- /dev/null
> +++ b/include/linux/pwm-led.h
> @@ -0,0 +1,34 @@
> +#ifndef __LINUX_PWM_LED_H
> +#define __LINUX_PWM_LED_H
> +
> +/*
> + * include/linux/pwm-led.h

the ifdef protection typically goes after the header comment blob, not before
-mike
--
To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: How to create a git repo on git.kernel.org?

2009-10-19 Thread Bill Gatliff

Bill Gatliff wrote:

Guys:


How does one go about creating a git tree on git.kernel.org?  I'd like 
to create one there as a public repository for the PWM API stuff, and 
for a couple of boards that I'd like to get into mainline.


Thanks!

b.g.



Why is it that you don't find the relevant line in the FAQ until ten 
seconds after clicking ?  :)


Disregard. 



b.g.

--
Bill Gatliff
b...@billgatliff.com

--
To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: How to create a git repo on git.kernel.org?

2009-10-19 Thread Sam Ravnborg
On Mon, Oct 19, 2009 at 03:40:44PM -0500, Bill Gatliff wrote:
> Guys:
>
>
> How does one go about creating a git tree on git.kernel.org?  I'd like  
> to create one there as a public repository for the PWM API stuff, and  
> for a couple of boards that I'd like to get into mainline.

Step one is to get an account on kernel.org.
Try to send an email to ftpad...@kernel.org and explain why you
think you need an account / git tree there.

You will have to PGP sign your mail and having someone to recommend
you will help.

Sam
--
To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


How to create a git repo on git.kernel.org?

2009-10-19 Thread Bill Gatliff

Guys:


How does one go about creating a git tree on git.kernel.org?  I'd like 
to create one there as a public repository for the PWM API stuff, and 
for a couple of boards that I'd like to get into mainline.


Thanks!

b.g.

--
Bill Gatliff
b...@billgatliff.com

--
To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[[RFC] 0/5] Generic PWM API Proposal

2009-10-19 Thread Bill Gatliff

This patch series extends the existing PWM API into something more
generic, and adds support for hotplugging.  A driver for the Atmel
SAM9263 PWMC peripheral is provided, as well as a "leds-pwm" wedge
and an "led-dim" trigger that allow the LED API to take advantage
of this new API.  The code has been run-tested on a SAM9263 platform,
the OMAP3430 Beagleboard platform, and a few others.

The motivation for creating this API is the author's need for hotpluggable
PWM devices, including i2c GPIO expander chips.  The existing PWM API's
use of integers to enumerate PWM channels was problematic for generic
hotplug situations, so the new API creates pwm_device and pwm_channel
structures.  Under the new API, PWM channels are identified by the platform
device's bus_id plus an integer channel selector for that device.

The API does not deal with I/O line multiplexing.  It assumes that the
platform code or bootloader have set up the output lines as necessary.

The proposed API targets the most basic capabilities required by PWM signals,
namely the ability to vary the period and duty cycle of the waveform.  The
author believes that other, more-sophisticated features like channel
synchronization, frequency slewing, end-of-period interrupts, etc. that
are necessary for applications like motor control can be added without
substantially altering the proposed API.  Testing of these features
will in some cases require hardware that the author does not currently have
access to.  (Hint, hint).

The author wishes to express his appreciation to Russell King, David Brownell,
Ulf Samuelsson, Eric Maio, Haavard Skinnemoen, and others who helped him
formulate the API and reviewed early releases of the code.

Bill Gatliff (5):
  API to consolidate PWM devices behind a common user and kernel
interface
  Emulates PWM hardware using a high-resolution timer and a GPIO pin
  Expunge old Atmel PWMC driver, replacing it with one that conforms to
the PWM API
  An LED "dimmer" trigger, which uses the PWM API to vary the
brightness of an LED according to system load
  Incorporate PWM API code into KBuild

 Documentation/pwm.txt  |  258 
 drivers/Kconfig|2 +
 drivers/Makefile   |2 +
 drivers/leds/Kconfig   |   32 ++-
 drivers/leds/Makefile  |3 +
 drivers/leds/leds-pwm.c|  224 ---
 drivers/leds/ledtrig-dim.c |   95 ++
 drivers/misc/Makefile  |6 +-
 drivers/misc/atmel_pwm.c   |  409 --
 drivers/pwm/Kconfig|   30 ++
 drivers/pwm/Makefile   |7 +
 drivers/pwm/atmel-pwm.c|  633 
 drivers/pwm/gpio.c |  318 
 drivers/pwm/pwm.c  |  692 
 include/linux/pwm-led.h|   34 +++
 include/linux/pwm.h|  179 ++--
 16 files changed, 2384 insertions(+), 540 deletions(-)
 create mode 100644 Documentation/pwm.txt
 create mode 100644 drivers/leds/ledtrig-dim.c
 delete mode 100644 drivers/misc/atmel_pwm.c
 create mode 100644 drivers/pwm/Kconfig
 create mode 100644 drivers/pwm/Makefile
 create mode 100644 drivers/pwm/atmel-pwm.c
 create mode 100644 drivers/pwm/gpio.c
 create mode 100644 drivers/pwm/pwm.c
 create mode 100644 include/linux/pwm-led.h

--
To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[[RFC] 5/5] Incorporate PWM API code into KBuild

2009-10-19 Thread Bill Gatliff

Signed-off-by: Bill Gatliff 
---
 drivers/Kconfig  |2 ++
 drivers/Makefile |2 ++
 drivers/pwm/Kconfig  |   30 ++
 drivers/pwm/Makefile |7 +++
 4 files changed, 41 insertions(+), 0 deletions(-)
 create mode 100644 drivers/pwm/Kconfig
 create mode 100644 drivers/pwm/Makefile

diff --git a/drivers/Kconfig b/drivers/Kconfig
index 48bbdbe..ee45cf7 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -56,6 +56,8 @@ source "drivers/pps/Kconfig"
 
 source "drivers/gpio/Kconfig"
 
+source "drivers/pwm/Kconfig"
+
 source "drivers/w1/Kconfig"
 
 source "drivers/power/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index 6ee53c7..e6143f3 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -6,6 +6,8 @@
 #
 
 obj-y  += gpio/
+obj-$(CONFIG_GENERIC_PWM)  += pwm/
+
 obj-$(CONFIG_PCI)  += pci/
 obj-$(CONFIG_PARISC)   += parisc/
 obj-$(CONFIG_RAPIDIO)  += rapidio/
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
new file mode 100644
index 000..0ead279
--- /dev/null
+++ b/drivers/pwm/Kconfig
@@ -0,0 +1,30 @@
+#
+# PWM infrastructure and devices
+#
+
+menuconfig GENERIC_PWM
+   tristate "PWM Support"
+   help
+ This enables PWM support through the generic PWM library.
+ If unsure, say N.
+
+if GENERIC_PWM
+
+config ATMEL_PWM
+   tristate "Atmel AT32/AT91 PWM support"
+   depends on AVR32 || ARCH_AT91
+   help
+ This option enables device driver support for the PWMC
+ peripheral channels found on certain Atmel processors.
+ Pulse Width Modulation is used many for purposes, including
+ software controlled power-efficent backlights on LCD
+ displays, motor control, and waveform generation.  If
+ unsure, say N.
+
+config GPIO_PWM
+   tristate "PWM emulation using GPIO"
+   help
+ This option enables a single-channel PWM device using
+a kernel interval timer and a GPIO pin.  If unsure, say N.
+
+endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
new file mode 100644
index 000..e42246b
--- /dev/null
+++ b/drivers/pwm/Makefile
@@ -0,0 +1,7 @@
+#
+# Makefile for pwm devices
+#
+obj-y := pwm.o
+
+obj-$(CONFIG_ATMEL_PWM)+= atmel-pwm.o
+obj-$(CONFIG_GPIO_PWM) += gpio.o
\ No newline at end of file
-- 
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[[RFC] 1/5] API to consolidate PWM devices behind a common user and kernel interface

2009-10-19 Thread Bill Gatliff

Signed-off-by: Bill Gatliff 
---
 Documentation/pwm.txt |  258 ++
 drivers/pwm/pwm.c |  692 +
 include/linux/pwm.h   |  179 +++--
 3 files changed, 1109 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/pwm.txt
 create mode 100644 drivers/pwm/pwm.c

diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
new file mode 100644
index 000..b8932dd
--- /dev/null
+++ b/Documentation/pwm.txt
@@ -0,0 +1,258 @@
+   Generic PWM Device API
+
+  October 8, 2008
+   Bill Gatliff
+   
+
+
+
+The code in drivers/pwm and include/linux/pwm.h implements an API for
+applications involving pulse-width-modulation signals.  This document
+describes how the API implementation facilitates both PWM-generating
+devices, and users of those devices.
+
+
+
+Motivation
+
+The primary goals for implementing the "generic PWM API" are to
+consolidate the various PWM implementations within a consistent and
+redundancy-reducing framework, and to facilitate the use of
+hotpluggable PWM devices.
+
+Previous PWM-related implementations within the Linux kernel achieved
+their consistency via cut-and-paste, but did not need to (and didn't)
+facilitate more than one PWM-generating device within the system---
+hotplug or otherwise.  The Generic PWM Device API might be most
+appropriately viewed as an update to those implementations, rather
+than a complete rewrite.
+
+
+
+Challenges
+
+One of the difficulties in implementing a generic PWM framework is the
+fact that pulse-width-modulation applications involve real-world
+signals, which often must be carefully managed to prevent destruction
+of hardware that is linked to those signals.  A DC motor that
+experiences a brief interruption in the PWM signal controlling it
+might destructively overheat; it could change speed, losing
+synchronization with a sensor; it could even suddenly change direction
+or torque, breaking the mechanical device connected to it.
+
+(A generic PWM device framework is not directly responsible for
+preventing the above scenarios: that responsibility lies with the
+hardware designer and the application and driver authors.  But it must
+to the greatest extent possible make it easy to avoid such problems).
+
+A generic PWM device framework must accomodate the substantial
+differences between available PWM-generating hardware devices, without
+becoming sub-optimal for any of them.
+
+Finally, a generic PWM device framework must be relatively
+lightweight, computationally speaking.  Some PWM users demand
+high-speed outputs, plus the ability to regulate those outputs
+quickly.  A device framework must be able to "keep up" with such
+hardware, while still leaving time to do real work.
+
+The Generic PWM Device API is an attempt to meet all of the above
+requirements.  At its initial publication, the API was already in use
+managing small DC motors through a custom-designed, optically-isolated
+H-bridge driver.
+
+
+
+Functional Overview
+
+The Generic PWM Device API framework is implemented in
+include/linux/pwm.h and drivers/pwm/pwm.c.  The functions therein use
+information from pwm_device, pwm_channel and pwm_channel_config
+structures to invoke services in PWM peripheral device drivers.
+Consult drivers/pwm/atmel-pwm.c for an example driver.
+
+There are two classes of adopters of the PWM framework:
+
+  "Users" -- those wishing to employ the API merely to produce PWM
+  signals; once they have identified the appropriate physical output
+  on the platform in question, they don't care about the details of
+  the underlying hardware
+
+  "Driver authors" -- those wishing to bind devices that can generate
+  PWM signals to the Generic PWM Device API, so that the services of
+  those devices become available to users; assuming the hardware can
+  support the needs of a user, driver authors don't care about the
+  details of the user's application
+
+Generally speaking, users will first invoke pwm_request() to obtain a
+handle to a PWM device.  They will then pass that handle to functions
+like pwm_duty_ns() and pwm_period_ns() to set the duty cycle and
+period of the PWM signal, respectively.  They will also invoke
+pwm_start() and pwm_stop() to turn the signal on and off.
+
+The framework also provides a sysfs interface to PWM devices, which is
+adequate for basic needs and testing.
+
+Driver authors fill out a pwm_device structure, which describes the
+capabilities of the PWM hardware being driven--- including the number
+of distinct output "channels" the peripheral offers.  They then invoke
+pwm_register() (usually from within their device's probe() handler) to
+make the PWM API aware of their device.  The framework will call back
+to the methods described in the pwm_device structure to configure and
+use the hardware.
+
+Note that PWM signals can be produced by a variety of peripherals,
+bey

[[RFC] 3/5] Expunge old Atmel PWMC driver, replacing it with one that conforms to the PWM API

2009-10-19 Thread Bill Gatliff

Signed-off-by: Bill Gatliff 
---
 drivers/misc/Makefile|6 +-
 drivers/misc/atmel_pwm.c |  409 --
 drivers/pwm/atmel-pwm.c  |  633 ++
 3 files changed, 638 insertions(+), 410 deletions(-)
 delete mode 100644 drivers/misc/atmel_pwm.c
 create mode 100644 drivers/pwm/atmel-pwm.c

diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index f982d2e..35f13c2 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -4,7 +4,11 @@
 
 obj-$(CONFIG_IBM_ASM)  += ibmasm/
 obj-$(CONFIG_HDPU_FEATURES)+= hdpuftrs/
-obj-$(CONFIG_ATMEL_PWM)+= atmel_pwm.o
+obj-$(CONFIG_ASUS_LAPTOP)  += asus-laptop.o
+obj-$(CONFIG_EEEPC_LAPTOP) += eeepc-laptop.o
+obj-$(CONFIG_MSI_LAPTOP)   += msi-laptop.o
+obj-$(CONFIG_COMPAL_LAPTOP)+= compal-laptop.o
+obj-$(CONFIG_ACER_WMI) += acer-wmi.o
 obj-$(CONFIG_ATMEL_SSC)+= atmel-ssc.o
 obj-$(CONFIG_ATMEL_TCLIB)  += atmel_tclib.o
 obj-$(CONFIG_ICS932S401)   += ics932s401.o
diff --git a/drivers/misc/atmel_pwm.c b/drivers/misc/atmel_pwm.c
deleted file mode 100644
index 6aa5294..000
--- a/drivers/misc/atmel_pwm.c
+++ /dev/null
@@ -1,409 +0,0 @@
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-
-
-/*
- * This is a simple driver for the PWM controller found in various newer
- * Atmel SOCs, including the AVR32 series and the AT91sam9263.
- *
- * Chips with current Linux ports have only 4 PWM channels, out of max 32.
- * AT32UC3A and AT32UC3B chips have 7 channels (but currently no Linux).
- * Docs are inconsistent about the width of the channel counter registers;
- * it's at least 16 bits, but several places say 20 bits.
- */
-#definePWM_NCHAN   4   /* max 32 */
-
-struct pwm {
-   spinlock_t  lock;
-   struct platform_device  *pdev;
-   u32 mask;
-   int irq;
-   void __iomem*base;
-   struct clk  *clk;
-   struct pwm_channel  *channel[PWM_NCHAN];
-   void(*handler[PWM_NCHAN])(struct pwm_channel *);
-};
-
-
-/* global PWM controller registers */
-#define PWM_MR 0x00
-#define PWM_ENA0x04
-#define PWM_DIS0x08
-#define PWM_SR 0x0c
-#define PWM_IER0x10
-#define PWM_IDR0x14
-#define PWM_IMR0x18
-#define PWM_ISR0x1c
-
-static inline void pwm_writel(const struct pwm *p, unsigned offset, u32 val)
-{
-   __raw_writel(val, p->base + offset);
-}
-
-static inline u32 pwm_readl(const struct pwm *p, unsigned offset)
-{
-   return __raw_readl(p->base + offset);
-}
-
-static inline void __iomem *pwmc_regs(const struct pwm *p, int index)
-{
-   return p->base + 0x200 + index * 0x20;
-}
-
-static struct pwm *pwm;
-
-static void pwm_dumpregs(struct pwm_channel *ch, char *tag)
-{
-   struct device   *dev = &pwm->pdev->dev;
-
-   dev_dbg(dev, "%s: mr %08x, sr %08x, imr %08x\n",
-   tag,
-   pwm_readl(pwm, PWM_MR),
-   pwm_readl(pwm, PWM_SR),
-   pwm_readl(pwm, PWM_IMR));
-   dev_dbg(dev,
-   "pwm ch%d - mr %08x, dty %u, prd %u, cnt %u\n",
-   ch->index,
-   pwm_channel_readl(ch, PWM_CMR),
-   pwm_channel_readl(ch, PWM_CDTY),
-   pwm_channel_readl(ch, PWM_CPRD),
-   pwm_channel_readl(ch, PWM_CCNT));
-}
-
-
-/**
- * pwm_channel_alloc - allocate an unused PWM channel
- * @index: identifies the channel
- * @ch: structure to be initialized
- *
- * Drivers allocate PWM channels according to the board's wiring, and
- * matching board-specific setup code.  Returns zero or negative errno.
- */
-int pwm_channel_alloc(int index, struct pwm_channel *ch)
-{
-   unsigned long   flags;
-   int status = 0;
-
-   /* insist on PWM init, with this signal pinned out */
-   if (!pwm || !(pwm->mask & 1 << index))
-   return -ENODEV;
-
-   if (index < 0 || index >= PWM_NCHAN || !ch)
-   return -EINVAL;
-   memset(ch, 0, sizeof *ch);
-
-   spin_lock_irqsave(&pwm->lock, flags);
-   if (pwm->channel[index])
-   status = -EBUSY;
-   else {
-   clk_enable(pwm->clk);
-
-   ch->regs = pwmc_regs(pwm, index);
-   ch->index = index;
-
-   /* REVISIT: ap7000 seems to go 2x as fast as we expect!! */
-   ch->mck = clk_get_rate(pwm->clk);
-
-   pwm->channel[index] = ch;
-   pwm->handler[index] = NULL;
-
-   /* channel and irq are always disabled when we return */
-   pwm_writel(pwm, PWM_DIS, 1 << index);
-   pwm_writel(pwm, PWM_IDR, 1 << index);
-   }
-   spin_unlock_irqrestore(&pwm->lock, flags);
-   return status;
-}
-EXPORT_SYMBOL(pwm_channel_al

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

2009-10-19 Thread Bill Gatliff

Signed-off-by: Bill Gatliff 
---
 drivers/pwm/gpio.c |  318 
 1 files changed, 318 insertions(+), 0 deletions(-)
 create mode 100644 drivers/pwm/gpio.c

diff --git a/drivers/pwm/gpio.c b/drivers/pwm/gpio.c
new file mode 100644
index 000..35e5969
--- /dev/null
+++ b/drivers/pwm/gpio.c
@@ -0,0 +1,318 @@
+#define DEBUG 99
+/*
+ * drivers/pwm/gpio.c
+ *
+ * Models a single-channel PWM device using a kernel interval timer
+ * and a GPIO pin.
+ *
+ * Copyright (C) 2008 Bill Gatliff.  All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License Version
+ * 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
+ * USA
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+struct gpio_pwm {
+   struct pwm_device pwm;
+   struct hrtimer t;
+   struct work_struct work;
+   pwm_callback_t callback;
+   int gpio;
+   unsigned long polarity : 1;
+   unsigned long active : 1;
+};
+
+
+static void
+gpio_pwm_work (struct work_struct *work)
+{
+   struct gpio_pwm *gp = container_of(work, struct gpio_pwm, work);
+
+   if (gp->active)
+   gpio_direction_output(gp->gpio, gp->polarity ? 1 : 0);
+   else
+   gpio_direction_output(gp->gpio, gp->polarity ? 0 : 1);
+}
+
+
+static enum hrtimer_restart
+gpio_pwm_timeout(struct hrtimer *t)
+{
+   struct gpio_pwm *gp = container_of(t, struct gpio_pwm, t);
+
+   if (unlikely(gp->pwm.channels[0].duty_ticks == 0))
+   gp->active = 0;
+   else if (unlikely(gp->pwm.channels[0].duty_ticks
+ == gp->pwm.channels[0].period_ticks))
+   gp->active = 1;
+   else
+   gp->active ^= 1;
+
+   if (gpio_cansleep(gp->gpio))
+   schedule_work(&gp->work);
+   else
+   gpio_pwm_work(&gp->work);
+
+   if (!gp->active && gp->pwm.channels[0].callback)
+   gp->pwm.channels[0].callback(&gp->pwm.channels[0]);
+
+   if (unlikely(!gp->active &&
+(gp->pwm.channels[0].flags & BIT(FLAG_STOP {
+   clear_bit(FLAG_STOP, &gp->pwm.channels[0].flags);
+   complete_all(&gp->pwm.channels[0].complete);
+   return HRTIMER_NORESTART;
+   }
+
+   if (gp->active)
+   hrtimer_start(&gp->t,
+ ktime_set(0, gp->pwm.channels[0].duty_ticks),
+ HRTIMER_MODE_REL);
+   else
+   hrtimer_start(&gp->t,
+ ktime_set(0,gp->pwm.channels[0].period_ticks
+   - gp->pwm.channels[0].duty_ticks),
+ HRTIMER_MODE_REL);
+   return HRTIMER_NORESTART;
+}
+
+
+static void gpio_pwm_start(struct pwm_channel *p)
+{
+   struct gpio_pwm *gp = container_of(p->pwm, struct gpio_pwm, pwm);
+
+   gp->active = 0;
+   gpio_pwm_timeout(&gp->t);
+   pr_debug("%s:%d start, %lu ticks\n",
+dev_name(p->pwm->dev), p->chan, p->duty_ticks);
+}
+
+
+static int
+gpio_pwm_config_nosleep(struct pwm_channel *p,
+   struct pwm_channel_config *c)
+{
+   struct gpio_pwm *gp = container_of(p->pwm, struct gpio_pwm, pwm);
+   int ret = 0;
+   unsigned long flags;
+
+   spin_lock_irqsave(&p->lock, flags);
+
+   switch (c->config_mask) {
+
+   case PWM_CONFIG_DUTY_TICKS:
+   p->duty_ticks = c->duty_ticks;
+   dev_dbg(p->pwm->dev, ":%d duty_ticks %lu\n",
+   p->chan, p->duty_ticks);
+   break;
+
+   case PWM_CONFIG_START:
+   if (!hrtimer_active(&gp->t)) {
+   gpio_pwm_start(p);
+   }
+   break;
+   default:
+   ret = -EINVAL;
+   break;
+   }
+
+   spin_unlock_irqrestore(&p->lock, flags);
+   return ret;
+}
+
+
+static int
+gpio_pwm_stop_sync(struct pwm_channel *p)
+{
+   struct gpio_pwm *gp = container_of(p->pwm, struct gpio_pwm, pwm);
+   int ret;
+   int was_on = hrtimer_active(&gp->t);
+
+   if (was_on) {
+   do {
+   init_completion(&p->complete);
+   set_bit(FLAG_STOP, &p->flags);
+
+   dev_dbg(p->pwm->dev, ":%d waiting on stop_sync 
completion...\n",
+   p->chan);
+
+  

[[RFC] 4/5] An LED "dimmer" trigger, which uses the PWM API to vary the brightness of an LED according to system load

2009-10-19 Thread Bill Gatliff

Signed-off-by: Bill Gatliff 
---
 drivers/leds/Kconfig   |   32 ++-
 drivers/leds/Makefile  |3 +
 drivers/leds/leds-pwm.c|  224 +++-
 drivers/leds/ledtrig-dim.c |   95 +++
 include/linux/pwm-led.h|   34 +++
 5 files changed, 278 insertions(+), 110 deletions(-)
 create mode 100644 drivers/leds/ledtrig-dim.c
 create mode 100644 include/linux/pwm-led.h

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index e4f599f..b39c863 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -17,12 +17,12 @@ config LEDS_CLASS
 
 comment "LED drivers"
 
-config LEDS_ATMEL_PWM
-   tristate "LED Support using Atmel PWM outputs"
-   depends on LEDS_CLASS && ATMEL_PWM
+config LEDS_CORGI
+   tristate "LED Support for the Sharp SL-C7x0 series"
+   depends on LEDS_CLASS && PXA_SHARP_C7xx
help
- This option enables support for LEDs driven using outputs
- of the dedicated PWM controller found on newer Atmel SOCs.
+ This option enables support for the LEDs on Sharp Zaurus
+ SL-C7x0 series (C700, C750, C760, C860).
 
 config LEDS_LOCOMO
tristate "LED Support for Locomo device"
@@ -141,6 +141,20 @@ config LEDS_GPIO_OF
bool "OpenFirmware platform device bindings for GPIO LEDs"
depends on LEDS_GPIO && OF_DEVICE
default y
+
+config LEDS_HP_DISK
+   tristate "LED Support for disk protection LED on HP notebooks"
+   depends on LEDS_CLASS && ACPI
+
+config LEDS_PWM
+   tristate "LED Support for PWM connected LEDs"
+   depends on LEDS_CLASS && GENERIC_PWM
+   help
+ Enables support for LEDs connected to PWM outputs.
+
+config LEDS_CM_X270
+   tristate "LED Support for the CM-X270 LEDs"
+   depends on LEDS_CLASS && MACH_ARMCORE
help
  Let the leds-gpio driver drive LEDs which have been defined as
  of_platform devices.  For instance, LEDs which are listed in a "dts"
@@ -263,6 +277,14 @@ config LEDS_TRIGGER_IDE_DISK
  This allows LEDs to be controlled by IDE disk activity.
  If unsure, say Y.
 
+config LEDS_TRIGGER_DIM
+   tristate "LED Dimmer Trigger"
+   depends on LEDS_TRIGGERS
+   help
+ Regulates the brightness of an LED based on the 1-minute CPU
+ load average.  Ideal for PWM-driven LEDs.
+ If unsure, say Y.
+
 config LEDS_TRIGGER_HEARTBEAT
tristate "LED Heartbeat Trigger"
depends on LEDS_TRIGGERS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 46d7270..f4ff10b 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -21,6 +21,8 @@ obj-$(CONFIG_LEDS_SUNFIRE)+= leds-sunfire.o
 obj-$(CONFIG_LEDS_PCA9532) += leds-pca9532.o
 obj-$(CONFIG_LEDS_GPIO)+= leds-gpio.o
 obj-$(CONFIG_LEDS_LP3944)  += leds-lp3944.o
+obj-$(CONFIG_LEDS_PWM) += leds-pwm.o
+obj-$(CONFIG_LEDS_CM_X270)  += leds-cm-x270.o
 obj-$(CONFIG_LEDS_CLEVO_MAIL)  += leds-clevo-mail.o
 obj-$(CONFIG_LEDS_HP6XX)   += leds-hp6xx.o
 obj-$(CONFIG_LEDS_FSG) += leds-fsg.o
@@ -36,6 +38,7 @@ obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
 # LED Triggers
 obj-$(CONFIG_LEDS_TRIGGER_TIMER)   += ledtrig-timer.o
 obj-$(CONFIG_LEDS_TRIGGER_IDE_DISK)+= ledtrig-ide-disk.o
+obj-$(CONFIG_LEDS_TRIGGER_DIM) += ledtrig-dim.o
 obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT)   += ledtrig-heartbeat.o
 obj-$(CONFIG_LEDS_TRIGGER_BACKLIGHT)   += ledtrig-backlight.o
 obj-$(CONFIG_LEDS_TRIGGER_GPIO)+= ledtrig-gpio.o
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index cdfdc87..3103dc3 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -1,153 +1,167 @@
-/*
- * linux/drivers/leds-pwm.c
- *
- * simple PWM based LED control
- *
- * Copyright 2009 Luotao Fu @ Pengutronix (l...@pengutronix.de)
- *
- * based on leds-gpio.c by Raphael Assenat 
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-
-#include 
 #include 
-#include 
 #include 
-#include 
 #include 
-#include 
+#include 
 #include 
-#include 
-
-struct led_pwm_data {
-   struct led_classdev cdev;
-   struct pwm_device   *pwm;
-   unsigned intactive_low;
-   unsigned intperiod;
-   unsigned intmax_brightness;
+#include 
+
+
+struct led_pwm {
+   struct led_classdev led;
+   struct pwm_channel  *pwm;
+   int percent;
 };
 
-static void led_pwm_set(struct led_classdev *led_cdev,
-   enum led_brightness brightness)
+
+static void
+led_pwm_brightness_set(struct led_classdev *c,
+  enum led_brightness b)
+{
+   struct led_pwm *led;
+   int percent;
+
+   percent = (b * 100) / (LE