[PATCH v5 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor

[PATCH v5 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor

[Krzysztof Kozlowski]

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 5b9621ca2b31..68f30a283a2c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16262,6 +16262,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

[PATCH v5 2/4] media: i2c: imx258: add support for binding via device tree

[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 ccb55fd1d506..ed79bfb4c991 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

[PATCH v5 3/4] media: i2c: imx258: simplify getting state container

[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 ed79bfb4c991..ae183b0dbba9 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

[PATCH v5 4/4] media: i2c: imx258: get clock from device properties and enable it via runtime PM

[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 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 | 73 +++++++++++++++++++++++++++++++++-----
 1 file changed, 64 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index ae183b0dbba9..038115471f17 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,28 @@ 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)
-		return -EINVAL;
+	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");
+	} else {
+		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 +1259,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 +1307,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 +1323,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 +1332,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

Re: [PATCH v5 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor

[Sakari Ailus]

Hi Krzysztof,

On Mon, Oct 19, 2020 at 07:02:44PM +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.
> 
> 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-*).

Using the assigned-* clock properties may be workable for this driver at
the moment. But using these properties does not guarantee the external
clock frequency intended to be used on the hardware. Using other
frequencies *is not* expected to work. That applies to this driver as well.

This, instead of the clock-frequency property, effectively removes the
ability to set the correct frequency from the driver, at least with current
set of the used APIs.

I suppose you could add a function to set the assigned clock frequency and
keep it, just as clk_set_rate_exclusive does?

Cc the common clock framework list + maintainers.

-- 
Regards,

Sakari Ailus

Re: [PATCH v5 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor

[Krzysztof Kozlowski]

On Tue, 20 Oct 2020 at 12:38, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Hi Krzysztof,
>
> On Mon, Oct 19, 2020 at 07:02:44PM +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.
> >
> > 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-*).
>
> Using the assigned-* clock properties may be workable for this driver at
> the moment. But using these properties does not guarantee the external
> clock frequency intended to be used on the hardware.

It guarantees it. The clock frequency will be as expected (except if
someone misconfigures the DTS).

> Using other
> frequencies *is not* expected to work. That applies to this driver as well.

This is the binding which is HW description. According to HW datasheet
other frequencies from described range are accepted and expected to
work.

> This, instead of the clock-frequency property, effectively removes the
> ability to set the correct frequency from the driver, at least with current
> set of the used APIs.

It seems you confuse DT bindings with some specific driver
implementation. Bindings do not describe the driver behavior but the
HW. The ability to set the correct frequency from the driver is not
removed. It was never part of the bindings and never should. It is
part of the driver.

>
> I suppose you could add a function to set the assigned clock frequency and
> keep it, just as clk_set_rate_exclusive does?
>
> Cc the common clock framework list + maintainers.

The bindings have Rob review which is the DT maintainer. His
ack/review is needed for the bindings to be accepted. What more do you
need? Shall I point to submitting-bindings document?

I am really tired of discussing this. You raise some concerns about
driver behavior in the wrong context - in the patch for device tree
bindings. You use the arguments about the driver while we talk about
bindings. This is clearly not correct. I am all the time repeating
myself - the bindings describe the hardware, not the driver.

Best regards,
Krzysztof

Re: [PATCH v5 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor

[Sakari Ailus]

Hi Krzysztof,

On Tue, Oct 20, 2020 at 12:54:09PM +0200, Krzysztof Kozlowski wrote:
> On Tue, 20 Oct 2020 at 12:38, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Krzysztof,
> >
> > On Mon, Oct 19, 2020 at 07:02:44PM +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.
> > >
> > > 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-*).
> >
> > Using the assigned-* clock properties may be workable for this driver at
> > the moment. But using these properties does not guarantee the external
> > clock frequency intended to be used on the hardware.
> 
> It guarantees it. The clock frequency will be as expected (except if
> someone misconfigures the DTS).

Is that guaranteed?

I'm not saying no to the approach, but if we change how camera sensor DT
bindings are defined, I'd prefer an informed decision is made on the
matter.

> 
> > Using other
> > frequencies *is not* expected to work. That applies to this driver as well.
> 
> This is the binding which is HW description. According to HW datasheet
> other frequencies from described range are accepted and expected to
> work.

As per datasheet, yes, different external clock frequencies can be used.
But the link frequency is still not independent of the external clock
frequency.

The properties of the sensor's PLL tree determines what can be achieved
given a certain external clock frequency. So picking a wrong external clock
frequency quite possibly means that none of the designated link frequencies
are available, rendering the sensor inoperable.

> 
> > This, instead of the clock-frequency property, effectively removes the
> > ability to set the correct frequency from the driver, at least with current
> > set of the used APIs.
> 
> It seems you confuse DT bindings with some specific driver
> implementation. Bindings do not describe the driver behavior but the
> HW. The ability to set the correct frequency from the driver is not
> removed. It was never part of the bindings and never should. It is
> part of the driver.
> 
> >
> > I suppose you could add a function to set the assigned clock frequency and
> > keep it, just as clk_set_rate_exclusive does?
> >
> > Cc the common clock framework list + maintainers.
> 
> The bindings have Rob review which is the DT maintainer. His
> ack/review is needed for the bindings to be accepted. What more do you
> need? Shall I point to submitting-bindings document?
> 
> I am really tired of discussing this. You raise some concerns about
> driver behavior in the wrong context - in the patch for device tree
> bindings. You use the arguments about the driver while we talk about
> bindings. This is clearly not correct. I am all the time repeating
> myself - the bindings describe the hardware, not the driver.

My concerns are not related to the current driver implementation nor the
driver patches in this set.

-- 
Regards,

Sakari Ailus

Re: [PATCH v5 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor

[Krzysztof Kozlowski]

On Tue, Oct 20, 2020 at 03:00:58PM +0300, Sakari Ailus wrote:
> Hi Krzysztof,
> 
> On Tue, Oct 20, 2020 at 12:54:09PM +0200, Krzysztof Kozlowski wrote:
> > On Tue, 20 Oct 2020 at 12:38, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Krzysztof,
> > >
> > > On Mon, Oct 19, 2020 at 07:02:44PM +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.
> > > >
> > > > 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-*).
> > >
> > > Using the assigned-* clock properties may be workable for this driver at
> > > the moment. But using these properties does not guarantee the external
> > > clock frequency intended to be used on the hardware.
> > 
> > It guarantees it. The clock frequency will be as expected (except if
> > someone misconfigures the DTS).
> 
> Is that guaranteed?
> 
> I'm not saying no to the approach, but if we change how camera sensor DT
> bindings are defined, I'd prefer an informed decision is made on the
> matter.
> 
> > 
> > > Using other
> > > frequencies *is not* expected to work. That applies to this driver as well.
> > 
> > This is the binding which is HW description. According to HW datasheet
> > other frequencies from described range are accepted and expected to
> > work.
> 
> As per datasheet, yes, different external clock frequencies can be used.
> But the link frequency is still not independent of the external clock
> frequency.
> 
> The properties of the sensor's PLL tree determines what can be achieved
> given a certain external clock frequency. So picking a wrong external clock
> frequency quite possibly means that none of the designated link frequencies
> are available, rendering the sensor inoperable.

The driver then controls the HW and knows exactly what is needed. If
link frequency (which has its own DT property) requires some clock
frequency, the driver will configure the clock to that value. The same
going other direction. Driver has the knowledge about both its input
clock and link frequency, therefore it can make the best decision.

If someone configures DT or the driver to wrong frequencies (making the
link frequencies not available), the solution is not to add more
properties. It would be a band aid. The solution is to configure it
correctly.

> 
> > 
> > > This, instead of the clock-frequency property, effectively removes the
> > > ability to set the correct frequency from the driver, at least with current
> > > set of the used APIs.
> > 
> > It seems you confuse DT bindings with some specific driver
> > implementation. Bindings do not describe the driver behavior but the
> > HW. The ability to set the correct frequency from the driver is not
> > removed. It was never part of the bindings and never should. It is
> > part of the driver.
> > 
> > >
> > > I suppose you could add a function to set the assigned clock frequency and
> > > keep it, just as clk_set_rate_exclusive does?

I did not reply to this comment, so let me know. Of course, one could
add such functions. It's not a job for DT bindings, though.

> > >
> > > Cc the common clock framework list + maintainers.
> > 
> > The bindings have Rob review which is the DT maintainer. His
> > ack/review is needed for the bindings to be accepted. What more do you
> > need? Shall I point to submitting-bindings document?
> > 
> > I am really tired of discussing this. You raise some concerns about
> > driver behavior in the wrong context - in the patch for device tree
> > bindings. You use the arguments about the driver while we talk about
> > bindings. This is clearly not correct. I am all the time repeating
> > myself - the bindings describe the hardware, not the driver.
> 
> My concerns are not related to the current driver implementation nor the
> driver patches in this set.

Yet you refer to driver related tasks (configuring provided clock) while
discussing the bindings.

Best regards,
Krzysztof

Re: [PATCH v5 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor

[Sakari Ailus]

On Tue, Oct 20, 2020 at 02:26:21PM +0200, Krzysztof Kozlowski wrote:
> On Tue, Oct 20, 2020 at 03:00:58PM +0300, Sakari Ailus wrote:
> > Hi Krzysztof,
> > 
> > On Tue, Oct 20, 2020 at 12:54:09PM +0200, Krzysztof Kozlowski wrote:
> > > On Tue, 20 Oct 2020 at 12:38, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Hi Krzysztof,
> > > >
> > > > On Mon, Oct 19, 2020 at 07:02:44PM +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.
> > > > >
> > > > > 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-*).
> > > >
> > > > Using the assigned-* clock properties may be workable for this driver at
> > > > the moment. But using these properties does not guarantee the external
> > > > clock frequency intended to be used on the hardware.
> > > 
> > > It guarantees it. The clock frequency will be as expected (except if
> > > someone misconfigures the DTS).
> > 
> > Is that guaranteed?
> > 
> > I'm not saying no to the approach, but if we change how camera sensor DT
> > bindings are defined, I'd prefer an informed decision is made on the
> > matter.
> > 
> > > 
> > > > Using other
> > > > frequencies *is not* expected to work. That applies to this driver as well.
> > > 
> > > This is the binding which is HW description. According to HW datasheet
> > > other frequencies from described range are accepted and expected to
> > > work.
> > 
> > As per datasheet, yes, different external clock frequencies can be used.
> > But the link frequency is still not independent of the external clock
> > frequency.
> > 
> > The properties of the sensor's PLL tree determines what can be achieved
> > given a certain external clock frequency. So picking a wrong external clock
> > frequency quite possibly means that none of the designated link frequencies
> > are available, rendering the sensor inoperable.
> 
> The driver then controls the HW and knows exactly what is needed. If
> link frequency (which has its own DT property) requires some clock
> frequency, the driver will configure the clock to that value. The same

Well it doesn't if it doesn't get that information from DT.

The frequency is usually a range, and looking at these bindings, it's from
6 MHz to 27 MHz. That'd be a lot of frequencies for a driver to try.

> going other direction. Driver has the knowledge about both its input
> clock and link frequency, therefore it can make the best decision.

Again you're assuming a particular driver implementation.

Typically only a few frequencies are really available on platforms, so a in
practice a driver would not be able to get any requested frequency. I
wouldn't start hard-coding every possible frequency to camera sensor
drivers.

> > > > This, instead of the clock-frequency property, effectively removes the
> > > > ability to set the correct frequency from the driver, at least with current
> > > > set of the used APIs.
> > > 
> > > It seems you confuse DT bindings with some specific driver
> > > implementation. Bindings do not describe the driver behavior but the
> > > HW. The ability to set the correct frequency from the driver is not
> > > removed. It was never part of the bindings and never should. It is
> > > part of the driver.
> > > 
> > > >
> > > > I suppose you could add a function to set the assigned clock frequency and
> > > > keep it, just as clk_set_rate_exclusive does?
> 
> I did not reply to this comment, so let me know. Of course, one could
> add such functions. It's not a job for DT bindings, though.

I'm not suggesting to add it to DT binding patch. What I'm saying that with
this approach is looks like it may well be needed.

-- 
Regards,

Sakari Ailus

Re: [PATCH v5 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor

[Krzysztof Kozlowski]

On Tue, Oct 20, 2020 at 03:46:54PM +0300, Sakari Ailus wrote:
> On Tue, Oct 20, 2020 at 02:26:21PM +0200, Krzysztof Kozlowski wrote:
> > On Tue, Oct 20, 2020 at 03:00:58PM +0300, Sakari Ailus wrote:
> > > Hi Krzysztof,
> > > 
> > > On Tue, Oct 20, 2020 at 12:54:09PM +0200, Krzysztof Kozlowski wrote:
> > > > On Tue, 20 Oct 2020 at 12:38, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > Hi Krzysztof,
> > > > >
> > > > > On Mon, Oct 19, 2020 at 07:02:44PM +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.
> > > > > >
> > > > > > 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-*).
> > > > >
> > > > > Using the assigned-* clock properties may be workable for this driver at
> > > > > the moment. But using these properties does not guarantee the external
> > > > > clock frequency intended to be used on the hardware.
> > > > 
> > > > It guarantees it. The clock frequency will be as expected (except if
> > > > someone misconfigures the DTS).
> > > 
> > > Is that guaranteed?
> > > 
> > > I'm not saying no to the approach, but if we change how camera sensor DT
> > > bindings are defined, I'd prefer an informed decision is made on the
> > > matter.
> > > 
> > > > 
> > > > > Using other
> > > > > frequencies *is not* expected to work. That applies to this driver as well.
> > > > 
> > > > This is the binding which is HW description. According to HW datasheet
> > > > other frequencies from described range are accepted and expected to
> > > > work.
> > > 
> > > As per datasheet, yes, different external clock frequencies can be used.
> > > But the link frequency is still not independent of the external clock
> > > frequency.
> > > 
> > > The properties of the sensor's PLL tree determines what can be achieved
> > > given a certain external clock frequency. So picking a wrong external clock
> > > frequency quite possibly means that none of the designated link frequencies
> > > are available, rendering the sensor inoperable.
> > 
> > The driver then controls the HW and knows exactly what is needed. If
> > link frequency (which has its own DT property) requires some clock
> > frequency, the driver will configure the clock to that value. The same
> 
> Well it doesn't if it doesn't get that information from DT.

It will get it - via clk_get_rate(). You do not need DT for this.

> The frequency is usually a range, and looking at these bindings, it's from
> 6 MHz to 27 MHz. That'd be a lot of frequencies for a driver to try.

It does not have to try all of them. Assuming link frequency is fixed,
just use any matching (or hard-coded) input clock frequency. Since the
input clock frequency most likely will be set with assigned-clock-rates,
there will be no job to do for the driver at all. Unless the driver
wants to do something more, of course.

> 
> > going other direction. Driver has the knowledge about both its input
> > clock and link frequency, therefore it can make the best decision.
> 
> Again you're assuming a particular driver implementation.

Actually not, I am talking about bindings as far away from the driver
implementation as possible.  This is why some specific frequency *is
not* part of the bindings.

> 
> Typically only a few frequencies are really available on platforms, so a in
> practice a driver would not be able to get any requested frequency. I
> wouldn't start hard-coding every possible frequency to camera sensor
> drivers

If the driver cannot get requested frequency which it apparently
requires, there is nothing more to do. It's broken HW implementation.
The input clock must be matching requirements, regardless of what
property you put in DT.  You can add "clock-frequency" property, you can
even add "really-i-require-clock-frequency" but if the real HW input
clock does not have, it won't work.

IOW, adding "clock-frequency" property does not change the reality - the
board (HW) must provide given frequency so the entire system works.

> 
> > > > > This, instead of the clock-frequency property, effectively removes the
> > > > > ability to set the correct frequency from the driver, at least with current
> > > > > set of the used APIs.
> > > > 
> > > > It seems you confuse DT bindings with some specific driver
> > > > implementation. Bindings do not describe the driver behavior but the
> > > > HW. The ability to set the correct frequency from the driver is not
> > > > removed. It was never part of the bindings and never should. It is
> > > > part of the driver.
> > > > 
> > > > >
> > > > > I suppose you could add a function to set the assigned clock frequency and
> > > > > keep it, just as clk_set_rate_exclusive does?
> > 
> > I did not reply to this comment, so let me know. Of course, one could
> > add such functions. It's not a job for DT bindings, though.
> 
> I'm not suggesting to add it to DT binding patch. What I'm saying that with
> this approach is looks like it may well be needed.

New properties can always be added to DT. However existing properties
cannot be removed. Their meaning or values cannot be changed.

Best regards,
Krzysztof

Re: [PATCH v5 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor

[Krzysztof Kozlowski]

On Tue, Oct 20, 2020 at 02:58:52PM +0200, Krzysztof Kozlowski wrote:
> On Tue, Oct 20, 2020 at 03:46:54PM +0300, Sakari Ailus wrote:
> > On Tue, Oct 20, 2020 at 02:26:21PM +0200, Krzysztof Kozlowski wrote:
> > > On Tue, Oct 20, 2020 at 03:00:58PM +0300, Sakari Ailus wrote:
> > > > Hi Krzysztof,
> > > > 
> > > > On Tue, Oct 20, 2020 at 12:54:09PM +0200, Krzysztof Kozlowski wrote:
> > > > > On Tue, 20 Oct 2020 at 12:38, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > > > > >
> > > > > > Hi Krzysztof,
> > > > > >
> > > > > > On Mon, Oct 19, 2020 at 07:02:44PM +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.
> > > > > > >
> > > > > > > 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-*).
> > > > > >
> > > > > > Using the assigned-* clock properties may be workable for this driver at
> > > > > > the moment. But using these properties does not guarantee the external
> > > > > > clock frequency intended to be used on the hardware.
> > > > > 
> > > > > It guarantees it. The clock frequency will be as expected (except if
> > > > > someone misconfigures the DTS).
> > > > 
> > > > Is that guaranteed?
> > > > 
> > > > I'm not saying no to the approach, but if we change how camera sensor DT
> > > > bindings are defined, I'd prefer an informed decision is made on the
> > > > matter.
> > > > 
> > > > > 
> > > > > > Using other
> > > > > > frequencies *is not* expected to work. That applies to this driver as well.
> > > > > 
> > > > > This is the binding which is HW description. According to HW datasheet
> > > > > other frequencies from described range are accepted and expected to
> > > > > work.
> > > > 
> > > > As per datasheet, yes, different external clock frequencies can be used.
> > > > But the link frequency is still not independent of the external clock
> > > > frequency.
> > > > 
> > > > The properties of the sensor's PLL tree determines what can be achieved
> > > > given a certain external clock frequency. So picking a wrong external clock
> > > > frequency quite possibly means that none of the designated link frequencies
> > > > are available, rendering the sensor inoperable.
> > > 
> > > The driver then controls the HW and knows exactly what is needed. If
> > > link frequency (which has its own DT property) requires some clock
> > > frequency, the driver will configure the clock to that value. The same
> > 
> > Well it doesn't if it doesn't get that information from DT.
> 
> It will get it - via clk_get_rate(). You do not need DT for this.
> 
> > The frequency is usually a range, and looking at these bindings, it's from
> > 6 MHz to 27 MHz. That'd be a lot of frequencies for a driver to try.
> 
> It does not have to try all of them. Assuming link frequency is fixed,
> just use any matching (or hard-coded) input clock frequency. Since the
> input clock frequency most likely will be set with assigned-clock-rates,
> there will be no job to do for the driver at all. Unless the driver
> wants to do something more, of course.
> 
> > 
> > > going other direction. Driver has the knowledge about both its input
> > > clock and link frequency, therefore it can make the best decision.
> > 
> > Again you're assuming a particular driver implementation.
> 
> Actually not, I am talking about bindings as far away from the driver
> implementation as possible.  This is why some specific frequency *is
> not* part of the bindings.
> 
> > 
> > Typically only a few frequencies are really available on platforms, so a in
> > practice a driver would not be able to get any requested frequency. I
> > wouldn't start hard-coding every possible frequency to camera sensor
> > drivers
> 
> If the driver cannot get requested frequency which it apparently
> requires, there is nothing more to do. It's broken HW implementation.
> The input clock must be matching requirements, regardless of what
> property you put in DT.  You can add "clock-frequency" property, you can
> even add "really-i-require-clock-frequency" but if the real HW input
> clock does not have, it won't work.
> 
> IOW, adding "clock-frequency" property does not change the reality - the
> board (HW) must provide given frequency so the entire system works.
> 
> > 
> > > > > > This, instead of the clock-frequency property, effectively removes the
> > > > > > ability to set the correct frequency from the driver, at least with current
> > > > > > set of the used APIs.
> > > > > 
> > > > > It seems you confuse DT bindings with some specific driver
> > > > > implementation. Bindings do not describe the driver behavior but the
> > > > > HW. The ability to set the correct frequency from the driver is not
> > > > > removed. It was never part of the bindings and never should. It is
> > > > > part of the driver.
> > > > > 
> > > > > >
> > > > > > I suppose you could add a function to set the assigned clock frequency and
> > > > > > keep it, just as clk_set_rate_exclusive does?
> > > 
> > > I did not reply to this comment, so let me know. Of course, one could
> > > add such functions. It's not a job for DT bindings, though.
> > 
> > I'm not suggesting to add it to DT binding patch. What I'm saying that with
> > this approach is looks like it may well be needed.
> 
> New properties can always be added to DT. However existing properties
> cannot be removed. Their meaning or values cannot be changed.

Any more comments on the bindings or the patchset?

Best regards,
Krzysztof

Re: [PATCH v5 1/4] dt-bindings: media: imx258: add bindings for IMX258 sensor

[Sakari Ailus]

Hi Krysztof,

On Mon, Oct 19, 2020 at 07:02:44PM +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.
> 
> 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

I discussed the matter of using assigned clocks with Rob some time ago and
the conclusion of that was that the sensor driver could use the default
frequency (set using assigned-clock-rates) instead of the explicit
frequency in DT. There are use cases (sharing the clock signal between two
sensors, but different frequencies) that would be affected by this but I
don't think we have any in mainline so I guess this approach works for now
without additional changes. If someone needs those use cases, it's likely
DT clock binding semantings and clock framework changes will be needed.
That'll be another discussion if it ever happens.

-- 
Regards,

Sakari Ailus

Re: [PATCH v5 4/4] media: i2c: imx258: get clock from device properties and enable it via runtime PM

[Sakari Ailus]

Hi Krysztof,

On Mon, Oct 19, 2020 at 07:26:17PM +0200, Krzysztof Kozlowski wrote:
> 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 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 | 73 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 64 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index ae183b0dbba9..038115471f17 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,28 @@ 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)
> -		return -EINVAL;
> +	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");
> +	} else {
> +		if (clk_get_rate(imx258->clk) != IMX258_INPUT_CLOCK_FREQ) {

Please move the check outside the conditional block. May be a separate
patch if you like.

> +			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 +1259,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 +1307,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 +1323,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 +1332,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

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v5 4/4] media: i2c: imx258: get clock from device properties and enable it via runtime PM

[Krzysztof Kozlowski]

On Mon, Nov 02, 2020 at 05:08:47PM +0200, Sakari Ailus wrote:
> Hi Krysztof,
> 
> On Mon, Oct 19, 2020 at 07:26:17PM +0200, Krzysztof Kozlowski wrote:
> > 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 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 | 73 +++++++++++++++++++++++++++++++++-----
> >  1 file changed, 64 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > index ae183b0dbba9..038115471f17 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,28 @@ 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)
> > -		return -EINVAL;
> > +	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");
> > +	} else {
> > +		if (clk_get_rate(imx258->clk) != IMX258_INPUT_CLOCK_FREQ) {
> 
> Please move the check outside the conditional block. May be a separate
> patch if you like.

OK

Best regards,
Krzysztof