mbox series

[v4,0/2] Add support for the Sony Exmor-RS IMX300 camera sensor

Message ID 20210113182934.444727-1-angelogioacchino.delregno@somainline.org
Headers show
Series Add support for the Sony Exmor-RS IMX300 camera sensor | expand

Message

AngeloGioacchino Del Regno Jan. 13, 2021, 6:29 p.m. UTC
This patch series adds support for the IMX300 camera sensor, (one of the)
first Exmor-RS Stacked CMOS sensor(s), with support for both of the
supported aspect ratios (4:3 and 16:9).
This driver came out from reverse engineering of so called "userspace
drivers" from Sony Xperia smartphones.

I tried to document all of my findings and giving a sense to the registers
as much as possible, but that was only partially possible and resembles
some names from the IMX219 public datasheet, even though the addresses are
basically completely different.

This camera sensor driver was tested with all the resolutions declared in
it on two phones: Sony Xperia XA2 and XA2 Ultra, on a SDM630 SoC and is
working great.

- Changes in v4:
  - My e-mail address was changed in all files to reflect the new one
  - Fixed selection targets and out-of-bounds rectangles (thanks Jacopo!)
  - Fixed a warning: changed do_div to div_s64 for 64-bit division
  - Added one more register definition

- Changes in v3:
  - Removed unneeded fallthrough statements
  - Fixed double mode initialization at probe time
  - Fixed typo in the dt-binding description (8->25MPixels)
  - Fixed dt-binding data-lanes description, added to required properties

- Changes in v2:
  - Changed dt-binding name and fixed a misconception about lane
    operation (sensor supports 2/4-Lane, driver supports 4-Lane only)
  - Now using lowercase names for regulator supplies
  - Fixed redefinition of clock-noncontinuous property
  - Added informations about constraints on data bus frequencies
  - Fixed MAINTAINERS: removed git tree

AngeloGioacchino Del Regno (2):
  media: i2c: Add driver for the Sony Exmor-RS IMX300 camera sensor
  media: dt-bindings: media: i2c: Add IMX300 CMOS sensor binding

 .../bindings/media/i2c/sony,imx300.yaml       |  112 +
 MAINTAINERS                                   |    7 +
 drivers/media/i2c/Kconfig                     |   13 +
 drivers/media/i2c/Makefile                    |    1 +
 drivers/media/i2c/imx300.c                    | 3084 +++++++++++++++++
 5 files changed, 3217 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml
 create mode 100644 drivers/media/i2c/imx300.c

Comments

Sakari Ailus Jan. 16, 2021, 11:44 p.m. UTC | #1
Hi AngeloGioacchino,

On Wed, Jan 13, 2021 at 07:29:34PM +0100, AngeloGioacchino Del Regno wrote:
> Add YAML device tree binding for IMX300 CMOS image sensor, and
> the relevant MAINTAINERS entries.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> ---
>  .../bindings/media/i2c/sony,imx300.yaml       | 112 ++++++++++++++++++
>  MAINTAINERS                                   |   7 ++
>  2 files changed, 119 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml
> new file mode 100644
> index 000000000000..4fa767feea80
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml
> @@ -0,0 +1,112 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/sony,imx300.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sony 1/2.3-Inch 25Mpixel Stacked CMOS Digital Image Sensor
> +
> +maintainers:
> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> +
> +description: |-
> +  The Sony IMX300 is a 1/2.3-inch Stacked CMOS (Exmor-RS) digital image
> +  sensor with a pixel size of 1.08um and an active array size of
> +  5948H x 4140V. It is programmable through I2C interface at address 0x10.
> +  Image data is sent through MIPI CSI-2, which is configured as either 2 or
> +  4 data lanes.
> +
> +properties:
> +  compatible:
> +    const: sony,imx300
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1

Please add assigned clock related properties; see
Documentation/driver-api/media/camera-sensor.rst .

