On Mon, Jul 23, 2012 at 14:00:32, Thierry Reding wrote: > On Wed, Jul 18, 2012 at 06:24:13PM +0530, Philip, Avinash wrote: > > Duty cycle inversion of PWM wave should achieved through PWM polarity > > inversion. Also polarity of PWM wave should configurable from slave > > drivers, > > Actually, I don't think that duty cycle inversion *should* be achieved > through polarity inversion. But it is true that the same effect *can* be > achieved by inverting the polarity. Correct. > > > Configure polarity > > 1. PWM_POLARITY_NORMAL -> duty ns defines ON period of PWM wave > > 2. PWM_POLARITY_INVERSE -> duty ns defines OFF period of PWM wave. > > Similarly, this text describes how polarity inversion can be used to > simular duty cycle inversion. > > I think you should describe that this change adds support for > configuring the PWM polarity. If you absolutely must note that it can be > used to simulate the duty cycle inversion, then you can give it as an > example below the actual description. > > > This patch adds support for configuring PWM polarity in PWM frame work. > > "framework" is one word. Ok. > > > > > Signed-off-by: Philip, Avinash <avinashphilip@xxxxxx> > > --- > > Configuring polarity to be done with PWM device disabled, if not > > failure reported. > > > > If PWM device is enabled while configuring polarity, disabling and > > re_enabling make it complex. Whoever uses this API has to taken > > care of the basic rules. > > These comments belong in the commit message because they are very useful > information about the change that you introduce. They probably belong > somewhere in the code as well. > > > Discussions related to this can found at > > http://www.spinics.net/lists/kernel/msg1372110.html > > Here's my proposal for a revised commit message: > > pwm: Add support for configuring the PWM polarity > > Some hardware supports inverting the polarity of the PWM signal. This > commit adds support to the PWM framework to allow users of the PWM API > to configure the polarity. Note that in order to reduce complexity, > changing the polarity of a PWM signal is only allowed while the PWM is > disabled. > > A practical example where this can prove useful is to simulate inversion > of the duty cycle. While inversion of polarity and duty cycle are not > exactly the same, the differences for most use-cases are negligible. Ok I will update. > > > :100644 100644 ecb7690... 24d5495... M drivers/pwm/core.c > > :100644 100644 21d076c... 2e4e960... M include/linux/pwm.h > > drivers/pwm/core.c | 32 ++++++++++++++++++++++++++++++++ > > include/linux/pwm.h | 15 +++++++++++++++ > > 2 files changed, 47 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > > index ecb7690..24d5495 100644 > > --- a/drivers/pwm/core.c > > +++ b/drivers/pwm/core.c > > @@ -379,6 +379,38 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns) > > EXPORT_SYMBOL_GPL(pwm_config); > > > > /** > > + * pwm_set_polarity() - change PWM device Polarity > > Maybe: "pwm_set_polarity() - configure the polarity of a PWM signal" > > > + * @pwm: PWM device > > + * @polarity: Configure polarity of PWM > > "new polarity of the PWM signal" > > > + * > > + * Polarity to be configured with PWM device disabled. > > "Note that the polarity cannot be configured while the PWM device is > enabled." Ok I will update. > > > + */ > > +int pwm_set_polarity(struct pwm_device *pwm, int polarity) > > The polarity parameter should be an enum pwm_polarity, see below. > > > +{ > > + int pwm_flags = PWM_POLARITY_NORMAL; > > I don't think this is needed. > > > + > > + if (!pwm || !pwm->chip->ops->set_polarity) > > + return -EINVAL; > > I'd prefer -ENOSYS if .set_polarity is not implemented, so this check > should probably be split up: > > if (!pwm || !pwm->chip || !pwm->chip->ops) > return -EINVAL; > > if (!pwm->chip->ops->set_polarity) > return -ENOSYS; I will correct it. > > > + > > + if (test_bit(PWMF_ENABLED, &pwm->flags)) { > > + dev_err(pwm->chip->dev, > > + "Polarity configuration Failed!, PWM device enabled\n"); > > + return -EBUSY; > > + } > > Maybe something like: "polarity cannot be configured while PWM device is > enabled"? Ok I will update. > Though I'm not sure the error message is all that useful. I'd > expect the user driver to handle -EBUSY specially. On EBUSY, client driver has to rework on it. Nothing to be done from framework > > > + > > + if (polarity == PWM_POLARITY_INVERSE) > > + pwm_flags = PWM_POLARITY_INVERSE; > > + > > + if (!pwm_flags) > > + clear_bit(PWMF_POLARITY_INVERSE, &pwm->flags); > > + else > > + set_bit(PWMF_POLARITY_INVERSE, &pwm->flags); > > You can make this decision based on the value of polarity, no need for > the additional variable. Also as I mention below, maybe this flag isn't > all that useful. Ok I will correct it. > > > + > > + return pwm->chip->ops->set_polarity(pwm->chip, pwm, polarity); > > +} > > +EXPORT_SYMBOL_GPL(pwm_set_polarity); > > + > > +/** > > * pwm_enable() - start a PWM output toggling > > * @pwm: PWM device > > */ > > 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. > > Maybe you can put a comment above the enum, but I think if you name it > pwm_polarity, the meaning will be quite obvious. Ok I will correct it. > > > + > > +/* > > + * pwm_set_polarity - set polarity of PWM device > > + */ > > +int pwm_set_polarity(struct pwm_device *pwm, int polarity); > > + > > The enumeration and this prototype need to move inside the #ifdef > CONFIG_PWM block because they are not available in the legacy API. > Also as indicated above you should change the type of the polarity > parameter to "enum pwm_polarity". Ok I will update. > > > /* > > * pwm_enable - start a PWM output toggling > > */ > > @@ -37,6 +47,7 @@ struct pwm_chip; > > enum { > > PWMF_REQUESTED = 1 << 0, > > PWMF_ENABLED = 1 << 1, > > + PWMF_POLARITY_INVERSE = 1 << 2, > > This should be named PWMF_POLARITY_INVERSED for consistency. Ok I will correct it. > I'm not sure that we really need this flag, though. It isn't used anywhere. But > maybe you have a use-case in mind? It can be used to find the polarity of the PWM at runtime. > > > }; > > > > struct pwm_device { > > @@ -66,6 +77,7 @@ static inline unsigned int pwm_get_period(struct pwm_device *pwm) > > * @request: optional hook for requesting a PWM > > * @free: optional hook for freeing a PWM > > * @config: configure duty cycles and period length for this PWM > > + * @set_polarity: configure polarity of PWM > > "configure the polarity of this PWM" > > > * @enable: enable PWM output toggling > > * @disable: disable PWM output toggling > > * @dbg_show: optional routine to show contents in debugfs > > @@ -79,6 +91,9 @@ struct pwm_ops { > > int (*config)(struct pwm_chip *chip, > > struct pwm_device *pwm, > > int duty_ns, int period_ns); > > + int (*set_polarity)(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + int polarity); > > Make sure these line up properly. I will align with existing lines. Thanks Avinash > > Thierry > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
