On Tue, 19 Sep 2023 10:21:28 +0000
Wadim Egorov <W.Egorov@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> >
> >> ---
> >> 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;
>
>
