[RFC] add pwmlib support
mbox series

Message ID 1296217283-14531-1-git-send-email-s.hauer@pengutronix.de
State New, archived
Headers show
Series
  • [RFC] add pwmlib support
Related show

Pull-request

git://git.pengutronix.de/git/imx/linux-2.6.git pwmlib

Message

Sascha Hauer Jan. 28, 2011, 12:21 p.m. UTC
Hi all,

While implementing just another pwm driver I thought it's time to implement
generic pwm support. The following series adds drivers/pwm/pwmlib.c and
a i.MX23/28 pwm driver which serves as a usage example for pwmlib. The
code is inspired by gpiolib support and tested using the backlight pwm
driver by Eric Miao.

Currently the pwm_request, pwm_add, pwm_remove and pwm_free operations are
protected with a single mutex wherea the pwm_enable, pwm_disable and
pwm_config operations are unlocked. It is assumed that the owners of the
pwm handle the serialization of the pwm accesses. This may not be enough, so
i'd like to discuss the locking (and type of locking) here.

I Cced the people working with PWMs in the kernel in the hope that they can
give input on what's missing / wrong in this implementation

Sascha


The following changes since commit 2b1caf6ed7b888c95a1909d343799672731651a5:

  Merge branch 'core-fixes-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip (2011-01-20 18:30:37 -0800)

are available in the git repository at:

  git://git.pengutronix.de/git/imx/linux-2.6.git pwmlib

Sascha Hauer (2):
      pwmlib: add pwm support
      pwm: Add a i.MX23/28 pwm driver

 drivers/Kconfig       |    2 +
 drivers/Makefile      |    1 +
 drivers/pwm/Kconfig   |   16 +++
 drivers/pwm/Makefile  |    3 +
 drivers/pwm/mxs-pwm.c |  275 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pwm/pwmlib.c  |  246 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/pwm.h   |   38 +++++++
 7 files changed, 581 insertions(+), 0 deletions(-)
 create mode 100644 drivers/pwm/Kconfig
 create mode 100644 drivers/pwm/Makefile
 create mode 100644 drivers/pwm/mxs-pwm.c
 create mode 100644 drivers/pwm/pwmlib.c

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Comments

Ben Dooks Jan. 28, 2011, 2:12 p.m. UTC | #1
On Fri, Jan 28, 2011 at 01:21:21PM +0100, Sascha Hauer wrote:
> Hi all,
> 
> While implementing just another pwm driver I thought it's time to implement
> generic pwm support. The following series adds drivers/pwm/pwmlib.c and
> a i.MX23/28 pwm driver which serves as a usage example for pwmlib. The
> code is inspired by gpiolib support and tested using the backlight pwm
> driver by Eric Miao.

You know, I really wish driver cores like this where not called lib... it
is hardly as if they can be taken out of the kernel and usefully re-used
in another project.
Matt Sealey Jan. 29, 2011, 12:21 a.m. UTC | #2
On Fri, Jan 28, 2011 at 8:12 AM, root <ben@fluff.org> wrote:
>
> On Fri, Jan 28, 2011 at 01:21:21PM +0100, Sascha Hauer wrote:
>> Hi all,
>>
>> While implementing just another pwm driver I thought it's time to implement
>> generic pwm support. The following series adds drivers/pwm/pwmlib.c and
>> a i.MX23/28 pwm driver which serves as a usage example for pwmlib. The
>> code is inspired by gpiolib support and tested using the backlight pwm
>> driver by Eric Miao.
>
> You know, I really wish driver cores like this where not called lib... it
> is hardly as if they can be taken out of the kernel and usefully re-used
> in another project.

Well, I've never known a collection of code shared across multiple
client applications with the specific intent of consolidating
functonality under a single framework to be called many other things
than a "library". It doesn't matter what address space it's in, it's a
library of reusable code, so I think the name stands up.

Sascha, this is awesome, BTW. I'm going to see if I can get PWM for
MX5x and MC13892 under this..
Jean Delvare Jan. 29, 2011, 8:38 p.m. UTC | #3
Hi Sascha,

On Fri, 28 Jan 2011 13:21:21 +0100, Sascha Hauer wrote:
> While implementing just another pwm driver I thought it's time to implement
> generic pwm support. The following series adds drivers/pwm/pwmlib.c and
> a i.MX23/28 pwm driver which serves as a usage example for pwmlib. The
> code is inspired by gpiolib support and tested using the backlight pwm
> driver by Eric Miao.
> 
> Currently the pwm_request, pwm_add, pwm_remove and pwm_free operations are
> protected with a single mutex wherea the pwm_enable, pwm_disable and
> pwm_config operations are unlocked. It is assumed that the owners of the
> pwm handle the serialization of the pwm accesses. This may not be enough, so
> i'd like to discuss the locking (and type of locking) here.
> 
> I Cced the people working with PWMs in the kernel in the hope that they can
> give input on what's missing / wrong in this implementation

I do not have the time to review your patches. But I think your
introduction above lacks a fundamental point, which is: what are you
trying to achieve? What would you be able to do with pwmlib, which you
currently aren't?

