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

[TY_Chang[張子逸]]

Hi Linus,

Thank you for the review!

>
>Hi Tzuyi!
>
>thanks for your patch!
>
>On Wed, Nov 1, 2023 at 3:58 AM Tzuyi Chang <tychang@xxxxxxxxxxx> 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 <tychang@xxxxxxxxxxx>
>(...)
>> +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