[v2,2/3] iio: adc: sun20i-gpadc: add A523 gpadc support

A523 differs from existing sun20i-gpadc-iio by having two clocks; bus
clock and module clock.

Change driver to enable all clocks.

Signed-off-by: Michal Piekos <michal.piekos@mmpsystems.pl>
---
 drivers/iio/adc/sun20i-gpadc-iio.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
  

On 13 May 2026 10:29:43 am IST, Michal Piekos <michal.piekos@mmpsystems.pl> wrote:
>A523 differs from existing sun20i-gpadc-iio by having two clocks; bus
>clock and module clock.
>
>Change driver to enable all clocks.
>
>Signed-off-by: Michal Piekos <michal.piekos@mmpsystems.pl>
>---
> drivers/iio/adc/sun20i-gpadc-iio.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/iio/adc/sun20i-gpadc-iio.c b/drivers/iio/adc/sun20i-gpadc-iio.c
>index 861c14da75ad..3f1f07b3a385 100644
>--- a/drivers/iio/adc/sun20i-gpadc-iio.c
>+++ b/drivers/iio/adc/sun20i-gpadc-iio.c
>@@ -180,7 +180,7 @@ static int sun20i_gpadc_probe(struct platform_device *pdev)
> 	struct iio_dev *indio_dev;
> 	struct sun20i_gpadc_iio *info;
> 	struct reset_control *rst;
>-	struct clk *clk;
>+	struct clk_bulk_data *clks;
> 	int irq;
> 	int ret;
> 
>@@ -205,9 +205,11 @@ static int sun20i_gpadc_probe(struct platform_device *pdev)
> 	if (IS_ERR(info->regs))
> 		return PTR_ERR(info->regs);
> 
>-	clk = devm_clk_get_enabled(dev, NULL);
>-	if (IS_ERR(clk))
>-		return dev_err_probe(dev, PTR_ERR(clk), "failed to enable bus clock\n");
>+	ret = devm_clk_bulk_get_all_enabled(dev, &clks);
>+	if (ret <= 0)

Thank you Michal for the change.

Have you validated the changes ?
It looks while success ret would be 0 and it would give return error.

Thanks, Sanjay


>+		return dev_err_probe(
>+			dev, ret,
>+			"failed to enable clocks or no clocks defined\n");
> 
> 	rst = devm_reset_control_get_exclusive(dev, NULL);
> 	if (IS_ERR(rst))
>@@ -243,6 +245,7 @@ static int sun20i_gpadc_probe(struct platform_device *pdev)
> 
> static const struct of_device_id sun20i_gpadc_of_id[] = {
> 	{ .compatible = "allwinner,sun20i-d1-gpadc" },
>+	{ .compatible = "allwinner,sun55i-a523-gpadc" },
> 	{ }
> };
> MODULE_DEVICE_TABLE(of, sun20i_gpadc_of_id);
>
  

Hi Sanjay,

thanks for having a look!

On 5/13/26 13:44, Sanjay Chitroda wrote:
> 
> 
> On 13 May 2026 10:29:43 am IST, Michal Piekos <michal.piekos@mmpsystems.pl> wrote:
>> A523 differs from existing sun20i-gpadc-iio by having two clocks; bus
>> clock and module clock.
>>
>> Change driver to enable all clocks.
>>
>> Signed-off-by: Michal Piekos <michal.piekos@mmpsystems.pl>
>> ---
>> drivers/iio/adc/sun20i-gpadc-iio.c | 11 +++++++----
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iio/adc/sun20i-gpadc-iio.c b/drivers/iio/adc/sun20i-gpadc-iio.c
>> index 861c14da75ad..3f1f07b3a385 100644
>> --- a/drivers/iio/adc/sun20i-gpadc-iio.c
>> +++ b/drivers/iio/adc/sun20i-gpadc-iio.c
>> @@ -180,7 +180,7 @@ static int sun20i_gpadc_probe(struct platform_device *pdev)
>> 	struct iio_dev *indio_dev;
>> 	struct sun20i_gpadc_iio *info;
>> 	struct reset_control *rst;
>> -	struct clk *clk;
>> +	struct clk_bulk_data *clks;
>> 	int irq;
>> 	int ret;
>>
>> @@ -205,9 +205,11 @@ static int sun20i_gpadc_probe(struct platform_device *pdev)
>> 	if (IS_ERR(info->regs))
>> 		return PTR_ERR(info->regs);
>>
>> -	clk = devm_clk_get_enabled(dev, NULL);
>> -	if (IS_ERR(clk))
>> -		return dev_err_probe(dev, PTR_ERR(clk), "failed to enable bus clock\n");
>> +	ret = devm_clk_bulk_get_all_enabled(dev, &clks);
>> +	if (ret <= 0)
> 
> Thank you Michal for the change.
> 
> Have you validated the changes ?
> It looks while success ret would be 0 and it would give return error.

