Messages in this thread | | | From | "Philip, Avinash" <> | Subject | RE: Adding support for configuring polarity in PWM framework. | Date | Mon, 16 Jul 2012 14:46:16 +0000 |
| |
On Mon, Jul 16, 2012 at 18:16:08, Thierry Reding wrote: > 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().
Ok.
> > 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 > explicit: > > enum pwm_polarity { > PWM_POLARITY_NORMAL, > PWM_POLARITY_INVERSE, > }; > > PWM_POLARITY_{HIGH,LOW} and PWM_POLARITY_{POSITIVE,NEGATIVE} would be > other good name pairs.
Ok. I will do.
> > > 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). >
Ok I will try to use modified of_xlate callback and hope this can rescue pwm_bl.c modification.
> 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. >
Here is the TI BSP link to support backlight inverse.
http://arago-project.org/git/projects/?p=linux-am33x.git;a=commitdiff;h= 59e96b24925e64fffd4664d696e41e1090c959b1;hp= b180dcb341db0ff4ca1adbfac3f5dcd07be9e91d
But here we were Supporting PWM frame work from Bill Gatliff. Also we were configuring PWM polarity directly using eCAP platform data not from backlight platform data. But I think controlling through device/client is more methodical than direct PWM data handling.
Thanks Avinash
> Thierry >
| |