| Message ID | 20230707210604.868002-1-robh@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: dt-bindings: Merge OV5695 into OV5693 binding | expand |
On Fri, Jul 07, 2023 at 03:06:04PM -0600, Rob Herring wrote: > The OV5695 binding is almost the same as the OV5693 binding. The only > difference is 'clock-names' is defined for OV5695. However, the lack of > clock-names is an omission as the Linux OV5693 driver expects the same > 'xvclk' clock name. > > 'link-frequencies' is required by OV5693, but not OV5695. Just drop it > from being required. Expressing it conditionally would be ugly. It > shouldn't really be required either as the driver only supports 1 > frequency anyways. I suppose the intent here is something like "the driver only supports 1 frequency and never bothers to read the property"? Either way, Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Cheers, Conor. > > The rockchip-isp1 binding example is missing required properties, so it > has to be updated as well. > > Signed-off-by: Rob Herring <robh@kernel.org> > --- > .../devicetree/bindings/media/i2c/ov5695.txt | 41 ------------------- > .../bindings/media/i2c/ovti,ov5693.yaml | 19 +++++---- > .../bindings/media/rockchip-isp1.yaml | 1 + > 3 files changed, 13 insertions(+), 48 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5695.txt > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5695.txt b/Documentation/devicetree/bindings/media/i2c/ov5695.txt > deleted file mode 100644 > index 640a63717d96..000000000000 > --- a/Documentation/devicetree/bindings/media/i2c/ov5695.txt > +++ /dev/null > @@ -1,41 +0,0 @@ > -* Omnivision OV5695 MIPI CSI-2 sensor > - > -Required Properties: > -- compatible: shall be "ovti,ov5695" > -- clocks: reference to the xvclk input clock > -- clock-names: shall be "xvclk" > -- avdd-supply: Analog voltage supply, 2.8 volts > -- dovdd-supply: Digital I/O voltage supply, 1.8 volts > -- dvdd-supply: Digital core voltage supply, 1.2 volts > -- reset-gpios: Low active reset gpio > - > -The device node shall contain one 'port' child node with an > -'endpoint' subnode for its digital output video port, > -in accordance with the video interface bindings defined in > -Documentation/devicetree/bindings/media/video-interfaces.txt. > -The endpoint optional property 'data-lanes' shall be "<1 2>". > - > -Example: > -&i2c7 { > - ov5695: camera-sensor@36 { > - compatible = "ovti,ov5695"; > - reg = <0x36>; > - pinctrl-names = "default"; > - pinctrl-0 = <&clk_24m_cam>; > - > - clocks = <&cru SCLK_TESTCLKOUT1>; > - clock-names = "xvclk"; > - > - avdd-supply = <&pp2800_cam>; > - dovdd-supply = <&pp1800>; > - dvdd-supply = <&pp1250_cam>; > - reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>; > - > - port { > - wcam_out: endpoint { > - remote-endpoint = <&mipi_in_wcam>; > - data-lanes = <1 2>; > - }; > - }; > - }; > -}; > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml > index 359dc08440a8..a3d73a87d797 100644 > --- a/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml > @@ -5,26 +5,29 @@ > $id: http://devicetree.org/schemas/media/i2c/ovti,ov5693.yaml# > $schema: http://devicetree.org/meta-schemas/core.yaml# > > -title: Omnivision OV5693 CMOS Sensor > +title: Omnivision OV5693/OV5695 CMOS Sensors > > maintainers: > - Tommaso Merciai <tommaso.merciai@amarulasolutions.com> > > description: | > - The Omnivision OV5693 is a high performance, 1/4-inch, 5 megapixel, CMOS > - image sensor that delivers 2592x1944 at 30fps. It provides full-frame, > + The Omnivision OV5693/OV5695 are high performance, 1/4-inch, 5 megapixel, CMOS > + image sensors that deliver 2592x1944 at 30fps. It provides full-frame, > sub-sampled, and windowed 10-bit MIPI images in various formats via the > Serial Camera Control Bus (SCCB) interface. > > - OV5693 is controlled via I2C and two-wire Serial Camera Control Bus (SCCB). > - The sensor output is available via CSI-2 serial data output (up to 2-lane). > + OV5693/OV5695 are controlled via I2C and two-wire Serial Camera Control Bus > + (SCCB). The sensor output is available via CSI-2 serial data output (up to > + 2-lane). > > allOf: > - $ref: /schemas/media/video-interface-devices.yaml# > > properties: > compatible: > - const: ovti,ov5693 > + enum: > + - ovti,ov5693 > + - ovti,ov5695 > > reg: > maxItems: 1 > @@ -34,6 +37,9 @@ properties: > System input clock (aka XVCLK). From 6 to 27 MHz. > maxItems: 1 > > + clock-names: > + const: xvclk > + > dovdd-supply: > description: > Digital I/O voltage supply, 1.8V. > @@ -72,7 +78,6 @@ properties: > > required: > - data-lanes > - - link-frequencies > > required: > - compatible > diff --git a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml > index 0bad7e640148..e466dff8286d 100644 > --- a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml > +++ b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml > @@ -199,6 +199,7 @@ examples: > wcam: camera@36 { > compatible = "ovti,ov5695"; > reg = <0x36>; > + clocks = <&cru SCLK_TESTCLKOUT1>; > > port { > wcam_out: endpoint { > -- > 2.40.1 >
On Mon, Jul 10, 2023 at 11:45 AM Conor Dooley <conor@kernel.org> wrote: > > On Fri, Jul 07, 2023 at 03:06:04PM -0600, Rob Herring wrote: > > The OV5695 binding is almost the same as the OV5693 binding. The only > > difference is 'clock-names' is defined for OV5695. However, the lack of > > clock-names is an omission as the Linux OV5693 driver expects the same > > 'xvclk' clock name. > > > > 'link-frequencies' is required by OV5693, but not OV5695. Just drop it > > from being required. Expressing it conditionally would be ugly. It > > shouldn't really be required either as the driver only supports 1 > > frequency anyways. > > I suppose the intent here is something like "the driver only supports 1 > frequency and never bothers to read the property"? It does read it and fails if it doesn't match. I don't really think the driver should if there is only 1 freq. I don't know if it's that the hw only supports 1 frequency or a driver limitation. If the h/w, then the property is pointless. > Either way, > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > > Cheers, > Conor. > > > > > The rockchip-isp1 binding example is missing required properties, so it > > has to be updated as well. > > > > Signed-off-by: Rob Herring <robh@kernel.org> > > --- > > .../devicetree/bindings/media/i2c/ov5695.txt | 41 ------------------- > > .../bindings/media/i2c/ovti,ov5693.yaml | 19 +++++---- > > .../bindings/media/rockchip-isp1.yaml | 1 + > > 3 files changed, 13 insertions(+), 48 deletions(-) > > delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5695.txt > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5695.txt b/Documentation/devicetree/bindings/media/i2c/ov5695.txt > > deleted file mode 100644 > > index 640a63717d96..000000000000 > > --- a/Documentation/devicetree/bindings/media/i2c/ov5695.txt > > +++ /dev/null > > @@ -1,41 +0,0 @@ > > -* Omnivision OV5695 MIPI CSI-2 sensor > > - > > -Required Properties: > > -- compatible: shall be "ovti,ov5695" > > -- clocks: reference to the xvclk input clock > > -- clock-names: shall be "xvclk" > > -- avdd-supply: Analog voltage supply, 2.8 volts > > -- dovdd-supply: Digital I/O voltage supply, 1.8 volts > > -- dvdd-supply: Digital core voltage supply, 1.2 volts > > -- reset-gpios: Low active reset gpio > > - > > -The device node shall contain one 'port' child node with an > > -'endpoint' subnode for its digital output video port, > > -in accordance with the video interface bindings defined in > > -Documentation/devicetree/bindings/media/video-interfaces.txt. > > -The endpoint optional property 'data-lanes' shall be "<1 2>". > > - > > -Example: > > -&i2c7 { > > - ov5695: camera-sensor@36 { > > - compatible = "ovti,ov5695"; > > - reg = <0x36>; > > - pinctrl-names = "default"; > > - pinctrl-0 = <&clk_24m_cam>; > > - > > - clocks = <&cru SCLK_TESTCLKOUT1>; > > - clock-names = "xvclk"; > > - > > - avdd-supply = <&pp2800_cam>; > > - dovdd-supply = <&pp1800>; > > - dvdd-supply = <&pp1250_cam>; > > - reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>; > > - > > - port { > > - wcam_out: endpoint { > > - remote-endpoint = <&mipi_in_wcam>; > > - data-lanes = <1 2>; > > - }; > > - }; > > - }; > > -}; > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml > > index 359dc08440a8..a3d73a87d797 100644 > > --- a/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml > > @@ -5,26 +5,29 @@ > > $id: http://devicetree.org/schemas/media/i2c/ovti,ov5693.yaml# > > $schema: http://devicetree.org/meta-schemas/core.yaml# > > > > -title: Omnivision OV5693 CMOS Sensor > > +title: Omnivision OV5693/OV5695 CMOS Sensors > > > > maintainers: > > - Tommaso Merciai <tommaso.merciai@amarulasolutions.com> > > > > description: | > > - The Omnivision OV5693 is a high performance, 1/4-inch, 5 megapixel, CMOS > > - image sensor that delivers 2592x1944 at 30fps. It provides full-frame, > > + The Omnivision OV5693/OV5695 are high performance, 1/4-inch, 5 megapixel, CMOS > > + image sensors that deliver 2592x1944 at 30fps. It provides full-frame, > > sub-sampled, and windowed 10-bit MIPI images in various formats via the > > Serial Camera Control Bus (SCCB) interface. > > > > - OV5693 is controlled via I2C and two-wire Serial Camera Control Bus (SCCB). > > - The sensor output is available via CSI-2 serial data output (up to 2-lane). > > + OV5693/OV5695 are controlled via I2C and two-wire Serial Camera Control Bus > > + (SCCB). The sensor output is available via CSI-2 serial data output (up to > > + 2-lane). > > > > allOf: > > - $ref: /schemas/media/video-interface-devices.yaml# > > > > properties: > > compatible: > > - const: ovti,ov5693 > > + enum: > > + - ovti,ov5693 > > + - ovti,ov5695 > > > > reg: > > maxItems: 1 > > @@ -34,6 +37,9 @@ properties: > > System input clock (aka XVCLK). From 6 to 27 MHz. > > maxItems: 1 > > > > + clock-names: > > + const: xvclk > > + > > dovdd-supply: > > description: > > Digital I/O voltage supply, 1.8V. > > @@ -72,7 +78,6 @@ properties: > > > > required: > > - data-lanes > > - - link-frequencies > > > > required: > > - compatible > > diff --git a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml > > index 0bad7e640148..e466dff8286d 100644 > > --- a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml > > +++ b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml > > @@ -199,6 +199,7 @@ examples: > > wcam: camera@36 { > > compatible = "ovti,ov5695"; > > reg = <0x36>; > > + clocks = <&cru SCLK_TESTCLKOUT1>; > > > > port { > > wcam_out: endpoint { > > -- > > 2.40.1 > >
Hi Rob, On Fri, Jul 07, 2023 at 03:06:04PM -0600, Rob Herring wrote: > The OV5695 binding is almost the same as the OV5693 binding. The only > difference is 'clock-names' is defined for OV5695. However, the lack of > clock-names is an omission as the Linux OV5693 driver expects the same > 'xvclk' clock name. > > 'link-frequencies' is required by OV5693, but not OV5695. Just drop it > from being required. Expressing it conditionally would be ugly. It > shouldn't really be required either as the driver only supports 1 > frequency anyways. The correct way to address this would appear to be to add link-frequencies for both of these devices. I think I've seen one or two sensors of this class (raw, CSI-2/parallel, external clock etc.) with link frequencies documented as "fixed" --- which is probably a documentation issue more than anything else. Also see: <URL:https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html#handling-clocks>.
On Mon, Jul 31, 2023 at 5:21 AM Sakari Ailus <sakari.ailus@iki.fi> wrote: > > Hi Rob, > > On Fri, Jul 07, 2023 at 03:06:04PM -0600, Rob Herring wrote: > > The OV5695 binding is almost the same as the OV5693 binding. The only > > difference is 'clock-names' is defined for OV5695. However, the lack of > > clock-names is an omission as the Linux OV5693 driver expects the same > > 'xvclk' clock name. > > > > 'link-frequencies' is required by OV5693, but not OV5695. Just drop it > > from being required. Expressing it conditionally would be ugly. It > > shouldn't really be required either as the driver only supports 1 > > frequency anyways. > > The correct way to address this would appear to be to add link-frequencies > for both of these devices. I think I've seen one or two sensors of this > class (raw, CSI-2/parallel, external clock etc.) with link frequencies > documented as "fixed" --- which is probably a documentation issue more than > anything else. link-frequencies is already supported. It's just a question of being required or not. Adding a property as required is an ABI break (if the OS starts requiring the property). Rob
Hi Rob, On Tue, Aug 01, 2023 at 07:45:08AM -0600, Rob Herring wrote: > On Mon, Jul 31, 2023 at 5:21 AM Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > Hi Rob, > > > > On Fri, Jul 07, 2023 at 03:06:04PM -0600, Rob Herring wrote: > > > The OV5695 binding is almost the same as the OV5693 binding. The only > > > difference is 'clock-names' is defined for OV5695. However, the lack of > > > clock-names is an omission as the Linux OV5693 driver expects the same > > > 'xvclk' clock name. > > > > > > 'link-frequencies' is required by OV5693, but not OV5695. Just drop it > > > from being required. Expressing it conditionally would be ugly. It > > > shouldn't really be required either as the driver only supports 1 > > > frequency anyways. > > > > The correct way to address this would appear to be to add link-frequencies > > for both of these devices. I think I've seen one or two sensors of this > > class (raw, CSI-2/parallel, external clock etc.) with link frequencies > > documented as "fixed" --- which is probably a documentation issue more than > > anything else. > > link-frequencies is already supported. It's just a question of being > required or not. Adding a property as required is an ABI break (if the > OS starts requiring the property). Currently the ov5693 driver requires a link-frequencies property, the ov5695 driver doesn't care. In this case the backwards compatible way would be to make it optional for ov5695 and if it exists, then allow only those frequencies. It's a fairly trivial driver, it only supports a single link frequency at the moment (as well as a single external clock frequency). At least link-frequencies should continue to be required for ov5693.
diff --git a/Documentation/devicetree/bindings/media/i2c/ov5695.txt b/Documentation/devicetree/bindings/media/i2c/ov5695.txt deleted file mode 100644 index 640a63717d96..000000000000 --- a/Documentation/devicetree/bindings/media/i2c/ov5695.txt +++ /dev/null @@ -1,41 +0,0 @@ -* Omnivision OV5695 MIPI CSI-2 sensor - -Required Properties: -- compatible: shall be "ovti,ov5695" -- clocks: reference to the xvclk input clock -- clock-names: shall be "xvclk" -- avdd-supply: Analog voltage supply, 2.8 volts -- dovdd-supply: Digital I/O voltage supply, 1.8 volts -- dvdd-supply: Digital core voltage supply, 1.2 volts -- reset-gpios: Low active reset gpio - -The device node shall contain one 'port' child node with an -'endpoint' subnode for its digital output video port, -in accordance with the video interface bindings defined in -Documentation/devicetree/bindings/media/video-interfaces.txt. -The endpoint optional property 'data-lanes' shall be "<1 2>". - -Example: -&i2c7 { - ov5695: camera-sensor@36 { - compatible = "ovti,ov5695"; - reg = <0x36>; - pinctrl-names = "default"; - pinctrl-0 = <&clk_24m_cam>; - - clocks = <&cru SCLK_TESTCLKOUT1>; - clock-names = "xvclk"; - - avdd-supply = <&pp2800_cam>; - dovdd-supply = <&pp1800>; - dvdd-supply = <&pp1250_cam>; - reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>; - - port { - wcam_out: endpoint { - remote-endpoint = <&mipi_in_wcam>; - data-lanes = <1 2>; - }; - }; - }; -}; diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml index 359dc08440a8..a3d73a87d797 100644 --- a/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml @@ -5,26 +5,29 @@ $id: http://devicetree.org/schemas/media/i2c/ovti,ov5693.yaml# $schema: http://devicetree.org/meta-schemas/core.yaml# -title: Omnivision OV5693 CMOS Sensor +title: Omnivision OV5693/OV5695 CMOS Sensors maintainers: - Tommaso Merciai <tommaso.merciai@amarulasolutions.com> description: | - The Omnivision OV5693 is a high performance, 1/4-inch, 5 megapixel, CMOS - image sensor that delivers 2592x1944 at 30fps. It provides full-frame, + The Omnivision OV5693/OV5695 are high performance, 1/4-inch, 5 megapixel, CMOS + image sensors that deliver 2592x1944 at 30fps. It provides full-frame, sub-sampled, and windowed 10-bit MIPI images in various formats via the Serial Camera Control Bus (SCCB) interface. - OV5693 is controlled via I2C and two-wire Serial Camera Control Bus (SCCB). - The sensor output is available via CSI-2 serial data output (up to 2-lane). + OV5693/OV5695 are controlled via I2C and two-wire Serial Camera Control Bus + (SCCB). The sensor output is available via CSI-2 serial data output (up to + 2-lane). allOf: - $ref: /schemas/media/video-interface-devices.yaml# properties: compatible: - const: ovti,ov5693 + enum: + - ovti,ov5693 + - ovti,ov5695 reg: maxItems: 1 @@ -34,6 +37,9 @@ properties: System input clock (aka XVCLK). From 6 to 27 MHz. maxItems: 1 + clock-names: + const: xvclk + dovdd-supply: description: Digital I/O voltage supply, 1.8V. @@ -72,7 +78,6 @@ properties: required: - data-lanes - - link-frequencies required: - compatible diff --git a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml index 0bad7e640148..e466dff8286d 100644 --- a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml +++ b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml @@ -199,6 +199,7 @@ examples: wcam: camera@36 { compatible = "ovti,ov5695"; reg = <0x36>; + clocks = <&cru SCLK_TESTCLKOUT1>; port { wcam_out: endpoint {
The OV5695 binding is almost the same as the OV5693 binding. The only difference is 'clock-names' is defined for OV5695. However, the lack of clock-names is an omission as the Linux OV5693 driver expects the same 'xvclk' clock name. 'link-frequencies' is required by OV5693, but not OV5695. Just drop it from being required. Expressing it conditionally would be ugly. It shouldn't really be required either as the driver only supports 1 frequency anyways. The rockchip-isp1 binding example is missing required properties, so it has to be updated as well. Signed-off-by: Rob Herring <robh@kernel.org> --- .../devicetree/bindings/media/i2c/ov5695.txt | 41 ------------------- .../bindings/media/i2c/ovti,ov5693.yaml | 19 +++++---- .../bindings/media/rockchip-isp1.yaml | 1 + 3 files changed, 13 insertions(+), 48 deletions(-) delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5695.txt