Re: [RESEND RFC/PATCH 1/8] dt-bindings: Add a binding for Mediatek Video Processor Unit

[Mark Rutland]

On Tue, Nov 17, 2015 at 08:54:38PM +0800, Tiffany Lin wrote:
> From: Andrew-CT Chen 
> 
> Add a DT binding documentation of Video Processor Unit for the
> MT8173 SoC from Mediatek.
> 
> Signed-off-by: Andrew-CT Chen 
> ---
>  .../devicetree/bindings/media/mediatek-vpu.txt |   27 
> 
>  1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/mediatek-vpu.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/mediatek-vpu.txt 
> b/Documentation/devicetree/bindings/media/mediatek-vpu.txt
> new file mode 100644
> index 000..99a4e5e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/mediatek-vpu.txt
> @@ -0,0 +1,27 @@
> +* Mediatek Video Processor Unit
> +
> +Video Processor Unit is a HW video controller. It controls HW Codec including
> +H.264/VP8/VP9 Decode, H.264/VP8 Encode and Image Processor 
> (scale/rotate/color convert).
> +
> +Required properties:
> +  - compatible: "mediatek,mt8173-vpu"
> +  - reg: Must contain an entry for each entry in reg-names.
> +  - reg-names: Must include the following entries:
> +"sram": SRAM base
> +"cfg_reg": Main configuration registers base
> +  - interrupts: interrupt number to the cpu.
> +  - clocks : clock name from clock manager
> +  - clock-names: the clocks of the VPU H/W

You need to explicitly define the set of clock-names you expect here.

Mark.

> +  - iommus : phandle and IOMMU spcifier for the IOMMU that serves the VPU.
> +
> +Example:
> + vpu: vpu@1002 {
> + compatible = "mediatek,mt8173-vpu";
> + reg = <0 0x1002 0 0x3>,
> +   <0 0x1005 0 0x100>;
> + reg-names = "sram", "cfg_reg";
> + interrupts = ;
> + clocks = < TOP_SCP_SEL>;
> + clock-names = "main";
> + iommus = < M4U_PORT_VENC_RCPU>;
> + };
> -- 
> 1.7.9.5
> 
--
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

Re: [PATCH v3] DT: Add documentation for the mfd Maxim max77693

[Mark Rutland]

Hi,

  +Optional properties:
  +- maxim,trigger-type : Flash trigger type.
  +  Possible trigger types:
  +  LEDS_TRIG_TYPE_EDGE (0) - Rising edge of the signal triggers
  +  the flash,
  +  LEDS_TRIG_TYPE_LEVEL (1) - Strobe pulse length controls duration
  +  of the flash.
 
  Surely this is required? What should be assumed if this property isn't
  present?
 
 LEDS_TRIG_TYPE_LEVEL allows for an ISP to do e.g. short flash blink
 before the actual strobe - it is used for eliminating photographs with
 closed eyes, or can serve for probing ambient light conditions.
 
 With LEDS_TRIG_TYPE_EDGE flash strobe is triggered on rising edge
 and lasts until programmed timeout expires.
 
 This setting is tightly related to a camera sensor, which generates
 the strobe signal. Effectively it depends on board configuration.

My comment wasn't to do with the semantics of eitehr option but rather
the optionality of the property.

Surely it's vital to know what this should be, and hence this property
should be required rather than optional?

If it isn't required, what would the assumed default be?

Mark.
--
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

Re: [PATCH v3] DT: Add documentation for the mfd Maxim max77693

[Mark Rutland]

On Mon, Mar 30, 2015 at 10:52:28AM +0100, Jacek Anaszewski wrote:
 This patch adds device tree binding documentation for
 the flash cell of the Maxim max77693 multifunctional device.
 
 Signed-off-by: Jacek Anaszewski j.anaszew...@samsung.com
 Signed-off-by: Andrzej Hajda a.ha...@samsung.com
 Acked-by: Kyungmin Park kyungmin.p...@samsung.com
 Acked-by: Sakari Ailus sakari.ai...@linux.intel.com
 Cc: Lee Jones lee.jo...@linaro.org
 Cc: Chanwoo Choi cw00.c...@samsung.com
 Cc: Bryan Wu coolo...@gmail.com
 Cc: Richard Purdie rpur...@rpsys.net
 Cc: devicet...@vger.kernel.org
 ---
  Documentation/devicetree/bindings/mfd/max77693.txt |   63 
 
  1 file changed, 63 insertions(+)
 
 - added range to maxim,boost-mvout and maxim,mvsys-min description
 
 diff --git a/Documentation/devicetree/bindings/mfd/max77693.txt 
 b/Documentation/devicetree/bindings/mfd/max77693.txt
 index 38e6440..3d2103a 100644
 --- a/Documentation/devicetree/bindings/mfd/max77693.txt
 +++ b/Documentation/devicetree/bindings/mfd/max77693.txt
 @@ -76,7 +76,55 @@ Optional properties:
  Valid values: 430, 470, 480, 490
  Default: 430
  
 +- led : the LED submodule device node
 +
 +There are two LED outputs available - FLED1 and FLED2. Each of them can
 +control a separate LED or they can be connected together to double
 +the maximum current for a single connected LED. One LED is represented
 +by one child node.
 +
 +Required properties:
 +- compatible : Must be maxim,max77693-led.
 +
 +Optional properties:
 +- maxim,trigger-type : Flash trigger type.
 + Possible trigger types:
 + LEDS_TRIG_TYPE_EDGE (0) - Rising edge of the signal triggers
 + the flash,
 + LEDS_TRIG_TYPE_LEVEL (1) - Strobe pulse length controls duration
 + of the flash.

Surely this is required? What should be assumed if this property isn't
present?

Otherwise this looks OK, but I'm not that familiar with LED/flash
bindings.

Mark.

 +- maxim,boost-mode :
 + In boost mode the device can produce up to 1.2A of total current
 + on both outputs. The maximum current on each output is reduced
 + to 625mA then. If not enabled explicitly, boost setting defaults to
 + LEDS_BOOST_FIXED in case both current sources are used.
 + Possible values:
 + LEDS_BOOST_OFF (0) - no boost,
 + LEDS_BOOST_ADAPTIVE (1) - adaptive mode,
 + LEDS_BOOST_FIXED (2) - fixed mode.
 +- maxim,boost-mvout : Output voltage of the boost module in millivolts.
 + Range: 3300 - 5500
 +- maxim,mvsys-min : Low input voltage level in millivolts. Flash is not fired
 + if chip estimates that system voltage could drop below this level due
 + to flash power consumption.
 + Range: 2400 - 3400
 +
 +Required properties of the LED child node:
 +- led-sources : see Documentation/devicetree/bindings/leds/common.txt;
 + device current output identifiers: 0 - FLED1, 1 - FLED2
 +
 +Optional properties of the LED child node:
 +- label : see Documentation/devicetree/bindings/leds/common.txt
 +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
 + Range: 15625 - 25
 +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
 + Range: 15625 - 100
 +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
 + Range: 62500 - 100
 +
  Example:
 +#include dt-bindings/leds/common.h
 +
   max77693@66 {
   compatible = maxim,max77693;
   reg = 0x66;
 @@ -117,5 +165,20 @@ Example:
   maxim,thermal-regulation-celsius = 75;
   maxim,battery-overcurrent-microamp = 300;
   maxim,charge-input-threshold-microvolt = 430;
 +
 + led {
 + compatible = maxim,max77693-led;
 + maxim,trigger-type = LEDS_TRIG_TYPE_LEVEL;
 + maxim,boost-mode = LEDS_BOOST_FIXED;
 + maxim,boost-mvout = 5000;
 + maxim,mvsys-min = 2400;
 +
 + camera_flash: flash-led {
 + label = max77693-flash;
 + led-sources = 0, 1;
 + max-microamp = 50;
 + flash-max-microamp = 125;
 + flash-timeout-us = 100;
 + };
   };
   };
 -- 
 1.7.9.5
 
 --
 To unsubscribe from this list: send the line unsubscribe devicetree in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
--
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

Re: [REVIEW PATCH v2.1 08/11] of: smiapp: Add documentation

[Mark Rutland]

On Sun, Nov 30, 2014 at 04:26:48PM +, Sakari Ailus wrote:
 Document the smiapp device tree properties.
 
 Signed-off-by: Sakari Ailus sakari.ai...@iki.fi
 ---
 since v2:
 - Cleanups
 - Removed clock-names property documentation
 - Port node documentation was really endpoint node documentation
 - Added remote-endpoint as mandatory endpoint node properties
 
  .../devicetree/bindings/media/i2c/nokia,smia.txt   |   64 
 
  MAINTAINERS|1 +
  2 files changed, 65 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/media/i2c/nokia,smia.txt
 
 diff --git a/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt 
 b/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt
 new file mode 100644
 index 000..2114a4d
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt
 @@ -0,0 +1,64 @@
 +SMIA/SMIA++ sensor
 +
 +SMIA (Standard Mobile Imaging Architecture) is an image sensor standard
 +defined jointly by Nokia and ST. SMIA++, defined by Nokia, is an extension
 +of that. These definitions are valid for both types of sensors.
 +
 +More detailed documentation can be found in
 +Documentation/devicetree/bindings/media/video-interfaces.txt .
 +
 +
 +Mandatory properties
 +
 +
 +- compatible: nokia,smia
 +- reg: I2C address (0x10, or an alternative address)
 +- vana-supply: Analogue voltage supply (VANA), typically 2,8 volts (sensor
 +  dependent).
 +- clocks: External clock phandle

Not just a phandle, there's a clock-specifier too.

Just describe what the clock logically is, don't bother with describing
the format of the property (whcih is standardised elsewhere).

 +- clock-frequency: Frequency of the external clock to the sensor

Is this the preferred frequency to operate the device at? Is there not a
standard frequency to use? We can query the rate from the clock
otherwise.

 +- link-frequency: List of allowed data link frequencies. An array of 64-bit
 +  elements.

Something like 'allowed-link-frequencies' might be better, unlesss this
is derived from another binding?

 +
 +
 +Optional properties
 +---
 +
 +- nokia,nvm-size: The size of the NVM, in bytes. If the size is not given,
 +  the NVM contents will not be read.

Where 'NVM' standas for what?

What is this used for?

 +- reset-gpios: XSHUTDOWN GPIO
 +
 +
 +Endpoint node mandatory properties
 +--
 +
 +- clock-lanes: 0
 +- data-lanes: 1..n
 +- remote-endpoint: A phandle to the bus receiver's endpoint node.
 +
 +
 +Example
 +---
 +
 +i2c2 {
 + clock-frequency = 40;
 +
 + smiapp_1: camera@10 {
 + compatible = nokia,smia;
 + reg = 0x10;
 + reset-gpios = gpio3 20 0;
 + vana-supply = vaux3;
 + clocks = omap3_isp 0;
 + clock-names = ext_clk;

This wasn't described above. Either mandate it in the binding (and
define clock in terms of clock-names) or drop it.

Thanks,
Mark.

 + clock-frequency = 960;
 + nokia,nvm-size = 512; /* 8 * 64 */
 + link-frequency = /bits/ 64 19920 21000 49920;
 + port {
 + smiapp_1_1: endpoint {
 + clock-lanes = 0;
 + data-lanes = 1 2;
 + remote-endpoint = csi2a_ep;
 + };
 + };
 + };
 +};
 diff --git a/MAINTAINERS b/MAINTAINERS
 index 2378a5f..285c1ba 100644
 --- a/MAINTAINERS
 +++ b/MAINTAINERS
 @@ -8619,6 +8619,7 @@ F:  include/media/smiapp.h
  F:   drivers/media/i2c/smiapp-pll.c
  F:   drivers/media/i2c/smiapp-pll.h
  F:   include/uapi/linux/smiapp.h
 +F:   Documentation/devicetree/bindings/media/i2c/nokia,smia.txt
  
  SMM665 HARDWARE MONITOR DRIVER
  M:   Guenter Roeck li...@roeck-us.net
 -- 
 1.7.10.4
 
 --
 To unsubscribe from this list: send the line unsubscribe devicetree in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
--
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

Re: [PATCH/RFC v8 08/14] DT: Add documentation for exynos4-is 'flashes' property

[Mark Rutland]

On Fri, Nov 28, 2014 at 09:18:00AM +, Jacek Anaszewski wrote:
 This patch adds a description of 'flashes' property
 to the samsung-fimc.txt.
 
 Signed-off-by: Jacek Anaszewski j.anaszew...@samsung.com
 Acked-by: Kyungmin Park kyungmin.p...@samsung.com
 Cc: Sylwester Nawrocki s.nawro...@samsung.com
 Cc: Rob Herring robh...@kernel.org
 Cc: Pawel Moll pawel.m...@arm.com
 Cc: Mark Rutland mark.rutl...@arm.com
 Cc: Ian Campbell ijc+devicet...@hellion.org.uk
 Cc: Kumar Gala ga...@codeaurora.org
 Cc: devicet...@vger.kernel.org
 ---
  .../devicetree/bindings/media/samsung-fimc.txt |7 +++
  1 file changed, 7 insertions(+)
 
 diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt 
 b/Documentation/devicetree/bindings/media/samsung-fimc.txt
 index 922d6f8..4b7ed03 100644
 --- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
 +++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
 @@ -40,6 +40,12 @@ should be inactive. For the active-a state the camera 
 port A must be activated
  and the port B deactivated and for the state active-b it should be the 
 other
  way around.
  
 +Optional properties:
 +
 +- flashes - array of strings with flash led names; the name has to
 + be same with the related led label
 + (see Documentation/devicetree/bindings/leds/common.txt)
 +

Why is this not an array of phandles to the LED nodes? That's much
better than strings.

Also, I only seem to have recevied the documentation patches and none of
the code -- in future when posting RFC DT patches, please Cc for the
code too as it's useful context.

Mark.

  The 'camera' node must include at least one 'fimc' child node.
  
  
 @@ -166,6 +172,7 @@ Example:
   clock-output-names = cam_a_clkout, cam_b_clkout;
   pinctrl-names = default;
   pinctrl-0 = cam_port_a_clk_active;
 + flashes = max77693-flash1, max77693-flash2;
   status = okay;
   #address-cells = 1;
   #size-cells = 1;
 -- 
 1.7.9.5
 
 --
 To unsubscribe from this list: send the line unsubscribe devicetree in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
--
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

Re: [PATCH/RFC v8 08/14] DT: Add documentation for exynos4-is 'flashes' property

[Mark Rutland]

On Fri, Nov 28, 2014 at 12:09:14PM +, Jacek Anaszewski wrote:
 On 11/28/2014 12:14 PM, Mark Rutland wrote:
  On Fri, Nov 28, 2014 at 09:18:00AM +, Jacek Anaszewski wrote:
  This patch adds a description of 'flashes' property
  to the samsung-fimc.txt.
 
  Signed-off-by: Jacek Anaszewski j.anaszew...@samsung.com
  Acked-by: Kyungmin Park kyungmin.p...@samsung.com
  Cc: Sylwester Nawrocki s.nawro...@samsung.com
  Cc: Rob Herring robh...@kernel.org
  Cc: Pawel Moll pawel.m...@arm.com
  Cc: Mark Rutland mark.rutl...@arm.com
  Cc: Ian Campbell ijc+devicet...@hellion.org.uk
  Cc: Kumar Gala ga...@codeaurora.org
  Cc: devicet...@vger.kernel.org
  ---
.../devicetree/bindings/media/samsung-fimc.txt |7 +++
1 file changed, 7 insertions(+)
 
  diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt 
  b/Documentation/devicetree/bindings/media/samsung-fimc.txt
  index 922d6f8..4b7ed03 100644
  --- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
  +++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
  @@ -40,6 +40,12 @@ should be inactive. For the active-a state the camera 
  port A must be activated
and the port B deactivated and for the state active-b it should be the 
  other
way around.
 
  +Optional properties:
  +
  +- flashes - array of strings with flash led names; the name has to
  +  be same with the related led label
  +  (see Documentation/devicetree/bindings/leds/common.txt)
  +
 
  Why is this not an array of phandles to the LED nodes? That's much
  better than strings.
 
 This is because a single flash led device can control many sub-leds,
 which are represented by child nodes in the Device Tree.
 Every sub-led is registered as a separate LED Flash class device
 in the LED subsystem, but in fact they share the same struct device
 and thus have access only to the parent's phandle.

