[v15,1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings

  Add YAML device tree binding for OV02A10 CMOS image sensor, and the
relevant MAINTAINERS entries.

Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
---
 .../bindings/media/i2c/ovti,ov02a10.yaml           | 162 +++++++++++++++++++++
 MAINTAINERS                                        |   7 +
 2 files changed, 169 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
  

Hi Dongchun,

On Tue, Oct 13, 2020 at 09:05:02PM +0800, Dongchun Zhu wrote:
> Add YAML device tree binding for OV02A10 CMOS image sensor, and the
> relevant MAINTAINERS entries.
> 
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> ---
>  .../bindings/media/i2c/ovti,ov02a10.yaml           | 162 +++++++++++++++++++++
>  MAINTAINERS                                        |   7 +
>  2 files changed, 169 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> new file mode 100644
> index 0000000..fb98964
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> @@ -0,0 +1,162 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright (c) 2020 MediaTek Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov02a10.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Omnivision OV02A10 CMOS Sensor Device Tree Bindings
> +
> +maintainers:
> +  - Dongchun Zhu <dongchun.zhu@mediatek.com>
> +
> +description: |-
> +  The Omnivision OV02A10 is a low-cost, high performance, 1/5-inch, 2 megapixel
> +  image sensor, which is the latest production derived from Omnivision's CMOS
> +  image sensor technology. Ihis chip supports high frame rate speeds up to 30fps
> +  @ 1600x1200 (UXGA) resolution transferred over a 1-lane MIPI interface. The
> +  sensor output is available via CSI-2 serial data output.
> +
> +properties:
> +  compatible:
> +    const: ovti,ov02a10
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    description:
> +      External clock for the sensor.
> +    items:
> +      - const: eclk
> +
> +  clock-frequency:
> +    description:
> +      Frequency of the eclk clock in Hz.
> +
> +  dovdd-supply:
> +    description:
> +      Definition of the regulator used as Digital I/O voltage supply.
> +
> +  avdd-supply:
> +    description:
> +      Definition of the regulator used as Analog voltage supply.
> +
> +  dvdd-supply:
> +    description:
> +      Definition of the regulator used as Digital core voltage supply.
> +
> +  powerdown-gpios:
> +    description:
> +      Must be the device tree identifier of the GPIO connected to the
> +      PD_PAD pin. This pin is used to place the OV02A10 into standby mode
> +      or shutdown mode. As the line needs to be high for the powerdown mode
> +      to be active, it should be marked GPIO_ACTIVE_HIGH.
> +    maxItems: 1
> +
> +  reset-gpios:
> +    description:
> +      Must be the device tree identifier of the GPIO connected to the
> +      RST_PD pin. If specified, it will be asserted during driver probe.
> +      As the line needs to be low for the reset to be active, it should be
> +      marked GPIO_ACTIVE_LOW.
> +    maxItems: 1
> +
> +  rotation:
> +    description:
> +      Definition of the sensor's placement.
> +    allOf:
> +      - $ref: "/schemas/types.yaml#/definitions/uint32"
> +      - enum:
> +          - 0    # Sensor Mounted Upright
> +          - 180  # Sensor Mounted Upside Down
> +        default: 0
> +
> +  ovti,mipi-clock-voltage:
> +    description:
> +      An array of 2-tuples items, and each item consists of link frequency and
> +      MIPI clock voltage unit like <freq-kHz volt-unit>. Clock voltage unit is
> +      dependent upon link speed, indicating MIPI transmission speed select that
> +      controls D-PHY timing setting by adjusting MIPI clock voltage to improve
> +      the clock driver capability.
> +    $ref: "/schemas/types.yaml#/definitions/uint32-array"
> +    minItems: 2
> +    default: [390000, 4]

Why do you have the link frequency here as well?

In principle this does belong to the endpoint as link frequencies are
specific to that, but I don't mind; there's just a single port anyway.

> +
> +  # See ../video-interfaces.txt for details
> +  port:
> +    type: object
> +    additionalProperties: false
> +    description:
> +      Output port node, single endpoint describing the CSI-2 transmitter.
> +
> +    properties:
> +      endpoint:
> +        type: object
> +        additionalProperties: false
> +
> +        properties:
> +          link-frequencies: true
> +          remote-endpoint: true
> +
> +        required:
> +          - link-frequencies
> +          - remote-endpoint
> +
> +    required:
> +      - endpoint
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - clock-frequency
> +  - dovdd-supply
> +  - avdd-supply
> +  - dvdd-supply
> +  - powerdown-gpios
> +  - reset-gpios
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ov02a10: camera-sensor@3d {
> +            compatible = "ovti,ov02a10";
> +            reg = <0x3d>;
> +
> +            powerdown-gpios = <&pio 107 GPIO_ACTIVE_HIGH>;
> +            reset-gpios = <&pio 109 GPIO_ACTIVE_LOW>;
> +
> +            clocks = <&ov02a10_clk>;
> +            clock-names = "eclk";
> +            clock-frequency = <24000000>;
> +
> +            rotation = <180>;
> +            ovti,mipi-clock-voltage = <390000 4>;
> +
> +            dovdd-supply = <&ov02a10_dovdd>;
> +            avdd-supply = <&ov02a10_avdd>;
> +            dvdd-supply = <&ov02a10_dvdd>;
> +
> +            port {
> +                wcam_out: endpoint {
> +                    link-frequencies = /bits/ 64 <390000000>;
> +                    remote-endpoint = <&mipi_in_wcam>;
> +                };
> +            };
> +        };
> +    };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index deaafb6..48aa7a7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12718,6 +12718,13 @@ M:	Harald Welte <laforge@gnumonks.org>
>  S:	Maintained
>  F:	drivers/char/pcmcia/cm4040_cs.*
>  
> +OMNIVISION OV02A10 SENSOR DRIVER
> +M:	Dongchun Zhu <dongchun.zhu@mediatek.com>
> +L:	linux-media@vger.kernel.org
> +S:	Maintained
> +T:	git git://linuxtv.org/media_tree.git
> +F:	Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> +
>  OMNIVISION OV13858 SENSOR DRIVER
>  M:	Sakari Ailus <sakari.ailus@linux.intel.com>
>  L:	linux-media@vger.kernel.org
> -- 
> 2.9.2
  

Hello Sakari,

Thanks for your timely review.

On Tue, 2020-10-13 at 19:19 +0300, Sakari Ailus wrote:
> Hi Dongchun,
> 
> On Tue, Oct 13, 2020 at 09:05:02PM +0800, Dongchun Zhu wrote:
> > Add YAML device tree binding for OV02A10 CMOS image sensor, and the
> > relevant MAINTAINERS entries.
> > 
> > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > ---
> >  .../bindings/media/i2c/ovti,ov02a10.yaml           | 162 +++++++++++++++++++++
> >  MAINTAINERS                                        |   7 +
> >  2 files changed, 169 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > 

[snip]...

> > +  ovti,mipi-clock-voltage:
> > +    description:
> > +      An array of 2-tuples items, and each item consists of link frequency and
> > +      MIPI clock voltage unit like <freq-kHz volt-unit>. Clock voltage unit is
> > +      dependent upon link speed, indicating MIPI transmission speed select that
> > +      controls D-PHY timing setting by adjusting MIPI clock voltage to improve
> > +      the clock driver capability.
> > +    $ref: "/schemas/types.yaml#/definitions/uint32-array"
> > +    minItems: 2
> > +    default: [390000, 4]
> 
> Why do you have the link frequency here as well?
> 
> In principle this does belong to the endpoint as link frequencies are
> specific to that, but I don't mind; there's just a single port anyway.
> 

This is an optional property which we model as an array of <link speed,
clock voltage> pairs. An example to have all link speeds up to 390MHz
use the value 4 for current driver. If one wants to select different
voltage for different link, they could do so as well.
  

