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 antonio.borneo@foss.st.com Fixes: 680532c50bca ("drm: adv7511: Add support for i2c_new_secondary_device") --- To: Andrzej Hajda a.hajda@samsung.com To: Neil Armstrong narmstrong@baylibre.com To: Robert Foss robert.foss@linaro.org To: Laurent Pinchart Laurent.pinchart@ideasonboard.com To: Jonas Karlman jonas@kwiboo.se To: Jernej Skrabec jernej.skrabec@gmail.com To: David Airlie airlied@linux.ie To: Daniel Vetter daniel@ffwll.ch To: Kieran Bingham kieran.bingham+renesas@ideasonboard.com To: dri-devel@lists.freedesktop.org Cc: linux-kernel@vger.kernel.org Cc: linux-stm32@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(&adv7511->hpd_work, adv7511_hpd_work);
if (i2c->irq) {
base-commit: fc74881c28d314b10efac016ef49df4ff40b8b97
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 kieran.bingham+renesas@ideasonboard.com
Tested with adv7533 and stm32mp157f.
Signed-off-by: Antonio Borneo antonio.borneo@foss.st.com Fixes: 680532c50bca ("drm: adv7511: Add support for i2c_new_secondary_device")
To: Andrzej Hajda a.hajda@samsung.com To: Neil Armstrong narmstrong@baylibre.com To: Robert Foss robert.foss@linaro.org To: Laurent Pinchart Laurent.pinchart@ideasonboard.com To: Jonas Karlman jonas@kwiboo.se To: Jernej Skrabec jernej.skrabec@gmail.com To: David Airlie airlied@linux.ie To: Daniel Vetter daniel@ffwll.ch To: Kieran Bingham kieran.bingham+renesas@ideasonboard.com To: dri-devel@lists.freedesktop.org Cc: linux-kernel@vger.kernel.org Cc: linux-stm32@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(&adv7511->hpd_work, adv7511_hpd_work); if (i2c->irq) {base-commit: fc74881c28d314b10efac016ef49df4ff40b8b97
2.34.1
On Mon, 2021-12-20 at 14:54 +0000, 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 kieran.bingham+renesas@ideasonboard.com
Thanks! Antonio
Tested with adv7533 and stm32mp157f.
Signed-off-by: Antonio Borneo antonio.borneo@foss.st.com Fixes: 680532c50bca ("drm: adv7511: Add support for i2c_new_secondary_device")
To: Andrzej Hajda a.hajda@samsung.com To: Neil Armstrong narmstrong@baylibre.com To: Robert Foss robert.foss@linaro.org To: Laurent Pinchart Laurent.pinchart@ideasonboard.com To: Jonas Karlman jonas@kwiboo.se To: Jernej Skrabec jernej.skrabec@gmail.com To: David Airlie airlied@linux.ie To: Daniel Vetter daniel@ffwll.ch To: Kieran Bingham kieran.bingham+renesas@ideasonboard.com To: dri-devel@lists.freedesktop.org Cc: linux-kernel@vger.kernel.org Cc: linux-stm32@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(&adv7511->hpd_work, adv7511_hpd_work); if (i2c->irq) {
base-commit: fc74881c28d314b10efac016ef49df4ff40b8b97
2.34.1
Quoting Antonio Borneo (2021-12-20 15:53:12)
On Mon, 2021-12-20 at 14:54 +0000, 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 kieran.bingham+renesas@ideasonboard.com
Thanks! Antonio
Tested with adv7533 and stm32mp157f.
Signed-off-by: Antonio Borneo antonio.borneo@foss.st.com Fixes: 680532c50bca ("drm: adv7511: Add support for i2c_new_secondary_device")
To: Andrzej Hajda a.hajda@samsung.com To: Neil Armstrong narmstrong@baylibre.com To: Robert Foss robert.foss@linaro.org To: Laurent Pinchart Laurent.pinchart@ideasonboard.com To: Jonas Karlman jonas@kwiboo.se To: Jernej Skrabec jernej.skrabec@gmail.com To: David Airlie airlied@linux.ie To: Daniel Vetter daniel@ffwll.ch To: Kieran Bingham kieran.bingham+renesas@ideasonboard.com To: dri-devel@lists.freedesktop.org Cc: linux-kernel@vger.kernel.org Cc: linux-stm32@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(&adv7511->hpd_work, adv7511_hpd_work); if (i2c->irq) {
base-commit: fc74881c28d314b10efac016ef49df4ff40b8b97
2.34.1
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 antonio.borneo@foss.st.com Fixes: 680532c50bca ("drm: adv7511: Add support for i2c_new_secondary_device") Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
---
This patch got somehow lost, I'm resending it. Added reviewed by Kieran Bingham. Rebased on drm-misc-next.
--- 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 5bb9300040dd..074c2e650cae 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -1065,6 +1065,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, @@ -1271,9 +1275,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(&adv7511->hpd_work, adv7511_hpd_work);
if (i2c->irq) {
base-commit: dfa687bffc8a4a21ed929c7dececf01b8f1f52ee
On Tue, 7 Jun 2022 at 23:32, Antonio Borneo antonio.borneo@foss.st.com wrote:
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 antonio.borneo@foss.st.com Fixes: 680532c50bca ("drm: adv7511: Add support for i2c_new_secondary_device") Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
This patch got somehow lost, I'm resending it. Added reviewed by Kieran Bingham. Rebased on drm-misc-next.
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 5bb9300040dd..074c2e650cae 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -1065,6 +1065,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,@@ -1271,9 +1275,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(&adv7511->hpd_work, adv7511_hpd_work); if (i2c->irq) {base-commit: dfa687bffc8a4a21ed929c7dececf01b8f1f52ee
2.36.1
Applied to drm-misc-next.
dri-devel@lists.freedesktop.org
-
Antonio Borneo -
Kieran Bingham -
Robert Foss