Message ID | 20200906185039.22700-2-digetx@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | Improvements for Tegra I2C driver | expand |
On Sun, Sep 6, 2020 at 9:51 PM Dmitry Osipenko <digetx@gmail.com> wrote: > > The tegra_i2c_flush_fifos() shouldn't sleep in atomic transfer and jiffies > are not updating if interrupts are disabled. Let's switch to use iopoll > API helpers for register-polling. The iopoll API provides helpers for both > atomic and non-atomic cases. > > Note that this patch doesn't fix any known problem because normally FIFO > is flushed at the time of starting a new transfer. ... > + if (i2c_dev->is_curr_atomic_xfer) > + err = readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask), > + 1000, 1000000); > + else > + err = readl_relaxed_poll_timeout(addr, val, !(val & mask), > + 1000, 1000000); > + > + if (err) { > + dev_err(i2c_dev->dev, "failed to flush FIFO\n"); > + return err; > } > return 0; return err; ?
07.09.2020 10:56, Andy Shevchenko пишет: > On Sun, Sep 6, 2020 at 9:51 PM Dmitry Osipenko <digetx@gmail.com> wrote: >> >> The tegra_i2c_flush_fifos() shouldn't sleep in atomic transfer and jiffies >> are not updating if interrupts are disabled. Let's switch to use iopoll >> API helpers for register-polling. The iopoll API provides helpers for both >> atomic and non-atomic cases. >> >> Note that this patch doesn't fix any known problem because normally FIFO >> is flushed at the time of starting a new transfer. > > ... > >> + if (i2c_dev->is_curr_atomic_xfer) >> + err = readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask), >> + 1000, 1000000); >> + else >> + err = readl_relaxed_poll_timeout(addr, val, !(val & mask), >> + 1000, 1000000); >> + >> + if (err) { >> + dev_err(i2c_dev->dev, "failed to flush FIFO\n"); > >> + return err; >> } >> return 0; > > return err; ? > The return 0 is intentional here because returned "err" implies that error happened and error code is returned, while 0 explicitly shows the success status. Hence it's always cleaner and more expressive to directly return 0 on success, IMO. I'd prefer to keep this part as-is.
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index 00d3e4d7a01e..ab88cdd70376 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -470,9 +470,9 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev) static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev) { - unsigned long timeout = jiffies + HZ; - unsigned int offset; - u32 mask, val; + u32 mask, val, offset, reg_offset; + void __iomem *addr; + int err; if (i2c_dev->hw->has_mst_fifo) { mask = I2C_MST_FIFO_CONTROL_TX_FLUSH | @@ -488,12 +488,19 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev) val |= mask; i2c_writel(i2c_dev, val, offset); - while (i2c_readl(i2c_dev, offset) & mask) { - if (time_after(jiffies, timeout)) { - dev_warn(i2c_dev->dev, "timeout waiting for fifo flush\n"); - return -ETIMEDOUT; - } - usleep_range(1000, 2000); + reg_offset = tegra_i2c_reg_addr(i2c_dev, offset); + addr = i2c_dev->base + reg_offset; + + if (i2c_dev->is_curr_atomic_xfer) + err = readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask), + 1000, 1000000); + else + err = readl_relaxed_poll_timeout(addr, val, !(val & mask), + 1000, 1000000); + + if (err) { + dev_err(i2c_dev->dev, "failed to flush FIFO\n"); + return err; } return 0; }