> +
> +  vdig-supply:
> +    description:
> +      Digital I/O voltage supply, 1.15-1.20 volts
> +
> +  vana-supply:
> +    description:
> +      Analog voltage supply, 2.2 volts
> +
> +  vddl-supply:
> +    description:
> +      Digital core voltage supply, 1.8 volts
> +
> +  reset-gpios:
> +    description: |-
> +      Reference to the GPIO connected to the xclr pin, if any.
> +      Must be released (set high) after all supplies are applied.
> +
> +  # See ../video-interfaces.txt for more details
> +  port:
> +    type: object
> +    properties:
> +      endpoint:
> +        type: object
> +
> +        properties:
> +          data-lanes:
> +            description: |-
> +              The driver only supports four-lane operation.

This can be removed as bindings describe hardware, not driver operation.

> +            items:
> +              - const: 0
> +              - const: 1
> +              - const: 2
> +              - const: 3

Two lanes here, too?

> +
> +          clock-noncontinuous: true
> +
> +          link-frequencies:
> +            $ref: /schemas/types.yaml#/definitions/uint64-array
> +            description:
> +              Allowed data bus frequencies. The driver currently needs
> +              to switch between 780000000 and 480000000 Hz in order to
> +              guarantee functionality of all modes.

You can omit this description, too.

> +
> +        required:
> +          - data-lanes
> +          - link-frequencies
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - vana-supply
> +  - vdig-supply
> +  - vddl-supply

Are the regulators really required? I'm not quite sure about the
established practices; still the common case is that one or two of these
are hard-wired.

> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        imx300: sensor@10 {
> +            compatible = "sony,imx300";
> +            reg = <0x10>;
> +            clocks = <&imx300_xclk>;
> +            vana-supply = <&imx300_vana>;   /* 2.2v */
> +            vdig-supply = <&imx300_vdig>;   /* 1.2v */
> +            vddl-supply = <&imx300_vddl>;   /* 1.8v */
> +
> +            port {
> +                imx300_0: endpoint {
> +                    remote-endpoint = <&csi1_ep>;
> +                    data-lanes = <0 1 2 3>;
> +                    clock-noncontinuous;
> +                    link-frequencies = /bits/ 64 <780000000 480000000>;
> +                };
> +            };
> +        };
> +    };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ad9abb42f852..5e0f08f48d48 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16633,6 +16633,13 @@ T:	git git://linuxtv.org/media_tree.git
>  F:	Documentation/devicetree/bindings/media/i2c/imx290.txt
>  F:	drivers/media/i2c/imx290.c
>  
> +SONY IMX300 SENSOR DRIVER
> +M:	AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> +L:	linux-media@vger.kernel.org

Please also add the git tree.

Ideally also the MAINTAINERS change comes with the first patch adding the
files, which should be the DT bindings. I.e. just reverse the order of the
patches.

> +S:	Maintained
> +F:	Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml
> +F:	drivers/media/i2c/imx300.c
> +
>  SONY IMX319 SENSOR DRIVER
>  M:	Bingbu Cao <bingbu.cao@intel.com>
>  L:	linux-media@vger.kernel.org
AngeloGioacchino Del Regno Jan. 17, 2021, 5:51 p.m. UTC | #2
Il 17/01/21 00:44, Sakari Ailus ha scritto:
> Hi AngeloGioacchino,

> 

> On Wed, Jan 13, 2021 at 07:29:34PM +0100, AngeloGioacchino Del Regno wrote:

>> Add YAML device tree binding for IMX300 CMOS image sensor, and

>> the relevant MAINTAINERS entries.

>>

>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

>> ---

>>   .../bindings/media/i2c/sony,imx300.yaml       | 112 ++++++++++++++++++

>>   MAINTAINERS                                   |   7 ++

>>   2 files changed, 119 insertions(+)

>>   create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml

>>

>> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml

>> new file mode 100644

>> index 000000000000..4fa767feea80

>> --- /dev/null

>> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml

>> @@ -0,0 +1,112 @@

>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)

>> +%YAML 1.2

>> +---

>> +$id: http://devicetree.org/schemas/media/i2c/sony,imx300.yaml#

>> +$schema: http://devicetree.org/meta-schemas/core.yaml#

>> +

>> +title: Sony 1/2.3-Inch 25Mpixel Stacked CMOS Digital Image Sensor

>> +

>> +maintainers:

>> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

>> +

>> +description: |-

