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

[Prabhakar Lad]

Hi Mark,

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.

Regards,
--Prabhakar Lad
--
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/