On Wed, Oct 14, 2020 at 10:10:18AM +0800, Dongchun Zhu wrote:
> Hello Sakari,
> 
> Thanks for your timely review.
> 
> On Tue, 2020-10-13 at 19:19 +0300, Sakari Ailus wrote:
> > Hi Dongchun,
> > 
> > On Tue, Oct 13, 2020 at 09:05:02PM +0800, Dongchun Zhu wrote:
> > > Add YAML device tree binding for OV02A10 CMOS image sensor, and the
> > > relevant MAINTAINERS entries.
> > > 
> > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > ---
> > >  .../bindings/media/i2c/ovti,ov02a10.yaml           | 162 +++++++++++++++++++++
> > >  MAINTAINERS                                        |   7 +
> > >  2 files changed, 169 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > 
> 
> [snip]...
> 
> > > +  ovti,mipi-clock-voltage:
> > > +    description:
> > > +      An array of 2-tuples items, and each item consists of link frequency and
> > > +      MIPI clock voltage unit like <freq-kHz volt-unit>. Clock voltage unit is
> > > +      dependent upon link speed, indicating MIPI transmission speed select that
> > > +      controls D-PHY timing setting by adjusting MIPI clock voltage to improve
> > > +      the clock driver capability.
> > > +    $ref: "/schemas/types.yaml#/definitions/uint32-array"
> > > +    minItems: 2
> > > +    default: [390000, 4]
> > 
> > Why do you have the link frequency here as well?
> > 
> > In principle this does belong to the endpoint as link frequencies are
> > specific to that, but I don't mind; there's just a single port anyway.
> > 
> 
> This is an optional property which we model as an array of <link speed,
> clock voltage> pairs. An example to have all link speeds up to 390MHz
> use the value 4 for current driver. If one wants to select different
> voltage for different link, they could do so as well.

If you think you'd need that, then you need to put this to the endpoint.
  

On Wed, Oct 14, 2020 at 10:31 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> On Wed, Oct 14, 2020 at 10:10:18AM +0800, Dongchun Zhu wrote:
> > Hello Sakari,
> >
> > Thanks for your timely review.
> >
> > On Tue, 2020-10-13 at 19:19 +0300, Sakari Ailus wrote:
> > > Hi Dongchun,
> > >
> > > On Tue, Oct 13, 2020 at 09:05:02PM +0800, Dongchun Zhu wrote:
> > > > Add YAML device tree binding for OV02A10 CMOS image sensor, and the
> > > > relevant MAINTAINERS entries.
> > > >
> > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > > ---
> > > >  .../bindings/media/i2c/ovti,ov02a10.yaml           | 162 +++++++++++++++++++++
> > > >  MAINTAINERS                                        |   7 +
> > > >  2 files changed, 169 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > >
> >
> > [snip]...
> >
> > > > +  ovti,mipi-clock-voltage:
> > > > +    description:
> > > > +      An array of 2-tuples items, and each item consists of link frequency and
> > > > +      MIPI clock voltage unit like <freq-kHz volt-unit>. Clock voltage unit is
> > > > +      dependent upon link speed, indicating MIPI transmission speed select that
> > > > +      controls D-PHY timing setting by adjusting MIPI clock voltage to improve
> > > > +      the clock driver capability.
> > > > +    $ref: "/schemas/types.yaml#/definitions/uint32-array"
> > > > +    minItems: 2
> > > > +    default: [390000, 4]
> > >
> > > Why do you have the link frequency here as well?
> > >
> > > In principle this does belong to the endpoint as link frequencies are
> > > specific to that, but I don't mind; there's just a single port anyway.
> > >
> >
> > This is an optional property which we model as an array of <link speed,
> > clock voltage> pairs. An example to have all link speeds up to 390MHz
> > use the value 4 for current driver. If one wants to select different
> > voltage for different link, they could do so as well.
>
> If you think you'd need that, then you need to put this to the endpoint.

The mipi-clock-voltage property is not a property of the endpoint. The
link frequency there does not set the link frequency - it only
specifies which link frequency the given voltage should be used for.

Best regards,
Tomasz
  

