Hi Sylwester,
On Thursday 17 April 2014 14:36:32 Laurent Pinchart wrote:
> 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: devicet...@vger.kernel.org
> > > Signed-off-by: Laurent Pinchart <laurent.pinch...@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.
On second thought, as the driver has non-DT users, keeping of_match_ptr() and
marking the table as __maybe_unused will optimize the table out if neither
CONFIG_OF nor CONFIG_MODULE is set. I'd thus prefer keeping of_match_ptr().
> > > },
> > > .probe = adv7604_probe,
> > > .remove = adv7604_remove,
> > >
> > > - .id_table = adv7604_id,
> > > + .id_table = adv7604_i2c_id,
> > >
> > > };
> > >
> > > module_i2c_driver(adv7604_driver);
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
