| Message ID | 1516625389-6362-3-git-send-email-kieran.bingham@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/22/2018 01:50 PM, Kieran Bingham wrote: > The ADV7511 has four 256-byte maps that can be accessed via the main I²C > ports. Each map has it own I²C address and acts as a standard slave > device on the I²C bus. > > Allow a device tree node to override the default addresses so that > address conflicts with other devices on the same bus may be resolved at > the board description level. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> I've been working on the same thing, but you've beat me to it! Patch looks mostly OK, but I think you are missing this piece: https://github.com/analogdevicesinc/linux/commit/ba9b57507cb78724a606eb24104e22fea942437d#diff-2cf1828c644e351adefabe9509410400L553 > --- > .../bindings/display/bridge/adi,adv7511.txt | 10 +++++- > drivers/gpu/drm/bridge/adv7511/adv7511.h | 4 +++ > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 36 ++++++++++++++-------- > 3 files changed, 37 insertions(+), 13 deletions(-) > > diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > index 0047b1394c70..f6bb9f6d3f48 100644 > --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > @@ -70,6 +70,9 @@ Optional properties: > rather than generate its own timings for HDMI output. > - clocks: from common clock binding: reference to the CEC clock. > - clock-names: from common clock binding: must be "cec". > +- reg-names : Names of maps with programmable addresses. > + It can contain any map needing a non-default address. > + Possible maps names are : "main", "edid", "cec", "packet" > > Required nodes: > > @@ -88,7 +91,12 @@ Example > > adv7511w: hdmi@39 { > compatible = "adi,adv7511w"; > - reg = <39>; > + /* > + * The EDID page will be accessible on address 0x66 on the i2c > + * bus. All other maps continue to use their default addresses. > + */ > + reg = <0x39 0x66>; > + reg-names = "main", "edid"; > interrupt-parent = <&gpio3>; > interrupts = <29 IRQ_TYPE_EDGE_FALLING>; > clocks = <&cec_clock>; > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h > index d034b2cb5eee..7d81ce3808e0 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h > @@ -53,8 +53,10 @@ > #define ADV7511_REG_POWER 0x41 > #define ADV7511_REG_STATUS 0x42 > #define ADV7511_REG_EDID_I2C_ADDR 0x43 > +#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT 0x3f > #define ADV7511_REG_PACKET_ENABLE1 0x44 > #define ADV7511_REG_PACKET_I2C_ADDR 0x45 > +#define ADV7511_REG_PACKET_I2C_ADDR_DEFAULT 0x38 > #define ADV7511_REG_DSD_ENABLE 0x46 > #define ADV7511_REG_VIDEO_INPUT_CFG2 0x48 > #define ADV7511_REG_INFOFRAME_UPDATE 0x4a > @@ -89,6 +91,7 @@ > #define ADV7511_REG_TMDS_CLOCK_INV 0xde > #define ADV7511_REG_ARC_CTRL 0xdf > #define ADV7511_REG_CEC_I2C_ADDR 0xe1 > +#define ADV7511_REG_CEC_I2C_ADDR_DEFAULT 0x3c > #define ADV7511_REG_CEC_CTRL 0xe2 > #define ADV7511_REG_CHIP_ID_HIGH 0xf5 > #define ADV7511_REG_CHIP_ID_LOW 0xf6 > @@ -322,6 +325,7 @@ struct adv7511 { > struct i2c_client *i2c_main; > struct i2c_client *i2c_edid; > struct i2c_client *i2c_cec; > + struct i2c_client *i2c_packet; > > struct regmap *regmap; > struct regmap *regmap_cec; > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index efa29db5fc2b..7ec33837752b 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -969,8 +969,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv) > { > int ret; > > - adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter, > - adv->i2c_main->addr - 1); > + adv->i2c_cec = i2c_new_secondary_device(adv->i2c_main, "cec", > + ADV7511_REG_CEC_I2C_ADDR_DEFAULT); > if (!adv->i2c_cec) > return -ENOMEM; > i2c_set_clientdata(adv->i2c_cec, adv); > @@ -1082,8 +1082,6 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) > struct adv7511_link_config link_config; > struct adv7511 *adv7511; > struct device *dev = &i2c->dev; > - unsigned int main_i2c_addr = i2c->addr << 1; > - unsigned int edid_i2c_addr = main_i2c_addr + 4; > unsigned int val; > int ret; > > @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) > if (ret) > goto uninit_regulators; > > - regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr); > - regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, > - main_i2c_addr - 0xa); > - regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, > - main_i2c_addr - 2); > - > adv7511_packet_disable(adv7511, 0xffff); > > - adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1); > + adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid", > + ADV7511_REG_EDID_I2C_ADDR_DEFAULT); > if (!adv7511->i2c_edid) { > ret = -ENOMEM; > goto uninit_regulators; > } > > + regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, > + adv7511->i2c_edid->addr << 1); > + > ret = adv7511_init_cec_regmap(adv7511); > if (ret) > goto err_i2c_unregister_edid; > > + regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, > + adv7511->i2c_cec->addr << 1); > + > + adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet", > + ADV7511_REG_PACKET_I2C_ADDR_DEFAULT); > + if (!adv7511->i2c_packet) { > + ret = -ENOMEM; > + goto err_unregister_cec; > + } > + > + regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, > + adv7511->i2c_packet->addr << 1); > + > INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work); > > if (i2c->irq) { > @@ -1181,7 +1190,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) > IRQF_ONESHOT, dev_name(dev), > adv7511); > if (ret) > - goto err_unregister_cec; > + goto err_unregister_packet; > } > > adv7511_power_off(adv7511); > @@ -1203,6 +1212,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) > adv7511_audio_init(dev, adv7511); > return 0; > > +err_unregister_packet: > + i2c_unregister_device(adv7511->i2c_packet); > err_unregister_cec: > i2c_unregister_device(adv7511->i2c_cec); > if (adv7511->cec_clk) > @@ -1234,6 +1245,7 @@ static int adv7511_remove(struct i2c_client *i2c) > cec_unregister_adapter(adv7511->cec_adap); > > i2c_unregister_device(adv7511->i2c_edid); > + i2c_unregister_device(adv7511->i2c_packet); > > return 0; > } >
On 22/01/18 13:00, Lars-Peter Clausen wrote: > On 01/22/2018 01:50 PM, Kieran Bingham wrote: >> The ADV7511 has four 256-byte maps that can be accessed via the main I²C >> ports. Each map has it own I²C address and acts as a standard slave >> device on the I²C bus. >> >> Allow a device tree node to override the default addresses so that >> address conflicts with other devices on the same bus may be resolved at >> the board description level. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > I've been working on the same thing, but you've beat me to it! Patch looks > mostly OK, but I think you are missing this piece: > > https://github.com/analogdevicesinc/linux/commit/ba9b57507cb78724a606eb24104e22fea942437d#diff-2cf1828c644e351adefabe9509410400L553 Ah yes - Thanks, - you're correct - I had missed that bit. Added locally for a v2. -- Kieran >> --- >> .../bindings/display/bridge/adi,adv7511.txt | 10 +++++- >> drivers/gpu/drm/bridge/adv7511/adv7511.h | 4 +++ >> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 36 ++++++++++++++-------- >> 3 files changed, 37 insertions(+), 13 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt >> index 0047b1394c70..f6bb9f6d3f48 100644 >> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt >> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt >> @@ -70,6 +70,9 @@ Optional properties: >> rather than generate its own timings for HDMI output. >> - clocks: from common clock binding: reference to the CEC clock. >> - clock-names: from common clock binding: must be "cec". >> +- reg-names : Names of maps with programmable addresses. >> + It can contain any map needing a non-default address. >> + Possible maps names are : "main", "edid", "cec", "packet" >> >> Required nodes: >> >> @@ -88,7 +91,12 @@ Example >> >> adv7511w: hdmi@39 { >> compatible = "adi,adv7511w"; >> - reg = <39>; >> + /* >> + * The EDID page will be accessible on address 0x66 on the i2c >> + * bus. All other maps continue to use their default addresses. >> + */ >> + reg = <0x39 0x66>; >> + reg-names = "main", "edid"; >> interrupt-parent = <&gpio3>; >> interrupts = <29 IRQ_TYPE_EDGE_FALLING>; >> clocks = <&cec_clock>; >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h >> index d034b2cb5eee..7d81ce3808e0 100644 >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h >> @@ -53,8 +53,10 @@ >> #define ADV7511_REG_POWER 0x41 >> #define ADV7511_REG_STATUS 0x42 >> #define ADV7511_REG_EDID_I2C_ADDR 0x43 >> +#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT 0x3f >> #define ADV7511_REG_PACKET_ENABLE1 0x44 >> #define ADV7511_REG_PACKET_I2C_ADDR 0x45 >> +#define ADV7511_REG_PACKET_I2C_ADDR_DEFAULT 0x38 >> #define ADV7511_REG_DSD_ENABLE 0x46 >> #define ADV7511_REG_VIDEO_INPUT_CFG2 0x48 >> #define ADV7511_REG_INFOFRAME_UPDATE 0x4a >> @@ -89,6 +91,7 @@ >> #define ADV7511_REG_TMDS_CLOCK_INV 0xde >> #define ADV7511_REG_ARC_CTRL 0xdf >> #define ADV7511_REG_CEC_I2C_ADDR 0xe1 >> +#define ADV7511_REG_CEC_I2C_ADDR_DEFAULT 0x3c >> #define ADV7511_REG_CEC_CTRL 0xe2 >> #define ADV7511_REG_CHIP_ID_HIGH 0xf5 >> #define ADV7511_REG_CHIP_ID_LOW 0xf6 >> @@ -322,6 +325,7 @@ struct adv7511 { >> struct i2c_client *i2c_main; >> struct i2c_client *i2c_edid; >> struct i2c_client *i2c_cec; >> + struct i2c_client *i2c_packet; >> >> struct regmap *regmap; >> struct regmap *regmap_cec; >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> index efa29db5fc2b..7ec33837752b 100644 >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> @@ -969,8 +969,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv) >> { >> int ret; >> >> - adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter, >> - adv->i2c_main->addr - 1); >> + adv->i2c_cec = i2c_new_secondary_device(adv->i2c_main, "cec", >> + ADV7511_REG_CEC_I2C_ADDR_DEFAULT); >> if (!adv->i2c_cec) >> return -ENOMEM; >> i2c_set_clientdata(adv->i2c_cec, adv); >> @@ -1082,8 +1082,6 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) >> struct adv7511_link_config link_config; >> struct adv7511 *adv7511; >> struct device *dev = &i2c->dev; >> - unsigned int main_i2c_addr = i2c->addr << 1; >> - unsigned int edid_i2c_addr = main_i2c_addr + 4; >> unsigned int val; >> int ret; >> >> @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) >> if (ret) >> goto uninit_regulators; >> >> - regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr); >> - regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, >> - main_i2c_addr - 0xa); >> - regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, >> - main_i2c_addr - 2); >> - >> adv7511_packet_disable(adv7511, 0xffff); >> >> - adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1); >> + adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid", >> + ADV7511_REG_EDID_I2C_ADDR_DEFAULT); >> if (!adv7511->i2c_edid) { >> ret = -ENOMEM; >> goto uninit_regulators; >> } >> >> + regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, >> + adv7511->i2c_edid->addr << 1); >> + >> ret = adv7511_init_cec_regmap(adv7511); >> if (ret) >> goto err_i2c_unregister_edid; >> >> + regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, >> + adv7511->i2c_cec->addr << 1); >> + >> + adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet", >> + ADV7511_REG_PACKET_I2C_ADDR_DEFAULT); >> + if (!adv7511->i2c_packet) { >> + ret = -ENOMEM; >> + goto err_unregister_cec; >> + } >> + >> + regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, >> + adv7511->i2c_packet->addr << 1); >> + >> INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work); >> >> if (i2c->irq) { >> @@ -1181,7 +1190,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) >> IRQF_ONESHOT, dev_name(dev), >> adv7511); >> if (ret) >> - goto err_unregister_cec; >> + goto err_unregister_packet; >> } >> >> adv7511_power_off(adv7511); >> @@ -1203,6 +1212,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) >> adv7511_audio_init(dev, adv7511); >> return 0; >> >> +err_unregister_packet: >> + i2c_unregister_device(adv7511->i2c_packet); >> err_unregister_cec: >> i2c_unregister_device(adv7511->i2c_cec); >> if (adv7511->cec_clk) >> @@ -1234,6 +1245,7 @@ static int adv7511_remove(struct i2c_client *i2c) >> cec_unregister_adapter(adv7511->cec_adap); >> >> i2c_unregister_device(adv7511->i2c_edid); >> + i2c_unregister_device(adv7511->i2c_packet); >> >> return 0; >> } >> >
Hi, On 01/22/2018 06:20 PM, Kieran Bingham wrote: > The ADV7511 has four 256-byte maps that can be accessed via the main I²C > ports. Each map has it own I²C address and acts as a standard slave > device on the I²C bus. > > Allow a device tree node to override the default addresses so that > address conflicts with other devices on the same bus may be resolved at > the board description level. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > .../bindings/display/bridge/adi,adv7511.txt | 10 +++++- > drivers/gpu/drm/bridge/adv7511/adv7511.h | 4 +++ > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 36 ++++++++++++++-------- > 3 files changed, 37 insertions(+), 13 deletions(-) > > diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > index 0047b1394c70..f6bb9f6d3f48 100644 > --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > @@ -70,6 +70,9 @@ Optional properties: > rather than generate its own timings for HDMI output. > - clocks: from common clock binding: reference to the CEC clock. > - clock-names: from common clock binding: must be "cec". > +- reg-names : Names of maps with programmable addresses. > + It can contain any map needing a non-default address. > + Possible maps names are : "main", "edid", "cec", "packet" > > Required nodes: > > @@ -88,7 +91,12 @@ Example > > adv7511w: hdmi@39 { > compatible = "adi,adv7511w"; > - reg = <39>; > + /* > + * The EDID page will be accessible on address 0x66 on the i2c > + * bus. All other maps continue to use their default addresses. > + */ > + reg = <0x39 0x66>; > + reg-names = "main", "edid"; > interrupt-parent = <&gpio3>; > interrupts = <29 IRQ_TYPE_EDGE_FALLING>; > clocks = <&cec_clock>; > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h > index d034b2cb5eee..7d81ce3808e0 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h > @@ -53,8 +53,10 @@ > #define ADV7511_REG_POWER 0x41 > #define ADV7511_REG_STATUS 0x42 > #define ADV7511_REG_EDID_I2C_ADDR 0x43 > +#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT 0x3f > #define ADV7511_REG_PACKET_ENABLE1 0x44 > #define ADV7511_REG_PACKET_I2C_ADDR 0x45 > +#define ADV7511_REG_PACKET_I2C_ADDR_DEFAULT 0x38 > #define ADV7511_REG_DSD_ENABLE 0x46 > #define ADV7511_REG_VIDEO_INPUT_CFG2 0x48 > #define ADV7511_REG_INFOFRAME_UPDATE 0x4a > @@ -89,6 +91,7 @@ > #define ADV7511_REG_TMDS_CLOCK_INV 0xde > #define ADV7511_REG_ARC_CTRL 0xdf > #define ADV7511_REG_CEC_I2C_ADDR 0xe1 > +#define ADV7511_REG_CEC_I2C_ADDR_DEFAULT 0x3c Minor comment: The defines above make look like new register defines. It would be nice to remove the "REG_" from them, and place them somewhere after the register definitions. > #define ADV7511_REG_CEC_CTRL 0xe2 > #define ADV7511_REG_CHIP_ID_HIGH 0xf5 > #define ADV7511_REG_CHIP_ID_LOW 0xf6 > @@ -322,6 +325,7 @@ struct adv7511 { > struct i2c_client *i2c_main; > struct i2c_client *i2c_edid; > struct i2c_client *i2c_cec; > + struct i2c_client *i2c_packet; > > struct regmap *regmap; > struct regmap *regmap_cec; > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index efa29db5fc2b..7ec33837752b 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -969,8 +969,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv) > { > int ret; > > - adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter, > - adv->i2c_main->addr - 1); This patch avoids deriving the CEC/EDID map addresses from the main map. I think this would break what the original patch tried to do: d25a4cbba4b9da7c2d674b2f8ecf84af1b24988e "drm/bridge: adv7511: add support for the 2nd chip" Maybe the default macros can be a function of the main address? > + adv->i2c_cec = i2c_new_secondary_device(adv->i2c_main, "cec", > + ADV7511_REG_CEC_I2C_ADDR_DEFAULT); Also, I'm a bit unclear on the default address values. For example, previously, the CEC address was calculated as "adv->i2c_main->addr - 1", which is 0x38. The new CEC_I2C_ADDR_DEFAULT define sets it to 0x3c. Thanks, Archit > if (!adv->i2c_cec) > return -ENOMEM; > i2c_set_clientdata(adv->i2c_cec, adv); > @@ -1082,8 +1082,6 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) > struct adv7511_link_config link_config; > struct adv7511 *adv7511; > struct device *dev = &i2c->dev; > - unsigned int main_i2c_addr = i2c->addr << 1; > - unsigned int edid_i2c_addr = main_i2c_addr + 4; > unsigned int val; > int ret; > > @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) > if (ret) > goto uninit_regulators; > > - regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr); > - regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, > - main_i2c_addr - 0xa); > - regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, > - main_i2c_addr - 2); > - > adv7511_packet_disable(adv7511, 0xffff); > > - adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1); > + adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid", > + ADV7511_REG_EDID_I2C_ADDR_DEFAULT); > if (!adv7511->i2c_edid) { > ret = -ENOMEM; > goto uninit_regulators; > } > > + regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, > + adv7511->i2c_edid->addr << 1); > + > ret = adv7511_init_cec_regmap(adv7511); > if (ret) > goto err_i2c_unregister_edid; > > + regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, > + adv7511->i2c_cec->addr << 1); > + > + adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet", > + ADV7511_REG_PACKET_I2C_ADDR_DEFAULT); > + if (!adv7511->i2c_packet) { > + ret = -ENOMEM; > + goto err_unregister_cec; > + } > + > + regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, > + adv7511->i2c_packet->addr << 1); > + > INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work); > > if (i2c->irq) { > @@ -1181,7 +1190,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) > IRQF_ONESHOT, dev_name(dev), > adv7511); > if (ret) > - goto err_unregister_cec; > + goto err_unregister_packet; > } > > adv7511_power_off(adv7511); > @@ -1203,6 +1212,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) > adv7511_audio_init(dev, adv7511); > return 0; > > +err_unregister_packet: > + i2c_unregister_device(adv7511->i2c_packet); > err_unregister_cec: > i2c_unregister_device(adv7511->i2c_cec); > if (adv7511->cec_clk) > @@ -1234,6 +1245,7 @@ static int adv7511_remove(struct i2c_client *i2c) > cec_unregister_adapter(adv7511->cec_adap); > > i2c_unregister_device(adv7511->i2c_edid); > + i2c_unregister_device(adv7511->i2c_packet); > > return 0; > } >
Hi Kieran, Thank you for the patch. On Monday, 22 January 2018 14:50:00 EET Kieran Bingham wrote: > The ADV7511 has four 256-byte maps that can be accessed via the main I²C > ports. Each map has it own I²C address and acts as a standard slave > device on the I²C bus. > > Allow a device tree node to override the default addresses so that > address conflicts with other devices on the same bus may be resolved at > the board description level. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > .../bindings/display/bridge/adi,adv7511.txt | 10 +++++- I don't mind personally, but device tree maintainers usually ask for DT bindings changes to be split to a separate patch. > drivers/gpu/drm/bridge/adv7511/adv7511.h | 4 +++ > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 36 ++++++++++++------- > 3 files changed, 37 insertions(+), 13 deletions(-) > > diff --git > a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > index 0047b1394c70..f6bb9f6d3f48 100644 > --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > @@ -70,6 +70,9 @@ Optional properties: > rather than generate its own timings for HDMI output. > - clocks: from common clock binding: reference to the CEC clock. > - clock-names: from common clock binding: must be "cec". > +- reg-names : Names of maps with programmable addresses. > + It can contain any map needing a non-default address. > + Possible maps names are : "main", "edid", "cec", "packet" Is the reg-names property (and the additional maps) mandatory or optional ? If mandatory you should also update the existing DT sources that use those bindings. If optional you should define which I2C addresses will be used when the maps are not specified (and in that case I think we should go for the addresses listed as default in the datasheet, which correspond to the current driver implementation when the main address is 0x3d/0x7a). You should also update the definition of the reg property that currently just states - reg: I2C slave address And finally you might want to define the term "map" in this context. Here's a proposal (if we make all maps mandatory). The ADV7511 internal registers are split into four pages exposed through different I2C addresses, creating four register maps. The I2C addresses of all four maps shall be specified by the reg and reg-names property. - reg: I2C slave addresses, one per reg-names entry - reg-names: map names, shall be "main", "edid", "cec", "packet" > Required nodes: > > @@ -88,7 +91,12 @@ Example > > adv7511w: hdmi@39 { > compatible = "adi,adv7511w"; > - reg = <39>; > + /* > + * The EDID page will be accessible on address 0x66 on the i2c > + * bus. All other maps continue to use their default addresses. > + */ > + reg = <0x39 0x66>; > + reg-names = "main", "edid"; > interrupt-parent = <&gpio3>; > interrupts = <29 IRQ_TYPE_EDGE_FALLING>; > clocks = <&cec_clock>; > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h > b/drivers/gpu/drm/bridge/adv7511/adv7511.h > index d034b2cb5eee..7d81ce3808e0 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h > @@ -53,8 +53,10 @@ > #define ADV7511_REG_POWER 0x41 > #define ADV7511_REG_STATUS 0x42 > #define ADV7511_REG_EDID_I2C_ADDR 0x43 > +#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT 0x3f > #define ADV7511_REG_PACKET_ENABLE1 0x44 > #define ADV7511_REG_PACKET_I2C_ADDR 0x45 > +#define ADV7511_REG_PACKET_I2C_ADDR_DEFAULT 0x38 > #define ADV7511_REG_DSD_ENABLE 0x46 > #define ADV7511_REG_VIDEO_INPUT_CFG2 0x48 > #define ADV7511_REG_INFOFRAME_UPDATE 0x4a > @@ -89,6 +91,7 @@ > #define ADV7511_REG_TMDS_CLOCK_INV 0xde > #define ADV7511_REG_ARC_CTRL 0xdf > #define ADV7511_REG_CEC_I2C_ADDR 0xe1 > +#define ADV7511_REG_CEC_I2C_ADDR_DEFAULT 0x3c > #define ADV7511_REG_CEC_CTRL 0xe2 > #define ADV7511_REG_CHIP_ID_HIGH 0xf5 > #define ADV7511_REG_CHIP_ID_LOW 0xf6 > @@ -322,6 +325,7 @@ struct adv7511 { > struct i2c_client *i2c_main; > struct i2c_client *i2c_edid; > struct i2c_client *i2c_cec; > + struct i2c_client *i2c_packet; > > struct regmap *regmap; > struct regmap *regmap_cec; > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index efa29db5fc2b..7ec33837752b 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -969,8 +969,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv) > { > int ret; > > - adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter, > - adv->i2c_main->addr - 1); > + adv->i2c_cec = i2c_new_secondary_device(adv->i2c_main, "cec", > + ADV7511_REG_CEC_I2C_ADDR_DEFAULT); > if (!adv->i2c_cec) > return -ENOMEM; > i2c_set_clientdata(adv->i2c_cec, adv); > @@ -1082,8 +1082,6 @@ static int adv7511_probe(struct i2c_client *i2c, const > struct i2c_device_id *id) > struct adv7511_link_config link_config; > struct adv7511 *adv7511; > struct device *dev = &i2c->dev; > - unsigned int main_i2c_addr = i2c->addr << 1; > - unsigned int edid_i2c_addr = main_i2c_addr + 4; > unsigned int val; > int ret; > > @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c, > const struct i2c_device_id *id) > if (ret) > goto uninit_regulators; > > - regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr); > - regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, > - main_i2c_addr - 0xa); > - regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, > - main_i2c_addr - 2); > - > adv7511_packet_disable(adv7511, 0xffff); > > - adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1); > + adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid", > + ADV7511_REG_EDID_I2C_ADDR_DEFAULT); > if (!adv7511->i2c_edid) { > ret = -ENOMEM; I wonder if this is the right error code. Maybe -EINVAL ? In most cases errors will be caused by invalid addresses (out of memory and device_register() failures can happen too but should be less common). It would be nice if i2c_new_secondary_device() returned an ERR_PTR, but that's out of scope. > goto uninit_regulators; > } > > + regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, > + adv7511->i2c_edid->addr << 1); > + > ret = adv7511_init_cec_regmap(adv7511); > if (ret) > goto err_i2c_unregister_edid; > > + regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, > + adv7511->i2c_cec->addr << 1); > + > + adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet", > + ADV7511_REG_PACKET_I2C_ADDR_DEFAULT); > + if (!adv7511->i2c_packet) { > + ret = -ENOMEM; > + goto err_unregister_cec; > + } > + > + regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, > + adv7511->i2c_packet->addr << 1); > + > INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work); > > if (i2c->irq) { > @@ -1181,7 +1190,7 @@ static int adv7511_probe(struct i2c_client *i2c, const > struct i2c_device_id *id) > IRQF_ONESHOT, dev_name(dev), > adv7511); > if (ret) > - goto err_unregister_cec; > + goto err_unregister_packet; > } > > adv7511_power_off(adv7511); > @@ -1203,6 +1212,8 @@ static int adv7511_probe(struct i2c_client *i2c, const > struct i2c_device_id *id) > adv7511_audio_init(dev, adv7511); > return 0; > > +err_unregister_packet: > + i2c_unregister_device(adv7511->i2c_packet); > err_unregister_cec: > i2c_unregister_device(adv7511->i2c_cec); > if (adv7511->cec_clk) > @@ -1234,6 +1245,7 @@ static int adv7511_remove(struct i2c_client *i2c) > cec_unregister_adapter(adv7511->cec_adap); > > i2c_unregister_device(adv7511->i2c_edid); > + i2c_unregister_device(adv7511->i2c_packet); > > return 0; > }
On Mon, Jan 22, 2018 at 12:50:01PM +0000, Kieran Bingham wrote: > The ADV7511 has four 256-byte maps that can be accessed via the main I²C > ports. Each map has it own I²C address and acts as a standard slave > device on the I²C bus. > > Allow a device tree node to override the default addresses so that > address conflicts with other devices on the same bus may be resolved at > the board description level. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > .../bindings/display/bridge/adi,adv7511.txt | 10 +++++- Reviewed-by: Rob Herring <robh@kernel.org> > drivers/gpu/drm/bridge/adv7511/adv7511.h | 4 +++ > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 36 ++++++++++++++-------- > 3 files changed, 37 insertions(+), 13 deletions(-)
On Mon, Jan 29, 2018 at 12:26:00PM +0200, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Monday, 22 January 2018 14:50:00 EET Kieran Bingham wrote: > > The ADV7511 has four 256-byte maps that can be accessed via the main I²C > > ports. Each map has it own I²C address and acts as a standard slave > > device on the I²C bus. > > > > Allow a device tree node to override the default addresses so that > > address conflicts with other devices on the same bus may be resolved at > > the board description level. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > .../bindings/display/bridge/adi,adv7511.txt | 10 +++++- > > I don't mind personally, but device tree maintainers usually ask for DT > bindings changes to be split to a separate patch. Or perfect bindings, then I won't ask to split it just for that (usually). > > drivers/gpu/drm/bridge/adv7511/adv7511.h | 4 +++ > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 36 ++++++++++++------- > > 3 files changed, 37 insertions(+), 13 deletions(-)
Hi Archit, Thank you for your review, On 29/01/18 04:11, Archit Taneja wrote: > Hi, > > On 01/22/2018 06:20 PM, Kieran Bingham wrote: >> The ADV7511 has four 256-byte maps that can be accessed via the main I²C >> ports. Each map has it own I²C address and acts as a standard slave >> device on the I²C bus. >> >> Allow a device tree node to override the default addresses so that >> address conflicts with other devices on the same bus may be resolved at >> the board description level. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> .../bindings/display/bridge/adi,adv7511.txt | 10 +++++- >> drivers/gpu/drm/bridge/adv7511/adv7511.h | 4 +++ >> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 36 ++++++++++++++-------- >> 3 files changed, 37 insertions(+), 13 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt >> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt >> index 0047b1394c70..f6bb9f6d3f48 100644 >> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt >> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt >> @@ -70,6 +70,9 @@ Optional properties: >> rather than generate its own timings for HDMI output. >> - clocks: from common clock binding: reference to the CEC clock. >> - clock-names: from common clock binding: must be "cec". >> +- reg-names : Names of maps with programmable addresses. >> + It can contain any map needing a non-default address. >> + Possible maps names are : "main", "edid", "cec", "packet" >> Required nodes: >> @@ -88,7 +91,12 @@ Example >> adv7511w: hdmi@39 { >> compatible = "adi,adv7511w"; >> - reg = <39>; >> + /* >> + * The EDID page will be accessible on address 0x66 on the i2c >> + * bus. All other maps continue to use their default addresses. >> + */ >> + reg = <0x39 0x66>; >> + reg-names = "main", "edid"; >> interrupt-parent = <&gpio3>; >> interrupts = <29 IRQ_TYPE_EDGE_FALLING>; >> clocks = <&cec_clock>; >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h >> b/drivers/gpu/drm/bridge/adv7511/adv7511.h >> index d034b2cb5eee..7d81ce3808e0 100644 >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h >> @@ -53,8 +53,10 @@ >> #define ADV7511_REG_POWER 0x41 >> #define ADV7511_REG_STATUS 0x42 >> #define ADV7511_REG_EDID_I2C_ADDR 0x43 >> +#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT 0x3f >> #define ADV7511_REG_PACKET_ENABLE1 0x44 >> #define ADV7511_REG_PACKET_I2C_ADDR 0x45 >> +#define ADV7511_REG_PACKET_I2C_ADDR_DEFAULT 0x38 >> #define ADV7511_REG_DSD_ENABLE 0x46 >> #define ADV7511_REG_VIDEO_INPUT_CFG2 0x48 >> #define ADV7511_REG_INFOFRAME_UPDATE 0x4a >> @@ -89,6 +91,7 @@ >> #define ADV7511_REG_TMDS_CLOCK_INV 0xde >> #define ADV7511_REG_ARC_CTRL 0xdf >> #define ADV7511_REG_CEC_I2C_ADDR 0xe1 >> +#define ADV7511_REG_CEC_I2C_ADDR_DEFAULT 0x3c > > Minor comment: The defines above make look like new register > defines. It would be nice to remove the "REG_" from them, and > place them somewhere after the register definitions. Sure. >> #define ADV7511_REG_CEC_CTRL 0xe2 >> #define ADV7511_REG_CHIP_ID_HIGH 0xf5 >> #define ADV7511_REG_CHIP_ID_LOW 0xf6 >> @@ -322,6 +325,7 @@ struct adv7511 { >> struct i2c_client *i2c_main; >> struct i2c_client *i2c_edid; >> struct i2c_client *i2c_cec; >> + struct i2c_client *i2c_packet; >> struct regmap *regmap; >> struct regmap *regmap_cec; >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> index efa29db5fc2b..7ec33837752b 100644 >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> @@ -969,8 +969,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv) >> { >> int ret; >> - adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter, >> - adv->i2c_main->addr - 1); > > This patch avoids deriving the CEC/EDID map addresses from the main map. I think > this would break what the original patch tried to do: That was intentional. The ADV7511 data-sheet defines default addresses for these maps. (or rather the hardware does) > > d25a4cbba4b9da7c2d674b2f8ecf84af1b24988e > "drm/bridge: adv7511: add support for the 2nd chip" > > Maybe the default macros can be a function of the main address? I'm loathed to do that, because then intrinsic knowledge must be known that if I define a device at address X ... it will also use magic offset A B and C. IMO - the driver should define the defaults to match the hardware. Anything else is an override ... <Welcoming comments, and arguments here> >> + adv->i2c_cec = i2c_new_secondary_device(adv->i2c_main, "cec", >> + ADV7511_REG_CEC_I2C_ADDR_DEFAULT); > > Also, I'm a bit unclear on the default address values. For example, previously, > the CEC > address was calculated as "adv->i2c_main->addr - 1", which is 0x38. The new > CEC_I2C_ADDR_DEFAULT > define sets it to 0x3c. The registers defined in the programming guide [0] define default addresses which is set by the hardware (and noteworthy, reset to; at low power mode!) Page : Register : Default : Purpose 165 : 0x43 : 0x3f << 1 : EDID Memory Address 166 : 0x45 : 0x38 << 1 : Packet Memory I2C Map Address 181 : 0xE1 : 0x3c << 1 : CEC ID Actually - I've just seen there's a good table at page 16 :D The ADV7511 main register map can appear at either 0x39 or 0x3D depending on the input to a pull-up/down pin. Thus - that implies that perhaps Sergei must have been working on a device located at 0x3D, to determine the offset of -1 ... but we have two values for computing the offset depending on whether you start at 0x39 or 0x3D. And also - I can't even be sure that I can infer the starting address, as the offsets for the EDID and Packet do not correlate here. [0] http://www.analog.com/media/en/technical-documentation/user-guides/ADV7511_Programming_Guide.pdf > Thanks, > Archit > >> if (!adv->i2c_cec) >> return -ENOMEM; >> i2c_set_clientdata(adv->i2c_cec, adv); >> @@ -1082,8 +1082,6 @@ static int adv7511_probe(struct i2c_client *i2c, const >> struct i2c_device_id *id) >> struct adv7511_link_config link_config; >> struct adv7511 *adv7511; >> struct device *dev = &i2c->dev; >> - unsigned int main_i2c_addr = i2c->addr << 1; >> - unsigned int edid_i2c_addr = main_i2c_addr + 4; This computation doesn't correspond to anything except a random (perhaps platform specific) offset. >> unsigned int val; >> int ret; >> @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c, >> const struct i2c_device_id *id) >> if (ret) >> goto uninit_regulators; >> - regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr); >> - regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, >> - main_i2c_addr - 0xa); Packet address here is written as an offset of -0x0a, which gives 0x2f or 0x33 ... ? I think these current offsets are platform specific to a specific platform :) -- Regards Kieran >> - regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, >> - main_i2c_addr - 2); >> - >> adv7511_packet_disable(adv7511, 0xffff); >> - adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1); >> + adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid", >> + ADV7511_REG_EDID_I2C_ADDR_DEFAULT); >> if (!adv7511->i2c_edid) { >> ret = -ENOMEM; >> goto uninit_regulators; >> } >> + regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, >> + adv7511->i2c_edid->addr << 1); >> + >> ret = adv7511_init_cec_regmap(adv7511); >> if (ret) >> goto err_i2c_unregister_edid; >> + regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, >> + adv7511->i2c_cec->addr << 1); >> + >> + adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet", >> + ADV7511_REG_PACKET_I2C_ADDR_DEFAULT); >> + if (!adv7511->i2c_packet) { >> + ret = -ENOMEM; >> + goto err_unregister_cec; >> + } >> + >> + regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, >> + adv7511->i2c_packet->addr << 1); >> + >> INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work); >> if (i2c->irq) { >> @@ -1181,7 +1190,7 @@ static int adv7511_probe(struct i2c_client *i2c, const >> struct i2c_device_id *id) >> IRQF_ONESHOT, dev_name(dev), >> adv7511); >> if (ret) >> - goto err_unregister_cec; >> + goto err_unregister_packet; >> } >> adv7511_power_off(adv7511); >> @@ -1203,6 +1212,8 @@ static int adv7511_probe(struct i2c_client *i2c, const >> struct i2c_device_id *id) >> adv7511_audio_init(dev, adv7511); >> return 0; >> +err_unregister_packet: >> + i2c_unregister_device(adv7511->i2c_packet); >> err_unregister_cec: >> i2c_unregister_device(adv7511->i2c_cec); >> if (adv7511->cec_clk) >> @@ -1234,6 +1245,7 @@ static int adv7511_remove(struct i2c_client *i2c) >> cec_unregister_adapter(adv7511->cec_adap); >> i2c_unregister_device(adv7511->i2c_edid); >> + i2c_unregister_device(adv7511->i2c_packet); >> return 0; >> } >> >
Hi Laurent, On 29/01/18 10:26, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. Thanks for your review, > On Monday, 22 January 2018 14:50:00 EET Kieran Bingham wrote: >> The ADV7511 has four 256-byte maps that can be accessed via the main I²C >> ports. Each map has it own I²C address and acts as a standard slave >> device on the I²C bus. >> >> Allow a device tree node to override the default addresses so that >> address conflicts with other devices on the same bus may be resolved at >> the board description level. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> .../bindings/display/bridge/adi,adv7511.txt | 10 +++++- > > I don't mind personally, but device tree maintainers usually ask for DT > bindings changes to be split to a separate patch. > >> drivers/gpu/drm/bridge/adv7511/adv7511.h | 4 +++ >> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 36 ++++++++++++------- >> 3 files changed, 37 insertions(+), 13 deletions(-) >> >> diff --git >> a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt >> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt >> index 0047b1394c70..f6bb9f6d3f48 100644 >> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt >> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt >> @@ -70,6 +70,9 @@ Optional properties: >> rather than generate its own timings for HDMI output. >> - clocks: from common clock binding: reference to the CEC clock. >> - clock-names: from common clock binding: must be "cec". >> +- reg-names : Names of maps with programmable addresses. >> + It can contain any map needing a non-default address. >> + Possible maps names are : "main", "edid", "cec", "packet" > > Is the reg-names property (and the additional maps) mandatory or optional ? > If mandatory you should also update the existing DT sources that use those > bindings. They are currently optional. I do prefer it that way - but perhaps due to an issue mentioned below we might need to make this driver mandatory ? > If optional you should define which I2C addresses will be used when > the maps are not specified > (and in that case I think we should go for the > addresses listed as default in the datasheet, which correspond to the current > driver implementation when the main address is 0x3d/0x7a). The current addresses do not correspond to the datasheet, even when the implementation main address is set to 0x3d. Thus, in my opinion - they are currently 'wrong' - but perhaps changing them is considered breakage too. A particular issue will arise here too - as on this device - when the device is in low-power mode (after probe, before use) - the maps will respond on their *hardware default* addresses (the ones implemented in this patch), and thus consume that address on the I2C bus regardless of their settings in the driver. > You should also update the definition of the reg property that currently just > states > > - reg: I2C slave address > > And finally you might want to define the term "map" in this context. Here's a > proposal (if we make all maps mandatory). > > The ADV7511 internal registers are split into four pages exposed through > different I2C addresses, creating four register maps. The I2C addresses of all > four maps shall be specified by the reg and reg-names property. > > - reg: I2C slave addresses, one per reg-names entry > - reg-names: map names, shall be "main", "edid", "cec", "packet" > >> Required nodes: >> >> @@ -88,7 +91,12 @@ Example >> >> adv7511w: hdmi@39 { >> compatible = "adi,adv7511w"; >> - reg = <39>; >> + /* >> + * The EDID page will be accessible on address 0x66 on the i2c >> + * bus. All other maps continue to use their default addresses. >> + */ >> + reg = <0x39 0x66>; >> + reg-names = "main", "edid"; >> interrupt-parent = <&gpio3>; >> interrupts = <29 IRQ_TYPE_EDGE_FALLING>; >> clocks = <&cec_clock>; >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h >> b/drivers/gpu/drm/bridge/adv7511/adv7511.h >> index d034b2cb5eee..7d81ce3808e0 100644 >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h >> @@ -53,8 +53,10 @@ >> #define ADV7511_REG_POWER 0x41 >> #define ADV7511_REG_STATUS 0x42 >> #define ADV7511_REG_EDID_I2C_ADDR 0x43 >> +#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT 0x3f >> #define ADV7511_REG_PACKET_ENABLE1 0x44 >> #define ADV7511_REG_PACKET_I2C_ADDR 0x45 >> +#define ADV7511_REG_PACKET_I2C_ADDR_DEFAULT 0x38 >> #define ADV7511_REG_DSD_ENABLE 0x46 >> #define ADV7511_REG_VIDEO_INPUT_CFG2 0x48 >> #define ADV7511_REG_INFOFRAME_UPDATE 0x4a >> @@ -89,6 +91,7 @@ >> #define ADV7511_REG_TMDS_CLOCK_INV 0xde >> #define ADV7511_REG_ARC_CTRL 0xdf >> #define ADV7511_REG_CEC_I2C_ADDR 0xe1 >> +#define ADV7511_REG_CEC_I2C_ADDR_DEFAULT 0x3c >> #define ADV7511_REG_CEC_CTRL 0xe2 >> #define ADV7511_REG_CHIP_ID_HIGH 0xf5 >> #define ADV7511_REG_CHIP_ID_LOW 0xf6 >> @@ -322,6 +325,7 @@ struct adv7511 { >> struct i2c_client *i2c_main; >> struct i2c_client *i2c_edid; >> struct i2c_client *i2c_cec; >> + struct i2c_client *i2c_packet; >> >> struct regmap *regmap; >> struct regmap *regmap_cec; >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> index efa29db5fc2b..7ec33837752b 100644 >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> @@ -969,8 +969,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv) >> { >> int ret; >> >> - adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter, >> - adv->i2c_main->addr - 1); >> + adv->i2c_cec = i2c_new_secondary_device(adv->i2c_main, "cec", >> + ADV7511_REG_CEC_I2C_ADDR_DEFAULT); >> if (!adv->i2c_cec) >> return -ENOMEM; >> i2c_set_clientdata(adv->i2c_cec, adv); >> @@ -1082,8 +1082,6 @@ static int adv7511_probe(struct i2c_client *i2c, const >> struct i2c_device_id *id) >> struct adv7511_link_config link_config; >> struct adv7511 *adv7511; >> struct device *dev = &i2c->dev; >> - unsigned int main_i2c_addr = i2c->addr << 1; >> - unsigned int edid_i2c_addr = main_i2c_addr + 4; >> unsigned int val; >> int ret; >> >> @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c, >> const struct i2c_device_id *id) >> if (ret) >> goto uninit_regulators; >> >> - regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr); >> - regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, >> - main_i2c_addr - 0xa); >> - regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, >> - main_i2c_addr - 2); >> - >> adv7511_packet_disable(adv7511, 0xffff); >> >> - adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1); >> + adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid", >> + ADV7511_REG_EDID_I2C_ADDR_DEFAULT); >> if (!adv7511->i2c_edid) { >> ret = -ENOMEM; > > I wonder if this is the right error code. Maybe -EINVAL ? In most cases errors > will be caused by invalid addresses (out of memory and device_register() > failures can happen too but should be less common). It's arbitrary, as multiple error paths simply return NULL, but I think you are right - the 'most common' fault here is likely to be an in use address. -ENOMEM was chosen here to mirror the equivalent code returned from the adv7604 driver. Either way - -EINVAL does feel better at the moment. Although when mirrored back to the ADV7604 that becomes a distinct change there. I don't think that matters though - so I'll apply this fix to both patches. > > It would be nice if i2c_new_secondary_device() returned an ERR_PTR, but that's > out of scope. I was about to say - well I'll add this, but the reality is it might require updating all users of i2c_new_device, and i2c_new_dummy. Updating i2c_new_secondary_device() on it's own seems a bit pointless otherwise. And that is definitely out of scope for the amount of time I currently have :D >> goto uninit_regulators; >> } >> >> + regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, >> + adv7511->i2c_edid->addr << 1); >> + >> ret = adv7511_init_cec_regmap(adv7511); >> if (ret) >> goto err_i2c_unregister_edid; >> >> + regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, >> + adv7511->i2c_cec->addr << 1); >> + >> + adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet", >> + ADV7511_REG_PACKET_I2C_ADDR_DEFAULT); >> + if (!adv7511->i2c_packet) { >> + ret = -ENOMEM; >> + goto err_unregister_cec; >> + } >> + >> + regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, >> + adv7511->i2c_packet->addr << 1); >> + >> INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work); >> >> if (i2c->irq) { >> @@ -1181,7 +1190,7 @@ static int adv7511_probe(struct i2c_client *i2c, const >> struct i2c_device_id *id) >> IRQF_ONESHOT, dev_name(dev), >> adv7511); >> if (ret) >> - goto err_unregister_cec; >> + goto err_unregister_packet; >> } >> >> adv7511_power_off(adv7511); >> @@ -1203,6 +1212,8 @@ static int adv7511_probe(struct i2c_client *i2c, const >> struct i2c_device_id *id) >> adv7511_audio_init(dev, adv7511); >> return 0; >> >> +err_unregister_packet: >> + i2c_unregister_device(adv7511->i2c_packet); >> err_unregister_cec: >> i2c_unregister_device(adv7511->i2c_cec); >> if (adv7511->cec_clk) >> @@ -1234,6 +1245,7 @@ static int adv7511_remove(struct i2c_client *i2c) >> cec_unregister_adapter(adv7511->cec_adap); >> >> i2c_unregister_device(adv7511->i2c_edid); >> + i2c_unregister_device(adv7511->i2c_packet); >> >> return 0; >> } >
Hi Kieran, On Wednesday, 7 February 2018 17:14:09 EET Kieran Bingham wrote: > On 29/01/18 10:26, Laurent Pinchart wrote: > > On Monday, 22 January 2018 14:50:00 EET Kieran Bingham wrote: > >> The ADV7511 has four 256-byte maps that can be accessed via the main I²C > >> ports. Each map has it own I²C address and acts as a standard slave > >> device on the I²C bus. > >> > >> Allow a device tree node to override the default addresses so that > >> address conflicts with other devices on the same bus may be resolved at > >> the board description level. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> > >> .../bindings/display/bridge/adi,adv7511.txt | 10 +++++- > > > > I don't mind personally, but device tree maintainers usually ask for DT > > bindings changes to be split to a separate patch. > > > >> drivers/gpu/drm/bridge/adv7511/adv7511.h | 4 +++ > >> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 36 ++++++++++----- > >> 3 files changed, 37 insertions(+), 13 deletions(-) > >> > >> diff --git > >> a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > >> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > >> index 0047b1394c70..f6bb9f6d3f48 100644 > >> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > >> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > >> > >> @@ -70,6 +70,9 @@ Optional properties: > >> rather than generate its own timings for HDMI output. > >> > >> - clocks: from common clock binding: reference to the CEC clock. > >> - clock-names: from common clock binding: must be "cec". > >> > >> +- reg-names : Names of maps with programmable addresses. > >> + It can contain any map needing a non-default address. > >> + Possible maps names are : "main", "edid", "cec", "packet" > > > > Is the reg-names property (and the additional maps) mandatory or optional > > ? If mandatory you should also update the existing DT sources that use > > those bindings. > > They are currently optional. I do prefer it that way - but perhaps due to an > issue mentioned below we might need to make this driver mandatory ? > > > If optional you should define which I2C addresses will be used when > > the maps are not specified (and in that case I think we should go for > > the addresses listed as default in the datasheet, which correspond to the > > current driver implementation when the main address is 0x3d/0x7a). > > The current addresses do not correspond to the datasheet, even when the > implementation main address is set to 0x3d. Don't they ? The driver currently uses the following (8-bit) I2C addresses: EDID: main + 4 = 0x7e (0x3f) Packet: main - 10 = 0x70 (0x38) CEC: main - 2 = 0x78 (0x3c) Those are the default addresses according to section 4.1 of the ADV7511W programming guide (rev. B), and they match the ones you set in this patch. > Thus, in my opinion - they are currently 'wrong' - but perhaps changing them > is considered breakage too. > > A particular issue will arise here too - as on this device - when the device > is in low-power mode (after probe, before use) - the maps will respond on > their *hardware default* addresses (the ones implemented in this patch), > and thus consume that address on the I2C bus regardless of their settings > in the driver. We've discussed this previously and I share you concern. Just to make sure I remember correctly, did all the secondary maps reset to their default addresses, or just the EDID map ? > > You should also update the definition of the reg property that currently > > just states > > > > - reg: I2C slave address > > > > And finally you might want to define the term "map" in this context. > > Here's a proposal (if we make all maps mandatory). > > > > The ADV7511 internal registers are split into four pages exposed through > > different I2C addresses, creating four register maps. The I2C addresses of > > all four maps shall be specified by the reg and reg-names property. > > > > - reg: I2C slave addresses, one per reg-names entry > > - reg-names: map names, shall be "main", "edid", "cec", "packet" > > > >> Required nodes: > >> @@ -88,7 +91,12 @@ Example > >> > >> adv7511w: hdmi@39 { > >> compatible = "adi,adv7511w"; > >> - reg = <39>; > >> + /* > >> + * The EDID page will be accessible on address 0x66 on the i2c > >> + * bus. All other maps continue to use their default addresses. > >> + */ > >> + reg = <0x39 0x66>; > >> + reg-names = "main", "edid"; > >> interrupt-parent = <&gpio3>; > >> interrupts = <29 IRQ_TYPE_EDGE_FALLING>; > >> clocks = <&cec_clock>; [snip] > >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > >> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > >> index efa29db5fc2b..7ec33837752b 100644 > >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c [snip] > >> @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c, > >> const struct i2c_device_id *id) > >> if (ret) > >> goto uninit_regulators; > >> > >> - regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, > >> edid_i2c_addr); > >> - regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, > >> - main_i2c_addr - 0xa); > >> - regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, > >> - main_i2c_addr - 2); > >> - > >> adv7511_packet_disable(adv7511, 0xffff); > >> - adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1); > >> + adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid", > >> + ADV7511_REG_EDID_I2C_ADDR_DEFAULT); > >> if (!adv7511->i2c_edid) { > >> ret = -ENOMEM; > > > > I wonder if this is the right error code. Maybe -EINVAL ? In most cases > > errors will be caused by invalid addresses (out of memory and > > device_register() failures can happen too but should be less common). > > It's arbitrary, as multiple error paths simply return NULL, but I think you > are right - the 'most common' fault here is likely to be an in use address. > > -ENOMEM was chosen here to mirror the equivalent code returned from the > adv7604 driver. > > Either way - -EINVAL does feel better at the moment. Although when mirrored > back to the ADV7604 that becomes a distinct change there. > > I don't think that matters though - so I'll apply this fix to both patches. > > > It would be nice if i2c_new_secondary_device() returned an ERR_PTR, but > > that's out of scope. > > I was about to say - well I'll add this, but the reality is it might require > updating all users of i2c_new_device, and i2c_new_dummy. Updating > i2c_new_secondary_device() on it's own seems a bit pointless otherwise. > > And that is definitely out of scope for the amount of time I currently have > :D > > >> goto uninit_regulators; > >> } > >> > >> + regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, > >> + adv7511->i2c_edid->addr << 1); > >> + > >> ret = adv7511_init_cec_regmap(adv7511); > >> if (ret) > >> goto err_i2c_unregister_edid; > >> > >> + regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, > >> + adv7511->i2c_cec->addr << 1); > >> + > >> + adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet", > >> + ADV7511_REG_PACKET_I2C_ADDR_DEFAULT); > >> + if (!adv7511->i2c_packet) { > >> + ret = -ENOMEM; > >> + goto err_unregister_cec; > >> + } > >> + > >> + regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, > >> + adv7511->i2c_packet->addr << 1); > >> + > >> INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work); > >> > >> if (i2c->irq) { [snip]
Hi Laurent, On 07/02/18 21:59, Laurent Pinchart wrote: > Hi Kieran, > > On Wednesday, 7 February 2018 17:14:09 EET Kieran Bingham wrote: >> On 29/01/18 10:26, Laurent Pinchart wrote: >>> On Monday, 22 January 2018 14:50:00 EET Kieran Bingham wrote: >>>> The ADV7511 has four 256-byte maps that can be accessed via the main I²C >>>> ports. Each map has it own I²C address and acts as a standard slave >>>> device on the I²C bus. >>>> >>>> Allow a device tree node to override the default addresses so that >>>> address conflicts with other devices on the same bus may be resolved at >>>> the board description level. >>>> >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>> --- >>>> >>>> .../bindings/display/bridge/adi,adv7511.txt | 10 +++++- >>> >>> I don't mind personally, but device tree maintainers usually ask for DT >>> bindings changes to be split to a separate patch. >>> >>>> drivers/gpu/drm/bridge/adv7511/adv7511.h | 4 +++ >>>> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 36 ++++++++++----- >>>> 3 files changed, 37 insertions(+), 13 deletions(-) >>>> >>>> diff --git >>>> a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt >>>> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt >>>> index 0047b1394c70..f6bb9f6d3f48 100644 >>>> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt >>>> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt >>>> >>>> @@ -70,6 +70,9 @@ Optional properties: >>>> rather than generate its own timings for HDMI output. >>>> >>>> - clocks: from common clock binding: reference to the CEC clock. >>>> - clock-names: from common clock binding: must be "cec". >>>> >>>> +- reg-names : Names of maps with programmable addresses. >>>> + It can contain any map needing a non-default address. >>>> + Possible maps names are : "main", "edid", "cec", "packet" >>> >>> Is the reg-names property (and the additional maps) mandatory or optional >>> ? If mandatory you should also update the existing DT sources that use >>> those bindings. >> >> They are currently optional. I do prefer it that way - but perhaps due to an >> issue mentioned below we might need to make this driver mandatory ? >> >>> If optional you should define which I2C addresses will be used when >>> the maps are not specified (and in that case I think we should go for >>> the addresses listed as default in the datasheet, which correspond to the >>> current driver implementation when the main address is 0x3d/0x7a). >> >> The current addresses do not correspond to the datasheet, even when the >> implementation main address is set to 0x3d. > > Don't they ? The driver currently uses the following (8-bit) I2C addresses: > > EDID: main + 4 = 0x7e (0x3f) > Packet: main - 10 = 0x70 (0x38) > CEC: main - 2 = 0x78 (0x3c) > > Those are the default addresses according to section 4.1 of the ADV7511W > programming guide (rev. B), and they match the ones you set in this patch. Sorry - I was clearly subtracting 8bit values from a 7bit address in my failed assertion, to both you and Archit. >> Thus, in my opinion - they are currently 'wrong' - but perhaps changing them >> is considered breakage too. >> >> A particular issue will arise here too - as on this device - when the device >> is in low-power mode (after probe, before use) - the maps will respond on >> their *hardware default* addresses (the ones implemented in this patch), >> and thus consume that address on the I2C bus regardless of their settings >> in the driver. > > We've discussed this previously and I share you concern. Just to make sure I > remember correctly, did all the secondary maps reset to their default > addresses, or just the EDID map ? The following responds on the bus when programmed at alternative addresses, and in low power mode. The responses are all 0, but that's still going to conflict with other hardware if it tries to use the 'un-used' addresses. Packet (0x38), Main (0x39), Fixed (set to 0 by software, but shows up at 0x3e) and EDID (0x3f). So actually it's only the CEC which don't respond when in "low-power-mode". As far as I can see, (git grep -B3 adi,adv75) - The r8a7792-wheat.dts is the only instance of a device using 0x3d, which means that Sergei's patch changed the behaviour of all the existing devices before that. Thus - this patch could be seen to be a 'correction' back to the original behaviour for all devices except the Wheat, and possibly devices added after Sergei's patch went in. However - by my understanding, - any device which has only one ADV75(3,1)+ should use the hardware defined addresses (the hardware defaults will be conflicting on the bus anyway, thus should be assigned to the ADV7511) Any platform which uses *two* ADV7511 devices on the same bus should actually set *both* devices to use entirely separate addresses - or they will still conflict with each other. Now - if my understanding is correct - then I believe - that means that all existing devices except Wheat *should* be using the default addresses as this patch updates them to. The Wheat - has an invalid configuration - and thus should be updated anyway. Regards Kieran >>> You should also update the definition of the reg property that currently >>> just states >>> >>> - reg: I2C slave address >>> >>> And finally you might want to define the term "map" in this context. >>> Here's a proposal (if we make all maps mandatory). >>> >>> The ADV7511 internal registers are split into four pages exposed through >>> different I2C addresses, creating four register maps. The I2C addresses of >>> all four maps shall be specified by the reg and reg-names property. >>> >>> - reg: I2C slave addresses, one per reg-names entry >>> - reg-names: map names, shall be "main", "edid", "cec", "packet" >>> >>>> Required nodes: >>>> @@ -88,7 +91,12 @@ Example >>>> >>>> adv7511w: hdmi@39 { >>>> compatible = "adi,adv7511w"; >>>> - reg = <39>; >>>> + /* >>>> + * The EDID page will be accessible on address 0x66 on the i2c >>>> + * bus. All other maps continue to use their default addresses. >>>> + */ >>>> + reg = <0x39 0x66>; >>>> + reg-names = "main", "edid"; >>>> interrupt-parent = <&gpio3>; >>>> interrupts = <29 IRQ_TYPE_EDGE_FALLING>; >>>> clocks = <&cec_clock>; > > [snip] > >>>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>>> index efa29db5fc2b..7ec33837752b 100644 >>>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >>>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > [snip] > >>>> @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c, >>>> const struct i2c_device_id *id) >>>> if (ret) >>>> goto uninit_regulators; >>>> >>>> - regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, >>>> edid_i2c_addr); >>>> - regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, >>>> - main_i2c_addr - 0xa); >>>> - regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, >>>> - main_i2c_addr - 2); >>>> - >>>> adv7511_packet_disable(adv7511, 0xffff); >>>> - adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1); >>>> + adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid", >>>> + ADV7511_REG_EDID_I2C_ADDR_DEFAULT); >>>> if (!adv7511->i2c_edid) { >>>> ret = -ENOMEM; >>> >>> I wonder if this is the right error code. Maybe -EINVAL ? In most cases >>> errors will be caused by invalid addresses (out of memory and >>> device_register() failures can happen too but should be less common). >> >> It's arbitrary, as multiple error paths simply return NULL, but I think you >> are right - the 'most common' fault here is likely to be an in use address. >> >> -ENOMEM was chosen here to mirror the equivalent code returned from the >> adv7604 driver. >> >> Either way - -EINVAL does feel better at the moment. Although when mirrored >> back to the ADV7604 that becomes a distinct change there. >> >> I don't think that matters though - so I'll apply this fix to both patches. >> >>> It would be nice if i2c_new_secondary_device() returned an ERR_PTR, but >>> that's out of scope. >> >> I was about to say - well I'll add this, but the reality is it might require >> updating all users of i2c_new_device, and i2c_new_dummy. Updating >> i2c_new_secondary_device() on it's own seems a bit pointless otherwise. >> >> And that is definitely out of scope for the amount of time I currently have >> :D >> >>>> goto uninit_regulators; >>>> } >>>> >>>> + regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, >>>> + adv7511->i2c_edid->addr << 1); >>>> + >>>> ret = adv7511_init_cec_regmap(adv7511); >>>> if (ret) >>>> goto err_i2c_unregister_edid; >>>> >>>> + regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, >>>> + adv7511->i2c_cec->addr << 1); >>>> + >>>> + adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet", >>>> + ADV7511_REG_PACKET_I2C_ADDR_DEFAULT); >>>> + if (!adv7511->i2c_packet) { >>>> + ret = -ENOMEM; >>>> + goto err_unregister_cec; >>>> + } >>>> + >>>> + regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, >>>> + adv7511->i2c_packet->addr << 1); >>>> + >>>> INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work); >>>> >>>> if (i2c->irq) { > > [snip] >
Hi Archit, On 07/02/18 12:33, Kieran Bingham wrote: > Hi Archit, > > Thank you for your review, > <snip> >>> unsigned int val; >>> int ret; >>> @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c, >>> const struct i2c_device_id *id) >>> if (ret) >>> goto uninit_regulators; >>> - regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr); >>> - regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, >>> - main_i2c_addr - 0xa); > > > Packet address here is written as an offset of -0x0a, which gives 0x2f or 0x33 ... ? > > I think these current offsets are platform specific to a specific platform :) I appear to be using platform specific maths specific to a non-conforming platform or universe :-) Sorry for the incorrect assertion here. - I mixed up 8bit and 7bit values. The offsets are valid (when calculated correctly) - however - as per my reply to Laurent - I believe the patch which determined the offsets was using the wrong base :). Also - due to a hardware issue on the ADV7511 - the Wheat (as the only known user of two chips on the same bus) will need to be updated to ensure the current assignments don't conflict. -- Kieran
Hi Kieran, On Thursday, 8 February 2018 01:30:43 EET Kieran Bingham wrote: > On 07/02/18 21:59, Laurent Pinchart wrote: > > On Wednesday, 7 February 2018 17:14:09 EET Kieran Bingham wrote: > >> On 29/01/18 10:26, Laurent Pinchart wrote: > >>> On Monday, 22 January 2018 14:50:00 EET Kieran Bingham wrote: > >>>> The ADV7511 has four 256-byte maps that can be accessed via the main > >>>> I²C ports. Each map has it own I²C address and acts as a standard slave > >>>> device on the I²C bus. > >>>> > >>>> Allow a device tree node to override the default addresses so that > >>>> address conflicts with other devices on the same bus may be resolved at > >>>> the board description level. > >>>> > >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>>> --- > >>>> > >>>> .../bindings/display/bridge/adi,adv7511.txt | 10 +++++- > >>> > >>> I don't mind personally, but device tree maintainers usually ask for DT > >>> bindings changes to be split to a separate patch. > >>> > >>>> drivers/gpu/drm/bridge/adv7511/adv7511.h | 4 +++ > >>>> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 36++++++++++----- > >>>> 3 files changed, 37 insertions(+), 13 deletions(-) > >>>> > >>>> diff --git > >>>> a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > >>>> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > >>>> index 0047b1394c70..f6bb9f6d3f48 100644 > >>>> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > >>>> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt > >>>> > >>>> @@ -70,6 +70,9 @@ Optional properties: > >>>> rather than generate its own timings for HDMI output. > >>>> - clocks: from common clock binding: reference to the CEC clock. > >>>> - clock-names: from common clock binding: must be "cec". > >>>> +- reg-names : Names of maps with programmable addresses. > >>>> + It can contain any map needing a non-default address. > >>>> + Possible maps names are : "main", "edid", "cec", "packet" > >>> > >>> Is the reg-names property (and the additional maps) mandatory or > >>> optional ? If mandatory you should also update the existing DT sources > >>> that use those bindings. > >> > >> They are currently optional. I do prefer it that way - but perhaps due to > >> an issue mentioned below we might need to make this driver mandatory ?>> > >> > >>> If optional you should define which I2C addresses will be used when > >>> the maps are not specified (and in that case I think we should go for > >>> the addresses listed as default in the datasheet, which correspond to > >>> the current driver implementation when the main address is 0x3d/0x7a). > >> > >> The current addresses do not correspond to the datasheet, even when the > >> implementation main address is set to 0x3d. > > > > Don't they ? The driver currently uses the following (8-bit) I2C > > addresses: > > > > EDID: main + 4 = 0x7e (0x3f) > > Packet: main - 10 = 0x70 (0x38) > > CEC: main - 2 = 0x78 (0x3c) > > > > Those are the default addresses according to section 4.1 of the ADV7511W > > programming guide (rev. B), and they match the ones you set in this patch. > > Sorry - I was clearly subtracting 8bit values from a 7bit address in my > failed assertion, to both you and Archit. No worries. > >> Thus, in my opinion - they are currently 'wrong' - but perhaps changing > >> them is considered breakage too. > >> > >> A particular issue will arise here too - as on this device - when the > >> device is in low-power mode (after probe, before use) - the maps will > >> respond on their *hardware default* addresses (the ones implemented in > >> this patch), and thus consume that address on the I2C bus regardless of > >> their settings in the driver. > > > > We've discussed this previously and I share you concern. Just to make sure > > I remember correctly, did all the secondary maps reset to their default > > addresses, or just the EDID map ? > > The following responds on the bus when programmed at alternative addresses, > and in low power mode. The responses are all 0, but that's still going to > conflict with other hardware if it tries to use the 'un-used' addresses. > > Packet (0x38), > Main (0x39), > Fixed (set to 0 by software, but shows up at 0x3e) > and EDID (0x3f). > > So actually it's only the CEC which don't respond when in "low-power-mode". > > As far as I can see, (git grep -B3 adi,adv75) - The r8a7792-wheat.dts is > the only instance of a device using 0x3d, which means that Sergei's patch > changed the behaviour of all the existing devices before that. > > Thus - this patch could be seen to be a 'correction' back to the original > behaviour for all devices except the Wheat, and possibly devices added after > Sergei's patch went in. > > However - by my understanding, - any device which has only one ADV75(3,1)+ > should use the hardware defined addresses (the hardware defaults will be > conflicting on the bus anyway, thus should be assigned to the ADV7511) > > Any platform which uses *two* ADV7511 devices on the same bus should > actually set *both* devices to use entirely separate addresses - or they > will still conflict with each other. Agreed. No access should be made to the default addresses for the secondary I2C clients, otherwise there's a risk of conflict. When only one ADV7511 is present, but conflicts with another device, we could reprogram the other device only (assuming it doesn't lose its configuration in low-power mode), or reprogram both. > Now - if my understanding is correct - then I believe - that means that all > existing devices except Wheat *should* be using the default addresses as > this patch updates them to. > > The Wheat - has an invalid configuration - and thus should be updated > anyway. I agree. > >>> You should also update the definition of the reg property that currently > >>> just states > >>> > >>> - reg: I2C slave address > >>> > >>> And finally you might want to define the term "map" in this context. > >>> Here's a proposal (if we make all maps mandatory). > >>> > >>> The ADV7511 internal registers are split into four pages exposed through > >>> different I2C addresses, creating four register maps. The I2C addresses > >>> of all four maps shall be specified by the reg and reg-names property. > >>> > >>> - reg: I2C slave addresses, one per reg-names entry > >>> - reg-names: map names, shall be "main", "edid", "cec", "packet" > >>> > >>>> Required nodes: > >>>> @@ -88,7 +91,12 @@ Example > >>>> > >>>> adv7511w: hdmi@39 { > >>>> compatible = "adi,adv7511w"; > >>>> > >>>> - reg = <39>; > >>>> + /* > >>>> + * The EDID page will be accessible on address 0x66 on the i2c > >>>> + * bus. All other maps continue to use their default addresses. > >>>> + */ > >>>> + reg = <0x39 0x66>; > >>>> + reg-names = "main", "edid"; > >>>> interrupt-parent = <&gpio3>; > >>>> interrupts = <29 IRQ_TYPE_EDGE_FALLING>; > >>>> clocks = <&cec_clock>; [snip]
diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt index 0047b1394c70..f6bb9f6d3f48 100644 --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt @@ -70,6 +70,9 @@ Optional properties: rather than generate its own timings for HDMI output. - clocks: from common clock binding: reference to the CEC clock. - clock-names: from common clock binding: must be "cec". +- reg-names : Names of maps with programmable addresses. + It can contain any map needing a non-default address. + Possible maps names are : "main", "edid", "cec", "packet" Required nodes: @@ -88,7 +91,12 @@ Example adv7511w: hdmi@39 { compatible = "adi,adv7511w"; - reg = <39>; + /* + * The EDID page will be accessible on address 0x66 on the i2c + * bus. All other maps continue to use their default addresses. + */ + reg = <0x39 0x66>; + reg-names = "main", "edid"; interrupt-parent = <&gpio3>; interrupts = <29 IRQ_TYPE_EDGE_FALLING>; clocks = <&cec_clock>; diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h index d034b2cb5eee..7d81ce3808e0 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h @@ -53,8 +53,10 @@ #define ADV7511_REG_POWER 0x41 #define ADV7511_REG_STATUS 0x42 #define ADV7511_REG_EDID_I2C_ADDR 0x43 +#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT 0x3f #define ADV7511_REG_PACKET_ENABLE1 0x44 #define ADV7511_REG_PACKET_I2C_ADDR 0x45 +#define ADV7511_REG_PACKET_I2C_ADDR_DEFAULT 0x38 #define ADV7511_REG_DSD_ENABLE 0x46 #define ADV7511_REG_VIDEO_INPUT_CFG2 0x48 #define ADV7511_REG_INFOFRAME_UPDATE 0x4a @@ -89,6 +91,7 @@ #define ADV7511_REG_TMDS_CLOCK_INV 0xde #define ADV7511_REG_ARC_CTRL 0xdf #define ADV7511_REG_CEC_I2C_ADDR 0xe1 +#define ADV7511_REG_CEC_I2C_ADDR_DEFAULT 0x3c #define ADV7511_REG_CEC_CTRL 0xe2 #define ADV7511_REG_CHIP_ID_HIGH 0xf5 #define ADV7511_REG_CHIP_ID_LOW 0xf6 @@ -322,6 +325,7 @@ struct adv7511 { struct i2c_client *i2c_main; struct i2c_client *i2c_edid; struct i2c_client *i2c_cec; + struct i2c_client *i2c_packet; struct regmap *regmap; struct regmap *regmap_cec; diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index efa29db5fc2b..7ec33837752b 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -969,8 +969,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv) { int ret; - adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter, - adv->i2c_main->addr - 1); + adv->i2c_cec = i2c_new_secondary_device(adv->i2c_main, "cec", + ADV7511_REG_CEC_I2C_ADDR_DEFAULT); if (!adv->i2c_cec) return -ENOMEM; i2c_set_clientdata(adv->i2c_cec, adv); @@ -1082,8 +1082,6 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) struct adv7511_link_config link_config; struct adv7511 *adv7511; struct device *dev = &i2c->dev; - unsigned int main_i2c_addr = i2c->addr << 1; - unsigned int edid_i2c_addr = main_i2c_addr + 4; unsigned int val; int ret; @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) if (ret) goto uninit_regulators; - regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr); - regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, - main_i2c_addr - 0xa); - regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, - main_i2c_addr - 2); - adv7511_packet_disable(adv7511, 0xffff); - adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1); + adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid", + ADV7511_REG_EDID_I2C_ADDR_DEFAULT); if (!adv7511->i2c_edid) { ret = -ENOMEM; goto uninit_regulators; } + regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, + adv7511->i2c_edid->addr << 1); + ret = adv7511_init_cec_regmap(adv7511); if (ret) goto err_i2c_unregister_edid; + regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, + adv7511->i2c_cec->addr << 1); + + adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet", + ADV7511_REG_PACKET_I2C_ADDR_DEFAULT); + if (!adv7511->i2c_packet) { + ret = -ENOMEM; + goto err_unregister_cec; + } + + regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, + adv7511->i2c_packet->addr << 1); + INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work); if (i2c->irq) { @@ -1181,7 +1190,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) IRQF_ONESHOT, dev_name(dev), adv7511); if (ret) - goto err_unregister_cec; + goto err_unregister_packet; } adv7511_power_off(adv7511); @@ -1203,6 +1212,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) adv7511_audio_init(dev, adv7511); return 0; +err_unregister_packet: + i2c_unregister_device(adv7511->i2c_packet); err_unregister_cec: i2c_unregister_device(adv7511->i2c_cec); if (adv7511->cec_clk) @@ -1234,6 +1245,7 @@ static int adv7511_remove(struct i2c_client *i2c) cec_unregister_adapter(adv7511->cec_adap); i2c_unregister_device(adv7511->i2c_edid); + i2c_unregister_device(adv7511->i2c_packet); return 0; }
The ADV7511 has four 256-byte maps that can be accessed via the main I²C ports. Each map has it own I²C address and acts as a standard slave device on the I²C bus. Allow a device tree node to override the default addresses so that address conflicts with other devices on the same bus may be resolved at the board description level. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- .../bindings/display/bridge/adi,adv7511.txt | 10 +++++- drivers/gpu/drm/bridge/adv7511/adv7511.h | 4 +++ drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 36 ++++++++++++++-------- 3 files changed, 37 insertions(+), 13 deletions(-) -- 2.7.4