Re: [PATCH 09/21] gpio: add driver for ADI ADSP-SC5xx platform

[Linus Walleij]

Hi Arturs,

thanks for your patch!

On Thu, Sep 12, 2024 at 8:20 PM Arturs Artamonovs via B4 Relay
<devnull+arturs.artamonovs.analog.com@xxxxxxxxxx> wrote:

> From: Arturs Artamonovs <arturs.artamonovs@xxxxxxxxxx>
>
> Add ADSP-SC5xx GPIO driver.
> - Support all GPIO ports
> - Each gpio support seperate PINT interrupt controller
>
> Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@xxxxxxxxxx>
> Co-developed-by: Nathan Barrett-Morrison <nathan.morrison@xxxxxxxxxxx>
> Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@xxxxxxxxxxx>
> Co-developed-by: Greg Malysa <greg.malysa@xxxxxxxxxxx>
> Signed-off-by: Greg Malysa <greg.malysa@xxxxxxxxxxx>

(...)

> +config GPIO_ADI_ADSP_PORT
> +       bool "ADI ADSP PORT GPIO driver"
> +       depends on OF_GPIO
> +       select GPIO_GENERIC

If you select this then you need to use it in the idiomatic way.

+#include <linux/soc/adi/adsp-gpio-port.h>

Drop this, just bring the contents into this file all register defines
etc.

+#include "gpiolib.h"

No way, do this:
#include <linux/gpio/driver.h>

> +static int adsp_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct adsp_gpio_port *port = to_adsp_gpio_port(chip);
> +
> +       __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_DIR_CLEAR);

Ah these __adsp_gpio_writew/readw things are too idiomatic. Just
use the base and common writew() please.

> +       __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_INEN_SET);

Interrupt enable in the direction function? No thanks, poke the
interrupt registers in your irqchip if you make one (you currently
do not) in this case I'd say just disable all interrupts in probe()
using something like writew(base + ADSP_PORT_REG_INEN_SET, 0xffff)
and be done with it.

> +static int adsp_gpio_get_value(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct adsp_gpio_port *port = to_adsp_gpio_port(chip);
> +
> +       return !!(__adsp_gpio_readw(port, ADSP_PORT_REG_DATA) & BIT(offset));
> +}

This becomes a reimplemenation of generic GPIO.

> +static int adsp_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct adsp_gpio_port *port = to_adsp_gpio_port(chip);
> +       irq_hw_number_t irq = offset + port->irq_offset;
> +       int map = irq_find_mapping(port->irq_domain, irq);
> +
> +       if (map)
> +               return map;
> +
> +       return irq_create_mapping(port->irq_domain, irq);
> +}

This irqdomain in the "port" looks weird.

Implement the irqchip in the GPIO driver instead.

If the domain *has* to be external to the GPIO driver then
you need to use hierarchical irqdomains.

> +static int adsp_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct adsp_gpio_port *gpio;
> +       int ret;
> +
> +       gpio = devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL);
> +       if (!gpio)
> +               return -ENOMEM;
> +
> +       gpio->regs = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(gpio->regs))
> +               return PTR_ERR(gpio->regs);

So you have gpio->regs which is the base.

> +       gpio->dev = dev;
> +
> +       ret = adsp_attach_pint_to_gpio(gpio);
> +       if  (ret)
> +               dev_err_probe(gpio->dev, ret, "error attaching interupt to gpio pin\n");
> +
> +       spin_lock_init(&gpio->lock);
> +
> +       gpio->gpio.label = "adsp-gpio";
> +       gpio->gpio.direction_input = adsp_gpio_direction_input;
> +       gpio->gpio.direction_output = adsp_gpio_direction_output;
> +       gpio->gpio.get = adsp_gpio_get_value;
> +       gpio->gpio.set = adsp_gpio_set_value;
> +       gpio->gpio.to_irq = adsp_gpio_to_irq;
> +       gpio->gpio.request = gpiochip_generic_request;
> +       gpio->gpio.free = gpiochip_generic_free;
> +       gpio->gpio.ngpio = ADSP_PORT_NGPIO;
> +       gpio->gpio.parent = dev;
> +       gpio->gpio.base = -1;
> +       return devm_gpiochip_add_data(dev, &gpio->gpio, gpio);

Look in e.g. drivers/gpio/gpio-ftgpio010.c for an example of
how to use generic GPIO (with an irqchip!). It will be something like:

        ret = bgpio_init(&g->gc, dev, 2,
                         gpio->regs + ADSP_PORT_REG_DATA,
                         gpio->regs + ADSP_PORT_REG_DATA_SET,
                         gpio->regs + ADSP_PORT_REG_DATA_CLEAR,
                         gpio->regs + ADSP_PORT_REG_DIR_SET,
                         gpio->regs + ADSP_PORT_REG_DIR_CLEAR,
                         0);
        if (ret) {
                dev_err(dev, "unable to init generic GPIO\n");
                goto dis_clk;
        }
        g->gc.label = dev_name(dev);
        g->gc.base = -1;
        g->gc.parent = dev;
        g->gc.owner = THIS_MODULE;
        /* ngpio is set by bgpio_init() */

You can augment the generic driver instance with an extra config function
to set the special open drain bits.

Yours,
Linus Walleij