Re: [PATCH] drm: adv7511: override i2c address of cec before accessing it
Quoting Antonio Borneo (2021-12-20 15:53:12)
> On Mon, 2021-12-20 at 14:54 +, Kieran Bingham wrote:
> > Hi Antonio,
> >
> > Quoting Antonio Borneo (2021-12-18 18:28:04)
> > > Commit 680532c50bca ("drm: adv7511: Add support for
> > > i2c_new_secondary_device") allows a device tree node to override
> > > the default addresses of the secondary i2c devices. This is useful
> > > for solving address conflicts on the i2c bus.
> > >
> > > In adv7511_init_cec_regmap() the new i2c address of cec device is
> > > read from device tree and immediately accessed, well before it is
> > > written in the proper register to override the default address.
> > > This can cause an i2c error during probe and a consequent probe
> > > failure.
> >
> > Ouch, it does seem that way. I guess no one has used the CEC for
> > quite
> > some time, as it must have been like this for a while?
>
> Using the default i2c address for cec works without problem; apparently
> everyone is happy with such default. The issue appears only when you
> have to override the default cec address.
> The commit 680532c50bca landed in v4.18.
Ok, phew - so the 'normal' case still worked. That makes sense.
Sorry for getting it wrong, and I hope it didn't take too long to find
and fix. I'm sure we'll see it percolate down the stable trees once
integrated.
--
Kieran
> > > Once the new i2c address is read from the device tree, override
> > > the default address before any attempt to access the cec.
> >
> > Reviewed-by: Kieran Bingham
>
> Thanks!
> Antonio
>
> > > Tested with adv7533 and stm32mp157f.
> > >
> > > Signed-off-by: Antonio Borneo
> > > Fixes: 680532c50bca ("drm: adv7511: Add support for
> > > i2c_new_secondary_device")
> > > ---
> > > To: Andrzej Hajda
> > > To: Neil Armstrong
> > > To: Robert Foss
> > > To: Laurent Pinchart
> > > To: Jonas Karlman
> > > To: Jernej Skrabec
> > > To: David Airlie
> > > To: Daniel Vetter
> > > To: Kieran Bingham
> > > To: dri-devel@lists.freedesktop.org
> > > Cc: linux-ker...@vger.kernel.org
> > > Cc: linux-st...@st-md-mailman.stormreply.com
> > > ---
> > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 7 ---
> > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > index 76555ae64e9c..629e05286fd9 100644
> > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > @@ -1048,6 +1048,10 @@ static int adv7511_init_cec_regmap(struct
> > > adv7511 *adv)
> > >
> > > ADV7511_CEC_I2C_ADDR_DEFAULT);
> > > if (IS_ERR(adv->i2c_cec))
> > > return PTR_ERR(adv->i2c_cec);
> > > +
> > > + regmap_write(adv->regmap, ADV7511_REG_CEC_I2C_ADDR,
> > > + adv->i2c_cec->addr << 1);
> > > +
> > > i2c_set_clientdata(adv->i2c_cec, adv);
> > >
> > > adv->regmap_cec = devm_regmap_init_i2c(adv->i2c_cec,
> > > @@ -1252,9 +1256,6 @@ static int adv7511_probe(struct i2c_client
> > > *i2c, const struct i2c_device_id *id)
> > > if (ret)
> > > goto err_i2c_unregister_packet;
> > >
> > > - regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
> > > - adv7511->i2c_cec->addr << 1);
> > > -
> > > INIT_WORK(>hpd_work, adv7511_hpd_work);
> > >
> > > if (i2c->irq) {
> > >
> > > base-commit: fc74881c28d314b10efac016ef49df4ff40b8b97
> > > --
> > > 2.34.1
> > >
>
Re: [PATCH] drm: adv7511: override i2c address of cec before accessing it
On Mon, 2021-12-20 at 14:54 +, Kieran Bingham wrote:
> Hi Antonio,
>
> Quoting Antonio Borneo (2021-12-18 18:28:04)
> > Commit 680532c50bca ("drm: adv7511: Add support for
> > i2c_new_secondary_device") allows a device tree node to override
> > the default addresses of the secondary i2c devices. This is useful
> > for solving address conflicts on the i2c bus.
> >
> > In adv7511_init_cec_regmap() the new i2c address of cec device is
> > read from device tree and immediately accessed, well before it is
> > written in the proper register to override the default address.
> > This can cause an i2c error during probe and a consequent probe
> > failure.
>
> Ouch, it does seem that way. I guess no one has used the CEC for
> quite
> some time, as it must have been like this for a while?
Using the default i2c address for cec works without problem; apparently
everyone is happy with such default. The issue appears only when you
have to override the default cec address.
The commit 680532c50bca landed in v4.18.
> > Once the new i2c address is read from the device tree, override
> > the default address before any attempt to access the cec.
>
> Reviewed-by: Kieran Bingham
Thanks!
Antonio
> > Tested with adv7533 and stm32mp157f.
> >
> > Signed-off-by: Antonio Borneo
> > Fixes: 680532c50bca ("drm: adv7511: Add support for
> > i2c_new_secondary_device")
> > ---
> > To: Andrzej Hajda
> > To: Neil Armstrong
> > To: Robert Foss
> > To: Laurent Pinchart
> > To: Jonas Karlman
> > To: Jernej Skrabec
> > To: David Airlie
> > To: Daniel Vetter
> > To: Kieran Bingham
> > To: dri-devel@lists.freedesktop.org
> > Cc: linux-ker...@vger.kernel.org
> > Cc: linux-st...@st-md-mailman.stormreply.com
> > ---
> > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 7 ---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > index 76555ae64e9c..629e05286fd9 100644
> > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > @@ -1048,6 +1048,10 @@ static int adv7511_init_cec_regmap(struct
> > adv7511 *adv)
> >
> > ADV7511_CEC_I2C_ADDR_DEFAULT);
> > if (IS_ERR(adv->i2c_cec))
> > return PTR_ERR(adv->i2c_cec);
> > +
> > + regmap_write(adv->regmap, ADV7511_REG_CEC_I2C_ADDR,
> > + adv->i2c_cec->addr << 1);
> > +
> > i2c_set_clientdata(adv->i2c_cec, adv);
> >
> > adv->regmap_cec = devm_regmap_init_i2c(adv->i2c_cec,
> > @@ -1252,9 +1256,6 @@ static int adv7511_probe(struct i2c_client
> > *i2c, const struct i2c_device_id *id)
> > if (ret)
> > goto err_i2c_unregister_packet;
> >
> > - regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
> > - adv7511->i2c_cec->addr << 1);
> > -
> > INIT_WORK(>hpd_work, adv7511_hpd_work);
> >
> > if (i2c->irq) {
> >
> > base-commit: fc74881c28d314b10efac016ef49df4ff40b8b97
> > --
> > 2.34.1
> >
Re: [PATCH] drm: adv7511: override i2c address of cec before accessing it
Hi Antonio,
Quoting Antonio Borneo (2021-12-18 18:28:04)
> Commit 680532c50bca ("drm: adv7511: Add support for
> i2c_new_secondary_device") allows a device tree node to override
> the default addresses of the secondary i2c devices. This is useful
> for solving address conflicts on the i2c bus.
>
> In adv7511_init_cec_regmap() the new i2c address of cec device is
> read from device tree and immediately accessed, well before it is
> written in the proper register to override the default address.
> This can cause an i2c error during probe and a consequent probe
> failure.
Ouch, it does seem that way. I guess no one has used the CEC for quite
some time, as it must have been like this for a while?
> Once the new i2c address is read from the device tree, override
> the default address before any attempt to access the cec.
Reviewed-by: Kieran Bingham
> Tested with adv7533 and stm32mp157f.
>
> Signed-off-by: Antonio Borneo
> Fixes: 680532c50bca ("drm: adv7511: Add support for i2c_new_secondary_device")
> ---
> To: Andrzej Hajda
> To: Neil Armstrong
> To: Robert Foss
> To: Laurent Pinchart
> To: Jonas Karlman
> To: Jernej Skrabec
> To: David Airlie
> To: Daniel Vetter
> To: Kieran Bingham
> To: dri-devel@lists.freedesktop.org
> Cc: linux-ker...@vger.kernel.org
> Cc: linux-st...@st-md-mailman.stormreply.com
> ---
> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 76555ae64e9c..629e05286fd9 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -1048,6 +1048,10 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
> ADV7511_CEC_I2C_ADDR_DEFAULT);
> if (IS_ERR(adv->i2c_cec))
> return PTR_ERR(adv->i2c_cec);
> +
> + regmap_write(adv->regmap, ADV7511_REG_CEC_I2C_ADDR,
> +adv->i2c_cec->addr << 1);
> +
> i2c_set_clientdata(adv->i2c_cec, adv);
>
> adv->regmap_cec = devm_regmap_init_i2c(adv->i2c_cec,
> @@ -1252,9 +1256,6 @@ static int adv7511_probe(struct i2c_client *i2c, const
> struct i2c_device_id *id)
> if (ret)
> goto err_i2c_unregister_packet;
>
> - regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
> -adv7511->i2c_cec->addr << 1);
> -
> INIT_WORK(>hpd_work, adv7511_hpd_work);
>
> if (i2c->irq) {
>
> base-commit: fc74881c28d314b10efac016ef49df4ff40b8b97
> --
> 2.34.1
>
[PATCH] drm: adv7511: override i2c address of cec before accessing it
Commit 680532c50bca ("drm: adv7511: Add support for
i2c_new_secondary_device") allows a device tree node to override
the default addresses of the secondary i2c devices. This is useful
for solving address conflicts on the i2c bus.
In adv7511_init_cec_regmap() the new i2c address of cec device is
read from device tree and immediately accessed, well before it is
written in the proper register to override the default address.
This can cause an i2c error during probe and a consequent probe
failure.
Once the new i2c address is read from the device tree, override
the default address before any attempt to access the cec.
Tested with adv7533 and stm32mp157f.
Signed-off-by: Antonio Borneo
Fixes: 680532c50bca ("drm: adv7511: Add support for i2c_new_secondary_device")
---
To: Andrzej Hajda
To: Neil Armstrong
To: Robert Foss
To: Laurent Pinchart
To: Jonas Karlman
To: Jernej Skrabec
To: David Airlie
To: Daniel Vetter
To: Kieran Bingham
To: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
Cc: linux-st...@st-md-mailman.stormreply.com
---
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 76555ae64e9c..629e05286fd9 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1048,6 +1048,10 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
ADV7511_CEC_I2C_ADDR_DEFAULT);
if (IS_ERR(adv->i2c_cec))
return PTR_ERR(adv->i2c_cec);
+
+ regmap_write(adv->regmap, ADV7511_REG_CEC_I2C_ADDR,
+adv->i2c_cec->addr << 1);
+
i2c_set_clientdata(adv->i2c_cec, adv);
adv->regmap_cec = devm_regmap_init_i2c(adv->i2c_cec,
@@ -1252,9 +1256,6 @@ static int adv7511_probe(struct i2c_client *i2c, const
struct i2c_device_id *id)
if (ret)
goto err_i2c_unregister_packet;
- regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR,
-adv7511->i2c_cec->addr << 1);
-
INIT_WORK(>hpd_work, adv7511_hpd_work);
if (i2c->irq) {
base-commit: fc74881c28d314b10efac016ef49df4ff40b8b97
--
2.34.1
