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

[Artamonovs, Arturs]

Thanks for feedback Linus, will address that in next patch version

> -----Original Message-----
> From: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Sent: Tuesday, October 1, 2024 1:45 PM
> To: Artamonovs, Arturs <Arturs.Artamonovs@xxxxxxxxxx>
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>; Will Deacon <will@xxxxxxxxxx>;
> Greg Malysa <greg.malysa@xxxxxxxxxxx>; Philipp Zabel
> <p.zabel@xxxxxxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzk+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; Agarwal, Utsav
> <Utsav.Agarwal@xxxxxxxxxx>; Michael Turquette <mturquette@xxxxxxxxxxxx>;
> Stephen Boyd <sboyd@xxxxxxxxxx>; Bartosz Golaszewski <brgl@xxxxxxxx>;
> Thomas Gleixner <tglx@xxxxxxxxxxxxx>; Andi Shyti <andi.shyti@xxxxxxxxxx>; Greg
> Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Jiri Slaby <jirislaby@xxxxxxxxxx>;
> Arnd Bergmann <arnd@xxxxxxxx>; Olof Johansson <olof@xxxxxxxxx>;
> soc@xxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-clk@xxxxxxxxxxxxxxx;
> linux-gpio@xxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; linux-
> serial@xxxxxxxxxxxxxxx; Linux Factory <adsp-linux@xxxxxxxxxx>; Nathan Barrett-
> Morrison <nathan.morrison@xxxxxxxxxxx>
> Subject: Re: [PATCH 09/21] gpio: add driver for ADI ADSP-SC5xx platform
> 
> [External]
> 
> 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