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

[PATCH v4 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>

---

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 | 100 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 101 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..520e75c7b451
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
@@ -0,0 +1,100 @@
+# 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
+
+  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:
+          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:
+          - 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>;
+                    data-lanes = <1 2 3 4>;
+                    link-frequencies = /bits/ 64 <320000000>;
+                };
+            };
+        };
+    };
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.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 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.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 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.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 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 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 | 71 +++++++++++++++++++++++++++++++++-----
 1 file changed, 62 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index ae183b0dbba9..7bedbfe5c4d6 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;
 }
 
@@ -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.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rob Herring]

On Wed, 23 Sep 2020 17:21:26 +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>
> 
> ---
> 
> 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 | 100 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 101 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/imx258.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sakari Ailus]

Hi Krzysztof,

On Wed, Sep 23, 2020 at 05:21:26PM +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>
> 
> ---
> 
> Changes since v3:
> 1. Document also two lane setup.
> 
> Changes since v2:
> 1. Remove clock-frequency, add reset GPIOs, add supplies.

Oops. I missed this one.

How does the driver know the appropriate clock frequency for the platform
if it's not in DT? The sensor supports a range of frequencies, not a single
frequency.

Could you add clock-frequency back?

-- 
Kind regards,

Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sakari Ailus]

Hi Krzysztof,

On Wed, Sep 23, 2020 at 05:21:29PM +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 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 | 71 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 62 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index ae183b0dbba9..7bedbfe5c4d6 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);

You'll need this in remove callback, too.

> +
>  	return ret;
>  }
>  
> @@ -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

-- 
Kind regards,

Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Krzysztof Kozlowski]

On Tue, 29 Sep 2020 at 11:15, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Hi Krzysztof,
>
> On Wed, Sep 23, 2020 at 05:21:26PM +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>
> >
> > ---
> >
> > Changes since v3:
> > 1. Document also two lane setup.
> >
> > Changes since v2:
> > 1. Remove clock-frequency, add reset GPIOs, add supplies.
>
> Oops. I missed this one.
>
> How does the driver know the appropriate clock frequency for the platform
> if it's not in DT? The sensor supports a range of frequencies, not a single
> frequency.
>
> Could you add clock-frequency back?

Not really, it was removed on Rob's request. The bindings do not
describe driver's behavior so how the driver gets frequency should not
be part of the bindings. Also it's not a real problem - the driver
just calls clk_get_rate().

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Krzysztof Kozlowski]

On Tue, Sep 29, 2020 at 12:17:04PM +0300, Sakari Ailus wrote:
> Hi Krzysztof,
> 
> On Wed, Sep 23, 2020 at 05:21:29PM +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 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 | 71 +++++++++++++++++++++++++++++++++-----
> >  1 file changed, 62 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > index ae183b0dbba9..7bedbfe5c4d6 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);
> 
> You'll need this in remove callback, too.

True, there is no runtime idle call so this is missing.

Thanks.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sakari Ailus]

On Tue, Sep 29, 2020 at 11:18:46AM +0200, Krzysztof Kozlowski wrote:
> On Tue, 29 Sep 2020 at 11:15, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Krzysztof,
> >
> > On Wed, Sep 23, 2020 at 05:21:26PM +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>
> > >
> > > ---
> > >
> > > Changes since v3:
> > > 1. Document also two lane setup.
> > >
> > > Changes since v2:
> > > 1. Remove clock-frequency, add reset GPIOs, add supplies.
> >
> > Oops. I missed this one.
> >
> > How does the driver know the appropriate clock frequency for the platform
> > if it's not in DT? The sensor supports a range of frequencies, not a single
> > frequency.
> >
> > Could you add clock-frequency back?
> 
> Not really, it was removed on Rob's request. The bindings do not
> describe driver's behavior so how the driver gets frequency should not
> be part of the bindings. Also it's not a real problem - the driver
> just calls clk_get_rate().

How is the rate determined? I mean, many ISPs or CSI-2 receivers that
provide the clock are also capable of using a variety of frequencies. But
only one can be used on the platform in general.

Where does it come from if it's not in DT?

Using another frequency generally leads to failure later on as the desired
link frequency likely is not available for a random external clock
frequency.

-- 
Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sakari Ailus]

