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
