Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

2018-02-08 Thread Laurent Pinchart 
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 
>  ---
>  
>   .../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

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

2018-02-07 Thread Kieran Bingham 
Hi Archit,

On 07/02/18 12:33, Kieran Bingham wrote:
> Hi Archit,
> 
> Thank you for your review,
> 



>>>   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

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

2018-02-07 Thread Kieran Bingham 
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 
 ---

  .../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

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

2018-02-07 Thread Laurent Pinchart 
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 
> >> ---
> >> 
> >>  .../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 = <>;
> >>interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
> >>clocks = <_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
> >> +++

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

2018-02-07 Thread Kieran Bingham 
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 
>> ---
>>  .../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 = <>;
>>  interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
>>  clocks = <_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_CFG20x48
>>  #define ADV7511_REG_INFOFRAME_UPDATE0x4a
>> @@ -89,6 +91,7 @@
>>  #define ADV7511_REG_TMDS_CLOCK_INV

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

2018-02-07 Thread Kieran Bingham 
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 
>> ---
>>   .../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 = <>;
>>   interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
>>   clocks = <_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 ...





>> +    adv->i2c_cec =

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

2018-01-29 Thread Rob Herring 
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 
> > ---
> >  .../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(-)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

2018-01-29 Thread Rob Herring 
On Mon, Jan 22, 2018 at 12:50:01PM +, 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 
> ---
>  .../bindings/display/bridge/adi,adv7511.txt| 10 +-

Reviewed-by: Rob Herring 

>  drivers/gpu/drm/bridge/adv7511/adv7511.h   |  4 +++
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c   | 36 
> ++
>  3 files changed, 37 insertions(+), 13 deletions(-)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

2018-01-29 Thread Laurent Pinchart 
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 
> ---
>  .../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 = <>;
>   interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
>   clocks = <_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_POWER0x41
>  #define ADV7511_REG_STATUS   0x42
>  #define ADV7511_REG_EDID_I2C_ADDR0x43
> +#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT0x3f
>  #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

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

2018-01-28 Thread Archit Taneja 

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 
---
  .../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 = <>;
interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
clocks = <_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_STATUS0x42
  #define ADV7511_REG_EDID_I2C_ADDR 0x43
+#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT  0x3f
  #define ADV7511_REG_PACKET_ENABLE10x44
  #define ADV7511_REG_PACKET_I2C_ADDR   0x45
+#define ADV7511_REG_PACKET_I2C_ADDR_DEFAULT0x38
  #define ADV7511_REG_DSD_ENABLE0x46
  #define ADV7511_REG_VIDEO_INPUT_CFG2  0x48
  #define ADV7511_REG_INFOFRAME_UPDATE  0x4a
@@ -89,6 +91,7 @@
  #define ADV7511_REG_TMDS_CLOCK_INV0xde
  #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)

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

2018-01-22 Thread Kieran Bingham 
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 
> 
> 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 = <>;
>>  interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
>>  clocks = <_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_CFG20x48
>>  #define ADV7511_REG_INFOFRAME_UPDATE0x4a
>> @@ -89,6 +91,7 @@
>>  #define ADV7511_REG_TMDS_CLOCK_INV  0xde
>>  #define ADV7511_REG_ARC_CTRL0xdf
>>  #define ADV7511_REG_CEC_I2C_ADDR0xe1
>> +#define ADV7511_REG_CEC_I2C_ADDR_DEFAULT0x3c
>>  #define ADV7511_REG_CEC_CTRL0xe2
>>  #define ADV7511_REG_CHIP_ID_HIGH0xf5
>>  #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 = >dev;
>> -unsigned int

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

2018-01-22 Thread Lars-Peter Clausen 
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 

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 = <>;
>   interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
>   clocks = <_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_POWER0x41
>  #define ADV7511_REG_STATUS   0x42
>  #define ADV7511_REG_EDID_I2C_ADDR0x43
> +#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT0x3f
>  #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 = >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