LinuxLists.cc - [PATCH v2] mfd: stm32-timers: Use dma_request_chan() instead dma_request_slave

2020-01-07 11:02:18

Subject: [PATCH v2] mfd: stm32-timers: Use dma_request_chan() instead dma_request_slave_channel()

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 <[email protected]>
---
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(-)

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);

--
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

2020-01-07 17:06:59

by Fabrice Gasnier

[permalink] [raw]

Subject: Re: [PATCH v2] mfd: stm32-timers: Use dma_request_chan() instead dma_request_slave_channel()

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 <[email protected]>
> ---
> 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 <[email protected]>

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);
>
>

2020-04-16 08:51:33

by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH v2] mfd: stm32-timers: Use dma_request_chan() instead dma_request_slave_channel()

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 <[email protected]>
> ---
> 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;
> }

--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

2020-05-12 14:09:55

by Peter Ujfalusi

[permalink] [raw]

Subject: Re: [PATCH v2] mfd: stm32-timers: Use dma_request_chan() instead dma_request_slave_channel()

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 <[email protected]>
>> ---
>> 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

Attachments:

pEpkey.asc (1.74 kB)

2020-05-20 08:52:08

by Lee Jones

[permalink] [raw]