But that's a Linux infrastrcture issue, no? You don't have to use the
node from the struct device to find the relevant phandle.

 The LED Flash
 class devices are wrapped by V4L2 sub-devices and register
 asynchronously within a media device. Since the v4l2_subdev structure
 has a 'name' field, it is convenient to initialize it with
 parsed 'label' property of a child led node and match the
 sub-devices in the media device basing on it.

While that might be convenient, I don't think it's fantastic to use that
to describe the relationship, as this leaks Linux internals (e.g. I can
refer to a name that doesn't exist in the DT but happens to be what
Linux used, and it would work). Also, are the labels guaranteed to be
globally unique?

Using phandles is much better for the binding. I appreciate that this
may require more code, but IMO it's worth that for the safety and
uniformity given by the use of phandles for referring to nodes.

  Also, I only seem to have recevied the documentation patches and none of
  the code -- in future when posting RFC DT patches, please Cc for the
  code too as it's useful context.
 
 Of course, I'll keep it in mind.

Thanks!

Mark.
--
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

Re: [PATCH/RFC v8 08/14] DT: Add documentation for exynos4-is 'flashes' property

[Mark Rutland]

On Fri, Nov 28, 2014 at 02:11:04PM +, Jacek Anaszewski wrote:
 On 11/28/2014 01:30 PM, Mark Rutland wrote:
  On Fri, Nov 28, 2014 at 12:09:14PM +, Jacek Anaszewski wrote:
  On 11/28/2014 12:14 PM, Mark Rutland wrote:
  On Fri, Nov 28, 2014 at 09:18:00AM +, Jacek Anaszewski wrote:
  This patch adds a description of 'flashes' property
  to the samsung-fimc.txt.
 
  Signed-off-by: Jacek Anaszewski j.anaszew...@samsung.com
  Acked-by: Kyungmin Park kyungmin.p...@samsung.com
  Cc: Sylwester Nawrocki s.nawro...@samsung.com
  Cc: Rob Herring robh...@kernel.org
  Cc: Pawel Moll pawel.m...@arm.com
  Cc: Mark Rutland mark.rutl...@arm.com
  Cc: Ian Campbell ijc+devicet...@hellion.org.uk
  Cc: Kumar Gala ga...@codeaurora.org
  Cc: devicet...@vger.kernel.org
  ---
 .../devicetree/bindings/media/samsung-fimc.txt |7 +++
 1 file changed, 7 insertions(+)
 
  diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt 
  b/Documentation/devicetree/bindings/media/samsung-fimc.txt
  index 922d6f8..4b7ed03 100644
  --- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
  +++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
  @@ -40,6 +40,12 @@ should be inactive. For the active-a state the 
  camera port A must be activated
 and the port B deactivated and for the state active-b it should be 
  the other
 way around.
 
  +Optional properties:
  +
  +- flashes - array of strings with flash led names; the name has to
  +be same with the related led label
  +(see Documentation/devicetree/bindings/leds/common.txt)
  +
 
  Why is this not an array of phandles to the LED nodes? That's much
  better than strings.
 
  This is because a single flash led device can control many sub-leds,
  which are represented by child nodes in the Device Tree.
  Every sub-led is registered as a separate LED Flash class device
  in the LED subsystem, but in fact they share the same struct device
  and thus have access only to the parent's phandle.
 
  But that's a Linux infrastrcture issue, no? You don't have to use the
  node from the struct device to find the relevant phandle.
 
 Right.
 
  The LED Flash
  class devices are wrapped by V4L2 sub-devices and register
  asynchronously within a media device. Since the v4l2_subdev structure
  has a 'name' field, it is convenient to initialize it with
  parsed 'label' property of a child led node and match the
  sub-devices in the media device basing on it.
 
  While that might be convenient, I don't think it's fantastic to use that
  to describe the relationship, as this leaks Linux internals (e.g. I can
  refer to a name that doesn't exist in the DT but happens to be what
  Linux used, and it would work). Also, are the labels guaranteed to be
  globally unique?
 
 The labels are used for initializing class device name and kernel
 doesn't allow to initialize two devices with same names.
 This implies that labels are guaranteed to be globally unique.

On Linux, yes, but that's an implementation detail, not a property of
the binding·

Mark.
--
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

Re: [PATCH 2/2] adv7604: Use DT parsing in dummy creation

[Mark Rutland]

Hi,

On Fri, Aug 29, 2014 at 02:28:17PM +0100, Jean-Michel Hautbois wrote:
 This patch uses DT in order to parse addresses for dummy devices of adv7604.
 If nothing is defined, it uses default addresses.
 The main prupose is using two adv76xx on the same i2c bus.

This is rather opaque.

It seems from the code below that a single adv7611 device has multiple
I2C addresses at which different registers may be accessed. I guess the
secondary instances of the unit have different addresses for all of the
pages?

 Signed-off-by: Jean-Michel Hautbois jean-michel.hautb...@vodalys.com
 ---
  .../devicetree/bindings/media/i2c/adv7604.txt  |  7 ++-
  drivers/media/i2c/adv7604.c| 56 
 ++
  2 files changed, 42 insertions(+), 21 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt 
 b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
 index c27cede..221b75c 100644
 --- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
 +++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
 @@ -10,6 +10,7 @@ Required Properties:
  
- compatible: Must contain one of the following
  - adi,adv7611 for the ADV7611
 +- adi,adv7604 for the ADV7604
  
- reg: I2C slave address

This should be updated, at least to say address(es).

  
 @@ -32,6 +33,8 @@ The digital output port node must contain at least one 
 endpoint.
  Optional Properties:
  
- reset-gpios: Reference to the GPIO connected to the device's reset pin.
 +  - reg-names : Names of registers to be reprogrammed.

This doesn't describe _which_ names you expect, and I have no idea what
is meant by to be reprogrammed.

 + Refer to source code for possible values.

Bindings shouldn't say things like this. The binding should describe the
contract between the DTB and the OS, which this clearly doesn't.