As far as hwmon driver are concerned, please understand that "PWM" is a
misnomer for "fan speed control". Some hwmon drivers have pwm
attributes which actually correspond to analog voltage outputs. In both
cases (PWM and DC) these are outputs which are dedicated to fan
control, and I've never seen any other use of these, so I doubt it
would make much sense to register them as generic pwm outputs.

So I believe that hwmon drivers are out of scope for pwmlib. Which
makes me wonder what you intend to do with pwmlib (back to my first
question...) as the only use I know of for PWM signals in computers is
for fan control.
Lars-Peter Clausen Jan. 29, 2011, 8:53 p.m. UTC | #4
On 01/28/2011 01:21 PM, Sascha Hauer wrote:
> Hi all,
> 
> While implementing just another pwm driver I thought it's time to implement
> generic pwm support. The following series adds drivers/pwm/pwmlib.c and
> a i.MX23/28 pwm driver which serves as a usage example for pwmlib. The
> code is inspired by gpiolib support and tested using the backlight pwm
> driver by Eric Miao.
> 
> Currently the pwm_request, pwm_add, pwm_remove and pwm_free operations are
> protected with a single mutex wherea the pwm_enable, pwm_disable and
> pwm_config operations are unlocked. It is assumed that the owners of the
> pwm handle the serialization of the pwm accesses. This may not be enough, so
> i'd like to discuss the locking (and type of locking) here.
> 
> I Cced the people working with PWMs in the kernel in the hope that they can
> give input on what's missing / wrong in this implementation
> 
> Sascha
> 

Hi

There have been two other proposals for a generic PWM api during the last year.
You might want to take a look at them.

https://lkml.org/lkml/2010/2/9/275
https://lkml.org/lkml/2010/9/28/107

I've added Bill Gatliff and Arun Murthy to Cc.

- Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Wolfram Sang Jan. 29, 2011, 9:51 p.m. UTC | #5
> > code is inspired by gpiolib support and tested using the backlight pwm
							     ^^^^^^^^^
> So I believe that hwmon drivers are out of scope for pwmlib. Which
> makes me wonder what you intend to do with pwmlib (back to my first
> question...) as the only use I know of for PWM signals in computers is
> for fan control.

It was meant for backlight control (was mentioned very indirectly).
I also pointed him to Bill's implementation, too.

Regards,

   Wolfram
arden jay Jan. 30, 2011, 3:49 a.m. UTC | #6
Sorry, I'm confused about this.
If it is for backlight control, why it don't go through led_class?

Do I miss anything?

2011/1/30 Wolfram Sang <w.sang@pengutronix.de>:
>
>> > code is inspired by gpiolib support and tested using the backlight pwm
>                                                             ^^^^^^^^^
>> So I believe that hwmon drivers are out of scope for pwmlib. Which
>> makes me wonder what you intend to do with pwmlib (back to my first
>> question...) as the only use I know of for PWM signals in computers is
>> for fan control.
>
> It was meant for backlight control (was mentioned very indirectly).
> I also pointed him to Bill's implementation, too.
>
> Regards,
>
>   Wolfram
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
>
> iEYEARECAAYFAk1Ei+0ACgkQD27XaX1/VRvjkACeOhJonBEZQX8U+ihKtiMjQT/U
> IfAAoMdZiOrAOvuH1Iv6ZKNYoT+VhhTM
> =ROYN
> -----END PGP SIGNATURE-----
>
>
Arun MURTHY Jan. 31, 2011, 3:35 a.m. UTC | #7
Hi Sascha,

> > I Cced the people working with PWMs in the kernel in the hope that
> they can
> > give input on what's missing / wrong in this implementation
> >
> > Sascha
> >
> 
> Hi
> 
> There have been two other proposals for a generic PWM api during the
> last year.
> You might want to take a look at them.
> 
> https://lkml.org/lkml/2010/2/9/275
> https://lkml.org/lkml/2010/9/28/107
> 
> I've added Bill Gatliff and Arun Murthy to Cc.
> 

As said by Lars, we already have developed the pwm core driver and
progressing towards aligning the existing pwm drivers to the pwm core
driver.
These set of patches are expected to be out in LKML by this week.

Thanks and Regards,
Arun R Murthy
------------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Sascha Hauer Jan. 31, 2011, 7:54 a.m. UTC | #8
On Mon, Jan 31, 2011 at 04:35:33AM +0100, Arun MURTHY wrote:
> Hi Sascha,
> 
> > > I Cced the people working with PWMs in the kernel in the hope that
> > they can
> > > give input on what's missing / wrong in this implementation
> > >
> > > Sascha
> > >
> > 
> > Hi
> > 
> > There have been two other proposals for a generic PWM api during the
> > last year.
> > You might want to take a look at them.
> > 
> > https://lkml.org/lkml/2010/2/9/275
> > https://lkml.org/lkml/2010/9/28/107
> > 
> > I've added Bill Gatliff and Arun Murthy to Cc.
> > 
> 
> As said by Lars, we already have developed the pwm core driver and
> progressing towards aligning the existing pwm drivers to the pwm core
> driver.
> These set of patches are expected to be out in LKML by this week.