But devm_clk_bulk_get_all_enabled() returns the number of clocks found 
and enabled. And since we need at least one, I think this is correct, 
and the error message below reflects that.

To me that change looks good:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre


> 
> Thanks, Sanjay
> 
> 
>> +		return dev_err_probe(
>> +			dev, ret,
>> +			"failed to enable clocks or no clocks defined\n");
>>
>> 	rst = devm_reset_control_get_exclusive(dev, NULL);
>> 	if (IS_ERR(rst))
>> @@ -243,6 +245,7 @@ static int sun20i_gpadc_probe(struct platform_device *pdev)
>>
>> static const struct of_device_id sun20i_gpadc_of_id[] = {
>> 	{ .compatible = "allwinner,sun20i-d1-gpadc" },
>> +	{ .compatible = "allwinner,sun55i-a523-gpadc" },
>> 	{ }
>> };
>> MODULE_DEVICE_TABLE(of, sun20i_gpadc_of_id);
>>
>
  

On Wed, 13 May 2026 13:53:49 +0200
Andre Przywara <andre.przywara@arm.com> wrote:

> Hi Sanjay,
> 
> thanks for having a look!
> 
> On 5/13/26 13:44, Sanjay Chitroda wrote:
> > 
> > 
> > On 13 May 2026 10:29:43 am IST, Michal Piekos <michal.piekos@mmpsystems.pl> wrote:  
> >> A523 differs from existing sun20i-gpadc-iio by having two clocks; bus
> >> clock and module clock.
> >>
> >> Change driver to enable all clocks.
> >>
> >> Signed-off-by: Michal Piekos <michal.piekos@mmpsystems.pl>
> >> ---
> >> drivers/iio/adc/sun20i-gpadc-iio.c | 11 +++++++----
> >> 1 file changed, 7 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/sun20i-gpadc-iio.c b/drivers/iio/adc/sun20i-gpadc-iio.c
> >> index 861c14da75ad..3f1f07b3a385 100644
> >> --- a/drivers/iio/adc/sun20i-gpadc-iio.c
> >> +++ b/drivers/iio/adc/sun20i-gpadc-iio.c
> >> @@ -180,7 +180,7 @@ static int sun20i_gpadc_probe(struct platform_device *pdev)
> >> 	struct iio_dev *indio_dev;
> >> 	struct sun20i_gpadc_iio *info;
> >> 	struct reset_control *rst;
> >> -	struct clk *clk;
> >> +	struct clk_bulk_data *clks;
> >> 	int irq;
> >> 	int ret;
> >>
> >> @@ -205,9 +205,11 @@ static int sun20i_gpadc_probe(struct platform_device *pdev)
> >> 	if (IS_ERR(info->regs))
> >> 		return PTR_ERR(info->regs);
> >>
> >> -	clk = devm_clk_get_enabled(dev, NULL);
> >> -	if (IS_ERR(clk))
> >> -		return dev_err_probe(dev, PTR_ERR(clk), "failed to enable bus clock\n");
> >> +	ret = devm_clk_bulk_get_all_enabled(dev, &clks);
> >> +	if (ret <= 0)  
> > 
> > Thank you Michal for the change.
> > 
> > Have you validated the changes ?
> > It looks while success ret would be 0 and it would give return error.  
> 
> But devm_clk_bulk_get_all_enabled() returns the number of clocks found 
> and enabled. And since we need at least one, I think this is correct, 
> and the error message below reflects that.

