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

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

[Andre Przywara]

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;
> -}

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

[Yixun Lan]

Hi Denis,

On 15:50 Fri 19 Jun     , Andre Przywara wrote:
> 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.
> 
I agree, describe what's problem to solve.. mainly I think this patch
coverting the driver into device tree aware, so it will probe axp-gpio
based on if the target dts support it or not..

> > 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.
> 
For Linux driver, I think Andre refer to the AXP209 GPIO driver [1]
and two more compatibles "x-powers,axp221-gpio", "x-powers,axp813-gpio" are needed

Please also check the ".gpio_status_offset" which differ in device data

Link: https://elixir.bootlin.com/linux/v7.1/source/drivers/pinctrl/pinctrl-axp209.c#L429 [1]
> 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.
> 
Agree, should fold these info into compatible data with multiple compatible

-- 
Yixun Lan (dlan)