[PATCH v2 0/6] PWM JZ4740 fixes and cleanups

[PATCH v2 0/6] PWM JZ4740 fixes and cleanups

[Paul Cercueil]

Hi Thierry,

I rebased my patchset on top of v5.2-rc3 since it didn't apply anymore.
Patches 1-4 are unmodified. The old patch 5 added a SPDX license
notifier, but somebody else added it in -rc3 apparently, so I dropped
it. In the meantime I added two more cleanups.

Cheers
-Paul

[PATCH v2 1/6] dt-bindings: Remove unused compatible strings

[Paul Cercueil]

Right now none of the Ingenic-based boards probe this driver from
devicetree. This driver defined three compatible strings for the exact
same behaviour. Before these strings are used, we can remove two of
them.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v2: Rebase on v5.2-rc3

 Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
index 7d9d3f90641b..493bec80d59b 100644
--- a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
@@ -2,10 +2,7 @@ Ingenic JZ47xx PWM Controller
 =============================
 
 Required properties:
-- compatible: One of:
-  * "ingenic,jz4740-pwm"
-  * "ingenic,jz4770-pwm"
-  * "ingenic,jz4780-pwm"
+- compatible: Should be "ingenic,jz4740-pwm"
 - #pwm-cells: Should be 3. See pwm.txt in this directory for a description
   of the cells format.
 - clocks : phandle to the external clock.
-- 
2.21.0.593.g511ec345e18

[PATCH v2 2/6] pwm: jz4740: Remove unused devicetree compatible strings

[Paul Cercueil]

Right now none of the Ingenic-based boards probe this driver from
devicetree. This driver defined three compatible strings for the exact
same behaviour. Before these strings are used, we can remove two of
them.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v2: Rebase on v5.2-rc3

 drivers/pwm/pwm-jz4740.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index 88a51a40e695..5c8bb6192201 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -184,8 +184,6 @@ static int jz4740_pwm_remove(struct platform_device *pdev)
 #ifdef CONFIG_OF
 static const struct of_device_id jz4740_pwm_dt_ids[] = {
 	{ .compatible = "ingenic,jz4740-pwm", },
-	{ .compatible = "ingenic,jz4770-pwm", },
-	{ .compatible = "ingenic,jz4780-pwm", },
 	{},
 };
 MODULE_DEVICE_TABLE(of, jz4740_pwm_dt_ids);
-- 
2.21.0.593.g511ec345e18

[PATCH v2 3/6] pwm: jz4740: Apply configuration atomically

[Paul Cercueil]

This is cleaner, more future-proof, and incidentally it also fixes the
PWM resetting its config when stopped/started several times.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v2: Rebase on v5.2-rc3

 drivers/pwm/pwm-jz4740.c | 37 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 25 deletions(-)

diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index 5c8bb6192201..75254360621c 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -74,17 +74,16 @@ static void jz4740_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	jz4740_timer_disable(pwm->hwpwm);
 }
 
