| Message ID | 20200107105959.18920-1-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 8d7de077834f978ea0adc9727c43a69129b6f107 |
| Headers | show |
| Series | [v2] mfd: stm32-timers: Use dma_request_chan() instead dma_request_slave_channel() | expand |
On 1/7/20 11:59 AM, Peter Ujfalusi 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> > --- > Hi, > > Changes since v1: > - Fall back to PIO mode only in case of ENODEV and report all other errors > > Regards, > Peter Hi Peter, Thanks for the patch, Acked-by: Fabrice Gasnier <fabrice.gasnier@st.com> Best Regards, Fabrice > > drivers/mfd/stm32-timers.c | 32 +++++++++++++++++++++++--------- > 1 file changed, 23 insertions(+), 9 deletions(-) > > diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c > index efcd4b980c94..add603359124 100644 > --- a/drivers/mfd/stm32-timers.c > +++ b/drivers/mfd/stm32-timers.c > @@ -167,10 +167,11 @@ static void stm32_timers_get_arr_size(struct stm32_timers *ddata) > regmap_write(ddata->regmap, TIM_ARR, 0x0); > } > > -static void stm32_timers_dma_probe(struct device *dev, > +static int stm32_timers_dma_probe(struct device *dev, > struct stm32_timers *ddata) > { > int i; > + int ret = 0; > char name[4]; > > init_completion(&ddata->dma.completion); > @@ -179,14 +180,23 @@ static void stm32_timers_dma_probe(struct device *dev, > /* Optional DMA support: get valid DMA channel(s) or NULL */ > for (i = STM32_TIMERS_DMA_CH1; i <= STM32_TIMERS_DMA_CH4; i++) { > snprintf(name, ARRAY_SIZE(name), "ch%1d", i + 1); > - ddata->dma.chans[i] = dma_request_slave_channel(dev, name); > + ddata->dma.chans[i] = dma_request_chan(dev, name); > } > - ddata->dma.chans[STM32_TIMERS_DMA_UP] = > - dma_request_slave_channel(dev, "up"); > - ddata->dma.chans[STM32_TIMERS_DMA_TRIG] = > - dma_request_slave_channel(dev, "trig"); > - ddata->dma.chans[STM32_TIMERS_DMA_COM] = > - dma_request_slave_channel(dev, "com"); > + ddata->dma.chans[STM32_TIMERS_DMA_UP] = dma_request_chan(dev, "up"); > + ddata->dma.chans[STM32_TIMERS_DMA_TRIG] = dma_request_chan(dev, "trig"); > + ddata->dma.chans[STM32_TIMERS_DMA_COM] = dma_request_chan(dev, "com"); > + > + for (i = STM32_TIMERS_DMA_CH1; i < STM32_TIMERS_MAX_DMAS; i++) { > + if (IS_ERR(ddata->dma.chans[i])) { > + /* Save the first error code to return */ > + if (PTR_ERR(ddata->dma.chans[i]) != -ENODEV && !ret) > + ret = PTR_ERR(ddata->dma.chans[i]); > + > + ddata->dma.chans[i] = NULL; > + } > + } > + > + return ret; > } > > static void stm32_timers_dma_remove(struct device *dev, > @@ -230,7 +240,11 @@ static int stm32_timers_probe(struct platform_device *pdev) > > stm32_timers_get_arr_size(ddata); > > - stm32_timers_dma_probe(dev, ddata); > + ret = stm32_timers_dma_probe(dev, ddata); > + if (ret) { > + stm32_timers_dma_remove(dev, ddata); > + return ret; > + } > > platform_set_drvdata(pdev, ddata); > >
On Tue, 07 Jan 2020, Peter Ujfalusi 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> > --- > Hi, > > Changes since v1: > - Fall back to PIO mode only in case of ENODEV and report all other errors > > Regards, > Peter > > drivers/mfd/stm32-timers.c | 32 +++++++++++++++++++++++--------- > 1 file changed, 23 insertions(+), 9 deletions(-) Apologies for not spotting this patch sooner, it had slipped through the net. If this happens again, please just submit a [RESEND]. > diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c > index efcd4b980c94..add603359124 100644 > --- a/drivers/mfd/stm32-timers.c > +++ b/drivers/mfd/stm32-timers.c > @@ -167,10 +167,11 @@ static void stm32_timers_get_arr_size(struct stm32_timers *ddata) > regmap_write(ddata->regmap, TIM_ARR, 0x0); > } > > -static void stm32_timers_dma_probe(struct device *dev, > +static int stm32_timers_dma_probe(struct device *dev, > struct stm32_timers *ddata) > { > int i; > + int ret = 0; > char name[4]; > > init_completion(&ddata->dma.completion); > @@ -179,14 +180,23 @@ static void stm32_timers_dma_probe(struct device *dev, > /* Optional DMA support: get valid DMA channel(s) or NULL */ > for (i = STM32_TIMERS_DMA_CH1; i <= STM32_TIMERS_DMA_CH4; i++) { > snprintf(name, ARRAY_SIZE(name), "ch%1d", i + 1); > - ddata->dma.chans[i] = dma_request_slave_channel(dev, name); > + ddata->dma.chans[i] = dma_request_chan(dev, name); > } > - ddata->dma.chans[STM32_TIMERS_DMA_UP] = > - dma_request_slave_channel(dev, "up"); > - ddata->dma.chans[STM32_TIMERS_DMA_TRIG] = > - dma_request_slave_channel(dev, "trig"); > - ddata->dma.chans[STM32_TIMERS_DMA_COM] = > - dma_request_slave_channel(dev, "com"); > + ddata->dma.chans[STM32_TIMERS_DMA_UP] = dma_request_chan(dev, "up"); > + ddata->dma.chans[STM32_TIMERS_DMA_TRIG] = dma_request_chan(dev, "trig"); > + ddata->dma.chans[STM32_TIMERS_DMA_COM] = dma_request_chan(dev, "com"); > + > + for (i = STM32_TIMERS_DMA_CH1; i < STM32_TIMERS_MAX_DMAS; i++) { > + if (IS_ERR(ddata->dma.chans[i])) { > + /* Save the first error code to return */ > + if (PTR_ERR(ddata->dma.chans[i]) != -ENODEV && !ret) > + ret = PTR_ERR(ddata->dma.chans[i]); > + > + ddata->dma.chans[i] = NULL; > + } > + } In my mind, it doesn't make sense to keep requesting channels if an error has occurred. Please remove all of the added complexity caused by the for() loop and simply check the return value after each call to dma_request_chan(), returning immediately on error. > + return ret; > }
On 16/04/2020 11.46, Lee Jones wrote: > On Tue, 07 Jan 2020, Peter Ujfalusi 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> >> --- >> Hi, >> >> Changes since v1: >> - Fall back to PIO mode only in case of ENODEV and report all other errors >> >> Regards, >> Peter >> >> drivers/mfd/stm32-timers.c | 32 +++++++++++++++++++++++--------- >> 1 file changed, 23 insertions(+), 9 deletions(-) > > Apologies for not spotting this patch sooner, it had slipped through > the net. If this happens again, please just submit a [RESEND]. No issues, I have also forgot about it ;) >> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c >> index efcd4b980c94..add603359124 100644 >> --- a/drivers/mfd/stm32-timers.c >> +++ b/drivers/mfd/stm32-timers.c >> @@ -167,10 +167,11 @@ static void stm32_timers_get_arr_size(struct stm32_timers *ddata) >> regmap_write(ddata->regmap, TIM_ARR, 0x0); >> } >> >> -static void stm32_timers_dma_probe(struct device *dev, >> +static int stm32_timers_dma_probe(struct device *dev, >> struct stm32_timers *ddata) >> { >> int i; >> + int ret = 0; >> char name[4]; >> >> init_completion(&ddata->dma.completion); >> @@ -179,14 +180,23 @@ static void stm32_timers_dma_probe(struct device *dev, >> /* Optional DMA support: get valid DMA channel(s) or NULL */ >> for (i = STM32_TIMERS_DMA_CH1; i <= STM32_TIMERS_DMA_CH4; i++) { >> snprintf(name, ARRAY_SIZE(name), "ch%1d", i + 1); >> - ddata->dma.chans[i] = dma_request_slave_channel(dev, name); >> + ddata->dma.chans[i] = dma_request_chan(dev, name); >> } >> - ddata->dma.chans[STM32_TIMERS_DMA_UP] = >> - dma_request_slave_channel(dev, "up"); >> - ddata->dma.chans[STM32_TIMERS_DMA_TRIG] = >> - dma_request_slave_channel(dev, "trig"); >> - ddata->dma.chans[STM32_TIMERS_DMA_COM] = >> - dma_request_slave_channel(dev, "com"); >> + ddata->dma.chans[STM32_TIMERS_DMA_UP] = dma_request_chan(dev, "up"); >> + ddata->dma.chans[STM32_TIMERS_DMA_TRIG] = dma_request_chan(dev, "trig"); >> + ddata->dma.chans[STM32_TIMERS_DMA_COM] = dma_request_chan(dev, "com"); >> + >> + for (i = STM32_TIMERS_DMA_CH1; i < STM32_TIMERS_MAX_DMAS; i++) { >> + if (IS_ERR(ddata->dma.chans[i])) { >> + /* Save the first error code to return */ >> + if (PTR_ERR(ddata->dma.chans[i]) != -ENODEV && !ret) >> + ret = PTR_ERR(ddata->dma.chans[i]); >> + >> + ddata->dma.chans[i] = NULL; >> + } >> + } > > In my mind, it doesn't make sense to keep requesting channels if an > error has occurred. Please remove all of the added complexity caused > by the for() loop and simply check the return value after each call to > dma_request_chan(), returning immediately on error. Yes. I have intentionally done the patch this way to _not_ change the behavior in any ways as it is not up to me to decide what is good or bad for a driver I can not test. > >> + return ret; >> } > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Tue, 07 Jan 2020, Peter Ujfalusi 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> > --- > Hi, > > Changes since v1: > - Fall back to PIO mode only in case of ENODEV and report all other errors > > Regards, > Peter > > drivers/mfd/stm32-timers.c | 32 +++++++++++++++++++++++--------- > 1 file changed, 23 insertions(+), 9 deletions(-) Applied, thanks.
diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c index efcd4b980c94..add603359124 100644 --- a/drivers/mfd/stm32-timers.c +++ b/drivers/mfd/stm32-timers.c @@ -167,10 +167,11 @@ static void stm32_timers_get_arr_size(struct stm32_timers *ddata) regmap_write(ddata->regmap, TIM_ARR, 0x0); } -static void stm32_timers_dma_probe(struct device *dev, +static int stm32_timers_dma_probe(struct device *dev, struct stm32_timers *ddata) { int i; + int ret = 0; char name[4]; init_completion(&ddata->dma.completion); @@ -179,14 +180,23 @@ static void stm32_timers_dma_probe(struct device *dev, /* Optional DMA support: get valid DMA channel(s) or NULL */ for (i = STM32_TIMERS_DMA_CH1; i <= STM32_TIMERS_DMA_CH4; i++) { snprintf(name, ARRAY_SIZE(name), "ch%1d", i + 1); - ddata->dma.chans[i] = dma_request_slave_channel(dev, name); + ddata->dma.chans[i] = dma_request_chan(dev, name); } - ddata->dma.chans[STM32_TIMERS_DMA_UP] = - dma_request_slave_channel(dev, "up"); - ddata->dma.chans[STM32_TIMERS_DMA_TRIG] = - dma_request_slave_channel(dev, "trig"); - ddata->dma.chans[STM32_TIMERS_DMA_COM] = - dma_request_slave_channel(dev, "com"); + ddata->dma.chans[STM32_TIMERS_DMA_UP] = dma_request_chan(dev, "up"); + ddata->dma.chans[STM32_TIMERS_DMA_TRIG] = dma_request_chan(dev, "trig"); + ddata->dma.chans[STM32_TIMERS_DMA_COM] = dma_request_chan(dev, "com"); + + for (i = STM32_TIMERS_DMA_CH1; i < STM32_TIMERS_MAX_DMAS; i++) { + if (IS_ERR(ddata->dma.chans[i])) { + /* Save the first error code to return */ + if (PTR_ERR(ddata->dma.chans[i]) != -ENODEV && !ret) + ret = PTR_ERR(ddata->dma.chans[i]); + + ddata->dma.chans[i] = NULL; + } + } + + return ret; } static void stm32_timers_dma_remove(struct device *dev, @@ -230,7 +240,11 @@ static int stm32_timers_probe(struct platform_device *pdev) stm32_timers_get_arr_size(ddata); - stm32_timers_dma_probe(dev, ddata); + ret = stm32_timers_dma_probe(dev, ddata); + if (ret) { + stm32_timers_dma_remove(dev, ddata); + return ret; + } platform_set_drvdata(pdev, ddata);
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> --- Hi, Changes since v1: - Fall back to PIO mode only in case of ENODEV and report all other errors Regards, Peter drivers/mfd/stm32-timers.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)