linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor
@ 2020-11-18 20:27 Krzysztof Kozlowski
  2020-11-18 20:27 ` [PATCH v6 2/4] media: i2c: imx258: add support for binding via device tree Krzysztof Kozlowski
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Krzysztof Kozlowski @ 2020-11-18 20:27 UTC (permalink / raw)
  To: Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, linux-media,
	devicetree, linux-arm-kernel, linux-kernel
  Cc: Krzysztof Kozlowski, Rob Herring

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 <krzk@kernel.org>
Reviewed-by: Rob Herring <robh@kernel.org>

---

Changes since v4:
1. Add clock-lanes,
2. Add Rob's review,
3. Add one more example and extend existing one,
4. Add common clock properties (assigned-*).

Changes since v3:
1. Document also two lane setup.

Changes since v2:
1. Remove clock-frequency, add reset GPIOs, add supplies.
2. Use additionalProperties.

Changes since v1:
1. None
---
 .../devicetree/bindings/media/i2c/imx258.yaml | 140 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 141 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 000000000000..4a3471fb88a1
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
@@ -0,0 +1,140 @@
+# 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 <krzk@kernel.org>
+
+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
+
+  assigned-clocks: true
+  assigned-clock-parents: true
+  assigned-clock-rates: true
+
+  clocks:
+    description:
+      Clock frequency from 6 to 27 MHz.
+    maxItems: 1
+
+  reg:
+    maxItems: 1
+
+  reset-gpios:
+    description: |-
+      Reference to the GPIO connected to the XCLR pin, if any.
+
+  vana-supply:
+    description:
+      Analog voltage (VANA) supply, 2.7 V
+
+  vdig-supply:
+    description:
+      Digital I/O voltage (VDIG) supply, 1.2 V
+
+  vif-supply:
+    description:
+      Interface voltage (VIF) supply, 1.8 V
+
+  # See ../video-interfaces.txt for more details
+  port:
+    type: object
+    properties:
+      endpoint:
+        type: object
+        properties:
+          clock-lanes:
+            const: 0
+
+          data-lanes:
+            oneOf:
+              - items:
+                  - const: 1
+                  - const: 2
+                  - const: 3
+                  - const: 4
+              - items:
+                  - const: 1
+                  - const: 2
+
+          link-frequencies:
+            allOf:
+              - $ref: /schemas/types.yaml#/definitions/uint64-array
+            description:
+              Allowed data bus frequencies.
+
+        required:
+          - clock-lanes
+          - data-lanes
+          - link-frequencies
+
+required:
+  - compatible
+  - reg
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        sensor@6c {
+            compatible = "sony,imx258";
+            reg = <0x6c>;
+            clocks = <&imx258_clk>;
+
+            port {
+                endpoint {
+                    remote-endpoint = <&csi1_ep>;
+                    clock-lanes = <0>;
+                    data-lanes = <1 2 3 4>;
+                    link-frequencies = /bits/ 64 <320000000>;
+                };
+            };
+        };
+    };
+
+    /* Oscillator on the camera board */
+    imx258_clk: clk {
+        compatible = "fixed-clock";
+        #clock-cells = <0>;
+        clock-frequency = <19200000>;
+    };
+
+  - |
+    i2c0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        sensor@6c {
+            compatible = "sony,imx258";
+            reg = <0x6c>;
+            clocks = <&imx258_clk>;
+
+            assigned-clocks = <&imx258_clk>;
+            assigned-clock-rates = <19200000>;
+
+            port {
+                endpoint {
+                    remote-endpoint = <&csi1_ep>;
+                    clock-lanes = <0>;
+                    data-lanes = <1 2 3 4>;
+                    link-frequencies = /bits/ 64 <633600000>;
+                };
+            };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 82ea6867ac2e..96ca9d2857db 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16395,6 +16395,7 @@ M:	Sakari Ailus <sakari.ailus@linux.intel.com>
 L:	linux-media@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.25.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v6 2/4] media: i2c: imx258: add support for binding via device tree
  2020-11-18 20:27 [PATCH v6 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor Krzysztof Kozlowski
@ 2020-11-18 20:27 ` Krzysztof Kozlowski
  2020-11-18 20:27 ` [PATCH v6 3/4] media: i2c: imx258: simplify getting state container Krzysztof Kozlowski
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Krzysztof Kozlowski @ 2020-11-18 20:27 UTC (permalink / raw)
  To: Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, linux-media,
	devicetree, linux-arm-kernel, linux-kernel
  Cc: Krzysztof Kozlowski

The IMX258 can be used also on embedded designs using device tree so
allow the sensor to bind to a device tree node.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

---

Changes since v1:
1. None
---
 drivers/media/i2c/imx258.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index df62c69a48c0..ba7f29622974 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -1291,11 +1291,18 @@ static const struct acpi_device_id imx258_acpi_ids[] = {
 MODULE_DEVICE_TABLE(acpi, imx258_acpi_ids);
 #endif
 
+static const struct of_device_id imx258_dt_ids[] = {
+	{ .compatible = "sony,imx258" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx258_dt_ids);
+
 static struct i2c_driver imx258_i2c_driver = {
 	.driver = {
 		.name = "imx258",
 		.pm = &imx258_pm_ops,
 		.acpi_match_table = ACPI_PTR(imx258_acpi_ids),
+		.of_match_table	= imx258_dt_ids,
 	},
 	.probe_new = imx258_probe,
 	.remove = imx258_remove,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v6 3/4] media: i2c: imx258: simplify getting state container
  2020-11-18 20:27 [PATCH v6 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor Krzysztof Kozlowski
  2020-11-18 20:27 ` [PATCH v6 2/4] media: i2c: imx258: add support for binding via device tree Krzysztof Kozlowski
