[1/3] dt-bindings: media: i2c: Add OV8865 bindings documentation

This introduces YAML bindings documentation for the OV8865
image sensor.

Co-developed-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 .../bindings/media/i2c/ovti,ov8865.yaml       | 124 ++++++++++++++++++
 1 file changed, 124 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml

On Fri, Oct 23, 2020 at 07:54:04PM +0200, Paul Kocialkowski wrote:
> This introduces YAML bindings documentation for the OV8865
> image sensor.
> 
> Co-developed-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  .../bindings/media/i2c/ovti,ov8865.yaml       | 124 ++++++++++++++++++
>  1 file changed, 124 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> new file mode 100644
> index 000000000000..807f1a94afae
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> @@ -0,0 +1,124 @@
> +# SPDX-License-Identifier: GPL-2.0

Dual license please. With that,

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

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov8865.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: OmniVision OV8865 Image Sensor Device Tree Bindings
> +
> +maintainers:
> +  - Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> +
> +properties:
> +  compatible:
> +    const: ovti,ov8865
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: EXTCLK Clock
> +
> +  clock-names:
> +    items:
> +      - const: extclk
> +
> +  dvdd-supply:
> +    description: Digital Domain Power Supply
> +
> +  avdd-supply:
> +    description: Analog Domain Power Supply (internal AVDD is used if missing)
> +
> +  dovdd-supply:
> +    description: I/O Domain Power Supply
> +
> +  powerdown-gpios:
> +    maxItems: 1
> +    description: Power Down Pin GPIO Control (active low)
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: Reset Pin GPIO Control (active low)
> +
> +  port:
> +    type: object
> +    description: Input port, connect to a MIPI CSI-2 receiver
> +
> +    properties:
> +      endpoint:
> +        type: object
> +
> +        properties:
> +          remote-endpoint: true
> +
> +          bus-type:
> +            const: 4
> +
> +          clock-lanes:
> +            maxItems: 1
> +
> +          data-lanes:
> +            minItems: 1
> +            maxItems: 4
> +
> +        required:
> +          - bus-type
> +          - data-lanes
> +          - remote-endpoint
> +
> +        additionalProperties: false
> +
> +    required:
> +      - endpoint
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - dvdd-supply
> +  - dovdd-supply
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/sun8i-a83t-ccu.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c2 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ov8865: camera@36 {
> +            compatible = "ovti,ov8865";
> +            reg = <0x36>;
> +
> +            pinctrl-names = "default";
> +            pinctrl-0 = <&csi_mclk_pin>;
> +
> +            clocks = <&ccu CLK_CSI_MCLK>;
> +            clock-names = "extclk";
> +
> +            avdd-supply = <&reg_ov8865_avdd>;
> +            dovdd-supply = <&reg_ov8865_dovdd>;
> +            dvdd-supply = <&reg_ov8865_dvdd>;
> +
> +            powerdown-gpios = <&pio 4 17 GPIO_ACTIVE_LOW>; /* PE17 */
> +            reset-gpios = <&pio 4 16 GPIO_ACTIVE_LOW>; /* PE16 */
> +
> +            port {
> +                ov8865_out_mipi_csi2: endpoint {
> +                    bus-type = <4>; /* MIPI CSI-2 D-PHY */
> +                    clock-lanes = <0>;
> +                    data-lanes = <1 2 3 4>;
> +
> +                    remote-endpoint = <&mipi_csi2_in_ov8865>;
> +                };
> +            };
> +        };
> +    };
> +
> +...
> -- 
> 2.28.0
>

Hi Paul,

On Fri, Oct 23, 2020 at 07:54:04PM +0200, Paul Kocialkowski wrote:
> This introduces YAML bindings documentation for the OV8865
> image sensor.
> 
> Co-developed-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  .../bindings/media/i2c/ovti,ov8865.yaml       | 124 ++++++++++++++++++
>  1 file changed, 124 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> new file mode 100644
> index 000000000000..807f1a94afae
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> @@ -0,0 +1,124 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov8865.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: OmniVision OV8865 Image Sensor Device Tree Bindings
> +
> +maintainers:
> +  - Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> +
> +properties:
> +  compatible:
> +    const: ovti,ov8865
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: EXTCLK Clock
> +
> +  clock-names:
> +    items:
> +      - const: extclk

Is this needed with a single clock?

And... shouldn't this also come with assigned-clock-rates etc., to set the
clock frequency?

> +
> +  dvdd-supply:
> +    description: Digital Domain Power Supply
> +
> +  avdd-supply:
> +    description: Analog Domain Power Supply (internal AVDD is used if missing)
> +
> +  dovdd-supply:
> +    description: I/O Domain Power Supply
> +
> +  powerdown-gpios:
> +    maxItems: 1
> +    description: Power Down Pin GPIO Control (active low)
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: Reset Pin GPIO Control (active low)
> +
> +  port:
> +    type: object
> +    description: Input port, connect to a MIPI CSI-2 receiver
> +
> +    properties:
> +      endpoint:
> +        type: object
> +
> +        properties:
> +          remote-endpoint: true
> +
> +          bus-type:
> +            const: 4
> +
> +          clock-lanes:
> +            maxItems: 1

I believe you can drop clock-lanes and bus-type; these are both constants.

I presume the device does not support lane remapping?

Could you also add link-frequencies, to list which frequencies are known to
be good?

Same comments on the other OV sensor bindings.

> +
> +          data-lanes:
> +            minItems: 1
> +            maxItems: 4
> +
> +        required:
> +          - bus-type
> +          - data-lanes
> +          - remote-endpoint
> +
> +        additionalProperties: false
> +
> +    required:
> +      - endpoint
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - dvdd-supply
> +  - dovdd-supply
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/sun8i-a83t-ccu.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c2 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ov8865: camera@36 {
> +            compatible = "ovti,ov8865";
> +            reg = <0x36>;
> +
> +            pinctrl-names = "default";
> +            pinctrl-0 = <&csi_mclk_pin>;
> +
> +            clocks = <&ccu CLK_CSI_MCLK>;
> +            clock-names = "extclk";
> +
> +            avdd-supply = <&reg_ov8865_avdd>;
> +            dovdd-supply = <&reg_ov8865_dovdd>;
> +            dvdd-supply = <&reg_ov8865_dvdd>;
> +
> +            powerdown-gpios = <&pio 4 17 GPIO_ACTIVE_LOW>; /* PE17 */
> +            reset-gpios = <&pio 4 16 GPIO_ACTIVE_LOW>; /* PE16 */
> +
> +            port {
> +                ov8865_out_mipi_csi2: endpoint {
> +                    bus-type = <4>; /* MIPI CSI-2 D-PHY */
> +                    clock-lanes = <0>;
> +                    data-lanes = <1 2 3 4>;
> +
> +                    remote-endpoint = <&mipi_csi2_in_ov8865>;
> +                };
> +            };
> +        };
> +    };
> +
> +...

Hi Sakari and thanks for the review!

On Tue 03 Nov 20, 01:24, Sakari Ailus wrote:
> On Fri, Oct 23, 2020 at 07:54:04PM +0200, Paul Kocialkowski wrote:
> > This introduces YAML bindings documentation for the OV8865
> > image sensor.
> > 
> > Co-developed-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  .../bindings/media/i2c/ovti,ov8865.yaml       | 124 ++++++++++++++++++
> >  1 file changed, 124 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> > new file mode 100644
> > index 000000000000..807f1a94afae
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> > @@ -0,0 +1,124 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov8865.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: OmniVision OV8865 Image Sensor Device Tree Bindings
> > +
> > +maintainers:
> > +  - Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: ovti,ov8865
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: EXTCLK Clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: extclk
> 
> Is this needed with a single clock?

Yes I think so: we grab the clock with devm_clk_get which takes a name string
that matches the clock-names property.

> And... shouldn't this also come with assigned-clock-rates etc., to set the
> clock frequency?

I'm a bit confused why we would need to do that in the device-tree rather than
setting the clock rate with clk_set_rate in the driver, like any other driver
does. I think this was discussed before (on the initial ov8865 series) and the
conclusion was that there is no particular reason for media i2c drivers to
behave differently. So I believe this is the correct approach.

> > +
> > +  dvdd-supply:
> > +    description: Digital Domain Power Supply
> > +
> > +  avdd-supply:
> > +    description: Analog Domain Power Supply (internal AVDD is used if missing)
> > +
> > +  dovdd-supply:
> > +    description: I/O Domain Power Supply
> > +
> > +  powerdown-gpios:
> > +    maxItems: 1
> > +    description: Power Down Pin GPIO Control (active low)
> > +
> > +  reset-gpios:
> > +    maxItems: 1
> > +    description: Reset Pin GPIO Control (active low)
> > +
> > +  port:
> > +    type: object
> > +    description: Input port, connect to a MIPI CSI-2 receiver
> > +
> > +    properties:
> > +      endpoint:
> > +        type: object
> > +
> > +        properties:
> > +          remote-endpoint: true
> > +
> > +          bus-type:
> > +            const: 4
> > +
> > +          clock-lanes:
> > +            maxItems: 1
> 
> I believe you can drop clock-lanes and bus-type; these are both constants.

I don't understand why bus-type should be dropped because it is constant:
if bus-type is set to something else, the driver will definitely not probe
since we're requesting V4L2_MBUS_CSI2_DPHY for v4l2_fwnode_endpoint_parse.
So I think it's quite important for the bindings to reflect this.

