[PATCH V2 2/2] dt-bindings: Document the Rockchip RK1608 bindings

[PATCH V2 2/2] dt-bindings: Document the Rockchip RK1608 bindings

[Wen Nuan]

From: Leo Wen <leo.wen@rock-chips.com>

Add DT bindings documentation for Rockchip RK1608.

Changes V2:
- Delete spi-min-frequency property.
- Add the external sensor's control pin and clock properties.
- Delete the '&pinctrl' node.

Signed-off-by: Leo Wen <leo.wen@rock-chips.com>
---
 Documentation/devicetree/bindings/media/rk1608.txt | 97 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 98 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/rk1608.txt

diff --git a/Documentation/devicetree/bindings/media/rk1608.txt b/Documentation/devicetree/bindings/media/rk1608.txt
new file mode 100644
index 0000000..a9721a8
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/rk1608.txt
@@ -0,0 +1,97 @@
+Rockchip RK1608 as a PreISP to link on Soc
+------------------------------------------
+
+Required properties:
+
+- compatible		: "rockchip,rk1608";
+- reg			: SPI slave address of the rk1608;
+- clocks		: Must contain an entry for each entry in clock-names;
+- clock-names		: Must contain "mclk" for the device's master clock;
+- reset-gpio		: GPIO connected to reset pin;
+- irq-gpio		: GPIO connected to irq pin;
+- sleepst-gpio		: GPIO connected to sleepst pin;
+- wakeup-gpio		: GPIO connected to wakeup pin;
+- powerdown-gpio	: GPIO connected to powerdown pin;
+- rockchip,powerdown0	: GPIO connected to the sensor0's powerdown pin;
+- rockchip,reset0	: GPIO connected to the sensor0's reset pin;
+- rockchip,powerdown1	: GPIO connected to the sensor1's powerdown pin;
+- rockchip,reset1	: GPIO connected to the sensor1's reset pin;
+- pinctrl-names		: Should contain only one value - "default";
+- pinctrl-0		: Pin control group to be used for this controller;
+
+Optional properties:
+
+- spi-max-frequency	: Maximum SPI clocking speed of the device;
+
+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. If present it should be <1> - the device
+	       supports only one data lane without re-mapping.
+
+Note1: Since no data is generated in RK1608，so this is meaningful that you need
+a extra sensor (such as a camera) mounted on RK1608. You need to use endpoint@x
+to match these sensors.
+
+Note2:You must set the current value of the spi pins to be 8mA, if they are not.
+
+Example:
+&spi0 {
+	status = "okay";
+	spi_rk1608@00 {
+		compatible =  "rockchip,rk1608";
+		status = "okay";
+		reg = <0>;
+		spi-max-frequency = <24000000>;
+		link-freqs = /bits/ 64 <400000000>;
+		clocks = <&cru SCLK_SPI0>, <&cru SCLK_VIP_OUT>,
+		<&cru DCLK_VOP0>, <&cru ACLK_VIP>, <&cru HCLK_VIP>,
+		<&cru PCLK_ISP_IN>, <&cru PCLK_ISP_IN>,
+		<&cru PCLK_ISP_IN>, <&cru SCLK_MIPIDSI_24M>,
+		<&cru PCLK_MIPI_CSI>;
+		clock-names = "mclk", "mipi_clk",  "pd_cif", "aclk_cif",
+			"hclk_cif", "cif0_in", "g_pclkin_cif",
+			"cif0_out", "clk_mipi_24m", "hclk_mipiphy";
+		reset-gpio = <&gpio6 0 GPIO_ACTIVE_HIGH>;
+		irq-gpio = <&gpio6 2 GPIO_ACTIVE_HIGH>;
+		sleepst-gpio = <&gpio6 1 GPIO_ACTIVE_HIGH>;
+		wakeup-gpio = <&gpio6 4 GPIO_ACTIVE_HIGH>;
+		powerdown-gpio = <&gpio8 0 GPIO_ACTIVE_HIGH>;
+
+		rockchip,powerdown1 = <&gpio5 9 GPIO_ACTIVE_HIGH>;
+		rockchip,reset1 = <&gpio6 8 GPIO_ACTIVE_HIGH>;
+
+		rockchip,powerdown0 = <&gpio5 8 GPIO_ACTIVE_HIGH>;
+		rockchip,reset0 = <&gpio6 7 GPIO_ACTIVE_HIGH>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&rk1608_irq_gpios &rk1608_wake_gpios
+			     &rk1608_sleep_gpios>;
+
+		port@0 {
+			mipi_dphy_out: endpoint {
+				remote-endpoint = <&mipi_dphy_in>;
+				clock-lanes = <0>;
+				data-lanes = <1 2 3 4>;
+				clock-noncontinuous;
+				link-frequencies =
+					/bits/ 64 <400000000>;
+			};
+		};
+		/* Example: we have two cameras */
+		port@1 {
+			sensor_in0: endpoint@0 {
+				remote-endpoint = <&sensor_out0>;
+			};
+			sensor_in1: endpoint@1 {
+				remote-endpoint = <&sensor_out1>;
+			};
+		};
+	};
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index b2a98e3..04d227b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -141,6 +141,7 @@ M:	Leo Wen <leo.wen@rock-chips.com>
 S:	Maintained
 F:	drivers/media/spi/rk1608.c
 F:	drivers/media/spi/rk1608.h
+F:	Documentation/devicetree/bindings/media/rk1608.txt
 
 3C59X NETWORK DRIVER
 M:	Steffen Klassert <klassert@mathematik.tu-chemnitz.de>
-- 
2.7.4

Re: [PATCH V2 2/2] dt-bindings: Document the Rockchip RK1608 bindings

[Linus Walleij]

On Mon, Feb 26, 2018 at 9:25 AM, Wen Nuan <leo.wen@rock-chips.com> wrote:

> From: Leo Wen <leo.wen@rock-chips.com>
>
> Add DT bindings documentation for Rockchip RK1608.
>
> Changes V2:
> - Delete spi-min-frequency property.
> - Add the external sensor's control pin and clock properties.
> - Delete the '&pinctrl' node.
>
> Signed-off-by: Leo Wen <leo.wen@rock-chips.com>

(...)
> +- reset-gpio           : GPIO connected to reset pin;
> +- irq-gpio             : GPIO connected to irq pin;
> +- sleepst-gpio         : GPIO connected to sleepst pin;
> +- wakeup-gpio          : GPIO connected to wakeup pin;
> +- powerdown-gpio       : GPIO connected to powerdown pin;

All these should be named something like:

reset-gpios = <>;
irq-gpios = <>;
etc

See
Documentation/devicetree/bindings/gpio/gpio.txt

So all in pluralis even if it is just one line, that is the standard.

> +- rockchip,powerdown0  : GPIO connected to the sensor0's powerdown pin;
> +- rockchip,reset0      : GPIO connected to the sensor0's reset pin;
> +- rockchip,powerdown1  : GPIO connected to the sensor1's powerdown pin;
> +- rockchip,reset1      : GPIO connected to the sensor1's reset pin;

Also get rid of the custom names here, either no lines should
have a "rockchip", prefix or all of them. Use the name of the
pin on the component, I suspect just

powerdown0-gpios
reset0-gpios
etc

By using the standard "*-gpios" suffix the kernel consumer API
will be much happier as well when you use gpiod_get() & friends.

Yours,
Linus Walleij

Re: [PATCH V2 2/2] dt-bindings: Document the Rockchip RK1608 bindings

[Sakari Ailus]

Hi Wen,

On Mon, Feb 26, 2018 at 04:25:04PM +0800, Wen Nuan wrote:
> From: Leo Wen <leo.wen@rock-chips.com>
> 
> Add DT bindings documentation for Rockchip RK1608.
> 
> Changes V2:
> - Delete spi-min-frequency property.
> - Add the external sensor's control pin and clock properties.
> - Delete the '&pinctrl' node.
> 
> Signed-off-by: Leo Wen <leo.wen@rock-chips.com>
> ---
>  Documentation/devicetree/bindings/media/rk1608.txt | 97 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 98 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/rk1608.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/rk1608.txt b/Documentation/devicetree/bindings/media/rk1608.txt
> new file mode 100644
> index 0000000..a9721a8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/rk1608.txt
> @@ -0,0 +1,97 @@
> +Rockchip RK1608 as a PreISP to link on Soc
> +------------------------------------------
> +
> +Required properties:
> +
> +- compatible		: "rockchip,rk1608";
> +- reg			: SPI slave address of the rk1608;
> +- clocks		: Must contain an entry for each entry in clock-names;
> +- clock-names		: Must contain "mclk" for the device's master clock;
> +- reset-gpio		: GPIO connected to reset pin;
> +- irq-gpio		: GPIO connected to irq pin;
> +- sleepst-gpio		: GPIO connected to sleepst pin;
> +- wakeup-gpio		: GPIO connected to wakeup pin;
> +- powerdown-gpio	: GPIO connected to powerdown pin;
> +- rockchip,powerdown0	: GPIO connected to the sensor0's powerdown pin;
> +- rockchip,reset0	: GPIO connected to the sensor0's reset pin;
> +- rockchip,powerdown1	: GPIO connected to the sensor1's powerdown pin;
> +- rockchip,reset1	: GPIO connected to the sensor1's reset pin;

Aren't these sensor's properties and not related to the ISP?

> +- pinctrl-names		: Should contain only one value - "default";
> +- pinctrl-0		: Pin control group to be used for this controller;
> +
> +Optional properties:
> +
> +- spi-max-frequency	: Maximum SPI clocking speed of the device;
> +
> +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. If present it should be <1> - the device
> +	       supports only one data lane without re-mapping.
> +
> +Note1: Since no data is generated in RK1608，so this is meaningful that you need
> +a extra sensor (such as a camera) mounted on RK1608. You need to use endpoint@x
> +to match these sensors.
> +
> +Note2:You must set the current value of the spi pins to be 8mA, if they are not.
> +
> +Example:
> +&spi0 {
> +	status = "okay";
> +	spi_rk1608@00 {
> +		compatible =  "rockchip,rk1608";
> +		status = "okay";
> +		reg = <0>;
> +		spi-max-frequency = <24000000>;
> +		link-freqs = /bits/ 64 <400000000>;
> +		clocks = <&cru SCLK_SPI0>, <&cru SCLK_VIP_OUT>,
> +		<&cru DCLK_VOP0>, <&cru ACLK_VIP>, <&cru HCLK_VIP>,
> +		<&cru PCLK_ISP_IN>, <&cru PCLK_ISP_IN>,
> +		<&cru PCLK_ISP_IN>, <&cru SCLK_MIPIDSI_24M>,
> +		<&cru PCLK_MIPI_CSI>;
> +		clock-names = "mclk", "mipi_clk",  "pd_cif", "aclk_cif",
> +			"hclk_cif", "cif0_in", "g_pclkin_cif",
> +			"cif0_out", "clk_mipi_24m", "hclk_mipiphy";
> +		reset-gpio = <&gpio6 0 GPIO_ACTIVE_HIGH>;
> +		irq-gpio = <&gpio6 2 GPIO_ACTIVE_HIGH>;
> +		sleepst-gpio = <&gpio6 1 GPIO_ACTIVE_HIGH>;
> +		wakeup-gpio = <&gpio6 4 GPIO_ACTIVE_HIGH>;
> +		powerdown-gpio = <&gpio8 0 GPIO_ACTIVE_HIGH>;
> +
> +		rockchip,powerdown1 = <&gpio5 9 GPIO_ACTIVE_HIGH>;
> +		rockchip,reset1 = <&gpio6 8 GPIO_ACTIVE_HIGH>;
> +
> +		rockchip,powerdown0 = <&gpio5 8 GPIO_ACTIVE_HIGH>;
> +		rockchip,reset0 = <&gpio6 7 GPIO_ACTIVE_HIGH>;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&rk1608_irq_gpios &rk1608_wake_gpios
> +			     &rk1608_sleep_gpios>;
> +
> +		port@0 {
> +			mipi_dphy_out: endpoint {
> +				remote-endpoint = <&mipi_dphy_in>;
> +				clock-lanes = <0>;
> +				data-lanes = <1 2 3 4>;
> +				clock-noncontinuous;
> +				link-frequencies =
> +					/bits/ 64 <400000000>;
> +			};
> +		};
> +		/* Example: we have two cameras */

What determines which one is active? The documentation above states there
may only be a single endpoint per port. And a single port only, not two as
you have here.

> +		port@1 {
> +			sensor_in0: endpoint@0 {
> +				remote-endpoint = <&sensor_out0>;
> +			};
> +			sensor_in1: endpoint@1 {
> +				remote-endpoint = <&sensor_out1>;
> +			};
> +		};
> +	};
> +};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b2a98e3..04d227b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -141,6 +141,7 @@ M:	Leo Wen <leo.wen@rock-chips.com>
>  S:	Maintained
>  F:	drivers/media/spi/rk1608.c
>  F:	drivers/media/spi/rk1608.h
> +F:	Documentation/devicetree/bindings/media/rk1608.txt
>  
>  3C59X NETWORK DRIVER
>  M:	Steffen Klassert <klassert@mathematik.tu-chemnitz.de>

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH V2 2/2] dt-bindings: Document the Rockchip RK1608 bindings

