Skip to contentSkip to conversation

Add gpio driver support for Realtek DHC SoCs

PATCH Superseded Tzuyi Chang 3 participants 12 messages Nov 1, 2023 – Nov 3, 2023

Subsystems (2)
Device tree bindings GPIO

v1 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Tzuyi Chang

author
#0 lore raw

cover Add gpio driver support for Realtek DHC SoCs

These patches add the bindings and the gpio driver for Realtek DHC(Digital Home Center) RTD SoCs, including RTD1295, RTD1395, RTD1619, RTD1319, RTD1619B, RTD1319D and RTD1315E.

Tzuyi Chang (2):
gpio: realtek: Add GPIO support for RTD SoC variants
dt-bindings: gpio: realtek: Add realtek,rtd-gpio bindings

 .../bindings/gpio/realtek,rtd-gpio.yaml       |  56 ++
 drivers/gpio/Kconfig                          |   8 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-rtd.c                       | 702 ++++++++++++++++++
 4 files changed, 767 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml
 create mode 100644 drivers/gpio/gpio-rtd.c
Signature · 1 line
2.42.0
author
#1 lore raw

PATCH 1/2 gpio: realtek: Add GPIO support for RTD SoC variants

This commit adds GPIO support for Realtek DHC RTD SoCs.

This driver enables configuration of GPIO direction, GPIO values, GPIO debounce settings and handles GPIO interrupts.

Signed-off-by Tzuyi Chang <[email protected]>
---
 drivers/gpio/Kconfig    |   8 +
 drivers/gpio/Makefile   |   1 +
 drivers/gpio/gpio-rtd.c | 702 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 711 insertions(+)
 create mode 100644 drivers/gpio/gpio-rtd.c
drivers/gpio/Kconfig +8 -0
index 673bafb8be58..8d55c1ce8f3b 100644
@@ -553,6 +553,14 @@ config GPIO_ROCKCHIP
 	help
 	  Say yes here to support GPIO on Rockchip SoCs.
 
+config GPIO_RTD
+	tristate "Realtek DHC GPIO support"
+	depends on ARCH_REALTEK
+	default y
+	select GPIOLIB_IRQCHIP
+	help
+	  Say yes here to support GPIO on Realtek DHC SoCs.
+
 config GPIO_SAMA5D2_PIOBU
 	tristate "SAMA5D2 PIOBU GPIO support"
 	depends on MFD_SYSCON
drivers/gpio/Makefile +1 -0
index eb73b5d633eb..16bb40717e87 100644
@@ -137,6 +137,7 @@ obj-$(CONFIG_GPIO_RDC321X)		+= gpio-rdc321x.o
 obj-$(CONFIG_GPIO_REALTEK_OTTO)		+= gpio-realtek-otto.o
 obj-$(CONFIG_GPIO_REG)			+= gpio-reg.o
 obj-$(CONFIG_GPIO_ROCKCHIP)	+= gpio-rockchip.o
+obj-$(CONFIG_GPIO_RTD)			+= gpio-rtd.o
 obj-$(CONFIG_ARCH_SA1100)		+= gpio-sa1100.o
 obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)	+= gpio-sama5d2-piobu.o
 obj-$(CONFIG_GPIO_SCH311X)		+= gpio-sch311x.o
drivers/gpio/gpio-rtd.c new file +702 -0

702 lines not on this page · raw message

Signature · 1 line
2.42.0
NAK
#2 lore raw

Re: [PATCH 1/2] gpio: realtek: Add GPIO support for RTD SoC variants

Quoted 1 line · Tzuyi Chang

This commit adds GPIO support for Realtek DHC RTD SoCs.

Please do not use "This commit/patch", but imperative mood. See longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

Quoted 6 lines

This driver enables configuration of GPIO direction, GPIO values, GPIO debounce settings and handles GPIO interrupts.

Signed-off-by Tzuyi Chang <[email protected]>

---

...

Quoted 15 lines
drivers/gpio/gpio-rtd.c new file +15 -0
+
+static int rtd_gpio_probe(struct platform_device *pdev)
+{
+	struct rtd_gpio *data;
+	const struct of_device_id *match;
+	struct device_node *node;
+	int ret;
+	int i;
+
+	node = pdev->dev.of_node;
+	match = of_match_node(rtd_gpio_of_matches, pdev->dev.of_node);
+	if (!match || !match->data)
+		return -EINVAL;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);

drivers/gpio/gpio-rtd.c line 618

Quoted 10 lines
drivers/gpio/gpio-rtd.c new file +10 -0
+	if (!data)
+		return -ENOMEM;
+
+	data->assert_irq = irq_of_parse_and_map(node, 0);
+	if (!data->assert_irq)
+		goto deferred;
+
+	data->deassert_irq = irq_of_parse_and_map(node, 1);
+	if (!data->deassert_irq)
+		goto deferred;

So this goes to cleanup path...

drivers/gpio/gpio-rtd.c line 625

Quoted 7 lines
drivers/gpio/gpio-rtd.c new file +7 -0
+
+	data->info = match->data;
+	spin_lock_init(&data->lock);
+
+	data->base = of_iomap(node, 0);
+	if (!data->base)
+		return -ENXIO;

But this does not? What?

Quoted 15 lines
drivers/gpio/gpio-rtd.c new file +15 -0
+
+	data->irq_base = of_iomap(node, 1);
+	if (!data->irq_base)
+		return -ENXIO;
+
+	data->gpio_chip.parent = &pdev->dev;
+	data->gpio_chip.label = dev_name(&pdev->dev);
+	data->gpio_chip.of_gpio_n_cells = 2;
+	data->gpio_chip.base = data->info->gpio_base;
+	data->gpio_chip.ngpio = data->info->num_gpios;
+	data->gpio_chip.request = rtd_gpio_request;
+	data->gpio_chip.free = rtd_gpio_free;
+	data->gpio_chip.get_direction = rtd_gpio_get_direction;
+	data->gpio_chip.direction_input = rtd_gpio_direction_input;
+	data->gpio_chip.direction_output = rtd_gpio_direction_output;

drivers/gpio/gpio-rtd.c line 650

Quoted 10 lines
drivers/gpio/gpio-rtd.c new file +10 -0
+	data->gpio_chip.set = rtd_gpio_set;
+	data->gpio_chip.get = rtd_gpio_get;
+	data->gpio_chip.set_config = rtd_gpio_set_config;
+	data->gpio_chip.to_irq = rtd_gpio_to_irq;
+	data->irq_chip = rtd_gpio_irq_chip;
+	data->irq_chip.name = data->info->name;
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &data->gpio_chip, data);
+	if (ret) {
+		dev_err(&pdev->dev, "Adding GPIO chip failed (%d)\n", ret);

And here no cleanup? It's some random choice.

drivers/gpio/gpio-rtd.c line 657

Quoted 7 lines
drivers/gpio/gpio-rtd.c new file +7 -0
+		return ret;
+	}
+
+	data->domain = irq_domain_add_linear(node, data->gpio_chip.ngpio,
+				&irq_domain_simple_ops, data);
+	if (!data->domain) {
+		devm_kfree(&pdev->dev, data);

NAK, test your patch.

Quoted 4 lines
drivers/gpio/gpio-rtd.c new file +4 -0
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < data->gpio_chip.ngpio; i++) {

drivers/gpio/gpio-rtd.c line 671

Quoted 10 lines
drivers/gpio/gpio-rtd.c new file +10 -0
+		int irq = irq_create_mapping(data->domain, i);
+
+		irq_set_chip_data(irq, data);
+		irq_set_chip_and_handler(irq, &data->irq_chip, handle_simple_irq);
+	}
+
+	irq_set_chained_handler_and_data(data->assert_irq, rtd_gpio_assert_irq_handle, data);
+	irq_set_chained_handler_and_data(data->deassert_irq, rtd_gpio_deassert_irq_handle, data);
+
+	dev_dbg(&pdev->dev, "probed\n");

