| Message ID | 20190304141838.4703-6-ismael.luceno@silicon-gears.com |
|---|---|
| State | Superseded |
| Delegated to: | Heiko Schocher |
| Headers | show |
| Series | [U-Boot,1/6] i2c: rcar_i2c: Fix u32 cast warnings | expand |
On 3/4/19 3:19 PM, Ismael Luceno Cortes wrote: Why is this change needed ? A commit message explaining it would help a lot. > Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com> > --- > drivers/i2c/rcar_i2c.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c > index 5e04b68d95..b829e8b20d 100644 > --- a/drivers/i2c/rcar_i2c.c > +++ b/drivers/i2c/rcar_i2c.c > @@ -68,7 +68,7 @@ static int rcar_i2c_finish(struct udevice *dev) > return ret; > } > > -static void rcar_i2c_recover(struct udevice *dev) > +static int rcar_i2c_recover(struct udevice *dev) > { > struct rcar_i2c_priv *priv = dev_get_priv(dev); > u32 mcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_OBPC; > @@ -93,6 +93,9 @@ static void rcar_i2c_recover(struct udevice *dev) > udelay(5); > writel(mcra | RCAR_I2C_ICMCR_FSCL, priv->base + RCAR_I2C_ICMCR); > udelay(5); > + > + u32 val = readl(priv->base + RCAR_I2C_ICMSR); > + return val & RCAR_I2C_ICMCR_FSDA; Declare all your variables at the beginning of the function, I believe the compiler would even complain about this. > } > > static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read) > @@ -108,12 +111,11 @@ static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read) > writel(0, priv->base + RCAR_I2C_ICMSR); > writel(priv->icccr, priv->base + RCAR_I2C_ICCCR); > > + /* Wait for the bus */ > ret = wait_for_bit_le32(priv->base + RCAR_I2C_ICMCR, > RCAR_I2C_ICMCR_FSDA, false, 2, true); > if (ret) { > - rcar_i2c_recover(dev); > - val = readl(priv->base + RCAR_I2C_ICMSR); > - if (val & RCAR_I2C_ICMCR_FSDA) { > + if (rcar_i2c_recover(dev)) { > dev_err(dev, "Bus busy, aborting\n"); > return ret; > } >
On 04/Mar/2019 20:47, Marek Vasut wrote: > On 3/4/19 3:19 PM, Ismael Luceno Cortes wrote: > > Why is this change needed ? A commit message explaining it would help a lot. It's cosmetic. Any call to the recover function would need to do the same check afterwards, so it' sensible to make it part of the function. > > Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com> > > --- > > drivers/i2c/rcar_i2c.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c > > index 5e04b68d95..b829e8b20d 100644 > > --- a/drivers/i2c/rcar_i2c.c > > +++ b/drivers/i2c/rcar_i2c.c > > @@ -68,7 +68,7 @@ static int rcar_i2c_finish(struct udevice *dev) > > return ret; > > } > > > > -static void rcar_i2c_recover(struct udevice *dev) > > +static int rcar_i2c_recover(struct udevice *dev) > > { > > struct rcar_i2c_priv *priv = dev_get_priv(dev); > > u32 mcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_OBPC; > > @@ -93,6 +93,9 @@ static void rcar_i2c_recover(struct udevice *dev) > > udelay(5); > > writel(mcra | RCAR_I2C_ICMCR_FSCL, priv->base + RCAR_I2C_ICMCR); > > udelay(5); > > + > > + u32 val = readl(priv->base + RCAR_I2C_ICMSR); > > + return val & RCAR_I2C_ICMCR_FSDA; > > Declare all your variables at the beginning of the function, I believe > the compiler would even complain about this. The default mode is -std=gnu11 since GCC 5, and the Makefile defines CSTD_FLAG to that too. > > } > > > > static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read) > > @@ -108,12 +111,11 @@ static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read) > > writel(0, priv->base + RCAR_I2C_ICMSR); > > writel(priv->icccr, priv->base + RCAR_I2C_ICCCR); > > > > + /* Wait for the bus */ > > ret = wait_for_bit_le32(priv->base + RCAR_I2C_ICMCR, > > RCAR_I2C_ICMCR_FSDA, false, 2, true); > > if (ret) { > > - rcar_i2c_recover(dev); > > - val = readl(priv->base + RCAR_I2C_ICMSR); > > - if (val & RCAR_I2C_ICMCR_FSDA) { > > + if (rcar_i2c_recover(dev)) { > > dev_err(dev, "Bus busy, aborting\n"); > > return ret; > > }
On 3/5/19 11:03 AM, Ismael Luceno Cortes wrote: > On 04/Mar/2019 20:47, Marek Vasut wrote: >> On 3/4/19 3:19 PM, Ismael Luceno Cortes wrote: >> >> Why is this change needed ? A commit message explaining it would help a lot. > > It's cosmetic. Any call to the recover function would need to do the > same check afterwards, so it' sensible to make it part of the function. This needs to be in the commit message then. >>> Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com> >>> --- >>> drivers/i2c/rcar_i2c.c | 10 ++++++---- >>> 1 file changed, 6 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c >>> index 5e04b68d95..b829e8b20d 100644 >>> --- a/drivers/i2c/rcar_i2c.c >>> +++ b/drivers/i2c/rcar_i2c.c >>> @@ -68,7 +68,7 @@ static int rcar_i2c_finish(struct udevice *dev) >>> return ret; >>> } >>> >>> -static void rcar_i2c_recover(struct udevice *dev) >>> +static int rcar_i2c_recover(struct udevice *dev) >>> { >>> struct rcar_i2c_priv *priv = dev_get_priv(dev); >>> u32 mcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_OBPC; >>> @@ -93,6 +93,9 @@ static void rcar_i2c_recover(struct udevice *dev) >>> udelay(5); >>> writel(mcra | RCAR_I2C_ICMCR_FSCL, priv->base + RCAR_I2C_ICMCR); >>> udelay(5); >>> + >>> + u32 val = readl(priv->base + RCAR_I2C_ICMSR); >>> + return val & RCAR_I2C_ICMCR_FSDA; >> >> Declare all your variables at the beginning of the function, I believe >> the compiler would even complain about this. > > The default mode is -std=gnu11 since GCC 5, and the Makefile defines > CSTD_FLAG to that too. > >>> } >>> >>> static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read) >>> @@ -108,12 +111,11 @@ static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read) >>> writel(0, priv->base + RCAR_I2C_ICMSR); >>> writel(priv->icccr, priv->base + RCAR_I2C_ICCCR); >>> >>> + /* Wait for the bus */ >>> ret = wait_for_bit_le32(priv->base + RCAR_I2C_ICMCR, >>> RCAR_I2C_ICMCR_FSDA, false, 2, true); >>> if (ret) { >>> - rcar_i2c_recover(dev); >>> - val = readl(priv->base + RCAR_I2C_ICMSR); >>> - if (val & RCAR_I2C_ICMCR_FSDA) { >>> + if (rcar_i2c_recover(dev)) { >>> dev_err(dev, "Bus busy, aborting\n"); >>> return ret; >>> }
diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c index 5e04b68d95..b829e8b20d 100644 --- a/drivers/i2c/rcar_i2c.c +++ b/drivers/i2c/rcar_i2c.c @@ -68,7 +68,7 @@ static int rcar_i2c_finish(struct udevice *dev) return ret; } -static void rcar_i2c_recover(struct udevice *dev) +static int rcar_i2c_recover(struct udevice *dev) { struct rcar_i2c_priv *priv = dev_get_priv(dev); u32 mcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_OBPC; @@ -93,6 +93,9 @@ static void rcar_i2c_recover(struct udevice *dev) udelay(5); writel(mcra | RCAR_I2C_ICMCR_FSCL, priv->base + RCAR_I2C_ICMCR); udelay(5); + + u32 val = readl(priv->base + RCAR_I2C_ICMSR); + return val & RCAR_I2C_ICMCR_FSDA; } static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read) @@ -108,12 +111,11 @@ static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read) writel(0, priv->base + RCAR_I2C_ICMSR); writel(priv->icccr, priv->base + RCAR_I2C_ICCCR); + /* Wait for the bus */ ret = wait_for_bit_le32(priv->base + RCAR_I2C_ICMCR, RCAR_I2C_ICMCR_FSDA, false, 2, true); if (ret) { - rcar_i2c_recover(dev); - val = readl(priv->base + RCAR_I2C_ICMSR); - if (val & RCAR_I2C_ICMCR_FSDA) { + if (rcar_i2c_recover(dev)) { dev_err(dev, "Bus busy, aborting\n"); return ret; }
Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com> --- drivers/i2c/rcar_i2c.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)