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
--
2.42.0
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
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 673bafb8be58..8d55c1ce8f3b 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -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
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index eb73b5d633eb..16bb40717e87 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -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
diff --git a/drivers/gpio/gpio-rtd.c b/drivers/gpio/gpio-rtd.c
new file mode 100644
index 000000000000..5fec261b602f
--- /dev/null
+++ b/drivers/gpio/gpio-rtd.c
@@ -0,0 +1,702 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Realtek DHC gpio driver
+ *
+ * Copyright (c) 2023 Realtek Semiconductor Corp.
+ */
+
+#include <linux/bitops.h>
+#include <linux/gpio.h>
+#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>
+#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define RTD_GPIO_DEBOUNCE_1US 0
+#define RTD_GPIO_DEBOUNCE_10US 1
+#define RTD_GPIO_DEBOUNCE_100US 2
+#define RTD_GPIO_DEBOUNCE_1MS 3
+#define RTD_GPIO_DEBOUNCE_10MS 4
+#define RTD_GPIO_DEBOUNCE_20MS 5
+#define RTD_GPIO_DEBOUNCE_30MS 6
+
+enum rtd_gpio_type {
+ RTD_ISO_GPIO = 0,
+ RTD1295_ISO_GPIO,
+ RTD1295_MISC_GPIO,
+ RTD1395_ISO_GPIO,
+ RTD1619_ISO_GPIO,
+};
+
+/**
+ * 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;
+ unsigned int *ie_offset;
+ unsigned int *dp_offset;
+ unsigned int *gpa_offset;
+ unsigned int *gpda_offset;
+ unsigned int *deb_offset;
+};
+
+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;
+ int assert_irq;
+ int deassert_irq;
+ struct irq_domain *domain;
+ spinlock_t lock;
+};
+
+
+static const struct rtd_gpio_info rtd_iso_gpio_info = {
+ .name = "rtd_iso_gpio",
+ .type = RTD_ISO_GPIO,
+ .gpio_base = 0,
+ .num_gpios = 82,
+ .dir_offset = (unsigned int []){ 0x0, 0x18, 0x2c },
+ .dato_offset = (unsigned int []){ 0x4, 0x1c, 0x30 },
+ .dati_offset = (unsigned int []){ 0x8, 0x20, 0x34 },
+ .ie_offset = (unsigned int []){ 0xc, 0x24, 0x38 },
+ .dp_offset = (unsigned int []){ 0x10, 0x28, 0x3c },
+ .gpa_offset = (unsigned int []){ 0x8, 0xe0, 0x90 },
+ .gpda_offset = (unsigned int []){ 0xc, 0xe4, 0x94 },
+ .deb_offset = (unsigned int []){ 0x44, 0x48, 0x4c, 0x50, 0x54, 0x58, 0x5c,
+ 0x60, 0x64, 0x68, 0x6c },
+};
+
+static const struct rtd_gpio_info rtd1619_iso_gpio_info = {
+ .name = "rtd1619_iso_gpio",
+ .type = RTD1619_ISO_GPIO,
+ .gpio_base = 0,
+ .num_gpios = 86,
+ .dir_offset = (unsigned int []){ 0x0, 0x18, 0x2c },
+ .dato_offset = (unsigned int []){ 0x4, 0x1c, 0x30 },
+ .dati_offset = (unsigned int []){ 0x8, 0x20, 0x34 },
+ .ie_offset = (unsigned int []){ 0xc, 0x24, 0x38 },
+ .dp_offset = (unsigned int []){ 0x10, 0x28, 0x3c },
+ .gpa_offset = (unsigned int []){ 0x8, 0xe0, 0x90 },
+ .gpda_offset = (unsigned int []){ 0xc, 0xe4, 0x94 },
+ .deb_offset = (unsigned int []){ 0x44, 0x48, 0x4c, 0x50, 0x54, 0x58, 0x5c,
+ 0x60, 0x64, 0x68, 0x6c },
+};
+
+static const struct rtd_gpio_info rtd1395_iso_gpio_info = {
+ .name = "rtd1395_iso_gpio",
+ .type = RTD1395_ISO_GPIO,
+ .gpio_base = 0,
+ .num_gpios = 57,
+ .dir_offset = (unsigned int []){ 0x0, 0x18 },
+ .dato_offset = (unsigned int []){ 0x4, 0x1c },
+ .dati_offset = (unsigned int []){ 0x8, 0x20 },
+ .ie_offset = (unsigned int []){ 0xc, 0x24 },
+ .dp_offset = (unsigned int []){ 0x10, 0x28 },
+ .gpa_offset = (unsigned int []){ 0x8, 0xe0 },
+ .gpda_offset = (unsigned int []){ 0xc, 0xe4 },
+ .deb_offset = (unsigned int []){ 0x30, 0x34, 0x38, 0x3c, 0x40, 0x44, 0x48, 0x4c },
+};
+
+static const struct rtd_gpio_info rtd1295_misc_gpio_info = {
+ .name = "rtd1295_misc_gpio",
+ .type = RTD1295_ISO_GPIO,
+ .gpio_base = 0,
+ .num_gpios = 101,
+ .dir_offset = (unsigned int []){ 0x0, 0x4, 0x8, 0xc },
+ .dato_offset = (unsigned int []){ 0x10, 0x14, 0x18, 0x1c },
+ .dati_offset = (unsigned int []){ 0x20, 0x24, 0x28, 0x2c },
+ .ie_offset = (unsigned int []){ 0x30, 0x34, 0x38, 0x3c },
+ .dp_offset = (unsigned int []){ 0x40, 0x44, 0x48, 0x4c },
+ .gpa_offset = (unsigned int []){ 0x40, 0x44, 0xa4, 0xb8 },
+ .gpda_offset = (unsigned int []){ 0x54, 0x58, 0xa8, 0xbc},
+ .deb_offset = (unsigned int []){ 0x50 },
+};
+
+static const struct rtd_gpio_info rtd1295_iso_gpio_info = {
+ .name = "rtd1295_iso_gpio",
+ .type = RTD1295_ISO_GPIO,
+ .gpio_base = 101,
+ .num_gpios = 35,
+ .dir_offset = (unsigned int []){ 0x0, 0x18 },
+ .dato_offset = (unsigned int []){ 0x4, 0x1c },
+ .dati_offset = (unsigned int []){ 0x8, 0x20 },
+ .ie_offset = (unsigned int []){ 0xc, 0x24 },
+ .dp_offset = (unsigned int []){ 0x10, 0x28 },
+ .gpa_offset = (unsigned int []){ 0x8, 0xe0 },
+ .gpda_offset = (unsigned int []){ 0xc, 0xe4 },
+ .deb_offset = (unsigned int []){ 0x14 },
+};
+
+static unsigned int rtd_gpio_dir_offset(struct rtd_gpio *data, unsigned int offset)
+{
+ return data->info->dir_offset[offset / 32];
+}
+
+static unsigned int rtd_gpio_dato_offset(struct rtd_gpio *data, unsigned int offset)
+{
+ return data->info->dato_offset[offset / 32];
+}
+
+static unsigned int rtd_gpio_dati_offset(struct rtd_gpio *data, unsigned int offset)
+{
+ return data->info->dati_offset[offset / 32];
+}
+
+static unsigned int rtd_gpio_ie_offset(struct rtd_gpio *data, unsigned int offset)
+{
+ return data->info->ie_offset[offset / 32];
+}
+
+static unsigned int rtd_gpio_dp_offset(struct rtd_gpio *data, unsigned int offset)
+{
+ return data->info->dp_offset[offset / 32];
+}
+
+static unsigned int rtd_gpio_gpa_offset(struct rtd_gpio *data, unsigned int offset)
+{
+ return data->info->gpa_offset[offset / 31];
+}
+
+static unsigned int rtd_gpio_gpda_offset(struct rtd_gpio *data, unsigned int offset)
+{
+ return data->info->gpda_offset[offset / 31];
+}
+
+static unsigned int rtd_gpio_deb_offset(struct rtd_gpio *data, unsigned int offset)
+{
+ return data->info->deb_offset[offset / 8];
+}
+
+static unsigned int rtd1295_gpio_deb_offset(struct rtd_gpio *data, unsigned int offset)
+{
+ return data->info->deb_offset[0];
+}
+
+static int rtd_gpio_set_debounce(struct gpio_chip *chip, unsigned int offset,
+ unsigned int debounce)
+{
+ struct rtd_gpio *data = gpiochip_get_data(chip);
+ unsigned long flags;
+ unsigned int reg_offset;
+ unsigned int shift;
+ unsigned int write_en;
+ u32 val;
+ u32 deb_val;
+
+ switch (debounce) {
+ case 1:
+ deb_val = RTD_GPIO_DEBOUNCE_1US;
+ break;
+ case 10:
+ deb_val = RTD_GPIO_DEBOUNCE_10US;
+ break;
+ case 100:
+ deb_val = RTD_GPIO_DEBOUNCE_100US;
+ break;
+ case 1000:
+ deb_val = RTD_GPIO_DEBOUNCE_1MS;
+ break;
+ case 10000:
+ deb_val = RTD_GPIO_DEBOUNCE_10MS;
+ break;
+ case 20000:
+ deb_val = RTD_GPIO_DEBOUNCE_20MS;
+ break;
+ case 30000:
+ deb_val = RTD_GPIO_DEBOUNCE_30MS;
+ break;
+ default:
+ return -ENOTSUPP;
+ }
+
+ 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);
+ } 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);
+ }
+ val = (deb_val << shift) | write_en;
+
+ spin_lock_irqsave(&data->lock, flags);
+ writel_relaxed(val, data->base + reg_offset);
+ spin_unlock_irqrestore(&data->lock, flags);
+
+ return 0;
+}
+
+static int rtd_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
+ unsigned long config)
+{
+ int debounce;
+
+ switch (pinconf_to_config_param(config)) {
+ case PIN_CONFIG_BIAS_DISABLE:
+ case PIN_CONFIG_BIAS_PULL_UP:
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ return pinctrl_gpio_set_config(chip->base + offset, config);
+ case PIN_CONFIG_INPUT_DEBOUNCE:
+ debounce = pinconf_to_config_argument(config);
+ return rtd_gpio_set_debounce(chip, offset, debounce);
+ default:
+ return -ENOTSUPP;
+ }
+}
+
+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);
+}
+
+static int rtd_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+ struct rtd_gpio *data = gpiochip_get_data(chip);
+ unsigned long flags;
+ unsigned int reg_offset;
+ u32 val;
+
+ reg_offset = rtd_gpio_dir_offset(data, offset);
+
+ spin_lock_irqsave(&data->lock, flags);
+
+ val = readl_relaxed(data->base + reg_offset);
+ val &= BIT(offset % 32);
+
+ spin_unlock_irqrestore(&data->lock, flags);
+
+ return val ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
+}
+
+static int rtd_gpio_set_direction(struct gpio_chip *chip, unsigned int offset, bool out)
+{
+ struct rtd_gpio *data = gpiochip_get_data(chip);
+ unsigned long flags;
+ unsigned int reg_offset;
+ u32 mask = BIT(offset % 32);
+ u32 val;
+
+ reg_offset = rtd_gpio_dir_offset(data, offset);
+
+ spin_lock_irqsave(&data->lock, flags);
+
+ val = readl_relaxed(data->base + reg_offset);
+ if (out)
+ val |= mask;
+ else
+ val &= ~mask;
+ writel_relaxed(val, data->base + reg_offset);
+
+ spin_unlock_irqrestore(&data->lock, flags);
+
+ return 0;
+}
+
+static int rtd_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+ return rtd_gpio_set_direction(chip, offset, false);
+}
+
+static int rtd_gpio_direction_output(struct gpio_chip *chip, unsigned int offset, int value)
+{
+ chip->set(chip, offset, value);
+
+ return rtd_gpio_set_direction(chip, offset, true);
+}
+
+static void rtd_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+ struct rtd_gpio *data = gpiochip_get_data(chip);
+ unsigned long flags;
+ unsigned int dato_reg_offset;
+ u32 mask = BIT(offset % 32);
+ u32 val;
+
+ dato_reg_offset = rtd_gpio_dato_offset(data, offset);
+
+ spin_lock_irqsave(&data->lock, flags);
+
+ val = readl_relaxed(data->base + dato_reg_offset);
+ if (value)
+ val |= mask;
+ else
+ val &= ~mask;
+ writel_relaxed(val, data->base + dato_reg_offset);
+
+ spin_unlock_irqrestore(&data->lock, flags);
+}
+
+static int rtd_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+ struct rtd_gpio *data = gpiochip_get_data(chip);
+ unsigned long flags;
+ unsigned int dir_reg_offset, dat_reg_offset;
+ u32 val;
+
+ dir_reg_offset = rtd_gpio_dir_offset(data, offset);
+
+ spin_lock_irqsave(&data->lock, flags);
+
+ val = readl_relaxed(data->base + dir_reg_offset);
+ val &= BIT(offset % 32);
+ dat_reg_offset = (val) ? rtd_gpio_dato_offset(data, offset) : rtd_gpio_dati_offset(data, offset);
+
+ val = readl_relaxed(data->base + dat_reg_offset);
+ val >>= offset % 32;
+ val &= 0x1;
+
+ spin_unlock_irqrestore(&data->lock, flags);
+
+ return val;
+}
+
+
+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;
+}
+
+static bool rtd_gpio_check_ie(struct rtd_gpio *data, int irq)
+{
+ unsigned int ie_reg_offset;
+ u32 enable;
+ int mask = BIT(irq % 32);
+
+ ie_reg_offset = rtd_gpio_ie_offset(data, irq);
+ enable = readl_relaxed(data->base + ie_reg_offset);
+
+ return enable & mask;
+}
+
+static void rtd_gpio_assert_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 gpa_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) {
+ 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);
+
+ 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);
+ }
+ }
+ }
+
+ chained_irq_exit(chip, desc);
+}
+
+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);
+ 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);
+}
+
+static void rtd_gpio_enable_irq(struct irq_data *d)
+{
+ struct rtd_gpio *data = irq_data_get_irq_chip_data(d);
+ unsigned long flags;
+ unsigned int ie_reg_offset;
+ unsigned int gpa_reg_offset;
+ unsigned int gpda_reg_offset;
+ u32 ie_mask = BIT(d->hwirq % 32);
+ u32 clr_mask = BIT(d->hwirq % 31) << 1;
+ u32 val;
+
+ ie_reg_offset = rtd_gpio_ie_offset(data, d->hwirq);
+ gpa_reg_offset = rtd_gpio_gpa_offset(data, d->hwirq);
+ gpda_reg_offset = rtd_gpio_gpda_offset(data, d->hwirq);
+
+ spin_lock_irqsave(&data->lock, flags);
+
+ writel_relaxed(clr_mask, data->irq_base + gpa_reg_offset);
+ writel_relaxed(clr_mask, data->irq_base + gpda_reg_offset);
+
+ val = readl_relaxed(data->base + ie_reg_offset);
+ val |= ie_mask;
+ writel_relaxed(val, data->base + ie_reg_offset);
+
+ spin_unlock_irqrestore(&data->lock, flags);
+
+}
+
+static void rtd_gpio_disable_irq(struct irq_data *d)
+{
+ struct rtd_gpio *data = irq_data_get_irq_chip_data(d);
+ unsigned long flags;
+ unsigned int ie_reg_offset;
+ u32 ie_mask = BIT(d->hwirq % 32);
+ u32 val;
+
+ ie_reg_offset = rtd_gpio_ie_offset(data, d->hwirq);
+
+ spin_lock_irqsave(&data->lock, flags);
+
+ val = readl_relaxed(data->base + ie_reg_offset);
+ val &= ~ie_mask;
+ writel_relaxed(val, data->base + ie_reg_offset);
+
+ spin_unlock_irqrestore(&data->lock, flags);
+}
+
+
+static int rtd_gpio_irq_set_type(struct irq_data *d, unsigned int type)
+{
+ struct rtd_gpio *data = irq_data_get_irq_chip_data(d);
+ unsigned long flags;
+ unsigned int dp_reg_offset;
+ u32 mask = BIT(d->hwirq % 32);
+ u32 val;
+ bool polarity;
+
+ dp_reg_offset = rtd_gpio_dp_offset(data, d->hwirq);
+
+ switch (type & IRQ_TYPE_SENSE_MASK) {
+ case IRQ_TYPE_EDGE_RISING:
+ polarity = 1;
+ break;
+
+ case IRQ_TYPE_EDGE_FALLING:
+ polarity = 0;
+ break;
+
+ case IRQ_TYPE_EDGE_BOTH:
+ polarity = 1;
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ spin_lock_irqsave(&data->lock, flags);
+
+ val = readl_relaxed(data->base + dp_reg_offset);
+ if (polarity)
+ val |= mask;
+ else
+ val &= ~mask;
+ writel_relaxed(val, data->base + dp_reg_offset);
+
+ spin_unlock_irqrestore(&data->lock, flags);
+
+ return 0;
+}
+
+
+static const struct irq_chip rtd_gpio_irq_chip = {
+ .irq_enable = rtd_gpio_enable_irq,
+ .irq_disable = rtd_gpio_disable_irq,
+ .irq_set_type = rtd_gpio_irq_set_type,
+};
+
+static const struct of_device_id rtd_gpio_of_matches[] = {
+ { .compatible = "realtek,rtd-gpio", .data = &rtd_iso_gpio_info },
+ { .compatible = "realtek,rtd1295-misc-gpio", .data = &rtd1295_misc_gpio_info },
+ { .compatible = "realtek,rtd1295-iso-gpio", .data = &rtd1295_iso_gpio_info },
+ { .compatible = "realtek,rtd1395-iso-gpio", .data = &rtd1395_iso_gpio_info },
+ { .compatible = "realtek,rtd1619-iso-gpio", .data = &rtd1619_iso_gpio_info },
+ { }
+};
+MODULE_DEVICE_TABLE(of, rtd_gpio_of_matches);
+
+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);
+ if (!data->assert_irq)
+ goto deferred;
+
+ data->deassert_irq = irq_of_parse_and_map(node, 1);
+ if (!data->deassert_irq)
+ goto deferred;
+
+ data->info = match->data;
+ spin_lock_init(&data->lock);
+
+ data->base = of_iomap(node, 0);
+ if (!data->base)
+ return -ENXIO;
+
+ 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;
+ 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);
+ 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);
+ 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);
+ }
+
+ 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");
+
+ return 0;
+
+deferred:
+ devm_kfree(&pdev->dev, data);
+ 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);
+}
+
+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");
--
2.42.0
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
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
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml
@@ -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>;
+ };
--
2.42.0
On 01/11/2023 03:58, Tzuyi Chang wrote:
> 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.
> 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?
>
> 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
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml
> @@ -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?
> +
> +maintainers:
> + - TY Chang <[email protected]>
> +
> +properties:
> + compatible:
> + enum:
> + - realtek,rtd-gpio
What is "rtd"? Generic name? Drop. You cannot have generic compatibles.
> + - 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.
> +
> + interrupts:
> + maxItems: 2
You need to describe the items instead.
> +
> + gpio-ranges: true
> +
> + gpio-controller: true
> +
> + "#gpio-cells":
> + const: 2
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - gpio-ranges
> + - gpio-controller
> + - "#gpio-cells"
Best regards,
Krzysztof
On 01/11/2023 03:58, Tzuyi Chang wrote:
> 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
>
> This driver enables configuration of GPIO direction, GPIO values, GPIO
> debounce settings and handles GPIO interrupts.
>
> Signed-off-by: Tzuyi Chang <[email protected]>
> ---
...
> +
> +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);
> + 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...
> +
> + 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?
> +
> + 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;
> + 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.
> + 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.
> + 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);
> + }
> +
> + 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.
> +
> + return 0;
> +
> +deferred:
> + devm_kfree(&pdev->dev, data);
NAK, test your patch.
> + 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);
> +}
> +
> +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
Hi Tzuyi!
thanks for your patch!
On Wed, Nov 1, 2023 at 3:58 AM Tzuyi Chang <[email protected]> wrote:
> 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.
> This driver enables configuration of GPIO direction, GPIO values, GPIO
> debounce settings and handles GPIO interrupts.
>
> Signed-off-by: Tzuyi Chang <[email protected]>
(...)
> +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.
> +#include <linux/bitops.h>
> +#include <linux/gpio.h>
Do not include this legacy header.
Include <linux/gpio/driver.h>
> +#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.
> +#include <linux/pinctrl/consumer.h>
Why?
> +/**
> + * 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;
> + 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[].
> +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.
> + int assert_irq;
> + int deassert_irq;
I don't quite understand these two, but let's see in the rest
of the driver.
> + .deb_offset = (unsigned int []){ 0x30, 0x34, 0x38, 0x3c, 0x40, 0x44, 0x48, 0x4c },
(...)
> + .deb_offset = (unsigned int []){ 0x50 },
So clearly u8[]
> +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?
> + 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);
> + } 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?
> +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.
> +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.
> + 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);
> +
> + 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.
> +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);
> + 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.
> +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;
> + int ret;
> + int i;
> +
> + node = pdev->dev.of_node;
Use #include <linux/property.h>
> + 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)...
> +
> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
With a local dev you can just devm_kzalloc(dev, ...) etc.
> + 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.
> + data->base = of_iomap(node, 0);
> + if (!data->base)
> + return -ENXIO;
Use
data->base = devm_platform_ioremap_resource(pdev, 0);
> + data->irq_base = of_iomap(node, 1);
> + if (!data->irq_base)
> + return -ENXIO;
Use
data->irq_base = platform_get_irq(pdev, 1);
> + data->gpio_chip.parent = &pdev->dev;
Don't assign this, the core will handle it.
> + 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.
> + 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.
> + data->irq_chip = rtd_gpio_irq_chip;
Convert to use immutable irq_chip. (Maybe several struct irq_chip if you need!)
> + 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);
> + }
> +
> + 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.
> +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
Hi Krzysztof,
Thank you for the review.
>On 01/11/2023 03:58, Tzuyi Chang wrote:
>> 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.
>> 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.
>>
>> 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
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml
>> @@ -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?
>
This is the abbreviation of "Digital Home Center". I will add the description to explain it.
>> +
>> +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.
>
>> + - 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.
>> +
>> + interrupts:
>> + maxItems: 2
>
>You need to describe the items instead.
>
I will add the description.
>> +
>> + gpio-ranges: true
>> +
>> + gpio-controller: true
>> +
>> + "#gpio-cells":
>> + const: 2
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - interrupts
>> + - gpio-ranges
>> + - gpio-controller
>> + - "#gpio-cells"
>
>Best regards,
>Krzysztof
Thanks,
Tzuyi Chang
Hi Krzysztof,
>On 01/11/2023 03:58, Tzuyi Chang wrote:
>> 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.
>> +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);
>> + 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.
>> +
>> + 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?
>
>> +
>> + 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;
>> + 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.
>
>> + 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.
>
I will remove it.
>> + 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);
>> + }
>> +
>> + 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.
>
I will remove it.
>> +
>> + return 0;
>> +
>> +deferred:
>> + devm_kfree(&pdev->dev, data);
>
>NAK, test your patch.
>
>> + return -EPROBE_DEFER;
I will remove this label.
>> +}
>
>Best regards,
>Krzysztof
Thanks,
Tzuyi Chang
On 02/11/2023 04:30, TY_Chang[張子逸] wrote:
> Hi Krzysztof,
>
>> On 01/11/2023 03:58, Tzuyi Chang wrote:
>>> 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.
>
>>> +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);
>>> + 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.
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
Hi Linus,
Thank you for the review!
>
>Hi Tzuyi!
>
>thanks for your patch!
>
>On Wed, Nov 1, 2023 at 3:58 AM Tzuyi Chang <[email protected]> wrote:
>
>> 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.
>> This driver enables configuration of GPIO direction, GPIO values, GPIO
>> debounce settings and handles GPIO interrupts.
>>
>> Signed-off-by: Tzuyi Chang <[email protected]>
>(...)
>> +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.
>
I will add the full name to the Kconfig.
>> +#include <linux/bitops.h>
>> +#include <linux/gpio.h>
>
>Do not include this legacy header.
>Include <linux/gpio/driver.h>
>
I will remove it.
>> +#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..
>> +#include <linux/pinctrl/consumer.h>
>
>Why?
>
I will remove it. Sorry about that.
>> +/**
>> + * 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;
>> + 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.
>> +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.
>
I will look at other drivers to fix it.
>> + int assert_irq;
>> + int deassert_irq;
>
>I don't quite understand these two, but let's see in the rest of the driver.
>
>> + .deb_offset = (unsigned int []){ 0x30, 0x34, 0x38, 0x3c, 0x40,
>> + 0x44, 0x48, 0x4c },
>(...)
>> + .deb_offset = (unsigned int []){ 0x50 },
>
>So clearly u8[]
>
I will fix it.
>> +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.
>> + 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);
>> + } 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?
>
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.)
>> +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 .
>> +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.
>
I will use the GPIOLIB_IRQCHIP helpers instead.
>> + 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);
>> +
>> + 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.
>
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.
>> +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);
>> + 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.
>
I will create a wrapper to avoid duplication.
>> +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;
>
>> + int ret;
>> + int i;
>> +
>> + node = pdev->dev.of_node;
>
>Use #include <linux/property.h>
>
>> + 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.
>> +
>> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>
>With a local dev you can just devm_kzalloc(dev, ...) etc.
>
I will fix it.
>> + 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.
>> + data->base = of_iomap(node, 0);
>> + if (!data->base)
>> + return -ENXIO;
>
>Use
>data->base = devm_platform_ioremap_resource(pdev, 0);
>
>> + data->irq_base = of_iomap(node, 1);
>> + if (!data->irq_base)
>> + return -ENXIO;
>
>Use
>data->irq_base = platform_get_irq(pdev, 1);
>
>> + data->gpio_chip.parent = &pdev->dev;
>
>Don't assign this, the core will handle it.
>
I will remove it.
>> + 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.
>> + 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.
>
I will use the helpers instead.
>> + 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.
>> + 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);
>> + }
>> +
>> + 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.
>
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?
>> +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.
>Yours,
>Linus Walleij
Thanks,
Tzuyi Chang
On Thu, Nov 2, 2023 at 1:40 PM TY_Chang[張子逸] <[email protected]> wrote:
> >On Wed, Nov 1, 2023 at 3:58 AM Tzuyi Chang <[email protected]> wrote:
> >> +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.
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
Hi Krzysztof,
>>>> +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);
>>>> + 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.
>
>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