Nop, drop all silly, simple entry/exit messages.

drivers/gpio/gpio-rtd.c line 676

Quoted 5 lines
drivers/gpio/gpio-rtd.c new file +5 -0
+
+	return 0;
+
+deferred:
+	devm_kfree(&pdev->dev, data);

NAK, test your patch.

Quoted 16 lines
drivers/gpio/gpio-rtd.c new file +16 -0
+	return -EPROBE_DEFER;
+}
+
+static struct platform_driver rtd_gpio_platform_driver = {
+	.driver = {
+		.name = "gpio-rtd",
+		.of_match_table = rtd_gpio_of_matches,
+	},
+	.probe = rtd_gpio_probe,
+};
+
+static int rtd_gpio_init(void)
+{
+	return platform_driver_register(&rtd_gpio_platform_driver);
+}
+

drivers/gpio/gpio-rtd.c line 702

Quoted 10 lines
drivers/gpio/gpio-rtd.c new file +10 -0
+subsys_initcall(rtd_gpio_init);
+
+static void __exit rtd_gpio_exit(void)
+{
+	platform_driver_unregister(&rtd_gpio_platform_driver);
+}
+module_exit(rtd_gpio_exit);
+
+MODULE_DESCRIPTION("Realtek DHC SoC gpio driver");
+MODULE_LICENSE("GPL v2");

Best regards,
Krzysztof

author
#3 lore raw

RE: [PATCH 1/2] gpio: realtek: Add GPIO support for RTD SoC variants

Hi Krzysztof,

Quoted 8 lines · Tzuyi Chang
Quoted 1 line

This commit adds GPIO support for Realtek DHC RTD SoCs.

Please do not use "This commit/patch", but imperative mood. See longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-
patches.rst#L95

I will remove these words.

Quoted 17 lines
Quoted 17 lines
drivers/gpio/gpio-rtd.c new file +17 -0
+static int rtd_gpio_probe(struct platform_device *pdev) {
+     struct rtd_gpio *data;
+     const struct of_device_id *match;
+     struct device_node *node;
+     int ret;
+     int i;
+
+     node = pdev->dev.of_node;
+     match = of_match_node(rtd_gpio_of_matches, pdev->dev.of_node);
+     if (!match || !match->data)
+             return -EINVAL;
+
+     data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+     if (!data)
+             return -ENOMEM;
+
+     data->assert_irq = irq_of_parse_and_map(node, 0);
Quoted 9 lines

drivers/gpio/gpio-rtd.c line 618

Quoted 6 lines
drivers/gpio/gpio-rtd.c new file +6 -0
+     if (!data->assert_irq)
+             goto deferred;
+
+     data->deassert_irq = irq_of_parse_and_map(node, 1);
+     if (!data->deassert_irq)
+             goto deferred;

So this goes to cleanup path...

Since there is no need to do devm_free, I will directly return -EPROBE_DEFER here.

Quoted 47 lines

drivers/gpio/gpio-rtd.c line 625

Quoted 7 lines
drivers/gpio/gpio-rtd.c new file +7 -0
+
+     data->info = match->data;
+     spin_lock_init(&data->lock);
+
+     data->base = of_iomap(node, 0);
+     if (!data->base)
+             return -ENXIO;

But this does not? What?

Quoted 16 lines
drivers/gpio/gpio-rtd.c new file +16 -0
+
+     data->irq_base = of_iomap(node, 1);
+     if (!data->irq_base)
+             return -ENXIO;
+
+     data->gpio_chip.parent = &pdev->dev;
+     data->gpio_chip.label = dev_name(&pdev->dev);
+     data->gpio_chip.of_gpio_n_cells = 2;
+     data->gpio_chip.base = data->info->gpio_base;
+     data->gpio_chip.ngpio = data->info->num_gpios;
+     data->gpio_chip.request = rtd_gpio_request;
+     data->gpio_chip.free = rtd_gpio_free;
+     data->gpio_chip.get_direction = rtd_gpio_get_direction;
+     data->gpio_chip.direction_input = rtd_gpio_direction_input;
+     data->gpio_chip.direction_output = rtd_gpio_direction_output;
+     data->gpio_chip.set = rtd_gpio_set;

drivers/gpio/gpio-rtd.c

Quoted 10 lines
drivers/gpio/gpio-rtd.c new file +10 -0
+     data->gpio_chip.get = rtd_gpio_get;
+     data->gpio_chip.set_config = rtd_gpio_set_config;
+     data->gpio_chip.to_irq = rtd_gpio_to_irq;
+     data->irq_chip = rtd_gpio_irq_chip;
+     data->irq_chip.name = data->info->name;
+
+     ret = devm_gpiochip_add_data(&pdev->dev, &data->gpio_chip, data);
+     if (ret) {
+             dev_err(&pdev->dev, "Adding GPIO chip failed (%d)\n",
+ ret);

And here no cleanup? It's some random choice.

drivers/gpio/gpio-rtd.c line 657

Quoted 7 lines
drivers/gpio/gpio-rtd.c new file +7 -0
+             return ret;
+     }
+
+     data->domain = irq_domain_add_linear(node, data->gpio_chip.ngpio,
+                             &irq_domain_simple_ops, data);
+     if (!data->domain) {
+             devm_kfree(&pdev->dev, data);
Quoted 2 lines

NAK, test your patch.

I will remove it.

Quoted 12 lines

drivers/gpio/gpio-rtd.c

Quoted 8 lines
drivers/gpio/gpio-rtd.c new file +8 -0
+             return -ENOMEM;
+     }
+
+     for (i = 0; i < data->gpio_chip.ngpio; i++) {
+             int irq = irq_create_mapping(data->domain, i);
+
+             irq_set_chip_data(irq, data);
+             irq_set_chip_and_handler(irq, &data->irq_chip,

handle_simple_irq);

drivers/gpio/gpio-rtd.c

Quoted 3 lines
drivers/gpio/gpio-rtd.c new file +3 -0
+     }
+
+     irq_set_chained_handler_and_data(data->assert_irq,
Quoted 8 lines

rtd_gpio_assert_irq_handle, data);

drivers/gpio/gpio-rtd.c line 671

Quoted 4 lines
drivers/gpio/gpio-rtd.c new file +4 -0
+     irq_set_chained_handler_and_data(data->deassert_irq,
+ rtd_gpio_deassert_irq_handle, data);
+
+     dev_dbg(&pdev->dev, "probed\n");

Nop, drop all silly, simple entry/exit messages.

I will remove it.

Quoted 9 lines

drivers/gpio/gpio-rtd.c line 676

Quoted 5 lines
drivers/gpio/gpio-rtd.c new file +5 -0
+
+     return 0;
+
+deferred:
+     devm_kfree(&pdev->dev, data);

NAK, test your patch.

Quoted 1 line

+ return -EPROBE_DEFER;

I will remove this label.

Quoted 4 lines
Quoted 1 line

+}

Best regards,
Krzysztof

Thanks,
Tzuyi Chang

#4 lore raw

Re: [PATCH 1/2] gpio: realtek: Add GPIO support for RTD SoC variants

Quoted 40 lines · TY_Chang[張子逸]

Hi Krzysztof,

Quoted 8 lines · Tzuyi Chang
Quoted 1 line

This commit adds GPIO support for Realtek DHC RTD SoCs.

Please do not use "This commit/patch", but imperative mood. See longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-
patches.rst#L95

I will remove these words.