A binding document shouldn't necessitate reading code.

  Optional Endpoint Properties:
  
 @@ -50,7 +53,9 @@ Example:
  
   hdmi_receiver@4c {
   compatible = adi,adv7611;
 - reg = 0x4c;
 + /* edid page will be accessible @ 0x66 on i2c bus*/
 + reg = 0x4c 0x66;
 + reg-names = main, edid;

What about the other IDs? Are they accessible or not?

Why didn't we always list the full set of IDs in the first place? That
would have made this far less painful.

Thanks,
Mark.
--
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

Re: [PATCH 2/2] media: adv7604: Add ability to read default input port from DT

[Mark Rutland]

On Wed, Aug 13, 2014 at 01:14:35PM +0100, Ian Molton wrote:
 On Mon, 11 Aug 2014 13:19:02 +0100
 Mark Rutland mark.rutl...@arm.com wrote:
 
   -  - pclk-sample: Pixel clock polarity. Defaults to output on the falling 
   edge.
   +  - pclk-sample:  Pixel clock polarity. Defaults to output on the 
   falling edge.
  
  Unrelated whitespace change?
 
 Is there a sensible way to get miniscule whitespace changes in?

Just mention it in the commit message.

  If none of hsync-active, vsync-active and pclk-sample is specified the
  endpoint will use embedded BT.656 synchronization.

   +  - default-input: Select which input is selected after reset.
  
  Valid values are?
 
 Chip dependent. 0 for 7611, 0-1 for 7612, I expect there are other chips in 
 the family with differing numbers of inputs.

Ok. Can this be mentioned in the documentation?

Cheers,
Mark.

   +  if (!of_property_read_u32(endpoint, default_input, v))
  
  This doesn't match the binding ('_' vs '-').
 
 Good catch!
 
 -- 
 Ian Molton ian.mol...@codethink.co.uk
 --
 To unsubscribe from this list: send the line unsubscribe devicetree in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
--
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

Re: [PATCH 2/2] media: adv7604: Add ability to read default input port from DT

[Mark Rutland]

On Mon, Aug 11, 2014 at 01:05:19PM +0100, Ian Molton wrote:
 This patch adds support to the adv7604 driver for reading the default
 selected input from the Device tree. If none is provided, the driver will not
 select an input without help from userspace.
 
 Tested-by: William Towle william.to...@codethink.co.uk
 Signed-off-by: Ian Molton ian.mol...@codethink.co.uk
 ---
  Documentation/devicetree/bindings/media/i2c/adv7604.txt | 5 -
  drivers/media/i2c/adv7604.c | 9 +++--
  2 files changed, 11 insertions(+), 3 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt 
 b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
 index cc0708c..6e8ace0 100644
 --- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
 +++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
 @@ -41,11 +41,12 @@ Optional Endpoint Properties:
  
- hsync-active: Horizontal synchronization polarity. Defaults to active 
 low.
- vsync-active: Vertical synchronization polarity. Defaults to active low.
 -  - pclk-sample: Pixel clock polarity. Defaults to output on the falling 
 edge.
 +  - pclk-sample:  Pixel clock polarity. Defaults to output on the falling 
 edge.

Unrelated whitespace change?

  
If none of hsync-active, vsync-active and pclk-sample is specified the
endpoint will use embedded BT.656 synchronization.
  
 +  - default-input: Select which input is selected after reset.

Valid values are?

  
  Example:
  
 @@ -59,6 +60,8 @@ Example:
   #address-cells = 1;
   #size-cells = 0;
  
 + default-input = 0;
 +
   port@0 {
   reg = 0;
   };
 diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
 index 9f73a7f..75e1942 100644
 --- a/drivers/media/i2c/adv7604.c
 +++ b/drivers/media/i2c/adv7604.c
 @@ -2732,7 +2732,7 @@ static int adv7604_parse_dt(struct adv7604_state *state)
   struct v4l2_of_endpoint bus_cfg;
   struct device_node *endpoint;
   struct device_node *np;
 - unsigned int flags;
 + unsigned int flags, v;
  
   np = state-i2c_clients[ADV7604_PAGE_IO]-dev.of_node;
  
 @@ -2742,6 +2742,12 @@ static int adv7604_parse_dt(struct adv7604_state 
 *state)
   return -EINVAL;
  
   v4l2_of_parse_endpoint(endpoint, bus_cfg);
 +
 +  if (!of_property_read_u32(endpoint, default_input, v))

This doesn't match the binding ('_' vs '-').

Thanks,
Mark.
--
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

Re: [PATCH v2 8/9] Documentation: devicetree: Document sclk-jpeg clock for exynos3250 SoC

[Mark Rutland]

On Fri, Jul 11, 2014 at 04:19:49PM +0100, Jacek Anaszewski wrote:
 JPEG IP on Exynos3250 SoC requires enabling two clock
 gates for its operation. This patch documents this
 requirement.
 
 Signed-off-by: Jacek Anaszewski j.anaszew...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
 Cc: Rob Herring robh...@kernel.org
 Cc: Pawel Moll pawel.m...@arm.com
 Cc: Mark Rutland mark.rutl...@arm.com
 Cc: Ian Campbell ijc+devicet...@hellion.org.uk
 Cc: Kumar Gala ga...@codeaurora.org
 Cc: devicet...@vger.kernel.org
 ---
  .../bindings/media/exynos-jpeg-codec.txt   |9 ++---
  1 file changed, 6 insertions(+), 3 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt 
 b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
 index 937b755..3142745 100644
 --- a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
 +++ b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
 @@ -3,9 +3,12 @@ Samsung S5P/EXYNOS SoC series JPEG codec
  Required properties:
  
  - compatible : should be one of:
 -   samsung,s5pv210-jpeg, samsung,exynos4210-jpeg;
 +   samsung,s5pv210-jpeg, samsung,exynos4210-jpeg,
 +   samsung,exynos3250-jpeg;
  - reg: address and length of the JPEG codec IP register set;
  - interrupts : specifies the JPEG codec IP interrupt;
 -- clocks : should contain the JPEG codec IP gate clock specifier, from 
 the
 +- clocks : should contain the JPEG codec IP gate clock specifier and
 +   for the Exynos3250 SoC additionally the SCLK_JPEG entry; from 
 the
 common clock bindings;
 -- clock-names: should contain jpeg entry.
 +- clock-names: should contain jpeg entry and additionally 
 sclk-jpeg entry
 +   for Exynos3250 SoC

Please turn this into a list for easier reading, e.g.

- clock-names: should contain:
  * jpeg for the gate clock.
  * sclk-jpeg for the SCLK_JPEG clock (only for Exynos3250).

You could also define clocks in terms of clock-names to avoid
redundancy.

The SCLK_JPEG name sounds like a global name for the clock. Is there a
name for the input line on the JPEG block this is plugged into?

Thanks,
Mark.
--
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

Re: [PATCH v2 1/2] media: soc_camera: pxa_camera documentation device-tree support

[Mark Rutland]

On Wed, Jun 25, 2014 at 08:44:31PM +0100, Robert Jarzmik wrote:
 Mark Rutland mark.rutl...@arm.com writes:
 
  On Sat, Jun 21, 2014 at 11:21:46PM +0100, Robert Jarzmik wrote:
  +Required properties:
  + - compatible: Should be marvell,pxa27x-qci
 
  Is that x a wildcard? Or is 'x' part of the name of a particular unit?
 It's kind of a wildcard for a group of platforms
 It stands for the 3 PXA27x SoCs I'm aware of : PXA270, PXA271, and PXA272. The
 difference between them is different core frequency range and embedded RAM.
 
  We prefer not to have wildcard compatible strings in DT.
 OK, then let's go for marvell,pxa270-qci.

That sounds fine to me.

 
  + - reg: register base and size
  + - interrupts: the interrupt number
  + - any required generic properties defined in video-interfaces.txt
  +
  +Optional properties:
  + - clock-frequency: host interface is driving MCLK, and MCLK rate is this 
  rate
 
  Is MCLK an input or an output of this block?
 An output clock.
 
  If the former, why isn't this described as a clock?
 It's a good point. I'll try to add that too. The little trouble I have is that
 the PXA clocks are not _yet_ in device-tree. Putting a clock description will
 make this patch dependant on the clock framework patches [1], right ?

Yes, this will.

 
  
  +Example:
  +
  +  pxa_camera: pxa_camera@5000 {
  +  compatible = marvell,pxa27x-qci;
  +  reg = 0x5000 0x1000;
  +  interrupts = 33;
  +
  +  clocks = pxa2xx_clks 24;
  +  clock-names = camera;
 
  These weren't mentioned above. Is the clock input line really called
  camera?
 This is another clock, an input clock, independant of the former one. This is
 the clock actually fed to make this IP block work. This is dependant on the
 clock framework patches [1].

Ok. This clock should be mentioned in the properties description above.

Thanks,
Mark.
--
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

Re: [PATCH v2 2/2] media: soc_camera: pxa_camera device-tree support

[Mark Rutland]

On Sat, Jun 21, 2014 at 11:21:47PM +0100, Robert Jarzmik wrote:
 Add device-tree support to pxa_camera host driver.
 
 Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
 ---
  drivers/media/platform/soc_camera/pxa_camera.c | 77 
 +-
  1 file changed, 75 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/media/platform/soc_camera/pxa_camera.c 
 b/drivers/media/platform/soc_camera/pxa_camera.c
 index d4df305..8c9de9e 100644
 --- a/drivers/media/platform/soc_camera/pxa_camera.c
 +++ b/drivers/media/platform/soc_camera/pxa_camera.c
 @@ -34,6 +34,7 @@
  #include media/videobuf-dma-sg.h
  #include media/soc_camera.h
  #include media/soc_mediabus.h
 +#include media/v4l2-of.h
  
  #include linux/videodev2.h
  
 @@ -1650,6 +1651,64 @@ static struct soc_camera_host_ops 
 pxa_soc_camera_host_ops = {
   .set_bus_param  = pxa_camera_set_bus_param,
  };
  
 +static int pxa_camera_pdata_from_dt(struct device *dev,
 + struct pxa_camera_dev *pcdev)
 +{
 + int err = 0;
 + struct device_node *np = dev-of_node;
 + struct v4l2_of_endpoint ep;
 +
 + err = of_property_read_u32(np, clock-frequency,
 +(u32 *)pcdev-mclk);

That cast is either unnecessary or this code is broken.

Use a temporary u32 if the types don't match.

Mark.
--
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

Re: [PATCH v2 1/2] media: soc_camera: pxa_camera documentation device-tree support

[Mark Rutland]

On Sat, Jun 21, 2014 at 11:21:46PM +0100, Robert Jarzmik wrote:
 Add device-tree bindings documentation for pxa_camera driver.
 
 Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
 ---
  .../devicetree/bindings/media/pxa-camera.txt   | 39 
 ++
  1 file changed, 39 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/media/pxa-camera.txt
 
 diff --git a/Documentation/devicetree/bindings/media/pxa-camera.txt 
 b/Documentation/devicetree/bindings/media/pxa-camera.txt
 new file mode 100644
 index 000..9835aae
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/media/pxa-camera.txt
 @@ -0,0 +1,39 @@
 +Marvell PXA camera host interface
 +
 +Required properties:
 + - compatible: Should be marvell,pxa27x-qci

Is that x a wildcard? Or is 'x' part of the name of a particular unit?

We prefer not to have wildcard compatible strings in DT.

 + - reg: register base and size
 + - interrupts: the interrupt number
 + - any required generic properties defined in video-interfaces.txt
 +
 +Optional properties:
 + - clock-frequency: host interface is driving MCLK, and MCLK rate is this 
 rate

Is MCLK an input or an output of this block?

If the former, why isn't this described as a clock?

 +
 +Example:
 +
 + pxa_camera: pxa_camera@5000 {
 + compatible = marvell,pxa27x-qci;
 + reg = 0x5000 0x1000;
 + interrupts = 33;
 +
 + clocks = pxa2xx_clks 24;
 + clock-names = camera;

These weren't mentioned above. Is the clock input line really called
camera?

Mark.
--
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

Re: [PATCH v8 3/10] Documentation: devicetree: Update Samsung FIMC DT binding

[Mark Rutland]

On Tue, Mar 11, 2014 at 04:34:30PM +, Sylwester Nawrocki wrote:
 This patch documents following updates of the Exynos4 SoC camera subsystem
 devicetree binding:
 
  - addition of #clock-cells and clock-output-names properties to 'camera'
node - these are now needed so the image sensor sub-devices can reference
clocks provided by the camera host interface,
  - dropped a note about required clock-frequency properties at the
image sensor nodes; the sensor devices can now control their clock
explicitly through the clk API and there is no need to require this
property in the camera host interface binding.
 
 Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com
 ---
 Changes since v7:
  - dropped a note about clock-frequency property in sensor nodes.
 
 Changes since v6:
  - #clock-cells, clock-output-names documented as mandatory properties;
  - renamed cam_mclk_{a,b} to cam_{a,b}_clkout in the example dts,
this now matches changes in exynos4.dtsi further in the patch series;
  - marked samsung,camclk-out property as deprecated.
 
 Changes since v5:
  - none.
 
 Changes since v4:
  - dropped a requirement of specific order of values in clocks/
clock-names properties (Mark) and reference to clock-names in
clock-output-names property description (Mark).
 ---
  .../devicetree/bindings/media/samsung-fimc.txt |   44 
 +---
  1 file changed, 29 insertions(+), 15 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt 
 b/Documentation/devicetree/bindings/media/samsung-fimc.txt
 index 96312f6..922d6f8 100644
 --- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
 +++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
 @@ -15,11 +15,21 @@ Common 'camera' node
 
  Required properties:
 
 -- compatible : must be samsung,fimc, simple-bus

While it was already the case, why is samsung,fimc also a simple bus?

If it has any properties which are required for the correct use of the
child nodes, it is _not_ a simple-bus.

 -- clocks : list of clock specifiers, corresponding to entries in
 -   the clock-names property;
 -- clock-names: must contain sclk_cam0, sclk_cam1, pxl_async0,
 -   pxl_async1 entries, matching entries in the clocks property.
 +- compatible: must be samsung,fimc, simple-bus
 +- clocks: list of clock specifiers, corresponding to entries in
 +  the clock-names property;
 +- clock-names : must contain sclk_cam0, sclk_cam1, pxl_async0,
 +  pxl_async1 entries, matching entries in the clocks property.
 +
 +- #clock-cells: from the common clock bindings (../clock/clock-bindings.txt),
 +  must be 1. A clock provider is associated with the 'camera' node and it 
 should
 +  be referenced by external sensors that use clocks provided by the SoC on
 +  CAM_*_CLKOUT pins. The clock specifier cell stores an index of a clock.
 +  The indices are 0, 1 for CAM_A_CLKOUT, CAM_B_CLKOUT clocks respectively.
 +
 +- clock-output-names: from the common clock bindings, should contain names of
 +  clocks registered by the camera subsystem corresponding to CAM_A_CLKOUT,
 +  CAM_B_CLKOUT output clocks respectively.

And if we're adding more stuff required by child nodes it's definitely
not a simple-bus.

Get rid of simple-bus. Get the driver for samsung,fimc to probe its
children once it has set up.

Apologies for the late reply, and sorry to have to be negative on this.

Thanks,
Mark.

 
  The pinctrl bindings defined in ../pinctrl/pinctrl-bindings.txt must be used
  to define a required pinctrl state named default and optional pinctrl 
 states:
 @@ -32,6 +42,7 @@ way around.
 
  The 'camera' node must include at least one 'fimc' child node.
 
 +
  'fimc' device nodes
  ---
 
 @@ -88,8 +99,8 @@ port nodes specifies data input - 0, 1 indicates input A, B 
 respectively.
 
  Optional properties
 
 -- samsung,camclk-out : specifies clock output for remote sensor,
 -0 - CAM_A_CLKOUT, 1 - CAM_B_CLKOUT;
 +- samsung,camclk-out (deprecated) : specifies clock output for remote sensor,
 +  0 - CAM_A_CLKOUT, 1 - CAM_B_CLKOUT;
 
  Image sensor nodes
  --
 @@ -97,8 +108,6 @@ Image sensor nodes
  The sensor device nodes should be added to their control bus controller (e.g.
  I2C0) nodes and linked to a port node in the csis or the parallel-ports node,
  using the common video interfaces bindings, defined in video-interfaces.txt.
 -The implementation of this bindings requires clock-frequency property to be
 -present in the sensor device nodes.
 
  Example:
 
 @@ -114,7 +123,7 @@ Example:
   vddio-supply = ...;
 
   clock-frequency = 2400;
 - clocks = ...;
 + clocks = camera 1;
   clock-names = mclk;
 
   port {
 @@ -135,7 +144,7 @@ Example:
   vddio-supply = ...;
 
   clock-frequency = 2400;
 

Re: [PATCH v4 02/10] Documentation: dt: Add DT binding documentation for S5C73M3 camera

[Mark Rutland]

;
 + ...
 + };
 +};

Otherwise I don't see anything problematic about the binding.

Acked-by: Mark Rutland mark.rutl...@arm.com

Cheers,
Mark.
--
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

Re: [PATCH v4 03/10] Documentation: devicetree: Update Samsung FIMC DT binding

[Mark Rutland]

On Thu, Feb 20, 2014 at 07:40:30PM +, Sylwester Nawrocki wrote:
 This patch documents following updates of the Exynos4 SoC camera subsystem
 devicetree binding:
  - addition of #clock-cells property to 'camera' node - the #clock-cells
property is needed when the sensor sub-devices use clock provided by
the camera host interface;
  - addition of an optional clock-output-names property;
  - change of the clock-frequency at image sensor node from mandatory to
an optional property - there should be no need to require this property
by the camera host device binding, a default frequency value can ofen
be used;
  - addition of a requirement of specific order of values in clocks/
clock-names properties, so the first two entry in the clock-names
property can be used as parent clock names for the camera master
clock provider.  It happens all in-kernel dts files list the clock
in such order, thus there should be no regression as far as in-kernel
dts files are concerned.

I'm not sure I follow the reasoning here. Why does this matter? Why can
child nodes not get these by name if they have to?

 
 Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com
 Acked-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
  .../devicetree/bindings/media/samsung-fimc.txt |   36 
 +++-
  1 file changed, 28 insertions(+), 8 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt 
 b/Documentation/devicetree/bindings/media/samsung-fimc.txt
 index 96312f6..1a5820d 100644
 --- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
 +++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
 @@ -20,6 +20,7 @@ Required properties:
 the clock-names property;
  - clock-names: must contain sclk_cam0, sclk_cam1, pxl_async0,
 pxl_async1 entries, matching entries in the clocks property.
 +   First two entries must be sclk_cam0, sclk_cam1.

I don't think this is a good idea.

  
  The pinctrl bindings defined in ../pinctrl/pinctrl-bindings.txt must be used
  to define a required pinctrl state named default and optional pinctrl 
 states:
 @@ -32,6 +33,22 @@ way around.
  
  The 'camera' node must include at least one 'fimc' child node.
  
 +Optional properties (*:

Is that a smiley face?

 +
 +- #clock-cells: from the common clock bindings (../clock/clock-bindings.txt),
 +  must be 1. A clock provider is associated with the 'camera' node and it 
 should
 +  be referenced by external sensors that use clocks provided by the SoC on
 +  CAM_*_CLKOUT pins. The clock specifier cell stores an index of a clock.
 +  The indices are 0, 1 for CAM_A_CLKOUT, CAM_B_CLKOUT clocks respectively.
 +
 +- clock-output-names: from the common clock bindings, should contain names of
 +  clocks registered by the camera subsystem corresponding to CAM_A_CLKOUT,
 +  CAM_B_CLKOUT output clocks, in this order. Parent clock of these clocks are
 +  specified be first two entries of the clock-names property.

Do you need this?

That's not how clock-names is supposed to work. The clock-names property
is for the names of the _input_ clock lines on the device, not the
output names on whichever parent clock they came from.

Any clock-names property description should define absolutely the set of
names. As this does not, NAK.

 +
 +(* #clock-cells and clock-output-names are mandatory properties if external
 +image sensor devices reference 'camera' device node as a clock provider.

s/(*/Note:/

 +
  'fimc' device nodes
  ---
  
 @@ -97,8 +114,8 @@ Image sensor nodes
  The sensor device nodes should be added to their control bus controller (e.g.
  I2C0) nodes and linked to a port node in the csis or the parallel-ports node,
  using the common video interfaces bindings, defined in video-interfaces.txt.
 -The implementation of this bindings requires clock-frequency property to be
 -present in the sensor device nodes.
 +An optional clock-frequency property needs to be present in the sensor device
 +nodes. Default value when this property is not present is 24 MHz.

s/needs to/should/ ?

What is this the frequency of?

Thanks,
Mark.
--
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

Re: [PATCH v4 07/10] exynos4-is: Add clock provider for the SCLK_CAM clock outputs

[Mark Rutland]

On Thu, Feb 20, 2014 at 07:40:34PM +, Sylwester Nawrocki wrote:
 This patch adds clock provider so the the SCLK_CAM0/1 output clocks
 can be accessed by image sensor devices through the clk API.
 
 Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com
 Acked-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
 Changes since v3:
  - use clock-output-names DT property instead of hard coding names
of registered clocks in the driver; first two entries of the
clock-names property value are used to specify parent clocks of
cam_{a,b}_clkout clocks, rather than hard coding it to sclk_cam{0,1}
in the driver.
  - addressed issues pointed out in review by Mauro.
 
 Changes since v2:
  - use 'camera' DT node drirectly as clock provider node, rather than
   creating additional clock-controller child node.
  - DT binding documentation moved to a separate patch.
 ---
  drivers/media/platform/exynos4-is/media-dev.c |  110 
 +
  drivers/media/platform/exynos4-is/media-dev.h |   19 -
  2 files changed, 128 insertions(+), 1 deletion(-)

[...]

 +static int fimc_md_register_clk_provider(struct fimc_md *fmd)
 +{
 + struct cam_clk_provider *cp = fmd-clk_provider;
 + struct device *dev = fmd-pdev-dev;
 + int i, ret;
 +
 + for (i = 0; i  ARRAY_SIZE(cp-clks); i++) {
 + struct cam_clk *camclk = cp-camclk[i];
 + struct clk_init_data init;
 +
 + ret = of_property_read_string_index(dev-of_node,
 + clock-output-names, i, init.name);

Are there not well-defined names for the clock outputs of the block?

 + if (ret  0)
 + break;
 +
 + ret = of_property_read_string_index(dev-of_node,
 + clock-names, i, init.parent_names);

This shouldn't be a parent name. It should be the input line name.

I don't think this makes sense.

Why do you need the name of the parent clock?

 + if (ret  0)
 + break;
 +
 + init.num_parents = 1;
 + init.ops = cam_clk_ops;
 + init.flags = CLK_SET_RATE_PARENT;
 + camclk-hw.init = init;
 + camclk-fmd = fmd;
 +
 + cp-clks[i] = clk_register(NULL, camclk-hw);
 + if (IS_ERR(cp-clks[i])) {
 + dev_err(dev, failed to register clock: %s (%ld)\n,
 + init.name, PTR_ERR(cp-clks[i]));
 + ret = PTR_ERR(cp-clks[i]);
 + goto err;
 + }
 + cp-num_clocks++;
 + }
 +
 + if (cp-num_clocks == 0) {
 + dev_warn(dev, clk provider not registered\n);
 + return 0;
 + }
 +
 + cp-clk_data.clks = cp-clks;
 + cp-clk_data.clk_num = cp-num_clocks;
 + cp-of_node = dev-of_node;
 + ret = of_clk_add_provider(dev-of_node, of_clk_src_onecell_get,
 +   cp-clk_data);

Are _all_ of the input clock lines available to children in hardware?

The binding and commit message(s) implied only two clocks were, so
what's the point in exporting clocks which aren't available?

Thanks,
Mark.
--
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

Re: [PATCH] V4L: s5k6a3: Add DT binding documentation

[Mark Rutland]

On Tue, Feb 18, 2014 at 03:43:11PM +, Sylwester Nawrocki wrote:
 On 22/12/13 22:27, Sylwester Nawrocki wrote:
  This patch adds DT binding documentation for the Samsung S5K6A3(YX)
  raw image sensor.
  
  Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com
  Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
  ---
  This patch adds missing documentation [1] for the samsung,s5k6a3
  compatible. Rob, can you please merge it through your tree if it 
  looks OK ?
 
 Anyone cares to Ack this patch so it can be merged through the media
 tree ?

It looks fine to me:

Acked-by: Mark Rutland mark.rutl...@arm.com

Thanks,
Mark.

 
  Thanks,
  Sylwester
  
  [1] http://www.spinics.net/lists/devicetree/msg10693.html
  
  Changes since v3 (https://linuxtv.org/patch/20429):
   - rephrased 'clocks' and 'clock-names' properties' description.
  ---
   .../devicetree/bindings/media/samsung-s5k6a3.txt   |   33 
  
   1 files changed, 33 insertions(+), 0 deletions(-)
   create mode 100644 
  Documentation/devicetree/bindings/media/samsung-s5k6a3.txt
  
  diff --git a/Documentation/devicetree/bindings/media/samsung-s5k6a3.txt 
  b/Documentation/devicetree/bindings/media/samsung-s5k6a3.txt
  new file mode 100644
  index 000..cce01e8
  --- /dev/null
  +++ b/Documentation/devicetree/bindings/media/samsung-s5k6a3.txt
  @@ -0,0 +1,33 @@
  +Samsung S5K6A3(YX) raw image sensor
  +-
  +
  +S5K6A3(YX) is a raw image sensor with MIPI CSI-2 and CCP2 image data 
  interfaces
  +and CCI (I2C compatible) control bus.
  +
  +Required properties:
  +
  +- compatible   : samsung,s5k6a3;
  +- reg  : I2C slave address of the sensor;
  +- svdda-supply : core voltage supply;
  +- svddio-supply: I/O voltage supply;
  +- afvdd-supply : AF (actuator) voltage supply;
  +- gpios: specifier of a GPIO connected to the RESET pin;
  +- clocks   : should contain list of phandle and clock specifier pairs
  + according to common clock bindings for the clocks described
  + in the clock-names property;
  +- clock-names  : should contain extclk entry for the sensor's EXTCLK 
  clock;
  +
  +Optional properties:
  +
  +- clock-frequency : the frequency at which the extclk clock should be
  +   configured to operate, in Hz; if this property is not
  +   specified default 24 MHz value will be used.
  +
  +The common video interfaces bindings (see video-interfaces.txt) should be
  +used to specify link to the image data receiver. The S5K6A3(YX) device
  +node should contain one 'port' child node with an 'endpoint' subnode.
  +
  +Following properties are valid for the endpoint node:
  +
  +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in
  +  video-interfaces.txt.  The sensor supports only one data lane.
 
 
--
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

Re: [PATCH v2 06/15] dt: binding: add binding for ImgTec IR block

[Mark Rutland]

Hi Rob,

On Fri, Feb 07, 2014 at 02:33:27PM +, Rob Herring wrote:
 On Thu, Feb 6, 2014 at 8:41 AM, James Hogan james.ho...@imgtec.com wrote:
  Hi Rob,
 
  On 06/02/14 14:33, Rob Herring wrote:
  On Fri, Jan 17, 2014 at 7:58 AM, James Hogan james.ho...@imgtec.com 
  wrote:
  +Required properties:
  +- compatible:  Should be img,ir1
 
  Kind of short for a name. I don't have anything much better, but how
  about img,ir-rev1.
 
  Okay, that sounds reasonable.
 
  +Optional properties:
  +- clocks:  List of clock specifiers as described in standard
  +   clock bindings.
  +- clock-names: List of clock names corresponding to the clocks
  +   specified in the clocks property.
  +   Accepted clock names are:
  +   core: Core clock (defaults to 32.768KHz if 
  omitted).
  +   sys:  System side (fast) clock.
  +   mod:  Power modulation clock.
 
  You need to define the order of clocks including how they are
  interpreted with different number of clocks (not relying on the name).
 
  Would it be sufficient to specify that clock-names is required if
  clocks is provided (i.e. unnamed clocks aren't used), or is there some
  other reason that clock-names shouldn't be relied upon?
 
 irq-names, reg-names, clock-names, etc. are considered optional to
 their associated property and the order is supposed to be defined.
 clock-names is a bit different in that clk_get needs a name, so it
 effectively is required by Linux when there is more than 1 clock.
 Really, we should fix Linux.

If they're optional then you can't handle optional entries (i.e.  when
nothing's wired to an input), and this is counter to the style I've been
recommending to people (defining clocks in terms of clock-names).

I really don't see the point in any *-names property if they don't
define the list and allow for optional / reordered lists. Why does the
order have to be fixed rather than using the -names properties? It's
already a de-facto standard.

 
 Regardless, my other point is still valid. A given h/w block has a
 fixed number of clocks. You may have them all connected to the same
 source in some cases, but that does not change the number of inputs.
 Defining what are the valid combinations needs to be done. Seems like
 this could be:
 
 none - default to 32KHz
 core - only a baud clock
 core, sys, mod - all clocks

For more complex IP blocks you might have more inputs than you actually
have clocks wired to.

How do you handle an unwired input in the middle of the list, or a new
revision of the IP block that got rid of the first clock input from the
list but is otherwise compatible?

I really don't think it makes sense to say that clock-names doesn't
define the list.

Thanks,
Mark.
--
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

Re: [PATCH v10 1/2] [media] exynos5-is: Adds DT binding documentation

[Mark Rutland]

On Mon, Feb 03, 2014 at 10:13:55AM +, Arun Kumar K wrote:
 Hi Mark,

Hi Arun,

 
 This patch and hence a full series of 13 patches is waiting for a long time 
 now
 due to your missing ack on this DT binding patch.
 I have addressed your review comments given on earlier version -
 http://www.spinics.net/lists/devicetree/msg11550.html
 
 Please check this and give an ack if it is fine to be merged.

Apologies for the delay.

As far as I can tell this looks ok:

Acked-by: Mark Rutland mark.rutl...@arm.com

 
 Regards
 Arun
 
 On Fri, Dec 13, 2013 at 10:42 AM, Arun Kumar K arun...@samsung.com wrote:
  From: Shaik Ameer Basha shaik.am...@samsung.com
 
  The patch adds the DT binding doc for exynos5 SoC camera
  subsystem.
 
  Signed-off-by: Shaik Ameer Basha shaik.am...@samsung.com
  Signed-off-by: Arun Kumar K arun...@samsung.com
  ---
   .../bindings/media/exynos5250-camera.txt   |  136 
  
   1 file changed, 136 insertions(+)
   create mode 100644 
  Documentation/devicetree/bindings/media/exynos5250-camera.txt
 
  diff --git a/Documentation/devicetree/bindings/media/exynos5250-camera.txt 
  b/Documentation/devicetree/bindings/media/exynos5250-camera.txt
  new file mode 100644
  index 000..0c36bc4
  --- /dev/null
  +++ b/Documentation/devicetree/bindings/media/exynos5250-camera.txt
  @@ -0,0 +1,136 @@
  +Samsung EXYNOS5 SoC Camera Subsystem
  +
  +
  +The Exynos5 SoC Camera subsystem comprises of multiple sub-devices
  +represented by separate device tree nodes. Currently this includes: 
  FIMC-LITE,
  +MIPI CSIS and FIMC-IS.
  +
  +The sub-device nodes are referenced using phandles in the common 'camera' 
  node
  +which also includes common properties of the whole subsystem not really
  +specific to any single sub-device, like common camera port pins or the 
  common
  +camera bus clocks.
  +
  +Common 'camera' node
  +
  +
  +Required properties:
  +
  +- compatible   : must be samsung,exynos5250-fimc
  +- clocks   : list of phandles and clock specifiers, 
  corresponding
  + to entries in the clock-names property
  +- clock-names  : must contain sclk_bayer entry
  +- samsung,csis : list of phandles to the mipi-csis device nodes
  +- samsung,fimc-lite: list of phandles to the fimc-lite device nodes
  +- samsung,fimc-is  : phandle to the fimc-is device node
  +
  +The pinctrl bindings defined in ../pinctrl/pinctrl-bindings.txt must be 
  used
  +to define a required pinctrl state named default.
  +
  +'parallel-ports' node
  +-
  +
  +This node should contain child 'port' nodes specifying active parallel 
  video
  +input ports. It includes camera A, camera B and RGB bay inputs.
  +'reg' property in the port nodes specifies the input type:
  + 1 - parallel camport A
  + 2 - parallel camport B
  + 5 - RGB camera bay
  +
  +3, 4 are for MIPI CSI-2 bus and are already described in 
  samsung-mipi-csis.txt
  +
  +Required properties:
  +
  +For describing the input type in the child nodes, the following properties
  +have to be present in the parallel-ports node:
  +- #address-cells: Must be 1
  +- #size-cells: Must be 0
  +
  +Image sensor nodes
  +--
  +
  +The sensor device nodes should be added to their control bus controller 
  (e.g.
  +I2C0) nodes and linked to a port node in the csis or the parallel-ports 
  node,
  +using the common video interfaces bindings, defined in 
  video-interfaces.txt.
  +
  +Example:
  +
  +   aliases {
  +   fimc-lite0 = fimc_lite_0
  +   };
  +
  +   /* Parallel bus IF sensor */
  +   i2c_0: i2c@1386 {
  +   s5k6aa: sensor@3c {
  +   compatible = samsung,s5k6aafx;
  +   reg = 0x3c;
  +   vddio-supply = ...;
  +
  +   clock-frequency = 2400;
  +   clocks = ...;
  +   clock-names = mclk;
  +
  +   port {
  +   s5k6aa_ep: endpoint {
  +   remote-endpoint = fimc0_ep;
  +   bus-width = 8;
  +   hsync-active = 0;
  +   vsync-active = 1;
  +   pclk-sample = 1;
  +   };
  +   };
  +   };
  +   };
  +
  +   /* MIPI CSI-2 bus IF sensor */
  +   s5c73m3: sensor@1a {
  +   compatible = samsung,s5c73m3;
  +   reg = 0x1a;
  +   vddio-supply = ...;
  +
  +   clock-frequency = 2400;
  +   clocks = ...;
  +   clock-names = mclk;
  +
  +   port {
  +   s5c73m3_1: endpoint {
  +   data-lanes = 1 2 3 4

Re: [PATCH v9] s5k5baf: add camera sensor driver

[Mark Rutland]

On Wed, Dec 04, 2013 at 11:17:43PM +, Sylwester Nawrocki wrote:
 On 10/31/2013 04:29 PM, Andrzej Hajda wrote:
  Driver for Samsung S5K5BAF UXGA 1/5 2M CMOS Image Sensor
  with embedded SoC ISP.
  The driver exposes the sensor as two V4L2 subdevices:
  - S5K5BAF-CIS - pure CMOS Image Sensor, fixed 1600x1200 format,
 no controls.
  - S5K5BAF-ISP - Image Signal Processor, formats up to 1600x1200,
 pre/post ISP cropping, downscaling via selection API, controls.
 
  Signed-off-by: Sylwester Nawrockis.nawro...@samsung.com
  Signed-off-by: Andrzej Hajdaa.ha...@samsung.com
  Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com
  ---
  Hi,
 
  This is the 9th iteration of the patch.
  In this iteration 'binary' blobs from source
  file have been moved to separate firmware file.
  Firmware file will be uploaded to appropriate
  repository.
 [...]
  v9
  - patch, ccm and cis configuration blobs moved to
 firmware set files,
  - minor improvements of bindings,
  - cosmetic changes

 Hi Mark,

Hi Sylwester,


 What do you think about this DT binding now, could we have your Ack ?

Other than a minor nit below, the binding looks fine to me.

With the fixed, for the binding:

Acked-by: Mark Rutland mark.rutl...@arm.com

 I think we still need to move the DT binding into a separate patch.

If you're going to post the patch again, then please do split the
binding into a separate patch.

[...]

  diff --git a/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt 
  b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
  new file mode 100644
  index 000..23ebe0f
  --- /dev/null
  +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
  @@ -0,0 +1,57 @@
  +Samsung S5K5BAF UXGA 1/5 2M CMOS Image Sensor with embedded SoC ISP
  +
  +
  +Required properties:
  +
  +- compatible   : samsung,s5k5baf;
  +- reg  : I2C slave address of the sensor;
  +- vdda-supply  : analog power supply 2.8V (2.6V to 3.0V);
  +- vddreg-supply: regulator input power supply 1.8V (1.7V to 1.9V)
  + or 2.8V (2.6V to 3.0);
  +- vddio-supply : I/O power supply 1.8V (1.65V to 1.95V)
  + or 2.8V (2.5V to 3.1V);
  +- stbyn-gpios  : GPIO connected to STDBYN pin;
  +- rstn-gpios   : GPIO connected to RSTN pin;
  +- clocks   : clock-specifiers (per the common clock bindings) for the
  + clocks described in clock-names;

Clocks are referred to by phandle + clock-specifier pairs rather than
just clock-specifiers, it would be nice to fix up the terminology here.

Thanks,
Mark.
--
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

Re: [early RFC] Device Tree bindings for OMAP3 Camera Subsystem

[Mark Rutland]

On Sun, Nov 03, 2013 at 10:03:15PM +, Sebastian Reichel wrote:
 Hi,
 
 This is an early RFC for omap3isp DT support. For now i just created a 
 potential DT
 binding documentation based on the existing platform data:
 
 Binding for the OMAP3 Camera subsystem with the image signal processor (ISP) 
 feature.
 
 omap3isp node
 -
 
 Required properties:
 
 - compatible  : should be ti,omap3isp for OMAP3;
 - reg : physical addresses and length of the registers set;
 - clocks  : list of clock specifiers, corresponding to entries in
 clock-names property;
 - clock-names : must contain cam_ick, cam_mclk, csi2_96m_fck,
 l3_ick entries, matching entries in the clocks property;
 - interrupts  : must contain mmu interrupt;
 - ti,iommu: phandle to isp mmu;

s/;/./ (or s/;//)

 
 Optional properties:
 
 - VDD_CSIPHY1-supply  : regulator for csi phy1
 - VDD_CSIPHY2-supply  : regulator for csi phy2

I'd make these lower-case. Upper case is unusual, and lower-case is
preferred.

 - ti,isp-xclk-1   : device(s) attached to ISP's first external 
 clock
 - ti,isp-xclk-2   : device(s) attached to ISP's second external 
 clock

If the ISP is acting as a clock controller, it should have #clock-cells,
and export clocks to the consumers. They can in turn refer to th ISP via
the standard clocks property.

 
 device-group subnode
 
 
 Required properties:
 - ti,isp-interface-type   : Integer describing the interface type, one of 
 the following
* 0 = ISP_INTERFACE_PARALLEL
* 1 = ISP_INTERFACE_CSI2A_PHY2
* 2 = ISP_INTERFACE_CCP2B_PHY1
* 3 = ISP_INTERFACE_CCP2B_PHY2
* 4 = ISP_INTERFACE_CSI2C_PHY1

Are these PHYs always present?

Are they external to the ISP?

It's not possible for several of these to be valid simultaneously?

 - ti,isp-devices  : Array of phandles to devices connected via the 
 interface

Which devices are these? This looks backwards to me...

 - One of the following configuration nodes (depending on 
 ti,isp-interface-type)
  - ti,ccp2-bus-cfg: CCP2 bus configuration (needed for ISP_INTERFACE_CCP*)
  - ti,parallel-bus-cfg: PARALLEL bus configuration (needed for 
 ISP_INTERFACE_PARALLEL)
  - ti,csi2-bus-cfg: CSI bus configuration (needed for ISP_INTERFACE_CSI*)
 
 ccp2-bus-cfg subnode
 
 
 Required properties:
 - ti,video-port-clock-divisor : integer; used for video port output clock 
 control
 
 Optional properties:
 - ti,inverted-clock   : boolean; clock/strobe signal is inverted
 - ti,enable-crc   : boolean; enable crc checking

Why can't this be a run-time option?

 - ti,ccp2-mode-mipi   : boolean; port is used in MIPI-CSI1 mode 
 (default: CCP2 mode)
 - ti,phy-layer-is-strobe  : boolean; use data/strobe physical layer 
 (default: data/clock physical layer)
 - ti,data-lane-configuration  : integer array with position and polarity 
 information for lane 1 and 2
 - ti,clock-lane-configuration : integer array with position and polarity 
 information for clock lane

In what precise format?

 
 parallel-bus-cfg subnode
 
 
 Required properties:
 - ti,data-lane-shift  : integer; shift data lanes by 
 this amount
 
 Optional properties:
 - ti,clock-falling-edge   : boolean; sample on 
 falling edge (default: rising edge)
 - ti,horizontal-synchronization-active-low: boolean; default: active high
 - ti,vertical-synchronization-active-low  : boolean; default: active high
 - ti,data-polarity-ones-complement: boolean; data polarity is 
 one's complement
 
 csi2-bus-cfg subnode
 
 
 Required properties:
 - ti,video-port-clock-divisor : integer; used for video port output clock 
 control
 
 Optional properties:
 - ti,data-lane-configuration  : integer array with position and polarity 
 information for lane 1 and 2
 - ti,clock-lane-configuration : integer array with position and polarity 
 information for clock lane
 - ti,enable-crc   : boolean; enable crc checking

Similarly, run-time selectable?

 
 Example for Nokia N900
 --
 
 omap3isp: isp@480BC000 {
   compatible = ti,omap3isp;
   reg = 
   /* OMAP3430+ */
   0x480BC000 0x070/* base */
   0x480BC100 0x078/* cbuf */
   0x480BC400 0x1F0/* cpp2 */
   0x480BC600 0x0A8/* ccdc */
   0x480BCA00 0x048/* hist */
   0x480BCC00 0x060/* h3a  */
   0x480BCE00 0x0A0/* prev */
   0x480BD000 0x0AC/* resz */
   0x480BD200 0x0FC/* sbl  */
   0x480BD400 0x070/* mmu  */
   ;

The binding implied a single contiguous reg entry. These look like they
are in a contiguous register space, is it not possible to describe 

Re: [PATCH v9 01/13] [media] exynos5-is: Adding media device driver for exynos5

[Mark Rutland]

On Fri, Sep 27, 2013 at 11:59:06AM +0100, Arun Kumar K wrote:
 From: Shaik Ameer Basha shaik.am...@samsung.com
 
 This patch adds support for media device for EXYNOS5 SoCs.
 The current media device supports the following ips to connect
 through the media controller framework.
 
[...]

 diff --git a/Documentation/devicetree/bindings/media/exynos5250-camera.txt 
 b/Documentation/devicetree/bindings/media/exynos5250-camera.txt
 new file mode 100644
 index 000..09420ba
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/media/exynos5250-camera.txt
 @@ -0,0 +1,126 @@
 +Samsung EXYNOS5 SoC Camera Subsystem
 +
 +
 +The Exynos5 SoC Camera subsystem comprises of multiple sub-devices
 +represented by separate device tree nodes. Currently this includes: 
 FIMC-LITE,
 +MIPI CSIS and FIMC-IS.
 +
 +The sub-device nodes are referenced using phandles in the common 'camera' 
 node
 +which also includes common properties of the whole subsystem not really
 +specific to any single sub-device, like common camera port pins or the common
 +camera bus clocks.
 +
 +Common 'camera' node
 +
 +
 +Required properties:
 +
 +- compatible   : must be samsung,exynos5250-fimc
 +- clocks   : list of clock specifiers, corresponding to entries 
 in
 +  the clock-names property

Minor nit: clocks are references by phandle + clock-specifier pairs, as
the clock-specifier is separate from the phandle to the clock. 

 +- clock-names  : must contain sclk_bayer entry
 +- samsung,csis : list of phandles to the mipi-csis device nodes
 +- samsung,fimc-lite: list of phandles to the fimc-lite device nodes
 +- samsung,fimc-is  : phandle to the fimc-is device node
 +
 +The pinctrl bindings defined in ../pinctrl/pinctrl-bindings.txt must be used
 +to define a required pinctrl state named default.
 +
 +'parallel-ports' node
 +-
 +
 +This node should contain child 'port' nodes specifying active parallel video
 +input ports. It includes camera A, camera B and RGB bay inputs.
 +'reg' property in the port nodes specifies the input type:
 + 1 - parallel camport A
 + 2 - parallel camport B
 + 5 - RGB camera bay
 +
 +3, 4 are for MIPI CSI-2 bus and are already described in 
 samsung-mipi-csis.txt

I believe the parallel ports node must have #address-cells and
#size-cells defined for the child nodes' reg properties to be
meaningful. Judging by the examples and code, it seems you expect
#address-cells = 1 and #size-cells = 0. It would be nice to have
that described.

Cheers,
Mark.
--
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

Re: [PATCH v9 02/13] [media] exynos5-fimc-is: Add Exynos5 FIMC-IS device tree bindings documentation

[Mark Rutland]

On Fri, Sep 27, 2013 at 11:59:07AM +0100, Arun Kumar K wrote:
 The patch adds the DT binding documentation for Samsung
 Exynos5 SoC series imaging subsystem (FIMC-IS).
 
 Signed-off-by: Arun Kumar K arun...@samsung.com
 Reviewed-by: Sylwester Nawrocki s.nawro...@samsung.com
 ---
  .../devicetree/bindings/media/exynos5-fimc-is.txt  |   84 
 
  1 file changed, 84 insertions(+)
  create mode 100644 
 Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
 
 diff --git a/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt 
 b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
 new file mode 100644
 index 000..0525417
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
 @@ -0,0 +1,84 @@
 +Samsung EXYNOS5 SoC series Imaging Subsystem (FIMC-IS)
 +--
 +
 +The camera subsystem on Samsung Exynos5 SoC has some changes relative
 +to previous SoC versions. Exynos5 has almost similar MIPI-CSIS and
 +FIMC-LITE IPs but has a much improved version of FIMC-IS which can
 +handle sensor controls and camera post-processing operations. The
 +Exynos5 FIMC-IS has a dedicated ARM Cortex A5 processor, many
 +post-processing blocks (ISP, DRC, FD, ODC, DIS, 3DNR) and two
 +dedicated scalers (SCC and SCP).
 +
 +fimc-is node
 +
 +
 +Required properties:
 +
 +- compatible: must be samsung,exynos5250-fimc-is

s/must be/should contain/

 +- reg   : physical base address and size of the memory mapped
 +  registers
 +- interrupt-parent  : parent interrupt controller

Is this actually necessary? Is it not implicit in most cases?

 +- interrupts: fimc-is interrupt to the parent interrupt controller

- interrupts: interrupt-specifier for the sole interrupt generated by
  the device.

 +- clocks: list of clock specifiers, corresponding to entries in
 +  clock-names property

- clocks: A list of phandle + clock-specifier pairs corresponding to
  entries in clock-names

 +- clock-names   : must contain isp, mcu_isp, isp_div0, isp_div1,
 +  isp_divmpwm, mcu_isp_div0, mcu_isp_div1 entries,
 +  matching entries in the clocks property
 +- samsung,pmu   : phandle to the Power Management Unit (PMU) node
 +

It may be worth point out that #address-cells, #size-cells, and ranges
need to be present as approriate for mapping sub-nodes.

 +i2c-isp (ISP I2C bus controller) nodes
 +--
 +The i2c-isp node is defined as the child node of fimc-is.
 +
 +Required properties:
 +
 +- compatible : should be samsung,exynos4212-i2c-isp for Exynos4212,
 +   Exynos4412 and Exynos5250 SoCs

s/should be/should contain/

 +- reg: physical base address and length of the registers set
 +- clocks : must contain gate clock specifier for this controller
 +- clock-names: must contain i2c_isp entry

Please reword clocks to be in terms of clock-names, as above. e.g.

- clocks: A list of phandle + clock-specifier pairs corresponding to
  entries in clock-names
- clock-names: Should contain i2c_isp fot the gate clock

Otherwise, I think this looks ok.

Cheers,
Mark.
--
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

Re: [PATCH v10 01/12] exynos5-fimc-is: Add Exynos5 FIMC-IS device tree bindings documentation

[Mark Rutland]

Hi,

Apologies for the late reply. I have a few comments, but nothing major.

On Fri, Oct 18, 2013 at 06:37:28AM +0100, Arun Kumar K wrote:
 The patch adds the DT binding documentation for Samsung
 Exynos5 SoC series imaging subsystem (FIMC-IS).
 
 Signed-off-by: Arun Kumar K arun...@samsung.com
 Reviewed-by: Sylwester Nawrocki s.nawro...@samsung.com
 ---
  .../devicetree/bindings/media/exynos5-fimc-is.txt  |   84 
 
  1 file changed, 84 insertions(+)
  create mode 100644 
 Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
 
 diff --git a/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt 
 b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
 new file mode 100644
 index 000..0525417
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt
 @@ -0,0 +1,84 @@
 +Samsung EXYNOS5 SoC series Imaging Subsystem (FIMC-IS)
 +--
 +
 +The camera subsystem on Samsung Exynos5 SoC has some changes relative
 +to previous SoC versions. Exynos5 has almost similar MIPI-CSIS and
 +FIMC-LITE IPs but has a much improved version of FIMC-IS which can
 +handle sensor controls and camera post-processing operations. The
 +Exynos5 FIMC-IS has a dedicated ARM Cortex A5 processor, many
 +post-processing blocks (ISP, DRC, FD, ODC, DIS, 3DNR) and two
 +dedicated scalers (SCC and SCP).
 +
 +fimc-is node
 +
 +
 +Required properties:
 +
 +- compatible: must be samsung,exynos5250-fimc-is

s/must be/should contain/

 +- reg   : physical base address and size of the memory mapped
 +  registers
 +- interrupt-parent  : parent interrupt controller

I don't think this is actually required in all cases. It's a standard property
that people can add if it happens to be required in a particular instance.

 +- interrupts: fimc-is interrupt to the parent interrupt controller

Is this the only interrupt the device generates? If so:

- interrupts: interrupt-specifier for the fimc-is interrupt.

 +- clocks: list of clock specifiers, corresponding to entries in
 +  clock-names property
 +- clock-names   : must contain isp, mcu_isp, isp_div0, isp_div1,
 +  isp_divmpwm, mcu_isp_div0, mcu_isp_div1 entries,
 +  matching entries in the clocks property
 +- samsung,pmu   : phandle to the Power Management Unit (PMU) node
 +
 +i2c-isp (ISP I2C bus controller) nodes
 +--
 +The i2c-isp node is defined as the child node of fimc-is.

There are multiple of these, so how about the following instead:

The i2c-isp nodes should be children of the fimc-is node.

It might be worth pointing out that ranges, #address-cells, and #size-cells
should be present as appropriate.

 +
 +Required properties:
 +
 +- compatible : should be samsung,exynos4212-i2c-isp for Exynos4212,
 +   Exynos4412 and Exynos5250 SoCs

Similarly, s/should be/must contain/

 +- reg: physical base address and length of the registers set
 +- clocks : must contain gate clock specifier for this controller
 +- clock-names: must contain i2c_isp entry

I'd prefer clocks to be described as for the simc-is, with clock names being
something like:

- clock-names: should contain i2c_isp for the gate clock.

 +
 +For the i2c-isp node, it is required to specify a pinctrl state named 
 default,
 +according to the pinctrl bindings defined in ../pinctrl/pinctrl-bindings.txt.

I'd prefer a mention of pinctrl-0 and pinctrl-names, as that's what most other
bindings do.

Also, as this is described as required it should be in the example.

 +
 +Device tree nodes of the image sensors controlled directly by the FIMC-IS
 +firmware must be child nodes of their corresponding ISP I2C bus controller 
 node.
 +The data link of these image sensors must be specified using the common video
 +interfaces bindings, defined in video-interfaces.txt.

These don't seem to be in the example and probably should be.

Otherwise, this looks fine. With those points fixed up:

Acked-by: Mark Rutland mark.rutl...@arm.com

Thanks,
Mark.
--
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

Re: [PATCH v10 11/12] V4L: Add DT binding doc for s5k4e5 image sensor

[Mark Rutland]

On Fri, Oct 18, 2013 at 06:37:38AM +0100, Arun Kumar K wrote:
 S5K4E5 is a Samsung raw image sensor controlled via I2C.
 This patch adds the DT binding documentation for the same.
 
 Signed-off-by: Arun Kumar K arun...@samsung.com
 Reviewed-by: Sylwester Nawrocki s.nawro...@samsung.com
 ---
  .../devicetree/bindings/media/samsung-s5k4e5.txt   |   45 
 
  1 file changed, 45 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/media/samsung-s5k4e5.txt
 
 diff --git a/Documentation/devicetree/bindings/media/samsung-s5k4e5.txt 
 b/Documentation/devicetree/bindings/media/samsung-s5k4e5.txt
 new file mode 100644
 index 000..0fca087
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/media/samsung-s5k4e5.txt
 @@ -0,0 +1,45 @@
 +* Samsung S5K4E5 Raw Image Sensor
 +
 +S5K4E5 is a raw image sensor with maximum resolution of 2560x1920
 +pixels. Data transfer is carried out via MIPI CSI-2 port and controls
 +via I2C bus.
 +
 +Required Properties:
 +- compatible : must be samsung,s5k4e5

s/must be/should contain/

 +- reg: I2C device address
 +- reset-gpios: specifier of a GPIO connected to the RESET pin
 +- clocks : should contain the sensor's EXTCLK clock specifier, from
 +   the common clock bindings

I would reword this to reference clock-names so as to make the ordering
relationship explicit.

With that, as everything else looks sane:

Acked-by: Mark Rutland mark.rutl...@arm.com

Thanks
Mark.
--
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

Re: [PATCH RFC] media: rc: OF: Add Generic bindings for remote-control

[Mark Rutland]

On Wed, Oct 09, 2013 at 01:17:30PM +0100, srinivas kandagatla wrote:
   .../devicetree/bindings/media/remote-control.txt   |   31 
  
   1 files changed, 31 insertions(+), 0 deletions(-)
   create mode 100644 
  Documentation/devicetree/bindings/media/remote-control.txt
 
  diff --git a/Documentation/devicetree/bindings/media/remote-control.txt 
  b/Documentation/devicetree/bindings/media/remote-control.txt
  new file mode 100644
  index 000..901ea56
  --- /dev/null
  +++ b/Documentation/devicetree/bindings/media/remote-control.txt
  @@ -0,0 +1,31 @@
  +Generic device tree bindings for remote control.
  +
  +properties:
  +- compatible: Can contain any remote control driver compatible 
  string.
  +  example: st-comms-irb, gpio-ir-receiver.
 
  This is more generic than remote control, could this not just be left
  for the specific binding to describe?
 
  +- reg: Base physical address of the controller and length of 
  memory
  +  mapped region.
 
  What if it's on a bus that isn't memory mapped (e.g. i2c, SPI)?
 
  +- interrupts: Interrupt-specifier for the sole interrupt 
  generated by
  +  the device. The interrupt specifier format depends on the
  +  interrupt controller parent. Iff the device supports 
  interrupts.
 
  What if it has multiple interrupts, and has interrupts-names?
 
 I think for properties like interrupts, reg can be left undocumented
 here and let the actual device bindings document on this.

If it has no particur meaning for the class of binding, leaving it to
the individual device bindings seems fine to me.

 
 
  It might be better to only describe the properties that relate
  specifically to remote controls, rather than listing all of the generic
  properties that device tree bidnings may have. That would match the
  style of the clock bindings.
 
  +- rx-mode: Can be infrared or uhf. rx-mode should be 
  present iff
  +  the rx pins are wired up.
 
  I'm unsure on this. What if the device has multiple receivers that can
  be independently configured? 
 
 Mauro C. had an option that this is not a real use-case and let's not
 overdesign the API, thinking on a possible scenario that may never happen.
 
 Do you still think that this use case should be considered in this
 discussion?

Given how simple a device we're attempting to describe, I'm not even
sure it makes sense to have a class of binding. We could leave this to
individual device bindings for the moment.

 
 
  Well, if a given remote controller hardware has more than one independent
  receiver (or transmitter), each one should have its own devnode.
  Likely, two entries at DT.
  
  Why? If an IP block happens to have support for N connections, that
  doesn't mean that each must be described individually. They likely share
  a bank of registers, and depending on the device they might not even be
  assigned consistently orgranised windows of that register bank.
  
 
  What if it supports something other than
  infrared or uhf? What if a device can only be wired up as
  infrared? 
 
 I think infrared and uhf covers all the use cases for remote controls.
 
 If the device only supports one mode. It can be documented in device
 specific bindings. something like phy-mode of ethernet bindings.

Given there's the possibility of a device supporting one mode, I think
it would make more sense to describe the *-mode properties in the
specific bindings which require them (though they may be identical).

 
  I'm not sure how generic these are, though we should certainly encourage
  bindings that can be described this way to be described in the same way.
 
  +- tx-mode: Can be infrared or uhf. tx-mode should be 
  present iff
  +  the tx pins are wired up.
 
  I have similar concerns here to those for the rx-mode property.
 
  +
  +Optional properties:
  +- linux,rc-map-name: Linux specific remote control map name. 
  Refer to
  +  include/media/rc-map.h for full list of maps.
 
  We shouldn't refer to Linux specifics (i.e. headers) in general in
  bindings. While it's possible to relax that a bit for linux,*
  properties, I'd prefer to explicitly list elements in the binding. That
  prevents the ABI from changing under our feet by someone altering what
  looks like a completely internal header file.
 
  Well, the remote controller keymaps at include/media/rc-map.h is just a
  bunch of string names, defined as macro to avoid duplicating those names
  everywhere, to avoid typos and to help some userspace parsing logic to get
  all in just one single place. I don't see why the same names couldn't be 
  used on any other OS using DT.
  
  To be used by another OS, they should be defined somewhere that's not
  subject to arbitrary changes at any time at the whim of Linux
  developers, without dt-related review.
  
  That's not to say we couldn't use strings the kernel happened to use.
  I'm saying that the names 

Re: [PATCH v8] s5k5baf: add camera sensor driver

[Mark Rutland]

  diff --git a/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt 
  b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
  new file mode 100644
  index 000..7704a1e
  --- /dev/null
  +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
  @@ -0,0 +1,58 @@
  +Samsung S5K5BAF UXGA 1/5 2M CMOS Image Sensor with embedded SoC ISP
  +
  +
  +Required properties:
  +
  +- compatible : samsung,s5k5baf;
  +- reg: I2C slave address of the sensor;
  +- vdda-supply: analog power supply 2.8V (2.6V to 3.0V);
  +- vddreg-supply  : regulator input power supply 1.8V (1.7V to 
  1.9V)
  +   or 2.8V (2.6V to 3.0);
  +- vddio-supply   : I/O power supply 1.8V (1.65V to 1.95V)
  +   or 2.8V (2.5V to 3.1V);
  +- stbyn-gpios: GPIO connected to STDBYN pin;
  +- rstn-gpios : GPIO connected to RSTN pin;
  +- clocks : the sensor's master clock specifier (from the common
  +   clock bindings);
  +- clock-names: must be mclk;
  I'd reword this slightly:
 
  - clocks: clock-specifiers (per the common clock bindings) for the
clocks described in clock-names
 OK
  - clock-names: should include mclk for the sensor's master clock
 IMHO it suggests there could be more than one clock, is it OK?

In general I'd prefer things to be described in this way so as to not
describe any requirements on the order of clocks in the binding (and to
suggest that clocks should be requested by name in code).

Future hardware variants could have multiple clocks that we need to add
to bindings, or it's possible we miss some clocks in the initial version
of a binding. In either case it's easier to extend the binding and code
if clocks are described by name and requested by name.

 
  +
  +Optional properties:
  +
  +- clock-frequency : the frequency at which the mclk clock should be
  +   configured to operate, in Hz; if this property is not
  +   specified default 24 MHz value will be used.
  +
  +The device node should contain one 'port' child node with one child 
  'endpoint'
  +node, according to the bindings defined in 
  Documentation/devicetree/bindings/
  +media/video-interfaces.txt. The following are properties specific to those
  +nodes.
  +
  +endpoint node
  +-
  +
  +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in
  +  video-interfaces.txt. This sensor doesn't support data lane remapping
  +  and physical lane indexes in subsequent elements of the array should
  +  have consecutive values.
  Do these need to start at 1, or may they have any initial value?
 After re-checking I have found some inconsistency in the specs,
 regarding lanes.
 Final conclusion is that sensor supports only one lane without re-mapping.
 I would then change description to:
 
 - data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in
   video-interfaces.txt. If present it should be 1 - the device supports 
 only one data lane
   without re-mapping.

OK. That sounds reasonable to me.

 
 
  +
  +Example:
  +
  +s5k5bafx@2d {
  +   compatible = samsung,s5k5baf;
  +   reg = 0x2d;
  +   vdda-supply = cam_io_en_reg;
  +   vddreg-supply = vt_core_15v_reg;
  +   vddio-supply = vtcam_reg;
  +   stbyn-gpios = gpl2 0 1;
  +   rstn-gpios = gpl2 1 1;
  +   clock-names = mclk;
  +   clocks = clock_cam 0;
  +   clock-frequency = 2400;
  +
  +   port {
  +   s5k5bafx_ep: endpoint {
  +   remote-endpoint = csis1_ep;
  +   data-lanes = 1;
  +   };
  +   };
  +};
  Otherwise, I think the binding looks fine.
 
  I took a quick skim over the driver and I have a few other comments.
 
  [...]
 
  +enum s5k5baf_gpio_id {
  +   STBY,
  +   RST,
  +   GPIO_NUM,
  I'd expect this to be NUM_GPIOS or MAX_GPIOS, as GPIO_NUM sounds like
  the index for a GPIO, rather than how many GPIOs there are.
 OK
 
  +};
  +
  +#define PAD_CIS 0
  +#define PAD_OUT 1
  +#define CIS_PAD_NUM 1
  +#define ISP_PAD_NUM 2
  Similarly here, I think NUM_*S or MAX_*S is preferable.
 OK
 
  [...]
 
  +static void s5k5baf_hw_patch(struct s5k5baf *state)
  +{
  +   static const u16 nseq_patch[] = {
  +   NSEQ(0x1668,
  +   0xb5fe, 0x0007, 0x683c, 0x687e, 0x1da5, 0x88a0, 0x2800, 
  0xd00b,
  +   0x88a8, 0x2800, 0xd008, 0x8820, 0x8829, 0x4288, 0xd301, 
  0x1a40,
  +   0xe000, 0x1a08, 0x9001, 0xe001, 0x2019, 0x9001, 0x4916, 
  0x466b,
  +   0x8a48, 0x8118, 0x8a88, 0x8158, 0x4814, 0x8940, 0x0040, 
  0x2103,
  +   0xf000, 0xf826, 0x88a1, 0x4288, 0xd908, 0x8828, 0x8030, 
  0x8868,
  +   0x8070, 0x88a8, 0x6038, 0xbcfe, 0xbc08, 0x4718, 0x88a9, 
  0x4288,
  +   0xd906, 0x8820, 0x8030, 0x8860, 0x8070, 0x88a0, 0x6038, 
  0xe7f2,
  +  

Re: [PATCH RFC] media: rc: OF: Add Generic bindings for remote-control

[Mark Rutland]

On Fri, Sep 27, 2013 at 02:47:19PM +0100, Mauro Carvalho Chehab wrote:
 Em Fri, 27 Sep 2013 12:34:58 +0100
 Mark Rutland mark.rutl...@arm.com escreveu:
 
  On Fri, Sep 27, 2013 at 10:33:11AM +0100, Srinivas KANDAGATLA wrote:
   From: Srinivas Kandagatla srinivas.kandaga...@st.com
   
   This patch attempts to collate generic bindings which can be used by
   the remote control hardwares. Currently the list is not long as there
   are only 2 drivers which are device tree'd.
   
   Mainly this patch tries to document few bindings used by ST IRB driver
   which can be generic as well. This document also add fews common
   bindings used by most of the drivers like, interrupts, regs, clocks and
   pinctrls.
   
   This document can also be holding place to describe generic bindings
   used in remote controls devices.
   
   Signed-off-by: Srinivas Kandagatla srinivas.kandaga...@st.com
   ---
   Hi All, 
   Following Stephen Warren's suggestions at 
   https://lkml.org/lkml/2013/9/24/452
   this patch is an attempt to document such generic bindings in a common
   document.
   
   This document currently collates all the generic bindings used with
   remote-controls and act as holding place to describe generic bindings for
   remote controls.
   
   Comments?
   
   Thanks,
   srini
   
.../devicetree/bindings/media/remote-control.txt   |   31 
   
1 files changed, 31 insertions(+), 0 deletions(-)
create mode 100644 
   Documentation/devicetree/bindings/media/remote-control.txt
   
   diff --git a/Documentation/devicetree/bindings/media/remote-control.txt 
   b/Documentation/devicetree/bindings/media/remote-control.txt
   new file mode 100644
   index 000..901ea56
   --- /dev/null
   +++ b/Documentation/devicetree/bindings/media/remote-control.txt
   @@ -0,0 +1,31 @@
   +Generic device tree bindings for remote control.
   +
   +properties:
   + - compatible: Can contain any remote control driver compatible string.
   +   example: st-comms-irb, gpio-ir-receiver.
  
  This is more generic than remote control, could this not just be left
  for the specific binding to describe?
  
   + - reg: Base physical address of the controller and length of memory
   +   mapped region.
  
  What if it's on a bus that isn't memory mapped (e.g. i2c, SPI)?
  
   + - interrupts: Interrupt-specifier for the sole interrupt generated by
   +   the device. The interrupt specifier format depends on the
   +   interrupt controller parent. Iff the device supports interrupts.
  
  What if it has multiple interrupts, and has interrupts-names?
  
  It might be better to only describe the properties that relate
  specifically to remote controls, rather than listing all of the generic
  properties that device tree bidnings may have. That would match the
  style of the clock bindings.
  
   + - rx-mode: Can be infrared or uhf. rx-mode should be present iff
   +   the rx pins are wired up.
  
  I'm unsure on this. What if the device has multiple receivers that can
  be independently configured? 
 
 Well, if a given remote controller hardware has more than one independent
 receiver (or transmitter), each one should have its own devnode.
 Likely, two entries at DT.

Why? If an IP block happens to have support for N connections, that
doesn't mean that each must be described individually. They likely share
a bank of registers, and depending on the device they might not even be
assigned consistently orgranised windows of that register bank.

 
  What if it supports something other than
  infrared or uhf? What if a device can only be wired up as
  infrared? 
 
 I would say that a hardware that has both infrared and uhf has actually
 two different devices. So, it should be mapped as two separate devnodes.

I would say that it is still one device, one which happens to have two
outputs. Just because we want two dev nodes does not mean the dt has to
be structured as two devices.

 
  I'm not sure how generic these are, though we should certainly encourage
  bindings that can be described this way to be described in the same way.
  
   + - tx-mode: Can be infrared or uhf. tx-mode should be present iff
   +   the tx pins are wired up.
  
  I have similar concerns here to those for the rx-mode property.
  
   +
   +Optional properties:
   + - linux,rc-map-name: Linux specific remote control map name. Refer to
   +   include/media/rc-map.h for full list of maps.
  
  We shouldn't refer to Linux specifics (i.e. headers) in general in
  bindings. While it's possible to relax that a bit for linux,*
  properties, I'd prefer to explicitly list elements in the binding. That
  prevents the ABI from changing under our feet by someone altering what
  looks like a completely internal header file.
 
 Well, the remote controller keymaps at include/media/rc-map.h is just a
 bunch of string names, defined as macro to avoid duplicating those names
 everywhere, to avoid typos and to help some userspace parsing logic to get
 all in just one

Re: [PATCH RFC] media: rc: OF: Add Generic bindings for remote-control

[Mark Rutland]

On Fri, Sep 27, 2013 at 10:33:11AM +0100, Srinivas KANDAGATLA wrote:
 From: Srinivas Kandagatla srinivas.kandaga...@st.com
 
 This patch attempts to collate generic bindings which can be used by
 the remote control hardwares. Currently the list is not long as there
 are only 2 drivers which are device tree'd.
 
 Mainly this patch tries to document few bindings used by ST IRB driver
 which can be generic as well. This document also add fews common
 bindings used by most of the drivers like, interrupts, regs, clocks and
 pinctrls.
 
 This document can also be holding place to describe generic bindings
 used in remote controls devices.
 
 Signed-off-by: Srinivas Kandagatla srinivas.kandaga...@st.com
 ---
 Hi All, 
 Following Stephen Warren's suggestions at https://lkml.org/lkml/2013/9/24/452
 this patch is an attempt to document such generic bindings in a common
 document.
 
 This document currently collates all the generic bindings used with
 remote-controls and act as holding place to describe generic bindings for
 remote controls.
 
 Comments?
 
 Thanks,
 srini
 
  .../devicetree/bindings/media/remote-control.txt   |   31 
 
  1 files changed, 31 insertions(+), 0 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/media/remote-control.txt
 
 diff --git a/Documentation/devicetree/bindings/media/remote-control.txt 
 b/Documentation/devicetree/bindings/media/remote-control.txt
 new file mode 100644
 index 000..901ea56
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/media/remote-control.txt
 @@ -0,0 +1,31 @@
 +Generic device tree bindings for remote control.
 +
 +properties:
 + - compatible: Can contain any remote control driver compatible string.
 +   example: st-comms-irb, gpio-ir-receiver.

This is more generic than remote control, could this not just be left
for the specific binding to describe?

 + - reg: Base physical address of the controller and length of memory
 +   mapped region.

What if it's on a bus that isn't memory mapped (e.g. i2c, SPI)?

 + - interrupts: Interrupt-specifier for the sole interrupt generated by
 +   the device. The interrupt specifier format depends on the
 +   interrupt controller parent. Iff the device supports interrupts.

What if it has multiple interrupts, and has interrupts-names?

It might be better to only describe the properties that relate
specifically to remote controls, rather than listing all of the generic
properties that device tree bidnings may have. That would match the
style of the clock bindings.

 + - rx-mode: Can be infrared or uhf. rx-mode should be present iff
 +   the rx pins are wired up.

I'm unsure on this. What if the device has multiple receivers that can
be independently configured? What if it supports something other than
infrared or uhf? What if a device can only be wired up as
infrared? 

I'm not sure how generic these are, though we should certainly encourage
bindings that can be described this way to be described in the same way.

 + - tx-mode: Can be infrared or uhf. tx-mode should be present iff
 +   the tx pins are wired up.

I have similar concerns here to those for the rx-mode property.

 +
 +Optional properties:
 + - linux,rc-map-name: Linux specific remote control map name. Refer to
 +   include/media/rc-map.h for full list of maps.

We shouldn't refer to Linux specifics (i.e. headers) in general in
bindings. While it's possible to relax that a bit for linux,*
properties, I'd prefer to explicitly list elements in the binding. That
prevents the ABI from changing under our feet by someone altering what
looks like a completely internal header file.

 + - pinctrl-names, pinctrl-0: The pincontrol settings to configure muxing
 +   properly for the device pins.
 + - clocks : phandle with clock-specifier pair for the device specified
 +   in compatible.

While devices may have these, they're also more general than remote
control devices. I'm not sure that they need to be listed here when they
need to be described fully in any binding that needs them anyway,
especially as this gives an impression that they are valid for bindings
that don't need them.

I think what we actually need to document is the process of creating a
binding in such a way as to encourage uniformity. Something like the
following steps:

1. Look to see if a binding already exists. If so, use it.

2. Is there a binding for a compatible device? If so, use/extend it.

3. Is there a binding for a similar (but incompatible) device? Use it as
   a template, possibly factor out portions into a class binding if
   those portions are truly general.

4. Is there a binding for the class of device? If so, build around that,
   possibly extending it.

5. If there's nothing relevant, create a binding aiming for as much
   commonality as possible with other devices of that class that may
   have bindings later.

Cheers,
Mark.

 +
 +example:
 +
 + rc: rc@fe518000 {
 +

Re: [PATCH v8] s5k5baf: add camera sensor driver

[Mark Rutland]

On Fri, Sep 06, 2013 at 11:31:06AM +0100, Andrzej Hajda wrote:
 Driver for Samsung S5K5BAF UXGA 1/5 2M CMOS Image Sensor
 with embedded SoC ISP.
 The driver exposes the sensor as two V4L2 subdevices:
 - S5K5BAF-CIS - pure CMOS Image Sensor, fixed 1600x1200 format,
   no controls.
 - S5K5BAF-ISP - Image Signal Processor, formats up to 1600x1200,
   pre/post ISP cropping, downscaling via selection API, controls.
 
 Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com
 Signed-off-by: Andrzej Hajda a.ha...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
 Hi,
 
 This is the 8th iteration of the patch.
 I have applied suggestions from Laurent, Sylwester and Mark, thanks.
 One exeception, I have left static struct v4l2_rect s5k5baf_cis_rect
 not const due to fact its address is passed to function which could
 modify its arguments, of course it never modifies s5k5baf_cis_rect.
 
 Regards
 Andrzej
 
 v8
 - improved description of data-lanes binding,
 - added algorithm caching,
 - added comments to functions,
 - video bus type checking moved to probe,
 - clk_get/put moved to probe,
 - moved streaming checking under mutex,
 - use proper functions for endian conversion,
 - probe returns -EPROBE_DEFER in case it cannot get clock,
 - v4l2_async_unregister_subdev is called on remove,
 - cosmetic changes
 
 v7
 - changed description of 'clock-frequency' DT property
 
 v6
 - endpoint node presence is now optional,
 - added asynchronous subdev registration support and clock
   handling,
 - use named gpios in DT bindings
 
 v5
 - removed hflip/vflip device tree properties
 
 v4
 - GPL changed to GPLv2,
 - bitfields replaced by u8,
 - cosmetic changes,
 - corrected s_stream flow,
 - gpio pins are no longer exported,
 - added I2C addresses to subdev names,
 - CIS subdev registration postponed after
   succesfull HW initialization,
 - added enums for pads,
 - selections are initialized only during probe,
 - default resolution changed to 1600x1200,
 - state-error pattern removed from few other functions,
 - entity link creation moved to registered callback.
 
 v3:
 - narrowed state-error usage to i2c and power errors,
 - private gain controls replaced by red/blue balance user controls,
 - added checks to devicetree gpio node parsing
 
 v2:
 - lower-cased driver name,
 - removed underscore from regulator names,
 - removed platform data code,
 - v4l controls grouped in anonymous structs,
 - added s5k5baf_clear_error function,
 - private controls definitions moved to uapi header file,
 - added v4l2-controls.h reservation for private controls,
 - corrected subdev registered/unregistered code,
 - .log_status sudbev op set to v4l2 helper,
 - moved entity link creation to probe routines,
 - added cleanup on error to probe function.
 ---
  .../devicetree/bindings/media/samsung-s5k5baf.txt  |   58 +
  MAINTAINERS|7 +
  drivers/media/i2c/Kconfig  |7 +
  drivers/media/i2c/Makefile |1 +
  drivers/media/i2c/s5k5baf.c| 2050 
 
  5 files changed, 2123 insertions(+)
  create mode 100644 
 Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
  create mode 100644 drivers/media/i2c/s5k5baf.c
 
 diff --git a/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt 
 b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
 new file mode 100644
 index 000..7704a1e
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
 @@ -0,0 +1,58 @@
 +Samsung S5K5BAF UXGA 1/5 2M CMOS Image Sensor with embedded SoC ISP
 +
 +
 +Required properties:
 +
 +- compatible : samsung,s5k5baf;
 +- reg: I2C slave address of the sensor;
 +- vdda-supply: analog power supply 2.8V (2.6V to 3.0V);
 +- vddreg-supply  : regulator input power supply 1.8V (1.7V to 1.9V)
 +   or 2.8V (2.6V to 3.0);
 +- vddio-supply   : I/O power supply 1.8V (1.65V to 1.95V)
 +   or 2.8V (2.5V to 3.1V);
 +- stbyn-gpios: GPIO connected to STDBYN pin;
 +- rstn-gpios : GPIO connected to RSTN pin;
 +- clocks : the sensor's master clock specifier (from the common
 +   clock bindings);
 +- clock-names: must be mclk;

I'd reword this slightly:

- clocks: clock-specifiers (per the common clock bindings) for the
  clocks described in clock-names
- clock-names: should include mclk for the sensor's master clock

 +
 +Optional properties:
 +
 +- clock-frequency : the frequency at which the mclk clock should be
 +   configured to operate, in Hz; if this property is not
 +   specified default 24 MHz value will be used.
 +
 +The device node should contain one 'port' child node with one child 
 'endpoint'
 +node, according to the bindings defined in Documentation/devicetree/bindings/
 

Re: [PATCH v3] media: st-rc: Add ST remote control driver

[Mark Rutland]

On Wed, Sep 18, 2013 at 11:11:29AM +0100, Srinivas KANDAGATLA wrote:
 Thanks Mark,
 
 On 16/09/13 15:10, Mark Rutland wrote:
  +
   +Required properties:
   +   - compatible: should be st,comms-irb.
  This should probably say should contain rather than should be. There
  may be future vairants of this device, which will also have a more
  specific compatible string.
 Ok, will change it to the suggest.
  
   +   - reg: base physical address of the controller and length of 
   memory
   +   mapped  region.
   +   - interrupts: interrupt number to the cpu. The interrupt 
   specifier
   +   format depends on the interrupt controller parent.
  I don't like the phrase interrupt number to the cpu. We already have
  the term interrupt-specifier to precisely define this. How about:
  
  - interrupts: interrupt-specifier for the sole interrupt generated by
the device.
  
 TBH, I did copy them from one of the existing bindings docs.
 I will change it.

There's a lot of inconsistency with existing docs, it would be nice to
use consistent, correct terminology going forward :)

   +
   +Optional properties:
   +   - rx-mode: can be infrared or uhf.
   +   - tx-mode: should be infrared.
  Are these required to use rx/tx?
 Yes, these are required for driver to be in rx/tx mode.
 
 One of them can be optional depending on the board setup.
 So, Is it ok to move such properties to required properties section?

I'd probably just make a note on each stating what they mean (i.e. that
rx-mode should be present iff the rx pins are wired up).

  
  If you don't have a tx-mode or rx-mode, I assume you can't do
  anything...
 Yes, driver errs out.
  
   +   - pinctrl-names, pinctrl-0: the pincontrol settings to configure
   +   muxing properly for IRB pins.
  If we're expecting names, the names we expect should be defined.
  
   +   - clocks : phandle of clock.
  This is not just a phandle. This is a phandle + clock-specifier pair.
 Yep, will change it.

Cheers.

Mark.
--
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

Re: [PATCH v3] media: st-rc: Add ST remote control driver

[Mark Rutland]

On Thu, Aug 29, 2013 at 01:06:52PM +0100, Srinivas KANDAGATLA wrote:
 From: Srinivas Kandagatla srinivas.kandaga...@st.com
 
 This patch adds support to ST RC driver, which is basically a IR/UHF
 receiver and transmitter. This IP (IRB) is common across all the ST
 parts for settop box platforms. IRB is embedded in ST COMMS IP block.
 It supports both Rx  Tx functionality.
 
 In this driver adds only Rx functionality via LIRC codec.
 
 Signed-off-by: Srinivas Kandagatla srinivas.kandaga...@st.com
 Acked-by: Sean Young s...@mess.org
 ---
 Hi Chehab,
 
 This is a very simple rc driver for IRB controller found in STi ARM CA9 SOCs.
 STi ARM SOC support went in 3.11 recently.
 This driver is a raw driver which feeds data to lirc codec for the user lircd
 to decode the keys.
 
 This patch is based on git://linuxtv.org/media_tree.git master branch.
 
 If its not too late can you please consider this driver for 3.12.
 
 Changes since v2:
 - Updated Kconfig dependencies as suggested by Sean and Chehab.
 - Fixed a logical check spoted by Sean.
 
 Changes since v1:
 - Device tree bindings cleaned up as suggested by Mark and Pawel
 - use ir_raw_event_reset under overflow conditions as suggested by 
 Sean.
 - call ir_raw_event_handle in interrupt handler as suggested by Sean.
 - correct allowed_protos flag with RC_BIT_ types as suggested by Sean.
 - timeout and rx resolution added as suggested by Sean.
 
 Thankyou all for reviewing and commenting.
 
 Thanks,
 srini
 
  Documentation/devicetree/bindings/media/st-rc.txt |   24 ++
  drivers/media/rc/Kconfig  |   10 +
  drivers/media/rc/Makefile |1 +
  drivers/media/rc/st_rc.c  |  396 
 +
  4 files changed, 431 insertions(+), 0 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/media/st-rc.txt
  create mode 100644 drivers/media/rc/st_rc.c
 
 diff --git a/Documentation/devicetree/bindings/media/st-rc.txt 
 b/Documentation/devicetree/bindings/media/st-rc.txt
 new file mode 100644
 index 000..20fe264
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/media/st-rc.txt
 @@ -0,0 +1,24 @@
 +Device-Tree bindings for ST IRB IP
 +
 +Required properties:
 +   - compatible: should be st,comms-irb.

This should probably say should contain rather than should be. There
may be future vairants of this device, which will also have a more
specific compatible string.

 +   - reg: base physical address of the controller and length of memory
 +   mapped  region.
 +   - interrupts: interrupt number to the cpu. The interrupt specifier
 +   format depends on the interrupt controller parent.

I don't like the phrase interrupt number to the cpu. We already have
the term interrupt-specifier to precisely define this. How about:

- interrupts: interrupt-specifier for the sole interrupt generated by
  the device.

 +
 +Optional properties:
 +   - rx-mode: can be infrared or uhf.
 +   - tx-mode: should be infrared.

Are these required to use rx/tx?

If you don't have a tx-mode or rx-mode, I assume you can't do
anything...

 +   - pinctrl-names, pinctrl-0: the pincontrol settings to configure
 +   muxing properly for IRB pins.

If we're expecting names, the names we expect should be defined.

 +   - clocks : phandle of clock.

This is not just a phandle. This is a phandle + clock-specifier pair.

Cheers,
Mark.

 +
 +Example node:
 +
 +   rc: rc@fe518000 {
 +   compatible  = st,comms-irb;
 +   reg = 0xfe518000 0x234;
 +   interrupts  =  0 203 0;
 +   rx-mode = infrared;
 +   };
 diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
 index 11e84bc..557afc5 100644
 --- a/drivers/media/rc/Kconfig
 +++ b/drivers/media/rc/Kconfig
 @@ -322,4 +322,14 @@ config IR_GPIO_CIR
To compile this driver as a module, choose M here: the module will
be called gpio-ir-recv.
 
 +config RC_ST
 +   tristate ST remote control receiver
 +   depends on ARCH_STI  RC_CORE
 +   help
 +Say Y here if you want support for ST remote control driver
 +which allows both IR and UHF RX.
 +The driver passes raw pluse and space information to the LIRC 
 decoder.
 +
 +If you're not sure, select N here.
 +
  endif #RC_DEVICES
 diff --git a/drivers/media/rc/Makefile b/drivers/media/rc/Makefile
 index 56bacf0..f4eb32c 100644
 --- a/drivers/media/rc/Makefile
 +++ b/drivers/media/rc/Makefile
 @@ -30,3 +30,4 @@ obj-$(CONFIG_RC_LOOPBACK) += rc-loopback.o
  obj-$(CONFIG_IR_GPIO_CIR) += gpio-ir-recv.o
  obj-$(CONFIG_IR_IGUANA) += iguanair.o
  obj-$(CONFIG_IR_TTUSBIR) += ttusbir.o
 +obj-$(CONFIG_RC_ST) += st_rc.o
 diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
 new file mode 100644
 index 000..e2dad9c
 --- /dev/null
 +++ b/drivers/media/rc/st_rc.c
 

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.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-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mark Rutland]

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 v7] s5k5baf: add camera sensor driver

[Mark Rutland]

On Mon, Sep 02, 2013 at 05:21:58PM +0100, Sylwester Nawrocki wrote:
 Hi Mark,

Hi Sylwester,

 
 On 08/27/2013 11:14 AM, Mark Rutland wrote:
  +endpoint node
  +-
  +
  +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in
  +  video-interfaces.txt. This property can be only used to specify number
  +  of data lanes, i.e. the array's content is unused, only its length is
  +  meaningful. When this property is not specified default value of 1 lane
  +  will be used.
  
  Apologies for having not replied to the last posting, but having looked
  at the documentation I was provided last time [1], I don't think the
  values in the data-lanes property should be described as unused. That
  may be the way the Linux driver functions at present, but it's not how
  the generic video-interfaces binding documentation describes the
  property.
  
  If the CSI transmitter hardware doesn't support logical remapping of
  lanes, then the only valid values for data-lanes would be a contiguous
  list of lane IDs starting at 1, ending at 4 at most. Valid values for
  the property would be one of:
  
  data-lanes = 1;
  data-lanes = 1, 2;
  data-lanes = 1, 2, 3;
  data-lanes = 1, 2, 3, 4;
  
  We can mention the fact the hardware doesn't support remapping of lanes,
  and therefore the list must start with lane 1 and end with (at most)
  lane 4. That way a dts will match the generic binding and actually
  describe the hardware, and it's possible for Linux (or any other OS) to
  factor out the parsing of data-lanes later as desired.
  
  I don't think we should offer freedom to encode garbage in the dt when
  we can just as easily encourage more standard use of bindings that will
  make our lives easier in the long-term.
 
 I entirely agree, that's a very accurate analysis.
 
 Presumably the data-lanes property's descriptions could be improved so
 it is said explicitly that array elements 0...N - 1, where N = 4, 
 correspond to logical data lanes 1...N.

That makes sense to me.

 
 Then considering the values in the data-lanes property, I didn't make
 the description terribly specific about the fact that pool of indexes
 0...4 is used for the clock lane and 4 data lanes. The values could well
 be H/W specific, but it seems more sensible to enforce common range.
 It may not match exactly with documentation of various hardware. E.g.
 OMAP, see page 1661, register CSI2_COMPLEXIO_CFG [1], uses indexes 
 1..5 to indicate position of a data lane and 0 is used to mark a lane 
 as unused.

Ah, I see. I guess this boils down to what the physical indexes
referred to by the video-interfaces binding actually are. If an
interface may only ever have 5 lanes, then using a logical index sounds
fine. But if we ever have the case where a CSI transmitter has more than
5 lanes (so it could communicate with multiple receviers), then the
value has to become hardware-specific. At that point, we'd possibly need
to define remapping of the clocks too. I'm not that familiar with CSI so
I'm not sure if such a device is possible.

 
 I think we should have similarly defined value 0 to indicate an unused 
 lane. None of drivers in mainline uses this line remapping feature, so 
 changing meaning of the array values wouldn't presumably have any bad 
 side effects. I'm not sure if it's OK to make a change like this now. 
 IIUC the MIPI CSI-2 standard requires consecutive data lane indexes, 
 so valid set of data lanes could be only: 1, 1, 2, 1, 2, 3, 
 1, 2, 3, 4. So there seems to be no issue for MIPI CSI-2. But for 
 future protocols the current convention might not have been flexible 
 enough.

Given that the video-interfaces binding refers to the clock being on
lane 0, I'm not sure it makes sense to reserve this as an unused id --
we'd be saying the lane is the clock to say it's unused, but maybe that
doesn't matter.

Thanks,
Mark.

 
  [1] http://www.mipi.org/specifications/camera-interface#CSI2
 
 [2] 
 http://www.ti.com/general/docs/lit/getliterature.tsp?literatureNumber=swpu231aofileType=pdf
 
 --
 Regards,
 Sylwester
 
--
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

Re: [PATCH v7] s5k5baf: add camera sensor driver

[Mark Rutland]

Hi,

[trimming down to relevant context]

 +endpoint node
 +-
 +
 +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in
 +  video-interfaces.txt. This property can be only used to specify number
 +  of data lanes, i.e. the array's content is unused, only its length is
 +  meaningful. When this property is not specified default value of 1 lane
 +  will be used.

Apologies for having not replied to the last posting, but having looked
at the documentation I was provided last time [1], I don't think the
values in the data-lanes property should be described as unused. That
may be the way the Linux driver functions at present, but it's not how
the generic video-interfaces binding documentation describes the
property.

If the CSI transmitter hardware doesn't support logical remapping of
lanes, then the only valid values for data-lanes would be a contiguous
list of lane IDs starting at 1, ending at 4 at most. Valid values for
the property would be one of:

data-lanes = 1;
data-lanes = 1, 2;
data-lanes = 1, 2, 3;
data-lanes = 1, 2, 3, 4;

We can mention the fact the hardware doesn't support remapping of lanes,
and therefore the list must start with lane 1 and end with (at most)
lane 4. That way a dts will match the generic binding and actually
describe the hardware, and it's possible for Linux (or any other OS) to
factor out the parsing of data-lanes later as desired.

I don't think we should offer freedom to encode garbage in the dt when
we can just as easily encourage more standard use of bindings that will
make our lives easier in the long-term.

Thanks,
Mark.

[1] http://www.mipi.org/specifications/camera-interface#CSI2
--
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

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

[Mark Rutland]

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-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mark Rutland]

[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-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC v5] s5k5baf: add camera sensor driver

[Mark Rutland]

On Mon, Aug 19, 2013 at 02:18:27PM +0100, Andrzej Hajda wrote:
 Driver for Samsung S5K5BAF UXGA 1/5 2M CMOS Image Sensor
 with embedded SoC ISP.
 The driver exposes the sensor as two V4L2 subdevices:
 - S5K5BAF-CIS - pure CMOS Image Sensor, fixed 1600x1200 format,
   no controls.
 - S5K5BAF-ISP - Image Signal Processor, formats up to 1600x1200,
   pre/post ISP cropping, downscaling via selection API, controls.
 
 Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com
 Signed-off-by: Andrzej Hajda a.ha...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
 v5
 - removed non-standard samsung hflip/vflip device tree bindings
 
 v4
 - GPL changed to GPLv2,
 - bitfields replaced by u8,
 - cosmetic changes,
 - corrected s_stream flow,
 - gpio pins are no longer exported,
 - added I2C addresses to subdev names,
 - CIS subdev registration postponed after
   succesfull HW initialization,
 - added enums for pads,
 - selections are initialized only during probe,
 - default resolution changed to 1600x1200,
 - state-error pattern removed from few other functions,
 - entity link creation moved to registered callback.
 
 v3:
 - narrowed state-error usage to i2c and power errors,
 - private gain controls replaced by red/blue balance user controls,
 - added checks to devicetree gpio node parsing
 
 v2:
 - lower-cased driver name,
 - removed underscore from regulator names,
 - removed platform data code,
 - v4l controls grouped in anonymous structs,
 - added s5k5baf_clear_error function,
 - private controls definitions moved to uapi header file,
 - added v4l2-controls.h reservation for private controls,
 - corrected subdev registered/unregistered code,
 - .log_status sudbev op set to v4l2 helper,
 - moved entity link creation to probe routines,
 - added cleanup on error to probe function.
 ---
  .../devicetree/bindings/media/samsung-s5k5baf.txt  |   51 +
  MAINTAINERS|7 +
  drivers/media/i2c/Kconfig  |7 +
  drivers/media/i2c/Makefile |1 +
  drivers/media/i2c/s5k5baf.c| 1980 
 
  5 files changed, 2046 insertions(+)
  create mode 100644 
 Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
  create mode 100644 drivers/media/i2c/s5k5baf.c
 
 diff --git a/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt 
 b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
 new file mode 100644
 index 000..b1f2fde
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
 @@ -0,0 +1,51 @@
 +Samsung S5K5BAF UXGA 1/5 2M CMOS Image Sensor with embedded SoC ISP
 +-
 +
 +Required properties:
 +
 +- compatible : samsung,s5k5baf;
 +- reg: I2C slave address of the sensor;
 +- vdda-supply: analog power supply 2.8V (2.6V to 3.0V);
 +- vddreg-supply  : regulator input power supply 1.8V (1.7V to 1.9V)
 +or 2.8V (2.6V to 3.0);
 +- vddio-supply   : I/O power supply 1.8V (1.65V to 1.95V)
 +or 2.8V (2.5V to 3.1V);
 +- gpios  : GPIOs connected to STDBYN and RSTN pins,
 +in order: STBYN, RSTN;
 +- clock-frequency : master clock frequency in Hz;

Why is this necessary? Could you not just require having a clocks
property? You could then get equivalent functionality to the
clock-frequency property by having a fixed-clock node if you don't ahve
a real clock specifier to wire up.

 +
 +Optional properties:
 +
 +- clocks : contains the sensor's master clock specifier;
 +- clock-names: contains mclk entry;
 +
 +The device node should contain one 'port' child node with one child 
 'endpoint'
 +node, according to the bindings defined in Documentation/devicetree/bindings/
 +media/video-interfaces.txt. The following are properties specific to those 
 nodes.
 +
 +endpoint node
 +-
 +
 +- data-lanes : (optional) an array specifying active physical MIPI-CSI2
 +   data output lanes and their mapping to logical lanes; the
 +   array's content is unused, only its length is meaningful;

Is that a property of the driver, or does the design of the hardware
mean that this can never encode useful information?

What does the length of the property imply?

 +
 +Example:
 +
 +s5k5bafx@2d {
 +   compatible = samsung,s5k5baf;
 +   reg = 0x2d;
 +   vdda-supply = cam_io_en_reg;
 +   vddreg-supply = vt_core_15v_reg;
 +   vddio-supply = vtcam_reg;
 +   gpios = gpl2 0 1,
 +   gpl2 1 1;
 +   clock-frequency = 2400;
 +
 +   port {
 +   s5k5bafx_ep: endpoint {
 +   remote-endpoint = csis1_ep;
 +   data-lanes = 1;
 +   };
 +   };
 +};

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a 

Re: [PATCH] media: st-rc: Add ST remote control driver

[Mark Rutland]

On Wed, Aug 14, 2013 at 06:27:01PM +0100, Srinivas KANDAGATLA wrote:
 From: Srinivas Kandagatla srinivas.kandaga...@st.com
 
 This patch adds support to ST RC driver, which is basically a IR/UHF
 receiver and transmitter. This IP is common across all the ST parts for
 settop box platforms. IRB is embedded in ST COMMS IP block.
 It supports both Rx  Tx functionality.
 
 In this driver adds only Rx functionality via LIRC codec.

Is there anything that additionally needs to be in the dt to support Tx
functionality?

 
 Signed-off-by: Srinivas Kandagatla srinivas.kandaga...@st.com
 ---
 Hi Chehab,
 
 This is a very simple rc driver for IRB controller found in STi ARM CA9 SOCs.
 STi ARM SOC support went in 3.11 recently.
 This driver is a raw driver which feeds data to lirc codec for the user lircd
 to decode the keys.
 
 This patch is based on git://linuxtv.org/media_tree.git master branch.
 
 Comments?
 
 Thanks,
 srini
 
  Documentation/devicetree/bindings/media/st-rc.txt |   18 +
  drivers/media/rc/Kconfig  |   10 +
  drivers/media/rc/Makefile |1 +
  drivers/media/rc/st_rc.c  |  371 
 +
  4 files changed, 400 insertions(+), 0 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/media/st-rc.txt
  create mode 100644 drivers/media/rc/st_rc.c
 
 diff --git a/Documentation/devicetree/bindings/media/st-rc.txt 
 b/Documentation/devicetree/bindings/media/st-rc.txt
 new file mode 100644
 index 000..57f9ee8
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/media/st-rc.txt
 @@ -0,0 +1,18 @@
 +Device-Tree bindings for ST IR and UHF receiver
 +
 +Required properties:
 +   - compatible: should be st,rc.

That rc should be made more specific, and it seems like this is a
subset of a larger block of IP (the ST COMMS IP block). Is this really a
standalone piece of hardware, or is it always in the larger comms block?

What's the full name of this unit as it appears in documentation?

 +   - st,uhfmode: boolean property to indicate if reception is in UHF.

That's not a very clear name. Is this a physical property of the device
(i.e. it's wired to either an IR receiver or a UHF receiver), or is this
a choice as to how it's used at runtime?

If it's fixed property of how the device is wired, it might make more
sense to have a string mode property that's either uhf or infared.

 +   - reg: base physical address of the controller and length of memory
 +   mapped  region.
 +   - interrupts: interrupt number to the cpu. The interrupt specifier
 +   format depends on the interrupt controller parent.
 +
 +Example node:
 +
 +   rc: rc@fe518000 {
 +   compatible  = st,rc;
 +   reg = 0xfe518000 0x234;
 +   interrupts  =  0 203 0;
 +   };
 +

[...]

 +++ b/drivers/media/rc/st_rc.c
 @@ -0,0 +1,371 @@
 +/*
 + * Copyright (C) 2013 STMicroelectronics Limited
 + * Author: Srinivas Kandagatla srinivas.kandaga...@st.com
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License as published by
 + * the Free Software Foundation; either version 2 of the License, or
 + * (at your option) any later version.
 + */
 +#include linux/kernel.h
 +#include linux/clk.h
 +#include linux/interrupt.h
 +#include linux/module.h
 +#include linux/of.h
 +#include linux/platform_device.h
 +#include media/rc-core.h
 +#include linux/pinctrl/consumer.h
 +
 +struct st_rc_device {
 +   struct device   *dev;
 +   int irq;
 +   int irq_wake;
 +   struct clk  *sys_clock;
 +   void*base;  /* Register base address */
 +   void*rx_base;/* RX Register base address 
 */
 +   struct rc_dev   *rdev;
 +   booloverclocking;
 +   int sample_mult;
 +   int sample_div;
 +   boolrxuhfmode;
 +};

[...]

 +static void st_rc_hardware_init(struct st_rc_device *dev)
 +{
 +   int baseclock, freqdiff;
 +   unsigned int rx_max_symbol_per = MAX_SYMB_TIME;
 +   unsigned int rx_sampling_freq_div;
 +
 +   clk_prepare_enable(dev-sys_clock);

This clock should be defined in the binding.

 +   baseclock = clk_get_rate(dev-sys_clock);
 +
 +   /* IRB input pins are inverted internally from high to low. */
 +   writel(1, dev-rx_base + IRB_RX_POLARITY_INV);
 +
 +   rx_sampling_freq_div = baseclock / IRB_SAMPLE_FREQ;
 +   writel(rx_sampling_freq_div, dev-base + IRB_SAMPLE_RATE_COMM);
 +
 +   freqdiff = baseclock - (rx_sampling_freq_div * IRB_SAMPLE_FREQ);
 +   if (freqdiff) { /* over clocking, workout the adjustment factors */
 +   dev-overclocking = 

Re: [PATCH] media: st-rc: Add ST remote control driver

[Mark Rutland]

On Thu, Aug 15, 2013 at 01:57:13PM +0100, Srinivas KANDAGATLA wrote:
 Thanks Mark for your comments.
 
 On 15/08/13 09:49, Mark Rutland wrote:
  On Wed, Aug 14, 2013 at 06:27:01PM +0100, Srinivas KANDAGATLA wrote:
  From: Srinivas Kandagatla srinivas.kandaga...@st.com
 
  This patch adds support to ST RC driver, which is basically a IR/UHF
  receiver and transmitter. This IP is common across all the ST parts for
  settop box platforms. IRB is embedded in ST COMMS IP block.
  It supports both Rx  Tx functionality.
 
  In this driver adds only Rx functionality via LIRC codec.
  
  Is there anything that additionally needs to be in the dt to support Tx
  functionality?
 
 We need an additional boolean property for TX enable.

Why? Becuase you might not have something wired up for Tx?

What needs to be present physically for Tx support?

 
 +
 
 Two more configurable parameters for TX sub-carrier frequency and
 duty-cycle. By default driver can set the sub carrier frequency to be
 38Khz and duty cycle as 50%.
 However I don't have strong use case to make these configurable.
 
 So, I think just one boolean property to enable tx should be Ok.
 
  
 
  Signed-off-by: Srinivas Kandagatla srinivas.kandaga...@st.com
  ---
  Hi Chehab,
 
  This is a very simple rc driver for IRB controller found in STi ARM CA9 
  SOCs.
  STi ARM SOC support went in 3.11 recently.
  This driver is a raw driver which feeds data to lirc codec for the user 
  lircd
  to decode the keys.
 
  This patch is based on git://linuxtv.org/media_tree.git master branch.
 
  Comments?
 
  Thanks,
  srini
 
   Documentation/devicetree/bindings/media/st-rc.txt |   18 +
   drivers/media/rc/Kconfig  |   10 +
   drivers/media/rc/Makefile |1 +
   drivers/media/rc/st_rc.c  |  371 
  +
   4 files changed, 400 insertions(+), 0 deletions(-)
   create mode 100644 Documentation/devicetree/bindings/media/st-rc.txt
   create mode 100644 drivers/media/rc/st_rc.c
 
  diff --git a/Documentation/devicetree/bindings/media/st-rc.txt 
  b/Documentation/devicetree/bindings/media/st-rc.txt
  new file mode 100644
  index 000..57f9ee8
  --- /dev/null
  +++ b/Documentation/devicetree/bindings/media/st-rc.txt
  @@ -0,0 +1,18 @@
  +Device-Tree bindings for ST IR and UHF receiver
  +
  +Required properties:
  +   - compatible: should be st,rc.
  
  That rc should be made more specific, and it seems like this is a
  subset of a larger block of IP (the ST COMMS IP block). Is this really a
  standalone piece of hardware, or is it always in the larger comms block?
 COMMS block is a collection of communication peripherals, and IRB(Infra
 red blaster) is always part of the COMMS block.
 
 May renaming the compatible to st,comms-rc might be more clear.

Given the actual name of the hardware seems to include irb, I think
irb makes more sense than rc in the compatible string. Is there no
more specific name than Infra Red Blaster?

 
  
  What's the full name of this unit as it appears in documentation?
 It is always refered as the Communication sub-system (COMMS) which is a
 collection of communication peripherals like UART, I2C, SPI, IRB.

Ok, are those separate IP blocks within a container, or is anything
shared? Does the COMMS block have any registers shared between those
units, for example?

 
  
  +   - st,uhfmode: boolean property to indicate if reception is in UHF.
  
  That's not a very clear name. Is this a physical property of the device
  (i.e. it's wired to either an IR receiver or a UHF receiver), or is this
  a choice as to how it's used at runtime?
 
 Its both, some boards have IR and others UHF receivers, So it becomes
 board choice here.

When you say it's both, what do you mean? Is it possible for a unit to
be wired with both IR and UHF support simultaneously, even if the Linux
driver can't handle that?

The dt should encode information about the hardware, not the way you
intend to use the hardware at the present moment in time.

 And also the driver has different register set for UHF receiver and IR
 receiver. This options selects which registers to use depending on mode
 of reception.

Ok, but that's an implementation issue. If you described that IR *may*
be used, and UHF *may* be used, the driver can choose what to do based
on that.

 
  
  If it's fixed property of how the device is wired, it might make more
  sense to have a string mode property that's either uhf or infared.
 
 Am ok with either approaches.

It sounds like we may need separate properties as mentioned above, or a
supported-modes list, perhaps.

 
  
  +   - reg: base physical address of the controller and length of memory
  +   mapped  region.
  +   - interrupts: interrupt number to the cpu. The interrupt specifier
  +   format depends on the interrupt controller parent.
  +
  +Example node:
  +
  +   rc: rc@fe518000 {
  +   compatible  = st,rc

Re: [PATCH] media: st-rc: Add ST remote control driver

[Mark Rutland]

On Thu, Aug 15, 2013 at 03:06:14PM +0100, Srinivas KANDAGATLA wrote:
 On 15/08/13 14:29, Mark Rutland wrote:
  On Thu, Aug 15, 2013 at 01:57:13PM +0100, Srinivas KANDAGATLA wrote:
  Thanks Mark for your comments.
 
  On 15/08/13 09:49, Mark Rutland wrote:
  On Wed, Aug 14, 2013 at 06:27:01PM +0100, Srinivas KANDAGATLA wrote:
  From: Srinivas Kandagatla srinivas.kandaga...@st.com
 
  This patch adds support to ST RC driver, which is basically a IR/UHF
  receiver and transmitter. This IP is common across all the ST parts for
  settop box platforms. IRB is embedded in ST COMMS IP block.
  It supports both Rx  Tx functionality.
 
  In this driver adds only Rx functionality via LIRC codec.
 
  Is there anything that additionally needs to be in the dt to support Tx
  functionality?
 
  We need an additional boolean property for TX enable.
  
  Why? Becuase you might not have something wired up for Tx?
 Yes.
  
  What needs to be present physically for Tx support?
 An IR transmitter LEDs need to be physically connected.

Ok.

 Also driver need to know about this to setup the IP to do tx.
  
 
  +
 
  Two more configurable parameters for TX sub-carrier frequency and
  duty-cycle. By default driver can set the sub carrier frequency to be
  38Khz and duty cycle as 50%.
  However I don't have strong use case to make these configurable.
 
  So, I think just one boolean property to enable tx should be Ok.
 
 
  +Device-Tree bindings for ST IR and UHF receiver
  +
  +Required properties:
  +   - compatible: should be st,rc.
 
  That rc should be made more specific, and it seems like this is a
  subset of a larger block of IP (the ST COMMS IP block). Is this really a
  standalone piece of hardware, or is it always in the larger comms block?
  COMMS block is a collection of communication peripherals, and IRB(Infra
  red blaster) is always part of the COMMS block.
 
  May renaming the compatible to st,comms-rc might be more clear.
  
  Given the actual name of the hardware seems to include irb, I think
  irb makes more sense than rc in the compatible string. Is there no
  more specific name than Infra Red Blaster?
 
 There is'nt, its always referred as IRB.
 
 I will rename the compatible to st,comms-irb does this sound Ok?

That sounds fine.

  
 
 
  What's the full name of this unit as it appears in documentation?
  It is always refered as the Communication sub-system (COMMS) which is a
  collection of communication peripherals like UART, I2C, SPI, IRB.
  
  Ok, are those separate IP blocks within a container, or is anything
  shared? Does the COMMS block have any registers shared between those
  units, for example?
 No, registers are not shared across the IP blocks.

Good to know.

  
 
 
  +   - st,uhfmode: boolean property to indicate if reception is in 
  UHF.
 
  That's not a very clear name. Is this a physical property of the device
  (i.e. it's wired to either an IR receiver or a UHF receiver), or is this
  a choice as to how it's used at runtime?
 
  Its both, some boards have IR and others UHF receivers, So it becomes
  board choice here.

Ok, so we can never have both wired up simultaneously? Any board will
have only one of the two wired up (and the other will be unusable
becasue it's not wired up).

  
  When you say it's both, what do you mean? Is it possible for a unit to
  be wired with both IR and UHF support simultaneously, even if the Linux
  driver can't handle that?
  
 I meant that this property is physical property of the device(board
 wired up to either IR or UHF receiver) and also choice on how the IP is
 configured.
 
  The dt should encode information about the hardware, not the way you
  intend to use the hardware at the present moment in time.
 Ok.
  
  And also the driver has different register set for UHF receiver and IR
  receiver. This options selects which registers to use depending on mode
  of reception.
  
  Ok, but that's an implementation issue. If you described that IR *may*
  be used, and UHF *may* be used, the driver can choose what to do based
  on that.
  
 
 
  If it's fixed property of how the device is wired, it might make more
  sense to have a string mode property that's either uhf or infared.
 
  Am ok with either approaches.
  
  It sounds like we may need separate properties as mentioned above, or a
  supported-modes list, perhaps.
 
 I think having rx-mode and tx-mode properties makes more sense rather
 than supported-modes.
 
 like:
 rx-mode = uhf;
 
 or
 
 rx-mode = infrared;
 
 Similarly tx-mode.

That makes sense to me.

 
  
 
 
  +   - reg: base physical address of the controller and length of 
  memory
  +   mapped  region.
  +   - interrupts: interrupt number to the cpu. The interrupt 
  specifier
  +   format depends on the interrupt controller parent.
  +
  +Example node:
  +
  +   rc: rc@fe518000 {
  +   compatible  = st,rc;
  +   reg = 0xfe518000 0x234;
  +   interrupts