Re: [PATCH v7 01/34] i2c: tegra: Make tegra_i2c_flush_fifos() usable in atomic transfer

[Thierry Reding]

On Wed, Sep 09, 2020 at 01:39:33AM +0300, Dmitry Osipenko 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.
> 
> Reviewed-by: Michał Mirosław <mirq-li...@rere.qmqm.pl>
> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>
> ---
>  drivers/i2c/busses/i2c-tegra.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> 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;

Is there are reason why we need reg_offset? Seems to me like we could
simplify this, see below.

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

Why not just:

        offset = tegra_i2c_reg_offset(i2c_dev, offset);
        addr = i2c_dev->base + offset;

or even just:

        addr = i2c_dev->base + tegra_i2c_reg_offset(i2c_dev, offset);

? That makes the patch much smaller because you don't have to rewrite
the whole variable declaration block and just add the "addr" and "err"
variables while removing "timeout".

Thierry

signature.asc
Description: PGP signature