Hi Alvin, thanks for your patch! On Fri, May 17, 2024 at 2:58 PM Alvin Šipraga <alvin@xxxxxxx> wrote: > From: Alvin Šipraga <alsi@xxxxxxxxxxxxxxx> > > This driver adds GPIO function support for AD24xx A2B transceiver chips. > When a GPIO is requested, the relevant pin is automatically muxed to > GPIO mode. The device tree property gpio-reserved-ranges can be used to > protect certain pins which are reserved for other functionality such as > I2S/TDM data. > > Signed-off-by: Alvin Šipraga <alsi@xxxxxxxxxxxxxxx> (...) > config A2B_AD24XX_NODE > tristate "Analog Devices Inc. AD24xx node support" > select REGMAP_A2B > + imply GPIO_AD24XX Maybe it should even be select, if it's hard to think about a case where this is not desired? > +config GPIO_AD24XX > + tristate "Analog Devies Inc. AD24xx GPIO support" > + depends on A2B_AD24XX_NODE > + help > + Say Y here to enable GPIO support for AD24xx A2B transceivers. > + > config GPIO_ARIZONA > tristate "Wolfson Microelectronics Arizona class devices" > depends on MFD_ARIZONA This is grouped with the MFD devices but as I understand it A2B is a completely new bus type? Is MFD even always selected when A2B is in use? To me it's fine to add a new submenu for A2B devices, if there will be more of them. > +static int ad24xx_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) > +{ > + struct ad24xx_gpio *adg = gpiochip_get_data(gc); > + unsigned int val; > + int ret; > + > + ret = regmap_read(adg->regmap, A2B_GPIOOEN, &val); > + if (ret) > + return ret; > + > + if (val & BIT(offset)) > + return 0; /* output */ > + > + return 1; /* input */ Please use GPIO_LINE_DIRECTION_OUT/GPIO_LINE_DIRECTION_IN instead of 0/1 here? Then you don't need the comments because it's evident. > +static int ad24xx_gpio_get(struct gpio_chip *gc, unsigned int offset) > +{ > + struct ad24xx_gpio *adg = gpiochip_get_data(gc); > + unsigned int val; > + int ret; > + > + ret = regmap_read(adg->regmap, A2B_GPIOIN, &val); > + if (ret) > + return ret; > + > + if (val & BIT(offset)) > + return 1; /* high */ > + > + return 0; /* low */ Just return !!(val & BIT(offset)); > +static int ad24xx_gpio_child_to_parent_hwirq(struct gpio_chip *gc, > + unsigned int child, > + unsigned int child_type, > + unsigned int *parent, > + unsigned int *parent_type) > +{ > + *parent = child; > + return 0; > +} This deserves a comment, is IRQ 0 the singular parent of everything? Then it doesn't seem very hierarchical but rather cascaded don't you think? > +static int ad24xx_gpio_irq_set_type(struct irq_data *d, unsigned int type) > +{ > + struct gpio_chip *gpio_chip = irq_data_get_irq_chip_data(d); > + struct ad24xx_gpio *adg = gpiochip_get_data(gpio_chip); > + irq_hw_number_t hwirq = irqd_to_hwirq(d); > + > + switch (type) { > + case IRQ_TYPE_EDGE_RISING: > + adg->irq_invert &= ~BIT(hwirq); > + break; > + case IRQ_TYPE_EDGE_FALLING: > + adg->irq_invert |= BIT(hwirq); > + break; > + default: > + return -EINVAL; > + } No need for the "toggling trick" for supporting IRQ on both edges? Implementing that hack (which is in several drivers) will be nice to have for e.g. pushbuttons. > +static void ad24xx_gpio_irq_bus_lock(struct irq_data *d) > +{ > + struct gpio_chip *gpio_chip = irq_data_get_irq_chip_data(d); > + struct ad24xx_gpio *adg = gpiochip_get_data(gpio_chip); > + > + mutex_lock(&adg->mutex); > +} Is this mutex needed since there is already a mutex or spinlock in the regmap? Isn't this the case for A2B? Yours, Linus Walleij