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
