Skip to contentSkip to conversation

gpio: realtek-otto: add pinctrl support

PATCH Applied Michael Zavertkin 6 participants 11 messages Aug 27 – Sep 14

Subsystems (2)
Device tree bindings GPIO
lore
Share & tools
lore

Michael Zavertkin

author
#0 lore raw

v4 cover gpio: realtek-otto: add pinctrl support

This series adds pinctrl support for RTL9607C and referes it in documentation.

Signed-off-by Michael Zavertkin <[email protected]>

---
Changes in v4:
- Fixed SoB mismatch
- Link to v3: https://patch.msgid.link/[email protected]

Changes in v3:
- 'gpio-ranges' is optional in dt-bindings
- pinctrl supported for all SoCs (no-op if disabled in configuration)
- Dropped Reviewed-by (patch changed)
- Link to v2: https://patch.msgid.link/[email protected]

Changes in v2:
- Add 'required' property to dt-bindings
- Dropped the ellipsis and added Reviewed-by
- Link to v1: https://patch.msgid.link/amRpvtNAwIQak_9y@gamepc
---
Michael Zavertkin (2):
dt-bindings: gpio: otto-gpio: Add gpio-ranges
gpio: realtek-otto: add pinctrl support

 .../devicetree/bindings/gpio/realtek,otto-gpio.yaml          | 2 ++
 drivers/gpio/gpio-realtek-otto.c                             | 5 +++--
 2 files changed, 5 insertions(+), 2 deletions(-)
Signature · 1 line
2.54.0
author
#1 lore raw

PATCH v4 1/2 dt-bindings: gpio: otto-gpio: Add gpio-ranges

Add 'gpio-ranges' optional property to reflect driver changes.

Signed-off-by Michael Zavertkin <[email protected]>
---
 Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml | 2 ++
 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml +2 -0
index b18f8f0ca0ae..33c25df7b815 100644
@@ -40,6 +40,8 @@ properties:
 
   gpio-controller: true
 
+  gpio-ranges: true
+
   ngpios:
     minimum: 1
     maximum: 32
Signature · 1 line
2.54.0
author
#4 lore raw

PATCH v4 2/2 gpio: realtek-otto: add pinctrl support

In reply to Michael Zavertkin

RTL9607C relies on pinctrl to mux SoC pins. Use pinctrl to request pins for GPIO.
pinctrl driver for other SoCs might be implemented later.

Signed-off-by Michael Zavertkin <[email protected]>
---
 drivers/gpio/gpio-realtek-otto.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/gpio/gpio-realtek-otto.c +3 -2
index 4a606bad5848..01be8baa25c3 100644
@@ -393,13 +393,14 @@ static int realtek_gpio_probe(struct platform_device *pdev)
 
 	raw_spin_lock_init(&ctrl->lock);
 
