[PATCH 0/2] Introduce Airoha EN7581 i2c support

[PATCH 0/2] Introduce Airoha EN7581 i2c support

[Lorenzo Bianconi]

Introduce i2c support to Airoha EN7581 SoC

Lorenzo Bianconi (2):
  i2c: mt7621: Add Airoha EN7581 i2c support
  i2c: mt7621: make device_reset optional

 drivers/i2c/busses/Kconfig      | 2 +-
 drivers/i2c/busses/i2c-mt7621.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.45.2

[PATCH 1/2] i2c: mt7621: Add Airoha EN7581 i2c support

[Lorenzo Bianconi]

Introduce i2c support to Airoha EN7581 SoC through the i2c-mt7621
driver.

Tested-by: Ray Liu <ray.liu@airoha.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/i2c/busses/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index fe6e8a1bb607..4d93c00264fc 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -857,7 +857,7 @@ config I2C_MT65XX
 
 config I2C_MT7621
 	tristate "MT7621/MT7628 I2C Controller"
-	depends on (RALINK && (SOC_MT7620 || SOC_MT7621)) || COMPILE_TEST
+	depends on (RALINK && (SOC_MT7620 || SOC_MT7621)) || ARCH_AIROHA || COMPILE_TEST
 	help
 	  Say Y here to include support for I2C controller in the
 	  MediaTek MT7621/MT7628 SoCs.
-- 
2.45.2

Re: [PATCH 1/2] i2c: mt7621: Add Airoha EN7581 i2c support

[AngeloGioacchino Del Regno]

Il 05/07/24 00:16, Lorenzo Bianconi ha scritto:
> Introduce i2c support to Airoha EN7581 SoC through the i2c-mt7621
> driver.
> 
> Tested-by: Ray Liu <ray.liu@airoha.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

[PATCH 2/2] i2c: mt7621: make device_reset optional

[Lorenzo Bianconi]

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
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");
 
-- 
2.45.2

Re: [PATCH 2/2] i2c: mt7621: make device_reset optional

[AngeloGioacchino Del Regno]

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");
>   

Re: [PATCH 2/2] i2c: mt7621: make device_reset optional

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2519 bytes --]

> 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");
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]