Message-ID: <518397C60809E147AF5323E0420B992E3E97189D@DBDE01.ent.ti.com>
Date:	Mon, 23 Jul 2012 12:51:11 +0000
From:	"Philip, Avinash" <avinashphilip@...com>
To:	Thierry Reding <thierry.reding@...onic-design.de>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
	"Nori, Sekhar" <nsekhar@...com>,
	"Hebbar, Gururaja" <gururaja.hebbar@...com>
Subject: RE: [PATCH] PWM: Add support for configuring polarity of PWM

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@...com>
> > ---
> > 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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists