[PATCH v2 0/3] pwm: lpc32xx: Add support to control PWM_PIN_LEVEL bit

[PATCH v2 0/3] pwm: lpc32xx: Add support to control PWM_PIN_LEVEL bit

[Sylvain Lemieux]

From: Sylvain Lemieux <slemieux@tycoint.com>

The PWM_PIN_LEVEL bit is leave unset by the kernel PWM driver.

Prior to commit 08ee77b5a5de27ad63c92262ebcb4efe0da93b58,
the PWM_PIN_LEVEL bit was always clear when the PWM was disable
and a 0 logic level was apply to the output.

According to the LPC32x0 User Manual [1],
the default value for bit 30 (PWM_PIN_LEVEL) is 0.

First patch:
* initialize the pin level to 0 (default value) and update
  the register value accordingly.

Second anf third patches:
*  provide support to configure the pin output (i.e. PWM_PIN_LEVEL bit)
   when the PWM is disabled.

Note:
* Follow this URL to access the discussion for version 1 of this
  patch: http://thread.gmane.org/gmane.linux.pwm/3882

[1] http://www.nxp.com/documents/user_manual/UM10326.pdf

Sylvain Lemieux (3):
  pwm: lpc32xx: Set PWM_PIN_LEVEL bit to default value
  pwm: lpc32xx: Add support for PWM_PIN_LEVEL bit configuration
  dt-bindings: pwm: lpc32xx: Add nxp,pwm-disabled-level-high property

 Documentation/devicetree/bindings/pwm/lpc32xx-pwm.txt |  3 +++
 drivers/pwm/pwm-lpc32xx.c                             | 11 +++++++++++
 2 files changed, 14 insertions(+)

-- 
1.8.3.1

[PATCH v2 1/3] pwm: lpc32xx: Set PWM_PIN_LEVEL bit to default value

[Sylvain Lemieux]

From: Sylvain Lemieux <slemieux@tycoint.com>

The PWM_PIN_LEVEL bit is leave unset by the kernel PWM driver.

Prior to commit 08ee77b5a5de27ad63c92262ebcb4efe0da93b58,
the PWM_PIN_LEVEL bit was always clear when the PWM was disable
and a 0 logic level was apply to the output.

According to the LPC32x0 User Manual [1],
the default value for bit 30 (PWM_PIN_LEVEL) is 0.

This change initialize the pin level to 0 (default value) and
update the register value accordingly.

[1] http://www.nxp.com/documents/user_manual/UM10326.pdf

Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com>
---
Changes from v1 to v2:
* Only setup the "PWMx_PIN_LEVEL" once on probe.

 drivers/pwm/pwm-lpc32xx.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/pwm/pwm-lpc32xx.c b/drivers/pwm/pwm-lpc32xx.c
index 4d470c1..a9b3cff 100644
--- a/drivers/pwm/pwm-lpc32xx.c
+++ b/drivers/pwm/pwm-lpc32xx.c
@@ -25,6 +25,7 @@ struct lpc32xx_pwm_chip {
 };
 
 #define PWM_ENABLE	BIT(31)
+#define PWM_PIN_LEVEL	BIT(30)
 
 #define to_lpc32xx_pwm_chip(_chip) \
 	container_of(_chip, struct lpc32xx_pwm_chip, chip)
@@ -103,6 +104,7 @@ static int lpc32xx_pwm_probe(struct platform_device *pdev)
 	struct lpc32xx_pwm_chip *lpc32xx;
 	struct resource *res;
 	int ret;
+	u32 val;
 
 	lpc32xx = devm_kzalloc(&pdev->dev, sizeof(*lpc32xx), GFP_KERNEL);
 	if (!lpc32xx)
@@ -128,6 +130,11 @@ static int lpc32xx_pwm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	/* When PWM is disable, configure the output to the default value */
+	val = readl(lpc32xx->base + (lpc32xx->chip.pwms[0].hwpwm << 2));
+	val &= ~PWM_PIN_LEVEL;
+	writel(val, lpc32xx->base + (lpc32xx->chip.pwms[0].hwpwm << 2));
+
 	platform_set_drvdata(pdev, lpc32xx);
 
 	return 0;
-- 
1.8.3.1

Re: [PATCH v2 1/3] pwm: lpc32xx: Set PWM_PIN_LEVEL bit to default value

[Thierry Reding]

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

On Mon, Jun 27, 2016 at 09:09:55AM -0400, Sylvain Lemieux wrote:
> From: Sylvain Lemieux <slemieux@tycoint.com>
> 
> The PWM_PIN_LEVEL bit is leave unset by the kernel PWM driver.
> 
> Prior to commit 08ee77b5a5de27ad63c92262ebcb4efe0da93b58,
> the PWM_PIN_LEVEL bit was always clear when the PWM was disable
> and a 0 logic level was apply to the output.
> 
> According to the LPC32x0 User Manual [1],
> the default value for bit 30 (PWM_PIN_LEVEL) is 0.
> 
> This change initialize the pin level to 0 (default value) and
> update the register value accordingly.
> 
> [1] http://www.nxp.com/documents/user_manual/UM10326.pdf
> 
> Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com>
> ---
> Changes from v1 to v2:
> * Only setup the "PWMx_PIN_LEVEL" once on probe.
> 
>  drivers/pwm/pwm-lpc32xx.c | 7 +++++++
>  1 file changed, 7 insertions(+)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v2 2/3] pwm: lpc32xx: Add support for PWM_PIN_LEVEL bit configuration

[Sylvain Lemieux]

From: Sylvain Lemieux <slemieux@tycoint.com>

Provide support to configure the pin output
(i.e. PWM_PIN_LEVEL bit) when the PWM is disabled.

Note:
When the LPC32xx PWM is disabled, both options of pin
output high and low level values are valid; however this
particular controller does not have polarity control.

Refer to the LPC32x0 User Manual [1], for details.

[1] http://www.nxp.com/documents/user_manual/UM10326.pdf

Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com>
---
Changes from v1 to v2:
* New patch in version 2.

 drivers/pwm/pwm-lpc32xx.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-lpc32xx.c b/drivers/pwm/pwm-lpc32xx.c
index a9b3cff..d1bd901 100644
--- a/drivers/pwm/pwm-lpc32xx.c
+++ b/drivers/pwm/pwm-lpc32xx.c
@@ -130,9 +130,13 @@ static int lpc32xx_pwm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	/* When PWM is disable, configure the output to the default value */
+	/* When PWM is disable, configure the output correctly */
 	val = readl(lpc32xx->base + (lpc32xx->chip.pwms[0].hwpwm << 2));
-	val &= ~PWM_PIN_LEVEL;
+	if (device_property_read_bool(&pdev->dev,
+				      "nxp,pwm-disabled-level-high"))
+		val |= PWM_PIN_LEVEL;
+	else
+		val &= ~PWM_PIN_LEVEL;
 	writel(val, lpc32xx->base + (lpc32xx->chip.pwms[0].hwpwm << 2));
 
 	platform_set_drvdata(pdev, lpc32xx);
-- 
1.8.3.1

[PATCH v2 3/3] dt-bindings: pwm: lpc32xx: Add nxp,pwm-disabled-level-high property

[Sylvain Lemieux]

From: Sylvain Lemieux <slemieux@tycoint.com>

Document the nxp,pwm-disabled-level-high property used by the
pwm-lpc32xx driver to setup the pin output level to high when
the PWM is disabled.

The driver setup the pin output level to the reset value
by default.

Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com>
---
Changes from v1 to v2:
* New patch in version 2.

 Documentation/devicetree/bindings/pwm/lpc32xx-pwm.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/lpc32xx-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc32xx-pwm.txt
index 74b5bc5..5829f3f 100644
--- a/Documentation/devicetree/bindings/pwm/lpc32xx-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/lpc32xx-pwm.txt
@@ -4,6 +4,9 @@ Required properties:
 - compatible: should be "nxp,lpc3220-pwm"
 - reg: physical base address and length of the controller's registers
 