>> +  The Sony IMX300 is a 1/2.3-inch Stacked CMOS (Exmor-RS) digital image

>> +  sensor with a pixel size of 1.08um and an active array size of

>> +  5948H x 4140V. It is programmable through I2C interface at address 0x10.

>> +  Image data is sent through MIPI CSI-2, which is configured as either 2 or

>> +  4 data lanes.

>> +

>> +properties:

>> +  compatible:

>> +    const: sony,imx300

>> +

>> +  reg:

>> +    maxItems: 1

>> +

>> +  clocks:

>> +    maxItems: 1

> 

> Please add assigned clock related properties; see

> Documentation/driver-api/media/camera-sensor.rst .

> 

Will do!

>> +

>> +  vdig-supply:

>> +    description:

>> +      Digital I/O voltage supply, 1.15-1.20 volts

>> +

>> +  vana-supply:

>> +    description:

>> +      Analog voltage supply, 2.2 volts

>> +

>> +  vddl-supply:

>> +    description:

>> +      Digital core voltage supply, 1.8 volts

>> +

>> +  reset-gpios:

>> +    description: |-

>> +      Reference to the GPIO connected to the xclr pin, if any.

>> +      Must be released (set high) after all supplies are applied.

>> +

>> +  # See ../video-interfaces.txt for more details

>> +  port:

>> +    type: object

>> +    properties:

>> +      endpoint:

>> +        type: object

>> +

>> +        properties:

>> +          data-lanes:

>> +            description: |-

>> +              The driver only supports four-lane operation.

> 

> This can be removed as bindings describe hardware, not driver operation.

> 

Ack.

>> +            items:

>> +              - const: 0

>> +              - const: 1

>> +              - const: 2

>> +              - const: 3

> 

> Two lanes here, too?

> 


The driver only supports four-lane operation.
I am 100% sure that this sensor can also work with two lanes, but it 
needs special configuration which I'm not able to produce, nor test.

As you may imagine (and as you can read in the driver itself), all of 
this was reverse-engineering work, as Sony has never released any 
datasheet for this sensor - and I have a hunch - they never will (but 
that's another story).

>> +

>> +          clock-noncontinuous: true

>> +

>> +          link-frequencies:

>> +            $ref: /schemas/types.yaml#/definitions/uint64-array

>> +            description:

>> +              Allowed data bus frequencies. The driver currently needs

>> +              to switch between 780000000 and 480000000 Hz in order to

>> +              guarantee functionality of all modes.

> 

> You can omit this description, too.

> 


The intention here was to be clear and provide as much information as I 
could gather during the very time-consuming reverse engineering process 
that took place in the making of this driver.

But okay, I will remove this.

>> +

>> +        required:

>> +          - data-lanes

>> +          - link-frequencies

>> +

>> +required:

>> +  - compatible

>> +  - reg

>> +  - clocks

>> +  - vana-supply

>> +  - vdig-supply

>> +  - vddl-supply

> 

> Are the regulators really required? I'm not quite sure about the

> established practices; still the common case is that one or two of these

> are hard-wired.

> 


On all the Sony phones that I have (....many), with MSM8956, MSM8996, 
SDM630, equipped with the IMX300 camera assy, none of these three are 
hard-wired: sometimes they're wired to the LDOs of the PMIC, sometimes 
they're wired to fixed LDOs, enabled through GPIOs (fixed-regulator 
binding in this case).

So.. yeah, they're really required.

>> +  - port

>> +

>> +additionalProperties: false

>> +

>> +examples:

>> +  - |

>> +    i2c0 {

>> +        #address-cells = <1>;

>> +        #size-cells = <0>;

>> +

>> +        imx300: sensor@10 {

>> +            compatible = "sony,imx300";

>> +            reg = <0x10>;

>> +            clocks = <&imx300_xclk>;

>> +            vana-supply = <&imx300_vana>;   /* 2.2v */

>> +            vdig-supply = <&imx300_vdig>;   /* 1.2v */

>> +            vddl-supply = <&imx300_vddl>;   /* 1.8v */

>> +

>> +            port {

>> +                imx300_0: endpoint {

>> +                    remote-endpoint = <&csi1_ep>;

>> +                    data-lanes = <0 1 2 3>;

>> +                    clock-noncontinuous;

>> +                    link-frequencies = /bits/ 64 <780000000 480000000>;

>> +                };

>> +            };

>> +        };

>> +    };

