| Message ID | 20190305175434.13107-2-wsa+renesas@sang-engineering.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | i2c: rcar: make DMA more robust | expand |
On Tue, Mar 5, 2019 at 7:52 PM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > Use a macro for the hardcoded value and apply a build check. If it is > not met, the driver logic will not work anymore. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
Hi Wolfram, On Tue, Mar 5, 2019 at 7:52 PM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > Use a macro for the hardcoded value and apply a build check. If it is > not met, the driver logic will not work anymore. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/i2c/busses/i2c-rcar.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c > index 3ce74edcd70c..925858915569 100644 > --- a/drivers/i2c/busses/i2c-rcar.c > +++ b/drivers/i2c/busses/i2c-rcar.c > @@ -921,6 +922,9 @@ static int rcar_i2c_probe(struct platform_device *pdev) > struct i2c_timings i2c_t; > int irq, ret; > > + /* Otherwise logic will break because some bytes must always use PIO */ > + BUILD_BUG_ON_MSG(RCAR_MIN_DMA_LEN < 3, "Invalid min DMA length"); Given patch 3/3, it should still work with RCAR_MIN_DMA_LEN == 2, right? > + > priv = devm_kzalloc(dev, sizeof(struct rcar_i2c_priv), GFP_KERNEL); > if (!priv) > return -ENOMEM; Gr{oetje,eeting}s, Geert
On Mon, Mar 11, 2019 at 11:08:13AM +0100, Geert Uytterhoeven wrote: > Hi Wolfram, > > On Tue, Mar 5, 2019 at 7:52 PM Wolfram Sang > <wsa+renesas@sang-engineering.com> wrote: > > Use a macro for the hardcoded value and apply a build check. If it is > > not met, the driver logic will not work anymore. > > > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > --- > > drivers/i2c/busses/i2c-rcar.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c > > index 3ce74edcd70c..925858915569 100644 > > --- a/drivers/i2c/busses/i2c-rcar.c > > +++ b/drivers/i2c/busses/i2c-rcar.c > > > @@ -921,6 +922,9 @@ static int rcar_i2c_probe(struct platform_device *pdev) > > struct i2c_timings i2c_t; > > int irq, ret; > > > > + /* Otherwise logic will break because some bytes must always use PIO */ > > + BUILD_BUG_ON_MSG(RCAR_MIN_DMA_LEN < 3, "Invalid min DMA length"); > > Given patch 3/3, it should still work with RCAR_MIN_DMA_LEN == 2, right? Nope. It is not that we transfer one byte more with PIO now. The change in patch 3 is that we explicitly wait for an interrupt when the (already existing) PIO transfer ended. Before that patch, we assumed DMA would take over on its own once the data register is empty again. Should I update the commit message to make this more clear? Also, it is the _read_ case which needs the minimum lenght of 3. This is fixing the _write_ code path :)
On Tue, Mar 05, 2019 at 06:54:32PM +0100, Wolfram Sang wrote: > Use a macro for the hardcoded value and apply a build check. If it is > not met, the driver logic will not work anymore. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Reviewed-by: Simon Horman <horms+renesas@verge.net.au> > --- > drivers/i2c/busses/i2c-rcar.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c > index 3ce74edcd70c..925858915569 100644 > --- a/drivers/i2c/busses/i2c-rcar.c > +++ b/drivers/i2c/busses/i2c-rcar.c > @@ -85,6 +85,7 @@ > /* ICFBSCR */ > #define TCYC17 0x0f /* 17*Tcyc delay 1st bit between SDA and SCL */ > > +#define RCAR_MIN_DMA_LEN 8 > > #define RCAR_BUS_PHASE_START (MDBS | MIE | ESG) > #define RCAR_BUS_PHASE_DATA (MDBS | MIE) > @@ -412,8 +413,8 @@ static void rcar_i2c_dma(struct rcar_i2c_priv *priv) > int len; > > /* Do various checks to see if DMA is feasible at all */ > - if (IS_ERR(chan) || msg->len < 8 || !(msg->flags & I2C_M_DMA_SAFE) || > - (read && priv->flags & ID_P_NO_RXDMA)) > + if (IS_ERR(chan) || msg->len < RCAR_MIN_DMA_LEN || > + !(msg->flags & I2C_M_DMA_SAFE) || (read && priv->flags & ID_P_NO_RXDMA)) > return; > > if (read) { > @@ -921,6 +922,9 @@ static int rcar_i2c_probe(struct platform_device *pdev) > struct i2c_timings i2c_t; > int irq, ret; > > + /* Otherwise logic will break because some bytes must always use PIO */ > + BUILD_BUG_ON_MSG(RCAR_MIN_DMA_LEN < 3, "Invalid min DMA length"); > + > priv = devm_kzalloc(dev, sizeof(struct rcar_i2c_priv), GFP_KERNEL); > if (!priv) > return -ENOMEM; > -- > 2.11.0 >
On Tue, Mar 05, 2019 at 06:54:32PM +0100, Wolfram Sang wrote: > Use a macro for the hardcoded value and apply a build check. If it is > not met, the driver logic will not work anymore. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Applied to for-next, thanks!
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c index 3ce74edcd70c..925858915569 100644 --- a/drivers/i2c/busses/i2c-rcar.c +++ b/drivers/i2c/busses/i2c-rcar.c @@ -85,6 +85,7 @@ /* ICFBSCR */ #define TCYC17 0x0f /* 17*Tcyc delay 1st bit between SDA and SCL */ +#define RCAR_MIN_DMA_LEN 8 #define RCAR_BUS_PHASE_START (MDBS | MIE | ESG) #define RCAR_BUS_PHASE_DATA (MDBS | MIE) @@ -412,8 +413,8 @@ static void rcar_i2c_dma(struct rcar_i2c_priv *priv) int len; /* Do various checks to see if DMA is feasible at all */ - if (IS_ERR(chan) || msg->len < 8 || !(msg->flags & I2C_M_DMA_SAFE) || - (read && priv->flags & ID_P_NO_RXDMA)) + if (IS_ERR(chan) || msg->len < RCAR_MIN_DMA_LEN || + !(msg->flags & I2C_M_DMA_SAFE) || (read && priv->flags & ID_P_NO_RXDMA)) return; if (read) { @@ -921,6 +922,9 @@ static int rcar_i2c_probe(struct platform_device *pdev) struct i2c_timings i2c_t; int irq, ret; + /* Otherwise logic will break because some bytes must always use PIO */ + BUILD_BUG_ON_MSG(RCAR_MIN_DMA_LEN < 3, "Invalid min DMA length"); + priv = devm_kzalloc(dev, sizeof(struct rcar_i2c_priv), GFP_KERNEL); if (!priv) return -ENOMEM;
Use a macro for the hardcoded value and apply a build check. If it is not met, the driver logic will not work anymore. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/i2c/busses/i2c-rcar.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)