Re: [PATCH v3 1/5] dt-bindings: media: adv7604: Add support for i2c_new_secondary_device

[Laurent Pinchart]

Hi Kieran,

On Tuesday, 13 February 2018 15:14:43 EET Kieran Bingham wrote:
> On 13/02/18 12:06, Laurent Pinchart wrote:
> > On Tuesday, 13 February 2018 00:07:49 EET Kieran Bingham wrote:
> >> From: Jean-Michel Hautbois 
> >> 
> >> The ADV7604 has thirteen 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.
> >> 
> >> Extend the device tree node bindings to be able 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: Jean-Michel Hautbois 
> >> [Kieran: Re-adapted for mainline]
> >> Signed-off-by: Kieran Bingham 
> >> Reviewed-by: Rob Herring 
> > 
> > Nitpicking, I might not mention i2c_new_secondary_device in the subject,
> > as this is a DT bindings change. I don't mind too much though, as long as
> > the bindings themselves don't contain Linux-specific information, and they
> > don't, so
> 
> How about: ... adv7604: Extend bindings to allow specifying slave map
> addresses

Sounds good to me.

> > Reviewed-by: Laurent Pinchart 
> 
> Collected, thanks.
> 
> --
> Kieran
> 
> >> ---
> >> 
> >> Based upon the original posting :
> >>   https://lkml.org/lkml/2014/10/22/469
> >> 
> >> v2:
> >>  - DT Binding update separated from code change
> >>  - Minor reword to commit message to account for DT only change.
> >>  - Collected Rob's RB tag.
> >> 
> >> v3:
> >>  - Split map register addresses into individual declarations.
> >>  
> >>  .../devicetree/bindings/media/i2c/adv7604.txt  | 18
> >> 
> >> -- 1 file changed, 16 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> >> b/Documentation/devicetree/bindings/media/i2c/adv7604.txt index
> >> 9cbd92eb5d05..ebb5f070c05b 100644
> >> --- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> >> +++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> >> 
> >> @@ -13,7 +13,11 @@ Required Properties:
> >>  - "adi,adv7611" for the ADV7611
> >>  - "adi,adv7612" for the ADV7612
> >> 
> >> -  - reg: I2C slave address
> >> +  - reg: I2C slave addresses
> >> +The ADV76xx has up to thirteen 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. The main address is mandatory,
> >> others are +optional and revert to defaults if not specified.
> >> 
> >>- hpd-gpios: References to the GPIOs that control the HDMI hot-plug
> >>
> >>  detection pins, one per HDMI input. The active flag indicates the
> >>  GPIO
> >> 
> >> @@ -35,6 +39,11 @@ Optional Properties:
> >>- reset-gpios: Reference to the GPIO connected to the device's reset
> >>pin.
> >> 
> >> - default-input: Select which input is selected after reset.
> >> +  - reg-names : Names of maps with programmable addresses.
> >> +  It can contain any map needing a non-default address.
> >> +  Possible maps names are :
> >> +"main", "avlink", "cec", "infoframe", "esdp", "dpp", "afe",
> >> +"rep", "edid", "hdmi", "test", "cp", "vdp"
> >> 
> >>  Optional Endpoint Properties:
> >> @@ -52,7 +61,12 @@ Example:
> >>hdmi_receiver@4c {
> >>
> >>compatible = "adi,adv7611";
> >> 
> >> -  reg = <0x4c>;
> >> +  /*
> >> +   * The edid page will be accessible @ 0x66 on the i2c bus. All
> >> +   * other maps will retain their default addresses.
> >> +   */
> >> +  reg = <0x4c>, <0x66>;
> >> +  reg-names "main", "edid";
> >> 
> >>reset-gpios = < 0 GPIO_ACTIVE_LOW>;
> >>hpd-gpios = < 2 GPIO_ACTIVE_HIGH>;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/5] dt-bindings: media: adv7604: Add support for i2c_new_secondary_device

[Kieran Bingham]

Hi Laurent,

On 13/02/18 12:06, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.

Thank you for your review,

> On Tuesday, 13 February 2018 00:07:49 EET Kieran Bingham wrote:
>> From: Jean-Michel Hautbois 
>>
>> The ADV7604 has thirteen 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.
>>
>> Extend the device tree node bindings to be able 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: Jean-Michel Hautbois 
>> [Kieran: Re-adapted for mainline]
>> Signed-off-by: Kieran Bingham 
>> Reviewed-by: Rob Herring 
> 
> Nitpicking, I might not mention i2c_new_secondary_device in the subject, as 
> this is a DT bindings change. I don't mind too much though, as long as the 
> bindings themselves don't contain Linux-specific information, and they don't, 
> so
How about: ... adv7604: Extend bindings to allow specifying slave map addresses



> Reviewed-by: Laurent Pinchart 

Collected, thanks.

--
Kieran


> 
>> ---
>> Based upon the original posting :
>>   https://lkml.org/lkml/2014/10/22/469
>>
>> v2:
>>  - DT Binding update separated from code change
>>  - Minor reword to commit message to account for DT only change.
>>  - Collected Rob's RB tag.
>>
>> v3:
>>  - Split map register addresses into individual declarations.
>>
>>  .../devicetree/bindings/media/i2c/adv7604.txt  | 18
>> -- 1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
>> b/Documentation/devicetree/bindings/media/i2c/adv7604.txt index
>> 9cbd92eb5d05..ebb5f070c05b 100644
>> --- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
>> +++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
>> @@ -13,7 +13,11 @@ Required Properties:
>>  - "adi,adv7611" for the ADV7611
>>  - "adi,adv7612" for the ADV7612
>>
>> -  - reg: I2C slave address
>> +  - reg: I2C slave addresses
>> +The ADV76xx has up to thirteen 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. The main address is mandatory,
>> others are +optional and revert to defaults if not specified.
>>
>>- hpd-gpios: References to the GPIOs that control the HDMI hot-plug
>>  detection pins, one per HDMI input. The active flag indicates the GPIO
>> @@ -35,6 +39,11 @@ Optional Properties:
>>
>>- reset-gpios: Reference to the GPIO connected to the device's reset pin.
>> - default-input: Select which input is selected after reset.
>> +  - reg-names : Names of maps with programmable addresses.
>> +It can contain any map needing a non-default address.
>> +Possible maps names are :
>> +  "main", "avlink", "cec", "infoframe", "esdp", "dpp", "afe",
>> +  "rep", "edid", "hdmi", "test", "cp", "vdp"
>>
>>  Optional Endpoint Properties:
>>
>> @@ -52,7 +61,12 @@ Example:
>>
>>  hdmi_receiver@4c {
>>  compatible = "adi,adv7611";
>> -reg = <0x4c>;
>> +/*
>> + * The edid page will be accessible @ 0x66 on the i2c bus. All
>> + * other maps will retain their default addresses.
>> + */
>> +reg = <0x4c>, <0x66>;
>> +reg-names "main", "edid";
>>
>>  reset-gpios = < 0 GPIO_ACTIVE_LOW>;
>>  hpd-gpios = < 2 GPIO_ACTIVE_HIGH>;
> 
> 

Re: [PATCH v3 1/5] dt-bindings: media: adv7604: Add support for i2c_new_secondary_device

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Tuesday, 13 February 2018 00:07:49 EET Kieran Bingham wrote:
> From: Jean-Michel Hautbois 
> 
> The ADV7604 has thirteen 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.
> 
> Extend the device tree node bindings to be able 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: Jean-Michel Hautbois 
> [Kieran: Re-adapted for mainline]
> Signed-off-by: Kieran Bingham 
> Reviewed-by: Rob Herring 

Nitpicking, I might not mention i2c_new_secondary_device in the subject, as 
this is a DT bindings change. I don't mind too much though, as long as the 
bindings themselves don't contain Linux-specific information, and they don't, 
so

Reviewed-by: Laurent Pinchart 

> ---
> Based upon the original posting :
>   https://lkml.org/lkml/2014/10/22/469
> 
> v2:
>  - DT Binding update separated from code change
>  - Minor reword to commit message to account for DT only change.
>  - Collected Rob's RB tag.
> 
> v3:
>  - Split map register addresses into individual declarations.
> 
>  .../devicetree/bindings/media/i2c/adv7604.txt  | 18
> -- 1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> b/Documentation/devicetree/bindings/media/i2c/adv7604.txt index
> 9cbd92eb5d05..ebb5f070c05b 100644
> --- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> @@ -13,7 +13,11 @@ Required Properties:
>  - "adi,adv7611" for the ADV7611
>  - "adi,adv7612" for the ADV7612
> 
> -  - reg: I2C slave address
> +  - reg: I2C slave addresses
> +The ADV76xx has up to thirteen 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. The main address is mandatory,
> others are +optional and revert to defaults if not specified.
> 
>- hpd-gpios: References to the GPIOs that control the HDMI hot-plug
>  detection pins, one per HDMI input. The active flag indicates the GPIO
> @@ -35,6 +39,11 @@ Optional Properties:
> 
>- reset-gpios: Reference to the GPIO connected to the device's reset pin.
> - default-input: Select which input is selected after reset.
> +  - reg-names : Names of maps with programmable addresses.
> + It can contain any map needing a non-default address.
> + Possible maps names are :
> +   "main", "avlink", "cec", "infoframe", "esdp", "dpp", "afe",
> +   "rep", "edid", "hdmi", "test", "cp", "vdp"
> 
>  Optional Endpoint Properties:
> 
> @@ -52,7 +61,12 @@ Example:
> 
>   hdmi_receiver@4c {
>   compatible = "adi,adv7611";
> - reg = <0x4c>;
> + /*
> +  * The edid page will be accessible @ 0x66 on the i2c bus. All
> +  * other maps will retain their default addresses.
> +  */
> + reg = <0x4c>, <0x66>;
> + reg-names "main", "edid";
> 
>   reset-gpios = < 0 GPIO_ACTIVE_LOW>;
>   hpd-gpios = < 2 GPIO_ACTIVE_HIGH>;


-- 
Regards,

Laurent Pinchart

[PATCH v3 1/5] dt-bindings: media: adv7604: Add support for i2c_new_secondary_device

[Kieran Bingham]

From: Jean-Michel Hautbois 

The ADV7604 has thirteen 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.

Extend the device tree node bindings to be able 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: Jean-Michel Hautbois 
[Kieran: Re-adapted for mainline]
Signed-off-by: Kieran Bingham 
Reviewed-by: Rob Herring 

---
Based upon the original posting :
  https://lkml.org/lkml/2014/10/22/469

v2:
 - DT Binding update separated from code change
 - Minor reword to commit message to account for DT only change.
 - Collected Rob's RB tag.

v3:
 - Split map register addresses into individual declarations.

 .../devicetree/bindings/media/i2c/adv7604.txt  | 18 --
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt 
b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
index 9cbd92eb5d05..ebb5f070c05b 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
@@ -13,7 +13,11 @@ Required Properties:
 - "adi,adv7611" for the ADV7611
 - "adi,adv7612" for the ADV7612
 
-  - reg: I2C slave address
+  - reg: I2C slave addresses
+The ADV76xx has up to thirteen 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. The main address is mandatory, others are
+optional and revert to defaults if not specified.
 
   - hpd-gpios: References to the GPIOs that control the HDMI hot-plug
 detection pins, one per HDMI input. The active flag indicates the GPIO
@@ -35,6 +39,11 @@ Optional Properties:
 
   - reset-gpios: Reference to the GPIO connected to the device's reset pin.
   - default-input: Select which input is selected after reset.
+  - reg-names : Names of maps with programmable addresses.
+   It can contain any map needing a non-default address.
+   Possible maps names are :
+ "main", "avlink", "cec", "infoframe", "esdp", "dpp", "afe",
+ "rep", "edid", "hdmi", "test", "cp", "vdp"
 
 Optional Endpoint Properties:
 
@@ -52,7 +61,12 @@ Example:
 
hdmi_receiver@4c {
compatible = "adi,adv7611";
-   reg = <0x4c>;
+   /*
+* The edid page will be accessible @ 0x66 on the i2c bus. All
+* other maps will retain their default addresses.
+*/
+   reg = <0x4c>, <0x66>;
+   reg-names "main", "edid";
 
reset-gpios = < 0 GPIO_ACTIVE_LOW>;
hpd-gpios = < 2 GPIO_ACTIVE_HIGH>;
-- 
2.7.4