Quoted 17 lines
Quoted 17 lines
drivers/gpio/gpio-rtd.c new file +17 -0
+static int rtd_gpio_probe(struct platform_device *pdev) {
+     struct rtd_gpio *data;
+     const struct of_device_id *match;
+     struct device_node *node;
+     int ret;
+     int i;
+
+     node = pdev->dev.of_node;
+     match = of_match_node(rtd_gpio_of_matches, pdev->dev.of_node);
+     if (!match || !match->data)
+             return -EINVAL;
+
+     data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+     if (!data)
+             return -ENOMEM;
+
+     data->assert_irq = irq_of_parse_and_map(node, 0);
Quoted 9 lines

drivers/gpio/gpio-rtd.c line 618

Quoted 6 lines
drivers/gpio/gpio-rtd.c new file +6 -0
+     if (!data->assert_irq)
+             goto deferred;
+
+     data->deassert_irq = irq_of_parse_and_map(node, 1);
+     if (!data->deassert_irq)
+             goto deferred;

So this goes to cleanup path...

Quoted 1 line

Since there is no need to do devm_free, I will directly return -EPROBE_DEFER here.

That's not a correct return value. You do not return DEFER on missing IRQ. This should anyway be different call: platform_get_irq().

Best regards,
Krzysztof

author
#5 lore raw

RE: [PATCH 1/2] gpio: realtek: Add GPIO support for RTD SoC variants

Hi Krzysztof,

Quoted 27 lines
Quoted 27 lines
Quoted 26 lines
Quoted 13 lines
drivers/gpio/gpio-rtd.c new file +13 -0
+static int rtd_gpio_probe(struct platform_device *pdev) {
+     struct rtd_gpio *data;
+     const struct of_device_id *match;
+     struct device_node *node;
+     int ret;
+     int i;
+
+     node = pdev->dev.of_node;
+     match = of_match_node(rtd_gpio_of_matches, pdev->dev.of_node);
+     if (!match || !match->data)
+             return -EINVAL;
+
+     data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);

drivers/gpio/gpio-rtd.c line 618

Quoted 10 lines
drivers/gpio/gpio-rtd.c new file +10 -0
+     if (!data)
+             return -ENOMEM;
+
+     data->assert_irq = irq_of_parse_and_map(node, 0);
+     if (!data->assert_irq)
+             goto deferred;
+
+     data->deassert_irq = irq_of_parse_and_map(node, 1);
+     if (!data->deassert_irq)
+             goto deferred;

So this goes to cleanup path...

Quoted 6 lines
Quoted 1 line

Since there is no need to do devm_free, I will directly return -EPROBE_DEFER

here.

That's not a correct return value. You do not return DEFER on missing IRQ. This should anyway be different call: platform_get_irq().

I got it . Thank you for the reminder.

Thanks,
Tzuyi Chang

#6 lore raw

Re: [PATCH 1/2] gpio: realtek: Add GPIO support for RTD SoC variants

In reply to Tzuyi Chang

Hi Tzuyi!

thanks for your patch!

Quoted 1 line · Tzuyi Chang

This commit adds GPIO support for Realtek DHC RTD SoCs.

What does "DHC" mean? Please spell it out in the commit and Kconfig so we know what it is.

Quoted 4 lines

This driver enables configuration of GPIO direction, GPIO values, GPIO debounce settings and handles GPIO interrupts.

Signed-off-by Tzuyi Chang <[email protected]>

(...)

drivers/gpio/Kconfig config GPIO_ROCKCHIP · line 562

Quoted 7 lines
drivers/gpio/Kconfig +7 -0
+config GPIO_RTD
+       tristate "Realtek DHC GPIO support"
+       depends on ARCH_REALTEK
+       default y
+       select GPIOLIB_IRQCHIP
+       help
+         Say yes here to support GPIO on Realtek DHC SoCs.

Explain what DHC is i.e. the acronym expansion, family, use case or something.

drivers/gpio/gpio-rtd.c line 9

Quoted 2 lines
drivers/gpio/gpio-rtd.c new file +2 -0
+#include <linux/bitops.h>
+#include <linux/gpio.h>

Do not include this legacy header.
Include <linux/gpio/driver.h>

drivers/gpio/gpio-rtd.c line 18

Quoted 9 lines
drivers/gpio/gpio-rtd.c new file +9 -0
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_gpio.h>
+#include <linux/of_irq.h>

I don't think you need any of thexe of_* includes. Try it without them.

Quoted 1 line

+#include <linux/pinctrl/consumer.h>

Why?

Quoted 18 lines
drivers/gpio/gpio-rtd.c new file +18 -0
+/**
+ * struct rtd_gpio_info - Specific GPIO register information
+ * @name: GPIO device name
+ * @type: RTD GPIO ID
+ * @gpio_base: GPIO base number
+ * @num_gpios: Number of GPIOs
+ * @dir_offset: Offset for GPIO direction registers
+ * @dato_offset: Offset for GPIO data output registers
+ * @dati_offset: Offset for GPIO data input registers
+ * @ie_offset: Offset for GPIO interrupt enable registers
+ * @dp_offset: Offset for GPIO detection polarity registers
+ * @gpa_offset: Offset for GPIO assert interrupt status registers
+ * @gpda_offset: Offset for GPIO deassert interrupt status registers
+ * @deb_offset: Offset for GPIO debounce registers
+ */
+struct rtd_gpio_info {
+       const char *name;
+       enum rtd_gpio_type type;

drivers/gpio/gpio-rtd.c line 66

Quoted 10 lines
drivers/gpio/gpio-rtd.c new file +10 -0
+       unsigned int gpio_base;
+       unsigned int num_gpios;
+       unsigned int *dir_offset;
+       unsigned int *dato_offset;
+       unsigned int *dati_offset;
+       unsigned int *ie_offset;
+       unsigned int *dp_offset;
+       unsigned int *gpa_offset;
+       unsigned int *gpda_offset;
+       unsigned int *deb_offset;

Use u8 instead of unsigned int for the offsets. It is clear from the arrays you assign them that they are all u8[].

drivers/gpio/gpio-rtd.c line 75

Quoted 7 lines
drivers/gpio/gpio-rtd.c new file +7 -0
+struct rtd_gpio {
+       struct platform_device *pdev;
+       const struct rtd_gpio_info *info;
+       void __iomem *base;
+       void __iomem *irq_base;
+       struct gpio_chip gpio_chip;
+       struct irq_chip irq_chip;

Do not use a dynamic irq_chip, create an immutable irq_chip using a const struct.

See recent commits and virtually all current drivers in the tree for examples on how to do that.

drivers/gpio/gpio-rtd.c line 77

Quoted 2 lines
drivers/gpio/gpio-rtd.c new file +2 -0
+       int assert_irq;
+       int deassert_irq;

I don't quite understand these two, but let's see in the rest of the driver.

Quoted 1 line

+ .deb_offset = (unsigned int []){ 0x30, 0x34, 0x38, 0x3c, 0x40, 0x44, 0x48, 0x4c },

(...)

Quoted 1 line

+ .deb_offset = (unsigned int []){ 0x50 },

So clearly u8[]

drivers/gpio/gpio-rtd.c line 198

Quoted 4 lines
drivers/gpio/gpio-rtd.c new file +4 -0
+static unsigned int rtd_gpio_deb_offset(struct rtd_gpio *data, unsigned int offset)
+{
+       return data->info->deb_offset[offset / 8];
+}

So this is clearly counted by the GPIO number offset and the GPIO number determines how far into the array we can index.

It looks a bit dangerous, it it possible to encode the array lengths better?

Quoted 5 lines
drivers/gpio/gpio-rtd.c new file +5 -0
+       if (data->info->type == RTD1295_ISO_GPIO) {
+               shift = 0;
+               deb_val += 1;
+               write_en = BIT(shift + 3);
+               reg_offset = rtd1295_gpio_deb_offset(data, offset);

drivers/gpio/gpio-rtd.c line 256

Quoted 10 lines
drivers/gpio/gpio-rtd.c new file +10 -0
+       } else if (data->info->type == RTD1295_MISC_GPIO) {
+               shift = (offset >> 4) * 4;
+               deb_val += 1;
+               write_en = BIT(shift + 3);
+               reg_offset = rtd1295_gpio_deb_offset(data, offset);
+       } else {
+               shift = (offset % 8) * 4;
+               write_en = BIT(shift + 3);
+               reg_offset = rtd_gpio_deb_offset(data, offset);
+       }

These three different offset functions seem a bit awkward. Can we do this by just another index instead?

drivers/gpio/gpio-rtd.c line 292

Quoted 9 lines
drivers/gpio/gpio-rtd.c new file +9 -0
+static int rtd_gpio_request(struct gpio_chip *chip, unsigned int offset)
+{
+       return pinctrl_gpio_request(chip->base + offset);
+}
+
+static void rtd_gpio_free(struct gpio_chip *chip, unsigned int offset)
+{
+       pinctrl_gpio_free(chip->base + offset);
+}

IIRC Bartosz has changed this for kernel v6.7, please check his upstream commits and adjust the code accordingly.

drivers/gpio/gpio-rtd.c line 408

Quoted 13 lines
drivers/gpio/gpio-rtd.c new file +13 -0
+static int rtd_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+       struct rtd_gpio *data = gpiochip_get_data(chip);
+       u32 irq = 0;
+
+       irq = irq_find_mapping(data->domain, offset);
+       if (!irq) {
+               dev_err(&data->pdev->dev, "%s: can not find irq number for hwirq= %d\n",
+                       __func__, offset);
+               return -EINVAL;
+       }
+       return irq;
+}

Don't implement your own gpio_to_irq, just use the GPIOLIB_IRQCHIP helpers. See other drivers that select GPIOLIB_IRQCHIP, this driver is nothing special.

Quoted 6 lines
drivers/gpio/gpio-rtd.c new file +6 -0
+       chained_irq_enter(chip, desc);
+
+       for (i = 0; i < data->info->num_gpios; i = i + 31) {
+               gpa_reg_offset = rtd_gpio_gpa_offset(data, i);
+               status = readl_relaxed(data->irq_base + gpa_reg_offset) >> 1;
+               writel_relaxed(status << 1, data->irq_base + gpa_reg_offset);

drivers/gpio/gpio-rtd.c line 447

Quoted 10 lines
drivers/gpio/gpio-rtd.c new file +10 -0
+
+               while (status) {
+                       j = __ffs(status);
+                       status &= ~BIT(j);
+                       hwirq = i + j;
+                       if (rtd_gpio_check_ie(data, hwirq)) {
+                               int irq = irq_find_mapping(data->domain, hwirq);
+
+                               generic_handle_irq(irq);
+                       }

So you skip the interrupt handler if the interrupt is not enabled?

I think you should report spurious interrupts if they occur without being enabled, unless there is some hardware flunky making these lines flicker with noise interrupts too much.

Quoted 23 lines
drivers/gpio/gpio-rtd.c new file +23 -0
+static void rtd_gpio_deassert_irq_handle(struct irq_desc *desc)
+{
+       struct rtd_gpio *data = irq_desc_get_handler_data(desc);
+       struct irq_chip *chip = irq_desc_get_chip(desc);
+       unsigned int gpda_reg_offset;
+       u32 status;
+       int hwirq;
+       int i;
+       int j;
+
+       chained_irq_enter(chip, desc);
+
+       for (i = 0; i < data->info->num_gpios; i = i + 31) {
+               gpda_reg_offset = rtd_gpio_gpda_offset(data, i);
+               status = readl_relaxed(data->irq_base + gpda_reg_offset) >> 1;
+               writel_relaxed(status << 1, data->irq_base + gpda_reg_offset);
+
+               while (status) {
+                       j = __ffs(status);
+                       status &= ~BIT(j);
+                       hwirq = i + j;
+                       if (rtd_gpio_check_ie(data, hwirq)) {
+                               int irq = irq_find_mapping(data->domain, hwirq);

drivers/gpio/gpio-rtd.c line 486

Quoted 10 lines
drivers/gpio/gpio-rtd.c new file +10 -0
+                               u32 irq_type = irq_get_trigger_type(irq);
+
+                               if ((irq_type & IRQ_TYPE_SENSE_MASK) == IRQ_TYPE_EDGE_BOTH)
+                                       generic_handle_irq(irq);
+                       }
+               }
+       }
+
+       chained_irq_exit(chip, desc);
+}

There is some code duplication here. Create wrapper calls with parameters so you don't need to have several functions that look almost the same.

drivers/gpio/gpio-rtd.c line 599

Quoted 5 lines
drivers/gpio/gpio-rtd.c new file +5 -0
+static int rtd_gpio_probe(struct platform_device *pdev)
+{
+       struct rtd_gpio *data;
+       const struct of_device_id *match;
+       struct device_node *node;

Don't go looking by the OF node, use the device:

struct device *dev = &pdev->dev;

drivers/gpio/gpio-rtd.c line 603

Quoted 4 lines
drivers/gpio/gpio-rtd.c new file +4 -0
+       int ret;
+       int i;
+
+       node = pdev->dev.of_node;

Use #include <linux/property.h>

drivers/gpio/gpio-rtd.c line 606

Quoted 3 lines
drivers/gpio/gpio-rtd.c new file +3 -0
+       match = of_match_node(rtd_gpio_of_matches, pdev->dev.of_node);
+       if (!match || !match->data)
+               return -EINVAL;

Use
data->info = device_get_match_data(dev); instead
if (!data->info)...

drivers/gpio/gpio-rtd.c line 608

Quoted 2 lines
drivers/gpio/gpio-rtd.c new file +2 -0
+
+       data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);

With a local dev you can just devm_kzalloc(dev, ...) etc.

drivers/gpio/gpio-rtd.c line 618

Quoted 7 lines
drivers/gpio/gpio-rtd.c new file +7 -0
+       data->assert_irq = irq_of_parse_and_map(node, 0);
+       if (!data->assert_irq)
+               goto deferred;
+
+       data->deassert_irq = irq_of_parse_and_map(node, 1);
+       if (!data->deassert_irq)
+               goto deferred;

So one handler for rising and one handler for falling edge? Hm that's different. I guess you need separate handlers.

drivers/gpio/gpio-rtd.c line 625

Quoted 3 lines
drivers/gpio/gpio-rtd.c new file +3 -0
+       data->base = of_iomap(node, 0);
+       if (!data->base)
+               return -ENXIO;

Use
data->base = devm_platform_ioremap_resource(pdev, 0);

drivers/gpio/gpio-rtd.c line 629

Quoted 3 lines
drivers/gpio/gpio-rtd.c new file +3 -0
+       data->irq_base = of_iomap(node, 1);
+       if (!data->irq_base)
+               return -ENXIO;

Use
data->irq_base = platform_get_irq(pdev, 1);

Quoted 1 line

+ data->gpio_chip.parent = &pdev->dev;

Don't assign this, the core will handle it.

drivers/gpio/gpio-rtd.c line 633

Quoted 2 lines
drivers/gpio/gpio-rtd.c new file +2 -0
+       data->gpio_chip.label = dev_name(&pdev->dev);
+       data->gpio_chip.of_gpio_n_cells = 2;

This is the default, let the core handle OF translation.

drivers/gpio/gpio-rtd.c line 644

Quoted 11 lines
drivers/gpio/gpio-rtd.c new file +11 -0
+       data->gpio_chip.base = data->info->gpio_base;
+       data->gpio_chip.ngpio = data->info->num_gpios;
+       data->gpio_chip.request = rtd_gpio_request;
+       data->gpio_chip.free = rtd_gpio_free;
+       data->gpio_chip.get_direction = rtd_gpio_get_direction;
+       data->gpio_chip.direction_input = rtd_gpio_direction_input;
+       data->gpio_chip.direction_output = rtd_gpio_direction_output;
+       data->gpio_chip.set = rtd_gpio_set;
+       data->gpio_chip.get = rtd_gpio_get;
+       data->gpio_chip.set_config = rtd_gpio_set_config;
+       data->gpio_chip.to_irq = rtd_gpio_to_irq;

Use the GPIOLIB_IRQCHIP to provide this for you.

Quoted 1 line

+ data->irq_chip = rtd_gpio_irq_chip;

Convert to use immutable irq_chip. (Maybe several struct irq_chip if you need!)

Quoted 6 lines
drivers/gpio/gpio-rtd.c new file +6 -0
+       data->domain = irq_domain_add_linear(node, data->gpio_chip.ngpio,
+                               &irq_domain_simple_ops, data);
+       if (!data->domain) {
+               devm_kfree(&pdev->dev, data);
+               return -ENOMEM;
+       }

drivers/gpio/gpio-rtd.c line 669

Quoted 10 lines
drivers/gpio/gpio-rtd.c new file +10 -0
+
+       for (i = 0; i < data->gpio_chip.ngpio; i++) {
+               int irq = irq_create_mapping(data->domain, i);
+
+               irq_set_chip_data(irq, data);
+               irq_set_chip_and_handler(irq, &data->irq_chip, handle_simple_irq);
+       }
+
+       irq_set_chained_handler_and_data(data->assert_irq, rtd_gpio_assert_irq_handle, data);
+       irq_set_chained_handler_and_data(data->deassert_irq, rtd_gpio_deassert_irq_handle, data);

Instead of doing this use GPIOLIB_IRQCHIP.

Before registering the gpio_chip set up stuff somewhat like this:

        girq = &data->gpio_chip.irq;
        gpio_irq_chip_set_chip(girq, &my_irq_chip);
        girq->parent_handler = my_gpio_irq_handler;
        girq->num_parents = 1;
        girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
                                     GFP_KERNEL);
        if (!girq->parents)
                ret = -ENOMEM;
        girq->default_type = IRQ_TYPE_NONE;
        girq->handler = handle_bad_irq;
        girq->parents[0] = irq;

But maybe in this case you want two parent IRQs? Not sure.

drivers/gpio/gpio-rtd.c line 677

Quoted 3 lines
drivers/gpio/gpio-rtd.c new file +3 -0
+deferred:
+       devm_kfree(&pdev->dev, data);
+       return -EPROBE_DEFER;

Nope, when you return with an error from probe() all allocations using devm_* are automatically free:ed that is kind of the point of the managed resources.

Yours,
Linus Walleij

author
#7 lore raw

RE: [PATCH 1/2] gpio: realtek: Add GPIO support for RTD SoC variants

Hi Linus,

Thank you for the review!

Quoted 12 lines

Hi Tzuyi!

thanks for your patch!

Quoted 1 line · Tzuyi Chang

This commit adds GPIO support for Realtek DHC RTD SoCs.

What does "DHC" mean? Please spell it out in the commit and Kconfig so we know what it is.

"DHC" is the abbreviation of "Digital Home Center". I will add the full name to the commit and Kconfig to ensure clarity.

Quoted 13 lines
Quoted 4 lines

This driver enables configuration of GPIO direction, GPIO values, GPIO debounce settings and handles GPIO interrupts.

Signed-off-by Tzuyi Chang <[email protected]>

(...)

drivers/gpio/Kconfig config GPIO_ROCKCHIP · line 562

Quoted 7 lines
drivers/gpio/Kconfig +7 -0
+config GPIO_RTD
+       tristate "Realtek DHC GPIO support"
+       depends on ARCH_REALTEK
+       default y
+       select GPIOLIB_IRQCHIP
+       help
+         Say yes here to support GPIO on Realtek DHC SoCs.
Quoted 2 lines

Explain what DHC is i.e. the acronym expansion, family, use case or something.

I will add the full name to the Kconfig.

Quoted 6 lines

drivers/gpio/gpio-rtd.c line 9

Quoted 2 lines
drivers/gpio/gpio-rtd.c new file +2 -0
+#include <linux/bitops.h>
+#include <linux/gpio.h>

Do not include this legacy header.
Include <linux/gpio/driver.h>

I will remove it.

Quoted 10 lines

Review

Quoted 6 lines
(quoted diff) +6 -0
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h>
+#include <linux/module.h> #include <linux/of.h> #include
+<linux/of_address.h> #include <linux/of_gpio.h> #include
+<linux/of_irq.h>

I don't think you need any of thexe of_* includes. Try it without them.

I will remove it and change the related API in the driver..

Quoted 4 lines
Quoted 1 line

+#include <linux/pinctrl/consumer.h>

Why?

I will remove it. Sorry about that.

Quoted 22 lines
Quoted 22 lines
drivers/gpio/gpio-rtd.c new file +22 -0
+/**
+ * struct rtd_gpio_info - Specific GPIO register information
+ * @name: GPIO device name
+ * @type: RTD GPIO ID
+ * @gpio_base: GPIO base number
+ * @num_gpios: Number of GPIOs
+ * @dir_offset: Offset for GPIO direction registers
+ * @dato_offset: Offset for GPIO data output registers
+ * @dati_offset: Offset for GPIO data input registers
+ * @ie_offset: Offset for GPIO interrupt enable registers
+ * @dp_offset: Offset for GPIO detection polarity registers
+ * @gpa_offset: Offset for GPIO assert interrupt status registers
+ * @gpda_offset: Offset for GPIO deassert interrupt status registers
+ * @deb_offset: Offset for GPIO debounce registers  */ struct
+rtd_gpio_info {
+       const char *name;
+       enum rtd_gpio_type type;
+       unsigned int gpio_base;
+       unsigned int num_gpios;
+       unsigned int *dir_offset;
+       unsigned int *dato_offset;
+       unsigned int *dati_offset;
Quoted 9 lines

drivers/gpio/gpio-rtd.c line 66

Quoted 5 lines
drivers/gpio/gpio-rtd.c new file +5 -0
+       unsigned int *ie_offset;
+       unsigned int *dp_offset;
+       unsigned int *gpa_offset;
+       unsigned int *gpda_offset;
+       unsigned int *deb_offset;

Use u8 instead of unsigned int for the offsets. It is clear from the arrays you assign them that they are all u8[].

I will use u8 insead.

Quoted 8 lines

drivers/gpio/gpio-rtd.c line 75

Quoted 7 lines
drivers/gpio/gpio-rtd.c new file +7 -0
+struct rtd_gpio {
+       struct platform_device *pdev;
+       const struct rtd_gpio_info *info;
+       void __iomem *base;
+       void __iomem *irq_base;
+       struct gpio_chip gpio_chip;
+       struct irq_chip irq_chip;
Quoted 5 lines

Do not use a dynamic irq_chip, create an immutable irq_chip using a const struct.

See recent commits and virtually all current drivers in the tree for examples on how to do that.

I will look at other drivers to fix it.

Quoted 12 lines

drivers/gpio/gpio-rtd.c line 77

Quoted 2 lines
drivers/gpio/gpio-rtd.c new file +2 -0
+       int assert_irq;
+       int deassert_irq;

I don't quite understand these two, but let's see in the rest of the driver.

Quoted 2 lines

+ .deb_offset = (unsigned int []){ 0x30, 0x34, 0x38, 0x3c, 0x40,
+ 0x44, 0x48, 0x4c },

(...)

Quoted 1 line

+ .deb_offset = (unsigned int []){ 0x50 },

So clearly u8[]

I will fix it.

Quoted 9 lines

Review

Quoted 3 lines
(quoted diff) +3 -0
+static unsigned int rtd_gpio_deb_offset(struct rtd_gpio *data,
+unsigned int offset) {
+       return data->info->deb_offset[offset / 8]; }

So this is clearly counted by the GPIO number offset and the GPIO number determines how far into the array we can index.

It looks a bit dangerous, it it possible to encode the array lengths better?

I think I can add array size members for each offset array within the rtd_gpio_info structure and utilize them to prevent accessing elements outside the array.

Quoted 16 lines
Quoted 5 lines
drivers/gpio/gpio-rtd.c new file +5 -0
+       if (data->info->type == RTD1295_ISO_GPIO) {
+               shift = 0;
+               deb_val += 1;
+               write_en = BIT(shift + 3);
+               reg_offset = rtd1295_gpio_deb_offset(data, offset);

drivers/gpio/gpio-rtd.c line 256

Quoted 10 lines
drivers/gpio/gpio-rtd.c new file +10 -0
+       } else if (data->info->type == RTD1295_MISC_GPIO) {
+               shift = (offset >> 4) * 4;
+               deb_val += 1;
+               write_en = BIT(shift + 3);
+               reg_offset = rtd1295_gpio_deb_offset(data, offset);
+       } else {
+               shift = (offset % 8) * 4;
+               write_en = BIT(shift + 3);
+               reg_offset = rtd_gpio_deb_offset(data, offset);
+       }
Quoted 3 lines

These three different offset functions seem a bit awkward. Can we do this by just another index instead?

Do you mean to create an array for the "shift"?

For the RTD SoCs other than RTD1295, each GPIO can have its own debounce configuration. Each GPIO uses 4 bits to specify the debounce value (bits [2:0]), and bit [3] is used for write enable. As a result, each GPIO debounce register accommodates the configuration of 8 GPIOs.

However, the GPIO debounce design in the RTD1295 is a little weird.

For RTD1295 ISO GPIO, there's only one register to configure debounce, and all ISO GPIO pins share the same debounce setting. Bits [2:0] are used to specify the debounce value, while bit [3] is the write_enable flag.

As for RTD1295 MISC GPIO, there's also just one register to set debounce, with 16 GPIOs grouped together. Each group uses 4 bits to define the debounce value (bits [2:0] for the debounce value, and bit [3] for write_enable). All GPIOs within the same group share the same debounce setting.

For instance:
GPIO 0 to 15 use bits [0:2] to configure the debounce value, with bit [3] as the write_enable. GPIO 16 to 31 use bits [6:4] for debounce configuration, with bit [7] indicating the write_enable.
...

The debounce range for RTD1295 is also 1us to 30ms, but the valid register values range from 1 to 7. (For other RTD SoCs, the range is 0 to 6.)

Quoted 11 lines

Review

Quoted 7 lines
(quoted diff) +7 -0
+static int rtd_gpio_request(struct gpio_chip *chip, unsigned int
+offset) {
+       return pinctrl_gpio_request(chip->base + offset); }
+
+static void rtd_gpio_free(struct gpio_chip *chip, unsigned int
+offset) {
+       pinctrl_gpio_free(chip->base + offset); }

IIRC Bartosz has changed this for kernel v6.7, please check his upstream commits and adjust the code accordingly.

I will check the commit to fix it .

Quoted 9 lines

drivers/gpio/gpio-rtd.c

Quoted 8 lines
drivers/gpio/gpio-rtd.c new file +8 -0
+static int rtd_gpio_to_irq(struct gpio_chip *chip, unsigned int
+offset) {
+       struct rtd_gpio *data = gpiochip_get_data(chip);
+       u32 irq = 0;
+
+       irq = irq_find_mapping(data->domain, offset);
+       if (!irq) {
+               dev_err(&data->pdev->dev, "%s: can not find irq number for

hwirq= %d\n",

Quoted 9 lines

drivers/gpio/gpio-rtd.c line 408

Quoted 5 lines
drivers/gpio/gpio-rtd.c new file +5 -0
+                       __func__, offset);
+               return -EINVAL;
+       }
+       return irq;
+}

Don't implement your own gpio_to_irq, just use the GPIOLIB_IRQCHIP helpers. See other drivers that select GPIOLIB_IRQCHIP, this driver is nothing special.

I will use the GPIOLIB_IRQCHIP helpers instead.

Quoted 22 lines

drivers/gpio/gpio-rtd.c

Quoted 5 lines
drivers/gpio/gpio-rtd.c new file +5 -0
+       chained_irq_enter(chip, desc);
+
+       for (i = 0; i < data->info->num_gpios; i = i + 31) {
+               gpa_reg_offset = rtd_gpio_gpa_offset(data, i);
+               status = readl_relaxed(data->irq_base + gpa_reg_offset) >>

1;

drivers/gpio/gpio-rtd.c line 447

Quoted 13 lines
drivers/gpio/gpio-rtd.c new file +13 -0
+               writel_relaxed(status << 1, data->irq_base +
+ gpa_reg_offset);
+
+               while (status) {
+                       j = __ffs(status);
+                       status &= ~BIT(j);
+                       hwirq = i + j;
+                       if (rtd_gpio_check_ie(data, hwirq)) {
+                               int irq =
+ irq_find_mapping(data->domain, hwirq);
+
+                               generic_handle_irq(irq);
+                       }

So you skip the interrupt handler if the interrupt is not enabled?

Quoted 4 lines

I think you should report spurious interrupts if they occur without being enabled, unless there is some hardware flunky making these lines flicker with noise interrupts too much.

On our platform, GPIO interrupts are managed using two parent interrupts:
'assert irq' for a rising edge and 'deassert irq' for a falling edge. When we receive one of these interrupts, we need to check the corresponding status register(gpa_reg or gpda_reg ) to determine which GPIO interrupt has occurred.

The gpa_reg and gpda_reg are unmasked interrupt status registers. Even if we do not enable the specific GPIO interrupt, the gpa_reg and gpda_reg registers still change their status when the GPIO signal changes (it will not trigger the interrupt to CPU). Therefore, it is necessary to check whether the GPIO interrupt is enabled.

Quoted 30 lines
Quoted 4 lines
drivers/gpio/gpio-rtd.c new file +4 -0
+static void rtd_gpio_deassert_irq_handle(struct irq_desc *desc) {
+       struct rtd_gpio *data = irq_desc_get_handler_data(desc);
+       struct irq_chip *chip = irq_desc_get_chip(desc);
+       unsigned int gpda_reg_offset;

drivers/gpio/gpio-rtd.c

Quoted 10 lines
drivers/gpio/gpio-rtd.c new file +10 -0
+       u32 status;
+       int hwirq;
+       int i;
+       int j;
+
+       chained_irq_enter(chip, desc);
+
+       for (i = 0; i < data->info->num_gpios; i = i + 31) {
+               gpda_reg_offset = rtd_gpio_gpda_offset(data, i);
+               status = readl_relaxed(data->irq_base + gpda_reg_offset) >>

1;

drivers/gpio/gpio-rtd.c

Quoted 9 lines
drivers/gpio/gpio-rtd.c new file +9 -0
+               writel_relaxed(status << 1, data->irq_base +
+ gpda_reg_offset);
+
+               while (status) {
+                       j = __ffs(status);
+                       status &= ~BIT(j);
+                       hwirq = i + j;
+                       if (rtd_gpio_check_ie(data, hwirq)) {
+                               int irq = irq_find_mapping(data->domain,

hwirq);

Review

Quoted 4 lines
(quoted diff) +4 -0
+                               u32 irq_type =
+ irq_get_trigger_type(irq);
+
+                               if ((irq_type & IRQ_TYPE_SENSE_MASK)

== IRQ_TYPE_EDGE_BOTH)

Quoted 11 lines

drivers/gpio/gpio-rtd.c line 452

Quoted 7 lines
drivers/gpio/gpio-rtd.c new file +7 -0
+                                       generic_handle_irq(irq);
+                       }
+               }
+       }
+
+       chained_irq_exit(chip, desc);
+}

There is some code duplication here. Create wrapper calls with parameters so you don't need to have several functions that look almost the same.

I will create a wrapper to avoid duplication.

Quoted 16 lines

drivers/gpio/gpio-rtd.c line 599

Quoted 4 lines
drivers/gpio/gpio-rtd.c new file +4 -0
+static int rtd_gpio_probe(struct platform_device *pdev) {
+       struct rtd_gpio *data;
+       const struct of_device_id *match;
+       struct device_node *node;

Don't go looking by the OF node, use the device:

struct device *dev = &pdev->dev;

drivers/gpio/gpio-rtd.c line 603

Quoted 4 lines
drivers/gpio/gpio-rtd.c new file +4 -0
+       int ret;
+       int i;
+
+       node = pdev->dev.of_node;

Use #include <linux/property.h>

Quoted 8 lines

drivers/gpio/gpio-rtd.c line 606

Quoted 3 lines
drivers/gpio/gpio-rtd.c new file +3 -0
+       match = of_match_node(rtd_gpio_of_matches, pdev->dev.of_node);
+       if (!match || !match->data)
+               return -EINVAL;

Use
data->info = device_get_match_data(dev); instead
if (!data->info)...

I will change to this method.

Quoted 5 lines

drivers/gpio/gpio-rtd.c line 608

Quoted 2 lines
drivers/gpio/gpio-rtd.c new file +2 -0
+
+       data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);

With a local dev you can just devm_kzalloc(dev, ...) etc.

I will fix it.

Quoted 11 lines

drivers/gpio/gpio-rtd.c line 618

Quoted 7 lines
drivers/gpio/gpio-rtd.c new file +7 -0
+       data->assert_irq = irq_of_parse_and_map(node, 0);
+       if (!data->assert_irq)
+               goto deferred;
+
+       data->deassert_irq = irq_of_parse_and_map(node, 1);
+       if (!data->deassert_irq)
+               goto deferred;

So one handler for rising and one handler for falling edge? Hm that's different. I guess you need separate handlers.

Yes, assert_irq is for rising edge and deassert_irq is for falling edge.

Quoted 11 lines

drivers/gpio/gpio-rtd.c line 625

Quoted 3 lines
drivers/gpio/gpio-rtd.c new file +3 -0
+       data->base = of_iomap(node, 0);
+       if (!data->base)
+               return -ENXIO;

Use
data->base = devm_platform_ioremap_resource(pdev, 0);

drivers/gpio/gpio-rtd.c line 629

Quoted 3 lines
drivers/gpio/gpio-rtd.c new file +3 -0
+       data->irq_base = of_iomap(node, 1);
+       if (!data->irq_base)
+               return -ENXIO;
Quoted 7 lines

Use
data->irq_base = platform_get_irq(pdev, 1);

Quoted 1 line

+ data->gpio_chip.parent = &pdev->dev;

Don't assign this, the core will handle it.

I will remove it.

Quoted 5 lines

drivers/gpio/gpio-rtd.c line 633

Quoted 2 lines
drivers/gpio/gpio-rtd.c new file +2 -0
+       data->gpio_chip.label = dev_name(&pdev->dev);
+       data->gpio_chip.of_gpio_n_cells = 2;

This is the default, let the core handle OF translation.

I will remove it.

Quoted 12 lines

drivers/gpio/gpio-rtd.c line 644

Quoted 11 lines
drivers/gpio/gpio-rtd.c new file +11 -0
+       data->gpio_chip.base = data->info->gpio_base;
+       data->gpio_chip.ngpio = data->info->num_gpios;
+       data->gpio_chip.request = rtd_gpio_request;
+       data->gpio_chip.free = rtd_gpio_free;
+       data->gpio_chip.get_direction = rtd_gpio_get_direction;
+       data->gpio_chip.direction_input = rtd_gpio_direction_input;
+       data->gpio_chip.direction_output = rtd_gpio_direction_output;
+       data->gpio_chip.set = rtd_gpio_set;
+       data->gpio_chip.get = rtd_gpio_get;
+       data->gpio_chip.set_config = rtd_gpio_set_config;
+       data->gpio_chip.to_irq = rtd_gpio_to_irq;
Quoted 2 lines

Use the GPIOLIB_IRQCHIP to provide this for you.

I will use the helpers instead.

Quoted 4 lines
Quoted 1 line

+ data->irq_chip = rtd_gpio_irq_chip;

Convert to use immutable irq_chip. (Maybe several struct irq_chip if you need!)

I will fix it.

Quoted 36 lines

drivers/gpio/gpio-rtd.c

Quoted 12 lines
drivers/gpio/gpio-rtd.c new file +12 -0
+       data->domain = irq_domain_add_linear(node, data->gpio_chip.ngpio,
+                               &irq_domain_simple_ops, data);
+       if (!data->domain) {
+               devm_kfree(&pdev->dev, data);
+               return -ENOMEM;
+       }
+
+       for (i = 0; i < data->gpio_chip.ngpio; i++) {
+               int irq = irq_create_mapping(data->domain, i);
+
+               irq_set_chip_data(irq, data);
+               irq_set_chip_and_handler(irq, &data->irq_chip,

handle_simple_irq);

drivers/gpio/gpio-rtd.c

Quoted 3 lines
drivers/gpio/gpio-rtd.c new file +3 -0
+       }
+
+       irq_set_chained_handler_and_data(data->assert_irq,

rtd_gpio_assert_irq_handle, data);

Quoted 2 lines

+ irq_set_chained_handler_and_data(data->deassert_irq,
+ rtd_gpio_deassert_irq_handle, data);

Instead of doing this use GPIOLIB_IRQCHIP.

Before registering the gpio_chip set up stuff somewhat like this:

       girq = &data->gpio_chip.irq;
       gpio_irq_chip_set_chip(girq, &my_irq_chip);
       girq->parent_handler = my_gpio_irq_handler;
       girq->num_parents = 1;
       girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
                                    GFP_KERNEL);
       if (!girq->parents)
               ret = -ENOMEM;
       girq->default_type = IRQ_TYPE_NONE;
       girq->handler = handle_bad_irq;
       girq->parents[0] = irq;
Quoted 2 lines

But maybe in this case you want two parent IRQs? Not sure.

Yes, one for asserting IRQ and one for deasserting IRQ. I will try to use gpio_irq_chip to implement them.
It appears that only one parent handler can be registered in the gpio_irq_chip for all the parent IRQs. Asserting IRQ and deasserting IRQ each have their own status register to check. Does this mean that it should distinguish which interrupt has occurred and then check the corresponding status register in the parent IRQ handler?

Quoted 7 lines

drivers/gpio/gpio-rtd.c line 677

Quoted 3 lines
drivers/gpio/gpio-rtd.c new file +3 -0
+deferred:
+       devm_kfree(&pdev->dev, data);
+       return -EPROBE_DEFER;

Nope, when you return with an error from probe() all allocations using devm_* are automatically free:ed that is kind of the point of the managed resources.

I will remove it.

Quoted 2 lines

Yours,
Linus Walleij

Thanks,
Tzuyi Chang

#8 lore raw

Re: [PATCH 1/2] gpio: realtek: Add GPIO support for RTD SoC variants

Quoted 8 lines · TY_Chang[張子逸]
Quoted 8 lines · Tzuyi Chang

Review

Quoted 3 lines
(quoted diff) +3 -0
+static unsigned int rtd_gpio_deb_offset(struct rtd_gpio *data,
+unsigned int offset) {
+       return data->info->deb_offset[offset / 8]; }

So this is clearly counted by the GPIO number offset and the GPIO number determines how far into the array we can index.

Quoted 4 lines
Quoted 1 line

It looks a bit dangerous, it it possible to encode the array lengths better?

I think I can add array size members for each offset array within the rtd_gpio_info structure and utilize them to prevent accessing elements outside the array.

I don't know about that for constant arrays, if you look at recent commits from Kees Cook you will find examples of how to use the compiler helpers for dynamic arrays, e.g.

git log -p --author=Kees

will find you things like this:

@@ -60,7 +60,7 @@ struct reset_control {

 struct reset_control_array {
        struct reset_control base;
        unsigned int num_rstcs;

- struct reset_control *rstc[];
+ struct reset_control *rstc[] __counted_by(num_rstcs);
};

So the compiler instruction __counted_by() is used pointing back to the variable above to avoid outofbounds accesses.

BUT: those are *dynamic* arrays, placed *last* in the struct.

You have several *constant* arrays, in the same struct (and C allows this).

I don't really know what is the best practice for constants, maybe Kees has some pointers?

Maybe what you have is the best we can do.

Yours,
Linus Walleij

author
#9 lore raw

PATCH 2/2 dt-bindings: gpio: realtek: Add realtek,rtd-gpio bindings

In reply to Tzuyi Chang

This patch adds the device tree bindings for the Realtek DHC RTD SoCs GPIO controllers.

Signed-off-by Tzuyi Chang <[email protected]>
---
 .../bindings/gpio/realtek,rtd-gpio.yaml       | 56 +++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml
Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml new file +56 -0
new file mode 100644
index 000000000000..6cab7ec50c88
@@ -0,0 +1,56 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2023 Realtek Semiconductor Corporation
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/realtek,rtd-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek DHC GPIO controller
+
+maintainers:
+  - TY Chang <[email protected]>
+
+properties:
+  compatible:
+    enum:
+      - realtek,rtd-gpio
+      - realtek,rtd1295-misc-gpio
+      - realtek,rtd1295-iso-gpio
+      - realtek,rtd1395-iso-gpio
+      - realtek,rtd1619-iso-gpio
+
+  reg:
+    maxItems: 2
+
+  interrupts:
+    maxItems: 2
+
+  gpio-ranges: true
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - gpio-ranges
+  - gpio-controller
+  - "#gpio-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    gpio@100 {
+      compatible = "realtek,rtd-gpio";
+      reg = <0x100 0x100>,
+            <0x000 0x0b0>;
+      interrupt-parent = <&iso_irq_mux>;
+      interrupts = <19>, <20>;
+      gpio-ranges = <&pinctrl 0 0 82>;
+      gpio-controller;
+      #gpio-cells = <2>;
+    };
Signature · 1 line
2.42.0
#10 lore raw

Re: [PATCH 2/2] dt-bindings: gpio: realtek: Add realtek,rtd-gpio bindings

Quoted 3 lines · Tzuyi Chang

This patch adds the device tree bindings for the Realtek DHC RTD SoCs GPIO controllers.

A nit, subject: drop second/last, redundant "bindings". The "dt-bindings" prefix is already stating that these are bindings.

Quoted 5 lines
Signed-off-by Tzuyi Chang <[email protected]>
---
 .../bindings/gpio/realtek,rtd-gpio.yaml       | 56 +++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml

How does your binding come after the user?

Quoted 7 lines
Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml new file +0 -0
new file mode 100644
index 000000000000..6cab7ec50c88

Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml line 8

Quoted 8 lines
Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml new file +8 -0
@@ -0,0 +1,56 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2023 Realtek Semiconductor Corporation
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/realtek,rtd-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek DHC GPIO controller

What is DHC? Where is it explained in the binding?

Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml line 16

Quoted 8 lines
Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml new file +8 -0
+
+maintainers:
+  - TY Chang <[email protected]>
+
+properties:
+  compatible:
+    enum:
+      - realtek,rtd-gpio

What is "rtd"? Generic name? Drop. You cannot have generic compatibles.

Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml line 23

Quoted 7 lines
Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml new file +7 -0
+      - realtek,rtd1295-misc-gpio
+      - realtek,rtd1295-iso-gpio
+      - realtek,rtd1395-iso-gpio
+      - realtek,rtd1619-iso-gpio
+
+  reg:
+    maxItems: 2

You need to describe the items instead.

Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml line 26

Quoted 3 lines
Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml new file +3 -0
+
+  interrupts:
+    maxItems: 2

You need to describe the items instead.

Quoted 5 lines
Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml new file +5 -0
+
+  gpio-ranges: true
+
+  gpio-controller: true
+

Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml line 41

Quoted 10 lines
Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml new file +10 -0
+  "#gpio-cells":
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - gpio-ranges
+  - gpio-controller
+  - "#gpio-cells"

Best regards,
Krzysztof

author
#11 lore raw

RE: [PATCH 2/2] dt-bindings: gpio: realtek: Add realtek,rtd-gpio bindings

Hi Krzysztof,

Thank you for the review.

Quoted 8 lines · Tzuyi Chang
Quoted 3 lines

This patch adds the device tree bindings for the Realtek DHC RTD SoCs GPIO controllers.

A nit, subject: drop second/last, redundant "bindings". The "dt-bindings" prefix is already stating that these are bindings.

I will remove it.

Quoted 9 lines
Quoted 6 lines
Signed-off-by Tzuyi Chang <[email protected]>
---
 .../bindings/gpio/realtek,rtd-gpio.yaml       | 56 +++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644

Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml

How does your binding come after the user?

I wil move the binding to the first patch.

Quoted 10 lines
Quoted 10 lines

diff --git a/Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml b/Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml new file mode 100644
index 000000000000..6cab7ec50c88

Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml new file +1 -0
@@ -0,0 +1,56 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) # Copyright 2023
Quoted 9 lines

Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml line 8

Quoted 6 lines
Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml new file +6 -0
+Realtek Semiconductor Corporation %YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/realtek,rtd-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek DHC GPIO controller

What is DHC? Where is it explained in the binding?

This is the abbreviation of "Digital Home Center". I will add the description to explain it.

Quoted 11 lines

Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml line 16

Quoted 8 lines
Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml new file +8 -0
+
+maintainers:
+  - TY Chang <[email protected]>
+
+properties:
+  compatible:
+    enum:
+      - realtek,rtd-gpio

What is "rtd"? Generic name? Drop. You cannot have generic compatibles.

This is a generic name for the others SoCs without the specific compatible. I will fix it.

Quoted 11 lines

Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml line 23

Quoted 8 lines
Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml new file +7 -0
+      - realtek,rtd1295-misc-gpio
+      - realtek,rtd1295-iso-gpio
+      - realtek,rtd1395-iso-gpio
+      - realtek,rtd1619-iso-gpio
+
+  reg:
+    maxItems: 2

You need to describe the items instead.

I will add the description.

Quoted 6 lines

Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml line 26

Quoted 3 lines
Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml new file +3 -0
+
+  interrupts:
+    maxItems: 2

You need to describe the items instead.

I will add the description.

Quoted 10 lines
Quoted 10 lines
Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml new file +10 -0
+
+  gpio-ranges: true
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+required:
+  - compatible
Quoted 8 lines

Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml line 41

Quoted 5 lines
Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml new file +5 -0
+  - reg
+  - interrupts
+  - gpio-ranges
+  - gpio-controller
+  - "#gpio-cells"

Best regards,
Krzysztof

Thanks,
Tzuyi Chang