linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor
@ 2020-09-02  7:18 Krzysztof Kozlowski
  2020-09-02  7:18 ` [PATCH v2 2/3] media: imx258: Add support for binding via device tree Krzysztof Kozlowski
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Krzysztof Kozlowski @ 2020-09-02  7:18 UTC (permalink / raw)
  To: Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Krzysztof Kozlowski, linux-media, devicetree,
	linux-arm-kernel, linux-kernel

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 v1:
1. None
---
 .../devicetree/bindings/media/i2c/imx258.yaml      | 92 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 93 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/imx258.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/imx258.yaml b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
new file mode 100644
index 000000000000..ef789ad31143
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
@@ -0,0 +1,92 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/imx258.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sony IMX258 13 Mpixel CMOS Digital Image Sensor
+
+maintainers:
+  - Krzysztof Kozlowski <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:
+    maxItems: 1
+
+  clock-frequency:
+    description: Frequency of input clock if clock is not provided
+    deprecated: true
+    const: 19200000
+
+  reg:
+    maxItems: 1
+
+  # See ../video-interfaces.txt for more details
+  port:
+    type: object
+    properties:
+      endpoint:
+        type: object
+        properties:
+          data-lanes:
+            items:
+              - const: 1
+              - const: 2
+              - const: 3
+              - const: 4
+
+          link-frequencies:
+            allOf:
+              - $ref: /schemas/types.yaml#/definitions/uint64-array
+            description:
+              Allowed data bus frequencies.
+
+        required:
+          - data-lanes
+          - link-frequencies
+
+required:
+  - compatible
+  - reg
+  - port
+
+
+if:
+  not:
+    required:
+      - clocks
+then:
+  required:
+    - clock-frequency
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        sensor@6c {
+            compatible = "sony,imx258";
+            reg = <0x6c>;
+            clocks = <&imx258_clk>;
+
+            port {
+                imx258_ep: endpoint {
+                    remote-endpoint = <&csi1_ep>;
+                    data-lanes = <1 2 3 4>;
+                    link-frequencies = /bits/ 64 <320000000>;
+                };
+            };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index dc2aa691b75a..cca13d483d22 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16130,6 +16130,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.7.4


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

* [PATCH v2 2/3] media: imx258: Add support for binding via device tree
  2020-09-02  7:18 [PATCH v2 1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor Krzysztof Kozlowski
@ 2020-09-02  7:18 ` Krzysztof Kozlowski
  2020-09-02  7:18 ` [PATCH v2 3/3] media: imx258: Get clock from device properties and enable it via runtime PM Krzysztof Kozlowski
  2020-09-14 20:13 ` [PATCH v2 1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor Rob Herring
  2 siblings, 0 replies; 9+ messages in thread
From: Krzysztof Kozlowski @ 2020-09-02  7:18 UTC (permalink / raw)
  To: Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Krzysztof Kozlowski, linux-media, devicetree,
	linux-arm-kernel, linux-kernel

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 f86ae18bc104..c20bac9b00ec 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.7.4


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

* [PATCH v2 3/3] media: imx258: Get clock from device properties and enable it via runtime PM
  2020-09-02  7:18 [PATCH v2 1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor Krzysztof Kozlowski
  2020-09-02  7:18 ` [PATCH v2 2/3] media: imx258: Add support for binding via device tree Krzysztof Kozlowski
@ 2020-09-02  7:18 ` Krzysztof Kozlowski
  2020-09-02  7:39   ` Sakari Ailus
  2020-09-14 20:13 ` [PATCH v2 1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor Rob Herring
  2 siblings, 1 reply; 9+ messages in thread
From: Krzysztof Kozlowski @ 2020-09-02  7:18 UTC (permalink / raw)
  To: Sakari Ailus, Mauro Carvalho Chehab, Rob Herring, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Krzysztof Kozlowski, linux-media, devicetree,
	linux-arm-kernel, linux-kernel

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 v1:
1. Use runtime PM for clock toggling
---
 drivers/media/i2c/imx258.c | 68 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 59 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index c20bac9b00ec..ee38dafb8450 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,27 @@ static int imx258_stop_streaming(struct imx258 *imx258)
 	return 0;
 }
 
+static int imx258_power_on(struct device *dev)
+{
+	struct imx258 *imx258 = dev_get_drvdata(dev);
+	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 imx258 *imx258 = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(imx258->clk);
+
+	return 0;
+}
+
 static int imx258_set_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct imx258 *imx258 = to_imx258(sd);
@@ -1201,9 +1228,27 @@ 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;
+
+	dev_set_drvdata(&client->dev, imx258);
+
+	imx258->clk = devm_clk_get_optional(&client->dev, NULL);
+	if (!imx258->clk) {
+		dev_info(&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
@@ -1213,24 +1258,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;
@@ -1260,6 +1306,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;
 }
 
@@ -1280,6 +1329,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.7.4


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

* Re: [PATCH v2 3/3] media: imx258: Get clock from device properties and enable it via runtime PM
  2020-09-02  7:18 ` [PATCH v2 3/3] media: imx258: Get clock from device properties and enable it via runtime PM Krzysztof Kozlowski
@ 2020-09-02  7:39   ` Sakari Ailus
  2020-09-21 12:02     ` Krzysztof Kozlowski
  0 siblings, 1 reply; 9+ messages in thread
From: Sakari Ailus @ 2020-09-02  7:39 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Mauro Carvalho Chehab, Rob Herring, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	linux-media, devicetree, linux-arm-kernel, linux-kernel

Hi Krzysztof,

Thanks for the update.

On Wed, Sep 02, 2020 at 09:18:10AM +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 v1:
> 1. Use runtime PM for clock toggling
> ---
>  drivers/media/i2c/imx258.c | 68 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 59 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index c20bac9b00ec..ee38dafb8450 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,27 @@ static int imx258_stop_streaming(struct imx258 *imx258)
>  	return 0;
>  }
>  
> +static int imx258_power_on(struct device *dev)
> +{
> +	struct imx258 *imx258 = dev_get_drvdata(dev);
> +	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 imx258 *imx258 = dev_get_drvdata(dev);
> +
> +	clk_disable_unprepare(imx258->clk);
> +
> +	return 0;
> +}
> +
>  static int imx258_set_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct imx258 *imx258 = to_imx258(sd);
> @@ -1201,9 +1228,27 @@ 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;
> +
> +	dev_set_drvdata(&client->dev, imx258);

This you cannot do --- it'll be overwritten by v4l2_i2c_subdev_init().

> +
> +	imx258->clk = devm_clk_get_optional(&client->dev, NULL);
> +	if (!imx258->clk) {

You can move declaration of val here (I think).

> +		dev_info(&client->dev, "no clock provided, using clock-frequency property\n");

As this is showing up on all ACPI based systems, I guess dev_dbg() would be
more appropriate.

Please also wrap lines over 80 if they reasonably can be.

> +
> +		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
> @@ -1213,24 +1258,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;
> @@ -1260,6 +1306,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;
>  }
>  
> @@ -1280,6 +1329,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

-- 
Sakari Ailus

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

* Re: [PATCH v2 1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor
  2020-09-02  7:18 [PATCH v2 1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor Krzysztof Kozlowski
  2020-09-02  7:18 ` [PATCH v2 2/3] media: imx258: Add support for binding via device tree Krzysztof Kozlowski
  2020-09-02  7:18 ` [PATCH v2 3/3] media: imx258: Get clock from device properties and enable it via runtime PM Krzysztof Kozlowski
@ 2020-09-14 20:13 ` Rob Herring
  2020-09-21 11:26   ` Krzysztof Kozlowski
  2 siblings, 1 reply; 9+ messages in thread
From: Rob Herring @ 2020-09-14 20:13 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Sakari Ailus, Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	linux-media, devicetree, linux-arm-kernel, linux-kernel

On Wed, Sep 02, 2020 at 09:18:08AM +0200, Krzysztof Kozlowski wrote:
> Add bindings for the IMX258 camera sensor.  The bindings, just like the
> driver, are quite limited, e.g. do not support regulator supplies.

Bindings should be complete, not what a driver happens to currently 
support.

> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> ---
> 
> Changes since v1:
> 1. None
> ---
>  .../devicetree/bindings/media/i2c/imx258.yaml      | 92 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 93 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/imx258.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/imx258.yaml b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
> new file mode 100644
> index 000000000000..ef789ad31143
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
> @@ -0,0 +1,92 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/imx258.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sony IMX258 13 Mpixel CMOS Digital Image Sensor
> +
> +maintainers:
> +  - Krzysztof Kozlowski <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:
> +    maxItems: 1
> +
> +  clock-frequency:
> +    description: Frequency of input clock if clock is not provided
> +    deprecated: true

Why are we adding something deprecated on a new binding?

> +    const: 19200000
> +
> +  reg:
> +    maxItems: 1
> +
> +  # See ../video-interfaces.txt for more details
> +  port:
> +    type: object
> +    properties:
> +      endpoint:
> +        type: object
> +        properties:
> +          data-lanes:
> +            items:
> +              - const: 1
> +              - const: 2
> +              - const: 3
> +              - const: 4

If this is the only config, why does it need to be in DT?

> +
> +          link-frequencies:
> +            allOf:
> +              - $ref: /schemas/types.yaml#/definitions/uint64-array
> +            description:
> +              Allowed data bus frequencies.
> +
> +        required:
> +          - data-lanes
> +          - link-frequencies
> +
> +required:
> +  - compatible
> +  - reg
> +  - port
> +
> +
> +if:
> +  not:
> +    required:
> +      - clocks
> +then:
> +  required:
> +    - clock-frequency
> +
> +unevaluatedProperties: false

additionalProperties

> +
> +examples:
> +  - |
> +    i2c0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        sensor@6c {
> +            compatible = "sony,imx258";
> +            reg = <0x6c>;
> +            clocks = <&imx258_clk>;
> +
> +            port {
> +                imx258_ep: endpoint {
> +                    remote-endpoint = <&csi1_ep>;
> +                    data-lanes = <1 2 3 4>;
> +                    link-frequencies = /bits/ 64 <320000000>;
> +                };
> +            };
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dc2aa691b75a..cca13d483d22 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16130,6 +16130,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.7.4
> 

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

* Re: [PATCH v2 1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor
  2020-09-14 20:13 ` [PATCH v2 1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor Rob Herring
@ 2020-09-21 11:26   ` Krzysztof Kozlowski
  2020-09-22 19:23     ` Rob Herring
  0 siblings, 1 reply; 9+ messages in thread
From: Krzysztof Kozlowski @ 2020-09-21 11:26 UTC (permalink / raw)
  To: Rob Herring
  Cc: Sakari Ailus, Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	linux-media, devicetree, linux-arm-kernel, linux-kernel

On Mon, Sep 14, 2020 at 02:13:10PM -0600, Rob Herring wrote:
> On Wed, Sep 02, 2020 at 09:18:08AM +0200, Krzysztof Kozlowski wrote:
> > Add bindings for the IMX258 camera sensor.  The bindings, just like the
> > driver, are quite limited, e.g. do not support regulator supplies.
> 
> Bindings should be complete, not what a driver happens to currently 
> support.

I'll add then more complete picture.

> 
> > 
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > 
> > ---
> > 
> > Changes since v1:
> > 1. None
> > ---
> >  .../devicetree/bindings/media/i2c/imx258.yaml      | 92 ++++++++++++++++++++++
> >  MAINTAINERS                                        |  1 +
> >  2 files changed, 93 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/imx258.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/imx258.yaml b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
> > new file mode 100644
> > index 000000000000..ef789ad31143
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
> > @@ -0,0 +1,92 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/imx258.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Sony IMX258 13 Mpixel CMOS Digital Image Sensor
> > +
> > +maintainers:
> > +  - Krzysztof Kozlowski <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:
> > +    maxItems: 1
> > +
> > +  clock-frequency:
> > +    description: Frequency of input clock if clock is not provided
> > +    deprecated: true
> 
> Why are we adding something deprecated on a new binding?

My intention was also to document it but indeed easier to skip it.

> 
> > +    const: 19200000
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  # See ../video-interfaces.txt for more details
> > +  port:
> > +    type: object
> > +    properties:
> > +      endpoint:
> > +        type: object
> > +        properties:
> > +          data-lanes:
> > +            items:
> > +              - const: 1
> > +              - const: 2
> > +              - const: 3
> > +              - const: 4
> 
> If this is the only config, why does it need to be in DT?

The sensor is capable of two settings: two lanes (1 and 2) and four
lanes described above.  However Linux driver requires the latter (four
lanes, 1+2+3+4).

If I were to describe the bindings for HW, someone would really be
confused and try to use two lanes setup, which won't work. Driver won't
allow it.

I understand that bindings document the HW and describe its interface
but do we really want to put "theoretical" bindings which cannot be used
in practice with Linux kernel?

If yes, how to nicely document this that only one setting is currently
working?

> 
> > +
> > +          link-frequencies:
> > +            allOf:
> > +              - $ref: /schemas/types.yaml#/definitions/uint64-array
> > +            description:
> > +              Allowed data bus frequencies.
> > +
> > +        required:
> > +          - data-lanes
> > +          - link-frequencies
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - port
> > +
> > +
> > +if:
> > +  not:
> > +    required:
> > +      - clocks
> > +then:
> > +  required:
> > +    - clock-frequency
> > +
> > +unevaluatedProperties: false
> 
> additionalProperties

Right.

Best regards,
Krzysztof

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

* Re: [PATCH v2 3/3] media: imx258: Get clock from device properties and enable it via runtime PM
  2020-09-02  7:39   ` Sakari Ailus
@ 2020-09-21 12:02     ` Krzysztof Kozlowski
  0 siblings, 0 replies; 9+ messages in thread
From: Krzysztof Kozlowski @ 2020-09-21 12:02 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Mauro Carvalho Chehab, Rob Herring, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	linux-media, devicetree, linux-arm-kernel, linux-kernel

On Wed, Sep 02, 2020 at 10:39:35AM +0300, Sakari Ailus wrote:
> Hi Krzysztof,
> 
> Thanks for the update.
> 
> On Wed, Sep 02, 2020 at 09:18:10AM +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 v1:
> > 1. Use runtime PM for clock toggling
> > ---
> >  drivers/media/i2c/imx258.c | 68 ++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 59 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > index c20bac9b00ec..ee38dafb8450 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,27 @@ static int imx258_stop_streaming(struct imx258 *imx258)
> >  	return 0;
> >  }
> >  
> > +static int imx258_power_on(struct device *dev)
> > +{
> > +	struct imx258 *imx258 = dev_get_drvdata(dev);
> > +	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 imx258 *imx258 = dev_get_drvdata(dev);
> > +
> > +	clk_disable_unprepare(imx258->clk);
> > +
> > +	return 0;
> > +}
> > +
> >  static int imx258_set_stream(struct v4l2_subdev *sd, int enable)
> >  {
> >  	struct imx258 *imx258 = to_imx258(sd);
> > @@ -1201,9 +1228,27 @@ 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;
> > +
> > +	dev_set_drvdata(&client->dev, imx258);
> 
> This you cannot do --- it'll be overwritten by v4l2_i2c_subdev_init().

Right, thanks.

> 
> > +
> > +	imx258->clk = devm_clk_get_optional(&client->dev, NULL);
> > +	if (!imx258->clk) {
> 
> You can move declaration of val here (I think).

No, the val is used later in further device_property_read* calls.

> 
> > +		dev_info(&client->dev, "no clock provided, using clock-frequency property\n");
> 
> As this is showing up on all ACPI based systems, I guess dev_dbg() would be
> more appropriate.

Sure, I'll make it debug.

> 
> Please also wrap lines over 80 if they reasonably can be.

OK

Thanks for the review.

Best regards,
Krzysztof

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

* Re: [PATCH v2 1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor
  2020-09-21 11:26   ` Krzysztof Kozlowski
@ 2020-09-22 19:23     ` Rob Herring
  2020-09-23 15:06       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 9+ messages in thread
From: Rob Herring @ 2020-09-22 19:23 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Sakari Ailus, Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Linux Media Mailing List, devicetree, linux-arm-kernel,
	linux-kernel

On Mon, Sep 21, 2020 at 5:27 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Mon, Sep 14, 2020 at 02:13:10PM -0600, Rob Herring wrote:
> > On Wed, Sep 02, 2020 at 09:18:08AM +0200, Krzysztof Kozlowski wrote:
> > > Add bindings for the IMX258 camera sensor.  The bindings, just like the
> > > driver, are quite limited, e.g. do not support regulator supplies.
> >
> > Bindings should be complete, not what a driver happens to currently
> > support.
>
> I'll add then more complete picture.
>
> >
> > >
> > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > >
> > > ---
> > >
> > > Changes since v1:
> > > 1. None
> > > ---
> > >  .../devicetree/bindings/media/i2c/imx258.yaml      | 92 ++++++++++++++++++++++
> > >  MAINTAINERS                                        |  1 +
> > >  2 files changed, 93 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/imx258.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/imx258.yaml b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
> > > new file mode 100644
> > > index 000000000000..ef789ad31143
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/imx258.yaml
> > > @@ -0,0 +1,92 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/i2c/imx258.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Sony IMX258 13 Mpixel CMOS Digital Image Sensor
> > > +
> > > +maintainers:
> > > +  - Krzysztof Kozlowski <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:
> > > +    maxItems: 1
> > > +
> > > +  clock-frequency:
> > > +    description: Frequency of input clock if clock is not provided
> > > +    deprecated: true
> >
> > Why are we adding something deprecated on a new binding?
>
> My intention was also to document it but indeed easier to skip it.
>
> >
> > > +    const: 19200000
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  # See ../video-interfaces.txt for more details
> > > +  port:
> > > +    type: object
> > > +    properties:
> > > +      endpoint:
> > > +        type: object
> > > +        properties:
> > > +          data-lanes:
> > > +            items:
> > > +              - const: 1
> > > +              - const: 2
> > > +              - const: 3
> > > +              - const: 4
> >
> > If this is the only config, why does it need to be in DT?
>
> The sensor is capable of two settings: two lanes (1 and 2) and four
> lanes described above.  However Linux driver requires the latter (four
> lanes, 1+2+3+4).
>
> If I were to describe the bindings for HW, someone would really be
> confused and try to use two lanes setup, which won't work. Driver won't
> allow it.

If someone has h/w with only 2 lanes connected, then they have to go
add support to the driver whether we've documented 2 lanes in the
binding or not.

> I understand that bindings document the HW and describe its interface
> but do we really want to put "theoretical" bindings which cannot be used
> in practice with Linux kernel?
>
> If yes, how to nicely document this that only one setting is currently
> working?

You don't, at least in the binding. That's a driver issue. Bindings
are separate. They are stored in the kernel tree for convenience, not
because they are part of the kernel.

Rob

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

* Re: [PATCH v2 1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor
  2020-09-22 19:23     ` Rob Herring
@ 2020-09-23 15:06       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 9+ messages in thread
From: Krzysztof Kozlowski @ 2020-09-23 15:06 UTC (permalink / raw)
  To: Rob Herring
  Cc: Sakari Ailus, Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Linux Media Mailing List, devicetree, linux-arm-kernel,
	linux-kernel

On Tue, 22 Sep 2020 at 21:23, Rob Herring <robh@kernel.org> wrote:
>
> > > If this is the only config, why does it need to be in DT?
> >
> > The sensor is capable of two settings: two lanes (1 and 2) and four
> > lanes described above.  However Linux driver requires the latter (four
> > lanes, 1+2+3+4).
> >
> > If I were to describe the bindings for HW, someone would really be
> > confused and try to use two lanes setup, which won't work. Driver won't
> > allow it.
>
> If someone has h/w with only 2 lanes connected, then they have to go
> add support to the driver whether we've documented 2 lanes in the
> binding or not.
>
> > I understand that bindings document the HW and describe its interface
> > but do we really want to put "theoretical" bindings which cannot be used
> > in practice with Linux kernel?
> >
> > If yes, how to nicely document this that only one setting is currently
> > working?
>
> You don't, at least in the binding. That's a driver issue. Bindings
> are separate. They are stored in the kernel tree for convenience, not
> because they are part of the kernel.

Mhmm... okay. I already sent v3 for this with fixed four lanes so I
will re-spin with 2 or 4 lanes setup.

Thanks for the review!

Best regards,
Krzysztof

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

end of thread, other threads:[~2020-09-23 15:06 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-02  7:18 [PATCH v2 1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor Krzysztof Kozlowski
2020-09-02  7:18 ` [PATCH v2 2/3] media: imx258: Add support for binding via device tree Krzysztof Kozlowski
2020-09-02  7:18 ` [PATCH v2 3/3] media: imx258: Get clock from device properties and enable it via runtime PM Krzysztof Kozlowski
2020-09-02  7:39   ` Sakari Ailus
2020-09-21 12:02     ` Krzysztof Kozlowski
2020-09-14 20:13 ` [PATCH v2 1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor Rob Herring
2020-09-21 11:26   ` Krzysztof Kozlowski
2020-09-22 19:23     ` Rob Herring
2020-09-23 15:06       ` 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).