Re: [PATCH] PWM: Add support for configuring polarity of PWM

[Thierry Reding]

On Mon, Jul 23, 2012 at 10:15:07PM +0200, Lars-Peter Clausen wrote:
> On 07/23/2012 10:30 AM, Thierry Reding wrote:
> > On Wed, Jul 18, 2012 at 06:24:13PM +0530, Philip, Avinash wrote:
> >>[...]
> >> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> >> index 21d076c..2e4e960 100644
> >> --- a/include/linux/pwm.h
> >> +++ b/include/linux/pwm.h
> >> @@ -21,6 +21,16 @@ void pwm_free(struct pwm_device *pwm);
> >>   */
> >>  int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);
> >>  
> >> +enum {
> >> +	PWM_POLARITY_NORMAL,	/* ON period depends on duty_ns */
> >> +	PWM_POLARITY_INVERSE,	/* OFF period depends on duty_ns */
> >> +};
> > 
> > You should name this enumeration so that it can actually be used as a
> > type (enum pwm_polarity). Also you can drop the comments because they
> > only apply to the specific use-case of simulating duty-cycle inversion
> 
> I think we should make it very explicit what normal polarity and inverse
> polarity is. There are certain applications where it is important. E.g. one
> such application would be using it in the IIO framework to generate a trigger
> pulse to synchronize devices. If we do not specify how each of these modes
> should behave drivers may interpret and implement them differently.

I agree, the definition should be on a physical level.

> I'd vote for the following definitions:
> PWM_POLARITY_NORMAL: A high signal for the duration of duty_ns, followed by a
> low signal for the duration of (period_ns - duty_ns).
> PWM_POLARITY_INVERSE: A low signal for the duration duty_ns, followed by a high
> signal for the duration of (period_ns - duty_ns).

That's my understanding of normal vs. inversed as well. I haven't yet
seen a formal definition of the standard PWM waveform, but I believe
this describes the most common implementation.

> Maybe even rename them to PWM_POLARITY_ACTIVE_HIGH and PWM_POLARITY_ACTIVE_LOW
> since it is a bit more explicit on how the waveform should look like. "NORMAL"
> and "INVERSE" sort of depend on what you consider to be normal.

But aren't active-high and -low equally arbitrary? They don't make it
obvious as to where the active period is, either. I think it'd be enough
if we use your definitions above as comments for the enumerations. After
all the important thing here is to have an unambiguous definition. And I
think for consistency we should call it PWM_POLARITY_INVERSED, that is
if we keep those two definitions.

How about the following?

/**
 * enum pwm_polarity - polarity of a PWM signal
 * @PWM_POLARITY_NORMAL: a high signal for the duration of the duty-
 *   cycle, followed by a low signal for the remainder of the pulse
 *   period
 * @PWM_POLARITY_INVERSED: a low signal for the duration of the duty-
 *   cycle, followed by a high signal for the remainder of the pulse
 *   period
 */
enum pwm_polarity {
	PWM_POLARITY_NORMAL,
	PWM_POLARITY_INVERSED,
};

Thierry

Attachment: pgp00000.pgp
Description: PGP signature