On Wed, Oct 14, 2020 at 01:48:00PM +0200, Tomasz Figa wrote:
> On Wed, Oct 14, 2020 at 10:31 AM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > On Wed, Oct 14, 2020 at 10:10:18AM +0800, Dongchun Zhu wrote:
> > > Hello Sakari,
> > >
> > > Thanks for your timely review.
> > >
> > > On Tue, 2020-10-13 at 19:19 +0300, Sakari Ailus wrote:
> > > > Hi Dongchun,
> > > >
> > > > On Tue, Oct 13, 2020 at 09:05:02PM +0800, Dongchun Zhu wrote:
> > > > > Add YAML device tree binding for OV02A10 CMOS image sensor, and the
> > > > > relevant MAINTAINERS entries.
> > > > >
> > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > > > ---
> > > > >  .../bindings/media/i2c/ovti,ov02a10.yaml           | 162 +++++++++++++++++++++
> > > > >  MAINTAINERS                                        |   7 +
> > > > >  2 files changed, 169 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > >
> > >
> > > [snip]...
> > >
> > > > > +  ovti,mipi-clock-voltage:
> > > > > +    description:
> > > > > +      An array of 2-tuples items, and each item consists of link frequency and
> > > > > +      MIPI clock voltage unit like <freq-kHz volt-unit>. Clock voltage unit is
> > > > > +      dependent upon link speed, indicating MIPI transmission speed select that
> > > > > +      controls D-PHY timing setting by adjusting MIPI clock voltage to improve
> > > > > +      the clock driver capability.
> > > > > +    $ref: "/schemas/types.yaml#/definitions/uint32-array"
> > > > > +    minItems: 2
> > > > > +    default: [390000, 4]
> > > >
> > > > Why do you have the link frequency here as well?
> > > >
> > > > In principle this does belong to the endpoint as link frequencies are
> > > > specific to that, but I don't mind; there's just a single port anyway.
> > > >
> > >
> > > This is an optional property which we model as an array of <link speed,
> > > clock voltage> pairs. An example to have all link speeds up to 390MHz
> > > use the value 4 for current driver. If one wants to select different
> > > voltage for different link, they could do so as well.
> >
> > If you think you'd need that, then you need to put this to the endpoint.
> 
> The mipi-clock-voltage property is not a property of the endpoint. The
> link frequency there does not set the link frequency - it only
> specifies which link frequency the given voltage should be used for.

Seems to me it's in the right place.

Rob
  

On Tue, 13 Oct 2020 21:05:02 +0800, Dongchun Zhu wrote:
> Add YAML device tree binding for OV02A10 CMOS image sensor, and the
> relevant MAINTAINERS entries.
> 
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> ---
>  .../bindings/media/i2c/ovti,ov02a10.yaml           | 162 +++++++++++++++++++++
>  MAINTAINERS                                        |   7 +
>  2 files changed, 169 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>
  

On Wed, Oct 14, 2020 at 01:48:00PM +0200, Tomasz Figa wrote:
> On Wed, Oct 14, 2020 at 10:31 AM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > On Wed, Oct 14, 2020 at 10:10:18AM +0800, Dongchun Zhu wrote:
> > > Hello Sakari,
> > >
> > > Thanks for your timely review.
> > >
> > > On Tue, 2020-10-13 at 19:19 +0300, Sakari Ailus wrote:
> > > > Hi Dongchun,
> > > >
> > > > On Tue, Oct 13, 2020 at 09:05:02PM +0800, Dongchun Zhu wrote:
> > > > > Add YAML device tree binding for OV02A10 CMOS image sensor, and the
> > > > > relevant MAINTAINERS entries.
> > > > >
> > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > > > ---
> > > > >  .../bindings/media/i2c/ovti,ov02a10.yaml           | 162 +++++++++++++++++++++
> > > > >  MAINTAINERS                                        |   7 +
> > > > >  2 files changed, 169 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > >
> > >
> > > [snip]...
> > >
> > > > > +  ovti,mipi-clock-voltage:
> > > > > +    description:
> > > > > +      An array of 2-tuples items, and each item consists of link frequency and
> > > > > +      MIPI clock voltage unit like <freq-kHz volt-unit>. Clock voltage unit is
> > > > > +      dependent upon link speed, indicating MIPI transmission speed select that
> > > > > +      controls D-PHY timing setting by adjusting MIPI clock voltage to improve
> > > > > +      the clock driver capability.
> > > > > +    $ref: "/schemas/types.yaml#/definitions/uint32-array"
> > > > > +    minItems: 2
> > > > > +    default: [390000, 4]
> > > >
> > > > Why do you have the link frequency here as well?
> > > >
> > > > In principle this does belong to the endpoint as link frequencies are
> > > > specific to that, but I don't mind; there's just a single port anyway.
> > > >
> > >
> > > This is an optional property which we model as an array of <link speed,
> > > clock voltage> pairs. An example to have all link speeds up to 390MHz
> > > use the value 4 for current driver. If one wants to select different
> > > voltage for different link, they could do so as well.
> >
> > If you think you'd need that, then you need to put this to the endpoint.
> 
> The mipi-clock-voltage property is not a property of the endpoint. The
> link frequency there does not set the link frequency - it only
> specifies which link frequency the given voltage should be used for.

If it is not bound to the endpoint --- that specifies the properties of the
link --- then you could just put it to the driver, couldn't you?

But earlier on, we established that this is specific to a given board, or a
given link. Endpoints are used to describe connections that a device may
have to other devices. Therefore their properties are different.

