diff mbox series

[v7,21/34] i2c: tegra: Don't fall back to PIO mode if DMA configuration fails

Message ID 20200908224006.25636-22-digetx@gmail.com
State New
Headers show
Series Improvements for Tegra I2C driver | expand

Checks

Context Check Description
tagr/GVS success 1122533
tagr/GVS pending 1122533
tagr/GVS-1122491 fail None
tagr/GVS-1122491 pending None

Commit Message

Dmitry Osipenko Sept. 8, 2020, 10:39 p.m. UTC
The DMA code path has been tested well enough and the DMA configuration
performed by tegra_i2c_config_fifo_trig() shouldn't ever fail in practice.
Hence let's remove the obscure transfer-mode switching in order to have a
cleaner and simpler code. Now I2C transfer will be failed if DMA
configuration fails.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/i2c/busses/i2c-tegra.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Comments

Thierry Reding Sept. 17, 2020, 11:47 a.m. UTC | #1
On Wed, Sep 09, 2020 at 01:39:53AM +0300, Dmitry Osipenko wrote:
> The DMA code path has been tested well enough and the DMA configuration
> performed by tegra_i2c_config_fifo_trig() shouldn't ever fail in practice.
> Hence let's remove the obscure transfer-mode switching in order to have a
> cleaner and simpler code. Now I2C transfer will be failed if DMA
> configuration fails.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/i2c/busses/i2c-tegra.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)

I'm not sure that's a good idea. It's always possible that the DMA setup
is going to break because of something that's not related to the I2C
driver itself. Having the system completely break instead of falling
back to PIO mode seems like it would only complicate troubleshooting any
such issues.

Thierry
Dmitry Osipenko Sept. 17, 2020, 3:03 p.m. UTC | #2
17.09.2020 14:47, Thierry Reding пишет:
> On Wed, Sep 09, 2020 at 01:39:53AM +0300, Dmitry Osipenko wrote:
>> The DMA code path has been tested well enough and the DMA configuration
>> performed by tegra_i2c_config_fifo_trig() shouldn't ever fail in practice.
>> Hence let's remove the obscure transfer-mode switching in order to have a
>> cleaner and simpler code. Now I2C transfer will be failed if DMA
>> configuration fails.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-tegra.c | 18 ++++++++++--------
>>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> I'm not sure that's a good idea. It's always possible that the DMA setup
> is going to break because of something that's not related to the I2C
> driver itself. Having the system completely break instead of falling
> back to PIO mode seems like it would only complicate troubleshooting any
> such issues.

That code has zero test coverage because this problem never happens in
practice, hence it should be better to have it removed. We may consider
re-adding it back if there will be a real-world incident, okay?
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 76d3a05b10fc..574b59a8b2a2 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -940,8 +940,7 @@  static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static void tegra_i2c_config_fifo_trig(struct tegra_i2c_dev *i2c_dev,
-				       size_t len)
+static int tegra_i2c_config_fifo_trig(struct tegra_i2c_dev *i2c_dev, size_t len)
 {
 	u32 val, reg;
 	u8 dma_burst;
@@ -992,12 +991,10 @@  static void tegra_i2c_config_fifo_trig(struct tegra_i2c_dev *i2c_dev,
 		if (ret < 0) {
 			dev_err(i2c_dev->dev, "DMA slave config failed: %d\n",
 				ret);
-			dev_err(i2c_dev->dev, "falling back to PIO\n");
-			tegra_i2c_release_dma(i2c_dev);
-			i2c_dev->is_curr_dma_xfer = false;
-		} else {
-			goto out;
+			return ret;
 		}
+
+		goto out;
 	}
 
 	if (i2c_dev->hw->has_mst_fifo)
@@ -1008,6 +1005,8 @@  static void tegra_i2c_config_fifo_trig(struct tegra_i2c_dev *i2c_dev,
 		      I2C_FIFO_CONTROL_RX_TRIG(1);
 out:
 	i2c_writel(i2c_dev, val, reg);
+
+	return 0;
 }
 
 static unsigned long
@@ -1141,7 +1140,10 @@  static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	i2c_dev->is_curr_dma_xfer = (xfer_size > I2C_PIO_MODE_PREFERRED_LEN) &&
 				    i2c_dev->dma_buf &&
 				    !i2c_dev->is_curr_atomic_xfer;
-	tegra_i2c_config_fifo_trig(i2c_dev, xfer_size);
+
+	err = tegra_i2c_config_fifo_trig(i2c_dev, xfer_size);
+	if (err)
+		return err;
 
 	/*
 	 * Transfer time in mSec = Total bits / transfer rate