[v2,1/3] dt-bindings: media: i2c: Add IMX290 CMOS sensor binding

  Add devicetree binding for IMX290 CMOS image sensor.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/media/i2c/imx290.txt  | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/imx290.txt
  

Hi Manivannan,

On Tue, Aug 06, 2019 at 06:39:36PM +0530, Manivannan Sadhasivam wrote:
> Add devicetree binding for IMX290 CMOS image sensor.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>  .../devicetree/bindings/media/i2c/imx290.txt  | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/imx290.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/imx290.txt b/Documentation/devicetree/bindings/media/i2c/imx290.txt
> new file mode 100644
> index 000000000000..7535b5b5b24b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/imx290.txt
> @@ -0,0 +1,51 @@
> +* Sony IMX290 1/2.8-Inch CMOS Image Sensor
> +
> +The Sony IMX290 is a 1/2.8-Inch CMOS Solid-state image sensor with
> +Square Pixel for Color Cameras. It is programmable through I2C and 4-wire
> +interfaces. The sensor output is available via CMOS logic parallel SDR output,
> +Low voltage LVDS DDR output and CSI-2 serial data output.

If there are three to choose from, then you should specify which one is in
use. Given that I think chances remain slim we'd add support for the other
two (it's certainly not ruled out though), CSI-2 could be the default. But
this needs to be documented.

> +
> +Required Properties:
> +- compatible: Should be "sony,imx290"
> +- reg: I2C bus address of the device
> +- clocks: Reference to the xclk clock.
> +- clock-names: Should be "xclk".
> +- clock-frequency: Frequency of the xclk clock.

...in Hz.

> +- vdddo-supply: Sensor digital IO regulator.
> +- vdda-supply: Sensor analog regulator.
> +- vddd-supply: Sensor digital core regulator.
> +
> +Optional Properties:
> +- reset-gpios: Sensor reset GPIO
> +
> +The imx290 device node should contain one 'port' child node with
> +an 'endpoint' subnode. For further reading on port node refer to
> +Documentation/devicetree/bindings/media/video-interfaces.txt.

Which other properties are relevant for the device? I suppose you can't
change the lane order, so clock-lanes is redundant (don't use it in the
example) and data-lanes should be monotonically incrementing series from 1
to 4.

> +
> +Example:
> +	&i2c1 {
> +		...
> +		imx290: imx290@1a {

imx290: camera-sensor@1a {

> +			compatible = "sony,imx290";
> +			reg = <0x1a>;
> +
> +			reset-gpios = <&msmgpio 35 GPIO_ACTIVE_LOW>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&camera_rear_default>;
> +
> +			clocks = <&gcc GCC_CAMSS_MCLK0_CLK>;
> +			clock-names = "xclk";
> +			clock-frequency = <37125000>;
> +
> +			vdddo-supply = <&camera_vdddo_1v8>;
> +			vdda-supply = <&camera_vdda_2v8>;
> +			vddd-supply = <&camera_vddd_1v5>;
> +
> +			port {
> +				imx290_ep: endpoint {
> +					clock-lanes = <1>;
> +					data-lanes = <0 2 3 4>;
> +					remote-endpoint = <&csiphy0_ep>;
> +				};
> +			};
> +		};
  

Hi Sakari,

Thanks for the review!

On Tue, Aug 13, 2019 at 12:45:26PM +0300, Sakari Ailus wrote:
> Hi Manivannan,
> 
> On Tue, Aug 06, 2019 at 06:39:36PM +0530, Manivannan Sadhasivam wrote:
> > Add devicetree binding for IMX290 CMOS image sensor.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > ---
> >  .../devicetree/bindings/media/i2c/imx290.txt  | 51 +++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/imx290.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/imx290.txt b/Documentation/devicetree/bindings/media/i2c/imx290.txt
> > new file mode 100644
> > index 000000000000..7535b5b5b24b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/imx290.txt
> > @@ -0,0 +1,51 @@
> > +* Sony IMX290 1/2.8-Inch CMOS Image Sensor
> > +
> > +The Sony IMX290 is a 1/2.8-Inch CMOS Solid-state image sensor with
> > +Square Pixel for Color Cameras. It is programmable through I2C and 4-wire
> > +interfaces. The sensor output is available via CMOS logic parallel SDR output,
> > +Low voltage LVDS DDR output and CSI-2 serial data output.
> 
> If there are three to choose from, then you should specify which one is in
> use. Given that I think chances remain slim we'd add support for the other
> two (it's certainly not ruled out though), CSI-2 could be the default. But
> this needs to be documented.
> 

Hmm... I'm not sure here. Bindings should describe the hardware and not the
limitations of the driver. Here as you said, the sensor can output frames
in 3 different modes/formats but the driver only supports CSI2. I can add a
note in the driver but not sure whether dt-binding is the right place or not!

> > +
> > +Required Properties:
> > +- compatible: Should be "sony,imx290"
> > +- reg: I2C bus address of the device
> > +- clocks: Reference to the xclk clock.
> > +- clock-names: Should be "xclk".
> > +- clock-frequency: Frequency of the xclk clock.
> 
> ...in Hz.
> 

Ack.

> > +- vdddo-supply: Sensor digital IO regulator.
> > +- vdda-supply: Sensor analog regulator.
> > +- vddd-supply: Sensor digital core regulator.
> > +
> > +Optional Properties:
> > +- reset-gpios: Sensor reset GPIO
> > +
> > +The imx290 device node should contain one 'port' child node with
> > +an 'endpoint' subnode. For further reading on port node refer to
> > +Documentation/devicetree/bindings/media/video-interfaces.txt.
> 
> Which other properties are relevant for the device?

Not much other than, clock/data lanes.

> I suppose you can't change the lane order, so clock-lanes is redundant
> (don't use it in the example) and data-lanes should be monotonically
> incrementing series from 1 to 4.
> 

We can change the order and the example here illustrates how it has been
wired in FRAMOS module. If I change the lane order like you said, it won't
work.

> > +
> > +Example:
> > +	&i2c1 {
> > +		...
> > +		imx290: imx290@1a {
> 
> imx290: camera-sensor@1a {

Ack.

Thanks,
Mani

> 
> > +			compatible = "sony,imx290";
> > +			reg = <0x1a>;
> > +
> > +			reset-gpios = <&msmgpio 35 GPIO_ACTIVE_LOW>;
> > +			pinctrl-names = "default";
> > +			pinctrl-0 = <&camera_rear_default>;
> > +
> > +			clocks = <&gcc GCC_CAMSS_MCLK0_CLK>;
> > +			clock-names = "xclk";
> > +			clock-frequency = <37125000>;
> > +
> > +			vdddo-supply = <&camera_vdddo_1v8>;
> > +			vdda-supply = <&camera_vdda_2v8>;
> > +			vddd-supply = <&camera_vddd_1v5>;
> > +
> > +			port {
> > +				imx290_ep: endpoint {
> > +					clock-lanes = <1>;
> > +					data-lanes = <0 2 3 4>;
> > +					remote-endpoint = <&csiphy0_ep>;
> > +				};
> > +			};
> > +		};
> 
> -- 
> Regards,
> 
> Sakari Ailus
  

On Tue, Aug 06, 2019 at 06:39:36PM +0530, Manivannan Sadhasivam wrote:
> +Required Properties:
> +- compatible: Should be "sony,imx290"
> +- reg: I2C bus address of the device
> +- clocks: Reference to the xclk clock.
> +- clock-names: Should be "xclk".
> +- clock-frequency: Frequency of the xclk clock.

Driver code:

+       ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq);+       if (ret) {
+               dev_err(dev, "Could not get xclk frequency\n");
+               return ret;
+       }
+
+       /* external clock must be 37.125 MHz */
+       if (xclk_freq != 37125000) {
+               dev_err(dev, "External clock frequency %u is not supported\n",
+                       xclk_freq);
+               return -EINVAL;
+       }

So, only 37125000 is supported - is that not worth mentioning in this
description?  Is this a hard requirement of the sensor?  If so, why
does it need to be mentioned in the DT binding?
  

Hi Manivannan,

On Tue, Aug 13, 2019 at 05:03:58PM +0530, Manivannan Sadhasivam wrote:
> Hi Sakari,
> 
> Thanks for the review!
> 
> On Tue, Aug 13, 2019 at 12:45:26PM +0300, Sakari Ailus wrote:
> > Hi Manivannan,
> > 
> > On Tue, Aug 06, 2019 at 06:39:36PM +0530, Manivannan Sadhasivam wrote:
> > > Add devicetree binding for IMX290 CMOS image sensor.
> > > 
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > ---
> > >  .../devicetree/bindings/media/i2c/imx290.txt  | 51 +++++++++++++++++++
> > >  1 file changed, 51 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/imx290.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/imx290.txt b/Documentation/devicetree/bindings/media/i2c/imx290.txt
> > > new file mode 100644
> > > index 000000000000..7535b5b5b24b
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/imx290.txt
> > > @@ -0,0 +1,51 @@
> > > +* Sony IMX290 1/2.8-Inch CMOS Image Sensor
> > > +
> > > +The Sony IMX290 is a 1/2.8-Inch CMOS Solid-state image sensor with
> > > +Square Pixel for Color Cameras. It is programmable through I2C and 4-wire
> > > +interfaces. The sensor output is available via CMOS logic parallel SDR output,
> > > +Low voltage LVDS DDR output and CSI-2 serial data output.
> > 
> > If there are three to choose from, then you should specify which one is in
> > use. Given that I think chances remain slim we'd add support for the other
> > two (it's certainly not ruled out though), CSI-2 could be the default. But
> > this needs to be documented.
> > 
> 
> Hmm... I'm not sure here. Bindings should describe the hardware and not the
> limitations of the driver. Here as you said, the sensor can output frames
> in 3 different modes/formats but the driver only supports CSI2. I can add a
> note in the driver but not sure whether dt-binding is the right place or not!

I guess alternatively you could document the necessary bindings for the
other two busses.

But what I'm saying here is that it's highly unlikely they'll be ever
needed, and it'd be mostly a waste of time to implement that. (That said, I
have nothing against the use of these busses, but I've never seen anyone
using them.) Many other devices use defaults for more contentious settings.

> 
> > > +
> > > +Required Properties:
> > > +- compatible: Should be "sony,imx290"
> > > +- reg: I2C bus address of the device
> > > +- clocks: Reference to the xclk clock.
> > > +- clock-names: Should be "xclk".
> > > +- clock-frequency: Frequency of the xclk clock.
> > 
> > ...in Hz.
> > 
> 
> Ack.
> 
> > > +- vdddo-supply: Sensor digital IO regulator.
> > > +- vdda-supply: Sensor analog regulator.
> > > +- vddd-supply: Sensor digital core regulator.
> > > +
> > > +Optional Properties:
> > > +- reset-gpios: Sensor reset GPIO
> > > +
> > > +The imx290 device node should contain one 'port' child node with
> > > +an 'endpoint' subnode. For further reading on port node refer to
> > > +Documentation/devicetree/bindings/media/video-interfaces.txt.
> > 
> > Which other properties are relevant for the device?
> 
> Not much other than, clock/data lanes.

Please document data-lanes, and which values it may have.

> 
> > I suppose you can't change the lane order, so clock-lanes is redundant
> > (don't use it in the example) and data-lanes should be monotonically
> > incrementing series from 1 to 4.
> > 
> 
> We can change the order and the example here illustrates how it has been
> wired in FRAMOS module. If I change the lane order like you said, it won't
> work.

I highly doubt that. Neither the driver nor the sensor uses the lane
ordering information.

And even if the driver only supported four lanes, then it should check the
number of lanes is actually four.
  

Hi Russel,

On Tue, Aug 13, 2019 at 12:38:46PM +0100, Russell King - ARM Linux admin wrote:
> On Tue, Aug 06, 2019 at 06:39:36PM +0530, Manivannan Sadhasivam wrote:
> > +Required Properties:
> > +- compatible: Should be "sony,imx290"
> > +- reg: I2C bus address of the device
> > +- clocks: Reference to the xclk clock.
> > +- clock-names: Should be "xclk".
> > +- clock-frequency: Frequency of the xclk clock.
> 
> Driver code:
> 
> +       ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq);+       if (ret) {
> +               dev_err(dev, "Could not get xclk frequency\n");
> +               return ret;
> +       }
> +
> +       /* external clock must be 37.125 MHz */
> +       if (xclk_freq != 37125000) {
> +               dev_err(dev, "External clock frequency %u is not supported\n",
> +                       xclk_freq);
> +               return -EINVAL;
> +       }
> 
> So, only 37125000 is supported - is that not worth mentioning in this
> description?  Is this a hard requirement of the sensor?  If so, why
> does it need to be mentioned in the DT binding?
> 

Actually, sensor supports only 2 clock frequencies: 37.125 MHz and 74.25 MHz.
And the driver currently supports only 37.125, because that's what I can test
with my setup.

So how about below:

clock-frequency: Frequency of the xclk clock in Hz. It should be one of the
		 following:
		 - 37125000
		 - 74250000

Thanks,
Mani

> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up
  

Hi Russell,

On Tue, Aug 13, 2019 at 12:38:46PM +0100, Russell King - ARM Linux admin wrote:
> On Tue, Aug 06, 2019 at 06:39:36PM +0530, Manivannan Sadhasivam wrote:
> > +Required Properties:
> > +- compatible: Should be "sony,imx290"
> > +- reg: I2C bus address of the device
> > +- clocks: Reference to the xclk clock.
> > +- clock-names: Should be "xclk".
> > +- clock-frequency: Frequency of the xclk clock.
> 
> Driver code:
> 
> +       ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq);+       if (ret) {
> +               dev_err(dev, "Could not get xclk frequency\n");
> +               return ret;
> +       }
> +
> +       /* external clock must be 37.125 MHz */
> +       if (xclk_freq != 37125000) {
> +               dev_err(dev, "External clock frequency %u is not supported\n",
> +                       xclk_freq);
> +               return -EINVAL;
> +       }
> 
> So, only 37125000 is supported - is that not worth mentioning in this
> description?  Is this a hard requirement of the sensor?  If so, why
> does it need to be mentioned in the DT binding?

The driver only supports a particular frequency, but the sensor is not
limited to that. Unfortunately this is not uncommon for camera sensors, for
the vendors often provide register settings for a given configuration only
(external clock frequency, number of CSI-2 lanes, CSI-2 bus frequency,
image cropping, binning etc.).

That still doesn't mean there are no alternative configurations for
different external clock frequencies, or that there could not be a driver
that was able to configure the sensor to use a given frequency.
  

On Tue, Aug 06, 2019 at 06:39:36PM +0530, Manivannan Sadhasivam wrote:
...
> +Required Properties:
> +- compatible: Should be "sony,imx290"
> +- reg: I2C bus address of the device
> +- clocks: Reference to the xclk clock.
> +- clock-names: Should be "xclk".
> +- clock-frequency: Frequency of the xclk clock.
> +- vdddo-supply: Sensor digital IO regulator.
> +- vdda-supply: Sensor analog regulator.
> +- vddd-supply: Sensor digital core regulator.

Could you also add the link-frequencies property, please?
  

Hi Sakari,

On Tue, Aug 13, 2019 at 02:46:43PM +0300, Sakari Ailus wrote:
> Hi Manivannan,
> 
> On Tue, Aug 13, 2019 at 05:03:58PM +0530, Manivannan Sadhasivam wrote:
> > Hi Sakari,
> > 
> > Thanks for the review!
> > 
> > On Tue, Aug 13, 2019 at 12:45:26PM +0300, Sakari Ailus wrote:
> > > Hi Manivannan,
> > > 
> > > On Tue, Aug 06, 2019 at 06:39:36PM +0530, Manivannan Sadhasivam wrote:
> > > > Add devicetree binding for IMX290 CMOS image sensor.
> > > > 
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > > ---
> > > >  .../devicetree/bindings/media/i2c/imx290.txt  | 51 +++++++++++++++++++
> > > >  1 file changed, 51 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/imx290.txt
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/imx290.txt b/Documentation/devicetree/bindings/media/i2c/imx290.txt
> > > > new file mode 100644
> > > > index 000000000000..7535b5b5b24b
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/imx290.txt
> > > > @@ -0,0 +1,51 @@
> > > > +* Sony IMX290 1/2.8-Inch CMOS Image Sensor
> > > > +
> > > > +The Sony IMX290 is a 1/2.8-Inch CMOS Solid-state image sensor with
> > > > +Square Pixel for Color Cameras. It is programmable through I2C and 4-wire
> > > > +interfaces. The sensor output is available via CMOS logic parallel SDR output,
> > > > +Low voltage LVDS DDR output and CSI-2 serial data output.
> > > 
> > > If there are three to choose from, then you should specify which one is in
> > > use. Given that I think chances remain slim we'd add support for the other
> > > two (it's certainly not ruled out though), CSI-2 could be the default. But
> > > this needs to be documented.
> > > 
> > 
> > Hmm... I'm not sure here. Bindings should describe the hardware and not the
> > limitations of the driver. Here as you said, the sensor can output frames
> > in 3 different modes/formats but the driver only supports CSI2. I can add a
> > note in the driver but not sure whether dt-binding is the right place or not!
> 
> I guess alternatively you could document the necessary bindings for the
> other two busses.
> 
> But what I'm saying here is that it's highly unlikely they'll be ever
> needed, and it'd be mostly a waste of time to implement that. (That said, I
> have nothing against the use of these busses, but I've never seen anyone
> using them.) Many other devices use defaults for more contentious settings.
> 

Agree with you but my question was, whether I could document the supported
mode in bindings or not! I have seen comments from Rob in the past that the
binding should not document the limitations of the driver. But anyway, one
can infer from the current binding that only CSI2 is supported for now, it's
just stating it explicitly makes me doubtful!

> > 
> > > > +
> > > > +Required Properties:
> > > > +- compatible: Should be "sony,imx290"
> > > > +- reg: I2C bus address of the device
> > > > +- clocks: Reference to the xclk clock.
> > > > +- clock-names: Should be "xclk".
> > > > +- clock-frequency: Frequency of the xclk clock.
> > > 
> > > ...in Hz.
> > > 
> > 
> > Ack.
> > 
> > > > +- vdddo-supply: Sensor digital IO regulator.
> > > > +- vdda-supply: Sensor analog regulator.
> > > > +- vddd-supply: Sensor digital core regulator.
> > > > +
> > > > +Optional Properties:
> > > > +- reset-gpios: Sensor reset GPIO
> > > > +
> > > > +The imx290 device node should contain one 'port' child node with
> > > > +an 'endpoint' subnode. For further reading on port node refer to
> > > > +Documentation/devicetree/bindings/media/video-interfaces.txt.
> > > 
> > > Which other properties are relevant for the device?
> > 
> > Not much other than, clock/data lanes.
> 
> Please document data-lanes, and which values it may have.
>

Ack.

> > 
> > > I suppose you can't change the lane order, so clock-lanes is redundant
> > > (don't use it in the example) and data-lanes should be monotonically
> > > incrementing series from 1 to 4.
> > > 
> > 
> > We can change the order and the example here illustrates how it has been
> > wired in FRAMOS module. If I change the lane order like you said, it won't
> > work.
> 
> I highly doubt that. Neither the driver nor the sensor uses the lane
> ordering information.
> 

Agree but CSI2 host will need this informtion, right? Please correct me if
I'm wrong!

> And even if the driver only supported four lanes, then it should check the
> number of lanes is actually four.
> 

Ack. Sensor works with 2/4 lanes but the driver only handles 4 for now. Will
add the check in driver.

Thanks,
Mani

> -- 
> Regards,
> 
> Sakari Ailus
  

On Tue, Aug 13, 2019 at 02:54:27PM +0300, Sakari Ailus wrote:
> On Tue, Aug 06, 2019 at 06:39:36PM +0530, Manivannan Sadhasivam wrote:
> ...
> > +Required Properties:
> > +- compatible: Should be "sony,imx290"
> > +- reg: I2C bus address of the device
> > +- clocks: Reference to the xclk clock.
> > +- clock-names: Should be "xclk".
> > +- clock-frequency: Frequency of the xclk clock.
> > +- vdddo-supply: Sensor digital IO regulator.
> > +- vdda-supply: Sensor analog regulator.
> > +- vddd-supply: Sensor digital core regulator.
> 
> Could you also add the link-frequencies property, please?
> 

Sure, will do.

Thanks,
Mani

> -- 
> Sakari Ailus
  

Hi Manivannan,

On Tue, Aug 13, 2019 at 05:44:00PM +0530, Manivannan Sadhasivam wrote:
> Hi Sakari,
> 
> On Tue, Aug 13, 2019 at 02:46:43PM +0300, Sakari Ailus wrote:
> > Hi Manivannan,
> > 
> > On Tue, Aug 13, 2019 at 05:03:58PM +0530, Manivannan Sadhasivam wrote:
> > > Hi Sakari,
> > > 
> > > Thanks for the review!
> > > 
> > > On Tue, Aug 13, 2019 at 12:45:26PM +0300, Sakari Ailus wrote:
> > > > Hi Manivannan,
> > > > 
> > > > On Tue, Aug 06, 2019 at 06:39:36PM +0530, Manivannan Sadhasivam wrote:
> > > > > Add devicetree binding for IMX290 CMOS image sensor.
> > > > > 
> > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > > > ---
> > > > >  .../devicetree/bindings/media/i2c/imx290.txt  | 51 +++++++++++++++++++
> > > > >  1 file changed, 51 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/imx290.txt
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/imx290.txt b/Documentation/devicetree/bindings/media/i2c/imx290.txt
> > > > > new file mode 100644
> > > > > index 000000000000..7535b5b5b24b
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/media/i2c/imx290.txt
> > > > > @@ -0,0 +1,51 @@
> > > > > +* Sony IMX290 1/2.8-Inch CMOS Image Sensor
> > > > > +
> > > > > +The Sony IMX290 is a 1/2.8-Inch CMOS Solid-state image sensor with
> > > > > +Square Pixel for Color Cameras. It is programmable through I2C and 4-wire
> > > > > +interfaces. The sensor output is available via CMOS logic parallel SDR output,
> > > > > +Low voltage LVDS DDR output and CSI-2 serial data output.
> > > > 
> > > > If there are three to choose from, then you should specify which one is in
> > > > use. Given that I think chances remain slim we'd add support for the other
> > > > two (it's certainly not ruled out though), CSI-2 could be the default. But
> > > > this needs to be documented.
> > > > 
> > > 
> > > Hmm... I'm not sure here. Bindings should describe the hardware and not the
> > > limitations of the driver. Here as you said, the sensor can output frames
> > > in 3 different modes/formats but the driver only supports CSI2. I can add a
> > > note in the driver but not sure whether dt-binding is the right place or not!
> > 
> > I guess alternatively you could document the necessary bindings for the
> > other two busses.
> > 
> > But what I'm saying here is that it's highly unlikely they'll be ever
> > needed, and it'd be mostly a waste of time to implement that. (That said, I
> > have nothing against the use of these busses, but I've never seen anyone
> > using them.) Many other devices use defaults for more contentious settings.
> > 
> 
> Agree with you but my question was, whether I could document the supported
> mode in bindings or not! I have seen comments from Rob in the past that the
> binding should not document the limitations of the driver. But anyway, one
> can infer from the current binding that only CSI2 is supported for now, it's
> just stating it explicitly makes me doubtful!

I think it could be e.g.:

The CSI-2 bus is the default. No bindings have been defined for the other
busses.

...

> > > > I suppose you can't change the lane order, so clock-lanes is redundant
> > > > (don't use it in the example) and data-lanes should be monotonically
> > > > incrementing series from 1 to 4.
> > > > 
> > > 
> > > We can change the order and the example here illustrates how it has been
> > > wired in FRAMOS module. If I change the lane order like you said, it won't
> > > work.
> > 
> > I highly doubt that. Neither the driver nor the sensor uses the lane
> > ordering information.
> > 
> 
> Agree but CSI2 host will need this informtion, right? Please correct me if
> I'm wrong!

The CSI-2 receiver may need that configuration, but it's not addressed by a
sensor's binding documentation (it's configured in the endpoint on the
receiver's side).
  

Hi Sakari,

On Tue, Aug 13, 2019 at 03:22:12PM +0300, Sakari Ailus wrote:
> Hi Manivannan,
> 
> On Tue, Aug 13, 2019 at 05:44:00PM +0530, Manivannan Sadhasivam wrote:
> > Hi Sakari,
> > 
> > On Tue, Aug 13, 2019 at 02:46:43PM +0300, Sakari Ailus wrote:
> > > Hi Manivannan,
> > > 
> > > On Tue, Aug 13, 2019 at 05:03:58PM +0530, Manivannan Sadhasivam wrote:
> > > > Hi Sakari,
> > > > 
> > > > Thanks for the review!
> > > > 
> > > > On Tue, Aug 13, 2019 at 12:45:26PM +0300, Sakari Ailus wrote:
> > > > > Hi Manivannan,
> > > > > 
> > > > > On Tue, Aug 06, 2019 at 06:39:36PM +0530, Manivannan Sadhasivam wrote:
> > > > > > Add devicetree binding for IMX290 CMOS image sensor.
> > > > > > 
> > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > > > > ---
> > > > > >  .../devicetree/bindings/media/i2c/imx290.txt  | 51 +++++++++++++++++++
> > > > > >  1 file changed, 51 insertions(+)
> > > > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/imx290.txt
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/imx290.txt b/Documentation/devicetree/bindings/media/i2c/imx290.txt
> > > > > > new file mode 100644
> > > > > > index 000000000000..7535b5b5b24b
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/imx290.txt
> > > > > > @@ -0,0 +1,51 @@
> > > > > > +* Sony IMX290 1/2.8-Inch CMOS Image Sensor
> > > > > > +
> > > > > > +The Sony IMX290 is a 1/2.8-Inch CMOS Solid-state image sensor with
> > > > > > +Square Pixel for Color Cameras. It is programmable through I2C and 4-wire
> > > > > > +interfaces. The sensor output is available via CMOS logic parallel SDR output,
> > > > > > +Low voltage LVDS DDR output and CSI-2 serial data output.
> > > > > 
> > > > > If there are three to choose from, then you should specify which one is in
> > > > > use. Given that I think chances remain slim we'd add support for the other
> > > > > two (it's certainly not ruled out though), CSI-2 could be the default. But
> > > > > this needs to be documented.
> > > > > 
> > > > 
> > > > Hmm... I'm not sure here. Bindings should describe the hardware and not the
> > > > limitations of the driver. Here as you said, the sensor can output frames
> > > > in 3 different modes/formats but the driver only supports CSI2. I can add a
> > > > note in the driver but not sure whether dt-binding is the right place or not!
> > > 
> > > I guess alternatively you could document the necessary bindings for the
> > > other two busses.
> > > 
> > > But what I'm saying here is that it's highly unlikely they'll be ever
> > > needed, and it'd be mostly a waste of time to implement that. (That said, I
> > > have nothing against the use of these busses, but I've never seen anyone
> > > using them.) Many other devices use defaults for more contentious settings.
> > > 
> > 
> > Agree with you but my question was, whether I could document the supported
> > mode in bindings or not! I have seen comments from Rob in the past that the
> > binding should not document the limitations of the driver. But anyway, one
> > can infer from the current binding that only CSI2 is supported for now, it's
> > just stating it explicitly makes me doubtful!
> 
> I think it could be e.g.:
> 
> The CSI-2 bus is the default. No bindings have been defined for the other
> busses.
> 

Ack.

> ...
> 
> > > > > I suppose you can't change the lane order, so clock-lanes is redundant
> > > > > (don't use it in the example) and data-lanes should be monotonically
> > > > > incrementing series from 1 to 4.
> > > > > 
> > > > 
> > > > We can change the order and the example here illustrates how it has been
> > > > wired in FRAMOS module. If I change the lane order like you said, it won't
> > > > work.
> > > 
> > > I highly doubt that. Neither the driver nor the sensor uses the lane
> > > ordering information.
> > > 
> > 
> > Agree but CSI2 host will need this informtion, right? Please correct me if
> > I'm wrong!
> 
> The CSI-2 receiver may need that configuration, but it's not addressed by a
> sensor's binding documentation (it's configured in the endpoint on the
> receiver's side).
> 

Yes but I thought that documenting the sensor lane configuration based on one
example implementation might help interfacing w/ different hosts. Anyway, to be
host agnostic, I can drop the clock lane and make data lane start from 1 as you
suggested.

Thanks,
Mani

> -- 
> Sakari Ailus
  

Hi Mani,

On Tue, Aug 13, 2019 at 07:22:09PM +0530, Manivannan Sadhasivam wrote:
> Hi Sakari,
> 
> On Tue, Aug 13, 2019 at 03:22:12PM +0300, Sakari Ailus wrote:
> > Hi Manivannan,
> > 
> > On Tue, Aug 13, 2019 at 05:44:00PM +0530, Manivannan Sadhasivam wrote:
> > > Hi Sakari,
> > > 
> > > On Tue, Aug 13, 2019 at 02:46:43PM +0300, Sakari Ailus wrote:
> > > > Hi Manivannan,
> > > > 
> > > > On Tue, Aug 13, 2019 at 05:03:58PM +0530, Manivannan Sadhasivam wrote:
> > > > > Hi Sakari,
> > > > > 
> > > > > Thanks for the review!
> > > > > 
> > > > > On Tue, Aug 13, 2019 at 12:45:26PM +0300, Sakari Ailus wrote:
> > > > > > Hi Manivannan,
> > > > > > 
> > > > > > On Tue, Aug 06, 2019 at 06:39:36PM +0530, Manivannan Sadhasivam wrote:
> > > > > > > Add devicetree binding for IMX290 CMOS image sensor.
> > > > > > > 
> > > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > > > > > ---
> > > > > > >  .../devicetree/bindings/media/i2c/imx290.txt  | 51 +++++++++++++++++++
> > > > > > >  1 file changed, 51 insertions(+)
> > > > > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/imx290.txt
> > > > > > > 
> > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/imx290.txt b/Documentation/devicetree/bindings/media/i2c/imx290.txt
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..7535b5b5b24b
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/imx290.txt
> > > > > > > @@ -0,0 +1,51 @@
> > > > > > > +* Sony IMX290 1/2.8-Inch CMOS Image Sensor
> > > > > > > +
> > > > > > > +The Sony IMX290 is a 1/2.8-Inch CMOS Solid-state image sensor with
> > > > > > > +Square Pixel for Color Cameras. It is programmable through I2C and 4-wire
> > > > > > > +interfaces. The sensor output is available via CMOS logic parallel SDR output,
> > > > > > > +Low voltage LVDS DDR output and CSI-2 serial data output.
> > > > > > 
> > > > > > If there are three to choose from, then you should specify which one is in
> > > > > > use. Given that I think chances remain slim we'd add support for the other
> > > > > > two (it's certainly not ruled out though), CSI-2 could be the default. But
> > > > > > this needs to be documented.
> > > > > > 
> > > > > 
> > > > > Hmm... I'm not sure here. Bindings should describe the hardware and not the
> > > > > limitations of the driver. Here as you said, the sensor can output frames
> > > > > in 3 different modes/formats but the driver only supports CSI2. I can add a
> > > > > note in the driver but not sure whether dt-binding is the right place or not!
> > > > 
> > > > I guess alternatively you could document the necessary bindings for the
> > > > other two busses.
> > > > 
> > > > But what I'm saying here is that it's highly unlikely they'll be ever
> > > > needed, and it'd be mostly a waste of time to implement that. (That said, I
> > > > have nothing against the use of these busses, but I've never seen anyone
> > > > using them.) Many other devices use defaults for more contentious settings.
> > > > 
> > > 
> > > Agree with you but my question was, whether I could document the supported
> > > mode in bindings or not! I have seen comments from Rob in the past that the
> > > binding should not document the limitations of the driver. But anyway, one
> > > can infer from the current binding that only CSI2 is supported for now, it's
> > > just stating it explicitly makes me doubtful!
> > 
> > I think it could be e.g.:
> > 
> > The CSI-2 bus is the default. No bindings have been defined for the other
> > busses.
> > 
> 
> Ack.
> 
> > ...
> > 
> > > > > > I suppose you can't change the lane order, so clock-lanes is redundant
> > > > > > (don't use it in the example) and data-lanes should be monotonically
> > > > > > incrementing series from 1 to 4.
> > > > > > 
> > > > > 
> > > > > We can change the order and the example here illustrates how it has been
> > > > > wired in FRAMOS module. If I change the lane order like you said, it won't
> > > > > work.
> > > > 
> > > > I highly doubt that. Neither the driver nor the sensor uses the lane
> > > > ordering information.
> > > > 
> > > 
> > > Agree but CSI2 host will need this informtion, right? Please correct me if
> > > I'm wrong!
> > 
> > The CSI-2 receiver may need that configuration, but it's not addressed by a
> > sensor's binding documentation (it's configured in the endpoint on the
> > receiver's side).
> > 
> 
> Yes but I thought that documenting the sensor lane configuration based on one
> example implementation might help interfacing w/ different hosts. Anyway, to be
> host agnostic, I can drop the clock lane and make data lane start from 1 as you
> suggested.

You could well have a different configuration from the default in an
example. But my point is that this is simply the wrong place for
configuring the lane mapping on the host.