Re: Adding support for configuring polarity in PWM framework.

Re: Adding support for configuring polarity in PWM framework.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 939 bytes --]

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.

> 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 and you may be able to implement it in the driver itself
instead of putting it into the framework. Anyway I'm interested in
seeing your patch.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

RE: Adding support for configuring polarity in PWM framework.

[Philip, Avinash]

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)

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

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

2. Add "set_polarity" operation support in pwm_ops.

3. Modification in backlight driver (pwm_bl.c) to support polarity configuration.

Thanks
Avinash

> 
> Thierry
> 

Re: Adding support for configuring polarity in PWM framework.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3413 bytes --]

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

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

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: Adding support for configuring polarity in PWM framework.

[Lars-Peter Clausen]

On 07/16/2012 02:23 PM, 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. 
> 

I think this is a common feature amongst PWM chips that they are able to
invert the PWM signal. And some applications of PWM require that you are
able to specify the polarity, so I think it makes sense to put this into the
common framework.

>> 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.
> 
> 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.
>  	*/

Isn't this more about whether we start with a low or a high signal? If it is
just about the duty time you can easily achieve the same effect by setting
it to (period_ns - duty_ns).

- Lars

RE: Adding support for configuring polarity in PWM framework.

[Philip, Avinash]

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
> 

RE: Adding support for configuring polarity in PWM framework.

[Philip, Avinash]

On Mon, Jul 16, 2012 at 18:16:13, Lars-Peter Clausen wrote:
> On 07/16/2012 02:23 PM, Philip, Avinash wrote:
> 
> > 
> > 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.
> >  	*/
> 
> Isn't this more about whether we start with a low or a high signal?

No, it is more about the average on time. On time determines the on duration of
the panel power and there by brightness. On one custom board, backlight power
booster gives power output on the OFF time of PWM (hardware connection).

> If it is
> just about the duty time you can easily achieve the same effect by setting
> it to (period_ns - duty_ns).

Yes this is possible. But the PWM hardware we uses supports configuration of
polarity. So I prefer PWM polarity configuration.
Also with polarity configuration support, we can achieve (period_ns - duty_ns),
for PWM devices don't have hardware support. Polarity configuration support
can be used for setting flags, duty cycle can be configured from config
depending on flag. 

Thanks
Avinash

> 
> - Lars
> 
>