[leo]

Hi Sakari,

Thanks for your advice,  i'll revise it.
> Hi Wen,
>
> On Mon, Feb 26, 2018 at 04:25:04PM +0800, Wen Nuan wrote:
>> From: Leo Wen <leo.wen@rock-chips.com>
>>
>> Add DT bindings documentation for Rockchip RK1608.
>>
>> Changes V2:
>> - Delete spi-min-frequency property.
>> - Add the external sensor's control pin and clock properties.
>> - Delete the '&pinctrl' node.
>>
>> Signed-off-by: Leo Wen <leo.wen@rock-chips.com>
>> ---
>>   Documentation/devicetree/bindings/media/rk1608.txt | 97 ++++++++++++++++++++++
>>   MAINTAINERS                                        |  1 +
>>   2 files changed, 98 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/media/rk1608.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/rk1608.txt b/Documentation/devicetree/bindings/media/rk1608.txt
>> new file mode 100644
>> index 0000000..a9721a8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/rk1608.txt
>> @@ -0,0 +1,97 @@
>> +Rockchip RK1608 as a PreISP to link on Soc
>> +------------------------------------------
>> +
>> +Required properties:
>> +
>> +- compatible		: "rockchip,rk1608";
>> +- reg			: SPI slave address of the rk1608;
>> +- clocks		: Must contain an entry for each entry in clock-names;
>> +- clock-names		: Must contain "mclk" for the device's master clock;
>> +- reset-gpio		: GPIO connected to reset pin;
>> +- irq-gpio		: GPIO connected to irq pin;
>> +- sleepst-gpio		: GPIO connected to sleepst pin;
>> +- wakeup-gpio		: GPIO connected to wakeup pin;
>> +- powerdown-gpio	: GPIO connected to powerdown pin;
>> +- rockchip,powerdown0	: GPIO connected to the sensor0's powerdown pin;
>> +- rockchip,reset0	: GPIO connected to the sensor0's reset pin;
>> +- rockchip,powerdown1	: GPIO connected to the sensor1's powerdown pin;
>> +- rockchip,reset1	: GPIO connected to the sensor1's reset pin;
> Aren't these sensor's properties and not related to the ISP?
There not related to the ISP,  just connected to the external sensor0/1.
>
>> +- pinctrl-names		: Should contain only one value - "default";
>> +- pinctrl-0		: Pin control group to be used for this controller;
>> +
>> +Optional properties:
>> +
>> +- spi-max-frequency	: Maximum SPI clocking speed of the device;
>> +
>> +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. If present it should be <1> - the device
>> +	       supports only one data lane without re-mapping.
>> +
>> +Note1: Since no data is generated in RK1608，so this is meaningful that you need
>> +a extra sensor (such as a camera) mounted on RK1608. You need to use endpoint@x
>> +to match these sensors.
>> +
>> +Note2:You must set the current value of the spi pins to be 8mA, if they are not.
>> +
>> +Example:
>> +&spi0 {
>> +	status = "okay";
>> +	spi_rk1608@00 {
>> +		compatible =  "rockchip,rk1608";
>> +		status = "okay";
>> +		reg = <0>;
>> +		spi-max-frequency = <24000000>;
>> +		link-freqs = /bits/ 64 <400000000>;
>> +		clocks = <&cru SCLK_SPI0>, <&cru SCLK_VIP_OUT>,
>> +		<&cru DCLK_VOP0>, <&cru ACLK_VIP>, <&cru HCLK_VIP>,
>> +		<&cru PCLK_ISP_IN>, <&cru PCLK_ISP_IN>,
>> +		<&cru PCLK_ISP_IN>, <&cru SCLK_MIPIDSI_24M>,
>> +		<&cru PCLK_MIPI_CSI>;
>> +		clock-names = "mclk", "mipi_clk",  "pd_cif", "aclk_cif",
>> +			"hclk_cif", "cif0_in", "g_pclkin_cif",
>> +			"cif0_out", "clk_mipi_24m", "hclk_mipiphy";
>> +		reset-gpio = <&gpio6 0 GPIO_ACTIVE_HIGH>;
>> +		irq-gpio = <&gpio6 2 GPIO_ACTIVE_HIGH>;
>> +		sleepst-gpio = <&gpio6 1 GPIO_ACTIVE_HIGH>;
>> +		wakeup-gpio = <&gpio6 4 GPIO_ACTIVE_HIGH>;
>> +		powerdown-gpio = <&gpio8 0 GPIO_ACTIVE_HIGH>;
>> +
>> +		rockchip,powerdown1 = <&gpio5 9 GPIO_ACTIVE_HIGH>;
>> +		rockchip,reset1 = <&gpio6 8 GPIO_ACTIVE_HIGH>;
>> +
>> +		rockchip,powerdown0 = <&gpio5 8 GPIO_ACTIVE_HIGH>;
>> +		rockchip,reset0 = <&gpio6 7 GPIO_ACTIVE_HIGH>;
>> +
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&rk1608_irq_gpios &rk1608_wake_gpios
>> +			     &rk1608_sleep_gpios>;
>> +
>> +		port@0 {
>> +			mipi_dphy_out: endpoint {
>> +				remote-endpoint = <&mipi_dphy_in>;
>> +				clock-lanes = <0>;
>> +				data-lanes = <1 2 3 4>;
>> +				clock-noncontinuous;
>> +				link-frequencies =
>> +					/bits/ 64 <400000000>;
>> +			};
>> +		};
>> +		/* Example: we have two cameras */
> What determines which one is active? The documentation above states there
> may only be a single endpoint per port. And a single port only, not two as
> you have here.
>
I'm  sorry , i wrote the wrongcommend.
There can be a lot of it on every port.
If you just want sensor0 to be active, delete the 'sensor_in1' node.
>> +		port@1 {
>> +			sensor_in0: endpoint@0 {
>> +				remote-endpoint = <&sensor_out0>;
>> +			};
>> +			sensor_in1: endpoint@1 {
>> +				remote-endpoint = <&sensor_out1>;
>> +			};
>> +		};
>> +	};
>> +};
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index b2a98e3..04d227b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -141,6 +141,7 @@ M:	Leo Wen <leo.wen@rock-chips.com>
>>   S:	Maintained
>>   F:	drivers/media/spi/rk1608.c
>>   F:	drivers/media/spi/rk1608.h
>> +F:	Documentation/devicetree/bindings/media/rk1608.txt
>>   
>>   3C59X NETWORK DRIVER
>>   M:	Steffen Klassert <klassert@mathematik.tu-chemnitz.de>