Re: [PATCH v3 2/2] media: i2c: adv7343: add OF support

[Mark Rutland]

On Wed, Sep 04, 2013 at 08:13:38AM +0100, Prabhakar Lad wrote:
> Hi Mark,

Hi Prabhakar,

> 
> On Mon, Sep 2, 2013 at 9:47 PM, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> > On Wed, Aug 28, 2013 at 03:43:04AM +0100, Prabhakar Lad wrote:
> >> Hi Mark,
> >>
> >> On Tue, Aug 27, 2013 at 8:54 PM, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> >> > [fixing up devicetree list address]
> >> >
> >> Thanks!
> >>
> >> > On Mon, Aug 26, 2013 at 03:41:45AM +0100, Prabhakar Lad wrote:
> >> >> Hi Sylwester,
> >> >>
> >> >> On Fri, Aug 23, 2013 at 11:33 PM, Sylwester Nawrocki
> >> >> <s.nawrocki@xxxxxxxxxxx> wrote:
> >> >> > Cc: DT binding maintainers
> >> >
> >> > Cheers!
> >> >
> >> >> >
> >> >> > On 07/20/2013 08:21 AM, Lad, Prabhakar wrote:
> >> >> >> From: "Lad, Prabhakar" <prabhakar.csengg@xxxxxxxxx>
> >> >> >>
> >> >> >> add OF support for the adv7343 driver.
> >> >> >>
> >> >> >> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx>
> >> >> >> ---
> >> >> > [...]
> >> >> >>  .../devicetree/bindings/media/i2c/adv7343.txt      |   48 ++++++++++++++++++++
> >> >> >>  drivers/media/i2c/adv7343.c                        |   46 ++++++++++++++++++-
> >> >> >>  2 files changed, 93 insertions(+), 1 deletion(-)
> >> >> >>  create mode 100644 Documentation/devicetree/bindings/media/i2c/adv7343.txt
> >> >> >>
> >> >> >> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7343.txt b/Documentation/devicetree/bindings/media/i2c/adv7343.txt
> >> >> >> new file mode 100644
> >> >> >> index 0000000..5653bc2
> >> >> >> --- /dev/null
> >> >> >> +++ b/Documentation/devicetree/bindings/media/i2c/adv7343.txt
> >> >> >> @@ -0,0 +1,48 @@
> >> >> >> +* Analog Devices adv7343 video encoder
> >> >> >> +
> >> >> >> +The ADV7343 are high speed, digital-to-analog video encoders in a 64-lead LQFP
> >> >> >> +package. Six high speed, 3.3 V, 11-bit video DACs provide support for composite
> >> >> >> +(CVBS), S-Video (Y-C), and component (YPrPb/RGB) analog outputs in standard
> >> >> >> +definition (SD), enhanced definition (ED), or high definition (HD) video
> >> >> >> +formats.
> >> >> >> +
> >> >> >> +Required Properties :
> >> >> >> +- compatible: Must be "adi,adv7343"
> >> >> >> +
> >> >> >> +Optional Properties :
> >> >> >> +- adi,power-mode-sleep-mode: on enable the current consumption is reduced to
> >> >> >> +                           micro ampere level. All DACs and the internal PLL
> >> >> >> +                           circuit are disabled.
> >> >
> >> > This seems to be a boolean property, and I couldn't find any description
> >> > in the linked datasheet of the constraints under which the unit may be
> >> > put into sleep mode.
> >> >
> >> > Why do we require this property in the dt? Can the driver not always put
> >> > a adv734x into sleep mode if it wants to, and then wake it up as
> >> > required?
> >> >
> >> The adv7343 decoder, fits on da850/dm6467 etc.. For the da850 it supports
> >> only SD where as the dm6467 supports HD/SD/ED for which DAC 1-6 of
> >> Register 0x0 varies for this board so I added them as the platform data
> >> but I got a review comment in the ML asking to add entire register as
> >> the pdata instead of DAC 1-6, so because of which it is being converted
> >> in the same way for DT.
> >
> > Not everything that appears in platform data should appear in the dt.
> > This seems more like a run-time decision that a description of the
> > hardware.
> >
> > I don't see why we need the "adi,power-mode-sleep-mode" property.
> >
> Ok I will drop "adi,power-mode-sleep-mode" and "adi,power-mode-pll-ctrl"
> property from the DT bindings and just have "adi,dac-enable",
> "adi,sd-dac-enable" properties as this cannot be handled runtime.

I'm still somewhat confused by these properties. The "adi,sd-dac-enable"
property only describes two, the "sd" DACs, and "adi,dac-enable"
describes 6. From the block diagram in the device manual, there are only
6 DACs (1...6). None of the DACs seem to be limited in what they support
(unless that's described elsewhere), so I don't understand what the "sd"
DACs are. Could you elaborate?

Which DACs are being described by each property? They seem to overlap.

Why do we need to program them as on or off? Surely that depends on
whether or not they're connected to anything and whether or not we want
something to appear on that output? Can the "REFERENCE AND CABLE DETECT"
unit tell us that?

Until the binding is clarified and stabilised, I don't think the binding
or driver should be merged.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/