> I presume the device does not support lane remapping?

That's correct so this is indeed not something we can configure.
But shouldn't we instead specift clock-lanes = <0> as a const rather than
getting rid of it?

> Could you also add link-frequencies, to list which frequencies are known to
> be good?

Ah right, I had missed it. I'm a bit unsure about what I should do with the
information from the driver though: should I refuse to use link frequencies that
are not in the list?

Cheers,

Paul

> Same comments on the other OV sensor bindings.
> 
> > +
> > +          data-lanes:
> > +            minItems: 1
> > +            maxItems: 4
> > +
> > +        required:
> > +          - bus-type
> > +          - data-lanes
> > +          - remote-endpoint
> > +
> > +        additionalProperties: false
> > +
> > +    required:
> > +      - endpoint
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - dvdd-supply
> > +  - dovdd-supply
> > +  - port
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/sun8i-a83t-ccu.h>
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    i2c2 {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        ov8865: camera@36 {
> > +            compatible = "ovti,ov8865";
> > +            reg = <0x36>;
> > +
> > +            pinctrl-names = "default";
> > +            pinctrl-0 = <&csi_mclk_pin>;
> > +
> > +            clocks = <&ccu CLK_CSI_MCLK>;
> > +            clock-names = "extclk";
> > +
> > +            avdd-supply = <&reg_ov8865_avdd>;
> > +            dovdd-supply = <&reg_ov8865_dovdd>;
> > +            dvdd-supply = <&reg_ov8865_dvdd>;
> > +
> > +            powerdown-gpios = <&pio 4 17 GPIO_ACTIVE_LOW>; /* PE17 */
> > +            reset-gpios = <&pio 4 16 GPIO_ACTIVE_LOW>; /* PE16 */
> > +
> > +            port {
> > +                ov8865_out_mipi_csi2: endpoint {
> > +                    bus-type = <4>; /* MIPI CSI-2 D-PHY */
> > +                    clock-lanes = <0>;
> > +                    data-lanes = <1 2 3 4>;
> > +
> > +                    remote-endpoint = <&mipi_csi2_in_ov8865>;
> > +                };
> > +            };
> > +        };
> > +    };
> > +
> > +...
> 
> -- 
> Regards,
> 
> Sakari Ailus

Hi Paul,

On Wed, Nov 04, 2020 at 11:26:43AM +0100, Paul Kocialkowski wrote:
> Hi Sakari and thanks for the review!
> 
> On Tue 03 Nov 20, 01:24, Sakari Ailus wrote:
> > On Fri, Oct 23, 2020 at 07:54:04PM +0200, Paul Kocialkowski wrote:
> > > This introduces YAML bindings documentation for the OV8865
> > > image sensor.
> > > 
> > > Co-developed-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > ---
> > >  .../bindings/media/i2c/ovti,ov8865.yaml       | 124 ++++++++++++++++++
> > >  1 file changed, 124 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> > > new file mode 100644
> > > index 000000000000..807f1a94afae
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> > > @@ -0,0 +1,124 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov8865.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: OmniVision OV8865 Image Sensor Device Tree Bindings
> > > +
> > > +maintainers:
> > > +  - Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: ovti,ov8865
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    items:
> > > +      - description: EXTCLK Clock
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: extclk
> > 
> > Is this needed with a single clock?
> 
> Yes I think so: we grab the clock with devm_clk_get which takes a name string
> that matches the clock-names property.

That argument may be NULL.

> 
> > And... shouldn't this also come with assigned-clock-rates etc., to set the
> > clock frequency?
> 
> I'm a bit confused why we would need to do that in the device-tree rather than
> setting the clock rate with clk_set_rate in the driver, like any other driver
> does. I think this was discussed before (on the initial ov8865 series) and the
> conclusion was that there is no particular reason for media i2c drivers to
> behave differently. So I believe this is the correct approach.

I'm not exactly sure about that conclusion.

You can use clk_set_rate() if you get the frequency from DT, but we
recently did conclude that camera sensor drivers can expect to get the
frequency indicated by assigned-clock-rate property.

In other words, the driver may not be specific to a particular board and
SoC you have.

Please also read Documentation/driver-api/media/camera-sensor.rst .