>> +

>> +...

>> diff --git a/MAINTAINERS b/MAINTAINERS

>> index ad9abb42f852..5e0f08f48d48 100644

>> --- a/MAINTAINERS

>> +++ b/MAINTAINERS

>> @@ -16633,6 +16633,13 @@ T:	git git://linuxtv.org/media_tree.git

>>   F:	Documentation/devicetree/bindings/media/i2c/imx290.txt

>>   F:	drivers/media/i2c/imx290.c

>>   

>> +SONY IMX300 SENSOR DRIVER

>> +M:	AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

>> +L:	linux-media@vger.kernel.org

> 

> Please also add the git tree.

> 

> Ideally also the MAINTAINERS change comes with the first patch adding the

> files, which should be the DT bindings. I.e. just reverse the order of the

> patches.

> 


I haven't added it because last time I did that I got reviews saying 
that if I'm not the owner of the git tree I shall not put it in.
Though, if that's a requirement for media, then I didn't know that...

>> +S:	Maintained

>> +F:	Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml

>> +F:	drivers/media/i2c/imx300.c

>> +

>>   SONY IMX319 SENSOR DRIVER

>>   M:	Bingbu Cao <bingbu.cao@intel.com>

>>   L:	linux-media@vger.kernel.org

> 


Thank you for your review!

Yours,
- Angelo
AngeloGioacchino Del Regno Jan. 17, 2021, 10:22 p.m. UTC | #3
Il 17/01/21 00:44, Sakari Ailus ha scritto:
> Hi AngeloGioacchino,
> 
> On Wed, Jan 13, 2021 at 07:29:34PM +0100, AngeloGioacchino Del Regno wrote:
>> Add YAML device tree binding for IMX300 CMOS image sensor, and
>> the relevant MAINTAINERS entries.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
>> ---
>>   .../bindings/media/i2c/sony,imx300.yaml       | 112 ++++++++++++++++++
>>   MAINTAINERS                                   |   7 ++
>>   2 files changed, 119 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml
>> new file mode 100644
>> index 000000000000..4fa767feea80
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml
>> @@ -0,0 +1,112 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/i2c/sony,imx300.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Sony 1/2.3-Inch 25Mpixel Stacked CMOS Digital Image Sensor
>> +
>> +maintainers:
>> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
>> +
>> +description: |-
>> +  The Sony IMX300 is a 1/2.3-inch Stacked CMOS (Exmor-RS) digital image
>> +  sensor with a pixel size of 1.08um and an active array size of
>> +  5948H x 4140V. It is programmable through I2C interface at address 0x10.
>> +  Image data is sent through MIPI CSI-2, which is configured as either 2 or
>> +  4 data lanes.
>> +
>> +properties:
>> +  compatible:
>> +    const: sony,imx300
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
> 
> Please add assigned clock related properties; see
> Documentation/driver-api/media/camera-sensor.rst .
> 
>> +
>> +  vdig-supply:
>> +    description:
>> +      Digital I/O voltage supply, 1.15-1.20 volts
>> +
>> +  vana-supply:
>> +    description:
>> +      Analog voltage supply, 2.2 volts
>> +
>> +  vddl-supply:
>> +    description:
>> +      Digital core voltage supply, 1.8 volts
>> +
>> +  reset-gpios:
>> +    description: |-
>> +      Reference to the GPIO connected to the xclr pin, if any.
>> +      Must be released (set high) after all supplies are applied.
>> +
>> +  # See ../video-interfaces.txt for more details
>> +  port:
>> +    type: object
>> +    properties:
>> +      endpoint:
>> +        type: object
>> +
>> +        properties:
>> +          data-lanes:
>> +            description: |-
>> +              The driver only supports four-lane operation.
> 
> This can be removed as bindings describe hardware, not driver operation.
> 
>> +            items:
>> +              - const: 0
>> +              - const: 1
>> +              - const: 2
>> +              - const: 3
> 
> Two lanes here, too?
> 
>> +
>> +          clock-noncontinuous: true
>> +
>> +          link-frequencies:
>> +            $ref: /schemas/types.yaml#/definitions/uint64-array
>> +            description:
>> +              Allowed data bus frequencies. The driver currently needs
>> +              to switch between 780000000 and 480000000 Hz in order to
>> +              guarantee functionality of all modes.
> 
> You can omit this description, too.
> 
>> +
>> +        required:
>> +          - data-lanes
>> +          - link-frequencies
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - vana-supply
>> +  - vdig-supply
>> +  - vddl-supply
> 
> Are the regulators really required? I'm not quite sure about the
> established practices; still the common case is that one or two of these
> are hard-wired.
> 
>> +  - port
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c0 {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        imx300: sensor@10 {
>> +            compatible = "sony,imx300";
>> +            reg = <0x10>;
>> +            clocks = <&imx300_xclk>;
>> +            vana-supply = <&imx300_vana>;   /* 2.2v */
>> +            vdig-supply = <&imx300_vdig>;   /* 1.2v */
>> +            vddl-supply = <&imx300_vddl>;   /* 1.8v */
>> +
>> +            port {
>> +                imx300_0: endpoint {
>> +                    remote-endpoint = <&csi1_ep>;
>> +                    data-lanes = <0 1 2 3>;
>> +                    clock-noncontinuous;
>> +                    link-frequencies = /bits/ 64 <780000000 480000000>;
>> +                };
>> +            };
>> +        };
>> +    };
>> +
>> +...
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ad9abb42f852..5e0f08f48d48 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -16633,6 +16633,13 @@ T:	git git://linuxtv.org/media_tree.git
>>   F:	Documentation/devicetree/bindings/media/i2c/imx290.txt
>>   F:	drivers/media/i2c/imx290.c
>>   
>> +SONY IMX300 SENSOR DRIVER
>> +M:	AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
>> +L:	linux-media@vger.kernel.org
> 
> Please also add the git tree.
> 
> Ideally also the MAINTAINERS change comes with the first patch adding the
> files, which should be the DT bindings. I.e. just reverse the order of the
> patches.
> 
>> +S:	Maintained
>> +F:	Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml
>> +F:	drivers/media/i2c/imx300.c
>> +
>>   SONY IMX319 SENSOR DRIVER
>>   M:	Bingbu Cao <bingbu.cao@intel.com>
>>   L:	linux-media@vger.kernel.org
> 

Besides that, I'm sorry I forgot to port the following tag from V3 of 
this patch, which didn't change.

I'm adding the forgotten R-b from Rob here:

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

Sorry again.
AngeloGioacchino Del Regno Jan. 19, 2021, 12:26 a.m. UTC | #4
Il 18/01/21 23:40, Sakari Ailus ha scritto:
> On Sun, Jan 17, 2021 at 06:51:04PM +0100, AngeloGioacchino Del Regno wrote:

>> Il 17/01/21 00:44, Sakari Ailus ha scritto:

>>> Hi AngeloGioacchino,

>>>

>>> On Wed, Jan 13, 2021 at 07:29:34PM +0100, AngeloGioacchino Del Regno wrote:

>>>> Add YAML device tree binding for IMX300 CMOS image sensor, and

>>>> the relevant MAINTAINERS entries.

>>>>

>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

>>>> ---

>>>>    .../bindings/media/i2c/sony,imx300.yaml       | 112 ++++++++++++++++++

>>>>    MAINTAINERS                                   |   7 ++

>>>>    2 files changed, 119 insertions(+)

>>>>    create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml

>>>>

>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml

>>>> new file mode 100644

>>>> index 000000000000..4fa767feea80

>>>> --- /dev/null

>>>> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml

>>>> @@ -0,0 +1,112 @@

>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)

>>>> +%YAML 1.2

>>>> +---

>>>> +$id: http://devicetree.org/schemas/media/i2c/sony,imx300.yaml#

>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#

>>>> +

>>>> +title: Sony 1/2.3-Inch 25Mpixel Stacked CMOS Digital Image Sensor

>>>> +

>>>> +maintainers:

>>>> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

>>>> +

>>>> +description: |-

>>>> +  The Sony IMX300 is a 1/2.3-inch Stacked CMOS (Exmor-RS) digital image

>>>> +  sensor with a pixel size of 1.08um and an active array size of

>>>> +  5948H x 4140V. It is programmable through I2C interface at address 0x10.

>>>> +  Image data is sent through MIPI CSI-2, which is configured as either 2 or

>>>> +  4 data lanes.

>>>> +

>>>> +properties:

>>>> +  compatible:

>>>> +    const: sony,imx300

>>>> +

>>>> +  reg:

>>>> +    maxItems: 1

>>>> +

>>>> +  clocks:

>>>> +    maxItems: 1

>>>

>>> Please add assigned clock related properties; see

>>> Documentation/driver-api/media/camera-sensor.rst .

>>>

>> Will do!

>>

>>>> +

>>>> +  vdig-supply:

>>>> +    description:

>>>> +      Digital I/O voltage supply, 1.15-1.20 volts

>>>> +

>>>> +  vana-supply:

>>>> +    description:

>>>> +      Analog voltage supply, 2.2 volts

>>>> +

>>>> +  vddl-supply:

>>>> +    description:

>>>> +      Digital core voltage supply, 1.8 volts

>>>> +

>>>> +  reset-gpios:

>>>> +    description: |-

>>>> +      Reference to the GPIO connected to the xclr pin, if any.

>>>> +      Must be released (set high) after all supplies are applied.

>>>> +

>>>> +  # See ../video-interfaces.txt for more details

>>>> +  port:

>>>> +    type: object

>>>> +    properties:

>>>> +      endpoint:

>>>> +        type: object

>>>> +

>>>> +        properties:

>>>> +          data-lanes:

>>>> +            description: |-

>>>> +              The driver only supports four-lane operation.

>>>

>>> This can be removed as bindings describe hardware, not driver operation.

>>>

>> Ack.

>>

>>>> +            items:

>>>> +              - const: 0

>>>> +              - const: 1

>>>> +              - const: 2

>>>> +              - const: 3

>>>

>>> Two lanes here, too?

>>>

>>

>> The driver only supports four-lane operation.

>> I am 100% sure that this sensor can also work with two lanes, but it needs

>> special configuration which I'm not able to produce, nor test.

>>

>> As you may imagine (and as you can read in the driver itself), all of this

>> was reverse-engineering work, as Sony has never released any datasheet for

>> this sensor - and I have a hunch - they never will (but that's another

>> story).

> 

> That's all fine. The bindings describe the hardware, not the driver's

> capabilities.

> 


Ok, will add the dual-lane configuration!

>>

>>>> +

>>>> +          clock-noncontinuous: true

>>>> +

>>>> +          link-frequencies:

>>>> +            $ref: /schemas/types.yaml#/definitions/uint64-array

>>>> +            description:

>>>> +              Allowed data bus frequencies. The driver currently needs

>>>> +              to switch between 780000000 and 480000000 Hz in order to

>>>> +              guarantee functionality of all modes.

>>>

>>> You can omit this description, too.

>>>

>>

>> The intention here was to be clear and provide as much information as I

>> could gather during the very time-consuming reverse engineering process that

>> took place in the making of this driver.

>>

>> But okay, I will remove this.

> 

> Again, this is about the hardware, not the driver. That information is also

> part of the driver.

> 


Sure! Removing for the next version!

>>

>>>> +

>>>> +        required:

>>>> +          - data-lanes

>>>> +          - link-frequencies

>>>> +

>>>> +required:

>>>> +  - compatible

>>>> +  - reg

>>>> +  - clocks

>>>> +  - vana-supply

>>>> +  - vdig-supply

>>>> +  - vddl-supply

>>>

>>> Are the regulators really required? I'm not quite sure about the

>>> established practices; still the common case is that one or two of these

>>> are hard-wired.

>>>

>>

>> On all the Sony phones that I have (....many), with MSM8956, MSM8996,

>> SDM630, equipped with the IMX300 camera assy, none of these three are

>> hard-wired: sometimes they're wired to the LDOs of the PMIC, sometimes

