Re: [PATCH] gpio: axp: Add support for devicetree

[Andre Przywara <andre.przywara@arm.com>]

Hi Denis,

thanks for sending this patch!

On 1/21/26 12:54, Denis Burkov wrote:

I am afraid you need to put some commit message in here. You can refer 
to the comment you are removing, so that there *is* now DT support for 
the AXP GPIOs. But also please mention what this fixes - why do we need 
this patch? If you can provide an example usage, it would be easier to 
take this patch.

> Signed-off-by: Denis Burkov <hitechshell@mail.ru>
> ---
>   board/sunxi/board.c     |  4 ----
>   drivers/gpio/axp_gpio.c | 28 +++++++++++-----------------
>   2 files changed, 11 insertions(+), 21 deletions(-)
> 
> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index 2929bc17f08..3c61bfb8822 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -226,10 +226,6 @@ int board_init(void)
>   	}
>   #endif /* !CONFIG_ARM64 && !CONFIG_MACH_SUNIV */
>   
> -	ret = axp_gpio_init();
> -	if (ret)
> -		return ret;
> -
>   	eth_init_board();
>   
>   	return 0;
> diff --git a/drivers/gpio/axp_gpio.c b/drivers/gpio/axp_gpio.c
> index 181c53bfe72..1c5882a25d4 100644
> --- a/drivers/gpio/axp_gpio.c
> +++ b/drivers/gpio/axp_gpio.c
> @@ -98,35 +98,29 @@ static const struct dm_gpio_ops gpio_axp_ops = {
>   
>   static int gpio_axp_probe(struct udevice *dev)
>   {
> +	int ret;
>   	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
>   
>   	/* Tell the uclass how many GPIOs we have */
>   	uc_priv->bank_name = AXP_GPIO_PREFIX;
>   	uc_priv->gpio_count = AXP_GPIO_COUNT;
>   
> +	ret = pmic_bus_init();
> +	if (ret)
> +		return ret;
> +
>   	return 0;
>   }
>   
> +static const struct udevice_id gpio_axp_match[] = {
> +	{ .compatible = "x-powers,axp209-gpio" },

Why is it only this compatible string? What about the others? I guess 
the driver was just not enabled on newer AXPs before?
Can you maybe look what it takes to support the other PMICs? If you look 
at the Linux driver, that's mostly about what pins each AXP supports.

And peeking into the driver here, there are some #ifdef's in there 
(AXP_GPIO[23]_CTRL), I think now collides with the DT description, and 
whatever this was doing should be detected at runtime, potentially by 
supporting multiple compatible strings.

Cheers,
Andre

> +	{ }
> +};
> +
>   U_BOOT_DRIVER(gpio_axp) = {
>   	.name	= "gpio_axp",
>   	.id	= UCLASS_GPIO,
> +	.of_match = gpio_axp_match,
>   	.ops	= &gpio_axp_ops,
>   	.probe	= gpio_axp_probe,
>   };
> -
> -int axp_gpio_init(void)
> -{
> -	struct udevice *dev;
> -	int ret;
> -
> -	ret = pmic_bus_init();
> -	if (ret)
> -		return ret;
> -
> -	/* There is no devicetree support for the axp yet, so bind directly */
> -	ret = device_bind_driver(dm_root(), "gpio_axp", "AXP-gpio", &dev);
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> -}