> 
> > > +
> > > +  dvdd-supply:
> > > +    description: Digital Domain Power Supply
> > > +
> > > +  avdd-supply:
> > > +    description: Analog Domain Power Supply (internal AVDD is used if missing)
> > > +
> > > +  dovdd-supply:
> > > +    description: I/O Domain Power Supply
> > > +
> > > +  powerdown-gpios:
> > > +    maxItems: 1
> > > +    description: Power Down Pin GPIO Control (active low)
> > > +
> > > +  reset-gpios:
> > > +    maxItems: 1
> > > +    description: Reset Pin GPIO Control (active low)
> > > +
> > > +  port:
> > > +    type: object
> > > +    description: Input port, connect to a MIPI CSI-2 receiver
> > > +
> > > +    properties:
> > > +      endpoint:
> > > +        type: object
> > > +
> > > +        properties:
> > > +          remote-endpoint: true
> > > +
> > > +          bus-type:
> > > +            const: 4
> > > +
> > > +          clock-lanes:
> > > +            maxItems: 1
> > 
> > I believe you can drop clock-lanes and bus-type; these are both constants.
> 
> I don't understand why bus-type should be dropped because it is constant:
> if bus-type is set to something else, the driver will definitely not probe
> since we're requesting V4L2_MBUS_CSI2_DPHY for v4l2_fwnode_endpoint_parse.
> So I think it's quite important for the bindings to reflect this.

This driver is for a particular device that has MIPI CSI-2 on D-PHY as the
data bus. You can assume that in the driver.

> 
> > I presume the device does not support lane remapping?
> 
> That's correct so this is indeed not something we can configure.
> But shouldn't we instead specift clock-lanes = <0> as a const rather than
> getting rid of it?

Why would you put redundant information to DT?

> 
> > Could you also add link-frequencies, to list which frequencies are known to
> > be good?
> 
> Ah right, I had missed it. I'm a bit unsure about what I should do with the
> information from the driver though: should I refuse to use link frequencies that
> are not in the list?

Yes, please.

Hi Sakari,

On Thu 05 Nov 20, 10:19, Sakari Ailus wrote:
> Hi Paul,
> 
> On Wed, Nov 04, 2020 at 11:26:43AM +0100, Paul Kocialkowski wrote:
> > Hi Sakari and thanks for the review!
> > 
> > On Tue 03 Nov 20, 01:24, Sakari Ailus wrote:
> > > On Fri, Oct 23, 2020 at 07:54:04PM +0200, Paul Kocialkowski wrote:
> > > > This introduces YAML bindings documentation for the OV8865
> > > > image sensor.
> > > > 
> > > > Co-developed-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > ---
> > > >  .../bindings/media/i2c/ovti,ov8865.yaml       | 124 ++++++++++++++++++
> > > >  1 file changed, 124 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> > > > new file mode 100644
> > > > index 000000000000..807f1a94afae
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> > > > @@ -0,0 +1,124 @@
> > > > +# SPDX-License-Identifier: GPL-2.0
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov8865.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: OmniVision OV8865 Image Sensor Device Tree Bindings
> > > > +
> > > > +maintainers:
> > > > +  - Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: ovti,ov8865
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  clocks:
> > > > +    items:
> > > > +      - description: EXTCLK Clock
> > > > +
> > > > +  clock-names:
> > > > +    items:
> > > > +      - const: extclk
> > > 
> > > Is this needed with a single clock?
> > 
> > Yes I think so: we grab the clock with devm_clk_get which takes a name string
> > that matches the clock-names property.
> 
> That argument may be NULL.

Understood, let's get rid of clock-names then. I see this is done in a few
drivers already, but many also give it a name with a single clock.

It would be nice if that was consistent across all drivers just so that the
expectation is clear (that the best way for that to happen is probably to
fix up a patch myself though).

> > > And... shouldn't this also come with assigned-clock-rates etc., to set the
> > > clock frequency?
> > 
> > I'm a bit confused why we would need to do that in the device-tree rather than
> > setting the clock rate with clk_set_rate in the driver, like any other driver
> > does. I think this was discussed before (on the initial ov8865 series) and the
> > conclusion was that there is no particular reason for media i2c drivers to
> > behave differently. So I believe this is the correct approach.
> 
> I'm not exactly sure about that conclusion.

I may have jumped too far here. It's not exactly clear to me what was the
conclusion from...
https://lore.kernel.org/linux-arm-kernel/20200401080705.j4goeqcqhoswhx4u@gilmour.lan/

> You can use clk_set_rate() if you get the frequency from DT, but we
> recently did conclude that camera sensor drivers can expect to get the
> frequency indicated by assigned-clock-rate property.

...but it looks like clock-frequency was preferred over assigned-clock-rates
and this is what the binding that was merged suggests. Is that correct?

I now understand that the clock frequency may depend on the system integration
for this special case so we have to specify it via dt.

> In other words, the driver may not be specific to a particular board and
> SoC you have.

Although this is sadly more than often the case, because handling a variable
clock rate in the driver is quite complex (and even more with static init tables
that include PLL configuration). And sadly my driver is no exception and
only supports 24 MHz input.