The graph bindings allow for several endpoints on a single physical port,
some may be disabled in that case. This is admittedly mostly done with
CSI-2 receivers.

Either way, putting this property to the device may affect how DT source is
arranged for boards that have this device. Any similar configuration on
other devices with multiple transmitters or receivers would be in endpoints
anyway --- because they have to, so I don't see why bindings for this
device should be different.

If you put it to endpoint, the driver also doesn't need to match the link
frequencies in kHz and Hz between the two properties, but just use the same
index for both.
  

On Mon, Oct 19, 2020 at 10:28 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> On Wed, Oct 14, 2020 at 01:48:00PM +0200, Tomasz Figa wrote:
> > On Wed, Oct 14, 2020 at 10:31 AM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > On Wed, Oct 14, 2020 at 10:10:18AM +0800, Dongchun Zhu wrote:
> > > > Hello Sakari,
> > > >
> > > > Thanks for your timely review.
> > > >
> > > > On Tue, 2020-10-13 at 19:19 +0300, Sakari Ailus wrote:
> > > > > Hi Dongchun,
> > > > >
> > > > > On Tue, Oct 13, 2020 at 09:05:02PM +0800, Dongchun Zhu wrote:
> > > > > > Add YAML device tree binding for OV02A10 CMOS image sensor, and the
> > > > > > relevant MAINTAINERS entries.
> > > > > >
> > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > > > > ---
> > > > > >  .../bindings/media/i2c/ovti,ov02a10.yaml           | 162 +++++++++++++++++++++
> > > > > >  MAINTAINERS                                        |   7 +
> > > > > >  2 files changed, 169 insertions(+)
> > > > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > > >
> > > >
> > > > [snip]...
> > > >
> > > > > > +  ovti,mipi-clock-voltage:
> > > > > > +    description:
> > > > > > +      An array of 2-tuples items, and each item consists of link frequency and
> > > > > > +      MIPI clock voltage unit like <freq-kHz volt-unit>. Clock voltage unit is
> > > > > > +      dependent upon link speed, indicating MIPI transmission speed select that
> > > > > > +      controls D-PHY timing setting by adjusting MIPI clock voltage to improve
> > > > > > +      the clock driver capability.
> > > > > > +    $ref: "/schemas/types.yaml#/definitions/uint32-array"
> > > > > > +    minItems: 2
> > > > > > +    default: [390000, 4]
> > > > >
> > > > > Why do you have the link frequency here as well?
> > > > >
> > > > > In principle this does belong to the endpoint as link frequencies are
> > > > > specific to that, but I don't mind; there's just a single port anyway.
> > > > >
> > > >
> > > > This is an optional property which we model as an array of <link speed,
> > > > clock voltage> pairs. An example to have all link speeds up to 390MHz
> > > > use the value 4 for current driver. If one wants to select different
> > > > voltage for different link, they could do so as well.
> > >
> > > If you think you'd need that, then you need to put this to the endpoint.
> >
> > The mipi-clock-voltage property is not a property of the endpoint. The
> > link frequency there does not set the link frequency - it only
> > specifies which link frequency the given voltage should be used for.
>
> If it is not bound to the endpoint --- that specifies the properties of the
> link --- then you could just put it to the driver, couldn't you?
>
> But earlier on, we established that this is specific to a given board, or a
> given link. Endpoints are used to describe connections that a device may
> have to other devices. Therefore their properties are different.
>
> The graph bindings allow for several endpoints on a single physical port,
> some may be disabled in that case. This is admittedly mostly done with
> CSI-2 receivers.
>
> Either way, putting this property to the device may affect how DT source is
> arranged for boards that have this device. Any similar configuration on
> other devices with multiple transmitters or receivers would be in endpoints
> anyway --- because they have to, so I don't see why bindings for this
> device should be different.
>
> If you put it to endpoint, the driver also doesn't need to match the link
> frequencies in kHz and Hz between the two properties, but just use the same
> index for both.

I agree in general, but I think in this case this is a device-specific
property and the sensor only has 1 CSI-2 interface, so maybe it
doesn't really matter?

Specifying frequency, voltage pairs is a common practice in other
bindings, such as the OPP bindings, by the way.

That said, if you insist, I'm fine with this being moved to the
endpoint node too.

Best regards,
Tomasz
  

