| Message ID | 3ea697be239c450cbc622f3e2e9dff8bf17515c0.1720130844.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce Airoha EN7581 i2c support | expand |
Il 05/07/24 00:16, Lorenzo Bianconi ha scritto: > Rely on device_reset_optional() instead of device_reset() in > mtk_i2c_reset routine since Airoha EN7581 reset controller does not > implement the reset callback and device reset is not mandatory on this Not mandatory means that the reset line for I2C exists, but you don't want to use it? If so, that's not right. On the other hand, if there is an issue with using that reset line and you want to avoid using it for a good reason, please describe it into the commit description and - in that case, you'll have to add a EN7581 compatible and code to avoid calling device_reset() on that SoC. What you're doing here is removing an error log on SoCs that do *need* that reset line to be in place, so, hiding a problem that could be causing other issues on peripherals connected to the I2C bus of those other SoCs. Cheers, Angelo > SoC. This will remove the following log: > > i2c-mt7621 1fbf8000.i2c0: I2C reset failed! > > Tested-by: Ray Liu <ray.liu@airoha.com> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > --- > drivers/i2c/busses/i2c-mt7621.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-mt7621.c b/drivers/i2c/busses/i2c-mt7621.c > index 81d46169bc1f..8cd89a2f59df 100644 > --- a/drivers/i2c/busses/i2c-mt7621.c > +++ b/drivers/i2c/busses/i2c-mt7621.c > @@ -85,7 +85,7 @@ static void mtk_i2c_reset(struct mtk_i2c *i2c) > { > int ret; > > - ret = device_reset(i2c->adap.dev.parent); > + ret = device_reset_optional(i2c->adap.dev.parent); > if (ret) > dev_err(i2c->dev, "I2C reset failed!\n"); >
> Il 05/07/24 00:16, Lorenzo Bianconi ha scritto: > > Rely on device_reset_optional() instead of device_reset() in > > mtk_i2c_reset routine since Airoha EN7581 reset controller does not > > implement the reset callback and device reset is not mandatory on this > > Not mandatory means that the reset line for I2C exists, but you don't want > to use it? If so, that's not right. ack, it was not so clear, sorry for that. What I mean with "not mandatory" is the mt7621 i2c controller works on EN7581 SoC even w/o this reset configuration. > > On the other hand, if there is an issue with using that reset line and you > want to avoid using it for a good reason, please describe it into the commit > description and - in that case, you'll have to add a EN7581 compatible and > code to avoid calling device_reset() on that SoC. what I mean is device_reset() will call .reset() callback of reset APIs. EN7581 reset driver does not implemetn this callback, just .assert() and .deassert() (I think the hw does not support the reset line automatic re-arm) so the call to device_reset() will always fails. Maybe a possible approach would be adding in the reset driver something like: en7581_reset() { en7581_assert(); msleep(x); en7581_deassert(); msleep(x); } What do you think? It seems a bit hacky to me and I am not sure what would be the right delay in this case. Regards, Lorenzo > > What you're doing here is removing an error log on SoCs that do *need* that > reset line to be in place, so, hiding a problem that could be causing other > issues on peripherals connected to the I2C bus of those other SoCs. > > Cheers, > Angelo > > > SoC. This will remove the following log: > > > > i2c-mt7621 1fbf8000.i2c0: I2C reset failed! > > > > Tested-by: Ray Liu <ray.liu@airoha.com> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > --- > > drivers/i2c/busses/i2c-mt7621.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/i2c/busses/i2c-mt7621.c b/drivers/i2c/busses/i2c-mt7621.c > > index 81d46169bc1f..8cd89a2f59df 100644 > > --- a/drivers/i2c/busses/i2c-mt7621.c > > +++ b/drivers/i2c/busses/i2c-mt7621.c > > @@ -85,7 +85,7 @@ static void mtk_i2c_reset(struct mtk_i2c *i2c) > > { > > int ret; > > - ret = device_reset(i2c->adap.dev.parent); > > + ret = device_reset_optional(i2c->adap.dev.parent); > > if (ret) > > dev_err(i2c->dev, "I2C reset failed!\n"); > >
diff --git a/drivers/i2c/busses/i2c-mt7621.c b/drivers/i2c/busses/i2c-mt7621.c index 81d46169bc1f..8cd89a2f59df 100644 --- a/drivers/i2c/busses/i2c-mt7621.c +++ b/drivers/i2c/busses/i2c-mt7621.c @@ -85,7 +85,7 @@ static void mtk_i2c_reset(struct mtk_i2c *i2c) { int ret; - ret = device_reset(i2c->adap.dev.parent); + ret = device_reset_optional(i2c->adap.dev.parent); if (ret) dev_err(i2c->dev, "I2C reset failed!\n");