diff mbox series

[1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor

Message ID 20200828160053.6064-1-krzk@kernel.org
State Superseded, archived
Headers show
Series [1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor | expand

Checks

Context Check Description
robh/checkpatch success
robh/dt-meta-schema success

Commit Message

Krzysztof Kozlowski Aug. 28, 2020, 4 p.m. UTC 
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>
---
 .../devicetree/bindings/media/i2c/imx258.yaml | 92 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 93 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/imx258.yaml

Comments

Sakari Ailus Aug. 31, 2020, 9:08 a.m. UTC | #1 
Hi Krzysztof,

Thanks for the patchset.

On Fri, Aug 28, 2020 at 06:00:53PM +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.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  drivers/media/i2c/imx258.c | 107 ++++++++++++++++++++++++++++++-------
>  1 file changed, 87 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index c20bac9b00ec..4d763dcabb1d 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)
> @@ -747,6 +753,12 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
>  	if (pm_runtime_get_if_in_use(&client->dev) == 0)
>  		return 0;
>  
> +	ret = clk_prepare_enable(imx258->clk);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to enable clock\n");
> +		goto out;
> +	}
> +
>  	switch (ctrl->id) {
>  	case V4L2_CID_ANALOGUE_GAIN:
>  		ret = imx258_write_reg(imx258, IMX258_REG_ANALOG_GAIN,
> @@ -779,6 +791,8 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
>  		break;
>  	}
>  
> +out:
> +	clk_disable_unprepare(imx258->clk);
>  	pm_runtime_put(&client->dev);
>  
>  	return ret;
> @@ -972,10 +986,40 @@ static int imx258_stop_streaming(struct imx258 *imx258)
>  	return 0;
>  }
>  
> +static int imx258_power_on(struct imx258 *imx258)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
> +	int ret;
> +
> +	ret = pm_runtime_get_sync(&client->dev);
> +	if (ret < 0)
> +		goto err;

Please continue to use runtime PM directly, and move the clock control to
runtime PM callbacks (apart from probe and remove).

> +
> +	ret = clk_prepare_enable(imx258->clk);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to enable clock\n");
> +		goto err;
> +	}
> +
> +	return 0;
> +
> +err:
> +	pm_runtime_put_noidle(&client->dev);
> +
> +	return ret;
> +}
> +
> +static void imx258_power_off(struct imx258 *imx258)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
> +
> +	clk_disable_unprepare(imx258->clk);
> +	pm_runtime_put(&client->dev);
> +}
> +
>  static int imx258_set_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct imx258 *imx258 = to_imx258(sd);
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  	int ret = 0;
>  
>  	mutex_lock(&imx258->mutex);
> @@ -985,11 +1029,9 @@ static int imx258_set_stream(struct v4l2_subdev *sd, int enable)
>  	}
>  
>  	if (enable) {
> -		ret = pm_runtime_get_sync(&client->dev);
> -		if (ret < 0) {
> -			pm_runtime_put_noidle(&client->dev);
> +		ret = imx258_power_on(imx258);
> +		if (ret < 0)
>  			goto err_unlock;
> -		}
>  
>  		/*
>  		 * Apply default & customized values
> @@ -997,10 +1039,10 @@ static int imx258_set_stream(struct v4l2_subdev *sd, int enable)
>  		 */
>  		ret = imx258_start_streaming(imx258);
>  		if (ret)
> -			goto err_rpm_put;
> +			goto err_power_off;
>  	} else {
>  		imx258_stop_streaming(imx258);
> -		pm_runtime_put(&client->dev);
> +		imx258_power_off(imx258);
>  	}
>  
>  	imx258->streaming = enable;
> @@ -1008,8 +1050,8 @@ static int imx258_set_stream(struct v4l2_subdev *sd, int enable)
>  
>  	return ret;
>  
> -err_rpm_put:
> -	pm_runtime_put(&client->dev);
> +err_power_off:
> +	imx258_power_off(imx258);
>  err_unlock:
>  	mutex_unlock(&imx258->mutex);
>  
> @@ -1201,21 +1243,41 @@ 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_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;
> +		}
> +
> +		ret = clk_prepare_enable(imx258->clk);
> +		if (ret) {
> +			dev_err(&client->dev, "failed to enable clock\n");
> +			return ret;
> +		}
> +	}
>  
>  	/*
>  	 * Check that the device is mounted upside down. The driver only
>  	 * supports a single pixel order right now.
>  	 */
>  	ret = device_property_read_u32(&client->dev, "rotation", &val);
> -	if (ret || val != 180)
> -		return -EINVAL;
> -
> -	imx258 = devm_kzalloc(&client->dev, sizeof(*imx258), GFP_KERNEL);
> -	if (!imx258)
> -		return -ENOMEM;
> +	if (ret || val != 180) {
> +		ret = -EINVAL;
> +		goto error_prop_read;
> +	}
>  
>  	/* Initialize subdev */
>  	v4l2_i2c_subdev_init(&imx258->sd, client, &imx258_subdev_ops);
> @@ -1223,14 +1285,14 @@ static int imx258_probe(struct i2c_client *client)
>  	/* Check module identity */
>  	ret = imx258_identify_module(imx258);
>  	if (ret)
> -		return ret;
> +		goto error_prop_read;
>  
>  	/* Set default mode to max resolution */
>  	imx258->cur_mode = &supported_modes[0];
>  
>  	ret = imx258_init_controls(imx258);
>  	if (ret)
> -		return ret;
> +		goto error_prop_read;
>  
>  	/* Initialize subdev */
>  	imx258->sd.internal_ops = &imx258_internal_ops;
> @@ -1252,8 +1314,13 @@ static int imx258_probe(struct i2c_client *client)
>  	pm_runtime_enable(&client->dev);
>  	pm_runtime_idle(&client->dev);
>  
> +	clk_disable_unprepare(imx258->clk);
> +
>  	return 0;
>  
> +error_prop_read:
> +	clk_disable_unprepare(imx258->clk);