On Mon, Oct 19, 2020 at 9:20 PM Tomasz Figa <tfiga@chromium.org> wrote:
>
> On Mon, Oct 19, 2020 at 10:28 AM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > On Wed, Oct 14, 2020 at 01:48:00PM +0200, Tomasz Figa wrote:
> > > On Wed, Oct 14, 2020 at 10:31 AM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > On Wed, Oct 14, 2020 at 10:10:18AM +0800, Dongchun Zhu wrote:
> > > > > Hello Sakari,
> > > > >
> > > > > Thanks for your timely review.
> > > > >
> > > > > On Tue, 2020-10-13 at 19:19 +0300, Sakari Ailus wrote:
> > > > > > Hi Dongchun,
> > > > > >
> > > > > > On Tue, Oct 13, 2020 at 09:05:02PM +0800, Dongchun Zhu wrote:
> > > > > > > Add YAML device tree binding for OV02A10 CMOS image sensor, and the
> > > > > > > relevant MAINTAINERS entries.
> > > > > > >
> > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > > > > > ---
> > > > > > >  .../bindings/media/i2c/ovti,ov02a10.yaml           | 162 +++++++++++++++++++++
> > > > > > >  MAINTAINERS                                        |   7 +
> > > > > > >  2 files changed, 169 insertions(+)
> > > > > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > > > >
> > > > >
> > > > > [snip]...
> > > > >
> > > > > > > +  ovti,mipi-clock-voltage:
> > > > > > > +    description:
> > > > > > > +      An array of 2-tuples items, and each item consists of link frequency and
> > > > > > > +      MIPI clock voltage unit like <freq-kHz volt-unit>. Clock voltage unit is
> > > > > > > +      dependent upon link speed, indicating MIPI transmission speed select that
> > > > > > > +      controls D-PHY timing setting by adjusting MIPI clock voltage to improve
> > > > > > > +      the clock driver capability.
> > > > > > > +    $ref: "/schemas/types.yaml#/definitions/uint32-array"
> > > > > > > +    minItems: 2
> > > > > > > +    default: [390000, 4]
> > > > > >
> > > > > > Why do you have the link frequency here as well?
> > > > > >
> > > > > > In principle this does belong to the endpoint as link frequencies are
> > > > > > specific to that, but I don't mind; there's just a single port anyway.
> > > > > >
> > > > >
> > > > > This is an optional property which we model as an array of <link speed,
> > > > > clock voltage> pairs. An example to have all link speeds up to 390MHz
> > > > > use the value 4 for current driver. If one wants to select different
> > > > > voltage for different link, they could do so as well.
> > > >
> > > > If you think you'd need that, then you need to put this to the endpoint.
> > >
> > > The mipi-clock-voltage property is not a property of the endpoint. The
> > > link frequency there does not set the link frequency - it only
> > > specifies which link frequency the given voltage should be used for.
> >
> > If it is not bound to the endpoint --- that specifies the properties of the
> > link --- then you could just put it to the driver, couldn't you?
> >
> > But earlier on, we established that this is specific to a given board, or a
> > given link. Endpoints are used to describe connections that a device may
> > have to other devices. Therefore their properties are different.
> >
> > The graph bindings allow for several endpoints on a single physical port,
> > some may be disabled in that case. This is admittedly mostly done with
> > CSI-2 receivers.
> >
> > Either way, putting this property to the device may affect how DT source is
> > arranged for boards that have this device. Any similar configuration on
> > other devices with multiple transmitters or receivers would be in endpoints
> > anyway --- because they have to, so I don't see why bindings for this
> > device should be different.
> >
> > If you put it to endpoint, the driver also doesn't need to match the link
> > frequencies in kHz and Hz between the two properties, but just use the same
> > index for both.
>
> I agree in general, but I think in this case this is a device-specific
> property and the sensor only has 1 CSI-2 interface, so maybe it
> doesn't really matter?
>
> Specifying frequency, voltage pairs is a common practice in other
> bindings, such as the OPP bindings, by the way.
>
> That said, if you insist, I'm fine with this being moved to the
> endpoint node too.

Is the conclusion that it needs to be moved?

Best regards,
Tomasz
  

Hi Tomasz, Dongchun,

