Re: Adding support for configuring polarity in PWM framework.

From: Thierry Reding
Date: Mon Jul 16 2012 - 08:46:08 EST

On Mon, Jul 16, 2012 at 12:23:46PM +0000, Philip, Avinash wrote:
> On Mon, Jul 16, 2012 at 17:09:21, Thierry Reding wrote:
> > On Mon, Jul 16, 2012 at 11:15:50AM +0000, Philip, Avinash wrote:
> > > Hi Thierry,
> > >
> > > On one of the custom boards we are using, uses PWM to drive the backlight. However, for
> > > this device, PWM signal needs to be inversed.
> > > So, we need to a platform data to indicate this parameter.
> > > Current PWM framework doesn't provide .support for setting polarity (or inverse polarity).
> > >
> > > Have you come across any such requirements? If so, do you have any plans to implement it?
> >
> > I don't have any plans to implement such a feature.
> Ok. Thanks for the quick response.
> >
> > > I am planning to add support for the same but want to avoid duplication of work.
> > >
> > > If you have no plans, then I will send a patch to support the same.
> >
> > I wonder how you want to implement this. You'll need special hardware
> > support for it
> Yes. Our custom hardware (backlight booster) requires the pwm signal to be
> inverted.
> > you may be able to implement it in the driver itself
> > instead of putting it into the framework.
> This is a client specific data (backlight needs pwm signal inversed)
> and not the main device feature (not PWM IP). So we cannot send this in
> pwm platform data. This would come as call from client driver (which in
> our case is from pwm_bl.c)

Okay, I see.

> > Anyway I'm interested in seeing your patch.
> I am planning to modify PWM framework as below.
> 1. Configure PWM polarity from client driver (using platform data provided
> to pwm backlight driver).
> 2. PWM device needs to be disabled before calling the set-polarity API.

Okay, that sounds sensible. A couple of comments though.

> This involves
> 1. PWM framework API addition.
> PWM frame work API support.
> /**
> * pwm_setpolarity() - change a PWM device Polarity
> * @pwm: PWM device
> * @polarity: Configure polarity of PWM
> *
> * polarity - false -> "on" time defined by duty ns
> * - true -> "off' time defined by duty ns.
> */
> int pwm_setpolarity(struct pwm_device *pwm, bool inversepol);

This should match the pwm_ops name, i.e.: pwm_set_polarity().

Making the polarity argument a boolean is slightly confusing. For
instance I'd say the logical value if I want normal behaviour would be
to set it to true, which doesn't match your example. So I propose you
define the polarity parameter as an enumeration to make its meaning more

enum pwm_polarity {

other good name pairs.

> 2. Add "set_polarity" operation support in pwm_ops.
> 3. Modification in backlight driver (pwm_bl.c) to support polarity
> configuration.

We also need to think about how this could be represented in the device
tree. The most obvious choice seems to be a third cell for the specifier
and use a custom of_xlate callback for controllers that support polarity
inversion (and later perhaps other flags).

Also would you mind sharing the board setup code that you need this for?
I find it easier to get into the right mindset when looking at code that
actually uses this.


Attachment: pgp00000.pgp
Description: PGP signature