-static int jz4740_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			     int duty_ns, int period_ns)
+static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			    struct pwm_state *state)
 {
 	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
 	unsigned long long tmp;
 	unsigned long period, duty;
 	unsigned int prescaler = 0;
 	uint16_t ctrl;
-	bool is_enabled;
 
-	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * period_ns;
+	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * state->period;
 	do_div(tmp, 1000000000);
 	period = tmp;
 
@@ -96,16 +95,14 @@ static int jz4740_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (prescaler == 6)
 		return -EINVAL;
 
-	tmp = (unsigned long long)period * duty_ns;
-	do_div(tmp, period_ns);
+	tmp = (unsigned long long)period * state->duty_cycle;
+	do_div(tmp, state->period);
 	duty = period - tmp;
 
 	if (duty >= period)
 		duty = period - 1;
 
-	is_enabled = jz4740_timer_is_enabled(pwm->hwpwm);
-	if (is_enabled)
-		jz4740_pwm_disable(chip, pwm);
+	jz4740_pwm_disable(chip, pwm);
 
 	jz4740_timer_set_count(pwm->hwpwm, 0);
 	jz4740_timer_set_duty(pwm->hwpwm, duty);
@@ -116,18 +113,7 @@ static int jz4740_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
 
-	if (is_enabled)
-		jz4740_pwm_enable(chip, pwm);
-
-	return 0;
-}
-
-static int jz4740_pwm_set_polarity(struct pwm_chip *chip,
-		struct pwm_device *pwm, enum pwm_polarity polarity)
-{
-	uint32_t ctrl = jz4740_timer_get_ctrl(pwm->pwm);
-
-	switch (polarity) {
+	switch (state->polarity) {
 	case PWM_POLARITY_NORMAL:
 		ctrl &= ~JZ_TIMER_CTRL_PWM_ACTIVE_LOW;
 		break;
@@ -137,16 +123,17 @@ static int jz4740_pwm_set_polarity(struct pwm_chip *chip,
 	}
 
 	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
+
+	if (state->enabled)
+		jz4740_pwm_enable(chip, pwm);
+
 	return 0;
 }
 
 static const struct pwm_ops jz4740_pwm_ops = {
 	.request = jz4740_pwm_request,
 	.free = jz4740_pwm_free,
-	.config = jz4740_pwm_config,
-	.set_polarity = jz4740_pwm_set_polarity,
-	.enable = jz4740_pwm_enable,
-	.disable = jz4740_pwm_disable,
+	.apply = jz4740_pwm_apply,
 	.owner = THIS_MODULE,
 };
 
-- 
2.21.0.593.g511ec345e18

[PATCH v2 4/6] pwm: jz4740: Drop dependency on MACH_INGENIC

[Paul Cercueil]

Depending on MACH_INGENIC prevent us from creating a generic kernel that
works on more than one MIPS board. Instead, we just depend on MIPS being
set.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v2: Rebase on v5.2-rc3

 drivers/pwm/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index dff5a93f7daa..26d484e22838 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -224,7 +224,7 @@ config PWM_IMX_TPM
 
 config PWM_JZ4740
 	tristate "Ingenic JZ47xx PWM support"
-	depends on MACH_INGENIC
+	depends on MIPS
 	help
 	  Generic PWM framework driver for Ingenic JZ47xx based
 	  machines.
-- 
2.21.0.593.g511ec345e18

[PATCH v2 5/6] pwm: jz4740: Force TCU2 channels to return to their init level

[Paul Cercueil]

When the PWM mode of TCU2 channels is disabled, their corresponding pin
does not always return to its initial level. Force this by using a small
trick: we set duty > period, which is an invalid configuration for the
hardware, which then correctly resets the pin to the initial level.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v2: New patch

 drivers/pwm/pwm-jz4740.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index 75254360621c..f901e8a0d33d 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -63,7 +63,15 @@ static void jz4740_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	uint32_t ctrl = jz4740_timer_get_ctrl(pwm->hwpwm);
 
-	/* Disable PWM output.
+	/*
+	 * Set duty > period. This trick allows the TCU channels in TCU2 mode to
+	 * properly return to their init level.
+	 */
+	jz4740_timer_set_duty(pwm->hwpwm, 0xffff);
+	jz4740_timer_set_period(pwm->hwpwm, 0x0);
+
+	/*
+	 * Disable PWM output.
 	 * In TCU2 mode (channel 1/2 on JZ4750+), this must be done before the
 	 * counter is stopped, while in TCU1 mode the order does not matter.
 	 */
-- 
2.21.0.593.g511ec345e18

[PATCH v2 6/6] pwm: jz4740: Use __init_or_module and __exit for .probe and .remove

[Paul Cercueil]

This allows the probe function to be dropped after the kernel finished
its initialization, in the case where the driver was not compiled as a
module.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v2: New patch

 drivers/pwm/pwm-jz4740.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index f901e8a0d33d..1b5077825721 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -145,7 +145,7 @@ static const struct pwm_ops jz4740_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
-static int jz4740_pwm_probe(struct platform_device *pdev)
+static int __init_or_module jz4740_pwm_probe(struct platform_device *pdev)
 {
 	struct jz4740_pwm_chip *jz4740;
 
@@ -169,7 +169,7 @@ static int jz4740_pwm_probe(struct platform_device *pdev)
 	return pwmchip_add(&jz4740->chip);
 }
 
-static int jz4740_pwm_remove(struct platform_device *pdev)
+static int __exit jz4740_pwm_remove(struct platform_device *pdev)
 {
 	struct jz4740_pwm_chip *jz4740 = platform_get_drvdata(pdev);
 
-- 
2.21.0.593.g511ec345e18

Re: [PATCH v2 6/6] pwm: jz4740: Use __init_or_module and __exit for .probe and .remove

[Paul Cercueil]

I misunderstood what __init_or_module was for. Please ignore this patch
(the rest of the patchset is OK). Sorry for the noise.


Le ven. 7 juin 2019 à 17:44, Paul Cercueil <paul@crapouillou.net> a 
écrit :
> This allows the probe function to be dropped after the kernel finished
> its initialization, in the case where the driver was not compiled as a
> module.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
> Notes:
>     v2: New patch
> 
>  drivers/pwm/pwm-jz4740.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> index f901e8a0d33d..1b5077825721 100644
> --- a/drivers/pwm/pwm-jz4740.c
> +++ b/drivers/pwm/pwm-jz4740.c
> @@ -145,7 +145,7 @@ static const struct pwm_ops jz4740_pwm_ops = {
>  	.owner = THIS_MODULE,
>  };
> 
> -static int jz4740_pwm_probe(struct platform_device *pdev)
> +static int __init_or_module jz4740_pwm_probe(struct platform_device 
> *pdev)
>  {
>  	struct jz4740_pwm_chip *jz4740;
> 
> @@ -169,7 +169,7 @@ static int jz4740_pwm_probe(struct 
> platform_device *pdev)
>  	return pwmchip_add(&jz4740->chip);
>  }
> 
> -static int jz4740_pwm_remove(struct platform_device *pdev)
> +static int __exit jz4740_pwm_remove(struct platform_device *pdev)
>  {
>  	struct jz4740_pwm_chip *jz4740 = platform_get_drvdata(pdev);
> 
> --
> 2.21.0.593.g511ec345e18
> 

Re: [PATCH v2 1/6] dt-bindings: Remove unused compatible strings

[Rob Herring]

On Fri, Jun 07, 2019 at 05:44:05PM +0200, Paul Cercueil wrote:
> Right now none of the Ingenic-based boards probe this driver from
> devicetree. This driver defined three compatible strings for the exact
> same behaviour. Before these strings are used, we can remove two of
> them.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
> Notes:
>     v2: Rebase on v5.2-rc3
> 
>  Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> index 7d9d3f90641b..493bec80d59b 100644
> --- a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> +++ b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> @@ -2,10 +2,7 @@ Ingenic JZ47xx PWM Controller
>  =============================
>  
>  Required properties:
> -- compatible: One of:
> -  * "ingenic,jz4740-pwm"
> -  * "ingenic,jz4770-pwm"
> -  * "ingenic,jz4780-pwm"
> +- compatible: Should be "ingenic,jz4740-pwm"

Are you sure all 3 chips are exactly the same features and bugs?

The correct thing to do here generally is the 4770 and 4780 would also 
have ingenic,jz4740-pwm as a fallback compatible. Then the driver can 
match on that until you find some difference and can use the more 
specific compatible.

>  - #pwm-cells: Should be 3. See pwm.txt in this directory for a description
>    of the cells format.
>  - clocks : phandle to the external clock.
> -- 
> 2.21.0.593.g511ec345e18
> 

Re: [PATCH v2 1/6] dt-bindings: Remove unused compatible strings

[Paul Cercueil]

Le lun. 8 juil. 2019 à 22:04, Rob Herring <robh@kernel.org> a écrit :
> On Fri, Jun 07, 2019 at 05:44:05PM +0200, Paul Cercueil wrote:
>>  Right now none of the Ingenic-based boards probe this driver from
>>  devicetree. This driver defined three compatible strings for the 
>> exact
>>  same behaviour. Before these strings are used, we can remove two of
>>  them.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>> 
>>  Notes:
>>      v2: Rebase on v5.2-rc3
>> 
>>   Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt | 5 
>> +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>> 
>>  diff --git 
>> a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt 
>> b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
>>  index 7d9d3f90641b..493bec80d59b 100644
>>  --- a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
>>  +++ b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
>>  @@ -2,10 +2,7 @@ Ingenic JZ47xx PWM Controller
>>   =============================
>> 
>>   Required properties:
>>  -- compatible: One of:
>>  -  * "ingenic,jz4740-pwm"
>>  -  * "ingenic,jz4770-pwm"
>>  -  * "ingenic,jz4780-pwm"
>>  +- compatible: Should be "ingenic,jz4740-pwm"
> 
> Are you sure all 3 chips are exactly the same features and bugs?

The hardware on these chips have tiny differences, but the current
driver code works on all the SoCs.


> The correct thing to do here generally is the 4770 and 4780 would also
> have ingenic,jz4740-pwm as a fallback compatible. Then the driver can
> match on that until you find some difference and can use the more
> specific compatible.

Why not just update the devicetree with the new compatible string when
a difference is found?


>>   - #pwm-cells: Should be 3. See pwm.txt in this directory for a 
>> description
>>     of the cells format.
>>   - clocks : phandle to the external clock.
>>  --
>>  2.21.0.593.g511ec345e18
>> 

Re: [PATCH v2 1/6] dt-bindings: Remove unused compatible strings

[Rob Herring]

On Mon, Jul 8, 2019 at 9:18 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
>
>
> Le lun. 8 juil. 2019 à 22:04, Rob Herring <robh@kernel.org> a écrit :
> > On Fri, Jun 07, 2019 at 05:44:05PM +0200, Paul Cercueil wrote:
> >>  Right now none of the Ingenic-based boards probe this driver from
> >>  devicetree. This driver defined three compatible strings for the
> >> exact
> >>  same behaviour. Before these strings are used, we can remove two of
> >>  them.
> >>
> >>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> >>  ---
> >>
> >>  Notes:
> >>      v2: Rebase on v5.2-rc3
> >>
> >>   Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt | 5
> >> +----
> >>   1 file changed, 1 insertion(+), 4 deletions(-)
> >>
> >>  diff --git
> >> a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> >> b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> >>  index 7d9d3f90641b..493bec80d59b 100644
> >>  --- a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> >>  +++ b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> >>  @@ -2,10 +2,7 @@ Ingenic JZ47xx PWM Controller
> >>   =============================
> >>
> >>   Required properties:
> >>  -- compatible: One of:
> >>  -  * "ingenic,jz4740-pwm"
> >>  -  * "ingenic,jz4770-pwm"
> >>  -  * "ingenic,jz4780-pwm"
> >>  +- compatible: Should be "ingenic,jz4740-pwm"
> >
> > Are you sure all 3 chips are exactly the same features and bugs?
>
> The hardware on these chips have tiny differences, but the current
> driver code works on all the SoCs.
>
>
> > The correct thing to do here generally is the 4770 and 4780 would also
> > have ingenic,jz4740-pwm as a fallback compatible. Then the driver can
> > match on that until you find some difference and can use the more
> > specific compatible.
>
> Why not just update the devicetree with the new compatible string when
> a difference is found?

Because dtb's should be part of the firmware/bootloader and can be
updated independently. Maybe you don't care on your platform, but
that's the preferred model for DT and what we follow. DTs live in the
kernel tree for convenience, but are independent from the kernel.

Rob

Re: [PATCH v2 3/6] pwm: jz4740: Apply configuration atomically

[Uwe Kleine-König]

Hello Paul,

On Fri, Jun 07, 2019 at 05:44:07PM +0200, Paul Cercueil wrote:
> -static int jz4740_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> -			     int duty_ns, int period_ns)
> +static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			    struct pwm_state *state)
>  {
>  	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
>  	unsigned long long tmp;
>  	unsigned long period, duty;
>  	unsigned int prescaler = 0;
>  	uint16_t ctrl;
> -	bool is_enabled;
>  
> -	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * period_ns;
> +	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * state->period;
>  	do_div(tmp, 1000000000);
>  	period = tmp;
>  
> @@ -96,16 +95,14 @@ static int jz4740_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (prescaler == 6)
>  		return -EINVAL;
>  
> -	tmp = (unsigned long long)period * duty_ns;
> -	do_div(tmp, period_ns);
> +	tmp = (unsigned long long)period * state->duty_cycle;
> +	do_div(tmp, state->period);
>  	duty = period - tmp;
>  
>  	if (duty >= period)
>  		duty = period - 1;
>  
> -	is_enabled = jz4740_timer_is_enabled(pwm->hwpwm);
> -	if (is_enabled)
> -		jz4740_pwm_disable(chip, pwm);
> +	jz4740_pwm_disable(chip, pwm);

I assume this stops the PWM. Does this complete the currently running
period? How does the PWM behave then? (Does it still drive the output?
If so, on which level?)

>  
>  	jz4740_timer_set_count(pwm->hwpwm, 0);
>  	jz4740_timer_set_duty(pwm->hwpwm, duty);

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 3/6] pwm: jz4740: Apply configuration atomically

[Paul Cercueil]

Hi Uwe,



Le lun. 22 juil. 2019 à 15:34, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@pengutronix.de> a écrit :
> Hello Paul,
> 
> On Fri, Jun 07, 2019 at 05:44:07PM +0200, Paul Cercueil wrote:
>>  -static int jz4740_pwm_config(struct pwm_chip *chip, struct 
>> pwm_device *pwm,
>>  -			     int duty_ns, int period_ns)
>>  +static int jz4740_pwm_apply(struct pwm_chip *chip, struct 
>> pwm_device *pwm,
>>  +			    struct pwm_state *state)
>>   {
>>   	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
>>   	unsigned long long tmp;
>>   	unsigned long period, duty;
>>   	unsigned int prescaler = 0;
>>   	uint16_t ctrl;
>>  -	bool is_enabled;
>> 
>>  -	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * period_ns;
>>  +	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * 
>> state->period;
>>   	do_div(tmp, 1000000000);
>>   	period = tmp;
>> 
>>  @@ -96,16 +95,14 @@ static int jz4740_pwm_config(struct pwm_chip 
>> *chip, struct pwm_device *pwm,
>>   	if (prescaler == 6)
>>   		return -EINVAL;
>> 
>>  -	tmp = (unsigned long long)period * duty_ns;
>>  -	do_div(tmp, period_ns);
>>  +	tmp = (unsigned long long)period * state->duty_cycle;
>>  +	do_div(tmp, state->period);
>>   	duty = period - tmp;
>> 
>>   	if (duty >= period)
>>   		duty = period - 1;
>> 
>>  -	is_enabled = jz4740_timer_is_enabled(pwm->hwpwm);
>>  -	if (is_enabled)
>>  -		jz4740_pwm_disable(chip, pwm);
>>  +	jz4740_pwm_disable(chip, pwm);
> 
> I assume this stops the PWM. Does this complete the currently running
> period? How does the PWM behave then? (Does it still drive the output?
> If so, on which level?)

Some PWM channels work in one mode "TCU1" and others work in "TCU2". The
mode in which channels work depends on the version of the SoC.

When stopped, the pins of TCU1 channels will be driven to the inactive
level (which depends on the polarity). It is unknown whether or not the
currently running period is completed. We set a bit to configure for
"abrupt shutdown", so I expect that it's not, but somebody would need
to hook up a logic analyzer to see what's the exact behaviour with
and without that bit.

TCU2 channels on the other hand will stop in the middle of a period,
leaving the pin hanging at whatever level it was before the stop.
That's the rationale behind the trick in commit 6580fd173070 ("pwm:
jz4740: Force TCU2 channels to return to their init level").

Regards,
-Paul


>> 
>>   	jz4740_timer_set_count(pwm->hwpwm, 0);
>>   	jz4740_timer_set_duty(pwm->hwpwm, duty);
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> http://www.pengutronix.de/  |

Re: [PATCH v2 3/6] pwm: jz4740: Apply configuration atomically

[Uwe Kleine-König]

Hello Paul,

On Tue, Jul 23, 2019 at 04:46:40PM -0400, Paul Cercueil wrote:
> Le lun. 22 juil. 2019 à 15:34, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
> <u.kleine-koenig@pengutronix.de> a écrit :
> > On Fri, Jun 07, 2019 at 05:44:07PM +0200, Paul Cercueil wrote:
> > >  -	is_enabled = jz4740_timer_is_enabled(pwm->hwpwm);
> > >  -	if (is_enabled)
> > >  -		jz4740_pwm_disable(chip, pwm);
> > >  +	jz4740_pwm_disable(chip, pwm);
> > 
> > I assume this stops the PWM. Does this complete the currently running
> > period? How does the PWM behave then? (Does it still drive the output?
> > If so, on which level?)
> 
> Some PWM channels work in one mode "TCU1" and others work in "TCU2". The
> mode in which channels work depends on the version of the SoC.
> 
> When stopped, the pins of TCU1 channels will be driven to the inactive
> level (which depends on the polarity). It is unknown whether or not the
> currently running period is completed. We set a bit to configure for
> "abrupt shutdown", so I expect that it's not, but somebody would need
> to hook up a logic analyzer to see what's the exact behaviour with
> and without that bit.

This might be done even without a logic analyzer. Just do something
like:

	pwm_apply_state(pwm, { .enabled = 1, .period = 5s })
	pwm_apply_state(pwm, { .enabled = 1, .period = 5s, .duty = 5s })

and if that takes less then 5s the period is not completed.

And note that "abrupt shutdown" is a bug.

> TCU2 channels on the other hand will stop in the middle of a period,
> leaving the pin hanging at whatever level it was before the stop.
> That's the rationale behind the trick in commit 6580fd173070 ("pwm:
> jz4740: Force TCU2 channels to return to their init level").

Strange, but ok.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 3/6] pwm: jz4740: Apply configuration atomically

[Paul Cercueil]

Hi Uwe,


Le mer. 24 juil. 2019 à 2:47, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@pengutronix.de> a écrit :
> Hello Paul,
> 
> On Tue, Jul 23, 2019 at 04:46:40PM -0400, Paul Cercueil wrote:
>>  Le lun. 22 juil. 2019 à 15:34, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
>>  <u.kleine-koenig@pengutronix.de> a écrit :
>>  > On Fri, Jun 07, 2019 at 05:44:07PM +0200, Paul Cercueil wrote:
>>  > >  -	is_enabled = jz4740_timer_is_enabled(pwm->hwpwm);
>>  > >  -	if (is_enabled)
>>  > >  -		jz4740_pwm_disable(chip, pwm);
>>  > >  +	jz4740_pwm_disable(chip, pwm);
>>  >
>>  > I assume this stops the PWM. Does this complete the currently 
>> running
>>  > period? How does the PWM behave then? (Does it still drive the 
>> output?
>>  > If so, on which level?)
>> 
>>  Some PWM channels work in one mode "TCU1" and others work in 
>> "TCU2". The
>>  mode in which channels work depends on the version of the SoC.
>> 
>>  When stopped, the pins of TCU1 channels will be driven to the 
>> inactive
>>  level (which depends on the polarity). It is unknown whether or not 
>> the
>>  currently running period is completed. We set a bit to configure for
>>  "abrupt shutdown", so I expect that it's not, but somebody would 
>> need
>>  to hook up a logic analyzer to see what's the exact behaviour with
>>  and without that bit.
> 
> This might be done even without a logic analyzer. Just do something
> like:
> 
> 	pwm_apply_state(pwm, { .enabled = 1, .period = 5s })
> 	pwm_apply_state(pwm, { .enabled = 1, .period = 5s, .duty = 5s })
> 
> and if that takes less then 5s the period is not completed.
> 
> And note that "abrupt shutdown" is a bug.

I remember you asked that already in an older patchset.
The result of this test is that the period is never completed,
independently of the "abrupt shutdown" bit.

Cheers,
-Paul


>>  TCU2 channels on the other hand will stop in the middle of a period,
>>  leaving the pin hanging at whatever level it was before the stop.
>>  That's the rationale behind the trick in commit 6580fd173070 ("pwm:
>>  jz4740: Force TCU2 channels to return to their init level").
> 
> Strange, but ok.
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> http://www.pengutronix.de/  |

Re: [PATCH v2 2/6] pwm: jz4740: Remove unused devicetree compatible strings

[Uwe Kleine-König]

On Fri, Jun 07, 2019 at 05:44:06PM +0200, Paul Cercueil wrote:
> Right now none of the Ingenic-based boards probe this driver from
> devicetree. This driver defined three compatible strings for the exact
> same behaviour. Before these strings are used, we can remove two of
> them.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

No hard opinion on my side on this one. Keeping the unused compatibles
around (maybe with a comment to prefer ingenic,jz4740-pwm) is what I
would have done. If you're really sure nobody used the two removed ones
this is fine. The binding already requests that ingenic,jz4740-pwm is to
be used.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 1/6] dt-bindings: Remove unused compatible strings

[Uwe Kleine-König]

On Mon, Jul 08, 2019 at 08:04:25PM -0600, Rob Herring wrote:
> On Fri, Jun 07, 2019 at 05:44:05PM +0200, Paul Cercueil wrote:
> > Right now none of the Ingenic-based boards probe this driver from
> > devicetree. This driver defined three compatible strings for the exact
> > same behaviour. Before these strings are used, we can remove two of
> > them.
> > 
> > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > ---
> > 
> > Notes:
> >     v2: Rebase on v5.2-rc3
> > 
> >  Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> > index 7d9d3f90641b..493bec80d59b 100644
> > --- a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> > +++ b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> > @@ -2,10 +2,7 @@ Ingenic JZ47xx PWM Controller
> >  =============================
> >  
> >  Required properties:
> > -- compatible: One of:
> > -  * "ingenic,jz4740-pwm"
> > -  * "ingenic,jz4770-pwm"
> > -  * "ingenic,jz4780-pwm"
> > +- compatible: Should be "ingenic,jz4740-pwm"
> 
> Are you sure all 3 chips are exactly the same features and bugs?
> 
> The correct thing to do here generally is the 4770 and 4780 would also 
> have ingenic,jz4740-pwm as a fallback compatible. Then the driver can 
> match on that until you find some difference and can use the more 
> specific compatible.

Note that Thierry already applied this patch before your concern reached
the mailing list. Is this bad enough to revert
925488e8df4f396ad96ff008a84f5b14d8b73347?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 1/6] dt-bindings: Remove unused compatible strings

[Rob Herring]

On Thu, Aug 8, 2019 at 2:28 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Mon, Jul 08, 2019 at 08:04:25PM -0600, Rob Herring wrote:
> > On Fri, Jun 07, 2019 at 05:44:05PM +0200, Paul Cercueil wrote:
> > > Right now none of the Ingenic-based boards probe this driver from
> > > devicetree. This driver defined three compatible strings for the exact
> > > same behaviour. Before these strings are used, we can remove two of
> > > them.
> > >
> > > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > > ---
> > >
> > > Notes:
> > >     v2: Rebase on v5.2-rc3
> > >
> > >  Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> > > index 7d9d3f90641b..493bec80d59b 100644
> > > --- a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> > > +++ b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> > > @@ -2,10 +2,7 @@ Ingenic JZ47xx PWM Controller
> > >  =============================
> > >
> > >  Required properties:
> > > -- compatible: One of:
> > > -  * "ingenic,jz4740-pwm"
> > > -  * "ingenic,jz4770-pwm"
> > > -  * "ingenic,jz4780-pwm"
> > > +- compatible: Should be "ingenic,jz4740-pwm"
> >
> > Are you sure all 3 chips are exactly the same features and bugs?
> >
> > The correct thing to do here generally is the 4770 and 4780 would also
> > have ingenic,jz4740-pwm as a fallback compatible. Then the driver can
> > match on that until you find some difference and can use the more
> > specific compatible.
>
> Note that Thierry already applied this patch before your concern reached
> the mailing list. Is this bad enough to revert
> 925488e8df4f396ad96ff008a84f5b14d8b73347?

It won't be my problem when you find and need to handle any of the
differences in the driver.

Rob