Messages in this thread |  | | Subject | Re: [PATCH v4 2/2] media: dt-bindings: media: i2c: Add IMX300 CMOS sensor binding | From | AngeloGioacchino Del Regno <> | Date | Tue, 19 Jan 2021 01:26:14 +0100 |
| |
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! >
|  |