True but passing 0 to dev_err_probe() isn't going to do the right thing.

Though from this function, 0 is an error you need to return an error code
not 0 which to the caller looks like a success.



> 
> To me that change looks good:
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> 
> Cheers,
> Andre
> 
> 
> > 
> > Thanks, Sanjay
> > 
> >   
> >> +		return dev_err_probe(
> >> +			dev, ret,
> >> +			"failed to enable clocks or no clocks defined\n");
> >>
> >> 	rst = devm_reset_control_get_exclusive(dev, NULL);
> >> 	if (IS_ERR(rst))
> >> @@ -243,6 +245,7 @@ static int sun20i_gpadc_probe(struct platform_device *pdev)
> >>
> >> static const struct of_device_id sun20i_gpadc_of_id[] = {
> >> 	{ .compatible = "allwinner,sun20i-d1-gpadc" },
> >> +	{ .compatible = "allwinner,sun55i-a523-gpadc" },
> >> 	{ }
> >> };
> >> MODULE_DEVICE_TABLE(of, sun20i_gpadc_of_id);
> >>  
> >   
>
  

On Wed, May 13, 2026 at 06:59:43AM +0200, Michal Piekos wrote:
> A523 differs from existing sun20i-gpadc-iio by having two clocks; bus
> clock and module clock.
> 
> Change driver to enable all clocks.

...

>  	struct iio_dev *indio_dev;
>  	struct sun20i_gpadc_iio *info;
>  	struct reset_control *rst;
> -	struct clk *clk;
> +	struct clk_bulk_data *clks;

Try to follow reversed xmas tree order.

>  	int irq;
>  	int ret;

...

> -		return dev_err_probe(dev, PTR_ERR(clk), "failed to enable bus clock\n");
> +	ret = devm_clk_bulk_get_all_enabled(dev, &clks);
> +	if (ret <= 0)
> +		return dev_err_probe(
> +			dev, ret,
> +			"failed to enable clocks or no clocks defined\n");

Is this done by clang-format or so? Please, don't do wrapping on the open
parenthesis. Also note for more than 10 years checkpatch does not complain
on the trailing string literals that go over 80 characters.
  

On Wed, May 13, 2026 at 01:53:49PM +0200, Andre Przywara wrote:
> On 5/13/26 13:44, Sanjay Chitroda wrote:
> > On 13 May 2026 10:29:43 am IST, Michal Piekos <michal.piekos@mmpsystems.pl> wrote:

> > > +	if (ret <= 0)
> > 
> > Thank you Michal for the change.
> > 
> > Have you validated the changes ?
> > It looks while success ret would be 0 and it would give return error.

Good catch!

> But devm_clk_bulk_get_all_enabled() returns the number of clocks found and
> enabled. And since we need at least one, I think this is correct, and the
> error message below reflects that.
> 
> To me that change looks good:
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

== 0 ???
Doesn't look like correct code.
  

On Wed, 13 May 2026 23:12:05 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

Hi Andy,

> On Wed, May 13, 2026 at 01:53:49PM +0200, Andre Przywara wrote:
> > On 5/13/26 13:44, Sanjay Chitroda wrote:  
> > > On 13 May 2026 10:29:43 am IST, Michal Piekos <michal.piekos@mmpsystems.pl> wrote:  
> 
> > > > +	if (ret <= 0)  
> > > 
> > > Thank you Michal for the change.
> > > 
> > > Have you validated the changes ?
> > > It looks while success ret would be 0 and it would give return error.

No, it doesn't. Returning 0 means no clocks found:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-devres.c#n300

> 
> Good catch!
> 
> > But devm_clk_bulk_get_all_enabled() returns the number of clocks found and
> > enabled. And since we need at least one, I think this is correct, and the
> > error message below reflects that.
> > 
> > To me that change looks good:
> > 
> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>  
> 
> == 0 ???
> Doesn't look like correct code.

Not sure I follow:
devm_clk_bulk_get_all_enabled() returns the number of clocks in that
node, or a negative error value. If it returns 0, that means no clocks
have been found, which is an error in our case, since we expect at
least one clock. This is what the second part of the error message
refers to.
So we want one or two as the return value, with the current bindings,
but really anything greater than 0 is fine, from the driver's
perspective, since we don't care about the clocks beyond them being
enabled.

