linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] mfd: stm32-timers: Use dma_request_chan() instead dma_request_slave_channel()
@ 2020-01-07 10:59 Peter Ujfalusi
  2020-01-07 17:04 ` Fabrice Gasnier
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Peter Ujfalusi @ 2020-01-07 10:59 UTC (permalink / raw)
  To: fabrice.gasnier, lee.jones, mcoquelin.stm32, alexandre.torgue
  Cc: vkoul, linux-stm32, linux-arm-kernel, linux-kernel

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

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


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] mfd: stm32-timers: Use dma_request_chan() instead dma_request_slave_channel()
  2020-01-07 10:59 [PATCH v2] mfd: stm32-timers: Use dma_request_chan() instead dma_request_slave_channel() Peter Ujfalusi
@ 2020-01-07 17:04 ` Fabrice Gasnier
  2020-04-16  8:46 ` Lee Jones
  2020-05-20  8:49 ` Lee Jones
  2 siblings, 0 replies; 5+ messages in thread
From: Fabrice Gasnier @ 2020-01-07 17:04 UTC (permalink / raw)
  To: Peter Ujfalusi, lee.jones, mcoquelin.stm32, alexandre.torgue
  Cc: vkoul, linux-stm32, linux-arm-kernel, linux-kernel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] mfd: stm32-timers: Use dma_request_chan() instead dma_request_slave_channel()
  2020-01-07 10:59 [PATCH v2] mfd: stm32-timers: Use dma_request_chan() instead dma_request_slave_channel() Peter Ujfalusi
  2020-01-07 17:04 ` Fabrice Gasnier
@ 2020-04-16  8:46 ` Lee Jones
  2020-05-12 14:04   ` Peter Ujfalusi
  2020-05-20  8:49 ` Lee Jones
  2 siblings, 1 reply; 5+ messages in thread
From: Lee Jones @ 2020-04-16  8:46 UTC (permalink / raw)
  To: Peter Ujfalusi
  Cc: fabrice.gasnier, mcoquelin.stm32, alexandre.torgue, vkoul,
	linux-stm32, linux-arm-kernel, linux-kernel

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

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] mfd: stm32-timers: Use dma_request_chan() instead dma_request_slave_channel()
  2020-04-16  8:46 ` Lee Jones
@ 2020-05-12 14:04   ` Peter Ujfalusi
  0 siblings, 0 replies; 5+ messages in thread
From: Peter Ujfalusi @ 2020-05-12 14:04 UTC (permalink / raw)
  To: Lee Jones
  Cc: fabrice.gasnier, mcoquelin.stm32, alexandre.torgue, vkoul,
	linux-stm32, linux-arm-kernel, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 3330 bytes --]



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

[-- Attachment #2: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 1783 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] mfd: stm32-timers: Use dma_request_chan() instead dma_request_slave_channel()
  2020-01-07 10:59 [PATCH v2] mfd: stm32-timers: Use dma_request_chan() instead dma_request_slave_channel() Peter Ujfalusi
  2020-01-07 17:04 ` Fabrice Gasnier
  2020-04-16  8:46 ` Lee Jones
@ 2020-05-20  8:49 ` Lee Jones
  2 siblings, 0 replies; 5+ messages in thread
From: Lee Jones @ 2020-05-20  8:49 UTC (permalink / raw)
  To: Peter Ujfalusi
  Cc: fabrice.gasnier, mcoquelin.stm32, alexandre.torgue, vkoul,
	linux-stm32, linux-arm-kernel, linux-kernel

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.

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-05-20  8:49 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-07 10:59 [PATCH v2] mfd: stm32-timers: Use dma_request_chan() instead dma_request_slave_channel() Peter Ujfalusi
2020-01-07 17:04 ` Fabrice Gasnier
2020-04-16  8:46 ` Lee Jones
2020-05-12 14:04   ` Peter Ujfalusi
2020-05-20  8:49 ` Lee Jones

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).