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

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

[Philip, Avinash]

Duty cycle inversion of PWM wave should achieved through PWM polarity
inversion. Also polarity of PWM wave should configurable from slave
drivers,

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.

This patch adds support for configuring PWM polarity in PWM frame work.

Signed-off-by: Philip, Avinash <avinashphilip@ti.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.

Discussions related to this can found at
http://www.spinics.net/lists/kernel/msg1372110.html

: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
+ * @pwm: PWM device
+ * @polarity: Configure polarity of PWM
+ *
+ * Polarity to be configured with PWM device disabled.
+ */
+int pwm_set_polarity(struct pwm_device *pwm, int polarity)
+{
+	int pwm_flags = PWM_POLARITY_NORMAL;
+
+	if (!pwm || !pwm->chip->ops->set_polarity)
+		return -EINVAL;
+
+	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
+		dev_err(pwm->chip->dev,
+			"Polarity configuration Failed!, PWM device enabled\n");
+		return -EBUSY;
+	}
+
+	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);
+
+	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 */
+};
+
+/*
+ * pwm_set_polarity - set polarity of PWM device
+ */
+int pwm_set_polarity(struct pwm_device *pwm, int polarity);
+
 /*
  * 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,
 };
 
 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
  * @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);
 	int			(*enable)(struct pwm_chip *chip,
 					  struct pwm_device *pwm);
 	void			(*disable)(struct pwm_chip *chip,
-- 
1.7.1

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

[Thierry Reding]

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

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.

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

> 
> Signed-off-by: Philip, Avinash <avinashphilip@ti.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.

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

> + */
> +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;

> +
> +	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"? Though I'm not sure the error message is all that useful. I'd
expect the user driver to handle -EBUSY specially.

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

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

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

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

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

Thierry

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

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

[Philip, Avinash]

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

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

[Thierry Reding]

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

On Mon, Jul 23, 2012 at 12:51:11PM +0000, Philip, Avinash wrote:
> 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:
[...]
> > > +
> > > +	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

Exactly, so I think that if an error is displayed because the PWM has
been enabled, then that client (== user) driver should output an error
message, not the framework. Also, it really shouldn't happen because it
clearly is a driver problem that needs to be fixed.

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

Yes, but is there any use-case where this information would be required?

Thierry

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

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

[Philip, Avinash]

On Mon, Jul 23, 2012 at 18:38:25, Thierry Reding wrote:
> On Mon, Jul 23, 2012 at 12:51:11PM +0000, Philip, Avinash wrote:
> > 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:
> [...]
> > > > +
> > > > +	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
> 
> Exactly, so I think that if an error is displayed because the PWM has
> been enabled, then that client (== user) driver should output an error
> message, not the framework. Also, it really shouldn't happen because it
> clearly is a driver problem that needs to be fixed.

Ok. I will remove.

> 
> > > >  /*
> > > >   * 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.
> 
> Yes, but is there any use-case where this information would be required?

It's been added as a feature enhancement. May be it can ignore?


Thanks
Avinash

> 
> Thierry
> 

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

[Thierry Reding]

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

On Mon, Jul 23, 2012 at 03:05:26PM +0000, Philip, Avinash wrote:
> On Mon, Jul 23, 2012 at 18:38:25, Thierry Reding wrote:
> > On Mon, Jul 23, 2012 at 12:51:11PM +0000, Philip, Avinash wrote:
> > > 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:
> > > > >  /*
> > > > >   * 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.
> > 
> > Yes, but is there any use-case where this information would be required?
> 
> It's been added as a feature enhancement. May be it can ignore?

I think it can be removed for now. It can be added back if we ever
really need it.

Thierry

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

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

[Philip, Avinash]

On Tue, Jul 24, 2012 at 01:22:49, Thierry Reding wrote:
> On Mon, Jul 23, 2012 at 03:05:26PM +0000, Philip, Avinash wrote:
> > On Mon, Jul 23, 2012 at 18:38:25, Thierry Reding wrote:
> > > On Mon, Jul 23, 2012 at 12:51:11PM +0000, Philip, Avinash wrote:
> > > > 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:
> > > > > >  /*
> > > > > >   * 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.
> > > 
> > > Yes, but is there any use-case where this information would be required?
> > 
> > It's been added as a feature enhancement. May be it can ignore?
> 
> I think it can be removed for now. It can be added back if we ever
> really need it.

Ok. I will remove it.

Thanks
Avinash

> 
> Thierry
> 

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

[Lars-Peter Clausen]

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

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.

- Lars

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

[Thierry Reding]

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

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 #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Lars-Peter Clausen]

On 07/24/2012 08:51 AM, Thierry Reding wrote:
> 
> 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,
> };
> 

Looks fine :)

Thanks,
- Lars

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

[Thierry Reding]

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

On Tue, Jul 24, 2012 at 10:25:17AM +0200, Lars-Peter Clausen wrote:
> On 07/24/2012 08:51 AM, Thierry Reding wrote:
> > 
> > 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,
> > };
> > 
> 
> Looks fine :)

Philip: can you fold this into your patch?

Thierry

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

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

[Philip, Avinash]

On Tue, Jul 24, 2012 at 13:56:24, Thierry Reding wrote:
> On Tue, Jul 24, 2012 at 10:25:17AM +0200, Lars-Peter Clausen wrote:
> > On 07/24/2012 08:51 AM, Thierry Reding wrote:
> > > 
> > > 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,
> > > };
> > > 
> > 
> > Looks fine :)
> 
> Philip: can you fold this into your patch?

Sure. I will add.

Thanks
Avinash

> 
> Thierry
>