How about remove? Think of how this works when runtime PM is disabled.

> +
>  error_media_entity:
>  	media_entity_cleanup(&imx258->sd.entity);
>
Krzysztof Kozlowski Sept. 2, 2020, 6:25 a.m. UTC | #2 
On Mon, 31 Aug 2020 at 11:09, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Hi Krzysztof,
>
> Thanks for the patchset.
>
> On Fri, Aug 28, 2020 at 06:00:53PM +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.
> >
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > ---
> >  drivers/media/i2c/imx258.c | 107 ++++++++++++++++++++++++++++++-------
> >  1 file changed, 87 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > index c20bac9b00ec..4d763dcabb1d 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)
> > @@ -747,6 +753,12 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
> >       if (pm_runtime_get_if_in_use(&client->dev) == 0)
> >               return 0;
> >
> > +     ret = clk_prepare_enable(imx258->clk);
> > +     if (ret) {
> > +             dev_err(&client->dev, "failed to enable clock\n");
> > +             goto out;
> > +     }
> > +
> >       switch (ctrl->id) {
> >       case V4L2_CID_ANALOGUE_GAIN:
> >               ret = imx258_write_reg(imx258, IMX258_REG_ANALOG_GAIN,
> > @@ -779,6 +791,8 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
> >               break;
> >       }
> >
> > +out:
> > +     clk_disable_unprepare(imx258->clk);
> >       pm_runtime_put(&client->dev);
> >
> >       return ret;
> > @@ -972,10 +986,40 @@ static int imx258_stop_streaming(struct imx258 *imx258)
> >       return 0;
> >  }
> >
> > +static int imx258_power_on(struct imx258 *imx258)
> > +{
> > +     struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
> > +     int ret;
> > +
> > +     ret = pm_runtime_get_sync(&client->dev);
> > +     if (ret < 0)
> > +             goto err;
>
> Please continue to use runtime PM directly, and move the clock control to
> runtime PM callbacks (apart from probe and remove).

Runtime PM comes with its own overhead and using it for just toggling
clock is slightly waste of cycle but if this is preferred I can do it.

Best regards,
Krzysztof
diff mbox series

Patch

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