So am I missing something?

Cheers,
Andre
  

On Wed, May 13, 2026 at 11:19:01PM +0200, Andre Przywara wrote:
> On Wed, 13 May 2026 23:12:05 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Wed, May 13, 2026 at 01:53:49PM +0200, Andre Przywara wrote:
> > > On 5/13/26 13:44, Sanjay Chitroda wrote:  
> > > > On 13 May 2026 10:29:43 am IST, Michal Piekos <michal.piekos@mmpsystems.pl> wrote:  

...

> > > > > +	if (ret <= 0)  
> > > > 
> > > > Thank you Michal for the change.
> > > > 
> > > > Have you validated the changes ?
> > > > It looks while success ret would be 0 and it would give return error.
> 
> No, it doesn't. Returning 0 means no clocks found:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-devres.c#n300
> 
> > Good catch!
> > 
> > > But devm_clk_bulk_get_all_enabled() returns the number of clocks found and
> > > enabled. And since we need at least one, I think this is correct, and the
> > > error message below reflects that.
> > > 
> > > To me that change looks good:
> > > 
> > > Reviewed-by: Andre Przywara <andre.przywara@arm.com>  
> > 
> > == 0 ???
> > Doesn't look like correct code.
> 
> Not sure I follow:
> devm_clk_bulk_get_all_enabled() returns the number of clocks in that
> node, or a negative error value. If it returns 0, that means no clocks
> have been found,

Not in this code. Here it will be resent to the caller as success.

> which is an error in our case, since we expect at
> least one clock. This is what the second part of the error message
> refers to.

But not the error code itself! There will be no error message, IIRC the
implementation of dev_err_probe().

> So we want one or two as the return value, with the current bindings,
> but really anything greater than 0 is fine, from the driver's
> perspective, since we don't care about the clocks beyond them being
> enabled.
> 
> So am I missing something?

Yes!

You returned that to the caller, meaning everything is fine. There is a success
that is returned. The code is buggy (okay, not that, it rather will behave not
as intended).

TL;DR:
You should have something like
	if (ret < 0)
		return dev_err_probe(ret);
	if (ret == 0)
		return dev_err_probe(-Exxx, "Needs at least one clock!\n");
  

On Wed, 13 May 2026 17:16:38 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

Hi,

> On Wed, 13 May 2026 13:53:49 +0200
> Andre Przywara <andre.przywara@arm.com> wrote:
> 
> > Hi Sanjay,
> > 
> > thanks for having a look!
> > 
> > On 5/13/26 13:44, Sanjay Chitroda wrote:  
> > > 
> > > 
> > > On 13 May 2026 10:29:43 am IST, Michal Piekos <michal.piekos@mmpsystems.pl> wrote:    
> > >> A523 differs from existing sun20i-gpadc-iio by having two clocks; bus
> > >> clock and module clock.
> > >>
> > >> Change driver to enable all clocks.
> > >>
> > >> Signed-off-by: Michal Piekos <michal.piekos@mmpsystems.pl>
> > >> ---
> > >> drivers/iio/adc/sun20i-gpadc-iio.c | 11 +++++++----
> > >> 1 file changed, 7 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/drivers/iio/adc/sun20i-gpadc-iio.c b/drivers/iio/adc/sun20i-gpadc-iio.c
> > >> index 861c14da75ad..3f1f07b3a385 100644
> > >> --- a/drivers/iio/adc/sun20i-gpadc-iio.c
> > >> +++ b/drivers/iio/adc/sun20i-gpadc-iio.c
> > >> @@ -180,7 +180,7 @@ static int sun20i_gpadc_probe(struct platform_device *pdev)
> > >> 	struct iio_dev *indio_dev;
> > >> 	struct sun20i_gpadc_iio *info;
> > >> 	struct reset_control *rst;
> > >> -	struct clk *clk;
> > >> +	struct clk_bulk_data *clks;
> > >> 	int irq;
> > >> 	int ret;
> > >>
> > >> @@ -205,9 +205,11 @@ static int sun20i_gpadc_probe(struct platform_device *pdev)
> > >> 	if (IS_ERR(info->regs))
> > >> 		return PTR_ERR(info->regs);
> > >>
> > >> -	clk = devm_clk_get_enabled(dev, NULL);
> > >> -	if (IS_ERR(clk))
> > >> -		return dev_err_probe(dev, PTR_ERR(clk), "failed to enable bus clock\n");
> > >> +	ret = devm_clk_bulk_get_all_enabled(dev, &clks);
> > >> +	if (ret <= 0)    
> > > 
> > > Thank you Michal for the change.
> > > 
> > > Have you validated the changes ?
> > > It looks while success ret would be 0 and it would give return error.    
> > 
> > But devm_clk_bulk_get_all_enabled() returns the number of clocks found 
> > and enabled. And since we need at least one, I think this is correct, 
> > and the error message below reflects that.  
> 
> True but passing 0 to dev_err_probe() isn't going to do the right thing.
> 
> Though from this function, 0 is an error you need to return an error code
> not 0 which to the caller looks like a success.

