| Message ID | 20230905233118.183140-2-paul.elder@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: Add driver for THine THP7312 ISP | expand |
On Wed, 06 Sep 2023 08:31:16 +0900, Paul Elder wrote: > Add bindings for the THine THP7312 ISP. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > Since the THP7312 supports multiple sensors, thine,rx-data-lanes alone > might not be enough. I was consdering using sensor nodes like what the > AP1302 does [1]. This way we can also move the power supplies that only > concern the sensor in there as well. I was wondering what to do about > the model name, though, as the thp7312 completely isolates that from the > rest of the system. > > I'm planning to add sensor nodes in somehow in a v2. > > [1] https://lore.kernel.org/linux-media/20211006113254.3470-2-anil.mamidala@xilinx.com/ > > .../bindings/media/thine,thp7312.yaml | 170 ++++++++++++++++++ > 1 file changed, 170 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/thine,thp7312.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/media/thine,thp7312.yaml:35:20: [error] syntax error: mapping values are not allowed here (syntax) dtschema/dtc warnings/errors: make[2]: *** Deleting file 'Documentation/devicetree/bindings/media/thine,thp7312.example.dts' Documentation/devicetree/bindings/media/thine,thp7312.yaml:35:20: mapping values are not allowed here make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/media/thine,thp7312.example.dts] Error 1 make[2]: *** Waiting for unfinished jobs.... ./Documentation/devicetree/bindings/media/thine,thp7312.yaml:35:20: mapping values are not allowed here /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/thine,thp7312.yaml: ignoring, error parsing file make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1500: dt_binding_check] Error 2 make: *** [Makefile:234: __sub-make] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230905233118.183140-2-paul.elder@ideasonboard.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On 06/09/2023 01:31, Paul Elder wrote: > Add bindings for the THine THP7312 ISP. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > Since the THP7312 supports multiple sensors, thine,rx-data-lanes alone > might not be enough. I was consdering using sensor nodes like what the > AP1302 does [1]. This way we can also move the power supplies that only > concern the sensor in there as well. I was wondering what to do about > the model name, though, as the thp7312 completely isolates that from the > rest of the system. > > I'm planning to add sensor nodes in somehow in a v2. > > [1] https://lore.kernel.org/linux-media/20211006113254.3470-2-anil.mamidala@xilinx.com/ > > .../bindings/media/thine,thp7312.yaml | 170 ++++++++++++++++++ > 1 file changed, 170 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/thine,thp7312.yaml > > diff --git a/Documentation/devicetree/bindings/media/thine,thp7312.yaml b/Documentation/devicetree/bindings/media/thine,thp7312.yaml > new file mode 100644 > index 000000000000..e8d203dcda81 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/thine,thp7312.yaml > @@ -0,0 +1,170 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright (c) 2023 Ideas on Board > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/thine,thp7312.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: THine THP7312 > + > +maintainers: > + - Paul Elder <paul.elder@@ideasonboard.com> > + > +description: > + The THP7312 is a standalone ISP controlled over i2c, and is capable of > + various image processing and correction functions, including 3A control. It > + can be connected to CMOS image sensors from various vendors, supporting both > + MIPI CSI-2 and parallel interfaces. It can also output on either MIPI CSI-2 > + or parallel. The hardware is capable of transmitting and receiving MIPI > + interlaved data strams with data types or multiple virtual channel > + identifiers. > + > +allOf: > + - $ref: ../video-interface-devices.yaml# > + > +properties: > + compatible: > + const: thine,thp7312 > + > + reg: > + description: I2C device address You can skip description. It is obvious. > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + - description: CLKI clock input This was absolutely never tested. > + > + reset-gpios: > + maxItems: 1 > + description: |- > + Reference to the GPIO connected to the RESET_N pin, if any. > + Must be released (set high) after all supplies are applied. > + > + vddcore-supply: > + description: > + 1.2V supply for core, PLL, MIPI rx and MIPI tx. > + > + vhtermnx-supply: > + description: > + Supply for input (rx). 1.8V for MIPI, or 1.8/2.8/3.3V for parallel. > + > + vddtx-supply: > + description: > + Supply for output (tx). 1.8V for MIPI, or 1.8/2.8/3.3V for parallel. > + > + vddhost-supply: > + description: > + Supply for host interface. 1.8V, 2.8V, or 3.3V. > + > + vddcmos-supply: > + description: > + Supply for sensor interface. 1.8V, 2.8V, or 3.3V. > + > + vddgpio_0-supply: No, underscores are not allowed in names. > + description: > + Supply for GPIO_0. 1.8V, 2.8V, or 3.3V. > + > + vddgpio_1-supply: > + description: > + Supply for GPIO_1. 1.8V, 2.8V, or 3.3V. > + > + DOVDD-supply: lowercase. Look at your other supplies. VDD is spelled there "vdd", so do not introduce random style. > + description: > + Digital I/O (1.8V) supply for image sensor. > + > + AVDD-supply: lowercase > + description: > + Analog (2.8V) supply for image sensor. > + > + DVDD-supply: lowercase > + description: > + Digital Core (1.2V) supply for image sensor. > + > + orientation: true > + rotation: true > + > + thine,rx,data-lanes: Why are you duplicating properties? With wrong name? No, that's not a property of a device node, but endpoint. > + minItems: 4 > + maxItems: 4 > + $ref: /schemas/media/video-interfaces.yaml#data-lanes > + description: |- Drop |- where not needed. > + This property is for lane reordering between the THP7312 and the imaging > + sensor that it is connected to. > + > + port: > + $ref: /schemas/graph.yaml#/$defs/port-base > + additionalProperties: false > + > + properties: > + endpoint: > + $ref: /schemas/media/video-interfaces.yaml# > + unevaluatedProperties: false > + > + properties: > + data-lanes: > + description: |- > + The sensor supports either two-lane, or four-lane operation. > + This property is for lane reordering between the THP7312 and > + the SoC. If this property is omitted four-lane operation is > + assumed. For two-lane operation the property must be set to <1 2>. > + minItems: 2 > + maxItems: 4 > + items: > + maximum: 4 > + > +required: > + - compatible > + - reg > + - reset-gpios > + - clocks > + - vddcore-supply > + - vhtermrx-supply > + - vddtx-supply > + - vddhost-supply > + - vddcmos-supply > + - vddgpio_0-supply > + - vddgpio_1-supply > + - DOVDD-supply > + - AVDD-supply > + - DVDD-supply > + - thine,rx,data-lanes > + - port > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + camera@61 { > + compatible = "thine,thp7312"; > + reg = <0x61>; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&cam1_pins_default>; > + > + reset-gpios = <&pio 119 GPIO_ACTIVE_LOW>; > + clocks = <&camera61_clk>; > + > + vddcore-supply = <&vsys_v4p2>; > + AVDD-supply = <&vsys_v4p2>; > + DVDD-supply = <&vsys_v4p2>; Srlsy, test it before sending. Look how many supplies you require and what is provided here. How any of this could possibly work? Best regards, Krzysztof
On Wed, Sep 06, 2023 at 09:18:30AM +0200, Krzysztof Kozlowski wrote: > On 06/09/2023 01:31, Paul Elder wrote: > > Add bindings for the THine THP7312 ISP. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > Since the THP7312 supports multiple sensors, thine,rx-data-lanes alone > > might not be enough. I was consdering using sensor nodes like what the > > AP1302 does [1]. This way we can also move the power supplies that only > > concern the sensor in there as well. I was wondering what to do about > > the model name, though, as the thp7312 completely isolates that from the > > rest of the system. > > > > I'm planning to add sensor nodes in somehow in a v2. > > > > [1] https://lore.kernel.org/linux-media/20211006113254.3470-2-anil.mamidala@xilinx.com/ > > > > .../bindings/media/thine,thp7312.yaml | 170 ++++++++++++++++++ > > 1 file changed, 170 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/thine,thp7312.yaml > > > > diff --git a/Documentation/devicetree/bindings/media/thine,thp7312.yaml b/Documentation/devicetree/bindings/media/thine,thp7312.yaml > > new file mode 100644 > > index 000000000000..e8d203dcda81 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/thine,thp7312.yaml > > @@ -0,0 +1,170 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +# Copyright (c) 2023 Ideas on Board > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/thine,thp7312.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: THine THP7312 > > + > > +maintainers: > > + - Paul Elder <paul.elder@@ideasonboard.com> > > + > > +description: > > + The THP7312 is a standalone ISP controlled over i2c, and is capable of > > + various image processing and correction functions, including 3A control. It > > + can be connected to CMOS image sensors from various vendors, supporting both > > + MIPI CSI-2 and parallel interfaces. It can also output on either MIPI CSI-2 > > + or parallel. The hardware is capable of transmitting and receiving MIPI > > + interlaved data strams with data types or multiple virtual channel > > + identifiers. > > + > > +allOf: > > + - $ref: ../video-interface-devices.yaml# > > + > > +properties: > > + compatible: > > + const: thine,thp7312 > > + > > + reg: > > + description: I2C device address > > You can skip description. It is obvious. > > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > + - description: CLKI clock input > > This was absolutely never tested. Paul, before sending DT bindings, please test them. The procedure involves running `make dt_binding_check` as described towards the end of Documentation/devicetree/bindings/writing-schema.rst. There's an environment variable that you can use to restrict the test to a particular binding file. > > + > > + reset-gpios: > > + maxItems: 1 > > + description: |- > > + Reference to the GPIO connected to the RESET_N pin, if any. > > + Must be released (set high) after all supplies are applied. > > + > > + vddcore-supply: > > + description: > > + 1.2V supply for core, PLL, MIPI rx and MIPI tx. > > + > > + vhtermnx-supply: > > + description: > > + Supply for input (rx). 1.8V for MIPI, or 1.8/2.8/3.3V for parallel. > > + > > + vddtx-supply: > > + description: > > + Supply for output (tx). 1.8V for MIPI, or 1.8/2.8/3.3V for parallel. > > + > > + vddhost-supply: > > + description: > > + Supply for host interface. 1.8V, 2.8V, or 3.3V. > > + > > + vddcmos-supply: > > + description: > > + Supply for sensor interface. 1.8V, 2.8V, or 3.3V. > > + > > + vddgpio_0-supply: > > No, underscores are not allowed in names. > > > + description: > > + Supply for GPIO_0. 1.8V, 2.8V, or 3.3V. > > + > > + vddgpio_1-supply: > > + description: > > + Supply for GPIO_1. 1.8V, 2.8V, or 3.3V. > > + > > + DOVDD-supply: > > lowercase. Look at your other supplies. VDD is spelled there "vdd", so > do not introduce random style. > > > + description: > > + Digital I/O (1.8V) supply for image sensor. > > + > > + AVDD-supply: > > lowercase > > > + description: > > + Analog (2.8V) supply for image sensor. > > + > > + DVDD-supply: > > lowercase > > > + description: > > + Digital Core (1.2V) supply for image sensor. Are those three supplies required ? It looks like the vdd* supplies are all you need. > > + > > + orientation: true > > + rotation: true > > + > > + thine,rx,data-lanes: > > Why are you duplicating properties? With wrong name? No, that's not a > property of a device node, but endpoint. > > > + minItems: 4 > > + maxItems: 4 > > + $ref: /schemas/media/video-interfaces.yaml#data-lanes > > + description: |- > > Drop |- where not needed. > > > + This property is for lane reordering between the THP7312 and the imaging > > + sensor that it is connected to. > > + > > + port: > > + $ref: /schemas/graph.yaml#/$defs/port-base > > + additionalProperties: false > > + > > + properties: > > + endpoint: > > + $ref: /schemas/media/video-interfaces.yaml# > > + unevaluatedProperties: false > > + > > + properties: > > + data-lanes: > > + description: |- > > + The sensor supports either two-lane, or four-lane operation. > > + This property is for lane reordering between the THP7312 and > > + the SoC. If this property is omitted four-lane operation is > > + assumed. For two-lane operation the property must be set to <1 2>. > > + minItems: 2 > > + maxItems: 4 > > + items: > > + maximum: 4 > > + > > +required: > > + - compatible > > + - reg > > + - reset-gpios > > + - clocks > > + - vddcore-supply > > + - vhtermrx-supply > > + - vddtx-supply > > + - vddhost-supply > > + - vddcmos-supply > > + - vddgpio_0-supply > > + - vddgpio_1-supply > > + - DOVDD-supply > > + - AVDD-supply > > + - DVDD-supply > > + - thine,rx,data-lanes > > + - port > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/gpio/gpio.h> > > + > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + camera@61 { > > + compatible = "thine,thp7312"; > > + reg = <0x61>; > > + > > + pinctrl-names = "default"; > > + pinctrl-0 = <&cam1_pins_default>; > > + > > + reset-gpios = <&pio 119 GPIO_ACTIVE_LOW>; > > + clocks = <&camera61_clk>; > > + > > + vddcore-supply = <&vsys_v4p2>; > > + AVDD-supply = <&vsys_v4p2>; > > + DVDD-supply = <&vsys_v4p2>; > > Srlsy, test it before sending. Look how many supplies you require and > what is provided here. How any of this could possibly work?
On Wed, Sep 06, 2023 at 11:15:13AM +0300, Laurent Pinchart wrote: > On Wed, Sep 06, 2023 at 09:18:30AM +0200, Krzysztof Kozlowski wrote: > > On 06/09/2023 01:31, Paul Elder wrote: > > > Add bindings for the THine THP7312 ISP. > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > --- > > > Since the THP7312 supports multiple sensors, thine,rx-data-lanes alone > > > might not be enough. I was consdering using sensor nodes like what the > > > AP1302 does [1]. This way we can also move the power supplies that only > > > concern the sensor in there as well. I was wondering what to do about > > > the model name, though, as the thp7312 completely isolates that from the > > > rest of the system. > > > > > > I'm planning to add sensor nodes in somehow in a v2. > > > > > > [1] https://lore.kernel.org/linux-media/20211006113254.3470-2-anil.mamidala@xilinx.com/ > > > > > > .../bindings/media/thine,thp7312.yaml | 170 ++++++++++++++++++ > > > 1 file changed, 170 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/media/thine,thp7312.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/media/thine,thp7312.yaml b/Documentation/devicetree/bindings/media/thine,thp7312.yaml > > > new file mode 100644 > > > index 000000000000..e8d203dcda81 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/media/thine,thp7312.yaml > > > @@ -0,0 +1,170 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +# Copyright (c) 2023 Ideas on Board > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/media/thine,thp7312.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: THine THP7312 > > > + > > > +maintainers: > > > + - Paul Elder <paul.elder@@ideasonboard.com> > > > + > > > +description: > > > + The THP7312 is a standalone ISP controlled over i2c, and is capable of > > > + various image processing and correction functions, including 3A control. It > > > + can be connected to CMOS image sensors from various vendors, supporting both > > > + MIPI CSI-2 and parallel interfaces. It can also output on either MIPI CSI-2 > > > + or parallel. The hardware is capable of transmitting and receiving MIPI > > > + interlaved data strams with data types or multiple virtual channel > > > + identifiers. > > > + > > > +allOf: > > > + - $ref: ../video-interface-devices.yaml# > > > + > > > +properties: > > > + compatible: > > > + const: thine,thp7312 > > > + > > > + reg: > > > + description: I2C device address > > > > You can skip description. It is obvious. > > > > > + maxItems: 1 > > > + > > > + clocks: > > > + maxItems: 1 > > > + - description: CLKI clock input > > > > This was absolutely never tested. > > Paul, before sending DT bindings, please test them. The procedure > involves running `make dt_binding_check` as described towards the end of > Documentation/devicetree/bindings/writing-schema.rst. There's an > environment variable that you can use to restrict the test to a > particular binding file. > ack > > > + > > > + reset-gpios: > > > + maxItems: 1 > > > + description: |- > > > + Reference to the GPIO connected to the RESET_N pin, if any. > > > + Must be released (set high) after all supplies are applied. > > > + > > > + vddcore-supply: > > > + description: > > > + 1.2V supply for core, PLL, MIPI rx and MIPI tx. > > > + > > > + vhtermnx-supply: > > > + description: > > > + Supply for input (rx). 1.8V for MIPI, or 1.8/2.8/3.3V for parallel. > > > + > > > + vddtx-supply: > > > + description: > > > + Supply for output (tx). 1.8V for MIPI, or 1.8/2.8/3.3V for parallel. > > > + > > > + vddhost-supply: > > > + description: > > > + Supply for host interface. 1.8V, 2.8V, or 3.3V. > > > + > > > + vddcmos-supply: > > > + description: > > > + Supply for sensor interface. 1.8V, 2.8V, or 3.3V. > > > + > > > + vddgpio_0-supply: > > > > No, underscores are not allowed in names. > > > > > + description: > > > + Supply for GPIO_0. 1.8V, 2.8V, or 3.3V. > > > + > > > + vddgpio_1-supply: > > > + description: > > > + Supply for GPIO_1. 1.8V, 2.8V, or 3.3V. > > > + > > > + DOVDD-supply: > > > > lowercase. Look at your other supplies. VDD is spelled there "vdd", so > > do not introduce random style. > > > > > + description: > > > + Digital I/O (1.8V) supply for image sensor. > > > + > > > + AVDD-supply: > > > > lowercase > > > > > + description: > > > + Analog (2.8V) supply for image sensor. > > > + > > > + DVDD-supply: > > > > lowercase > > > > > + description: > > > + Digital Core (1.2V) supply for image sensor. > > Are those three supplies required ? It looks like the vdd* supplies are > all you need. The THSCG101 camera module has these connected to the connector connected to the sensor. Which don't even match with the supplies that are in the imx258 bindings, so I'm not sure how to express these; they're not part of the thp7312 but they're technically necessary for the camera module, but they're also not part of the sensor that the ISP is connected to. Paul > > > > + > > > + orientation: true > > > + rotation: true > > > + > > > + thine,rx,data-lanes: > > > > Why are you duplicating properties? With wrong name? No, that's not a > > property of a device node, but endpoint. > > > > > + minItems: 4 > > > + maxItems: 4 > > > + $ref: /schemas/media/video-interfaces.yaml#data-lanes > > > + description: |- > > > > Drop |- where not needed. > > > > > + This property is for lane reordering between the THP7312 and the imaging > > > + sensor that it is connected to. > > > + > > > + port: > > > + $ref: /schemas/graph.yaml#/$defs/port-base > > > + additionalProperties: false > > > + > > > + properties: > > > + endpoint: > > > + $ref: /schemas/media/video-interfaces.yaml# > > > + unevaluatedProperties: false > > > + > > > + properties: > > > + data-lanes: > > > + description: |- > > > + The sensor supports either two-lane, or four-lane operation. > > > + This property is for lane reordering between the THP7312 and > > > + the SoC. If this property is omitted four-lane operation is > > > + assumed. For two-lane operation the property must be set to <1 2>. > > > + minItems: 2 > > > + maxItems: 4 > > > + items: > > > + maximum: 4 > > > + > > > +required: > > > + - compatible > > > + - reg > > > + - reset-gpios > > > + - clocks > > > + - vddcore-supply > > > + - vhtermrx-supply > > > + - vddtx-supply > > > + - vddhost-supply > > > + - vddcmos-supply > > > + - vddgpio_0-supply > > > + - vddgpio_1-supply > > > + - DOVDD-supply > > > + - AVDD-supply > > > + - DVDD-supply > > > + - thine,rx,data-lanes > > > + - port > > > + > > > +additionalProperties: false > > > + > > > +examples: > > > + - | > > > + #include <dt-bindings/gpio/gpio.h> > > > + > > > + i2c { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + camera@61 { > > > + compatible = "thine,thp7312"; > > > + reg = <0x61>; > > > + > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&cam1_pins_default>; > > > + > > > + reset-gpios = <&pio 119 GPIO_ACTIVE_LOW>; > > > + clocks = <&camera61_clk>; > > > + > > > + vddcore-supply = <&vsys_v4p2>; > > > + AVDD-supply = <&vsys_v4p2>; > > > + DVDD-supply = <&vsys_v4p2>; > > > > Srlsy, test it before sending. Look how many supplies you require and > > what is provided here. How any of this could possibly work? > > -- > Regards, > > Laurent Pinchart
On Wed, Sep 06, 2023 at 09:18:30AM +0200, Krzysztof Kozlowski wrote: > On 06/09/2023 01:31, Paul Elder wrote: > > Add bindings for the THine THP7312 ISP. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > Since the THP7312 supports multiple sensors, thine,rx-data-lanes alone > > might not be enough. I was consdering using sensor nodes like what the > > AP1302 does [1]. This way we can also move the power supplies that only > > concern the sensor in there as well. I was wondering what to do about > > the model name, though, as the thp7312 completely isolates that from the > > rest of the system. > > > > I'm planning to add sensor nodes in somehow in a v2. > > > > [1] https://lore.kernel.org/linux-media/20211006113254.3470-2-anil.mamidala@xilinx.com/ > > > > .../bindings/media/thine,thp7312.yaml | 170 ++++++++++++++++++ > > 1 file changed, 170 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/thine,thp7312.yaml > > > > diff --git a/Documentation/devicetree/bindings/media/thine,thp7312.yaml b/Documentation/devicetree/bindings/media/thine,thp7312.yaml > > new file mode 100644 > > index 000000000000..e8d203dcda81 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/thine,thp7312.yaml > > @@ -0,0 +1,170 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +# Copyright (c) 2023 Ideas on Board > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/thine,thp7312.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: THine THP7312 > > + > > +maintainers: > > + - Paul Elder <paul.elder@@ideasonboard.com> > > + > > +description: > > + The THP7312 is a standalone ISP controlled over i2c, and is capable of > > + various image processing and correction functions, including 3A control. It > > + can be connected to CMOS image sensors from various vendors, supporting both > > + MIPI CSI-2 and parallel interfaces. It can also output on either MIPI CSI-2 > > + or parallel. The hardware is capable of transmitting and receiving MIPI > > + interlaved data strams with data types or multiple virtual channel > > + identifiers. > > + > > +allOf: > > + - $ref: ../video-interface-devices.yaml# > > + > > +properties: > > + compatible: > > + const: thine,thp7312 > > + > > + reg: > > + description: I2C device address > > You can skip description. It is obvious. ack > > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > + - description: CLKI clock input > > This was absolutely never tested. I'll admit, yes, I forgot to run the checks. But I did test it on hardware; it's just that this camera module is always powered and the clock is always connected so it wouldn't have been caught :/ > > > + > > + reset-gpios: > > + maxItems: 1 > > + description: |- > > + Reference to the GPIO connected to the RESET_N pin, if any. > > + Must be released (set high) after all supplies are applied. > > + > > + vddcore-supply: > > + description: > > + 1.2V supply for core, PLL, MIPI rx and MIPI tx. > > + > > + vhtermnx-supply: > > + description: > > + Supply for input (rx). 1.8V for MIPI, or 1.8/2.8/3.3V for parallel. > > + > > + vddtx-supply: > > + description: > > + Supply for output (tx). 1.8V for MIPI, or 1.8/2.8/3.3V for parallel. > > + > > + vddhost-supply: > > + description: > > + Supply for host interface. 1.8V, 2.8V, or 3.3V. > > + > > + vddcmos-supply: > > + description: > > + Supply for sensor interface. 1.8V, 2.8V, or 3.3V. > > + > > + vddgpio_0-supply: > > No, underscores are not allowed in names. > > > + description: > > + Supply for GPIO_0. 1.8V, 2.8V, or 3.3V. > > + > > + vddgpio_1-supply: > > + description: > > + Supply for GPIO_1. 1.8V, 2.8V, or 3.3V. > > + > > + DOVDD-supply: > > lowercase. Look at your other supplies. VDD is spelled there "vdd", so > do not introduce random style. > > > > + description: > > + Digital I/O (1.8V) supply for image sensor. > > + > > + AVDD-supply: > > lowercase > > > + description: > > + Analog (2.8V) supply for image sensor. > > + > > + DVDD-supply: > > lowercase > > > + description: > > + Digital Core (1.2V) supply for image sensor. > > + > > + orientation: true > > + rotation: true > > + > > + thine,rx,data-lanes: > > Why are you duplicating properties? With wrong name? No, that's not a > property of a device node, but endpoint. As mentioned elsewhere, it is not duplicated; it's for the input to the ISP. The data-lanes below is for the output of the ISP. And since the input to the ISP is completely isolated from the rest of the system (besides power, I suppose), it's kind of overkill to make an entire endpoint for it. I suppose the description that I wrote for this property was slightly too concise to convey that. I quite like the sensors node introduced in the AP1302; I hope that's a more acceptable solution? Paul > > > + minItems: 4 > > + maxItems: 4 > > + $ref: /schemas/media/video-interfaces.yaml#data-lanes > > + description: |- > > Drop |- where not needed. > > > + This property is for lane reordering between the THP7312 and the imaging > > + sensor that it is connected to. > > + > > + port: > > + $ref: /schemas/graph.yaml#/$defs/port-base > > + additionalProperties: false > > + > > + properties: > > + endpoint: > > + $ref: /schemas/media/video-interfaces.yaml# > > + unevaluatedProperties: false > > + > > + properties: > > + data-lanes: > > + description: |- > > + The sensor supports either two-lane, or four-lane operation. > > + This property is for lane reordering between the THP7312 and > > + the SoC. If this property is omitted four-lane operation is > > + assumed. For two-lane operation the property must be set to <1 2>. > > + minItems: 2 > > + maxItems: 4 > > + items: > > + maximum: 4 > > + > > +required: > > + - compatible > > + - reg > > + - reset-gpios > > + - clocks > > + - vddcore-supply > > + - vhtermrx-supply > > + - vddtx-supply > > + - vddhost-supply > > + - vddcmos-supply > > + - vddgpio_0-supply > > + - vddgpio_1-supply > > + - DOVDD-supply > > + - AVDD-supply > > + - DVDD-supply > > + - thine,rx,data-lanes > > + - port > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/gpio/gpio.h> > > + > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + camera@61 { > > + compatible = "thine,thp7312"; > > + reg = <0x61>; > > + > > + pinctrl-names = "default"; > > + pinctrl-0 = <&cam1_pins_default>; > > + > > + reset-gpios = <&pio 119 GPIO_ACTIVE_LOW>; > > + clocks = <&camera61_clk>; > > + > > + vddcore-supply = <&vsys_v4p2>; > > + AVDD-supply = <&vsys_v4p2>; > > + DVDD-supply = <&vsys_v4p2>; > > Srlsy, test it before sending. Look how many supplies you require and > what is provided here. How any of this could possibly work? > > > > Best regards, > Krzysztof >
On Thu, Sep 07, 2023 at 11:49:57PM +0900, Paul Elder wrote: > On Wed, Sep 06, 2023 at 11:15:13AM +0300, Laurent Pinchart wrote: > > On Wed, Sep 06, 2023 at 09:18:30AM +0200, Krzysztof Kozlowski wrote: > > > On 06/09/2023 01:31, Paul Elder wrote: > > > > Add bindings for the THine THP7312 ISP. > > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > --- > > > > Since the THP7312 supports multiple sensors, thine,rx-data-lanes alone > > > > might not be enough. I was consdering using sensor nodes like what the > > > > AP1302 does [1]. This way we can also move the power supplies that only > > > > concern the sensor in there as well. I was wondering what to do about > > > > the model name, though, as the thp7312 completely isolates that from the > > > > rest of the system. > > > > > > > > I'm planning to add sensor nodes in somehow in a v2. > > > > > > > > [1] https://lore.kernel.org/linux-media/20211006113254.3470-2-anil.mamidala@xilinx.com/ > > > > > > > > .../bindings/media/thine,thp7312.yaml | 170 ++++++++++++++++++ > > > > 1 file changed, 170 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/media/thine,thp7312.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/thine,thp7312.yaml b/Documentation/devicetree/bindings/media/thine,thp7312.yaml > > > > new file mode 100644 > > > > index 000000000000..e8d203dcda81 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/media/thine,thp7312.yaml > > > > @@ -0,0 +1,170 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > +# Copyright (c) 2023 Ideas on Board > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/media/thine,thp7312.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: THine THP7312 > > > > + > > > > +maintainers: > > > > + - Paul Elder <paul.elder@@ideasonboard.com> > > > > + > > > > +description: > > > > + The THP7312 is a standalone ISP controlled over i2c, and is capable of > > > > + various image processing and correction functions, including 3A control. It > > > > + can be connected to CMOS image sensors from various vendors, supporting both > > > > + MIPI CSI-2 and parallel interfaces. It can also output on either MIPI CSI-2 > > > > + or parallel. The hardware is capable of transmitting and receiving MIPI > > > > + interlaved data strams with data types or multiple virtual channel > > > > + identifiers. > > > > + > > > > +allOf: > > > > + - $ref: ../video-interface-devices.yaml# > > > > + > > > > +properties: > > > > + compatible: > > > > + const: thine,thp7312 > > > > + > > > > + reg: > > > > + description: I2C device address > > > > > > You can skip description. It is obvious. > > > > > > > + maxItems: 1 > > > > + > > > > + clocks: > > > > + maxItems: 1 > > > > + - description: CLKI clock input > > > > > > This was absolutely never tested. > > > > Paul, before sending DT bindings, please test them. The procedure > > involves running `make dt_binding_check` as described towards the end of > > Documentation/devicetree/bindings/writing-schema.rst. There's an > > environment variable that you can use to restrict the test to a > > particular binding file. > > ack > > > > > + > > > > + reset-gpios: > > > > + maxItems: 1 > > > > + description: |- > > > > + Reference to the GPIO connected to the RESET_N pin, if any. > > > > + Must be released (set high) after all supplies are applied. > > > > + > > > > + vddcore-supply: > > > > + description: > > > > + 1.2V supply for core, PLL, MIPI rx and MIPI tx. > > > > + > > > > + vhtermnx-supply: > > > > + description: > > > > + Supply for input (rx). 1.8V for MIPI, or 1.8/2.8/3.3V for parallel. > > > > + > > > > + vddtx-supply: > > > > + description: > > > > + Supply for output (tx). 1.8V for MIPI, or 1.8/2.8/3.3V for parallel. > > > > + > > > > + vddhost-supply: > > > > + description: > > > > + Supply for host interface. 1.8V, 2.8V, or 3.3V. > > > > + > > > > + vddcmos-supply: > > > > + description: > > > > + Supply for sensor interface. 1.8V, 2.8V, or 3.3V. > > > > + > > > > + vddgpio_0-supply: > > > > > > No, underscores are not allowed in names. > > > > > > > + description: > > > > + Supply for GPIO_0. 1.8V, 2.8V, or 3.3V. > > > > + > > > > + vddgpio_1-supply: > > > > + description: > > > > + Supply for GPIO_1. 1.8V, 2.8V, or 3.3V. > > > > + > > > > + DOVDD-supply: > > > > > > lowercase. Look at your other supplies. VDD is spelled there "vdd", so > > > do not introduce random style. > > > > > > > + description: > > > > + Digital I/O (1.8V) supply for image sensor. > > > > + > > > > + AVDD-supply: > > > > > > lowercase > > > > > > > + description: > > > > + Analog (2.8V) supply for image sensor. > > > > + > > > > + DVDD-supply: > > > > > > lowercase > > > > > > > + description: > > > > + Digital Core (1.2V) supply for image sensor. > > > > Are those three supplies required ? It looks like the vdd* supplies are > > all you need. > > The THSCG101 camera module has these connected to the connector > connected to the sensor. Which don't even match with the supplies that > are in the imx258 bindings, so I'm not sure how to express these; > they're not part of the thp7312 but they're technically necessary for > the camera module, but they're also not part of the sensor that the ISP > is connected to. This is the DT binding for the THP7312, not the THSCG101 camera module (for readers who are not familiar with this, the THSCG101 is a module that integrates the THP7312 ISP, a sensor module, and glue such as level shifters or regulators). From the point of view of the THP7312 DT binding, the THSCG101 is irrelevant. The binding need to expose the supplies needed by the THP7312 (and, possibly, by the sensor module). > > > > + > > > > + orientation: true > > > > + rotation: true > > > > + > > > > + thine,rx,data-lanes: > > > > > > Why are you duplicating properties? With wrong name? No, that's not a > > > property of a device node, but endpoint. > > > > > > > + minItems: 4 > > > > + maxItems: 4 > > > > + $ref: /schemas/media/video-interfaces.yaml#data-lanes > > > > + description: |- > > > > > > Drop |- where not needed. > > > > > > > + This property is for lane reordering between the THP7312 and the imaging > > > > + sensor that it is connected to. > > > > + > > > > + port: > > > > + $ref: /schemas/graph.yaml#/$defs/port-base > > > > + additionalProperties: false > > > > + > > > > + properties: > > > > + endpoint: > > > > + $ref: /schemas/media/video-interfaces.yaml# > > > > + unevaluatedProperties: false > > > > + > > > > + properties: > > > > + data-lanes: > > > > + description: |- > > > > + The sensor supports either two-lane, or four-lane operation. > > > > + This property is for lane reordering between the THP7312 and > > > > + the SoC. If this property is omitted four-lane operation is > > > > + assumed. For two-lane operation the property must be set to <1 2>. > > > > + minItems: 2 > > > > + maxItems: 4 > > > > + items: > > > > + maximum: 4 > > > > + > > > > +required: > > > > + - compatible > > > > + - reg > > > > + - reset-gpios > > > > + - clocks > > > > + - vddcore-supply > > > > + - vhtermrx-supply > > > > + - vddtx-supply > > > > + - vddhost-supply > > > > + - vddcmos-supply > > > > + - vddgpio_0-supply > > > > + - vddgpio_1-supply > > > > + - DOVDD-supply > > > > + - AVDD-supply > > > > + - DVDD-supply > > > > + - thine,rx,data-lanes > > > > + - port > > > > + > > > > +additionalProperties: false > > > > + > > > > +examples: > > > > + - | > > > > + #include <dt-bindings/gpio/gpio.h> > > > > + > > > > + i2c { > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + > > > > + camera@61 { > > > > + compatible = "thine,thp7312"; > > > > + reg = <0x61>; > > > > + > > > > + pinctrl-names = "default"; > > > > + pinctrl-0 = <&cam1_pins_default>; > > > > + > > > > + reset-gpios = <&pio 119 GPIO_ACTIVE_LOW>; > > > > + clocks = <&camera61_clk>; > > > > + > > > > + vddcore-supply = <&vsys_v4p2>; > > > > + AVDD-supply = <&vsys_v4p2>; > > > > + DVDD-supply = <&vsys_v4p2>; > > > > > > Srlsy, test it before sending. Look how many supplies you require and > > > what is provided here. How any of this could possibly work?
diff --git a/Documentation/devicetree/bindings/media/thine,thp7312.yaml b/Documentation/devicetree/bindings/media/thine,thp7312.yaml new file mode 100644 index 000000000000..e8d203dcda81 --- /dev/null +++ b/Documentation/devicetree/bindings/media/thine,thp7312.yaml @@ -0,0 +1,170 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright (c) 2023 Ideas on Board +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/thine,thp7312.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: THine THP7312 + +maintainers: + - Paul Elder <paul.elder@@ideasonboard.com> + +description: + The THP7312 is a standalone ISP controlled over i2c, and is capable of + various image processing and correction functions, including 3A control. It + can be connected to CMOS image sensors from various vendors, supporting both + MIPI CSI-2 and parallel interfaces. It can also output on either MIPI CSI-2 + or parallel. The hardware is capable of transmitting and receiving MIPI + interlaved data strams with data types or multiple virtual channel + identifiers. + +allOf: + - $ref: ../video-interface-devices.yaml# + +properties: + compatible: + const: thine,thp7312 + + reg: + description: I2C device address + maxItems: 1 + + clocks: + maxItems: 1 + - description: CLKI clock input + + reset-gpios: + maxItems: 1 + description: |- + Reference to the GPIO connected to the RESET_N pin, if any. + Must be released (set high) after all supplies are applied. + + vddcore-supply: + description: + 1.2V supply for core, PLL, MIPI rx and MIPI tx. + + vhtermnx-supply: + description: + Supply for input (rx). 1.8V for MIPI, or 1.8/2.8/3.3V for parallel. + + vddtx-supply: + description: + Supply for output (tx). 1.8V for MIPI, or 1.8/2.8/3.3V for parallel. + + vddhost-supply: + description: + Supply for host interface. 1.8V, 2.8V, or 3.3V. + + vddcmos-supply: + description: + Supply for sensor interface. 1.8V, 2.8V, or 3.3V. + + vddgpio_0-supply: + description: + Supply for GPIO_0. 1.8V, 2.8V, or 3.3V. + + vddgpio_1-supply: + description: + Supply for GPIO_1. 1.8V, 2.8V, or 3.3V. + + DOVDD-supply: + description: + Digital I/O (1.8V) supply for image sensor. + + AVDD-supply: + description: + Analog (2.8V) supply for image sensor. + + DVDD-supply: + description: + Digital Core (1.2V) supply for image sensor. + + orientation: true + rotation: true + + thine,rx,data-lanes: + minItems: 4 + maxItems: 4 + $ref: /schemas/media/video-interfaces.yaml#data-lanes + description: |- + This property is for lane reordering between the THP7312 and the imaging + sensor that it is connected to. + + port: + $ref: /schemas/graph.yaml#/$defs/port-base + additionalProperties: false + + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + description: |- + The sensor supports either two-lane, or four-lane operation. + This property is for lane reordering between the THP7312 and + the SoC. If this property is omitted four-lane operation is + assumed. For two-lane operation the property must be set to <1 2>. + minItems: 2 + maxItems: 4 + items: + maximum: 4 + +required: + - compatible + - reg + - reset-gpios + - clocks + - vddcore-supply + - vhtermrx-supply + - vddtx-supply + - vddhost-supply + - vddcmos-supply + - vddgpio_0-supply + - vddgpio_1-supply + - DOVDD-supply + - AVDD-supply + - DVDD-supply + - thine,rx,data-lanes + - port + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + camera@61 { + compatible = "thine,thp7312"; + reg = <0x61>; + + pinctrl-names = "default"; + pinctrl-0 = <&cam1_pins_default>; + + reset-gpios = <&pio 119 GPIO_ACTIVE_LOW>; + clocks = <&camera61_clk>; + + vddcore-supply = <&vsys_v4p2>; + AVDD-supply = <&vsys_v4p2>; + DVDD-supply = <&vsys_v4p2>; + + orientation = <0>; + rotation = <0>; + + thine,rx,data-lanes = <4 1 3 2>; + + port { + thp7312_2_endpoint: endpoint { + remote-endpoint = <&mipi_thp7312_2>; + data-lanes = <4 2 1 3>; + }; + }; + }; + }; +...
Add bindings for the THine THP7312 ISP. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- Since the THP7312 supports multiple sensors, thine,rx-data-lanes alone might not be enough. I was consdering using sensor nodes like what the AP1302 does [1]. This way we can also move the power supplies that only concern the sensor in there as well. I was wondering what to do about the model name, though, as the thp7312 completely isolates that from the rest of the system. I'm planning to add sensor nodes in somehow in a v2. [1] https://lore.kernel.org/linux-media/20211006113254.3470-2-anil.mamidala@xilinx.com/ .../bindings/media/thine,thp7312.yaml | 170 ++++++++++++++++++ 1 file changed, 170 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/thine,thp7312.yaml