@ 2020-11-18 20:27 ` Krzysztof Kozlowski
  2020-11-18 20:27 ` [PATCH v6 4/4] media: i2c: imx258: get clock from device properties and enable it via runtime PM Krzysztof Kozlowski
  2021-01-22  9:18 ` [PATCH v6 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor Sakari Ailus
  3 siblings, 0 replies; 9+ messages in thread
From: Krzysztof Kozlowski @ 2020-11-18 20:27 UTC (permalink / raw)
  To: Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, linux-media,
	devicetree, linux-arm-kernel, linux-kernel
  Cc: Krzysztof Kozlowski

The pointer to 'struct v4l2_subdev' is stored in drvdata via
v4l2_i2c_subdev_init() so there is no point of a dance like:

struct i2c_client *client = to_i2c_client(struct device *dev)
struct v4l2_subdev *sd = i2c_get_clientdata(client);

This allows to remove local variable 'client' and few pointer
dereferences.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

---

Changes since v3:
1. None

Changes since v2:
1. New patch
---
 drivers/media/i2c/imx258.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index ba7f29622974..505981e02cff 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -1018,8 +1018,7 @@ static int imx258_set_stream(struct v4l2_subdev *sd, int enable)
 
 static int __maybe_unused imx258_suspend(struct device *dev)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct imx258 *imx258 = to_imx258(sd);
 
 	if (imx258->streaming)
@@ -1030,8 +1029,7 @@ static int __maybe_unused imx258_suspend(struct device *dev)
 
 static int __maybe_unused imx258_resume(struct device *dev)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct imx258 *imx258 = to_imx258(sd);
 	int ret;
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v6 4/4] media: i2c: imx258: get clock from device properties and enable it via runtime PM
  2020-11-18 20:27 [PATCH v6 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor Krzysztof Kozlowski
  2020-11-18 20:27 ` [PATCH v6 2/4] media: i2c: imx258: add support for binding via device tree Krzysztof Kozlowski
  2020-11-18 20:27 ` [PATCH v6 3/4] media: i2c: imx258: simplify getting state container Krzysztof Kozlowski
@ 2020-11-18 20:27 ` Krzysztof Kozlowski
  2021-01-22  9:18 ` [PATCH v6 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor Sakari Ailus
  3 siblings, 0 replies; 9+ messages in thread
From: Krzysztof Kozlowski @ 2020-11-18 20:27 UTC (permalink / raw)
  To: Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, linux-media,
	devicetree, linux-arm-kernel, linux-kernel
  Cc: Krzysztof Kozlowski

The IMX258 sensor driver checked in device properties for a
clock-frequency property which actually does not mean that the clock is
really running such frequency or is it even enabled.

Get the provided clock and check it frequency.  If none is provided,
fall back to old property.

Enable the clock when accessing the IMX258 registers and when streaming
starts with runtime PM.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

---

Changes since v5:
1. Move clk_get_rate() out of else block.

Changes since v4:
1. Add missing imx258_power_off.

Changes since v3:
1. None

Changes since v2:
1. Do not try to set drvdata, wrap lines.
2. Use dev_dbg.

Changes since v1:
1. Use runtime PM for clock toggling
---
 drivers/media/i2c/imx258.c | 69 +++++++++++++++++++++++++++++++++-----
 1 file changed, 61 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index 505981e02cff..61d74b794582 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -2,6 +2,7 @@
 // Copyright (C) 2018 Intel Corporation
 
 #include <linux/acpi.h>
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
@@ -68,6 +69,9 @@
 #define REG_CONFIG_MIRROR_FLIP		0x03
 #define REG_CONFIG_FLIP_TEST_PATTERN	0x02
 
+/* Input clock frequency in Hz */
+#define IMX258_INPUT_CLOCK_FREQ		19200000
+
 struct imx258_reg {
 	u16 address;
 	u8 val;
@@ -610,6 +614,8 @@ struct imx258 {
 
 	/* Streaming on/off */
 	bool streaming;
+
+	struct clk *clk;
 };
 
 static inline struct imx258 *to_imx258(struct v4l2_subdev *_sd)
@@ -972,6 +978,29 @@ static int imx258_stop_streaming(struct imx258 *imx258)
 	return 0;
 }
 
+static int imx258_power_on(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct imx258 *imx258 = to_imx258(sd);
+	int ret;
+
+	ret = clk_prepare_enable(imx258->clk);
+	if (ret)
+		dev_err(dev, "failed to enable clock\n");
+
+	return ret;
+}
+
+static int imx258_power_off(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct imx258 *imx258 = to_imx258(sd);
+
+	clk_disable_unprepare(imx258->clk);
+
+	return 0;
+}
+
 static int imx258_set_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct imx258 *imx258 = to_imx258(sd);
@@ -1199,9 +1228,26 @@ static int imx258_probe(struct i2c_client *client)
 	int ret;
 	u32 val = 0;
 
-	device_property_read_u32(&client->dev, "clock-frequency", &val);
-	if (val != 19200000)
+	imx258 = devm_kzalloc(&client->dev, sizeof(*imx258), GFP_KERNEL);
+	if (!imx258)
+		return -ENOMEM;
+
+	imx258->clk = devm_clk_get_optional(&client->dev, NULL);
+	if (!imx258->clk) {
+		dev_dbg(&client->dev,
+			"no clock provided, using clock-frequency property\n");
+
+		device_property_read_u32(&client->dev, "clock-frequency", &val);
+		if (val != IMX258_INPUT_CLOCK_FREQ)
+			return -EINVAL;
+	} else if (IS_ERR(imx258->clk)) {
+		return dev_err_probe(&client->dev, PTR_ERR(imx258->clk),
+				     "error getting clock\n");
+	}
+	if (clk_get_rate(imx258->clk) != IMX258_INPUT_CLOCK_FREQ) {
+		dev_err(&client->dev, "input clock frequency not supported\n");
 		return -EINVAL;
+	}
 
 	/*
 	 * Check that the device is mounted upside down. The driver only
@@ -1211,24 +1257,25 @@ static int imx258_probe(struct i2c_client *client)
 	if (ret || val != 180)
 		return -EINVAL;
 
-	imx258 = devm_kzalloc(&client->dev, sizeof(*imx258), GFP_KERNEL);
-	if (!imx258)
-		return -ENOMEM;
-
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&imx258->sd, client, &imx258_subdev_ops);
 
+	/* Will be powered off via pm_runtime_idle */
+	ret = imx258_power_on(&client->dev);
+	if (ret)
+		return ret;
+
 	/* Check module identity */
 	ret = imx258_identify_module(imx258);
 	if (ret)
-		return ret;
+		goto error_identify;
 
 	/* Set default mode to max resolution */
 	imx258->cur_mode = &supported_modes[0];
 
 	ret = imx258_init_controls(imx258);
 	if (ret)
-		return ret;
+		goto error_identify;
 
 	/* Initialize subdev */
 	imx258->sd.internal_ops = &imx258_internal_ops;
@@ -1258,6 +1305,9 @@ static int imx258_probe(struct i2c_client *client)
 error_handler_free:
 	imx258_free_controls(imx258);
 
+error_identify:
+	imx258_power_off(&client->dev);
+
 	return ret;
 }
 
@@ -1271,6 +1321,8 @@ static int imx258_remove(struct i2c_client *client)
 	imx258_free_controls(imx258);
 
 	pm_runtime_disable(&client->dev);
+	if (!pm_runtime_status_suspended(&client->dev))
+		imx258_power_off(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
 
 	return 0;
@@ -1278,6 +1330,7 @@ static int imx258_remove(struct i2c_client *client)
 
 static const struct dev_pm_ops imx258_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(imx258_suspend, imx258_resume)
+	SET_RUNTIME_PM_OPS(imx258_power_off, imx258_power_on, NULL)
 };
 
 #ifdef CONFIG_ACPI
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v6 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor
  2020-11-18 20:27 [PATCH v6 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor Krzysztof Kozlowski
                   ` (2 preceding siblings ...)
  2020-11-18 20:27 ` [PATCH v6 4/4] media: i2c: imx258: get clock from device properties and enable it via runtime PM Krzysztof Kozlowski
@ 2021-01-22  9:18 ` Sakari Ailus
  2021-01-27 12:23   ` Krzysztof Kozlowski
  2021-01-27 12:26   ` Krzysztof Kozlowski
  3 siblings, 2 replies; 9+ messages in thread
