lkml.org 
[lkml]   [2021]   [Jan]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v4 2/2] media: dt-bindings: media: i2c: Add IMX300 CMOS sensor binding
From
Date
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!
>

\
 
 \ /
  Last update: 2021-01-19 01:28    [W:0.092 / U:2.380 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site