[PATCH 0/2] improve pwm lookup support without device tree

[PATCH 0/2] improve pwm lookup support without device tree

[Alexandre Belloni]

Hi,

a small patch set as suggested byt Thierry to make lokkup with the lookup table
instead of device tree bahve more like when using device tree.

The first patch adds a period annd a polarity member to the lookup table and use
those to set period and polarity.

The second patch changes PWM_LOOKUP to set period an polarity. I was wondering
about adding a new macro to d that but the number of boards using it is limited
(only 3) so I guess it is ok to do that now.

The final goal would be to get rid of .pwm_period_ns in leds-pwm and pwm_bl.

Alexandre Belloni (2):
  pwm: add period and polarity to struct pwm_lookup
  pwm: use PWM_LOOKUP to set the period and polarity

 Documentation/pwm.txt                          | 3 ++-
 arch/arm/mach-omap2/board-omap3beagle.c        | 3 ++-
 arch/arm/mach-pxa/hx4700.c                     | 3 ++-
 arch/arm/mach-shmobile/board-armadillo800eva.c | 3 ++-
 drivers/pwm/core.c                             | 5 +++++
 include/linux/pwm.h                            | 6 +++++-
 6 files changed, 18 insertions(+), 5 deletions(-)

-- 
1.8.3.2

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

[Alexandre Belloni]

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);
 
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)	\
-- 
1.8.3.2

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

[Russell King - ARM Linux]

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?

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

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

[Alexandre Belloni]

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

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 2/2] pwm: use PWM_LOOKUP to set the period and polarity

[Alexandre Belloni]

Now that the PWM core is able to set the period and polarity based on
the lookup table, add those to PWM_LOOKUP to ease their usage.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 Documentation/pwm.txt                          | 3 ++-
 arch/arm/mach-omap2/board-omap3beagle.c        | 3 ++-
 arch/arm/mach-pxa/hx4700.c                     | 3 ++-
 arch/arm/mach-shmobile/board-armadillo800eva.c | 3 ++-
 include/linux/pwm.h                            | 4 +++-
 5 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
index 93cb97974986..f38f99cda64f 100644
--- a/Documentation/pwm.txt
+++ b/Documentation/pwm.txt
@@ -19,7 +19,8 @@ should instead register a static mapping that can be used to match PWM
 consumers to providers, as given in the following example:
 
 	static struct pwm_lookup board_pwm_lookup[] = {
-		PWM_LOOKUP("tegra-pwm", 0, "pwm-backlight", NULL),
+		PWM_LOOKUP("tegra-pwm", 0, "pwm-backlight", NULL,
+			   50000, PWM_POLARITY_NORMAL),
 	};
 
 	static void __init board_init(void)
diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
index d6ed819ff15c..54c135a5b4f7 100644
--- a/arch/arm/mach-omap2/board-omap3beagle.c
+++ b/arch/arm/mach-omap2/board-omap3beagle.c
@@ -61,7 +61,8 @@
 
 static struct pwm_lookup pwm_lookup[] = {
 	/* LEDB -> PMU_STAT */
-	PWM_LOOKUP("twl-pwmled", 1, "leds_pwm", "beagleboard::pmu_stat"),
+	PWM_LOOKUP("twl-pwmled", 1, "leds_pwm", "beagleboard::pmu_stat",
+		   7812500, PWM_POLARITY_NORMAL),
 };
 
 static struct led_pwm pwm_leds[] = {
diff --git a/arch/arm/mach-pxa/hx4700.c b/arch/arm/mach-pxa/hx4700.c
index a7c30eb0c8db..c66ad4edc5e3 100644
--- a/arch/arm/mach-pxa/hx4700.c
+++ b/arch/arm/mach-pxa/hx4700.c
@@ -574,7 +574,8 @@ static struct platform_device backlight = {
 };
 
 static struct pwm_lookup hx4700_pwm_lookup[] = {
-	PWM_LOOKUP("pxa27x-pwm.1", 0, "pwm-backlight", NULL),
+	PWM_LOOKUP("pxa27x-pwm.1", 0, "pwm-backlight", NULL,
+		   30923, PWM_POLARITY_NORMAL),
 };
 
 /*
diff --git a/arch/arm/mach-shmobile/board-armadillo800eva.c b/arch/arm/mach-shmobile/board-armadillo800eva.c
index 2858f380beae..e2c4c5010f19 100644
--- a/arch/arm/mach-shmobile/board-armadillo800eva.c
+++ b/arch/arm/mach-shmobile/board-armadillo800eva.c
@@ -416,7 +416,8 @@ static struct platform_device pwm_device = {
 };
 
 static struct pwm_lookup pwm_lookup[] = {
-	PWM_LOOKUP("renesas-tpu-pwm", 2, "pwm-backlight.0", NULL),
+	PWM_LOOKUP("renesas-tpu-pwm", 2, "pwm-backlight.0", NULL,
+		   33333, PWM_POLARITY_NORMAL),
 };
 
 /* LCDC and backlight */
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 2f45e2fe5b93..e90628cac8fa 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -278,12 +278,14 @@ struct pwm_lookup {
 	enum pwm_polarity polarity;
 };
 
-#define PWM_LOOKUP(_provider, _index, _dev_id, _con_id)	\
+#define PWM_LOOKUP(_provider, _index, _dev_id, _con_id, _period, _polarity) \
 	{						\
 		.provider = _provider,			\
 		.index = _index,			\
 		.dev_id = _dev_id,			\
 		.con_id = _con_id,			\
+		.period = _period,			\
+		.polarity = _polarity			\
 	}
 
 #if IS_ENABLED(CONFIG_PWM)
-- 
1.8.3.2

[PATCH 2/2] pwm: use PWM_LOOKUP to set the period and polarity

[Simon Horman]

On Wed, Apr 09, 2014 at 08:04:09PM +0200, Alexandre Belloni wrote:
> Now that the PWM core is able to set the period and polarity based on
> the lookup table, add those to PWM_LOOKUP to ease their usage.

I would prefer if this change was made in a non-atomic manner.

1. Add new infrastructure
2. Update users individually
3. Remove old infrastructure

> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>  Documentation/pwm.txt                          | 3 ++-
>  arch/arm/mach-omap2/board-omap3beagle.c        | 3 ++-
>  arch/arm/mach-pxa/hx4700.c                     | 3 ++-
>  arch/arm/mach-shmobile/board-armadillo800eva.c | 3 ++-
>  include/linux/pwm.h                            | 4 +++-
>  5 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
> index 93cb97974986..f38f99cda64f 100644
> --- a/Documentation/pwm.txt
> +++ b/Documentation/pwm.txt
> @@ -19,7 +19,8 @@ should instead register a static mapping that can be used to match PWM
>  consumers to providers, as given in the following example:
>  
>  	static struct pwm_lookup board_pwm_lookup[] = {
> -		PWM_LOOKUP("tegra-pwm", 0, "pwm-backlight", NULL),
> +		PWM_LOOKUP("tegra-pwm", 0, "pwm-backlight", NULL,
> +			   50000, PWM_POLARITY_NORMAL),
>  	};
>  
>  	static void __init board_init(void)
> diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
> index d6ed819ff15c..54c135a5b4f7 100644
> --- a/arch/arm/mach-omap2/board-omap3beagle.c
> +++ b/arch/arm/mach-omap2/board-omap3beagle.c
> @@ -61,7 +61,8 @@
>  
>  static struct pwm_lookup pwm_lookup[] = {
>  	/* LEDB -> PMU_STAT */
> -	PWM_LOOKUP("twl-pwmled", 1, "leds_pwm", "beagleboard::pmu_stat"),
> +	PWM_LOOKUP("twl-pwmled", 1, "leds_pwm", "beagleboard::pmu_stat",
> +		   7812500, PWM_POLARITY_NORMAL),
>  };
>  
>  static struct led_pwm pwm_leds[] = {
> diff --git a/arch/arm/mach-pxa/hx4700.c b/arch/arm/mach-pxa/hx4700.c
> index a7c30eb0c8db..c66ad4edc5e3 100644
> --- a/arch/arm/mach-pxa/hx4700.c
> +++ b/arch/arm/mach-pxa/hx4700.c
> @@ -574,7 +574,8 @@ static struct platform_device backlight = {
>  };
>  
>  static struct pwm_lookup hx4700_pwm_lookup[] = {
> -	PWM_LOOKUP("pxa27x-pwm.1", 0, "pwm-backlight", NULL),
> +	PWM_LOOKUP("pxa27x-pwm.1", 0, "pwm-backlight", NULL,
> +		   30923, PWM_POLARITY_NORMAL),
>  };
>  
>  /*
> diff --git a/arch/arm/mach-shmobile/board-armadillo800eva.c b/arch/arm/mach-shmobile/board-armadillo800eva.c
> index 2858f380beae..e2c4c5010f19 100644
> --- a/arch/arm/mach-shmobile/board-armadillo800eva.c
> +++ b/arch/arm/mach-shmobile/board-armadillo800eva.c
> @@ -416,7 +416,8 @@ static struct platform_device pwm_device = {
>  };
>  
>  static struct pwm_lookup pwm_lookup[] = {
> -	PWM_LOOKUP("renesas-tpu-pwm", 2, "pwm-backlight.0", NULL),
> +	PWM_LOOKUP("renesas-tpu-pwm", 2, "pwm-backlight.0", NULL,
> +		   33333, PWM_POLARITY_NORMAL),
>  };
>  
>  /* LCDC and backlight */
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 2f45e2fe5b93..e90628cac8fa 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -278,12 +278,14 @@ struct pwm_lookup {
>  	enum pwm_polarity polarity;
>  };
>  
> -#define PWM_LOOKUP(_provider, _index, _dev_id, _con_id)	\
> +#define PWM_LOOKUP(_provider, _index, _dev_id, _con_id, _period, _polarity) \
>  	{						\
>  		.provider = _provider,			\
>  		.index = _index,			\
>  		.dev_id = _dev_id,			\
>  		.con_id = _con_id,			\
> +		.period = _period,			\
> +		.polarity = _polarity			\
>  	}
>  
>  #if IS_ENABLED(CONFIG_PWM)
> -- 
> 1.8.3.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 2/2] pwm: use PWM_LOOKUP to set the period and polarity

[Alexandre Belloni]

On 10/04/2014 at 08:15:49 +0900, Simon Horman wrote :
> On Wed, Apr 09, 2014 at 08:04:09PM +0200, Alexandre Belloni wrote:
> > Now that the PWM core is able to set the period and polarity based on
> > the lookup table, add those to PWM_LOOKUP to ease their usage.
> 
> I would prefer if this change was made in a non-atomic manner.
> 
> 1. Add new infrastructure
> 2. Update users individually
> 3. Remove old infrastructure
> 

I agree this would be better but I'm not sure how you can modify a macro
without renaming it or changing it everywhere at once. Like said, I'm
open to creating a new macro.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 2/2] pwm: use PWM_LOOKUP to set the period and polarity

[Simon Horman]

On Thu, Apr 10, 2014 at 09:37:03AM +0200, Alexandre Belloni wrote:
> On 10/04/2014 at 08:15:49 +0900, Simon Horman wrote :
> > On Wed, Apr 09, 2014 at 08:04:09PM +0200, Alexandre Belloni wrote:
> > > Now that the PWM core is able to set the period and polarity based on
> > > the lookup table, add those to PWM_LOOKUP to ease their usage.
> > 
> > I would prefer if this change was made in a non-atomic manner.
> > 
> > 1. Add new infrastructure
> > 2. Update users individually
> > 3. Remove old infrastructure
> > 
> 
> I agree this would be better but I'm not sure how you can modify a macro
> without renaming it or changing it everywhere at once. Like said, I'm
> open to creating a new macro.

I for one would prefer the new macro approach.