On Mon, Oct 19, 2020 at 09:20:15PM +0200, Tomasz Figa wrote:
> On Mon, Oct 19, 2020 at 10:28 AM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > On Wed, Oct 14, 2020 at 01:48:00PM +0200, Tomasz Figa wrote:
> > > On Wed, Oct 14, 2020 at 10:31 AM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > On Wed, Oct 14, 2020 at 10:10:18AM +0800, Dongchun Zhu wrote:
> > > > > Hello Sakari,
> > > > >
> > > > > Thanks for your timely review.
> > > > >
> > > > > On Tue, 2020-10-13 at 19:19 +0300, Sakari Ailus wrote:
> > > > > > Hi Dongchun,
> > > > > >
> > > > > > On Tue, Oct 13, 2020 at 09:05:02PM +0800, Dongchun Zhu wrote:
> > > > > > > Add YAML device tree binding for OV02A10 CMOS image sensor, and the
> > > > > > > relevant MAINTAINERS entries.
> > > > > > >
> > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > > > > > ---
> > > > > > >  .../bindings/media/i2c/ovti,ov02a10.yaml           | 162 +++++++++++++++++++++
> > > > > > >  MAINTAINERS                                        |   7 +
> > > > > > >  2 files changed, 169 insertions(+)
> > > > > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > > > >
> > > > >
> > > > > [snip]...
> > > > >
> > > > > > > +  ovti,mipi-clock-voltage:
> > > > > > > +    description:
> > > > > > > +      An array of 2-tuples items, and each item consists of link frequency and
> > > > > > > +      MIPI clock voltage unit like <freq-kHz volt-unit>. Clock voltage unit is
> > > > > > > +      dependent upon link speed, indicating MIPI transmission speed select that
> > > > > > > +      controls D-PHY timing setting by adjusting MIPI clock voltage to improve
> > > > > > > +      the clock driver capability.
> > > > > > > +    $ref: "/schemas/types.yaml#/definitions/uint32-array"
> > > > > > > +    minItems: 2
> > > > > > > +    default: [390000, 4]
> > > > > >
> > > > > > Why do you have the link frequency here as well?
> > > > > >
> > > > > > In principle this does belong to the endpoint as link frequencies are
> > > > > > specific to that, but I don't mind; there's just a single port anyway.
> > > > > >
> > > > >
> > > > > This is an optional property which we model as an array of <link speed,
> > > > > clock voltage> pairs. An example to have all link speeds up to 390MHz
> > > > > use the value 4 for current driver. If one wants to select different
> > > > > voltage for different link, they could do so as well.
> > > >
> > > > If you think you'd need that, then you need to put this to the endpoint.
> > >
> > > The mipi-clock-voltage property is not a property of the endpoint. The
> > > link frequency there does not set the link frequency - it only
> > > specifies which link frequency the given voltage should be used for.
> >
> > If it is not bound to the endpoint --- that specifies the properties of the
> > link --- then you could just put it to the driver, couldn't you?
> >
> > But earlier on, we established that this is specific to a given board, or a
> > given link. Endpoints are used to describe connections that a device may
> > have to other devices. Therefore their properties are different.
> >
> > The graph bindings allow for several endpoints on a single physical port,
> > some may be disabled in that case. This is admittedly mostly done with
> > CSI-2 receivers.
> >
> > Either way, putting this property to the device may affect how DT source is
> > arranged for boards that have this device. Any similar configuration on
> > other devices with multiple transmitters or receivers would be in endpoints
> > anyway --- because they have to, so I don't see why bindings for this
> > device should be different.
> >
> > If you put it to endpoint, the driver also doesn't need to match the link
> > frequencies in kHz and Hz between the two properties, but just use the same
> > index for both.
> 
> I agree in general, but I think in this case this is a device-specific
> property and the sensor only has 1 CSI-2 interface, so maybe it
> doesn't really matter?
> 
> Specifying frequency, voltage pairs is a common practice in other
> bindings, such as the OPP bindings, by the way.
> 
> That said, if you insist, I'm fine with this being moved to the
> endpoint node too.

Please, unless there are sound technical reasons to do otherwise.
  

Hi Sakari,