+	gen_gc_flags = GPIO_GENERIC_PINCTRL_BACKEND;
+
 	if (dev_flags & GPIO_PORTS_REVERSED) {
-		gen_gc_flags = 0;
 		ctrl->bank_read = realtek_gpio_bank_read;
 		ctrl->bank_write = realtek_gpio_bank_write;
 		ctrl->line_imr_pos = realtek_gpio_line_imr_pos;
 	} else {
-		gen_gc_flags = GPIO_GENERIC_BIG_ENDIAN_BYTE_ORDER;
+		gen_gc_flags |= GPIO_GENERIC_BIG_ENDIAN_BYTE_ORDER;
 		ctrl->bank_read = realtek_gpio_bank_read_swapped;
 		ctrl->bank_write = realtek_gpio_bank_write_swapped;
 		ctrl->line_imr_pos = realtek_gpio_line_imr_pos_swapped;
Signature · 1 line
2.54.0
AI review
#5 lore raw
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Unconditional use of GPIO_GENERIC_PINCTRL_BACKEND breaks GPIO requests and direction configuration…

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditional use of GPIO_GENERIC_PINCTRL_BACKEND breaks GPIO requests and direction configuration on SoCs without a pinctrl driver. --

commit e286232a1c082edc58e67d8d4c62156ff41a0294
Author: Michael Zavertkin <[email protected]>

gpio: realtek-otto: add pinctrl support

This commit adds pinctrl backend support to the realtek-otto GPIO driver so that RTL9607C can use pinctrl to request pins.

drivers/gpio/gpio-realtek-otto.c static int realtek_gpio_probe(struct platform_device *pdev) · line 398

Quoted 10 lines
drivers/gpio/gpio-realtek-otto.c +2 -0
index 37ef56f453187..b53c2a9a04c76 100644
@@ -394,13 +394,14 @@ static int realtek_gpio_probe(struct platform_device *pdev)
 
 	raw_spin_lock_init(&ctrl->lock);
 
+	gen_gc_flags = GPIO_GENERIC_PINCTRL_BACKEND;
+

[Severity: High]
Does unconditionally setting GPIO_GENERIC_PINCTRL_BACKEND break GPIO functionality for SoCs that lack a pinctrl driver?

When a GPIO is requested or its direction is set on an SoC without a pinctrl driver, gpio_generic_chip_init() routes the calls to pinctrl_gpio_request() and pinctrl_gpio_direction().

Because pinctrl_gpio_direction() calls pinctrl_get_device_gpio_range(), it will return -EPROBE_DEFER when no pinctrl device overlaps with the GPIO chip. This would cause all GPIO requests and direction configurations to fail indefinitely, rendering GPIO unusable and breaking dependent devices on hardware like RTL8380 or RTL9300, which the commit message acknowledges might have pinctrl drivers implemented later.

Should this backend flag only be set conditionally, perhaps by checking for gpio-ranges in the device tree?

Quoted 11 lines
drivers/gpio/gpio-realtek-otto.c +1 -2
 	if (dev_flags & GPIO_PORTS_REVERSED) {
-		gen_gc_flags = 0;
 		ctrl->bank_read = realtek_gpio_bank_read;
 		ctrl->bank_write = realtek_gpio_bank_write;
 		ctrl->line_imr_pos = realtek_gpio_line_imr_pos;
 	} else {
-		gen_gc_flags = GPIO_GENERIC_BIG_ENDIAN_BYTE_ORDER;
+		gen_gc_flags |= GPIO_GENERIC_BIG_ENDIAN_BYTE_ORDER;
 		ctrl->bank_read = realtek_gpio_bank_read_swapped;
 		ctrl->bank_write = realtek_gpio_bank_write_swapped;
 		ctrl->line_imr_pos = realtek_gpio_line_imr_pos_swapped;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
maintainer
#6 lore raw
In reply to Michael Zavertkin
Quoted 5 lines · Michael Zavertkin

RTL9607C relies on pinctrl to mux SoC pins. Use pinctrl to request pins for GPIO.
pinctrl driver for other SoCs might be implemented later.

So for now shouldn't you make it conditional on the compatible for this one model?

Bart

author
#7 lore raw
Quoted 8 lines · Bartosz Golaszewski
Quoted 5 lines · Michael Zavertkin

RTL9607C relies on pinctrl to mux SoC pins. Use pinctrl to request pins for GPIO.
pinctrl driver for other SoCs might be implemented later.

Quoted 4 lines

So for now shouldn't you make it conditional on the compatible for this one model?

Bart

It might, but enabling it for all will spare time if/when pin controllers for other SoCs will be implemented. And it doesn't change anything if pinctrl not enabled/provided for them.

maintainer
#8 lore raw
Quoted 9 lines · Michael Zavertkin
Quoted 8 lines · Bartosz Golaszewski
Quoted 5 lines · Michael Zavertkin

RTL9607C relies on pinctrl to mux SoC pins. Use pinctrl to request pins for GPIO.
pinctrl driver for other SoCs might be implemented later.

Quoted 10 lines
Quoted 4 lines

So for now shouldn't you make it conditional on the compatible for this one model?

Bart

It might, but enabling it for all will spare time if/when pin controllers for other SoCs will be implemented. And it doesn't change anything if pinctrl not enabled/provided for them.

Sashiko claims otherwise[1] so I had a look and it seems to me that it's correct saying we'll defer indefinitely for GPIO chips not backed by a pin controller.

Bart

[1] https://sashiko.dev/#/patchset/cover.1787830440.git.misha.zavertkin%40mail.ru

author
#9 lore raw
Quoted 22 lines · Bartosz Golaszewski
Quoted 9 lines · Michael Zavertkin
Quoted 8 lines · Bartosz Golaszewski
Quoted 5 lines · Michael Zavertkin

RTL9607C relies on pinctrl to mux SoC pins. Use pinctrl to request pins for GPIO.
pinctrl driver for other SoCs might be implemented later.

Quoted 10 lines
Quoted 4 lines

So for now shouldn't you make it conditional on the compatible for this one model?

Bart

It might, but enabling it for all will spare time if/when pin controllers for other SoCs will be implemented. And it doesn't change anything if pinctrl not enabled/provided for them.

Quoted 7 lines

Sashiko claims otherwise[1] so I had a look and it seems to me that it's correct saying we'll defer indefinitely for GPIO chips not backed by a pin controller.

Bart

[1] https://sashiko.dev/#/patchset/cover.1787830440.git.misha.zavertkin%40mail.ru

Already addressed: https://patch.msgid.link/[email protected]

Tried it myself, without that patch infinite defers happens if no pinctrl provided. After patch applied probe loop doesn't happen.

Applied
#10 lore raw
In reply to Michael Zavertkin
Quoted 4 lines · Michael Zavertkin

This series adds pinctrl support for RTL9607C and referes it in documentation.

Applied, thanks!

[1/2] dt-bindings: gpio: otto-gpio: Add gpio-ranges

      https://git.kernel.org/brgl/c/3f860d25f7be981d78c1b85a8d4fafcd914d4df9
[2/2] gpio: realtek-otto: add pinctrl support
      https://git.kernel.org/brgl/c/7257c35db0fdb4fb02d857a8197a16afa84d92c0

Best regards,

Signature · 1 line
Bartosz Golaszewski <[email protected]>