Hi Sakari, On Tue, May 12, 2020 at 11:45:51AM +0300, Sakari Ailus wrote: > On Tue, May 12, 2020 at 02:34:55AM +0300, Laurent Pinchart wrote: > > Add device tree binding for the ON Semiconductor MT9M114 CMOS camera > > sensor. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > --- > > .../bindings/media/i2c/onnn,mt9m114.yaml | 188 ++++++++++++++++++ > > MAINTAINERS | 7 + > > 2 files changed, 195 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml > > new file mode 100644 > > index 000000000000..2c3c691aacfd > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml > > @@ -0,0 +1,188 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/i2c/onnn,mt9m114.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: ON Semiconductor 1/6-inch 720p CMOS Digital Image Sensor > > + > > +maintainers: > > + - Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > + > > +description: |- > > + The ON Semiconductor MT9M114 is a 1/6-inch 720p (1.26 Mp) CMOS digital image > > + sensor with an active pixel-array size of 1296H x 976V. It is programmable > > + through an I2C interface and outputs image data over a 8-bit parallel or > > + 1-lane MIPI CSI-2 connection. > > + > > +properties: > > + compatible: > > + const: onnn,mt9m114 > > + > > + reg: > > + description: I2C device address > > + enum: > > + - 0x48 > > + - 0x5d > > + > > + clocks: > > + description: EXTCLK clock signal > > + maxItems: 1 > > + > > + vdd-supply: > > + description: > > + Core digital voltage supply, 1.8V > > + > > + vddio-supply: > > + description: > > + I/O digital voltage supply, 1.8V or 2.8V > > + > > + vaa-supply: > > + description: > > + Analog voltage supply, 2.8V > > + > > + reset-gpios: > > + description: |- > > + Reference to the GPIO connected to the RESET_BAR pin, if any (active > > + low). > > + > > + # See ../video-interfaces.txt for more details > > + port: > > + type: object > > + properties: > > + endpoint: > > + type: object > > + properties: > > + bus-type: > > + enum: [4, 5, 6] > > + > > + clock-lanes: > > + items: > > + - const: 0 > > + > > + data-lanes: > > + items: > > + - const: 1 > > + > > + bus-width: > > + items: > > + - const: 8 > > + > > + hsync-active: > > + items: > > + - const: 1 > > + > > + vsync-active: > > + items: > > + - const: 1 > > Is there anything to configure here with these properties apart from > bus-type? If not, they should be omitted. The bindings make those properties optional. Do you think that we generally should require them not to be present when their value is fixed ? This implies that the mt9m114 driver won't be able to parse them (not that it would need any particular information anyway). It also implies that, if the driver wants to call __v4l2_fwnode_endpoint_parse() (indirectly as that's a static function), it will need to fill the flags with the hardcoded values before calling the function. This however conflicts with __v4l2_fwnode_endpoint_parse() zeroing vep->bus when the bus type is unknown, and the bus type can't be hardcoded as there are three options. The other option would be for the driver to fill the flags after calling __v4l2_fwnode_endpoint_parse() if it wants to rely on the contents of v4l2_fwnode_endpoint through the code, which could be the best option. And I've now read the documentation of v4l2_fwnode_endpoint_alloc_parse(): * This function parses the V4L2 fwnode endpoint specific parameters from the * firmware. The caller is responsible for assigning @vep.bus_type to a valid * media bus type. The caller may also set the default configuration for the * endpoint --- a configuration that shall be in line with the DT binding * documentation. Should a device support multiple bus types, the caller may * call this function once the correct type is found --- with a default * configuration valid for that type. * * As a compatibility means guessing the bus type is also supported by setting * @vep.bus_type to V4L2_MBUS_UNKNOWN. The caller may not provide a default * configuration in this case as the defaults are specific to a given bus type. * This functionality is deprecated and should not be used in new drivers and it * is only supported for CSI-2 D-PHY, parallel and Bt.656 buses. If V4L2_MBUS_UNKNOWN is deprecated, what is the caller supposed to do when multiple bus types are supported by the device ? I'm having a bit of trouble figuring out how everything fits together. The other consequence of forbidding these properties is that the connected entity won't be able to parse the DT properties of the sensor endpoint, but that's probably an improvement, as doing so would be layering violation. It however means that generic helper functions would have equal trouble (as shown by the above explanation). > > + > > + required: > > + - bus-type > > + > > + allOf: > > + - if: > > + properties: > > + bus-type: > > + const: 4 > > + then: > > + properties: > > + bus-width: false > > + hsync-active: false > > + vsync-active: false > > + > > + - if: > > + properties: > > + bus-type: > > + const: 5 > > + then: > > + properties: > > + clock-lanes: false > > + data-lanes: false > > + > > + - if: > > + properties: > > + bus-type: > > + const: 6 > > + then: > > + properties: > > + clock-lanes: false > > + data-lanes: false > > + hsync-active: false > > + vsync-active: false > > + > > + unevaluatedProperties: false > > + > > +required: > > + - compatible > > + - reg > > + - clocks > > + - vdd-supply > > + - vddio-supply > > + - vaa-supply > > + - port > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/gpio/gpio.h> > > + > > + i2c0 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + sensor@48 { > > + compatible = "onnn,mt9m114"; > > + reg = <0x48>; > > + > > + clocks = <&clk24m 0>; > > + > > + reset-gpios = <&gpio5 21 GPIO_ACTIVE_LOW>; > > + > > + vddio-supply = <®_cam_1v8>; > > + vdd-supply = <®_cam_1v8>; > > + vaa-supply = <®_2p8v>; > > + > > + port { > > + endpoint { > > + bus-type = <4>; > > + clock-lanes = <0>; > > + data-lanes = <1>; > > + remote-endpoint = <&mipi_csi_in>; > > + }; > > + }; > > + }; > > + }; > > + > > + - | > > + #include <dt-bindings/gpio/gpio.h> > > + > > + i2c0 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + sensor@5d { > > + compatible = "onnn,mt9m114"; > > + reg = <0x5d>; > > + > > + clocks = <&clk24m 0>; > > + > > + reset-gpios = <&gpio5 21 GPIO_ACTIVE_LOW>; > > + > > + vddio-supply = <®_cam_1v8>; > > + vdd-supply = <®_cam_1v8>; > > + vaa-supply = <®_2p8v>; > > + > > + port { > > + endpoint { > > + bus-type = <5>; > > + bus-width = <8>; > > + hsync-active = <1>; > > + vsync-active = <1>; > > + remote-endpoint = <¶llel_in>; > > + }; > > + }; > > + }; > > + }; > > + > > +... > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 091ec22c1a23..61d2fb6d049e 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -11457,6 +11457,13 @@ T: git git://linuxtv.org/media_tree.git > > F: drivers/media/i2c/mt9m032.c > > F: include/media/i2c/mt9m032.h > > > > +MT9M114 ON SEMICONDUCTOR SENSOR DRIVER > > +M: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > +L: linux-media@xxxxxxxxxxxxxxx > > +S: Maintained > > +T: git git://linuxtv.org/media_tree.git > > +F: Documentation/devicetree/bindings/media/i2c.onnn,mt9m114.yaml > > s/\./\// > > > + > > MT9P031 APTINA CAMERA SENSOR > > M: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > L: linux-media@xxxxxxxxxxxxxxx > > -- > > Regards, > > > > Laurent Pinchart > > Wow! Even your git format-patch is polite. 8-) :-) -- Regards, Laurent Pinchart
