Re: [PATCH v3 2/2] media: i2c: adv7343: add OF support
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 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 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 > >> >> wrote: > >> >> > Cc: DT binding maintainers > >> > > >> > Cheers! > >> > > >> >> > > >> >> > On 07/20/2013 08:21 AM, Lad, Prabhakar wrote: > >> >> >> From: "Lad, Prabhakar" > >> >> >> > >> >> >> add OF support for the adv7343 driver. > >> >> >> > >> >> >> Signed-off-by: Lad, Prabhakar > >> >> >> --- > >> >> > [...] > >> >> >> .../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 000..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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/2] media: i2c: adv7343: add OF support
Hi Mark, On Mon, Sep 2, 2013 at 9:47 PM, Mark Rutland 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 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 >> >> wrote: >> >> > Cc: DT binding maintainers >> > >> > Cheers! >> > >> >> > >> >> > On 07/20/2013 08:21 AM, Lad, Prabhakar wrote: >> >> >> From: "Lad, Prabhakar" >> >> >> >> >> >> add OF support for the adv7343 driver. >> >> >> >> >> >> Signed-off-by: Lad, Prabhakar >> >> >> --- >> >> > [...] >> >> >> .../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 000..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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/2] media: i2c: adv7343: add OF support
Hi Mark, On Mon, Sep 2, 2013 at 9:47 PM, Mark Rutland mark.rutl...@arm.com 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.rutl...@arm.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.nawro...@samsung.com wrote: Cc: DT binding maintainers Cheers! On 07/20/2013 08:21 AM, Lad, Prabhakar wrote: From: Lad, Prabhakar prabhakar.cse...@gmail.com add OF support for the adv7343 driver. Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.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 000..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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/2] media: i2c: adv7343: add OF support
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.rutl...@arm.com 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.rutl...@arm.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.nawro...@samsung.com wrote: Cc: DT binding maintainers Cheers! On 07/20/2013 08:21 AM, Lad, Prabhakar wrote: From: Lad, Prabhakar prabhakar.cse...@gmail.com add OF support for the adv7343 driver. Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.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 000..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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 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 > >> wrote: > >> > Cc: DT binding maintainers > > > > Cheers! > > > >> > > >> > On 07/20/2013 08:21 AM, Lad, Prabhakar wrote: > >> >> From: "Lad, Prabhakar" > >> >> > >> >> add OF support for the adv7343 driver. > >> >> > >> >> Signed-off-by: Lad, Prabhakar > >> >> --- > >> > [...] > >> >> .../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 000..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
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.rutl...@arm.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.nawro...@samsung.com wrote: Cc: DT binding maintainers Cheers! On 07/20/2013 08:21 AM, Lad, Prabhakar wrote: From: Lad, Prabhakar prabhakar.cse...@gmail.com add OF support for the adv7343 driver. Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.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 000..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
Re: [PATCH v3 2/2] media: i2c: adv7343: add OF support
Hi Mark, On Tue, Aug 27, 2013 at 8:54 PM, Mark Rutland 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 >> wrote: >> > Cc: DT binding maintainers > > Cheers! > >> > >> > On 07/20/2013 08:21 AM, Lad, Prabhakar wrote: >> >> From: "Lad, Prabhakar" >> >> >> >> add OF support for the adv7343 driver. >> >> >> >> Signed-off-by: Lad, Prabhakar >> >> --- >> > [...] >> >> .../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 000..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. >> > >> > 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 >> > >> > 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
Re: [PATCH v3 2/2] media: i2c: adv7343: add OF support
[fixing up devicetree list address] 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 > wrote: > > Cc: DT binding maintainers Cheers! > > > > On 07/20/2013 08:21 AM, Lad, Prabhakar wrote: > >> From: "Lad, Prabhakar" > >> > >> add OF support for the adv7343 driver. > >> > >> Signed-off-by: Lad, Prabhakar > >> --- > > [...] > >> .../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 000..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? > > > > 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. > > > > 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? 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?). Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at
Re: [PATCH v3 2/2] media: i2c: adv7343: add OF support
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 > wrote: > > Cc: DT binding maintainers Cheers! > > > > On 07/20/2013 08:21 AM, Lad, Prabhakar wrote: > >> From: "Lad, Prabhakar" > >> > >> add OF support for the adv7343 driver. > >> > >> Signed-off-by: Lad, Prabhakar > >> --- > > [...] > >> .../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 000..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? > > > > 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. > > > > 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? 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?). Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/2] media: i2c: adv7343: add OF support
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.nawro...@samsung.com wrote: Cc: DT binding maintainers Cheers! On 07/20/2013 08:21 AM, Lad, Prabhakar wrote: From: Lad, Prabhakar prabhakar.cse...@gmail.com add OF support for the adv7343 driver. Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.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 000..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? 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. 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? 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?). Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/2] media: i2c: adv7343: add OF support
[fixing up devicetree list address] 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.nawro...@samsung.com wrote: Cc: DT binding maintainers Cheers! On 07/20/2013 08:21 AM, Lad, Prabhakar wrote: From: Lad, Prabhakar prabhakar.cse...@gmail.com add OF support for the adv7343 driver. Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.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 000..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? 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. 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? 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?). Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/2] media: i2c: adv7343: add OF support
Hi Mark, On Tue, Aug 27, 2013 at 8:54 PM, Mark Rutland mark.rutl...@arm.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.nawro...@samsung.com wrote: Cc: DT binding maintainers Cheers! On 07/20/2013 08:21 AM, Lad, Prabhakar wrote: From: Lad, Prabhakar prabhakar.cse...@gmail.com add OF support for the adv7343 driver. Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.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 000..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. 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 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. I also note from the spec that the unit has two clocks, CLKIN_A and CLKIN_B that aren't in
Re: [PATCH v3 2/2] media: i2c: adv7343: add OF support
Hi Sylwester, On Fri, Aug 23, 2013 at 11:33 PM, Sylwester Nawrocki wrote: > Cc: DT binding maintainers > > On 07/20/2013 08:21 AM, Lad, Prabhakar wrote: >> From: "Lad, Prabhakar" >> >> add OF support for the adv7343 driver. >> >> Signed-off-by: Lad, Prabhakar >> --- > [...] >> .../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 000..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. > > 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. > > 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 >> +- 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 Regards, --Prabhakar Lad -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/2] media: i2c: adv7343: add OF support
Hi Sylwester, On Fri, Aug 23, 2013 at 11:33 PM, Sylwester Nawrocki s.nawro...@samsung.com wrote: Cc: DT binding maintainers On 07/20/2013 08:21 AM, Lad, Prabhakar wrote: From: Lad, Prabhakar prabhakar.cse...@gmail.com add OF support for the adv7343 driver. Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.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 000..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. 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. 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 +- 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 Regards, --Prabhakar Lad -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/2] media: i2c: adv7343: add OF support
Cc: DT binding maintainers
On 07/20/2013 08:21 AM, Lad, Prabhakar wrote:
> From: "Lad, Prabhakar"
>
> add OF support for the adv7343 driver.
>
> Signed-off-by: Lad, Prabhakar
> ---
[...]
> .../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 000..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.
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.
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.
> +- 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.
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".
> +- 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.
> +Example:
> +
> +i2c0@1c22000 {
> + ...
> + ...
> +
> + adv7343@2a {
> + compatible = "adi,adv7343";
> + reg = <0x2a>;
> +
> + port {
> + adv7343_1: endpoint {
> + adi,power-mode-sleep-mode;
> + adi,power-mode-pll-ctrl;
> + /* Use DAC1..3, DAC6 */
> + adi,dac-enable = <1 1 1 0 0 1>;
> + /* Use SD DAC output 1 */
> + adi,sd-dac-enable = <1 0>;
> + };
> + };
> + };
> + ...
> +};
> diff --git a/drivers/media/i2c/adv7343.c b/drivers/media/i2c/adv7343.c
> index f0238fb..aeb56c5 100644
> --- a/drivers/media/i2c/adv7343.c
> +++ b/drivers/media/i2c/adv7343.c
> @@ -30,6 +30,7 @@
> #include
> #include
> #include
> +#include
>
> #include "adv7343_regs.h"
>
> @@ -399,6 +400,40 @@ static int adv7343_initialize(struct v4l2_subdev *sd)
> return err;
> }
>
> +static struct adv7343_platform_data *
> +adv7343_get_pdata(struct i2c_client *client)
> +{
> + struct adv7343_platform_data *pdata;
> + struct device_node *np;
> +
> + if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
> + return client->dev.platform_data;
> +
> + np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
> + if (!np)
> + return NULL;
> +
> + pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + goto done;
> +
> + pdata->mode_config.sleep_mode =
> + of_property_read_bool(np, "adi,power-mode-sleep-mode");
> +
> + pdata->mode_config.pll_control =
> + of_property_read_bool(np, "adi,power-mode-pll-ctrl");
> +
> + of_property_read_u32_array(np, "adi,dac-enable",
> +pdata->mode_config.dac, 6);
> +
> + of_property_read_u32_array(np, "adi,sd-dac-enable",
> +
Re: [PATCH v3 2/2] media: i2c: adv7343: add OF support
Cc: DT binding maintainers
On 07/20/2013 08:21 AM, Lad, Prabhakar wrote:
From: Lad, Prabhakar prabhakar.cse...@gmail.com
add OF support for the adv7343 driver.
Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.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 000..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.
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.
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.
+- 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.
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.
+- 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.
+Example:
+
+i2c0@1c22000 {
+ ...
+ ...
+
+ adv7343@2a {
+ compatible = adi,adv7343;
+ reg = 0x2a;
+
+ port {
+ adv7343_1: endpoint {
+ adi,power-mode-sleep-mode;
+ adi,power-mode-pll-ctrl;
+ /* Use DAC1..3, DAC6 */
+ adi,dac-enable = 1 1 1 0 0 1;
+ /* Use SD DAC output 1 */
+ adi,sd-dac-enable = 1 0;
+ };
+ };
+ };
+ ...
+};
diff --git a/drivers/media/i2c/adv7343.c b/drivers/media/i2c/adv7343.c
index f0238fb..aeb56c5 100644
--- a/drivers/media/i2c/adv7343.c
+++ b/drivers/media/i2c/adv7343.c
@@ -30,6 +30,7 @@
#include media/v4l2-async.h
#include media/v4l2-device.h
#include media/v4l2-ctrls.h
+#include media/v4l2-of.h
#include adv7343_regs.h
@@ -399,6 +400,40 @@ static int adv7343_initialize(struct v4l2_subdev *sd)
return err;
}
+static struct adv7343_platform_data *
+adv7343_get_pdata(struct i2c_client *client)
+{
+ struct adv7343_platform_data *pdata;
+ struct device_node *np;
+
+ if (!IS_ENABLED(CONFIG_OF) || !client-dev.of_node)
+ return client-dev.platform_data;
+
+ np = v4l2_of_get_next_endpoint(client-dev.of_node, NULL);
+ if (!np)
+ return NULL;
+
+ pdata = devm_kzalloc(client-dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ goto done;
+
+ pdata-mode_config.sleep_mode =
+ of_property_read_bool(np, adi,power-mode-sleep-mode);
+
+ pdata-mode_config.pll_control =
+ of_property_read_bool(np, adi,power-mode-pll-ctrl);
+
+ of_property_read_u32_array(np, adi,dac-enable,
+pdata-mode_config.dac, 6);
+
+ of_property_read_u32_array(np, adi,sd-dac-enable,
+
