diff mbox series

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

Message ID 1599031090-21608-1-git-send-email-krzk@kernel.org
State Superseded
Headers show
Series [v2,1/3] dt-bindings: media: imx258: Add bindings for IMX258 sensor | expand

Commit Message

Krzysztof Kozlowski Sept. 2, 2020, 7:18 a.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>

---

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

Comments

Sakari Ailus Sept. 2, 2020, 7:39 a.m. UTC | #1 
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
Rob Herring (Arm) Sept. 14, 2020, 8:13 p.m. UTC | #2 
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
>
Krzysztof Kozlowski Sept. 21, 2020, 12:02 p.m. UTC | #3 
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
Rob Herring (Arm) Sept. 22, 2020, 7:23 p.m. UTC | #4 
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
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