>> they're wired to fixed LDOs, enabled through GPIOs (fixed-regulator binding

>> in this case).

>>

>> So.. yeah, they're really required.

> 

> As noted, that depends on the board. You just happen to have some where

> they are not hard-wired.

> 


Sure, but then the supplies are required properties, since we are
describing the hardware: it can't work without power.

Besides that, when a board supplies power through fixed always-on
rails, DT users will specify a fixed-regulator binding with the
right voltages: this is a good habit for describing the board in DT.

>>

>>>> +  - port

>>>> +

>>>> +additionalProperties: false

>>>> +

>>>> +examples:

>>>> +  - |

>>>> +    i2c0 {

>>>> +        #address-cells = <1>;

>>>> +        #size-cells = <0>;

>>>> +

>>>> +        imx300: sensor@10 {

>>>> +            compatible = "sony,imx300";

>>>> +            reg = <0x10>;

>>>> +            clocks = <&imx300_xclk>;

>>>> +            vana-supply = <&imx300_vana>;   /* 2.2v */

>>>> +            vdig-supply = <&imx300_vdig>;   /* 1.2v */

>>>> +            vddl-supply = <&imx300_vddl>;   /* 1.8v */

>>>> +

>>>> +            port {

>>>> +                imx300_0: endpoint {

>>>> +                    remote-endpoint = <&csi1_ep>;

>>>> +                    data-lanes = <0 1 2 3>;

>>>> +                    clock-noncontinuous;

>>>> +                    link-frequencies = /bits/ 64 <780000000 480000000>;

>>>> +                };

>>>> +            };

>>>> +        };

>>>> +    };

>>>> +

>>>> +...

>>>> diff --git a/MAINTAINERS b/MAINTAINERS

>>>> index ad9abb42f852..5e0f08f48d48 100644

>>>> --- a/MAINTAINERS

>>>> +++ b/MAINTAINERS

>>>> @@ -16633,6 +16633,13 @@ T:	git git://linuxtv.org/media_tree.git

>>>>    F:	Documentation/devicetree/bindings/media/i2c/imx290.txt

>>>>    F:	drivers/media/i2c/imx290.c

>>>> +SONY IMX300 SENSOR DRIVER

>>>> +M:	AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

>>>> +L:	linux-media@vger.kernel.org

>>>

>>> Please also add the git tree.

>>>

>>> Ideally also the MAINTAINERS change comes with the first patch adding the

>>> files, which should be the DT bindings. I.e. just reverse the order of the

>>> patches.

>>>

>>

>> I haven't added it because last time I did that I got reviews saying that if

>> I'm not the owner of the git tree I shall not put it in.

>> Though, if that's a requirement for media, then I didn't know that...

> 

> The documentation in MAINTAINERS doesn't say that at least.

> 

> I think it'd be useful to have it.

> 

>>

>>>> +S:	Maintained

>>>> +F:	Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml

>>>> +F:	drivers/media/i2c/imx300.c

>>>> +

>>>>    SONY IMX319 SENSOR DRIVER

>>>>    M:	Bingbu Cao <bingbu.cao@intel.com>

>>>>    L:	linux-media@vger.kernel.org

>>>

>>

>> Thank you for your review!

> 

> You're welcome!

>
Rob Herring (Arm) Jan. 25, 2021, 8:26 p.m. UTC | #5
On Wed, Jan 13, 2021 at 07:29:34PM +0100, AngeloGioacchino Del Regno wrote:
> Add YAML device tree binding for IMX300 CMOS image sensor, and

> the relevant MAINTAINERS entries.

> 

> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

> ---

>  .../bindings/media/i2c/sony,imx300.yaml       | 112 ++++++++++++++++++

>  MAINTAINERS                                   |   7 ++

>  2 files changed, 119 insertions(+)

>  create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml

> 

> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml

> new file mode 100644

> index 000000000000..4fa767feea80

> --- /dev/null

> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml

> @@ -0,0 +1,112 @@

> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)

> +%YAML 1.2

> +---

> +$id: http://devicetree.org/schemas/media/i2c/sony,imx300.yaml#

> +$schema: http://devicetree.org/meta-schemas/core.yaml#