On Tue, Sep 29, 2020 at 11:18:46AM +0200, Krzysztof Kozlowski wrote:
> On Tue, 29 Sep 2020 at 11:15, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Krzysztof,
> >
> > On Wed, Sep 23, 2020 at 05:21:26PM +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>
> > >
> > > ---
> > >
> > > Changes since v3:
> > > 1. Document also two lane setup.
> > >
> > > Changes since v2:
> > > 1. Remove clock-frequency, add reset GPIOs, add supplies.
> >
> > Oops. I missed this one.
> >
> > How does the driver know the appropriate clock frequency for the platform
> > if it's not in DT? The sensor supports a range of frequencies, not a single
> > frequency.
> >
> > Could you add clock-frequency back?
> 
> Not really, it was removed on Rob's request. The bindings do not
> describe driver's behavior so how the driver gets frequency should not
> be part of the bindings. Also it's not a real problem - the driver
> just calls clk_get_rate().

Btw. we also have this nowadays:
Documentation/driver-api/media/camera-sensor.rst .

-- 
Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Krzysztof Kozlowski]

On Tue, Sep 29, 2020 at 12:40:46PM +0300, Sakari Ailus wrote:
> On Tue, Sep 29, 2020 at 11:18:46AM +0200, Krzysztof Kozlowski wrote:
> > On Tue, 29 Sep 2020 at 11:15, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Krzysztof,
> > >
> > > On Wed, Sep 23, 2020 at 05:21:26PM +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>
> > > >
> > > > ---
> > > >
> > > > Changes since v3:
> > > > 1. Document also two lane setup.
> > > >
> > > > Changes since v2:
> > > > 1. Remove clock-frequency, add reset GPIOs, add supplies.
> > >
> > > Oops. I missed this one.
> > >
> > > How does the driver know the appropriate clock frequency for the platform
> > > if it's not in DT? The sensor supports a range of frequencies, not a single
> > > frequency.
> > >
> > > Could you add clock-frequency back?
> > 
> > Not really, it was removed on Rob's request. The bindings do not
> > describe driver's behavior so how the driver gets frequency should not
> > be part of the bindings. Also it's not a real problem - the driver
> > just calls clk_get_rate().
> 
> How is the rate determined? I mean, many ISPs or CSI-2 receivers that
> provide the clock are also capable of using a variety of frequencies. But
> only one can be used on the platform in general.

Having "clock-frequency" property in DTS did not solve that. It has no
effect on actual frequency.

> 
> Where does it come from if it's not in DT?

The frequency is either chosen by consumer (imx258) or pre-assigned from
DT, but not with "clock-frequency" property. There are generic
properties for this: assigned-clocks, assigned-clock-rates and
assigned-clock-parents.

These properties should be added to DTS if additionalProperties is
false, which is the case here... so I could add them. My DTS anyway does
not use them, as the clock is generated internally on a camera board so
I don't have a control over it.

Best regards,
Krzysztof


> 
> Using another frequency generally leads to failure later on as the desired
> link frequency likely is not available for a random external clock
> frequency.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sakari Ailus]

On Tue, Sep 29, 2020 at 11:46:36AM +0200, Krzysztof Kozlowski wrote:
> On Tue, Sep 29, 2020 at 12:40:46PM +0300, Sakari Ailus wrote:
> > On Tue, Sep 29, 2020 at 11:18:46AM +0200, Krzysztof Kozlowski wrote:
> > > On Tue, 29 Sep 2020 at 11:15, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Hi Krzysztof,
> > > >
> > > > On Wed, Sep 23, 2020 at 05:21:26PM +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>
> > > > >
> > > > > ---
> > > > >
> > > > > Changes since v3:
> > > > > 1. Document also two lane setup.
> > > > >
> > > > > Changes since v2:
> > > > > 1. Remove clock-frequency, add reset GPIOs, add supplies.
> > > >
> > > > Oops. I missed this one.
> > > >
> > > > How does the driver know the appropriate clock frequency for the platform
> > > > if it's not in DT? The sensor supports a range of frequencies, not a single
> > > > frequency.
> > > >
> > > > Could you add clock-frequency back?
> > > 
> > > Not really, it was removed on Rob's request. The bindings do not
> > > describe driver's behavior so how the driver gets frequency should not
> > > be part of the bindings. Also it's not a real problem - the driver
> > > just calls clk_get_rate().
> > 
> > How is the rate determined? I mean, many ISPs or CSI-2 receivers that
> > provide the clock are also capable of using a variety of frequencies. But
> > only one can be used on the platform in general.
> 
> Having "clock-frequency" property in DTS did not solve that. It has no
> effect on actual frequency.

