[PATCH 0/5] Improve error handling in Rockchip SPI drivers

[PATCH 0/5] Improve error handling in Rockchip SPI drivers

[Dragan Simic]

This is a small series that improves error handling in the probe path
of the Rockchip SPI drivers, by using dev_err_probe() properly in multiple
places.  It also removes one unnecessary check of a function return value,
and performs a bunch of small, rather trivial code cleanups, to make the
code neater and a bit easier to read.

Dragan Simic (5):
  spi: rockchip: Perform trivial code cleanups
  spi: rockchip-sfc: Perform trivial code cleanups
  spi: rockchip: Don't check for failed get_fifo_len()
  spi: rockchip: Use dev_err_probe() in the probe path
  spi: rockchip-sfc: Use dev_err_probe() in the probe path

 drivers/spi/spi-rockchip-sfc.c | 21 +++++--------
 drivers/spi/spi-rockchip.c     | 57 ++++++++++++++--------------------
 2 files changed, 32 insertions(+), 46 deletions(-)

[PATCH 1/5] spi: rockchip: Perform trivial code cleanups

[Dragan Simic]

Perform a few trivial code cleanups, to obey the reverse Christmas tree rule,
to avoid unnecessary line wrapping by using the 100-column width better, to
actually obey the 100-column width in one case, and to make the way a couple
of wrapped function arguments are indented a bit more readable.

No intended functional changes are introduced by these code cleanups.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 drivers/spi/spi-rockchip.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index e1ecd96c7858..81f2a966c124 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -742,34 +742,32 @@ static int rockchip_spi_setup(struct spi_device *spi)
 
 static int rockchip_spi_probe(struct platform_device *pdev)
 {
-	int ret;
-	struct rockchip_spi *rs;
+	struct device_node *np = pdev->dev.of_node;
 	struct spi_controller *ctlr;
+	struct rockchip_spi *rs;
 	struct resource *mem;
-	struct device_node *np = pdev->dev.of_node;
 	u32 rsd_nsecs, num_cs;
 	bool target_mode;
+	int ret;
 
 	target_mode = of_property_read_bool(np, "spi-slave");
 
 	if (target_mode)
-		ctlr = spi_alloc_target(&pdev->dev,
-				sizeof(struct rockchip_spi));
+		ctlr = spi_alloc_target(&pdev->dev, sizeof(struct rockchip_spi));
 	else
-		ctlr = spi_alloc_host(&pdev->dev,
-				sizeof(struct rockchip_spi));
+		ctlr = spi_alloc_host(&pdev->dev, sizeof(struct rockchip_spi));
 
 	if (!ctlr)
 		return -ENOMEM;
 
 	platform_set_drvdata(pdev, ctlr);
 
 	rs = spi_controller_get_devdata(ctlr);
 
 	/* Get basic io resource and map it */
 	rs->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &mem);
 	if (IS_ERR(rs->regs)) {
-		ret =  PTR_ERR(rs->regs);
+		ret = PTR_ERR(rs->regs);
 		goto err_put_ctlr;
 	}
 
@@ -794,26 +792,25 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 		goto err_put_ctlr;
 
 	ret = devm_request_threaded_irq(&pdev->dev, ret, rockchip_spi_isr, NULL,
-			IRQF_ONESHOT, dev_name(&pdev->dev), ctlr);
+					IRQF_ONESHOT, dev_name(&pdev->dev), ctlr);
 	if (ret)
 		goto err_put_ctlr;
 
 	rs->dev = &pdev->dev;
 	rs->freq = clk_get_rate(rs->spiclk);
 
 	if (!of_property_read_u32(pdev->dev.of_node, "rx-sample-delay-ns",
 				  &rsd_nsecs)) {
 		/* rx sample delay is expressed in parent clock cycles (max 3) */
-		u32 rsd = DIV_ROUND_CLOSEST(rsd_nsecs * (rs->freq >> 8),
-				1000000000 >> 8);
+		u32 rsd = DIV_ROUND_CLOSEST(rsd_nsecs * (rs->freq >> 8), 1000000000 >> 8);
 		if (!rsd) {
 			dev_warn(rs->dev, "%u Hz are too slow to express %u ns delay\n",
-					rs->freq, rsd_nsecs);
+				 rs->freq, rsd_nsecs);
 		} else if (rsd > CR0_RSD_MAX) {
 			rsd = CR0_RSD_MAX;
-			dev_warn(rs->dev, "%u Hz are too fast to express %u ns delay, clamping at %u ns\n",
-					rs->freq, rsd_nsecs,
-					CR0_RSD_MAX * 1000000000U / rs->freq);
+			dev_warn(rs->dev,
+				 "%u Hz are too fast to express %u ns delay, clamping at %u ns\n",
+				 rs->freq, rsd_nsecs, CR0_RSD_MAX * 1000000000U / rs->freq);
 		}
 		rs->rsd = rsd;
 	}

[PATCH 2/5] spi: rockchip-sfc: Perform trivial code cleanups

[Dragan Simic]

Perform a couple of trivial code cleanups, to avoid unnecessary line wrapping
by using the 100-column width a bit better, and to drop a stray empty line.

No intended functional changes are introduced by these code cleanups.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 drivers/spi/spi-rockchip-sfc.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-rockchip-sfc.c b/drivers/spi/spi-rockchip-sfc.c
index 0d7fadcd4ed3..505d5089bf03 100644
--- a/drivers/spi/spi-rockchip-sfc.c
+++ b/drivers/spi/spi-rockchip-sfc.c
@@ -591,19 +591,17 @@ static int rockchip_sfc_probe(struct platform_device *pdev)
 		return PTR_ERR(sfc->hclk);
 	}
 
-	sfc->use_dma = !of_property_read_bool(sfc->dev->of_node,
-					      "rockchip,sfc-no-dma");
+	sfc->use_dma = !of_property_read_bool(sfc->dev->of_node, "rockchip,sfc-no-dma");
 
 	if (sfc->use_dma) {
 		ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
 		if (ret) {
 			dev_warn(dev, "Unable to set dma mask\n");
 			return ret;
 		}
 
 		sfc->buffer = dmam_alloc_coherent(dev, SFC_MAX_IOSIZE_VER3,
-						  &sfc->dma_buffer,
-						  GFP_KERNEL);
+						  &sfc->dma_buffer, GFP_KERNEL);
 		if (!sfc->buffer)
 			return -ENOMEM;
 	}
@@ -629,7 +627,6 @@ static int rockchip_sfc_probe(struct platform_device *pdev)
 			       0, pdev->name, sfc);
 	if (ret) {
 		dev_err(dev, "Failed to request irq\n");
-
 		goto err_irq;
 	}
 

[PATCH 3/5] spi: rockchip: Don't check for failed get_fifo_len()

[Dragan Simic]

Since commit 13a96935e6f6 ("spi: rockchip: Support 64-location deep FIFOs"),
function get_fifo_len() can no longer return zero, so delete the redundant
check for zero in function rockchip_spi_probe().

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 drivers/spi/spi-rockchip.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 81f2a966c124..28879fed03f8 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -816,11 +816,6 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 	}
 
 	rs->fifo_len = get_fifo_len(rs);
-	if (!rs->fifo_len) {
-		dev_err(&pdev->dev, "Failed to get fifo length\n");
-		ret = -EINVAL;
-		goto err_put_ctlr;
-	}
 
 	pm_runtime_set_autosuspend_delay(&pdev->dev, ROCKCHIP_AUTOSUSPEND_TIMEOUT);
 	pm_runtime_use_autosuspend(&pdev->dev);

[PATCH 4/5] spi: rockchip: Use dev_err_probe() in the probe path

[Dragan Simic]

Use function dev_err_probe() in the probe path instead of dev_err() where
appropriate, to make the code a bit more uniform and compact, and to improve
error handling for the TX and RX DMA channel requests.

Previously, deferred requests for the TX and RX DMA channels produced no
debug messages, and the final error messages didn't include the error codes,
which are all highly useful when debugging permanently failed DMA channel
requests, such as when the required drivers aren't enabled.

