diff mbox series

[1/2] dt-bindings: media: i2c: Add IMX296 CMOS sensor binding

Message ID 20191011035613.13598-2-manivannan.sadhasivam@linaro.org (mailing list archive)
State New, archived
Headers show
Series Add IMX296 CMOS image sensor support | expand

Commit Message

Manivannan Sadhasivam Oct. 11, 2019, 3:56 a.m. UTC 
Add devicetree binding for IMX296 CMOS image sensor.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 .../devicetree/bindings/media/i2c/imx296.txt  | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/imx296.txt

Comments

Rob Herring (Arm) Oct. 15, 2019, 10:45 p.m. UTC | #1 
On Fri, Oct 11, 2019 at 09:26:12AM +0530, Manivannan Sadhasivam wrote:
> Add devicetree binding for IMX296 CMOS image sensor.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  .../devicetree/bindings/media/i2c/imx296.txt  | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/imx296.txt

You should know by now, use DT schema format please.

> diff --git a/Documentation/devicetree/bindings/media/i2c/imx296.txt b/Documentation/devicetree/bindings/media/i2c/imx296.txt
> new file mode 100644
> index 000000000000..25d3b15162c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/imx296.txt
> @@ -0,0 +1,55 @@
> +* Sony IMX296 1/2.8-Inch CMOS Image Sensor
> +
> +The Sony IMX296 is a 1/2.9-Inch active pixel type CMOS Solid-state image
> +sensor with square pixel array and 1.58 M effective pixels. This chip features
> +a global shutter with variable charge-integration time. It is programmable
> +through I2C and 4-wire interfaces. The sensor output is available via CSI-2
> +serial data output (1 Lane).
> +
> +Required Properties:
> +- compatible: Should be "sony,imx296"
> +- reg: I2C bus address of the device
> +- clocks: Reference to the mclk clock.
> +- clock-names: Should be "mclk".
> +- clock-frequency: Frequency of the mclk clock in Hz.
> +- vddo-supply: Interface power supply.
> +- vdda-supply: Analog power supply.
> +- vddd-supply: Digital power supply.
> +
> +Optional Properties:
> +- reset-gpios: Sensor reset GPIO
> +
> +The imx296 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.
> +
> +Required Properties on endpoint:
> +- data-lanes: check ../video-interfaces.txt

This should only be required when not using all the lanes on the device.

> +- remote-endpoint: check ../video-interfaces.txt

Don't really need to document this.