From: Sakari Ailus @ 2021-01-22  9:18 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Mauro Carvalho Chehab, Rob Herring, linux-media, devicetree,
	linux-arm-kernel, linux-kernel, Rob Herring

Hi Krysztof,

On Wed, Nov 18, 2020 at 09:27:12PM +0100, 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.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> 
> ---
> 
> Changes since v4:
> 1. Add clock-lanes,
> 2. Add Rob's review,
> 3. Add one more example and extend existing one,
> 4. Add common clock properties (assigned-*).
> 
> Changes since v3:
> 1. Document also two lane setup.
> 
> Changes since v2:
> 1. Remove clock-frequency, add reset GPIOs, add supplies.
> 2. Use additionalProperties.
> 
> Changes since v1:
> 1. None
> ---
>  .../devicetree/bindings/media/i2c/imx258.yaml | 140 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 141 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 000000000000..4a3471fb88a1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
> @@ -0,0 +1,140 @@
> +# 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 <krzk@kernel.org>
> +
> +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
> +
> +  assigned-clocks: true
> +  assigned-clock-parents: true
> +  assigned-clock-rates: true
> +
> +  clocks:
> +    description:
> +      Clock frequency from 6 to 27 MHz.
> +    maxItems: 1
> +
> +  reg:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    description: |-
> +      Reference to the GPIO connected to the XCLR pin, if any.
> +
> +  vana-supply:
> +    description:
> +      Analog voltage (VANA) supply, 2.7 V
> +
> +  vdig-supply:
> +    description:
> +      Digital I/O voltage (VDIG) supply, 1.2 V
> +
> +  vif-supply:
> +    description:
> +      Interface voltage (VIF) supply, 1.8 V
> +
> +  # See ../video-interfaces.txt for more details
> +  port:
> +    type: object
> +    properties:
> +      endpoint:
> +        type: object
> +        properties:
> +          clock-lanes:
> +            const: 0

This is redundant. Please remove, same for the examples. Can be a separate
patch, too.

With this change the set seems good to me.

-- 
Sakari Ailus

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v6 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor
  2021-01-22  9:18 ` [PATCH v6 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor Sakari Ailus
@ 2021-01-27 12:23   ` Krzysztof Kozlowski
  2021-01-27 12:26   ` Krzysztof Kozlowski
  1 sibling, 0 replies; 9+ messages in thread
From: Krzysztof Kozlowski @ 2021-01-27 12:23 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Mauro Carvalho Chehab, Rob Herring, linux-media, devicetree,
	linux-arm-kernel, linux-kernel, Rob Herring

On Fri, Jan 22, 2021 at 11:18:22AM +0200, Sakari Ailus wrote:
> Hi Krysztof,
> 
> On Wed, Nov 18, 2020 at 09:27:12PM +0100, 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.
> > 
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > 
> > ---
> > 
> > Changes since v4:
> > 1. Add clock-lanes,
> > 2. Add Rob's review,
> > 3. Add one more example and extend existing one,
> > 4. Add common clock properties (assigned-*).
> > 
> > Changes since v3:
> > 1. Document also two lane setup.
> > 
> > Changes since v2:
> > 1. Remove clock-frequency, add reset GPIOs, add supplies.
> > 2. Use additionalProperties.
> > 
> > Changes since v1:
> > 1. None
> > ---
> >  .../devicetree/bindings/media/i2c/imx258.yaml | 140 ++++++++++++++++++
> >  MAINTAINERS                                   |   1 +
> >  2 files changed, 141 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 000000000000..4a3471fb88a1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
> > @@ -0,0 +1,140 @@
> > +# 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 <krzk@kernel.org>
> > +
> > +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
> > +
> > +  assigned-clocks: true
> > +  assigned-clock-parents: true
> > +  assigned-clock-rates: true
> > +
> > +  clocks:
> > +    description:
> > +      Clock frequency from 6 to 27 MHz.
> > +    maxItems: 1
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  reset-gpios:
> > +    description: |-
> > +      Reference to the GPIO connected to the XCLR pin, if any.
> > +
> > +  vana-supply:
> > +    description:
> > +      Analog voltage (VANA) supply, 2.7 V
> > +
> > +  vdig-supply:
> > +    description:
> > +      Digital I/O voltage (VDIG) supply, 1.2 V
> > +
> > +  vif-supply:
> > +    description:
> > +      Interface voltage (VIF) supply, 1.8 V
> > +
> > +  # See ../video-interfaces.txt for more details
> > +  port:
> > +    type: object
> > +    properties:
> > +      endpoint:
> > +        type: object
> > +        properties:
> > +          clock-lanes:
> > +            const: 0
> 
> This is redundant. Please remove, same for the examples. Can be a separate
> patch, too.
> 
> With this change the set seems good to me.

OK, I'll remove it and send a v7.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v6 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor
  2021-01-22  9:18 ` [PATCH v6 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor Sakari Ailus
  2021-01-27 12:23   ` Krzysztof Kozlowski
@ 2021-01-27 12:26   ` Krzysztof Kozlowski
  2021-01-27 12:45     ` Sakari Ailus
  1 sibling, 1 reply; 9+ messages in thread
From: Krzysztof Kozlowski @ 2021-01-27 12:26 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Mauro Carvalho Chehab, Rob Herring, linux-media, devicetree,
	linux-arm-kernel, linux-kernel, Rob Herring

On Fri, Jan 22, 2021 at 11:18:22AM +0200, Sakari Ailus wrote:
> Hi Krysztof,
> 
> On Wed, Nov 18, 2020 at 09:27:12PM +0100, 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.
> > 
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > 
> > ---
> > 
> > Changes since v4:
> > 1. Add clock-lanes,
> > 2. Add Rob's review,
> > 3. Add one more example and extend existing one,
> > 4. Add common clock properties (assigned-*).
> > 
> > Changes since v3:
> > 1. Document also two lane setup.
> > 
> > Changes since v2:
> > 1. Remove clock-frequency, add reset GPIOs, add supplies.
> > 2. Use additionalProperties.
> > 
> > Changes since v1:
> > 1. None
> > ---
> >  .../devicetree/bindings/media/i2c/imx258.yaml | 140 ++++++++++++++++++
> >  MAINTAINERS                                   |   1 +
> >  2 files changed, 141 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 000000000000..4a3471fb88a1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
> > @@ -0,0 +1,140 @@
> > +# 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 <krzk@kernel.org>
> > +
> > +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
> > +
> > +  assigned-clocks: true
> > +  assigned-clock-parents: true
> > +  assigned-clock-rates: true
> > +
> > +  clocks:
> > +    description:
> > +      Clock frequency from 6 to 27 MHz.
> > +    maxItems: 1
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  reset-gpios:
> > +    description: |-
> > +      Reference to the GPIO connected to the XCLR pin, if any.
> > +
> > +  vana-supply:
> > +    description:
> > +      Analog voltage (VANA) supply, 2.7 V
> > +
> > +  vdig-supply:
> > +    description:
> > +      Digital I/O voltage (VDIG) supply, 1.2 V
> > +
> > +  vif-supply:
> > +    description:
> > +      Interface voltage (VIF) supply, 1.8 V
> > +
> > +  # See ../video-interfaces.txt for more details
> > +  port:
> > +    type: object
> > +    properties:
> > +      endpoint:
> > +        type: object
> > +        properties:
> > +          clock-lanes:
> > +            const: 0
> 
> This is redundant. Please remove, same for the examples. Can be a separate
> patch, too.

Before I remove too much - you mean "clock-lanes" is redundant or entire
set of properties in "port" node?

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v6 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor
  2021-01-27 12:26   ` Krzysztof Kozlowski
@ 2021-01-27 12:45     ` Sakari Ailus
  2021-01-27 12:51       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 9+ messages in thread
From: Sakari Ailus @ 2021-01-27 12:45 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Mauro Carvalho Chehab, Rob Herring, linux-media, devicetree,
	linux-arm-kernel, linux-kernel, Rob Herring

On Wed, Jan 27, 2021 at 01:26:12PM +0100, Krzysztof Kozlowski wrote:
> On Fri, Jan 22, 2021 at 11:18:22AM +0200, Sakari Ailus wrote:
> > Hi Krysztof,
> > 
> > On Wed, Nov 18, 2020 at 09:27:12PM +0100, 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.
> > > 
> > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > 
> > > ---
> > > 
> > > Changes since v4:
> > > 1. Add clock-lanes,
> > > 2. Add Rob's review,
> > > 3. Add one more example and extend existing one,
> > > 4. Add common clock properties (assigned-*).
> > > 
> > > Changes since v3:
> > > 1. Document also two lane setup.
> > > 
> > > Changes since v2:
> > > 1. Remove clock-frequency, add reset GPIOs, add supplies.
> > > 2. Use additionalProperties.
> > > 
> > > Changes since v1:
> > > 1. None
> > > ---
> > >  .../devicetree/bindings/media/i2c/imx258.yaml | 140 ++++++++++++++++++
> > >  MAINTAINERS                                   |   1 +
> > >  2 files changed, 141 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 000000000000..4a3471fb88a1
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
> > > @@ -0,0 +1,140 @@
> > > +# 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 <krzk@kernel.org>
> > > +
> > > +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
> > > +
> > > +  assigned-clocks: true
> > > +  assigned-clock-parents: true
> > > +  assigned-clock-rates: true
> > > +
> > > +  clocks:
> > > +    description:
> > > +      Clock frequency from 6 to 27 MHz.
> > > +    maxItems: 1
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  reset-gpios:
> > > +    description: |-
> > > +      Reference to the GPIO connected to the XCLR pin, if any.
> > > +
> > > +  vana-supply:
> > > +    description:
> > > +      Analog voltage (VANA) supply, 2.7 V
> > > +
> > > +  vdig-supply:
> > > +    description:
> > > +      Digital I/O voltage (VDIG) supply, 1.2 V
> > > +
> > > +  vif-supply:
> > > +    description:
> > > +      Interface voltage (VIF) supply, 1.8 V
> > > +
> > > +  # See ../video-interfaces.txt for more details
> > > +  port:
> > > +    type: object
> > > +    properties:
> > > +      endpoint:
> > > +        type: object
> > > +        properties:
> > > +          clock-lanes:
> > > +            const: 0
> > 
> > This is redundant. Please remove, same for the examples. Can be a separate
> > patch, too.
> 
> Before I remove too much - you mean "clock-lanes" is redundant or entire
> set of properties in "port" node?

Just clock-lanes. The rest are just as they should be IMO.

-- 
Sakari Ailus

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v6 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor
  2021-01-27 12:45     ` Sakari Ailus
@ 2021-01-27 12:51       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 9+ messages in thread
From: Krzysztof Kozlowski @ 2021-01-27 12:51 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Mauro Carvalho Chehab, Rob Herring, linux-media, devicetree,
	linux-arm-kernel, linux-kernel, Rob Herring

On Wed, Jan 27, 2021 at 02:45:53PM +0200, Sakari Ailus wrote:
> On Wed, Jan 27, 2021 at 01:26:12PM +0100, Krzysztof Kozlowski wrote:
> > On Fri, Jan 22, 2021 at 11:18:22AM +0200, Sakari Ailus wrote:
> > > Hi Krysztof,
> > > 
> > > On Wed, Nov 18, 2020 at 09:27:12PM +0100, 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.
> > > > 
> > > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > > 
> > > > ---
> > > > 
> > > > Changes since v4:
> > > > 1. Add clock-lanes,
> > > > 2. Add Rob's review,
> > > > 3. Add one more example and extend existing one,
> > > > 4. Add common clock properties (assigned-*).
> > > > 
> > > > Changes since v3:
> > > > 1. Document also two lane setup.
> > > > 
> > > > Changes since v2:
> > > > 1. Remove clock-frequency, add reset GPIOs, add supplies.
> > > > 2. Use additionalProperties.
> > > > 
> > > > Changes since v1:
> > > > 1. None
> > > > ---
> > > >  .../devicetree/bindings/media/i2c/imx258.yaml | 140 ++++++++++++++++++
> > > >  MAINTAINERS                                   |   1 +
> > > >  2 files changed, 141 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 000000000000..4a3471fb88a1
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
> > > > @@ -0,0 +1,140 @@
> > > > +# 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 <krzk@kernel.org>
> > > > +
> > > > +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
> > > > +
> > > > +  assigned-clocks: true
> > > > +  assigned-clock-parents: true
> > > > +  assigned-clock-rates: true
> > > > +
> > > > +  clocks:
> > > > +    description:
> > > > +      Clock frequency from 6 to 27 MHz.
> > > > +    maxItems: 1
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  reset-gpios:
> > > > +    description: |-
> > > > +      Reference to the GPIO connected to the XCLR pin, if any.
> > > > +
> > > > +  vana-supply:
> > > > +    description:
> > > > +      Analog voltage (VANA) supply, 2.7 V
> > > > +
> > > > +  vdig-supply:
> > > > +    description:
> > > > +      Digital I/O voltage (VDIG) supply, 1.2 V
> > > > +
> > > > +  vif-supply:
> > > > +    description:
> > > > +      Interface voltage (VIF) supply, 1.8 V
> > > > +
> > > > +  # See ../video-interfaces.txt for more details
> > > > +  port:
> > > > +    type: object
> > > > +    properties:
> > > > +      endpoint:
> > > > +        type: object
> > > > +        properties:
> > > > +          clock-lanes:
> > > > +            const: 0
> > > 
> > > This is redundant. Please remove, same for the examples. Can be a separate
> > > patch, too.
> > 
> > Before I remove too much - you mean "clock-lanes" is redundant or entire
> > set of properties in "port" node?
> 
> Just clock-lanes. The rest are just as they should be IMO.

Yes, makes sense. Thanks for confirmation.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-01-27 12:54 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-18 20:27 [PATCH v6 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor Krzysztof Kozlowski
2020-11-18 20:27 ` [PATCH v6 2/4] media: i2c: imx258: add support for binding via device tree Krzysztof Kozlowski
2020-11-18 20:27 ` [PATCH v6 3/4] media: i2c: imx258: simplify getting state container Krzysztof Kozlowski
2020-11-18 20:27 ` [PATCH v6 4/4] media: i2c: imx258: get clock from device properties and enable it via runtime PM Krzysztof Kozlowski
2021-01-22  9:18 ` [PATCH v6 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor Sakari Ailus
2021-01-27 12:23   ` Krzysztof Kozlowski
2021-01-27 12:26   ` Krzysztof Kozlowski
2021-01-27 12:45     ` Sakari Ailus
2021-01-27 12:51       ` Krzysztof Kozlowski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).