Re: [PATCH v2 46/48] adv7604: Add DT support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sylwester,

Thank you for the review.

On Thursday 17 April 2014 12:59:22 Sylwester Nawrocki wrote:
> On 11/03/14 00:15, Laurent Pinchart wrote:
> > Parse the device tree node to populate platform data.
> > 
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  .../devicetree/bindings/media/i2c/adv7604.txt      | 56 +++++++++++++
> >  drivers/media/i2c/adv7604.c                        | 92 +++++++++++++----
> >  2 files changed, 134 insertions(+), 14 deletions(-)
> >  create mode 100644
> >  Documentation/devicetree/bindings/media/i2c/adv7604.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> > b/Documentation/devicetree/bindings/media/i2c/adv7604.txt new file mode
> > 100644
> > index 0000000..0845c50
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> > @@ -0,0 +1,56 @@
> > +* Analog Devices ADV7604/11 video decoder with HDMI receiver
> > +
> > +The ADV7604 and ADV7611 are multiformat video decoders with an integrated
> > HDMI +receiver. The ADV7604 has four multiplexed HDMI inputs and one
> > analog input, +and the ADV7611 has one HDMI input and no analog input.
> > +
> > +Required Properties:
> > +
> > +  - compatible: Must contain one of the following
> > +    - "adi,adv7604" for the ADV7604
> > +    - "adi,adv7611" for the ADV7611
> > +
> > +  - reg: I2C slave address
> > +
> > +  - hpd-gpios: References to the GPIOs that control the HDMI hot-plug
> > +    detection pins, one per HDMI input. The active flag indicates the
> > GPIO
> > +    level that enables hot-plug detection.
> > +
> > +Optional Properties:
> > +
> > +  - reset-gpios: Reference to the GPIO connected to the device's reset
> > pin. +
> > +  - adi,default-input: Index of the input to be configured as default.
> > Valid
> > +    values are 0..5 for the ADV7604 and 0 for the ADV7611.
> 
> I have some doubts about this property. Firstly, it seems it is not needed
> for ADV7611 since it is always 0 for that device ?
> Why can't we hard code in the driver some default input ?

I've thought about hardcoding a default input in the driver as well, but Hans 
wasn't really keen on the idea. Hans, could you please comment on this ?

> And which inputs it refers to ? HDMI inputs A..D + analog ? If we keep this
> property I think exact mapping of numbers to inputs should be included
> in description of this property.
> 
> > +  - adi,disable-power-down: Boolean property. When set forces the device
> > to
> > +    ignore the power-down pin. The property is valid for the ADV7604 only
> > as
> > +    the ADV7611 has no power-down pin.
> 
> Does it refer to the !PWRDWN pin ? If so I would replace "power-down" with
> PWRDWN, so it is clear what we're talking about when someone looks only
> at the datasheet.
>
> > +  - adi,disable-cable-reset: Boolean property. When set disables the HDMI
> > +    receiver automatic reset when the HDMI cable is unplugged.
> 
> Couldn't this be configured from user space with some default assumed in the
> driver ?

Good question. I'm not sure what the exact use case for this is.

Let's be careful not to introduce unneeded properties, I'll drop those two 
properties for now, we can implement support for the features later when 
needed.

> > +Example:
> > +
> > +	hdmi_receiver@4c {
> > +		compatible = "adi,adv7611";
> > +		reg = <0x4c>;
> > +
> > +		reset-gpios = <&ioexp 0 GPIO_ACTIVE_LOW>;
> > +		hpd-gpios = <&ioexp 2 GPIO_ACTIVE_HIGH>;
> > +
> > +		adi,default-input = <0>;
> > +
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		port@0 {
> > +			reg = <0>;
> > +		};
> > +		port@1 {
> > +			reg = <1>;
> > +			hdmi_in: endpoint {
> > +				remote-endpoint = <&ccdc_in>;
> > +			};
> > +		};
> > +	};
> > diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> > index cce140c..de44213 100644
> > --- a/drivers/media/i2c/adv7604.c
> > +++ b/drivers/media/i2c/adv7604.c

[snip]

> > @@ -2836,21 +2906,15 @@ static int adv7604_remove(struct i2c_client
> > *client)> 
> >  /* ------------------------------------------------------------------- */
> > -static struct i2c_device_id adv7604_id[] = {
> > -	{ "adv7604", ADV7604 },
> > -	{ "adv7611", ADV7611 },
> > -	{ }
> > -};
> > -MODULE_DEVICE_TABLE(i2c, adv7604_id);
> > -
> >  static struct i2c_driver adv7604_driver = {
> >  	.driver = {
> >  		.owner = THIS_MODULE,
> >  		.name = "adv7604",
> > +		.of_match_table = of_match_ptr(adv7604_of_id),
> 
> of_match_ptr() isn't necessary here.

Thanks, will fix in v3.

> >  	},
> >  	.probe = adv7604_probe,
> >  	.remove = adv7604_remove,
> > -	.id_table = adv7604_id,
> > +	.id_table = adv7604_i2c_id,
> >  };
> >  
> >  module_i2c_driver(adv7604_driver);

-- 
Regards,

Laurent Pinchart