Message-ID: <CACRpkdZiKCiVsmkPnJRW1c5SBfzNmBqCH9bS8XsgUszD6H=vPg@mail.gmail.com>
Date:   Wed, 1 Nov 2023 09:39:55 +0100
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Tzuyi Chang <tychang@...ltek.com>
Cc:     Bartosz Golaszewski <brgl@...ev.pl>,
        Andy Shevchenko <andy@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>, linux-gpio@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH 1/2] gpio: realtek: Add GPIO support for RTD SoC variants

Hi Tzuyi!

thanks for your patch!

On Wed, Nov 1, 2023 at 3:58 AM Tzuyi Chang <tychang@...ltek.com> 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 <tychang@...ltek.com>
(...)
> +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

Powered by blists - more mailing lists