> Please also read Documentation/driver-api/media/camera-sensor.rst .

Thanks, I hadn't seen that document before. It's great that it exists!

Paul

> > > > +
> > > > +  dvdd-supply:
> > > > +    description: Digital Domain Power Supply
> > > > +
> > > > +  avdd-supply:
> > > > +    description: Analog Domain Power Supply (internal AVDD is used if missing)
> > > > +
> > > > +  dovdd-supply:
> > > > +    description: I/O Domain Power Supply
> > > > +
> > > > +  powerdown-gpios:
> > > > +    maxItems: 1
> > > > +    description: Power Down Pin GPIO Control (active low)
> > > > +
> > > > +  reset-gpios:
> > > > +    maxItems: 1
> > > > +    description: Reset Pin GPIO Control (active low)
> > > > +
> > > > +  port:
> > > > +    type: object
> > > > +    description: Input port, connect to a MIPI CSI-2 receiver
> > > > +
> > > > +    properties:
> > > > +      endpoint:
> > > > +        type: object
> > > > +
> > > > +        properties:
> > > > +          remote-endpoint: true
> > > > +
> > > > +          bus-type:
> > > > +            const: 4
> > > > +
> > > > +          clock-lanes:
> > > > +            maxItems: 1
> > > 
> > > I believe you can drop clock-lanes and bus-type; these are both constants.
> > 
> > I don't understand why bus-type should be dropped because it is constant:
> > if bus-type is set to something else, the driver will definitely not probe
> > since we're requesting V4L2_MBUS_CSI2_DPHY for v4l2_fwnode_endpoint_parse.
> > So I think it's quite important for the bindings to reflect this.
> 
> This driver is for a particular device that has MIPI CSI-2 on D-PHY as the
> data bus. You can assume that in the driver.
> 
> > 
> > > I presume the device does not support lane remapping?
> > 
> > That's correct so this is indeed not something we can configure.
> > But shouldn't we instead specift clock-lanes = <0> as a const rather than
> > getting rid of it?
> 
> Why would you put redundant information to DT?
> 
> > 
> > > Could you also add link-frequencies, to list which frequencies are known to
> > > be good?
> > 
> > Ah right, I had missed it. I'm a bit unsure about what I should do with the
> > information from the driver though: should I refuse to use link frequencies that
> > are not in the list?
> 
> Yes, please.
> 
> -- 
> Regards,
> 
> Sakari Ailus

Hi Paul,

On Thu, Nov 05, 2020 at 04:35:34PM +0100, Paul Kocialkowski wrote:
> Hi Sakari,
> 
> On Thu 05 Nov 20, 10:19, Sakari Ailus wrote:
> > Hi Paul,
> > 
> > On Wed, Nov 04, 2020 at 11:26:43AM +0100, Paul Kocialkowski wrote:
> > > Hi Sakari and thanks for the review!
> > > 
> > > On Tue 03 Nov 20, 01:24, Sakari Ailus wrote:
> > > > On Fri, Oct 23, 2020 at 07:54:04PM +0200, Paul Kocialkowski wrote:
> > > > > This introduces YAML bindings documentation for the OV8865
> > > > > image sensor.
> > > > > 
> > > > > Co-developed-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > > > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > ---
> > > > >  .../bindings/media/i2c/ovti,ov8865.yaml       | 124 ++++++++++++++++++
> > > > >  1 file changed, 124 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..807f1a94afae
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> > > > > @@ -0,0 +1,124 @@
> > > > > +# SPDX-License-Identifier: GPL-2.0
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov8865.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: OmniVision OV8865 Image Sensor Device Tree Bindings
> > > > > +
> > > > > +maintainers:
> > > > > +  - Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    const: ovti,ov8865
> > > > > +
> > > > > +  reg:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  clocks:
> > > > > +    items:
> > > > > +      - description: EXTCLK Clock
> > > > > +
> > > > > +  clock-names:
> > > > > +    items:
> > > > > +      - const: extclk
> > > > 
> > > > Is this needed with a single clock?
> > > 
> > > Yes I think so: we grab the clock with devm_clk_get which takes a name string
> > > that matches the clock-names property.
> > 
> > That argument may be NULL.
> 
> Understood, let's get rid of clock-names then. I see this is done in a few
> drivers already, but many also give it a name with a single clock.
> 
> It would be nice if that was consistent across all drivers just so that the
> expectation is clear (that the best way for that to happen is probably to
> fix up a patch myself though).

I guess somewhat different practices exist depending on the tree albeit
it's all DT bindings. It's also not wrong to have the name of the clock
there, no, but virtually all camera sensors consume a single clock, so you
may as well omit the information.