Suggested-by: Hélene Vulquin <oss@helene.moe>
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 drivers/spi/spi-rockchip.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 28879fed03f8..6b5c67a357bb 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -773,15 +773,15 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 
 	rs->apb_pclk = devm_clk_get_enabled(&pdev->dev, "apb_pclk");
 	if (IS_ERR(rs->apb_pclk)) {
-		dev_err(&pdev->dev, "Failed to get apb_pclk\n");
-		ret = PTR_ERR(rs->apb_pclk);
+		ret = dev_err_probe(&pdev->dev, PTR_ERR(rs->apb_pclk),
+				    "Failed to get apb_pclk\n");
 		goto err_put_ctlr;
 	}
 
 	rs->spiclk = devm_clk_get_enabled(&pdev->dev, "spiclk");
 	if (IS_ERR(rs->spiclk)) {
-		dev_err(&pdev->dev, "Failed to get spi_pclk\n");
-		ret = PTR_ERR(rs->spiclk);
+		ret = dev_err_probe(&pdev->dev, PTR_ERR(rs->spiclk),
+				    "Failed to get spi_pclk\n");
 		goto err_put_ctlr;
 	}
 
@@ -853,22 +853,21 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 
 	ctlr->dma_tx = dma_request_chan(rs->dev, "tx");
 	if (IS_ERR(ctlr->dma_tx)) {
-		/* Check tx to see if we need defer probing driver */
-		if (PTR_ERR(ctlr->dma_tx) == -EPROBE_DEFER) {
-			ret = -EPROBE_DEFER;
+		/* Check tx to see if we need to defer driver probing */
+		ret = dev_err_probe(rs->dev, PTR_ERR(ctlr->dma_tx),
+				    "Failed to request TX DMA channel\n");
+		if (ret == -EPROBE_DEFER)
 			goto err_disable_pm_runtime;
-		}
-		dev_warn(rs->dev, "Failed to request TX DMA channel\n");
 		ctlr->dma_tx = NULL;
 	}
 
 	ctlr->dma_rx = dma_request_chan(rs->dev, "rx");
 	if (IS_ERR(ctlr->dma_rx)) {
-		if (PTR_ERR(ctlr->dma_rx) == -EPROBE_DEFER) {
-			ret = -EPROBE_DEFER;
+		/* Check rx to see if we need to defer driver probing */
+		ret = dev_err_probe(rs->dev, PTR_ERR(ctlr->dma_rx),
+				    "Failed to request RX DMA channel\n");
+		if (ret == -EPROBE_DEFER)
 			goto err_free_dma_tx;
-		}
-		dev_warn(rs->dev, "Failed to request RX DMA channel\n");
 		ctlr->dma_rx = NULL;
 	}
 

[PATCH 5/5] spi: rockchip-sfc: Use dev_err_probe() in the probe path

[Dragan Simic]

Use function dev_err_probe() in the probe path instead of dev_err() where
appropriate, to make the code a bit more uniform and compact.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 drivers/spi/spi-rockchip-sfc.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-rockchip-sfc.c b/drivers/spi/spi-rockchip-sfc.c
index 505d5089bf03..7e0fb4944a34 100644
--- a/drivers/spi/spi-rockchip-sfc.c
+++ b/drivers/spi/spi-rockchip-sfc.c
@@ -580,16 +580,14 @@ static int rockchip_sfc_probe(struct platform_device *pdev)
 		return PTR_ERR(sfc->regbase);
 
 	sfc->clk = devm_clk_get(&pdev->dev, "clk_sfc");