It's up to the driver to do what's needed, yes.

Please see examples in e.g. drivers/media/i2c/ov8856.c and
Documentation/devicetree/bindings/media/i2c/ov8856.yaml .

-- 
Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Krzysztof Kozlowski]

On Tue, Sep 29, 2020 at 02:02:55PM +0300, Sakari Ailus wrote:
> On Tue, Sep 29, 2020 at 11:46:36AM +0200, Krzysztof Kozlowski wrote:
> > On Tue, Sep 29, 2020 at 12:40:46PM +0300, Sakari Ailus wrote:
> > > On Tue, Sep 29, 2020 at 11:18:46AM +0200, Krzysztof Kozlowski wrote:
> > > > On Tue, 29 Sep 2020 at 11:15, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > Hi Krzysztof,
> > > > >
> > > > > On Wed, Sep 23, 2020 at 05:21:26PM +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>
> > > > > >
> > > > > > ---
> > > > > >
> > > > > > Changes since v3:
> > > > > > 1. Document also two lane setup.
> > > > > >
> > > > > > Changes since v2:
> > > > > > 1. Remove clock-frequency, add reset GPIOs, add supplies.
> > > > >
> > > > > Oops. I missed this one.
> > > > >
> > > > > How does the driver know the appropriate clock frequency for the platform
> > > > > if it's not in DT? The sensor supports a range of frequencies, not a single
> > > > > frequency.
> > > > >
> > > > > Could you add clock-frequency back?
> > > > 
> > > > Not really, it was removed on Rob's request. The bindings do not
> > > > describe driver's behavior so how the driver gets frequency should not
> > > > be part of the bindings. Also it's not a real problem - the driver
> > > > just calls clk_get_rate().
> > > 
> > > How is the rate determined? I mean, many ISPs or CSI-2 receivers that
> > > provide the clock are also capable of using a variety of frequencies. But
> > > only one can be used on the platform in general.
> > 
> > Having "clock-frequency" property in DTS did not solve that. It has no
> > effect on actual frequency.
> 
> It's up to the driver to do what's needed, yes.
> 
> Please see examples in e.g. drivers/media/i2c/ov8856.c and
> Documentation/devicetree/bindings/media/i2c/ov8856.yaml .

It seems the ov8856 driver uses this property in different way than
imx258 driver. It is more appropriate and quite similar to clock
providers and buses - to set the desired frequency on input clock.

Therefore what is the point of using this DT property comparing to
assigned-clock-rates?

It's the same. So let's use generic (already documented)
assigned-clock-rates.  

For your question (not related to the bindings but to driver
implementation): "How is the rate determined?", simple: clk_get_rate.
The driver then uses it like this:
	if (clk_get_rate() != only_working_configuration_hz)
		return -EINVAL;

From the bindings point of view, the clock can be anything within a
range of sensor's accepted values. The clock frequency is a property of
a clock, not of a sensor. Therefore for HW description it should be
described in the clock bindings, not in the sensor bindings.

To summarize, the "clock-frequency" property of sensor node:
1. As a way to configure the clock should be replaced with
   assigned-clock properties,
2. As a way to describe the hardware is simply invalid. It is not a HW
   description, because HW requires just a clock of frequency within
   given range, not a fixed frequency clock.

Consider the example:
        camera@1a {
                compatible = "sony,imx258";
                reg = <0x1a>;

                clocks = <&imx258_clk>;
                clock-names = "clk";

                /* Oscillator on camera board */
                imx258_clk: clk {
                        compatible = "fixed-clock";
                        #clock-cells = <0>;
                        clock-frequency = <19200000>;
                };

                port {
			...
                };
        };

What is the point to add "clock-frequency" property to the camera
itself, since it is already clearly defined by the clock?

Or another example:

        camera@1a {
                compatible = "sony,imx258";
                reg = <0x1a>;

                clocks = <&iclk 0>;
                clock-names = "clk";
		assigned-clocks = <&clk 0>;
		assigned-clock-rates = <19200000>;

                port {
			...
                };
        };

Again, no reason for artificial clock-frequency property. It is not part
of HW description of the sensor.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel