[PATCH] iio: adc: ti_am335x_adc: Make DMAs optional

[PATCH] iio: adc: ti_am335x_adc: Make DMAs optional

[Wadim Egorov]

DMAs are optional. Even if the DMA request is unsuccessfully,
the ADC can still work properly.
Make tiadc_request_dma() not fail if we do not provide dmas &
dma-names properties.

This actually fixes the wrong error handling of the tiadc_request_dma()
result where the probing only failed if -EPROPE_DEFER was returned.

Fixes: f438b9da75eb ("drivers: iio: ti_am335x_adc: add dma support")

Signed-off-by: Wadim Egorov <w.egorov@phytec.de>
---
 drivers/iio/adc/ti_am335x_adc.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 8db7a01cb5fb..e14aa9254ab1 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -543,8 +543,11 @@ static int tiadc_request_dma(struct platform_device *pdev,
 	if (IS_ERR(dma->chan)) {
 		int ret = PTR_ERR(dma->chan);
 
+		if (ret != -ENODEV)
+			return dev_err_probe(&pdev->dev, ret,
+					     "RX DMA channel request failed\n");
 		dma->chan = NULL;
-		return ret;
+		return 0;
 	}
 
 	/* RX buffer */
@@ -670,7 +673,7 @@ static int tiadc_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, indio_dev);
 
 	err = tiadc_request_dma(pdev, adc_dev);
-	if (err && err == -EPROBE_DEFER)
+	if (err)
 		goto err_dma;
 
 	return 0;
-- 
2.25.1

Re: [PATCH] iio: adc: ti_am335x_adc: Make DMAs optional

[Jonathan Cameron]

On Thu, 14 Sep 2023 14:13:00 +0200
Wadim Egorov <w.egorov@phytec.de> wrote:

> DMAs are optional. Even if the DMA request is unsuccessfully,
> the ADC can still work properly.

> Make tiadc_request_dma() not fail if we do not provide dmas &
> dma-names properties.
> 
> This actually fixes the wrong error handling of the tiadc_request_dma()
> result where the probing only failed if -EPROPE_DEFER was returned.
> 
> Fixes: f438b9da75eb ("drivers: iio: ti_am335x_adc: add dma support")
> 
No line break here.  Fixes tag is part of the main tag block.
> Signed-off-by: Wadim Egorov <w.egorov@phytec.de>


> ---
>  drivers/iio/adc/ti_am335x_adc.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index 8db7a01cb5fb..e14aa9254ab1 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -543,8 +543,11 @@ static int tiadc_request_dma(struct platform_device *pdev,
>  	if (IS_ERR(dma->chan)) {
>  		int ret = PTR_ERR(dma->chan);
>  
> +		if (ret != -ENODEV)
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "RX DMA channel request failed\n");
>  		dma->chan = NULL;
> -		return ret;
> +		return 0;
>  	}
>  
>  	/* RX buffer */
> @@ -670,7 +673,7 @@ static int tiadc_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, indio_dev);
>  
>  	err = tiadc_request_dma(pdev, adc_dev);
> -	if (err && err == -EPROBE_DEFER)
> +	if (err)

So this looks like a more subtle change than you are describing.
In the original code, we backed off only if the return was a PROBE_DEFER, otherwise
we carried on.

Your change seems to make that happen for any non -ENODEV error, including PROBE_DEFER.
That's fine, but it's not what the description implies.

Whilst tiadc_request_dma will fail today if the dmas etc is not provided, that seems
like correct behavior to me.  A function requesting dma fails if it isn't available.
The handling of whether to carry on the job for the caller.

So I think it should just be
	if (err && err != -EINVAL)
		goto err_dma;

and no change in tiadc_request_dma()

However, the case you describe should have worked find with existing code
as it wasn't -EPROBE_DEFER, so I don't understand why you were looking at this
code block in the first place?

Jonathan


>  		goto err_dma;
>  
>  	return 0;

Re: [PATCH] iio: adc: ti_am335x_adc: Make DMAs optional

[Wadim Egorov]

Hi Jonathan,

Am 17.09.23 um 12:45 schrieb Jonathan Cameron:
> On Thu, 14 Sep 2023 14:13:00 +0200
> Wadim Egorov <w.egorov@phytec.de> wrote:
>
>> DMAs are optional. Even if the DMA request is unsuccessfully,
>> the ADC can still work properly.
>> Make tiadc_request_dma() not fail if we do not provide dmas &
>> dma-names properties.
>>
>> This actually fixes the wrong error handling of the tiadc_request_dma()
>> result where the probing only failed if -EPROPE_DEFER was returned.
>>
>> Fixes: f438b9da75eb ("drivers: iio: ti_am335x_adc: add dma support")
>>
> No line break here.  Fixes tag is part of the main tag block.
>> Signed-off-by: Wadim Egorov <w.egorov@phytec.de>
>
>> ---
>>   drivers/iio/adc/ti_am335x_adc.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
>> index 8db7a01cb5fb..e14aa9254ab1 100644
>> --- a/drivers/iio/adc/ti_am335x_adc.c
>> +++ b/drivers/iio/adc/ti_am335x_adc.c
>> @@ -543,8 +543,11 @@ static int tiadc_request_dma(struct platform_device *pdev,
>>   	if (IS_ERR(dma->chan)) {
>>   		int ret = PTR_ERR(dma->chan);
>>   
>> +		if (ret != -ENODEV)
>> +			return dev_err_probe(&pdev->dev, ret,
>> +					     "RX DMA channel request failed\n");
>>   		dma->chan = NULL;
>> -		return ret;
>> +		return 0;
>>   	}
>>   
>>   	/* RX buffer */
>> @@ -670,7 +673,7 @@ static int tiadc_probe(struct platform_device *pdev)
>>   	platform_set_drvdata(pdev, indio_dev);
>>   
>>   	err = tiadc_request_dma(pdev, adc_dev);
>> -	if (err && err == -EPROBE_DEFER)
>> +	if (err)
> So this looks like a more subtle change than you are describing.
> In the original code, we backed off only if the return was a PROBE_DEFER, otherwise
> we carried on.
>
> Your change seems to make that happen for any non -ENODEV error, including PROBE_DEFER.
> That's fine, but it's not what the description implies.
>
> Whilst tiadc_request_dma will fail today if the dmas etc is not provided, that seems
> like correct behavior to me.  A function requesting dma fails if it isn't available.
> The handling of whether to carry on the job for the caller.

That makes sense, yes. But stm32-adc is doing the same in its dma 
request function.
So I assumed we can do it like that.

>
> So I think it should just be
> 	if (err && err != -EINVAL)
> 		goto err_dma;

We will end up failing if no dmas are configured because the request 
returns -ENODEV.
So I think it needs to be a check for non -ENODEV.

>
> and no change in tiadc_request_dma()
>
> However, the case you describe should have worked find with existing code
> as it wasn't -EPROBE_DEFER, so I don't understand why you were looking at this
> code block in the first place?

Providing wrong dmas in the device tree should've made the driver fail 
to probe.

Regards,
Wadim

>
> Jonathan
>
>
>>   		goto err_dma;
>>   
>>   	return 0;

Re: [PATCH] iio: adc: ti_am335x_adc: Make DMAs optional

[Jonathan Cameron]

On Tue, 19 Sep 2023 10:21:28 +0000
Wadim Egorov <W.Egorov@phytec.de> wrote:

> Hi Jonathan,
> 
> Am 17.09.23 um 12:45 schrieb Jonathan Cameron:
> > On Thu, 14 Sep 2023 14:13:00 +0200
> > Wadim Egorov <w.egorov@phytec.de> wrote:
> >  
> >> DMAs are optional. Even if the DMA request is unsuccessfully,
> >> the ADC can still work properly.
> >> Make tiadc_request_dma() not fail if we do not provide dmas &
> >> dma-names properties.
> >>
> >> This actually fixes the wrong error handling of the tiadc_request_dma()
> >> result where the probing only failed if -EPROPE_DEFER was returned.
> >>
> >> Fixes: f438b9da75eb ("drivers: iio: ti_am335x_adc: add dma support")
> >>  
> > No line break here.  Fixes tag is part of the main tag block.  
> >> Signed-off-by: Wadim Egorov <w.egorov@phytec.de>  
> >  
> >> ---
> >>   drivers/iio/adc/ti_am335x_adc.c | 7 +++++--
> >>   1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> >> index 8db7a01cb5fb..e14aa9254ab1 100644
> >> --- a/drivers/iio/adc/ti_am335x_adc.c
> >> +++ b/drivers/iio/adc/ti_am335x_adc.c
> >> @@ -543,8 +543,11 @@ static int tiadc_request_dma(struct platform_device *pdev,
> >>   	if (IS_ERR(dma->chan)) {
> >>   		int ret = PTR_ERR(dma->chan);
> >>   
> >> +		if (ret != -ENODEV)
> >> +			return dev_err_probe(&pdev->dev, ret,
> >> +					     "RX DMA channel request failed\n");
> >>   		dma->chan = NULL;
> >> -		return ret;
> >> +		return 0;
> >>   	}
> >>   
> >>   	/* RX buffer */
> >> @@ -670,7 +673,7 @@ static int tiadc_probe(struct platform_device *pdev)
> >>   	platform_set_drvdata(pdev, indio_dev);
> >>   
> >>   	err = tiadc_request_dma(pdev, adc_dev);
> >> -	if (err && err == -EPROBE_DEFER)
> >> +	if (err)  
> > So this looks like a more subtle change than you are describing.
> > In the original code, we backed off only if the return was a PROBE_DEFER, otherwise
> > we carried on.
> >
> > Your change seems to make that happen for any non -ENODEV error, including PROBE_DEFER.
> > That's fine, but it's not what the description implies.
> >
> > Whilst tiadc_request_dma will fail today if the dmas etc is not provided, that seems
> > like correct behavior to me.  A function requesting dma fails if it isn't available.
> > The handling of whether to carry on the job for the caller.  
> 
> That makes sense, yes. But stm32-adc is doing the same in its dma 
> request function.
> So I assumed we can do it like that.
> 
> >
> > So I think it should just be
> > 	if (err && err != -EINVAL)
> > 		goto err_dma;  
> 
> We will end up failing if no dmas are configured because the request 
> returns -ENODEV.
> So I think it needs to be a check for non -ENODEV.

That makes sense. I wonder if a long time back that returned -EINVAL, hence the
wrong value here. If you can do a bit of checking in the git history that would
be good as it will change how far we backport this.

> 
> >
> > and no change in tiadc_request_dma()
> >
> > However, the case you describe should have worked find with existing code
> > as it wasn't -EPROBE_DEFER, so I don't understand why you were looking at this
> > code block in the first place?  
> 
> Providing wrong dmas in the device tree should've made the driver fail 
> to probe.

Agreed,

Thanks,

Jonathan

> 
> Regards,
> Wadim
> 
> >
> > Jonathan
> >
> >  
> >>   		goto err_dma;
> >>   
> >>   	return 0;  
> 
> 

Re: [PATCH] iio: adc: ti_am335x_adc: Make DMAs optional

[Bhavya Kapoor]

On 14/09/23 17:43, Wadim Egorov wrote:
> DMAs are optional. Even if the DMA request is unsuccessfully,
> the ADC can still work properly.
> Make tiadc_request_dma() not fail if we do not provide dmas &
> dma-names properties.
>
> This actually fixes the wrong error handling of the tiadc_request_dma()
> result where the probing only failed if -EPROPE_DEFER was returned.
>
> Fixes: f438b9da75eb ("drivers: iio: ti_am335x_adc: add dma support")
>
> Signed-off-by: Wadim Egorov <w.egorov@phytec.de>
> ---
>  drivers/iio/adc/ti_am335x_adc.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index 8db7a01cb5fb..e14aa9254ab1 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -543,8 +543,11 @@ static int tiadc_request_dma(struct platform_device *pdev,
>  	if (IS_ERR(dma->chan)) {
>  		int ret = PTR_ERR(dma->chan);
>
> +		if (ret != -ENODEV)

Hi Wadim, Thanks for adding this fix but now there is no deferred probe

mechanism available here for the driver. We need to include that as well.

Regards

~B-Kapoor

> +			return dev_err_probe(&pdev->dev, ret,
> +					     "RX DMA channel request failed\n");
>  		dma->chan = NULL;
> -		return ret;
> +		return 0;
>  	}
>
>  	/* RX buffer */
> @@ -670,7 +673,7 @@ static int tiadc_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, indio_dev);
>
>  	err = tiadc_request_dma(pdev, adc_dev);
> -	if (err && err == -EPROBE_DEFER)
> +	if (err)
>  		goto err_dma;
>
>  	return 0;
> --
> 2.25.1
>
>
>