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:
pgpIgaDf4iH0j.pgp
Description: PGP signature