Nice, problem solved without me having to work on it ;). Please consider
Ccing linux-arm-kernel as many of your potential users are following this
list.

Sascha
Sascha Hauer Jan. 31, 2011, 7:58 a.m. UTC | #9
On Sun, Jan 30, 2011 at 11:49:32AM +0800, arden jay wrote:
> Sorry, I'm confused about this.
> If it is for backlight control, why it don't go through led_class?

PWMs are not limited to backlight. As Jean pointed out they can be used
for fans (or more generally motors) aswell. Have a look at
drivers/leds/leds-pwm.c, it is a user of this pwm API.

Sascha
Lars-Peter Clausen Jan. 31, 2011, 12:48 p.m. UTC | #10
On 01/31/2011 08:54 AM, Sascha Hauer wrote:
> On Mon, Jan 31, 2011 at 04:35:33AM +0100, Arun MURTHY wrote:
>> Hi Sascha,
>>
>>>> I Cced the people working with PWMs in the kernel in the hope that
>>> they can
>>>> give input on what's missing / wrong in this implementation
>>>>
>>>> Sascha
>>>>
>>>
>>> Hi
>>>
>>> There have been two other proposals for a generic PWM api during the
>>> last year.
>>> You might want to take a look at them.
>>>
>>> https://lkml.org/lkml/2010/2/9/275
>>> https://lkml.org/lkml/2010/9/28/107
>>>
>>> I've added Bill Gatliff and Arun Murthy to Cc.
>>>
>>
>> As said by Lars, we already have developed the pwm core driver and
>> progressing towards aligning the existing pwm drivers to the pwm core
>> driver.
>> These set of patches are expected to be out in LKML by this week.
> 
> Nice, problem solved without me having to work on it ;).

I wouldn't call it problem solved yet.
I liked your approach better so far, but lets see how the next iteration of Aruns
patches turn out.

- Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Sascha Hauer Jan. 31, 2011, 1 p.m. UTC | #11
On Mon, Jan 31, 2011 at 01:48:43PM +0100, Lars-Peter Clausen wrote:
> On 01/31/2011 08:54 AM, Sascha Hauer wrote:
> > On Mon, Jan 31, 2011 at 04:35:33AM +0100, Arun MURTHY wrote:
> >> Hi Sascha,
> >>
> >>>> I Cced the people working with PWMs in the kernel in the hope that
> >>> they can
> >>>> give input on what's missing / wrong in this implementation
> >>>>
> >>>> Sascha
> >>>>
> >>>
> >>> Hi
> >>>
> >>> There have been two other proposals for a generic PWM api during the
> >>> last year.
> >>> You might want to take a look at them.
> >>>
> >>> https://lkml.org/lkml/2010/2/9/275
> >>> https://lkml.org/lkml/2010/9/28/107
> >>>
> >>> I've added Bill Gatliff and Arun Murthy to Cc.
> >>>
> >>
> >> As said by Lars, we already have developed the pwm core driver and
> >> progressing towards aligning the existing pwm drivers to the pwm core
> >> driver.
> >> These set of patches are expected to be out in LKML by this week.
> > 
> > Nice, problem solved without me having to work on it ;).
> 
> I wouldn't call it problem solved yet.
> I liked your approach better so far, but lets see how the next iteration of Aruns
> patches turn out.

What I don't like about Bills patches is the way PWMs are configured.

Bill, since you are on Cc, maybe you can comment on this:

> +enum {
> +	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),
> +	PWM_CONFIG_DUTY_PERCENT = BIT(7),
> +	PWM_CONFIG_PERIOD_NS = BIT(8),
> +};
> +
>
> ...
>
> +
> +struct pwm_channel_config {
> +	int config_mask;
> +	unsigned long duty_ticks;
> +	unsigned long period_ticks;
> +	int polarity;
> +
> +	pwm_handler_t handler;
> +
> +	unsigned long duty_ns;
> +	unsigned long period_ns;
> +	int duty_percent;
> +};
> 
> ...
> 
> +int pwm_config(struct pwm_channel *pwm,
> +	       struct pwm_channel_config *c);

I think we should have a single internal interpretation of how a pwm is
configured, either ticks or ns (or whatever else), but not ticks, ns and
percent. Instead we could provide helpers to convert between them.
Also, I don't like ioctl like function calls. Instead of dispatching
PWM_CONFIG_* we should use discrete functions for each functionality.

Sascha
Linus Walleij Feb. 6, 2011, 1:22 p.m. UTC | #12
2011/1/31 Sascha Hauer <s.hauer@pengutronix.de>:
> On Sun, Jan 30, 2011 at 11:49:32AM +0800, arden jay wrote:
>> Sorry, I'm confused about this.
>> If it is for backlight control, why it don't go through led_class?
>
> PWMs are not limited to backlight. As Jean pointed out they can be used
> for fans (or more generally motors) aswell. Have a look at
> drivers/leds/leds-pwm.c, it is a user of this pwm API.

Low-frequency PWM is used for vibrators of the type found in
silent mode cellphones, to mention another use case.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/