Ah, that's true - should have read your email first before answering to
Andy ;-)

So yeah, that needs a split handling, for == 0, and for < 0.

Cheers,
Andre

> 
> 
> 
> > 
> > To me that change looks good:
> > 
> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> > 
> > Cheers,
> > Andre
> > 
> >   
> > > 
> > > Thanks, Sanjay
> > > 
> > >     
> > >> +		return dev_err_probe(
> > >> +			dev, ret,
> > >> +			"failed to enable clocks or no clocks defined\n");
> > >>
> > >> 	rst = devm_reset_control_get_exclusive(dev, NULL);
> > >> 	if (IS_ERR(rst))
> > >> @@ -243,6 +245,7 @@ static int sun20i_gpadc_probe(struct platform_device *pdev)
> > >>
> > >> static const struct of_device_id sun20i_gpadc_of_id[] = {
> > >> 	{ .compatible = "allwinner,sun20i-d1-gpadc" },
> > >> +	{ .compatible = "allwinner,sun55i-a523-gpadc" },
> > >> 	{ }
> > >> };
> > >> MODULE_DEVICE_TABLE(of, sun20i_gpadc_of_id);
> > >>    
> > >     
> >   
> 
>
  

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] If `devm_clk_bulk_get_all_enabled()` returns 0 (meaning no clocks are defined in the device tree), the driver aborts the probe early but incorrectly returns 0 (success).
--

commit 6e1c58e982371b9be677bdb51d74117b3577cd6f
Author: Michal Piekos <michal.piekos@mmpsystems.pl>

iio: adc: sun20i-gpadc: add A523 gpadc support

This commit adds support for the A523 gpadc by switching to the bulk clock
API to handle both bus and module clocks.

> diff --git a/drivers/iio/adc/sun20i-gpadc-iio.c b/drivers/iio/adc/sun20i-gpadc-iio.c
> index 861c14da75add..3f1f07b3a3856 100644
> --- a/drivers/iio/adc/sun20i-gpadc-iio.c
> +++ b/drivers/iio/adc/sun20i-gpadc-iio.c
[ ... ]
> @@ -205,9 +205,11 @@ static int sun20i_gpadc_probe(struct platform_device *pdev)
>  	if (IS_ERR(info->regs))
>  		return PTR_ERR(info->regs);
>  
> -	clk = devm_clk_get_enabled(dev, NULL);
> -	if (IS_ERR(clk))
> -		return dev_err_probe(dev, PTR_ERR(clk), "failed to enable bus clock\n");
> +	ret = devm_clk_bulk_get_all_enabled(dev, &clks);
> +	if (ret <= 0)
> +		return dev_err_probe(
> +			dev, ret,
> +			"failed to enable clocks or no clocks defined\n");

When devm_clk_bulk_get_all_enabled() returns 0 (meaning no clocks are defined),
will this code return 0 from the probe function?

Since dev_err_probe() returns the error code passed to it, returning 0 here
would cause sun20i_gpadc_probe() to succeed while skipping all remaining
initialization like reset deassertion, IRQ registration, and IIO device
registration.

Should ret be explicitly set to a negative error code (like -ENODEV) before
calling dev_err_probe() if ret is 0?