Re: [PATCH v2 1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor
On Tue, 22 Sep 2020 at 21:23, Rob Herring wrote: > > > > If this is the only config, why does it need to be in DT? > > > > The sensor is capable of two settings: two lanes (1 and 2) and four > > lanes described above. However Linux driver requires the latter (four > > lanes, 1+2+3+4). > > > > If I were to describe the bindings for HW, someone would really be > > confused and try to use two lanes setup, which won't work. Driver won't > > allow it. > > If someone has h/w with only 2 lanes connected, then they have to go > add support to the driver whether we've documented 2 lanes in the > binding or not. > > > I understand that bindings document the HW and describe its interface > > but do we really want to put "theoretical" bindings which cannot be used > > in practice with Linux kernel? > > > > If yes, how to nicely document this that only one setting is currently > > working? > > You don't, at least in the binding. That's a driver issue. Bindings > are separate. They are stored in the kernel tree for convenience, not > because they are part of the kernel. Mhmm... okay. I already sent v3 for this with fixed four lanes so I will re-spin with 2 or 4 lanes setup. Thanks for the review! Best regards, Krzysztof
Re: [PATCH v2 1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor
On Mon, Sep 21, 2020 at 5:27 AM Krzysztof Kozlowski wrote: > > On Mon, Sep 14, 2020 at 02:13:10PM -0600, Rob Herring wrote: > > On Wed, Sep 02, 2020 at 09:18:08AM +0200, Krzysztof Kozlowski wrote: > > > Add bindings for the IMX258 camera sensor. The bindings, just like the > > > driver, are quite limited, e.g. do not support regulator supplies. > > > > Bindings should be complete, not what a driver happens to currently > > support. > > I'll add then more complete picture. > > > > > > > > > Signed-off-by: Krzysztof Kozlowski > > > > > > --- > > > > > > Changes since v1: > > > 1. None > > > --- > > > .../devicetree/bindings/media/i2c/imx258.yaml | 92 > > > ++ > > > MAINTAINERS| 1 + > > > 2 files changed, 93 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/media/i2c/imx258.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/imx258.yaml > > > b/Documentation/devicetree/bindings/media/i2c/imx258.yaml > > > new file mode 100644 > > > index ..ef789ad31143 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/media/i2c/imx258.yaml > > > @@ -0,0 +1,92 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/media/i2c/imx258.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Sony IMX258 13 Mpixel CMOS Digital Image Sensor > > > + > > > +maintainers: > > > + - Krzysztof Kozlowski > > > + > > > +description: |- > > > + IMX258 is a diagonal 5.867mm (Type 1/3.06) 13 Mega-pixel CMOS active > > > pixel > > > + type stacked image sensor with a square pixel array of size 4208 x > > > 3120. It > > > + is programmable through I2C interface. Image data is sent through MIPI > > > + CSI-2. > > > + > > > +properties: > > > + compatible: > > > +const: sony,imx258 > > > + > > > + clocks: > > > +maxItems: 1 > > > + > > > + clock-frequency: > > > +description: Frequency of input clock if clock is not provided > > > +deprecated: true > > > > Why are we adding something deprecated on a new binding? > > My intention was also to document it but indeed easier to skip it. > > > > > > +const: 1920 > > > + > > > + reg: > > > +maxItems: 1 > > > + > > > + # See ../video-interfaces.txt for more details > > > + port: > > > +type: object > > > +properties: > > > + endpoint: > > > +type: object > > > +properties: > > > + data-lanes: > > > +items: > > > + - const: 1 > > > + - const: 2 > > > + - const: 3 > > > + - const: 4 > > > > If this is the only config, why does it need to be in DT? > > The sensor is capable of two settings: two lanes (1 and 2) and four > lanes described above. However Linux driver requires the latter (four > lanes, 1+2+3+4). > > If I were to describe the bindings for HW, someone would really be > confused and try to use two lanes setup, which won't work. Driver won't > allow it. If someone has h/w with only 2 lanes connected, then they have to go add support to the driver whether we've documented 2 lanes in the binding or not. > I understand that bindings document the HW and describe its interface > but do we really want to put "theoretical" bindings which cannot be used > in practice with Linux kernel? > > If yes, how to nicely document this that only one setting is currently > working? You don't, at least in the binding. That's a driver issue. Bindings are separate. They are stored in the kernel tree for convenience, not because they are part of the kernel. Rob
Re: [PATCH v2 1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor
On Mon, Sep 14, 2020 at 02:13:10PM -0600, Rob Herring wrote: > On Wed, Sep 02, 2020 at 09:18:08AM +0200, Krzysztof Kozlowski wrote: > > Add bindings for the IMX258 camera sensor. The bindings, just like the > > driver, are quite limited, e.g. do not support regulator supplies. > > Bindings should be complete, not what a driver happens to currently > support. I'll add then more complete picture. > > > > > Signed-off-by: Krzysztof Kozlowski > > > > --- > > > > Changes since v1: > > 1. None > > --- > > .../devicetree/bindings/media/i2c/imx258.yaml | 92 > > ++ > > MAINTAINERS| 1 + > > 2 files changed, 93 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/i2c/imx258.yaml > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/imx258.yaml > > b/Documentation/devicetree/bindings/media/i2c/imx258.yaml > > new file mode 100644 > > index ..ef789ad31143 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/i2c/imx258.yaml > > @@ -0,0 +1,92 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/i2c/imx258.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Sony IMX258 13 Mpixel CMOS Digital Image Sensor > > + > > +maintainers: > > + - Krzysztof Kozlowski > > + > > +description: |- > > + IMX258 is a diagonal 5.867mm (Type 1/3.06) 13 Mega-pixel CMOS active > > pixel > > + type stacked image sensor with a square pixel array of size 4208 x 3120. > > It > > + is programmable through I2C interface. Image data is sent through MIPI > > + CSI-2. > > + > > +properties: > > + compatible: > > +const: sony,imx258 > > + > > + clocks: > > +maxItems: 1 > > + > > + clock-frequency: > > +description: Frequency of input clock if clock is not provided > > +deprecated: true > > Why are we adding something deprecated on a new binding? My intention was also to document it but indeed easier to skip it. > > > +const: 1920 > > + > > + reg: > > +maxItems: 1 > > + > > + # See ../video-interfaces.txt for more details > > + port: > > +type: object > > +properties: > > + endpoint: > > +type: object > > +properties: > > + data-lanes: > > +items: > > + - const: 1 > > + - const: 2 > > + - const: 3 > > + - const: 4 > > If this is the only config, why does it need to be in DT? The sensor is capable of two settings: two lanes (1 and 2) and four lanes described above. However Linux driver requires the latter (four lanes, 1+2+3+4). If I were to describe the bindings for HW, someone would really be confused and try to use two lanes setup, which won't work. Driver won't allow it. I understand that bindings document the HW and describe its interface but do we really want to put "theoretical" bindings which cannot be used in practice with Linux kernel? If yes, how to nicely document this that only one setting is currently working? > > > + > > + link-frequencies: > > +allOf: > > + - $ref: /schemas/types.yaml#/definitions/uint64-array > > +description: > > + Allowed data bus frequencies. > > + > > +required: > > + - data-lanes > > + - link-frequencies > > + > > +required: > > + - compatible > > + - reg > > + - port > > + > > + > > +if: > > + not: > > +required: > > + - clocks > > +then: > > + required: > > +- clock-frequency > > + > > +unevaluatedProperties: false > > additionalProperties Right. Best regards, Krzysztof
Re: [PATCH v2 1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor
On Wed, Sep 02, 2020 at 09:18:08AM +0200, Krzysztof Kozlowski wrote:
> Add bindings for the IMX258 camera sensor. The bindings, just like the
> driver, are quite limited, e.g. do not support regulator supplies.
Bindings should be complete, not what a driver happens to currently
support.
>
> Signed-off-by: Krzysztof Kozlowski
>
> ---
>
> Changes since v1:
> 1. None
> ---
> .../devicetree/bindings/media/i2c/imx258.yaml | 92
> ++
> MAINTAINERS| 1 +
> 2 files changed, 93 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/imx258.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/imx258.yaml
> b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
> new file mode 100644
> index ..ef789ad31143
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
> @@ -0,0 +1,92 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/imx258.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sony IMX258 13 Mpixel CMOS Digital Image Sensor
> +
> +maintainers:
> + - Krzysztof Kozlowski
> +
> +description: |-
> + IMX258 is a diagonal 5.867mm (Type 1/3.06) 13 Mega-pixel CMOS active pixel
> + type stacked image sensor with a square pixel array of size 4208 x 3120. It
> + is programmable through I2C interface. Image data is sent through MIPI
> + CSI-2.
> +
> +properties:
> + compatible:
> +const: sony,imx258
> +
> + clocks:
> +maxItems: 1
> +
> + clock-frequency:
> +description: Frequency of input clock if clock is not provided
> +deprecated: true
Why are we adding something deprecated on a new binding?
> +const: 1920
> +
> + reg:
> +maxItems: 1
> +
> + # See ../video-interfaces.txt for more details
> + port:
> +type: object
> +properties:
> + endpoint:
> +type: object
> +properties:
> + data-lanes:
> +items:
> + - const: 1
> + - const: 2
> + - const: 3
> + - const: 4
If this is the only config, why does it need to be in DT?
> +
> + link-frequencies:
> +allOf:
> + - $ref: /schemas/types.yaml#/definitions/uint64-array
> +description:
> + Allowed data bus frequencies.
> +
> +required:
> + - data-lanes
> + - link-frequencies
> +
> +required:
> + - compatible
> + - reg
> + - port
> +
> +
> +if:
> + not:
> +required:
> + - clocks
> +then:
> + required:
> +- clock-frequency
> +
> +unevaluatedProperties: false
additionalProperties
> +
> +examples:
> + - |
> +i2c0 {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +sensor@6c {
> +compatible = "sony,imx258";
> +reg = <0x6c>;
> +clocks = <_clk>;
> +
> +port {
> +imx258_ep: endpoint {
> +remote-endpoint = <_ep>;
> +data-lanes = <1 2 3 4>;
> +link-frequencies = /bits/ 64 <32000>;
> +};
> +};
> +};
> +};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dc2aa691b75a..cca13d483d22 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16130,6 +16130,7 @@ M:Sakari Ailus
> L: linux-me...@vger.kernel.org
> S: Maintained
> T: git git://linuxtv.org/media_tree.git
> +F: Documentation/devicetree/bindings/media/i2c/imx258.yaml
> F: drivers/media/i2c/imx258.c
>
> SONY IMX274 SENSOR DRIVER
> --
> 2.7.4
>
[PATCH v2 1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor
Add bindings for the IMX258 camera sensor. The bindings, just like the
driver, are quite limited, e.g. do not support regulator supplies.
Signed-off-by: Krzysztof Kozlowski
---
Changes since v1:
1. None
---
.../devicetree/bindings/media/i2c/imx258.yaml | 92 ++
MAINTAINERS| 1 +
2 files changed, 93 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/i2c/imx258.yaml
diff --git a/Documentation/devicetree/bindings/media/i2c/imx258.yaml
b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
new file mode 100644
index ..ef789ad31143
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
@@ -0,0 +1,92 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/imx258.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sony IMX258 13 Mpixel CMOS Digital Image Sensor
+
+maintainers:
+ - Krzysztof Kozlowski
+
+description: |-
+ IMX258 is a diagonal 5.867mm (Type 1/3.06) 13 Mega-pixel CMOS active pixel
+ type stacked image sensor with a square pixel array of size 4208 x 3120. It
+ is programmable through I2C interface. Image data is sent through MIPI
+ CSI-2.
+
+properties:
+ compatible:
+const: sony,imx258
+
+ clocks:
+maxItems: 1
+
+ clock-frequency:
+description: Frequency of input clock if clock is not provided
+deprecated: true
+const: 1920
+
+ reg:
+maxItems: 1
+
+ # See ../video-interfaces.txt for more details
+ port:
+type: object
+properties:
+ endpoint:
+type: object
+properties:
+ data-lanes:
+items:
+ - const: 1
+ - const: 2
+ - const: 3
+ - const: 4
+
+ link-frequencies:
+allOf:
+ - $ref: /schemas/types.yaml#/definitions/uint64-array
+description:
+ Allowed data bus frequencies.
+
+required:
+ - data-lanes
+ - link-frequencies
+
+required:
+ - compatible
+ - reg
+ - port
+
+
+if:
+ not:
+required:
+ - clocks
+then:
+ required:
+- clock-frequency
+
+unevaluatedProperties: false
+
+examples:
+ - |
+i2c0 {
+#address-cells = <1>;
+#size-cells = <0>;
+
+sensor@6c {
+compatible = "sony,imx258";
+reg = <0x6c>;
+clocks = <_clk>;
+
+port {
+imx258_ep: endpoint {
+remote-endpoint = <_ep>;
+data-lanes = <1 2 3 4>;
+link-frequencies = /bits/ 64 <32000>;
+};
+};
+};
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index dc2aa691b75a..cca13d483d22 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16130,6 +16130,7 @@ M: Sakari Ailus
L: linux-me...@vger.kernel.org
S: Maintained
T: git git://linuxtv.org/media_tree.git
+F: Documentation/devicetree/bindings/media/i2c/imx258.yaml
F: drivers/media/i2c/imx258.c
SONY IMX274 SENSOR DRIVER
--
2.7.4
