Improve error handling in Rockchip SPI drivers

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

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

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

Am Donnerstag, 26. September 2024, 10:38:14 CEST schrieb Dragan Simic:

Quoted 5 lines

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

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

Am Donnerstag, 26. September 2024, 10:38:14 CEST schrieb Dragan Simic:

Quoted 3 lines

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

Quoted 4 lines · Heiko Stuebner

Am Donnerstag, 26. September 2024, 10:38:14 CEST schrieb Dragan Simic:

Quoted 3 lines

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

Quoted 4 lines

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.

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

Signed-off-by Dragan Simic <[email protected]>

---
 drivers/spi/spi-rockchip.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

drivers/spi/spi-rockchip.c +0 -0

index 28879fed03f8..6b5c67a357bb 100644

drivers/spi/spi-rockchip.c +5 -6

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

Am Donnerstag, 26. September 2024, 10:38:15 CEST schrieb Dragan Simic:

Quoted 20 lines

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

Signed-off-by Dragan Simic <[email protected]>

---
 drivers/spi/spi-rockchip.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

drivers/spi/spi-rockchip.c +0 -0

index 28879fed03f8..6b5c67a357bb 100644

drivers/spi/spi-rockchip.c static int rockchip_spi_probe(struct · line 859

Quoted 10 lines

drivers/spi/spi-rockchip.c +3 -3

@@ -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.

Quoted 20 lines

drivers/spi/spi-rockchip.c +5 -6

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

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

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

[...]

Quoted 6 lines · 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.

Quoted 8 lines

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

Quoted 14 lines · Mark Brown

Quoted 13 lines · 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

Quoted 6 lines

Quoted 1 line

[...]

Applied to

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

Quoted 6 lines

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

Quoted 1 line · Diederik de Haas

On Tue Oct 1, 2024 at 1:51 AM CEST, Mark Brown wrote:

Thanks for applying a subset of the patches!

Quoted 10 lines

Quoted 6 lines

[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.

Quoted 1 line

https://lore.kernel.org/linux-rockchip/[email protected]/

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.