> +

> +title: Sony 1/2.3-Inch 25Mpixel Stacked CMOS Digital Image Sensor

> +

> +maintainers:

> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

> +

> +description: |-

> +  The Sony IMX300 is a 1/2.3-inch Stacked CMOS (Exmor-RS) digital image

> +  sensor with a pixel size of 1.08um and an active array size of

> +  5948H x 4140V. It is programmable through I2C interface at address 0x10.

> +  Image data is sent through MIPI CSI-2, which is configured as either 2 or

> +  4 data lanes.

> +

> +properties:

> +  compatible:

> +    const: sony,imx300

> +

> +  reg:

> +    maxItems: 1

> +

> +  clocks:

> +    maxItems: 1

> +

> +  vdig-supply:

> +    description:

> +      Digital I/O voltage supply, 1.15-1.20 volts

> +

> +  vana-supply:

> +    description:

> +      Analog voltage supply, 2.2 volts

> +

> +  vddl-supply:

> +    description:

> +      Digital core voltage supply, 1.8 volts

> +

> +  reset-gpios:

> +    description: |-

> +      Reference to the GPIO connected to the xclr pin, if any.

> +      Must be released (set high) after all supplies are applied.

> +

> +  # See ../video-interfaces.txt for more details

> +  port:

> +    type: object

> +    properties:

> +      endpoint:


This now needs to reference the graph.yaml and video-interfaces.yaml 
schemas. See the media tree or linux-next for examples.

> +        type: object

> +

> +        properties:

> +          data-lanes:

> +            description: |-

> +              The driver only supports four-lane operation.

> +            items:

> +              - const: 0

> +              - const: 1

> +              - const: 2

> +              - const: 3

> +

> +          clock-noncontinuous: true

> +

> +          link-frequencies:

> +            $ref: /schemas/types.yaml#/definitions/uint64-array

> +            description:

> +              Allowed data bus frequencies. The driver currently needs

> +              to switch between 780000000 and 480000000 Hz in order to

> +              guarantee functionality of all modes.

> +

> +        required:

> +          - data-lanes

> +          - link-frequencies

> +

> +required:

> +  - compatible

> +  - reg

> +  - clocks

> +  - vana-supply

> +  - vdig-supply

> +  - vddl-supply

> +  - port

> +

> +additionalProperties: false

> +

> +examples:

> +  - |

> +    i2c0 {

> +        #address-cells = <1>;

> +        #size-cells = <0>;

> +

> +        imx300: sensor@10 {

> +            compatible = "sony,imx300";

> +            reg = <0x10>;

> +            clocks = <&imx300_xclk>;

> +            vana-supply = <&imx300_vana>;   /* 2.2v */

> +            vdig-supply = <&imx300_vdig>;   /* 1.2v */

> +            vddl-supply = <&imx300_vddl>;   /* 1.8v */

> +

> +            port {

> +                imx300_0: endpoint {

> +                    remote-endpoint = <&csi1_ep>;

> +                    data-lanes = <0 1 2 3>;

> +                    clock-noncontinuous;

> +                    link-frequencies = /bits/ 64 <780000000 480000000>;

> +                };

> +            };

> +        };

> +    };

> +

> +...

> diff --git a/MAINTAINERS b/MAINTAINERS

> index ad9abb42f852..5e0f08f48d48 100644

> --- a/MAINTAINERS

> +++ b/MAINTAINERS

> @@ -16633,6 +16633,13 @@ T:	git git://linuxtv.org/media_tree.git

>  F:	Documentation/devicetree/bindings/media/i2c/imx290.txt

>  F:	drivers/media/i2c/imx290.c

>  

> +SONY IMX300 SENSOR DRIVER

> +M:	AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

> +L:	linux-media@vger.kernel.org

> +S:	Maintained

> +F:	Documentation/devicetree/bindings/media/i2c/sony,imx300.yaml

> +F:	drivers/media/i2c/imx300.c

> +

>  SONY IMX319 SENSOR DRIVER

>  M:	Bingbu Cao <bingbu.cao@intel.com>

>  L:	linux-media@vger.kernel.org

> -- 

> 2.29.2

>