> 
> > > > And... shouldn't this also come with assigned-clock-rates etc., to set the
> > > > clock frequency?
> > > 
> > > I'm a bit confused why we would need to do that in the device-tree rather than
> > > setting the clock rate with clk_set_rate in the driver, like any other driver
> > > does. I think this was discussed before (on the initial ov8865 series) and the
> > > conclusion was that there is no particular reason for media i2c drivers to
> > > behave differently. So I believe this is the correct approach.
> > 
> > I'm not exactly sure about that conclusion.
> 
> I may have jumped too far here. It's not exactly clear to me what was the
> conclusion from...
> https://lore.kernel.org/linux-arm-kernel/20200401080705.j4goeqcqhoswhx4u@gilmour.lan/

Yes, there has been more discussion on the topic, most recently in this
thread:

<URL:https://lore.kernel.org/linux-arm-kernel/20201102150547.GY26150@paasikivi.fi.intel.com/>

I think this deserves to be added to camera-sensor.rst .

> 
> > You can use clk_set_rate() if you get the frequency from DT, but we
> > recently did conclude that camera sensor drivers can expect to get the
> > frequency indicated by assigned-clock-rate property.
> 
> ...but it looks like clock-frequency was preferred over assigned-clock-rates
> and this is what the binding that was merged suggests. Is that correct?

assigned-clock-rates is fine. The assumption is that the clock frequency
does not change from the value set through DT, and the driver gets that
exact frequency.

> 
> I now understand that the clock frequency may depend on the system integration
> for this special case so we have to specify it via dt.

Correct.

> 
> > In other words, the driver may not be specific to a particular board and
> > SoC you have.
> 
> Although this is sadly more than often the case, because handling a variable
> clock rate in the driver is quite complex (and even more with static init tables
> that include PLL configuration). And sadly my driver is no exception and
> only supports 24 MHz input.

That's fine. If someone needs other frequencies, they can always add
support for those in the driver.

> 
> > Please also read Documentation/driver-api/media/camera-sensor.rst .
> 
> Thanks, I hadn't seen that document before. It's great that it exists!

You're welcome!

This was indeed written to reduce the number of patch revisions needed ot
get a driver to upstream. :-)

Hi Sakari,

On Wed, Nov 11, 2020 at 03:18:57PM +0200, Sakari Ailus wrote:
> Hi Paul,
> 
> On Thu, Nov 05, 2020 at 04:35:34PM +0100, Paul Kocialkowski wrote:
> > Hi Sakari,
> > 
> > On Thu 05 Nov 20, 10:19, Sakari Ailus wrote:
> > > Hi Paul,
> > > 
> > > On Wed, Nov 04, 2020 at 11:26:43AM +0100, Paul Kocialkowski wrote:
> > > > Hi Sakari and thanks for the review!
> > > > 
> > > > On Tue 03 Nov 20, 01:24, Sakari Ailus wrote:
> > > > > On Fri, Oct 23, 2020 at 07:54:04PM +0200, Paul Kocialkowski wrote:
> > > > > > This introduces YAML bindings documentation for the OV8865
> > > > > > image sensor.
> > > > > > 
> > > > > > Co-developed-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > > > > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > ---
> > > > > >  .../bindings/media/i2c/ovti,ov8865.yaml       | 124 ++++++++++++++++++
> > > > > >  1 file changed, 124 insertions(+)
> > > > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> > > > > > new file mode 100644
> > > > > > index 000000000000..807f1a94afae
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> > > > > > @@ -0,0 +1,124 @@
> > > > > > +# SPDX-License-Identifier: GPL-2.0
> > > > > > +%YAML 1.2
> > > > > > +---
> > > > > > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov8865.yaml#
> > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > +
> > > > > > +title: OmniVision OV8865 Image Sensor Device Tree Bindings
> > > > > > +
> > > > > > +maintainers:
> > > > > > +  - Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > +
> > > > > > +properties:
> > > > > > +  compatible:
> > > > > > +    const: ovti,ov8865
> > > > > > +
> > > > > > +  reg:
> > > > > > +    maxItems: 1
> > > > > > +
> > > > > > +  clocks:
> > > > > > +    items:
> > > > > > +      - description: EXTCLK Clock
> > > > > > +
> > > > > > +  clock-names:
> > > > > > +    items:
> > > > > > +      - const: extclk
> > > > > 
> > > > > Is this needed with a single clock?
> > > > 
> > > > Yes I think so: we grab the clock with devm_clk_get which takes a name string
> > > > that matches the clock-names property.
> > > 
> > > That argument may be NULL.
> > 
> > Understood, let's get rid of clock-names then. I see this is done in a few
> > drivers already, but many also give it a name with a single clock.
> > 
> > It would be nice if that was consistent across all drivers just so that the
> > expectation is clear (that the best way for that to happen is probably to
> > fix up a patch myself though).
> 
> I guess somewhat different practices exist depending on the tree albeit
> it's all DT bindings. It's also not wrong to have the name of the clock
> there, no, but virtually all camera sensors consume a single clock, so you
> may as well omit the information.
> 
> > 
> > > > > And... shouldn't this also come with assigned-clock-rates etc., to set the
> > > > > clock frequency?
> > > > 
> > > > I'm a bit confused why we would need to do that in the device-tree rather than
> > > > setting the clock rate with clk_set_rate in the driver, like any other driver
> > > > does. I think this was discussed before (on the initial ov8865 series) and the
> > > > conclusion was that there is no particular reason for media i2c drivers to
> > > > behave differently. So I believe this is the correct approach.
> > > 
> > > I'm not exactly sure about that conclusion.
> > 
> > I may have jumped too far here. It's not exactly clear to me what was the
> > conclusion from...
> > https://lore.kernel.org/linux-arm-kernel/20200401080705.j4goeqcqhoswhx4u@gilmour.lan/
> 
> Yes, there has been more discussion on the topic, most recently in this
> thread:
> 
> <URL:https://lore.kernel.org/linux-arm-kernel/20201102150547.GY26150@paasikivi.fi.intel.com/>
> 
> I think this deserves to be added to camera-sensor.rst .

