Re: [PATCH 1/2] media: dt-bindings: media: i2c: Add MT9M114 camera sensor binding

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Jacopo,

On Sat, May 16, 2020 at 04:32:08PM +0200, Jacopo Mondi 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
> 
> Does it make sense with a single data lane ?

That's an open question, which Sakari has raised too. As the value is
fixed for this particular device, I didn't make the property mandatory.
Should it be disallowed completely ? I see pros and cons, and there are
cases where properties that may vary but are fixed for a particular
device are still required in DT to help with generic parsing code.
However, making the property optional means that generic code can't rely
on it anyway, so it probably has little value.

I'd like to hear from Rob about a general policy for this type of
situation.

> > +
> > +          bus-width:
> > +            items:
> > +              - const: 8
> 
> Same question if the bus width is fixed to 8.
> 
> > +
> > +          hsync-active:
> > +            items:
> > +              - const: 1
> > +
> > +          vsync-active:
> > +            items:
> > +              - const: 1
> > +
> > +        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:
>       endpoint
> ?

Endpoints are not generally required, as ports can be left unconnected.
There probably little use for this in this particular case, but I could
imagine a system with two sensors and only one of them being connected
to the SoC (with 0Ω resistors to implement a cheap and hardwired mux).
We could have both sensors in DT, with only the endpoints being
dependent on the board mounting options. This could simplify writing
more generic DT. I'd thus rather not make the endpoint mandatory.

> > +
> > +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 = <&reg_cam_1v8>;
> > +            vdd-supply = <&reg_cam_1v8>;
> > +            vaa-supply = <&reg_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 = <&reg_cam_1v8>;
> > +            vdd-supply = <&reg_cam_1v8>;
> > +            vaa-supply = <&reg_2p8v>;
> > +
> > +            port {
> > +                endpoint {
> > +                    bus-type = <5>;
> > +                    bus-width = <8>;
> > +                    hsync-active = <1>;
> > +                    vsync-active = <1>;
> > +                    remote-endpoint = <&parallel_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
> > +
> >  MT9P031 APTINA CAMERA SENSOR
> >  M:	Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >  L:	linux-media@xxxxxxxxxxxxxxx

-- 
Regards,

Laurent Pinchart



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux