| Message ID | 20191217104025.23318-1-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: rcar_drif: Use dma_request_chan() instead dma_request_slave_channel() | expand |
On Tue, Dec 17, 2019 at 11:41 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > dma_request_slave_channel() is a wrapper on top of dma_request_chan() > eating up the error code. > > By using dma_request_chan() directly the driver can support deferred > probing against DMA. > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> One comment below. > --- a/drivers/media/platform/rcar_drif.c > +++ b/drivers/media/platform/rcar_drif.c > @@ -275,10 +275,10 @@ static int rcar_drif_alloc_dmachannels(struct rcar_drif_sdr *sdr) > for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) { > struct rcar_drif *ch = sdr->ch[i]; > > - ch->dmach = dma_request_slave_channel(&ch->pdev->dev, "rx"); > - if (!ch->dmach) { > + ch->dmach = dma_request_chan(&ch->pdev->dev, "rx"); > + if (IS_ERR(ch->dmach)) { > rdrif_err(sdr, "ch%u: dma channel req failed\n", i); Now there is an error code, you might (1) want to print it, and (2) only do so when it is not due to probe deferral: if (PTR_ERR(ch->dmach) != -EPROBE_DEFER) rdrif_err(sdr, "ch%u: dma channel req failed %pe\n", i, ch->dmach); > - ret = -ENODEV; > + ret = PTR_ERR(ch->dmach); > goto dmach_error; > } Gr{oetje,eeting}s, Geert
Hi Geert, On 20/01/2020 14.05, Geert Uytterhoeven wrote: > On Tue, Dec 17, 2019 at 11:41 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: >> dma_request_slave_channel() is a wrapper on top of dma_request_chan() >> eating up the error code. >> >> By using dma_request_chan() directly the driver can support deferred >> probing against DMA. >> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > One comment below. > >> --- a/drivers/media/platform/rcar_drif.c >> +++ b/drivers/media/platform/rcar_drif.c >> @@ -275,10 +275,10 @@ static int rcar_drif_alloc_dmachannels(struct rcar_drif_sdr *sdr) >> for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) { >> struct rcar_drif *ch = sdr->ch[i]; >> >> - ch->dmach = dma_request_slave_channel(&ch->pdev->dev, "rx"); >> - if (!ch->dmach) { >> + ch->dmach = dma_request_chan(&ch->pdev->dev, "rx"); >> + if (IS_ERR(ch->dmach)) { >> rdrif_err(sdr, "ch%u: dma channel req failed\n", i); > > Now there is an error code, you might (1) want to print it, and (2) only > do so when it is not due to probe deferral: > > if (PTR_ERR(ch->dmach) != -EPROBE_DEFER) > rdrif_err(sdr, "ch%u: dma channel req failed %pe\n", > i, ch->dmach); Yes, this is true. > >> - ret = -ENODEV; >> + ret = PTR_ERR(ch->dmach); if (ret != -EPROBE_DEFER) rdrif_err(sdr, "ch%u: dma channel req failed (%d)\n", i, ret); might be simpler. >> goto dmach_error; >> } > > Gr{oetje,eeting}s, > > Geert > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Peter, On Mon, Jan 20, 2020 at 1:09 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > On 20/01/2020 14.05, Geert Uytterhoeven wrote: > > On Tue, Dec 17, 2019 at 11:41 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > >> dma_request_slave_channel() is a wrapper on top of dma_request_chan() > >> eating up the error code. > >> > >> By using dma_request_chan() directly the driver can support deferred > >> probing against DMA. > >> > >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > One comment below. > > > >> --- a/drivers/media/platform/rcar_drif.c > >> +++ b/drivers/media/platform/rcar_drif.c > >> @@ -275,10 +275,10 @@ static int rcar_drif_alloc_dmachannels(struct rcar_drif_sdr *sdr) > >> for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) { > >> struct rcar_drif *ch = sdr->ch[i]; > >> > >> - ch->dmach = dma_request_slave_channel(&ch->pdev->dev, "rx"); > >> - if (!ch->dmach) { > >> + ch->dmach = dma_request_chan(&ch->pdev->dev, "rx"); > >> + if (IS_ERR(ch->dmach)) { > >> rdrif_err(sdr, "ch%u: dma channel req failed\n", i); > > > > Now there is an error code, you might (1) want to print it, and (2) only > > do so when it is not due to probe deferral: > > > > if (PTR_ERR(ch->dmach) != -EPROBE_DEFER) > > rdrif_err(sdr, "ch%u: dma channel req failed %pe\n", > > i, ch->dmach); > > Yes, this is true. > > > > >> - ret = -ENODEV; > >> + ret = PTR_ERR(ch->dmach); > > if (ret != -EPROBE_DEFER) > rdrif_err(sdr, "ch%u: dma channel req failed (%d)\n", i, ret); > > might be simpler. Sure, checking ret is simpler. But printing ch->dmach means you can use the new %pe format specifier to pretty-print the error code. Gr{oetje,eeting}s, Geert
On 20/01/2020 14.36, Geert Uytterhoeven wrote: > Hi Peter, > > On Mon, Jan 20, 2020 at 1:09 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: >> On 20/01/2020 14.05, Geert Uytterhoeven wrote: >>> On Tue, Dec 17, 2019 at 11:41 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: >>>> dma_request_slave_channel() is a wrapper on top of dma_request_chan() >>>> eating up the error code. >>>> >>>> By using dma_request_chan() directly the driver can support deferred >>>> probing against DMA. >>>> >>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >>> >>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> >>> >>> One comment below. >>> >>>> --- a/drivers/media/platform/rcar_drif.c >>>> +++ b/drivers/media/platform/rcar_drif.c >>>> @@ -275,10 +275,10 @@ static int rcar_drif_alloc_dmachannels(struct rcar_drif_sdr *sdr) >>>> for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) { >>>> struct rcar_drif *ch = sdr->ch[i]; >>>> >>>> - ch->dmach = dma_request_slave_channel(&ch->pdev->dev, "rx"); >>>> - if (!ch->dmach) { >>>> + ch->dmach = dma_request_chan(&ch->pdev->dev, "rx"); >>>> + if (IS_ERR(ch->dmach)) { >>>> rdrif_err(sdr, "ch%u: dma channel req failed\n", i); >>> >>> Now there is an error code, you might (1) want to print it, and (2) only >>> do so when it is not due to probe deferral: >>> >>> if (PTR_ERR(ch->dmach) != -EPROBE_DEFER) >>> rdrif_err(sdr, "ch%u: dma channel req failed %pe\n", >>> i, ch->dmach); >> >> Yes, this is true. >> >>> >>>> - ret = -ENODEV; >>>> + ret = PTR_ERR(ch->dmach); >> >> if (ret != -EPROBE_DEFER) >> rdrif_err(sdr, "ch%u: dma channel req failed (%d)\n", i, ret); >> >> might be simpler. > > Sure, checking ret is simpler. > But printing ch->dmach means you can use the new %pe format specifier > to pretty-print the error code. Ah, I was not aware of this, but there is another issue with the patch: I need to set the ch->dmach to NULL before jumping to dmach_error.. I'll send v3 in a minute. > > Gr{oetje,eeting}s, > > Geert > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c index 0f267a237b42..02ee0e2a09e9 100644 --- a/drivers/media/platform/rcar_drif.c +++ b/drivers/media/platform/rcar_drif.c @@ -275,10 +275,10 @@ static int rcar_drif_alloc_dmachannels(struct rcar_drif_sdr *sdr) for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) { struct rcar_drif *ch = sdr->ch[i]; - ch->dmach = dma_request_slave_channel(&ch->pdev->dev, "rx"); - if (!ch->dmach) { + ch->dmach = dma_request_chan(&ch->pdev->dev, "rx"); + if (IS_ERR(ch->dmach)) { rdrif_err(sdr, "ch%u: dma channel req failed\n", i); - ret = -ENODEV; + ret = PTR_ERR(ch->dmach); goto dmach_error; }
dma_request_slave_channel() is a wrapper on top of dma_request_chan() eating up the error code. By using dma_request_chan() directly the driver can support deferred probing against DMA. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- drivers/media/platform/rcar_drif.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)