It's not really a discussion though :)

We had back in that thread some issues with assigned-clock-rates that
don't get addressed at all.

> > > You can use clk_set_rate() if you get the frequency from DT, but we
> > > recently did conclude that camera sensor drivers can expect to get the
> > > frequency indicated by assigned-clock-rate property.
> > 
> > ...but it looks like clock-frequency was preferred over assigned-clock-rates
> > and this is what the binding that was merged suggests. Is that correct?
> 
> assigned-clock-rates is fine. The assumption is that the clock frequency
> does not change from the value set through DT, and the driver gets that
> exact frequency.

That's two fairly big assumptions though. A clock driver is free to
round the clock to whatever rate it wants, and assigned-clock-rates
doesn't provide any guarantee on this, nor does it let the potential
user know about it.

It might work for one-off cases, but there's no guarantee that it will
in the future since this is very much dependent on the clock setup,
driver and the other devices in the system (and to some extent the
configuration as well).

And since we only rely on it, we can't fix it properly either if it ever
occurs.

And then, semantically, this assigned-clock-rates isn't about what the
devices support but what the driver supports. The clock tree of
omnivision sensors (at least, I can't comment on the imx218 linked
above) allows for a very flexible input clock, it's only the driver that
requires it because most of its configuration relies on it.

It's definitely fine for the driver to do so, but it really doesn't
belong in the DT.

I thought we had an agreement on this last time we discussed it, but I'm
not really sure what made you change your mind.

Maxime

Hi Maxime,

Sorry for the late reply.