-	if (IS_ERR(sfc->clk)) {
-		dev_err(&pdev->dev, "Failed to get sfc interface clk\n");
-		return PTR_ERR(sfc->clk);
-	}
+	if (IS_ERR(sfc->clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(sfc->clk),
+				     "Failed to get sfc interface clk\n");
 
 	sfc->hclk = devm_clk_get(&pdev->dev, "hclk_sfc");
-	if (IS_ERR(sfc->hclk)) {
-		dev_err(&pdev->dev, "Failed to get sfc ahb clk\n");
-		return PTR_ERR(sfc->hclk);
-	}
+	if (IS_ERR(sfc->hclk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(sfc->hclk),
+				     "Failed to get sfc ahb clk\n");
 
 	sfc->use_dma = !of_property_read_bool(sfc->dev->of_node, "rockchip,sfc-no-dma");
 

Re: [PATCH 1/5] spi: rockchip: Perform trivial code cleanups

[Heiko Stuebner]

Am Donnerstag, 26. September 2024, 10:38:12 CEST schrieb Dragan Simic:
> Perform a few trivial code cleanups, to obey the reverse Christmas tree rule,
> to avoid unnecessary line wrapping by using the 100-column width better, to
> actually obey the 100-column width in one case, and to make the way a couple
> of wrapped function arguments are indented a bit more readable.
> 
> No intended functional changes are introduced by these code cleanups.
> 
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

Re: [PATCH 2/5] spi: rockchip-sfc: Perform trivial code cleanups

[Heiko Stuebner]

Am Donnerstag, 26. September 2024, 10:38:13 CEST schrieb Dragan Simic:
> Perform a couple of trivial code cleanups, to avoid unnecessary line wrapping
> by using the 100-column width a bit better, and to drop a stray empty line.
> 
> No intended functional changes are introduced by these code cleanups.
> 
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

Re: [PATCH 3/5] spi: rockchip: Don't check for failed get_fifo_len()

[Heiko Stuebner]

Am Donnerstag, 26. September 2024, 10:38:14 CEST schrieb Dragan Simic:
> Since commit 13a96935e6f6 ("spi: rockchip: Support 64-location deep FIFOs"),
> function get_fifo_len() can no longer return zero, so delete the redundant
> check for zero in function rockchip_spi_probe().
> 
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>

Didn't this topic come up in another recent patch too?

Anyway, having looked up the what the current get_fifo_len does,
the 0 case should never happen, as you describe, so

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

Re: [PATCH 4/5] spi: rockchip: Use dev_err_probe() in the probe path

[Heiko Stuebner]

Am Donnerstag, 26. September 2024, 10:38:15 CEST schrieb Dragan Simic:
> Use function dev_err_probe() in the probe path instead of dev_err() where
> appropriate, to make the code a bit more uniform and compact, and to improve
> error handling for the TX and RX DMA channel requests.
> 
> Previously, deferred requests for the TX and RX DMA channels produced no
> debug messages, and the final error messages didn't include the error codes,
> which are all highly useful when debugging permanently failed DMA channel
> requests, such as when the required drivers aren't enabled.
> 
> Suggested-by: Hélene Vulquin <oss@helene.moe>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
>  drivers/spi/spi-rockchip.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> index 28879fed03f8..6b5c67a357bb 100644
> --- a/drivers/spi/spi-rockchip.c
> +++ b/drivers/spi/spi-rockchip.c
> @@ -853,22 +853,21 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>  
>  	ctlr->dma_tx = dma_request_chan(rs->dev, "tx");
>  	if (IS_ERR(ctlr->dma_tx)) {
> -		/* Check tx to see if we need defer probing driver */
> -		if (PTR_ERR(ctlr->dma_tx) == -EPROBE_DEFER) {
> -			ret = -EPROBE_DEFER;
> +		/* Check tx to see if we need to defer driver probing */
> +		ret = dev_err_probe(rs->dev, PTR_ERR(ctlr->dma_tx),
> +				    "Failed to request TX DMA channel\n");

you're upgrading here from a warning to an error log level.
As it seems the controller may actually provide some level of functionality
even without dma, is this approriate?

Same for rx below.

Heiko

> +		if (ret == -EPROBE_DEFER)
>  			goto err_disable_pm_runtime;
> -		}
> -		dev_warn(rs->dev, "Failed to request TX DMA channel\n");
>  		ctlr->dma_tx = NULL;
>  	}
>  
>  	ctlr->dma_rx = dma_request_chan(rs->dev, "rx");
>  	if (IS_ERR(ctlr->dma_rx)) {
> -		if (PTR_ERR(ctlr->dma_rx) == -EPROBE_DEFER) {
> -			ret = -EPROBE_DEFER;
> +		/* Check rx to see if we need to defer driver probing */
> +		ret = dev_err_probe(rs->dev, PTR_ERR(ctlr->dma_rx),
> +				    "Failed to request RX DMA channel\n");
> +		if (ret == -EPROBE_DEFER)
>  			goto err_free_dma_tx;
> -		}
> -		dev_warn(rs->dev, "Failed to request RX DMA channel\n");
>  		ctlr->dma_rx = NULL;
>  	}
>  
> 

Re: [PATCH 5/5] spi: rockchip-sfc: Use dev_err_probe() in the probe path

[Heiko Stuebner]

Am Donnerstag, 26. September 2024, 10:38:16 CEST schrieb Dragan Simic:
> Use function dev_err_probe() in the probe path instead of dev_err() where
> appropriate, to make the code a bit more uniform and compact.
> 
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

Re: [PATCH 3/5] spi: rockchip: Don't check for failed get_fifo_len()

[Dragan Simic]

Hello Heiko,

On 2024-09-26 10:55, Heiko Stuebner wrote:
> Am Donnerstag, 26. September 2024, 10:38:14 CEST schrieb Dragan Simic:
>> Since commit 13a96935e6f6 ("spi: rockchip: Support 64-location deep 
>> FIFOs"),
>> function get_fifo_len() can no longer return zero, so delete the 
>> redundant
>> check for zero in function rockchip_spi_probe().
>> 
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> 
> Didn't this topic come up in another recent patch too?

Hmm, perhaps, but I'm unaware of that patch?  Maybe I missed it.

> Anyway, having looked up the what the current get_fifo_len does,
> the 0 case should never happen, as you describe, so
> 
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>

Thanks.

Re: [PATCH 3/5] spi: rockchip: Don't check for failed get_fifo_len()

[Mark Brown]

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

On Thu, Sep 26, 2024 at 10:55:01AM +0200, Heiko Stuebner wrote:
> Am Donnerstag, 26. September 2024, 10:38:14 CEST schrieb Dragan Simic:
> > Since commit 13a96935e6f6 ("spi: rockchip: Support 64-location deep FIFOs"),
> > function get_fifo_len() can no longer return zero, so delete the redundant
> > check for zero in function rockchip_spi_probe().

> Didn't this topic come up in another recent patch too?

> Anyway, having looked up the what the current get_fifo_len does,
> the 0 case should never happen, as you describe, so

One of the people doing random cleanups posted the same patch which I
pushed back on since probe() isn't a hot path and it means if
get_fifo_len() changes again it could silently break things.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/5] spi: rockchip: Use dev_err_probe() in the probe path

[Dragan Simic]

Hello Heiko,

On 2024-09-26 11:00, Heiko Stuebner wrote:
> Am Donnerstag, 26. September 2024, 10:38:15 CEST schrieb Dragan Simic:
>> Use function dev_err_probe() in the probe path instead of dev_err() 
>> where
>> appropriate, to make the code a bit more uniform and compact, and to 
>> improve
>> error handling for the TX and RX DMA channel requests.
>> 
>> Previously, deferred requests for the TX and RX DMA channels produced 
>> no
>> debug messages, and the final error messages didn't include the error 
>> codes,
>> which are all highly useful when debugging permanently failed DMA 
>> channel
>> requests, such as when the required drivers aren't enabled.
>> 
>> Suggested-by: Hélene Vulquin <oss@helene.moe>
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>>  drivers/spi/spi-rockchip.c | 25 ++++++++++++-------------
>>  1 file changed, 12 insertions(+), 13 deletions(-)
>> 
>> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
>> index 28879fed03f8..6b5c67a357bb 100644
>> --- a/drivers/spi/spi-rockchip.c
>> +++ b/drivers/spi/spi-rockchip.c
>> @@ -853,22 +853,21 @@ static int rockchip_spi_probe(struct 
>> platform_device *pdev)
>> 
>>  	ctlr->dma_tx = dma_request_chan(rs->dev, "tx");
>>  	if (IS_ERR(ctlr->dma_tx)) {
>> -		/* Check tx to see if we need defer probing driver */
>> -		if (PTR_ERR(ctlr->dma_tx) == -EPROBE_DEFER) {
>> -			ret = -EPROBE_DEFER;
>> +		/* Check tx to see if we need to defer driver probing */
>> +		ret = dev_err_probe(rs->dev, PTR_ERR(ctlr->dma_tx),
>> +				    "Failed to request TX DMA channel\n");
> 
> you're upgrading here from a warning to an error log level.
> As it seems the controller may actually provide some level of 
> functionality
> even without dma, is this approriate?
> 
> Same for rx below.

Thanks for your quick responses.

You're right about the driver still working without the DMA channels,
so emitting warnings would be much more appropriate.

We'd basically need dev_warn_probe() as a new function to cover these
two cases, but I'm not really sure how to proceed?  I could go ahead
and implement dev_warn_probe() in a good way, but I wonder what would
be the chances to have that accepted upstream?  Perhaps there would
be other users for dev_warn_probe().

>> +		if (ret == -EPROBE_DEFER)
>>  			goto err_disable_pm_runtime;
>> -		}
>> -		dev_warn(rs->dev, "Failed to request TX DMA channel\n");
>>  		ctlr->dma_tx = NULL;
>>  	}
>> 
>>  	ctlr->dma_rx = dma_request_chan(rs->dev, "rx");
>>  	if (IS_ERR(ctlr->dma_rx)) {
>> -		if (PTR_ERR(ctlr->dma_rx) == -EPROBE_DEFER) {
>> -			ret = -EPROBE_DEFER;
>> +		/* Check rx to see if we need to defer driver probing */
>> +		ret = dev_err_probe(rs->dev, PTR_ERR(ctlr->dma_rx),
>> +				    "Failed to request RX DMA channel\n");
>> +		if (ret == -EPROBE_DEFER)
>>  			goto err_free_dma_tx;
>> -		}
>> -		dev_warn(rs->dev, "Failed to request RX DMA channel\n");
>>  		ctlr->dma_rx = NULL;
>>  	}

Re: [PATCH 3/5] spi: rockchip: Don't check for failed get_fifo_len()

[Dragan Simic]

Hello Mark,

On 2024-09-26 11:17, Mark Brown wrote:
> On Thu, Sep 26, 2024 at 10:55:01AM +0200, Heiko Stuebner wrote:
>> Am Donnerstag, 26. September 2024, 10:38:14 CEST schrieb Dragan Simic:
>> > Since commit 13a96935e6f6 ("spi: rockchip: Support 64-location deep FIFOs"),
>> > function get_fifo_len() can no longer return zero, so delete the redundant
>> > check for zero in function rockchip_spi_probe().
> 
>> Didn't this topic come up in another recent patch too?
> 
>> Anyway, having looked up the what the current get_fifo_len does,
>> the 0 case should never happen, as you describe, so
> 
> One of the people doing random cleanups posted the same patch which I
> pushed back on since probe() isn't a hot path and it means if
> get_fifo_len() changes again it could silently break things.

Thanks for the clarification, it makes sense to keep the check for
future proofing.  I'll drop this patch in the v2 of this series.

Re: (subset) [PATCH 0/5] Improve error handling in Rockchip SPI drivers

[Mark Brown]

On Thu, 26 Sep 2024 10:38:11 +0200, Dragan Simic wrote:
> This is a small series that improves error handling in the probe path
> of the Rockchip SPI drivers, by using dev_err_probe() properly in multiple
> places.  It also removes one unnecessary check of a function return value,
> and performs a bunch of small, rather trivial code cleanups, to make the
> code neater and a bit easier to read.
> 
> Dragan Simic (5):
>   spi: rockchip: Perform trivial code cleanups
>   spi: rockchip-sfc: Perform trivial code cleanups
>   spi: rockchip: Don't check for failed get_fifo_len()
>   spi: rockchip: Use dev_err_probe() in the probe path
>   spi: rockchip-sfc: Use dev_err_probe() in the probe path
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/5] spi: rockchip: Perform trivial code cleanups
      commit: d87ec94e48dd2da27fbe948f2dc6c8fedc98fff4
[2/5] spi: rockchip-sfc: Perform trivial code cleanups
      commit: 6c510eac1528d8939bad8b6df72c7b23ffec8c25
[5/5] spi: rockchip-sfc: Use dev_err_probe() in the probe path
      commit: 1482c40b440fa58f956bc3e1ef3426e0cdbc09e0

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: (subset) [PATCH 0/5] Improve error handling in Rockchip SPI drivers

[Diederik de Haas]

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

On Tue Oct 1, 2024 at 1:51 AM CEST, Mark Brown wrote:
> On Thu, 26 Sep 2024 10:38:11 +0200, Dragan Simic wrote:
> > This is a small series that improves error handling in the probe path
> > of the Rockchip SPI drivers, by using dev_err_probe() properly in multiple
> > places.  It also removes one unnecessary check of a function return value,
> > and performs a bunch of small, rather trivial code cleanups, to make the
> > code neater and a bit easier to read.
> > 
> > Dragan Simic (5):
> >   spi: rockchip: Perform trivial code cleanups
> >   spi: rockchip-sfc: Perform trivial code cleanups
> >   spi: rockchip: Don't check for failed get_fifo_len()
> >   spi: rockchip: Use dev_err_probe() in the probe path
> >   spi: rockchip-sfc: Use dev_err_probe() in the probe path
> > 
> > [...]
>
> Applied to
>
>    https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
>
> Thanks!
>
> [1/5] spi: rockchip: Perform trivial code cleanups
>       commit: d87ec94e48dd2da27fbe948f2dc6c8fedc98fff4
> [2/5] spi: rockchip-sfc: Perform trivial code cleanups
>       commit: 6c510eac1528d8939bad8b6df72c7b23ffec8c25
> [5/5] spi: rockchip-sfc: Use dev_err_probe() in the probe path
>       commit: 1482c40b440fa58f956bc3e1ef3426e0cdbc09e0

It looks like you applied some patches from v1 of this series while the
current version is v3.

https://lore.kernel.org/linux-rockchip/cover.1727601608.git.dsimic@manjaro.org/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: (subset) [PATCH 0/5] Improve error handling in Rockchip SPI drivers

[Dragan Simic]

Hello Diederik and Mark,

On 2024-10-01 02:05, Diederik de Haas wrote:
> On Tue Oct 1, 2024 at 1:51 AM CEST, Mark Brown wrote:
>> On Thu, 26 Sep 2024 10:38:11 +0200, Dragan Simic wrote:
>> > This is a small series that improves error handling in the probe path
>> > of the Rockchip SPI drivers, by using dev_err_probe() properly in multiple
>> > places.  It also removes one unnecessary check of a function return value,
>> > and performs a bunch of small, rather trivial code cleanups, to make the
>> > code neater and a bit easier to read.
>> >
>> > Dragan Simic (5):
>> >   spi: rockchip: Perform trivial code cleanups
>> >   spi: rockchip-sfc: Perform trivial code cleanups
>> >   spi: rockchip: Don't check for failed get_fifo_len()
>> >   spi: rockchip: Use dev_err_probe() in the probe path
>> >   spi: rockchip-sfc: Use dev_err_probe() in the probe path
>> >
>> > [...]
>> 
>> Applied to
>> 
>>    https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 
>> for-next

Thanks for applying a subset of the patches!

>> [1/5] spi: rockchip: Perform trivial code cleanups
>>       commit: d87ec94e48dd2da27fbe948f2dc6c8fedc98fff4
>> [2/5] spi: rockchip-sfc: Perform trivial code cleanups
>>       commit: 6c510eac1528d8939bad8b6df72c7b23ffec8c25
>> [5/5] spi: rockchip-sfc: Use dev_err_probe() in the probe path
>>       commit: 1482c40b440fa58f956bc3e1ef3426e0cdbc09e0
> 
> It looks like you applied some patches from v1 of this series while the
> current version is v3.
> 
> https://lore.kernel.org/linux-rockchip/cover.1727601608.git.dsimic@manjaro.org/

Just checked this by hand, and the three patches that were applied
are the same as the respective patches from the v3 of the series,
albeit being picked from the v1 of the series.  It's just that the
patch 5/5 from the v1 became patch 3/5 in the v3, which pushed the
patches with no dependencies earlier within the series.