iio: adc: sophgo-saradc: Handle errors from optional IRQ lookup

[PATCH] iio: adc: sophgo-saradc: Handle errors from optional IRQ lookup

Posted by phucduc.bui@gmail.com 1 month, 2 weeks ago

From: bui duc phuc <phucduc.bui@gmail.com>

platform_get_irq_optional() returns a positive IRQ number on success or
a negative error code on failure. For an optional IRQ, -ENXIO indicates
that no optional IRQ is available. Other errors, such as -EPROBE_DEFER
and -EINVAL, should be propagated so that the caller can handle them
appropriately.

Propagate negative errors other than -ENXIO.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 drivers/iio/adc/sophgo-cv1800b-adc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/adc/sophgo-cv1800b-adc.c b/drivers/iio/adc/sophgo-cv1800b-adc.c
index bdc3e1326a9a..c6eed09acfcd 100644
--- a/drivers/iio/adc/sophgo-cv1800b-adc.c
+++ b/drivers/iio/adc/sophgo-cv1800b-adc.c
@@ -182,6 +182,8 @@ static int cv1800b_adc_probe(struct platform_device *pdev)
 		return PTR_ERR(saradc->regs);
 
 	saradc->irq = platform_get_irq_optional(pdev, 0);
+	if (saradc->irq < 0 && saradc->irq != -ENXIO)
+		return saradc->irq;
 	if (saradc->irq > 0) {
 		init_completion(&saradc->completion);
 		ret = devm_request_irq(dev, saradc->irq,
-- 
2.43.0

Re: [PATCH] iio: adc: sophgo-saradc: Handle errors from optional IRQ lookup

Posted by Jonathan Cameron 1 month, 1 week ago

On Thu, 13 Aug 2026 14:48:10 +0700
phucduc.bui@gmail.com wrote:

> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> platform_get_irq_optional() returns a positive IRQ number on success or
> a negative error code on failure. For an optional IRQ, -ENXIO indicates
> that no optional IRQ is available. Other errors, such as -EPROBE_DEFER
> and -EINVAL, should be propagated so that the caller can handle them
> appropriately.

That function is very much undocumented other than not printing a
message when it returns an error.  However I think you analysis is
correct.

I'm not going to rush this is because it is not known to have
been a problem in the wild (only odd loading orders should have
caused deferal).


> 
> Propagate negative errors other than -ENXIO.
> 
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
>  drivers/iio/adc/sophgo-cv1800b-adc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/adc/sophgo-cv1800b-adc.c b/drivers/iio/adc/sophgo-cv1800b-adc.c
> index bdc3e1326a9a..c6eed09acfcd 100644
> --- a/drivers/iio/adc/sophgo-cv1800b-adc.c
> +++ b/drivers/iio/adc/sophgo-cv1800b-adc.c
> @@ -182,6 +182,8 @@ static int cv1800b_adc_probe(struct platform_device *pdev)
>  		return PTR_ERR(saradc->regs);
>  
>  	saradc->irq = platform_get_irq_optional(pdev, 0);
> +	if (saradc->irq < 0 && saradc->irq != -ENXIO)
> +		return saradc->irq;
>  	if (saradc->irq > 0) {
>  		init_completion(&saradc->completion);
>  		ret = devm_request_irq(dev, saradc->irq,

Re: [PATCH] iio: adc: sophgo-saradc: Handle errors from optional IRQ lookup

Posted by Bui Duc Phuc 1 month, 1 week ago

Hi Jonathan,

Thank you for your feedback.

> >
> > platform_get_irq_optional() returns a positive IRQ number on success or
> > a negative error code on failure. For an optional IRQ, -ENXIO indicates
> > that no optional IRQ is available. Other errors, such as -EPROBE_DEFER
> > and -EINVAL, should be propagated so that the caller can handle them
> > appropriately.
>
> That function is very much undocumented other than not printing a
> message when it returns an error.  However I think you analysis is
> correct.
>

Yes, I agree. The documentation for this function may not be clear enough,
which could have led to error handling being implemented incorrectly
in some places.

There is also an inconsistency in this driver: if devm_request_irq() fails,
the error is returned and the probe fails. But if platform_get_irq_optional()
fails, the error is ignored.
I'm not sure whether the author misunderstood and assumed that any
negative return value simply means that there is no IRQ.

> I'm not going to rush this is because it is not known to have
> been a problem in the wild (only odd loading orders should have
> caused deferal).
>

I understand your point. However, in this case the error can be
completely hidden:
There is no error message or log, the error is not returned, and the
driver falls back
to polling:
-----------------------------------
if (saradc->irq < 0) {
        u32 reg;

        return readl_poll_timeout(saradc->regs + CV1800B_ADC_STATUS_REG,
                                                 reg, !(reg & CV1800B_ADC_BUSY),
                                                 500, CV1800B_READ_TIMEOUT_US);
}
------------------------------------

So there may never be an obvious failure for a user to report.

Best regards,
Phuc

Re: [PATCH] iio: adc: sophgo-saradc: Handle errors from optional IRQ lookup

Posted by Jonathan Cameron 1 month ago

On Sun, 23 Aug 2026 05:27:21 +0700
Bui Duc Phuc <phucduc.bui@gmail.com> wrote:

> Hi Jonathan,
> 
> Thank you for your feedback.
> 
> > >
> > > platform_get_irq_optional() returns a positive IRQ number on success or
> > > a negative error code on failure. For an optional IRQ, -ENXIO indicates
> > > that no optional IRQ is available. Other errors, such as -EPROBE_DEFER
> > > and -EINVAL, should be propagated so that the caller can handle them
> > > appropriately.  
> >
> > That function is very much undocumented other than not printing a
> > message when it returns an error.  However I think you analysis is
> > correct.
> >  
> 
> Yes, I agree. The documentation for this function may not be clear enough,
> which could have led to error handling being implemented incorrectly
> in some places.
> 
> There is also an inconsistency in this driver: if devm_request_irq() fails,
> the error is returned and the probe fails. But if platform_get_irq_optional()
> fails, the error is ignored.
> I'm not sure whether the author misunderstood and assumed that any
> negative return value simply means that there is no IRQ.

If the platform irq get fails because there isn't one in firmware, we expect
to just carry on (no interrupt support).  What we are missing is failing when
there is one but we get an error anyway.

For the later devm_request_irq() that is only called if we have an irq
from firmware, but something else goes wrong. That one should definitely 
always fail probe as it indicates a probe (rather than lack of interrupt
support)

> 
> > I'm not going to rush this is because it is not known to have
> > been a problem in the wild (only odd loading orders should have
> > caused deferal).
> >  
> 
> I understand your point. However, in this case the error can be
> completely hidden:
> There is no error message or log, the error is not returned, and the
> driver falls back
> to polling:
> -----------------------------------
> if (saradc->irq < 0) {
>         u32 reg;
> 
>         return readl_poll_timeout(saradc->regs + CV1800B_ADC_STATUS_REG,
>                                                  reg, !(reg & CV1800B_ADC_BUSY),
>                                                  500, CV1800B_READ_TIMEOUT_US);
> }
> ------------------------------------
> 
> So there may never be an obvious failure for a user to report.

Without evidence that there are real setups where interrupt controller
loads late enough to result in a deferral + are used with this chip
I'm fine with the small risk of just using polling in kernels prior
to the fix.  Everything still works, just potentially less efficiently.

Jonathan

> 
> Best regards,
> Phuc