All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH u-boot-mvebu] arm64: a37xx: pinctrl: Correct PWM pins definitions
@ 2021-07-23 17:57 Marek Behún
  2021-07-31  7:33 ` Stefan Roese
  2021-07-31 10:01 ` Stefan Roese
  0 siblings, 2 replies; 3+ messages in thread
From: Marek Behún @ 2021-07-23 17:57 UTC (permalink / raw)
  To: Stefan Roese; +Cc: u-boot, pali, Marek Behún

The PWM pins on North Bridge on Armada 37xx can be configured into PWM
or GPIO functions. When in PWM function, each pin can also be configured
to drive low on 0 and tri-state on 1 (LED mode).

The current definitions handle this by declaring two pin groups for each
pin:
- group "pwmN" with functions "pwm" and "gpio"
- group "ledN_od" ("od" for open drain) with functions "led" and "gpio"

This is semantically incorrect. The correct definition for each pin
should be one group with three functions: "pwm", "led" and "gpio".

Change the "pwmN" groups to support "led" function.

Remove "ledN_od" groups. This cannot break backwards compatibility with
older device trees: no device tree uses it since there is no PWM driver
for this SOC yet. Also "ledN_od" groups are not even documented.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
This was recently applied also in Linux, see
https://lore.kernel.org/linux-gpio/CACRpkdbkWxw8wEOt2nsiYKLw+=TjFrJX=WYALupCtWWc9xGw7A@mail.gmail.com/T/#t
---
 .../pinctrl/marvell,armada-37xx-pinctrl.txt     |  8 ++++----
 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c     | 17 ++++++++---------
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/doc/device-tree-bindings/pinctrl/marvell,armada-37xx-pinctrl.txt b/doc/device-tree-bindings/pinctrl/marvell,armada-37xx-pinctrl.txt
index 86ec11361c..3139a99fa9 100644
--- a/doc/device-tree-bindings/pinctrl/marvell,armada-37xx-pinctrl.txt
+++ b/doc/device-tree-bindings/pinctrl/marvell,armada-37xx-pinctrl.txt
@@ -38,19 +38,19 @@ group emmc_nb
 
 group pwm0
  - pin 11 (GPIO1-11)
- - functions pwm, gpio
+ - functions pwm, led, gpio
 
 group pwm1
  - pin 12
- - functions pwm, gpio
+ - functions pwm, led, gpio
 
 group pwm2
  - pin 13
- - functions pwm, gpio
+ - functions pwm, led, gpio
 
 group pwm3
  - pin 14
- - functions pwm, gpio
+ - functions pwm, led, gpio
 
 group pmic1
  - pin 7
diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
index b9d389e70f..1cf1f06f10 100644
--- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
+++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
@@ -160,10 +160,14 @@ static struct armada_37xx_pin_group armada_37xx_nb_groups[] = {
 	PIN_GRP_GPIO("jtag", 20, 5, BIT(0), "jtag"),
 	PIN_GRP_GPIO("sdio0", 8, 3, BIT(1), "sdio"),
 	PIN_GRP_GPIO("emmc_nb", 27, 9, BIT(2), "emmc"),
-	PIN_GRP_GPIO("pwm0", 11, 1, BIT(3), "pwm"),
-	PIN_GRP_GPIO("pwm1", 12, 1, BIT(4), "pwm"),
-	PIN_GRP_GPIO("pwm2", 13, 1, BIT(5), "pwm"),
-	PIN_GRP_GPIO("pwm3", 14, 1, BIT(6), "pwm"),
+	PIN_GRP_GPIO_3("pwm0", 11, 1, BIT(3) | BIT(20), 0, BIT(20), BIT(3),
+		       "pwm", "led"),
+	PIN_GRP_GPIO_3("pwm1", 11, 1, BIT(4) | BIT(21), 0, BIT(21), BIT(4),
+		       "pwm", "led"),
+	PIN_GRP_GPIO_3("pwm2", 11, 1, BIT(5) | BIT(22), 0, BIT(22), BIT(5),
+		       "pwm", "led"),
+	PIN_GRP_GPIO_3("pwm3", 11, 1, BIT(6) | BIT(23), 0, BIT(23), BIT(6),
+		       "pwm", "led"),
 	PIN_GRP_GPIO("pmic1", 7, 1, BIT(7), "pmic"),
 	PIN_GRP_GPIO("pmic0", 6, 1, BIT(8), "pmic"),
 	PIN_GRP_GPIO("i2c2", 2, 2, BIT(9), "i2c"),
@@ -177,11 +181,6 @@ static struct armada_37xx_pin_group armada_37xx_nb_groups[] = {
 	PIN_GRP_EXTRA("uart2", 9, 2, BIT(1) | BIT(13) | BIT(14) | BIT(19),
 		      BIT(1) | BIT(13) | BIT(14), BIT(1) | BIT(19),
 		      18, 2, "gpio", "uart"),
-	PIN_GRP_GPIO("led0_od", 11, 1, BIT(20), "led"),
-	PIN_GRP_GPIO("led1_od", 12, 1, BIT(21), "led"),
-	PIN_GRP_GPIO("led2_od", 13, 1, BIT(22), "led"),
-	PIN_GRP_GPIO("led3_od", 14, 1, BIT(23), "led"),
-
 };
 
 static struct armada_37xx_pin_group armada_37xx_sb_groups[] = {
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH u-boot-mvebu] arm64: a37xx: pinctrl: Correct PWM pins definitions
  2021-07-23 17:57 [PATCH u-boot-mvebu] arm64: a37xx: pinctrl: Correct PWM pins definitions Marek Behún
@ 2021-07-31  7:33 ` Stefan Roese
  2021-07-31 10:01 ` Stefan Roese
  1 sibling, 0 replies; 3+ messages in thread
From: Stefan Roese @ 2021-07-31  7:33 UTC (permalink / raw)
  To: Marek Behún; +Cc: u-boot, pali

On 23.07.21 19:57, Marek Behún wrote:
> The PWM pins on North Bridge on Armada 37xx can be configured into PWM
> or GPIO functions. When in PWM function, each pin can also be configured
> to drive low on 0 and tri-state on 1 (LED mode).
> 
> The current definitions handle this by declaring two pin groups for each
> pin:
> - group "pwmN" with functions "pwm" and "gpio"
> - group "ledN_od" ("od" for open drain) with functions "led" and "gpio"
> 
> This is semantically incorrect. The correct definition for each pin
> should be one group with three functions: "pwm", "led" and "gpio".
> 
> Change the "pwmN" groups to support "led" function.
> 
> Remove "ledN_od" groups. This cannot break backwards compatibility with
> older device trees: no device tree uses it since there is no PWM driver
> for this SOC yet. Also "ledN_od" groups are not even documented.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
> This was recently applied also in Linux, see
> https://lore.kernel.org/linux-gpio/CACRpkdbkWxw8wEOt2nsiYKLw+=TjFrJX=WYALupCtWWc9xGw7A@mail.gmail.com/T/#t
> ---
>   .../pinctrl/marvell,armada-37xx-pinctrl.txt     |  8 ++++----
>   drivers/pinctrl/mvebu/pinctrl-armada-37xx.c     | 17 ++++++++---------
>   2 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/doc/device-tree-bindings/pinctrl/marvell,armada-37xx-pinctrl.txt b/doc/device-tree-bindings/pinctrl/marvell,armada-37xx-pinctrl.txt
> index 86ec11361c..3139a99fa9 100644
> --- a/doc/device-tree-bindings/pinctrl/marvell,armada-37xx-pinctrl.txt
> +++ b/doc/device-tree-bindings/pinctrl/marvell,armada-37xx-pinctrl.txt
> @@ -38,19 +38,19 @@ group emmc_nb
>   
>   group pwm0
>    - pin 11 (GPIO1-11)
> - - functions pwm, gpio
> + - functions pwm, led, gpio
>   
>   group pwm1
>    - pin 12
> - - functions pwm, gpio
> + - functions pwm, led, gpio
>   
>   group pwm2
>    - pin 13
> - - functions pwm, gpio
> + - functions pwm, led, gpio
>   
>   group pwm3
>    - pin 14
> - - functions pwm, gpio
> + - functions pwm, led, gpio
>   
>   group pmic1
>    - pin 7
> diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
> index b9d389e70f..1cf1f06f10 100644
> --- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
> +++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
> @@ -160,10 +160,14 @@ static struct armada_37xx_pin_group armada_37xx_nb_groups[] = {
>   	PIN_GRP_GPIO("jtag", 20, 5, BIT(0), "jtag"),
>   	PIN_GRP_GPIO("sdio0", 8, 3, BIT(1), "sdio"),
>   	PIN_GRP_GPIO("emmc_nb", 27, 9, BIT(2), "emmc"),
> -	PIN_GRP_GPIO("pwm0", 11, 1, BIT(3), "pwm"),
> -	PIN_GRP_GPIO("pwm1", 12, 1, BIT(4), "pwm"),
> -	PIN_GRP_GPIO("pwm2", 13, 1, BIT(5), "pwm"),
> -	PIN_GRP_GPIO("pwm3", 14, 1, BIT(6), "pwm"),
> +	PIN_GRP_GPIO_3("pwm0", 11, 1, BIT(3) | BIT(20), 0, BIT(20), BIT(3),
> +		       "pwm", "led"),
> +	PIN_GRP_GPIO_3("pwm1", 11, 1, BIT(4) | BIT(21), 0, BIT(21), BIT(4),
> +		       "pwm", "led"),
> +	PIN_GRP_GPIO_3("pwm2", 11, 1, BIT(5) | BIT(22), 0, BIT(22), BIT(5),
> +		       "pwm", "led"),
> +	PIN_GRP_GPIO_3("pwm3", 11, 1, BIT(6) | BIT(23), 0, BIT(23), BIT(6),
> +		       "pwm", "led"),
>   	PIN_GRP_GPIO("pmic1", 7, 1, BIT(7), "pmic"),
>   	PIN_GRP_GPIO("pmic0", 6, 1, BIT(8), "pmic"),
>   	PIN_GRP_GPIO("i2c2", 2, 2, BIT(9), "i2c"),
> @@ -177,11 +181,6 @@ static struct armada_37xx_pin_group armada_37xx_nb_groups[] = {
>   	PIN_GRP_EXTRA("uart2", 9, 2, BIT(1) | BIT(13) | BIT(14) | BIT(19),
>   		      BIT(1) | BIT(13) | BIT(14), BIT(1) | BIT(19),
>   		      18, 2, "gpio", "uart"),
> -	PIN_GRP_GPIO("led0_od", 11, 1, BIT(20), "led"),
> -	PIN_GRP_GPIO("led1_od", 12, 1, BIT(21), "led"),
> -	PIN_GRP_GPIO("led2_od", 13, 1, BIT(22), "led"),
> -	PIN_GRP_GPIO("led3_od", 14, 1, BIT(23), "led"),
> -
>   };
>   
>   static struct armada_37xx_pin_group armada_37xx_sb_groups[] = {
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH u-boot-mvebu] arm64: a37xx: pinctrl: Correct PWM pins definitions
  2021-07-23 17:57 [PATCH u-boot-mvebu] arm64: a37xx: pinctrl: Correct PWM pins definitions Marek Behún
  2021-07-31  7:33 ` Stefan Roese
@ 2021-07-31 10:01 ` Stefan Roese
  1 sibling, 0 replies; 3+ messages in thread
From: Stefan Roese @ 2021-07-31 10:01 UTC (permalink / raw)
  To: Marek Behún; +Cc: u-boot, pali

On 23.07.21 19:57, Marek Behún wrote:
> The PWM pins on North Bridge on Armada 37xx can be configured into PWM
> or GPIO functions. When in PWM function, each pin can also be configured
> to drive low on 0 and tri-state on 1 (LED mode).
> 
> The current definitions handle this by declaring two pin groups for each
> pin:
> - group "pwmN" with functions "pwm" and "gpio"
> - group "ledN_od" ("od" for open drain) with functions "led" and "gpio"
> 
> This is semantically incorrect. The correct definition for each pin
> should be one group with three functions: "pwm", "led" and "gpio".
> 
> Change the "pwmN" groups to support "led" function.
> 
> Remove "ledN_od" groups. This cannot break backwards compatibility with
> older device trees: no device tree uses it since there is no PWM driver
> for this SOC yet. Also "ledN_od" groups are not even documented.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Applied to u-boot-marvell/master

Thanks,
Stefan

> ---
> This was recently applied also in Linux, see
> https://lore.kernel.org/linux-gpio/CACRpkdbkWxw8wEOt2nsiYKLw+=TjFrJX=WYALupCtWWc9xGw7A@mail.gmail.com/T/#t
> ---
>   .../pinctrl/marvell,armada-37xx-pinctrl.txt     |  8 ++++----
>   drivers/pinctrl/mvebu/pinctrl-armada-37xx.c     | 17 ++++++++---------
>   2 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/doc/device-tree-bindings/pinctrl/marvell,armada-37xx-pinctrl.txt b/doc/device-tree-bindings/pinctrl/marvell,armada-37xx-pinctrl.txt
> index 86ec11361c..3139a99fa9 100644
> --- a/doc/device-tree-bindings/pinctrl/marvell,armada-37xx-pinctrl.txt
> +++ b/doc/device-tree-bindings/pinctrl/marvell,armada-37xx-pinctrl.txt
> @@ -38,19 +38,19 @@ group emmc_nb
>   
>   group pwm0
>    - pin 11 (GPIO1-11)
> - - functions pwm, gpio
> + - functions pwm, led, gpio
>   
>   group pwm1
>    - pin 12
> - - functions pwm, gpio
> + - functions pwm, led, gpio
>   
>   group pwm2
>    - pin 13
> - - functions pwm, gpio
> + - functions pwm, led, gpio
>   
>   group pwm3
>    - pin 14
> - - functions pwm, gpio
> + - functions pwm, led, gpio
>   
>   group pmic1
>    - pin 7
> diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
> index b9d389e70f..1cf1f06f10 100644
> --- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
> +++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
> @@ -160,10 +160,14 @@ static struct armada_37xx_pin_group armada_37xx_nb_groups[] = {
>   	PIN_GRP_GPIO("jtag", 20, 5, BIT(0), "jtag"),
>   	PIN_GRP_GPIO("sdio0", 8, 3, BIT(1), "sdio"),
>   	PIN_GRP_GPIO("emmc_nb", 27, 9, BIT(2), "emmc"),
> -	PIN_GRP_GPIO("pwm0", 11, 1, BIT(3), "pwm"),
> -	PIN_GRP_GPIO("pwm1", 12, 1, BIT(4), "pwm"),
> -	PIN_GRP_GPIO("pwm2", 13, 1, BIT(5), "pwm"),
> -	PIN_GRP_GPIO("pwm3", 14, 1, BIT(6), "pwm"),
> +	PIN_GRP_GPIO_3("pwm0", 11, 1, BIT(3) | BIT(20), 0, BIT(20), BIT(3),
> +		       "pwm", "led"),
> +	PIN_GRP_GPIO_3("pwm1", 11, 1, BIT(4) | BIT(21), 0, BIT(21), BIT(4),
> +		       "pwm", "led"),
> +	PIN_GRP_GPIO_3("pwm2", 11, 1, BIT(5) | BIT(22), 0, BIT(22), BIT(5),
> +		       "pwm", "led"),
> +	PIN_GRP_GPIO_3("pwm3", 11, 1, BIT(6) | BIT(23), 0, BIT(23), BIT(6),
> +		       "pwm", "led"),
>   	PIN_GRP_GPIO("pmic1", 7, 1, BIT(7), "pmic"),
>   	PIN_GRP_GPIO("pmic0", 6, 1, BIT(8), "pmic"),
>   	PIN_GRP_GPIO("i2c2", 2, 2, BIT(9), "i2c"),
> @@ -177,11 +181,6 @@ static struct armada_37xx_pin_group armada_37xx_nb_groups[] = {
>   	PIN_GRP_EXTRA("uart2", 9, 2, BIT(1) | BIT(13) | BIT(14) | BIT(19),
>   		      BIT(1) | BIT(13) | BIT(14), BIT(1) | BIT(19),
>   		      18, 2, "gpio", "uart"),
> -	PIN_GRP_GPIO("led0_od", 11, 1, BIT(20), "led"),
> -	PIN_GRP_GPIO("led1_od", 12, 1, BIT(21), "led"),
> -	PIN_GRP_GPIO("led2_od", 13, 1, BIT(22), "led"),
> -	PIN_GRP_GPIO("led3_od", 14, 1, BIT(23), "led"),
> -
>   };
>   
>   static struct armada_37xx_pin_group armada_37xx_sb_groups[] = {
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-07-31 10:01 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-23 17:57 [PATCH u-boot-mvebu] arm64: a37xx: pinctrl: Correct PWM pins definitions Marek Behún
2021-07-31  7:33 ` Stefan Roese
2021-07-31 10:01 ` Stefan Roese

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.