+Optional properties:
+- nxp,pwm-disabled-level-high: Set the PWM output level to high when disabled
+
 Examples:
 
 pwm@4005c000 {
-- 
1.8.3.1

Re: [PATCH v2 0/3] pwm: lpc32xx: Add support to control PWM_PIN_LEVEL bit

[Vladimir Zapolskiy]

Hi Sylvain,

On 27.06.2016 16:09, Sylvain Lemieux wrote:
> From: Sylvain Lemieux <slemieux@tycoint.com>
> 
> The PWM_PIN_LEVEL bit is leave unset by the kernel PWM driver.
> 
> Prior to commit 08ee77b5a5de27ad63c92262ebcb4efe0da93b58,
> the PWM_PIN_LEVEL bit was always clear when the PWM was disable
> and a 0 logic level was apply to the output.
> 
> According to the LPC32x0 User Manual [1],
> the default value for bit 30 (PWM_PIN_LEVEL) is 0.
> 
> First patch:
> * initialize the pin level to 0 (default value) and update
>   the register value accordingly.
> 
> Second anf third patches:
> *  provide support to configure the pin output (i.e. PWM_PIN_LEVEL bit)
>    when the PWM is disabled.

a short question, how do you use PWM? Does it serve as a backlight
or something else?

> Note:
> * Follow this URL to access the discussion for version 1 of this
>   patch: http://thread.gmane.org/gmane.linux.pwm/3882
> 
> [1] http://www.nxp.com/documents/user_manual/UM10326.pdf
> 
> Sylvain Lemieux (3):
>   pwm: lpc32xx: Set PWM_PIN_LEVEL bit to default value
>   pwm: lpc32xx: Add support for PWM_PIN_LEVEL bit configuration
>   dt-bindings: pwm: lpc32xx: Add nxp,pwm-disabled-level-high property
> 
>  Documentation/devicetree/bindings/pwm/lpc32xx-pwm.txt |  3 +++
>  drivers/pwm/pwm-lpc32xx.c                             | 11 +++++++++++
>  2 files changed, 14 insertions(+)
> 

--
With best wishes,
Vladimir

Re: [PATCH v2 0/3] pwm: lpc32xx: Add support to control PWM_PIN_LEVEL bit

[Sylvain Lemieux]

Hi Vladimir,

On Thu, 2016-06-30 at 11:43 +0300, Vladimir Zapolskiy wrote:
> Hi Sylvain,
> 
> On 27.06.2016 16:09, Sylvain Lemieux wrote:
> > From: Sylvain Lemieux <slemieux@tycoint.com>
> > 
> > The PWM_PIN_LEVEL bit is leave unset by the kernel PWM driver.
> > 
> > Prior to commit 08ee77b5a5de27ad63c92262ebcb4efe0da93b58,
> > the PWM_PIN_LEVEL bit was always clear when the PWM was disable
> > and a 0 logic level was apply to the output.
> > 
> > According to the LPC32x0 User Manual [1],
> > the default value for bit 30 (PWM_PIN_LEVEL) is 0.
> > 
> > First patch:
> > * initialize the pin level to 0 (default value) and update
> >   the register value accordingly.
> > 
> > Second anf third patches:
> > *  provide support to configure the pin output (i.e. PWM_PIN_LEVEL bit)
> >    when the PWM is disabled.
> 
> a short question, how do you use PWM? Does it serve as a backlight
> or something else?
> 
The PWM is used to control the LCD backlight intensity.

When we setup the backlight brightness level to 0, 
it should turn off the LCD display by disabling the PWM.

This is what was added to our board dts for it:
	backlight {
		compatible = "pwm-backlight";
		pwms = <&pwm1 0 1250000>;

		brightness-levels = <0 20 100>;
		default-brightness-level = <2>;
	};

> > Note:
> > * Follow this URL to access the discussion for version 1 of this
> >   patch: http://thread.gmane.org/gmane.linux.pwm/3882
> > 
> > [1] http://www.nxp.com/documents/user_manual/UM10326.pdf
> > 
> > Sylvain Lemieux (3):
> >   pwm: lpc32xx: Set PWM_PIN_LEVEL bit to default value
> >   pwm: lpc32xx: Add support for PWM_PIN_LEVEL bit configuration
> >   dt-bindings: pwm: lpc32xx: Add nxp,pwm-disabled-level-high property
> > 
> >  Documentation/devicetree/bindings/pwm/lpc32xx-pwm.txt |  3 +++
> >  drivers/pwm/pwm-lpc32xx.c                             | 11 +++++++++++
> >  2 files changed, 14 insertions(+)
> > 
> 
> --
> With best wishes,
> Vladimir

Sylvain

Re: [PATCH v2 0/3] pwm: lpc32xx: Add support to control PWM_PIN_LEVEL bit

[Vladimir Zapolskiy]

Hi Sylvain,

On 30.06.2016 16:25, Sylvain Lemieux wrote:
> Hi Vladimir,
> 
> On Thu, 2016-06-30 at 11:43 +0300, Vladimir Zapolskiy wrote:
>> Hi Sylvain,
>>
>> On 27.06.2016 16:09, Sylvain Lemieux wrote:
>>> From: Sylvain Lemieux <slemieux@tycoint.com>
>>>
>>> The PWM_PIN_LEVEL bit is leave unset by the kernel PWM driver.
>>>
>>> Prior to commit 08ee77b5a5de27ad63c92262ebcb4efe0da93b58,
>>> the PWM_PIN_LEVEL bit was always clear when the PWM was disable
>>> and a 0 logic level was apply to the output.
>>>
>>> According to the LPC32x0 User Manual [1],
>>> the default value for bit 30 (PWM_PIN_LEVEL) is 0.
>>>
>>> First patch:
>>> * initialize the pin level to 0 (default value) and update
>>>   the register value accordingly.
>>>
>>> Second anf third patches:
>>> *  provide support to configure the pin output (i.e. PWM_PIN_LEVEL bit)
>>>    when the PWM is disabled.
>>
>> a short question, how do you use PWM? Does it serve as a backlight
>> or something else?
>>
> The PWM is used to control the LCD backlight intensity.
> 
> When we setup the backlight brightness level to 0, 
> it should turn off the LCD display by disabling the PWM.
> 
> This is what was added to our board dts for it:
> 	backlight {
> 		compatible = "pwm-backlight";
> 		pwms = <&pwm1 0 1250000>;
> 
> 		brightness-levels = <0 20 100>;
> 		default-brightness-level = <2>;
> 	};
> 

this is good that there is a consumer and we are not talking in
general, then it might be more visual that the new property probably
is not needed due to the lack of use cases.

I think that we should start from something as simple as one-time
setting of PWMx_PIN_LEVEL to default 0 in lpc32xx_pwm_probe() and then
implement a proper support of boundary values as suggested in discussion
of v1 series.

>>> Note:
>>> * Follow this URL to access the discussion for version 1 of this
>>>   patch: http://thread.gmane.org/gmane.linux.pwm/3882

--
With best wishes,
Vladimir

Re: [PATCH v2 0/3] pwm: lpc32xx: Add support to control PWM_PIN_LEVEL bit

[Sylvain Lemieux]

Hi Thierry,

On Fri, 2016-07-01 at 11:13 +0300, Vladimir Zapolskiy wrote:
> Hi Sylvain,
> 
> On 30.06.2016 16:25, Sylvain Lemieux wrote:
> > Hi Vladimir,
> > 
> > On Thu, 2016-06-30 at 11:43 +0300, Vladimir Zapolskiy wrote:
> >> Hi Sylvain,
> >>
> >> On 27.06.2016 16:09, Sylvain Lemieux wrote:
> >>> From: Sylvain Lemieux <slemieux@tycoint.com>
> >>>
> >>> The PWM_PIN_LEVEL bit is leave unset by the kernel PWM driver.
> >>>
> >>> Prior to commit 08ee77b5a5de27ad63c92262ebcb4efe0da93b58,
> >>> the PWM_PIN_LEVEL bit was always clear when the PWM was disable
> >>> and a 0 logic level was apply to the output.
> >>>
> >>> According to the LPC32x0 User Manual [1],
> >>> the default value for bit 30 (PWM_PIN_LEVEL) is 0.
> >>>
> >>> First patch:
> >>> * initialize the pin level to 0 (default value) and update
> >>>   the register value accordingly.
> >>>
> >>> Second anf third patches:
> >>> *  provide support to configure the pin output (i.e. PWM_PIN_LEVEL bit)
> >>>    when the PWM is disabled.
> >>
> >> a short question, how do you use PWM? Does it serve as a backlight
> >> or something else?
> >>
> > The PWM is used to control the LCD backlight intensity.
> > 
> > When we setup the backlight brightness level to 0, 
> > it should turn off the LCD display by disabling the PWM.
> > 
> > This is what was added to our board dts for it:
> > 	backlight {
> > 		compatible = "pwm-backlight";
> > 		pwms = <&pwm1 0 1250000>;
> > 
> > 		brightness-levels = <0 20 100>;
> > 		default-brightness-level = <2>;
> > 	};
> > 
> 
> this is good that there is a consumer and we are not talking in
> general, then it might be more visual that the new property probably
> is not needed due to the lack of use cases.
> 
> I think that we should start from something as simple as one-time
> setting of PWMx_PIN_LEVEL to default 0 in lpc32xx_pwm_probe() and then
> implement a proper support of boundary values as suggested in discussion
> of v1 series.
> 
Can you take the patch 1 of this series? This resolved an issue with 
the LPC32xx PWM when use to control the LCD backlight brightness level.

As suggested, we can later discuss the best way to 
implement proper support of boundary values.
 
> >>> Note:
> >>> * Follow this URL to access the discussion for version 1 of this
> >>>   patch: http://thread.gmane.org/gmane.linux.pwm/3882
> 
> --
> With best wishes,
> Vladimir