| Message ID | 20190305112305.17746-1-ismael.luceno@silicon-gears.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [U-Boot,v2,1/6] i2c: rcar_i2c: Setup SCL/SDA delay at rcar_i2c_set_speed | expand |
On 3/5/19 12:23 PM, Ismael Luceno Cortes wrote: > Cosmetic change. Any call to the recover function would need to do the > same check afterwards, so it's sensible to make it part of the function. > > Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com> > --- > > Notes: > Changes since v1: > - Rebased on top of patch 1050650 ("i2c: rcar_i2c: Add Gen3 SoC support") > - Explained the change > - Replaced C99-style variable declaration > > 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 4556b115bd..21bcf09101 100644 > --- a/drivers/i2c/rcar_i2c.c > +++ b/drivers/i2c/rcar_i2c.c > @@ -77,12 +77,13 @@ 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; > u32 mcra = mcr | RCAR_I2C_ICMCR_FSDA; > int i; > + u32 ret; Can you change this to "reg" or something like that ? $ret is usually a signed int return value, so this could be confusing. > > /* Send 9 SCL pulses */ > for (i = 0; i < 9; i++) { > @@ -102,6 +103,9 @@ static void rcar_i2c_recover(struct udevice *dev) > udelay(5); > writel(mcra | RCAR_I2C_ICMCR_FSCL, priv->base + RCAR_I2C_ICMCR); > udelay(5); > + > + ret = readl(priv->base + RCAR_I2C_ICMSR); > + return ret & RCAR_I2C_ICMCR_FSDA; What about doing this instead reg = readl... if (reg & ...FSDA) return -EBUSY; return 0; That way, the code will use standard errno.h return values. > } > > static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read) > @@ -121,9 +125,7 @@ static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read) > 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 05/Mar/2019 19:32, Marek Vasut wrote: > On 3/5/19 12:23 PM, Ismael Luceno Cortes wrote: > > Cosmetic change. Any call to the recover function would need to do the > > same check afterwards, so it's sensible to make it part of the function. > > > > Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com> > > --- > > > > Notes: > > Changes since v1: > > - Rebased on top of patch 1050650 ("i2c: rcar_i2c: Add Gen3 SoC support") > > - Explained the change > > - Replaced C99-style variable declaration > > > > 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 4556b115bd..21bcf09101 100644 > > --- a/drivers/i2c/rcar_i2c.c > > +++ b/drivers/i2c/rcar_i2c.c > > @@ -77,12 +77,13 @@ 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; > > u32 mcra = mcr | RCAR_I2C_ICMCR_FSDA; > > int i; > > + u32 ret; > > Can you change this to "reg" or something like that ? $ret is usually a > signed int return value, so this could be confusing. Ok. > > > > /* Send 9 SCL pulses */ > > for (i = 0; i < 9; i++) { > > @@ -102,6 +103,9 @@ static void rcar_i2c_recover(struct udevice *dev) > > udelay(5); > > writel(mcra | RCAR_I2C_ICMCR_FSCL, priv->base + RCAR_I2C_ICMCR); > > udelay(5); > > + > > + ret = readl(priv->base + RCAR_I2C_ICMSR); > > + return ret & RCAR_I2C_ICMCR_FSDA; > > What about doing this instead > > reg = readl... > if (reg & ...FSDA) > return -EBUSY; > > return 0; > > That way, the code will use standard errno.h return values. Makes sense.
diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c index 4556b115bd..21bcf09101 100644 --- a/drivers/i2c/rcar_i2c.c +++ b/drivers/i2c/rcar_i2c.c @@ -77,12 +77,13 @@ 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; u32 mcra = mcr | RCAR_I2C_ICMCR_FSDA; int i; + u32 ret; /* Send 9 SCL pulses */ for (i = 0; i < 9; i++) { @@ -102,6 +103,9 @@ static void rcar_i2c_recover(struct udevice *dev) udelay(5); writel(mcra | RCAR_I2C_ICMCR_FSCL, priv->base + RCAR_I2C_ICMCR); udelay(5); + + ret = readl(priv->base + RCAR_I2C_ICMSR); + return ret & RCAR_I2C_ICMCR_FSDA; } static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read) @@ -121,9 +125,7 @@ static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read) 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; }
Cosmetic change. Any call to the recover function would need to do the same check afterwards, so it's sensible to make it part of the function. Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com> --- Notes: Changes since v1: - Rebased on top of patch 1050650 ("i2c: rcar_i2c: Add Gen3 SoC support") - Explained the change - Replaced C99-style variable declaration drivers/i2c/rcar_i2c.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)