> +
> +Example:
> +	&i2c1 {
> +		...
> +		imx296: camera-sensor@1a {
> +			compatible = "sony,imx296";
> +			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 = "mclk";
> +			clock-frequency = <37125000>;
> +
> +			vddo-supply = <&camera_vddo_1v8>;
> +			vdda-supply = <&camera_vdda_3v3>;
> +			vddd-supply = <&camera_vddd_1v2>;
> +
> +			port {
> +				imx296_ep: endpoint {
> +					data-lanes = <1>;
> +					remote-endpoint = <&csiphy0_ep>;
> +				};
> +			};
> +		};
> -- 
> 2.17.1
>
Manivannan Sadhasivam Oct. 16, 2019, 8:37 a.m. UTC | #2 
Hi Rob,

On Tue, Oct 15, 2019 at 05:45:54PM -0500, Rob Herring wrote:
> On Fri, Oct 11, 2019 at 09:26:12AM +0530, Manivannan Sadhasivam wrote:
> > Add devicetree binding for IMX296 CMOS image sensor.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  .../devicetree/bindings/media/i2c/imx296.txt  | 55 +++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/imx296.txt
> 
> You should know by now, use DT schema format please.
> 

I know for other subsystems but by having a vague look at the existing bindings
I thought media subsystem is still using .txt. But I now see few yaml bindings
in linux-next and will switch over this.

Btw, is it mandatory now to use YAML bindings for all subsystems? I don't
see any issue (instead I prefer) but I remember that you defer to the preference
of the subsystem maintainers before!

> > diff --git a/Documentation/devicetree/bindings/media/i2c/imx296.txt b/Documentation/devicetree/bindings/media/i2c/imx296.txt
> > new file mode 100644
> > index 000000000000..25d3b15162c1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/imx296.txt
> > @@ -0,0 +1,55 @@
> > +* Sony IMX296 1/2.8-Inch CMOS Image Sensor
> > +
> > +The Sony IMX296 is a 1/2.9-Inch active pixel type CMOS Solid-state image
> > +sensor with square pixel array and 1.58 M effective pixels. This chip features
> > +a global shutter with variable charge-integration time. It is programmable
> > +through I2C and 4-wire interfaces. The sensor output is available via CSI-2
> > +serial data output (1 Lane).
> > +
> > +Required Properties:
> > +- compatible: Should be "sony,imx296"
> > +- reg: I2C bus address of the device
> > +- clocks: Reference to the mclk clock.
> > +- clock-names: Should be "mclk".
> > +- clock-frequency: Frequency of the mclk clock in Hz.
> > +- vddo-supply: Interface power supply.
> > +- vdda-supply: Analog power supply.
> > +- vddd-supply: Digital power supply.
> > +
> > +Optional Properties:
> > +- reset-gpios: Sensor reset GPIO
> > +
> > +The imx296 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.
> > +
> > +Required Properties on endpoint:
> > +- data-lanes: check ../video-interfaces.txt
> 
> This should only be required when not using all the lanes on the device.
> 

This is a bit weird! How will someone know how many lanes the device is using
by looking at the binding? He can anyway refer the datasheet but still...

> > +- remote-endpoint: check ../video-interfaces.txt
> 
> Don't really need to document this.
> 

okay.

Thanks,
Mani

> > +
> > +Example:
> > +	&i2c1 {
> > +		...
> > +		imx296: camera-sensor@1a {
> > +			compatible = "sony,imx296";
> > +			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 = "mclk";
> > +			clock-frequency = <37125000>;
> > +
> > +			vddo-supply = <&camera_vddo_1v8>;
> > +			vdda-supply = <&camera_vdda_3v3>;
> > +			vddd-supply = <&camera_vddd_1v2>;
> > +
> > +			port {
> > +				imx296_ep: endpoint {
> > +					data-lanes = <1>;
> > +					remote-endpoint = <&csiphy0_ep>;
> > +				};
> > +			};
> > +		};
> > -- 
> > 2.17.1
> >
Sakari Ailus Oct. 18, 2019, 10:06 p.m. UTC | #3 
Hi Manivannan, Rob,

On Wed, Oct 16, 2019 at 02:07:48PM +0530, Manivannan Sadhasivam wrote:
> Hi Rob,
> 
> On Tue, Oct 15, 2019 at 05:45:54PM -0500, Rob Herring wrote:
> > On Fri, Oct 11, 2019 at 09:26:12AM +0530, Manivannan Sadhasivam wrote:
> > > Add devicetree binding for IMX296 CMOS image sensor.
> > > 
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  .../devicetree/bindings/media/i2c/imx296.txt  | 55 +++++++++++++++++++
> > >  1 file changed, 55 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/imx296.txt
> > 
> > You should know by now, use DT schema format please.
> > 
> 
> I know for other subsystems but by having a vague look at the existing bindings
> I thought media subsystem is still using .txt. But I now see few yaml bindings
> in linux-next and will switch over this.
> 
> Btw, is it mandatory now to use YAML bindings for all subsystems? I don't
> see any issue (instead I prefer) but I remember that you defer to the preference
> of the subsystem maintainers before!
> 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/imx296.txt b/Documentation/devicetree/bindings/media/i2c/imx296.txt
> > > new file mode 100644
> > > index 000000000000..25d3b15162c1
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/imx296.txt
> > > @@ -0,0 +1,55 @@
> > > +* Sony IMX296 1/2.8-Inch CMOS Image Sensor
> > > +
> > > +The Sony IMX296 is a 1/2.9-Inch active pixel type CMOS Solid-state image
> > > +sensor with square pixel array and 1.58 M effective pixels. This chip features
> > > +a global shutter with variable charge-integration time. It is programmable
> > > +through I2C and 4-wire interfaces. The sensor output is available via CSI-2
> > > +serial data output (1 Lane).
> > > +
> > > +Required Properties:
> > > +- compatible: Should be "sony,imx296"
> > > +- reg: I2C bus address of the device
> > > +- clocks: Reference to the mclk clock.
> > > +- clock-names: Should be "mclk".
> > > +- clock-frequency: Frequency of the mclk clock in Hz.
> > > +- vddo-supply: Interface power supply.
> > > +- vdda-supply: Analog power supply.
> > > +- vddd-supply: Digital power supply.
> > > +
> > > +Optional Properties:
> > > +- reset-gpios: Sensor reset GPIO
> > > +
> > > +The imx296 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.
> > > +
> > > +Required Properties on endpoint:
> > > +- data-lanes: check ../video-interfaces.txt
> > 
> > This should only be required when not using all the lanes on the device.
> > 
> 
> This is a bit weird! How will someone know how many lanes the device is using
> by looking at the binding? He can anyway refer the datasheet but still...

Many current bindings document data-lanes as mandatory. Nothing prevents
making all lanes are connected the default though, thus making data-lanes
optional.

The V4L2 fwnode framework supports easy parsing of that, too, by driver
providing that default value before letting V4L2 fwnode framework to parse
the endpoint properties.

Looking at this particular sensor --- doesn't it only have a single lane,
and thus nothing to configure here?
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/media/i2c/imx296.txt b/Documentation/devicetree/bindings/media/i2c/imx296.txt
new file mode 100644
index 000000000000..25d3b15162c1
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/imx296.txt
@@ -0,0 +1,55 @@ 
+* Sony IMX296 1/2.8-Inch CMOS Image Sensor
+
+The Sony IMX296 is a 1/2.9-Inch active pixel type CMOS Solid-state image
+sensor with square pixel array and 1.58 M effective pixels. This chip features
+a global shutter with variable charge-integration time. It is programmable
+through I2C and 4-wire interfaces. The sensor output is available via CSI-2
+serial data output (1 Lane).
+
+Required Properties:
+- compatible: Should be "sony,imx296"
+- reg: I2C bus address of the device
+- clocks: Reference to the mclk clock.
+- clock-names: Should be "mclk".
+- clock-frequency: Frequency of the mclk clock in Hz.
+- vddo-supply: Interface power supply.
+- vdda-supply: Analog power supply.
+- vddd-supply: Digital power supply.
+
+Optional Properties:
+- reset-gpios: Sensor reset GPIO
+
+The imx296 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.
+
+Required Properties on endpoint:
+- data-lanes: check ../video-interfaces.txt
+- remote-endpoint: check ../video-interfaces.txt
+
+Example:
+	&i2c1 {
+		...
+		imx296: camera-sensor@1a {
+			compatible = "sony,imx296";
+			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 = "mclk";
+			clock-frequency = <37125000>;
+
+			vddo-supply = <&camera_vddo_1v8>;
+			vdda-supply = <&camera_vdda_3v3>;
+			vddd-supply = <&camera_vddd_1v2>;
+
+			port {
+				imx296_ep: endpoint {
+					data-lanes = <1>;
+					remote-endpoint = <&csiphy0_ep>;
+				};
+			};
+		};