diff mbox

[1/2] pwm: add period and polarity to struct pwm_lookup

Message ID 1397066649-3767-2-git-send-email-alexandre.belloni@free-electrons.com
State Superseded
Headers show

Commit Message

Alexandre Belloni April 9, 2014, 6:04 p.m. UTC
Adds a period and a polarity member to struct pwm_lookup so that when performing
a lookup using the lookup table instead of device tree, we are able to set the
period and the polarity accordingly like what is done in
of_pwm_xlate_with_flags.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/pwm/core.c  | 5 +++++
 include/linux/pwm.h | 2 ++
 2 files changed, 7 insertions(+)

Comments

Russell King - ARM Linux April 9, 2014, 7:37 p.m. UTC | #1
On Wed, Apr 09, 2014 at 08:04:08PM +0200, Alexandre Belloni wrote:
> Adds a period and a polarity member to struct pwm_lookup so that when performing
> a lookup using the lookup table instead of device tree, we are able to set the
> period and the polarity accordingly like what is done in
> of_pwm_xlate_with_flags.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>  drivers/pwm/core.c  | 5 +++++
>  include/linux/pwm.h | 2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index a80471399c20..206e5996359c 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -663,6 +663,11 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
>  
>  	if (chip)
>  		pwm = pwm_request_from_chip(chip, index, con_id ?: dev_id);
> +	if (IS_ERR(pwm))
> +		return pwm;
> +
> +	pwm_set_period(pwm, p->period);
> +	pwm_set_polarity(pwm, p->polarity);
>  
>  	mutex_unlock(&pwm_lookup_lock);

Clearly, this is not right.  Returning while leaving the mutex locked?
No.

The second issue is... with _just_ this patch applied, we end up with
"period" and "polarity" presumably initialised to zero, which means we
now end up with the above explicitly setting the period and polarity as
such.  Isn't that going to change the behaviour of this?
Alexandre Belloni April 9, 2014, 8:52 p.m. UTC | #2
On 09/04/2014 at 20:37:06 +0100, Russell King - ARM Linux wrote :
> On Wed, Apr 09, 2014 at 08:04:08PM +0200, Alexandre Belloni wrote:
> > Adds a period and a polarity member to struct pwm_lookup so that when performing
> > a lookup using the lookup table instead of device tree, we are able to set the
> > period and the polarity accordingly like what is done in
> > of_pwm_xlate_with_flags.
> > 
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > ---
> >  drivers/pwm/core.c  | 5 +++++
> >  include/linux/pwm.h | 2 ++
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index a80471399c20..206e5996359c 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -663,6 +663,11 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
> >  
> >  	if (chip)
> >  		pwm = pwm_request_from_chip(chip, index, con_id ?: dev_id);
> > +	if (IS_ERR(pwm))
> > +		return pwm;
> > +
> > +	pwm_set_period(pwm, p->period);
> > +	pwm_set_polarity(pwm, p->polarity);
> >  
> >  	mutex_unlock(&pwm_lookup_lock);
> 
> Clearly, this is not right.  Returning while leaving the mutex locked?
> No.
> 

Sure, I will fix that crap, sorry about that and thanks for pointing it
out.

> The second issue is... with _just_ this patch applied, we end up with
> "period" and "polarity" presumably initialised to zero, which means we
> now end up with the above explicitly setting the period and polarity as
> such.  Isn't that going to change the behaviour of this?
> 

I actually checked that.

For the polarity, for now, it is assumed that it is normal unless
specified otherwise.
The only driver that was supporting inverting it using platform_data is
pwm-renesas-tpu. It is used by board-armadillo800eva.c that I am
modifying now (and I just now realise that I forgot to invert it).

The only PWM controller that I know of that by default has its polarity
inversed is the allwinner one and in the driver I submitted, I actually
switch it to normal polarity in the probe instead of e.g. doing
pwm->polarity = PWM_POLARITY_INVERSED;

For the period, all the driver are assuming 0 after initialization.

I think this is not specified. If you think that may be a concern then I
suggest creating another macro and using a bitfield to know which value
is set.

I would also argue that when using device tree,
of_pwm_xlate_with_flags() will set the period and the polarity
unconditionally, this is replicating that behaviour.

However, I could agree that we may need to test for
pwm->chip->ops->set_polarity before calling pwm_set_polarity as we will
get an error if it is NULL (but we actually discard that return value).
diff mbox

Patch

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index a80471399c20..206e5996359c 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -663,6 +663,11 @@  struct pwm_device *pwm_get(struct device *dev, const char *con_id)
 
 	if (chip)
 		pwm = pwm_request_from_chip(chip, index, con_id ?: dev_id);
+	if (IS_ERR(pwm))
+		return pwm;
+
+	pwm_set_period(pwm, p->period);
+	pwm_set_polarity(pwm, p->polarity);
 
 	mutex_unlock(&pwm_lookup_lock);
 
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 4717f54051cb..2f45e2fe5b93 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -274,6 +274,8 @@  struct pwm_lookup {
 	unsigned int index;
 	const char *dev_id;
 	const char *con_id;
+	unsigned int period;
+	enum pwm_polarity polarity;
 };
 
 #define PWM_LOOKUP(_provider, _index, _dev_id, _con_id)	\