Date:   Tue, 19 Jan 2021 01:26:14 +0100
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...ainline.org>
To:     Sakari Ailus <sakari.ailus@....fi>
Cc:     mchehab@...nel.org, robh+dt@...nel.org, shawnguo@...nel.org,
        s.hauer@...gutronix.de, linux-media@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        phone-devel@...r.kernel.org, konrad.dybcio@...ainline.org,
        marijn.suijten@...ainline.org, martin.botka@...ainline.org
Subject: Re: [PATCH v4 2/2] media: dt-bindings: media: i2c: Add IMX300 CMOS
 sensor binding

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@...ainline.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@...ainline.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@...ainline.org>
>>>> +L:	linux-media@...r.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@...el.com>
>>>>    L:	linux-media@...r.kernel.org
>>>
>>
>> Thank you for your review!
> 
> You're welcome!
> 

Powered by blists - more mailing lists