On Fri, Nov 13, 2020 at 06:27:35PM +0100, Maxime Ripard wrote:
> Hi Sakari,
> 
> On Wed, Nov 11, 2020 at 03:18:57PM +0200, Sakari Ailus wrote:
> > Hi Paul,
> > 
> > On Thu, Nov 05, 2020 at 04:35:34PM +0100, Paul Kocialkowski wrote:
> > > Hi Sakari,
> > > 
> > > On Thu 05 Nov 20, 10:19, Sakari Ailus wrote:
> > > > Hi Paul,
> > > > 
> > > > On Wed, Nov 04, 2020 at 11:26:43AM +0100, Paul Kocialkowski wrote:
> > > > > Hi Sakari and thanks for the review!
> > > > > 
> > > > > On Tue 03 Nov 20, 01:24, Sakari Ailus wrote:
> > > > > > On Fri, Oct 23, 2020 at 07:54:04PM +0200, Paul Kocialkowski wrote:
> > > > > > > This introduces YAML bindings documentation for the OV8865
> > > > > > > image sensor.
> > > > > > > 
> > > > > > > Co-developed-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > > > > > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > > ---
> > > > > > >  .../bindings/media/i2c/ovti,ov8865.yaml       | 124 ++++++++++++++++++
> > > > > > >  1 file changed, 124 insertions(+)
> > > > > > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> > > > > > > 
> > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..807f1a94afae
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
> > > > > > > @@ -0,0 +1,124 @@
> > > > > > > +# SPDX-License-Identifier: GPL-2.0
> > > > > > > +%YAML 1.2
> > > > > > > +---
> > > > > > > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov8865.yaml#
> > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > +
> > > > > > > +title: OmniVision OV8865 Image Sensor Device Tree Bindings
> > > > > > > +
> > > > > > > +maintainers:
> > > > > > > +  - Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > > +
> > > > > > > +properties:
> > > > > > > +  compatible:
> > > > > > > +    const: ovti,ov8865
> > > > > > > +
> > > > > > > +  reg:
> > > > > > > +    maxItems: 1
> > > > > > > +
> > > > > > > +  clocks:
> > > > > > > +    items:
> > > > > > > +      - description: EXTCLK Clock
> > > > > > > +
> > > > > > > +  clock-names:
> > > > > > > +    items:
> > > > > > > +      - const: extclk
> > > > > > 
> > > > > > Is this needed with a single clock?
> > > > > 
> > > > > Yes I think so: we grab the clock with devm_clk_get which takes a name string
> > > > > that matches the clock-names property.
> > > > 
> > > > That argument may be NULL.
> > > 
> > > Understood, let's get rid of clock-names then. I see this is done in a few
> > > drivers already, but many also give it a name with a single clock.
> > > 
> > > It would be nice if that was consistent across all drivers just so that the
> > > expectation is clear (that the best way for that to happen is probably to
> > > fix up a patch myself though).
> > 
> > I guess somewhat different practices exist depending on the tree albeit
> > it's all DT bindings. It's also not wrong to have the name of the clock
> > there, no, but virtually all camera sensors consume a single clock, so you
> > may as well omit the information.
> > 
> > > 
> > > > > > And... shouldn't this also come with assigned-clock-rates etc., to set the
> > > > > > clock frequency?
> > > > > 
> > > > > I'm a bit confused why we would need to do that in the device-tree rather than
> > > > > setting the clock rate with clk_set_rate in the driver, like any other driver
> > > > > does. I think this was discussed before (on the initial ov8865 series) and the
> > > > > conclusion was that there is no particular reason for media i2c drivers to
> > > > > behave differently. So I believe this is the correct approach.
> > > > 
> > > > I'm not exactly sure about that conclusion.
> > > 
> > > I may have jumped too far here. It's not exactly clear to me what was the
> > > conclusion from...
> > > https://lore.kernel.org/linux-arm-kernel/20200401080705.j4goeqcqhoswhx4u@gilmour.lan/
> > 
> > Yes, there has been more discussion on the topic, most recently in this
> > thread:
> > 
> > <URL:https://lore.kernel.org/linux-arm-kernel/20201102150547.GY26150@paasikivi.fi.intel.com/>
> > 
> > I think this deserves to be added to camera-sensor.rst .
> 
> It's not really a discussion though :)
> 
> We had back in that thread some issues with assigned-clock-rates that
> don't get addressed at all.
> 
> > > > You can use clk_set_rate() if you get the frequency from DT, but we
> > > > recently did conclude that camera sensor drivers can expect to get the
> > > > frequency indicated by assigned-clock-rate property.
> > > 
> > > ...but it looks like clock-frequency was preferred over assigned-clock-rates
> > > and this is what the binding that was merged suggests. Is that correct?
> > 
> > assigned-clock-rates is fine. The assumption is that the clock frequency
> > does not change from the value set through DT, and the driver gets that
> > exact frequency.
> 
> That's two fairly big assumptions though. A clock driver is free to
> round the clock to whatever rate it wants, and assigned-clock-rates
> doesn't provide any guarantee on this, nor does it let the potential
> user know about it.
> 
> It might work for one-off cases, but there's no guarantee that it will
> in the future since this is very much dependent on the clock setup,
> driver and the other devices in the system (and to some extent the
> configuration as well).
> 
> And since we only rely on it, we can't fix it properly either if it ever
> occurs.
> 
> And then, semantically, this assigned-clock-rates isn't about what the
> devices support but what the driver supports. The clock tree of
> omnivision sensors (at least, I can't comment on the imx218 linked
> above) allows for a very flexible input clock, it's only the driver that
> requires it because most of its configuration relies on it.

There is a semantic difference, yes. The driver in this case assumes the
frequency is the one it is expected to use. In case the frequency is
different, the most likely outcome is that the image sensor is inoperable.
But again that is fairly easy to notice.

If someone prefers to fix this and make it reliable also in principle, that
would require changing the semantics a little as well as adding a function
to the clock framework to set the assigned frequency.

> 
> It's definitely fine for the driver to do so, but it really doesn't
> belong in the DT.
> 
> I thought we had an agreement on this last time we discussed it, but I'm
> not really sure what made you change your mind.

Most new sensor drivers come with assigned-clock-rates etc. properties and
I'll need to explain the same over and over again in review. Then using
"clock-frequency" property requires quite a few more lines of code in the
drivers.

I also recently discussed the matter with Rob H. and his opinion was
basically that we should be using assigned-clock-rates here instead.

I wonder how this works on other places where the frequency can be one of
several options, but the correct frequency is board specific.