On Fri, 2020-10-30 at 18:27 +0200, Sakari Ailus wrote:
> Hi Tomasz, Dongchun,
> 
> On Mon, Oct 19, 2020 at 09:20:15PM +0200, Tomasz Figa wrote:
> > On Mon, Oct 19, 2020 at 10:28 AM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > On Wed, Oct 14, 2020 at 01:48:00PM +0200, Tomasz Figa wrote:
> > > > On Wed, Oct 14, 2020 at 10:31 AM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > On Wed, Oct 14, 2020 at 10:10:18AM +0800, Dongchun Zhu wrote:
> > > > > > Hello Sakari,
> > > > > >
> > > > > > Thanks for your timely review.
> > > > > >
> > > > > > On Tue, 2020-10-13 at 19:19 +0300, Sakari Ailus wrote:
> > > > > > > Hi Dongchun,
> > > > > > >
> > > > > > > On Tue, Oct 13, 2020 at 09:05:02PM +0800, Dongchun Zhu wrote:
> > > > > > > > Add YAML device tree binding for OV02A10 CMOS image sensor, and the
> > > > > > > > relevant MAINTAINERS entries.
> > > > > > > >
> > > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > > > > > > ---
> > > > > > > >  .../bindings/media/i2c/ovti,ov02a10.yaml           | 162 +++++++++++++++++++++
> > > > > > > >  MAINTAINERS                                        |   7 +
> > > > > > > >  2 files changed, 169 insertions(+)
> > > > > > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > > > > >
> > > > > >
> > > > > > [snip]...
> > > > > >
> > > > > > > > +  ovti,mipi-clock-voltage:
> > > > > > > > +    description:
> > > > > > > > +      An array of 2-tuples items, and each item consists of link frequency and
> > > > > > > > +      MIPI clock voltage unit like <freq-kHz volt-unit>. Clock voltage unit is
> > > > > > > > +      dependent upon link speed, indicating MIPI transmission speed select that
> > > > > > > > +      controls D-PHY timing setting by adjusting MIPI clock voltage to improve
> > > > > > > > +      the clock driver capability.
> > > > > > > > +    $ref: "/schemas/types.yaml#/definitions/uint32-array"
> > > > > > > > +    minItems: 2
> > > > > > > > +    default: [390000, 4]
> > > > > > >
> > > > > > > Why do you have the link frequency here as well?
> > > > > > >
> > > > > > > In principle this does belong to the endpoint as link frequencies are
> > > > > > > specific to that, but I don't mind; there's just a single port anyway.
> > > > > > >
> > > > > >
> > > > > > This is an optional property which we model as an array of <link speed,
> > > > > > clock voltage> pairs. An example to have all link speeds up to 390MHz
> > > > > > use the value 4 for current driver. If one wants to select different
> > > > > > voltage for different link, they could do so as well.
> > > > >
> > > > > If you think you'd need that, then you need to put this to the endpoint.
> > > >
> > > > The mipi-clock-voltage property is not a property of the endpoint. The
> > > > link frequency there does not set the link frequency - it only
> > > > specifies which link frequency the given voltage should be used for.
> > >
> > > If it is not bound to the endpoint --- that specifies the properties of the
> > > link --- then you could just put it to the driver, couldn't you?
> > >
> > > But earlier on, we established that this is specific to a given board, or a
> > > given link. Endpoints are used to describe connections that a device may
> > > have to other devices. Therefore their properties are different.
> > >
> > > The graph bindings allow for several endpoints on a single physical port,
> > > some may be disabled in that case. This is admittedly mostly done with
> > > CSI-2 receivers.
> > >
> > > Either way, putting this property to the device may affect how DT source is
> > > arranged for boards that have this device. Any similar configuration on
> > > other devices with multiple transmitters or receivers would be in endpoints
> > > anyway --- because they have to, so I don't see why bindings for this
> > > device should be different.
> > >
> > > If you put it to endpoint, the driver also doesn't need to match the link
> > > frequencies in kHz and Hz between the two properties, but just use the same
> > > index for both.
> > 
> > I agree in general, but I think in this case this is a device-specific
> > property and the sensor only has 1 CSI-2 interface, so maybe it
> > doesn't really matter?
> > 
> > Specifying frequency, voltage pairs is a common practice in other
> > bindings, such as the OPP bindings, by the way.
> > 
> > That said, if you insist, I'm fine with this being moved to the
> > endpoint node too.
> 
> Please, unless there are sound technical reasons to do otherwise.
> 

Sorry for the late reply.

In fact I'm still wonder whether 'MIPI clock voltage unit' here is a
very common or meaningful property that needs to be moved to the
endpoint data structure in v4l2 framework. Just because it relates to
link frequency?

In another words, if we set clock voltage as a separate property, as
patchset 13 or earlier version presents, is there no need to move the
position?