| Message ID | 20230914121300.845493-1-w.egorov@phytec.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ti_am335x_adc: Make DMAs optional | expand |
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;
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 > > >
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;
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; > >
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;
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(-)