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@xxxxxxxxxxxxxx> > > > --- > > > .../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@xxxxxxxxxxxxxx> > > > + > > > +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. > > > > + > > > + 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. > > > > + > > > + 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. > > > > + - 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@xxxxxxxxxxxxxx> > > > +L: linux-media@xxxxxxxxxxxxxxx > > > > 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@xxxxxxxxx> > > > L: linux-media@xxxxxxxxxxxxxxx > > > > Thank you for your review! You're welcome! -- Sakari Ailus
