Re: [PATCH 2/2] adv7604: Use DT parsing in dummy creation

[Mark Rutland <mark.rutland@arm.com>]

Hi,

On Fri, Aug 29, 2014 at 02:28:17PM +0100, Jean-Michel Hautbois wrote:
> This patch uses DT in order to parse addresses for dummy devices of adv7604.
> If nothing is defined, it uses default addresses.
> The main prupose is using two adv76xx on the same i2c bus.

This is rather opaque.

It seems from the code below that a single adv7611 device has multiple
I2C addresses at which different registers may be accessed. I guess the
secondary instances of the unit have different addresses for all of the
pages?

> Signed-off-by: Jean-Michel Hautbois <jean-michel.hautbois@vodalys.com>
> ---
>  .../devicetree/bindings/media/i2c/adv7604.txt      |  7 ++-
>  drivers/media/i2c/adv7604.c                        | 56 ++++++++++++++--------
>  2 files changed, 42 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> index c27cede..221b75c 100644
> --- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> @@ -10,6 +10,7 @@ Required Properties:
>  
>    - compatible: Must contain one of the following
>      - "adi,adv7611" for the ADV7611
> +    - "adi,adv7604" for the ADV7604
>  
>    - reg: I2C slave address

This should be updated, at least to say "address(es)".

>  
> @@ -32,6 +33,8 @@ The digital output port node must contain at least one endpoint.
>  Optional Properties:
>  
>    - reset-gpios: Reference to the GPIO connected to the device's reset pin.
> +  - reg-names : Names of registers to be reprogrammed.

This doesn't describe _which_ names you expect, and I have no idea what
is meant by "to be reprogrammed".

> +		Refer to source code for possible values.

Bindings shouldn't say things like this. The binding should describe the
contract between the DTB and the OS, which this clearly doesn't.

A binding document shouldn't necessitate reading code.

>  Optional Endpoint Properties:
>  
> @@ -50,7 +53,9 @@ Example:
>  
>  	hdmi_receiver@4c {
>  		compatible = "adi,adv7611";
> -		reg = <0x4c>;
> +		/* edid page will be accessible @ 0x66 on i2c bus*/
> +		reg = <0x4c 0x66>;
> +		reg-names = "main", "edid";

What about the other IDs? Are they accessible or not?

Why didn't we always list the full set of IDs in the first place? That
would have made this far less painful.

Thanks,
Mark.