Message-ID: <20130902161715.GB18206@e106331-lin.cambridge.arm.com>
Date:	Mon, 2 Sep 2013 17:17:15 +0100
From:	Mark Rutland <mark.rutland@....com>
To:	Prabhakar Lad <prabhakar.csengg@...il.com>
Cc:	Sylwester Nawrocki <s.nawrocki@...sung.com>,
	LMML <linux-media@...r.kernel.org>,
	DLOS <davinci-linux-open-source@...ux.davincidsp.com>,
	LKML <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	LDOC <linux-doc@...r.kernel.org>,
	Stephen Warren <swarren@...dotorg.org>,
	Pawel Moll <Pawel.Moll@....com>,
	Kumar Gala <galak@...eaurora.org>,
	"rob.herring@...xeda.com" <rob.herring@...xeda.com>
Subject: Re: [PATCH v3 2/2] media: i2c: adv7343: add OF support

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@....com> 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@...sung.com> wrote:
> >> > Cc: DT binding maintainers
> >
> > Cheers!
> >
> >> >
> >> > On 07/20/2013 08:21 AM, Lad, Prabhakar wrote:
> >> >> From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
> >> >>
> >> >> add OF support for the adv7343 driver.
> >> >>
> >> >> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@...il.com>
> >> >> ---
> >> > [...]
> >> >>  .../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.

> 
> >> >
> >> > Sorry for getting back so late to this. I realize this is already queued in
> >> > the media tree. But this binding doesn't look good enough to me. I think it
> >> > will need to be corrected during upcoming -rc period.
> >> >
> >> Thanks for the catch :-)
> >>
> >> > It might be hard to figure out only from the chip's datasheet what
> >> > adi,power-mode-sleep-mode really refers to. AFAICS it is for assigning some
> >> > value to a specific register. If we really need to specify register values
> >> > in the device tree then it would probably make sense to describe to which
> >> > register this apply. Now the name looks like derived from some structure
> >> > member name in the Linux driver of the device.
> >> >
> >> the property is derived from the datasheet itself for example the
> >> 'adi,power-mode-sleep-mode' --> Register 0x0 power mode bit 0
> >> 'adi,power-mode-pll-ctrl' ---> Register 0x0 power mode bit 1
> >> 'adi,dac-enable' ----> Register 0x0 power mode bit 2-7
> >> 'adi,sd-dac-enable' ---> Register 0x82 SD mode register bit 1-2
> >>
> >> [1] http://www.analog.com/static/imported-files/data_sheets/ADV7342_7343.pdf
> >>
> >> >> +- adi,power-mode-pll-ctrl: PLL and oversampling control. This control allows
> >> >> +                        internal PLL 1 circuit to be powered down and the
> >> >> +                        oversampling to be switched off.
> >
> > This affects whether or not oversampling is possible. That sounds like
> > it should be a run-time configurable rather than a fixed property of the
> > device.
> >
> ditto

Not all platform data is suitable for dt. This seems like a decision as
to how to use the hardware, rather than a description of the hardware.
Hardware description should go in the dt, decisions should go in the
driver.

> 
> >> >
> >> > Similar comments applies to this property.
> >> >
> >> >> +- ad,adv7343-power-mode-dac: array configuring the power on/off DAC's 1..6,
> >> >> +                           0 = OFF and 1 = ON, Default value when this
> >> >> +                           property is not specified is <0 0 0 0 0 0>.
> >> >
> >> > Name of the property is incorrect here. It has changed to "adi,dac-enable".
> >> >
> >> OK
> >
> > Why do we need this property at all? Might some DACs not be wired up to
> > anything?
> >
> > Surely this could be configured at run-time as and when the user wants
> > to use the DACs.
> >
> >>
> >> >> +- ad,adv7343-sd-config-dac-out: array configure SD DAC Output's 1 and 2, 0 = OFF
> >> >> +                              and 1 = ON, Default value when this property is
> >> >> +                              not specified is <0 0>.
> >> >
> >> > Similarly, "adi,sd-dac-enable.
> >> >
> >> OK
> >
> > Again, couldn't this be done at run-time?
> >
> > Do we need to permanently disable/enable some DACs? If so, why?
> >
> ditto.

And ditto to my points above.

Cheers,
Mark.

> 
> > I also note from the spec that the unit has two clocks, CLKIN_A and
> > CLKIN_B that aren't in the binding, but should be. Some regulators
> > too (VDD, PVDD, VAA, VDD_IO, VREF?).
> >
> OK
> 
> Regards,
> --Prabhakar Lad
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists