LinuxLists.cc - [PATCH v5 0/5] ov5645: Deprecate usage of the clock-frequency

[permalink] [raw]

Subject: Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property

Hi Sakari,

On Mon, Apr 6, 2020 at 5:51 PM Sakari Ailus
<[email protected]> wrote:
>
> Hi Prabhakar,
>
> On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote:
> > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where
> > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to
> > 24MHz. So instead making clock-frequency as dt-property just let the
> > driver enforce the required clock frequency.
>
> Even if some current systems where the driver is used are using 24 MHz
> clock, that doesn't mean there wouldn't be systems using another frequency
> that the driver does not support right now.
>
> The driver really should not set the frequency unless it gets it from DT,
> but I think the preferred means is to use assigned-clock-rates instead, and
> not to involve the driver with setting the frequency.
>
> Otherwise we'll make it impossible to support other frequencies, at least
> without more or less random defaults.
>
Ouch! my previous version of patches switched the driver for using
assigned-clock-rates but I was asked to
not do so [1].

[1] https://patchwork.linuxtv.org/patch/62185/

Cheers,
--Prabhakar

> --
> Kind regards,
>
> Sakari Ailus

2020-04-06 17:43:41

[permalink] [raw]

Subject: Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property

Hi Prabhakar,

On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote:
> Modes in the driver are based on xvclk frequency fixed to 24MHz, but where
> as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to
> 24MHz. So instead making clock-frequency as dt-property just let the
> driver enforce the required clock frequency.

Even if some current systems where the driver is used are using 24 MHz
clock, that doesn't mean there wouldn't be systems using another frequency
that the driver does not support right now.

The driver really should not set the frequency unless it gets it from DT,
but I think the preferred means is to use assigned-clock-rates instead, and
not to involve the driver with setting the frequency.

Otherwise we'll make it impossible to support other frequencies, at least
without more or less random defaults.

--
Kind regards,

Sakari Ailus

2020-04-06 17:46:55

[permalink] [raw]

Subject: Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property

Hi Prabhakar,

Thank you for the patch.

On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote:
> Modes in the driver are based on xvclk frequency fixed to 24MHz, but where
> as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to
> 24MHz. So instead making clock-frequency as dt-property just let the
> driver enforce the required clock frequency.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> ---
> drivers/media/i2c/ov5645.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index a6c17d15d754..52848fff8a08 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -61,6 +61,8 @@
> #define OV5645_SDE_SAT_U 0x5583
> #define OV5645_SDE_SAT_V 0x5584
>
> +#define OV5645_XVCLK_FREQ 24000000
> +
> /* regulator supplies */
> static const char * const ov5645_supply_name[] = {
> "vdddo", /* Digital I/O (1.8V) supply */
> @@ -1094,25 +1096,19 @@ static int ov5645_probe(struct i2c_client *client)
> return PTR_ERR(ov5645->xclk);
> }
>
> - ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq);
> + ret = clk_set_rate(ov5645->xclk, OV5645_XVCLK_FREQ);
> if (ret) {
> - dev_err(dev, "could not get xclk frequency\n");
> + dev_err(dev, "could not set xclk frequency\n");
> return ret;
> }
> -

I think you can keep the blank line here.

Reviewed-by: Laurent Pinchart <[email protected]>

> /* external clock must be 24MHz, allow 1% tolerance */
> + xclk_freq = clk_get_rate(ov5645->xclk);
> if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> dev_err(dev, "external clock frequency %u is not supported\n",
> xclk_freq);
> return -EINVAL;
> }
>
> - ret = clk_set_rate(ov5645->xclk, xclk_freq);
> - if (ret) {
> - dev_err(dev, "could not set xclk frequency\n");
> - return ret;
> - }
> -
> for (i = 0; i < OV5645_NUM_SUPPLIES; i++)
> ov5645->supplies[i].supply = ov5645_supply_name[i];
>

--
Regards,

Laurent Pinchart

2020-04-06 17:47:20

[permalink] [raw]

Subject: Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property

Hi Sakari,

(CC'ing Maxime)

On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote:
> On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote:
> > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where
> > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to
> > 24MHz. So instead making clock-frequency as dt-property just let the
> > driver enforce the required clock frequency.
>
> Even if some current systems where the driver is used are using 24 MHz
> clock, that doesn't mean there wouldn't be systems using another frequency
> that the driver does not support right now.
>
> The driver really should not set the frequency unless it gets it from DT,
> but I think the preferred means is to use assigned-clock-rates instead, and
> not to involve the driver with setting the frequency.
>
> Otherwise we'll make it impossible to support other frequencies, at least
> without more or less random defaults.

We're running in circles here.

As the driver only supports 24MHz at the moment, the frequency should be
set by the driver, as it's a driver limitation. We can then work on
supporting additional frequencies, which will require DT to provide a
list of supported frequencies for the system, but that can be done on
top.

--
Regards,

Laurent Pinchart

2020-04-06 17:48:45

[permalink] [raw]

Subject: Re: [PATCH v5 5/5] media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema

Hi Prabhakar,

Thank you for the patch.

On Mon, Apr 06, 2020 at 05:42:41PM +0100, Lad Prabhakar wrote:
> Convert ov5645 bindings to json-schema.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> ---
> .../devicetree/bindings/media/i2c/ov5645.txt | 52 --------
> .../devicetree/bindings/media/i2c/ov5645.yaml | 126 ++++++++++++++++++
> 2 files changed, 126 insertions(+), 52 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt
> create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> deleted file mode 100644
> index a55bb728ea48..000000000000
> --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> +++ /dev/null
> @@ -1,52 +0,0 @@
> -* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
> -
> -The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
> -an active array size of 2592H x 1944V. It is programmable through a serial I2C
> -interface.
> -
> -Required Properties:
> -- compatible: Value should be "ovti,ov5645".
> -- clocks: Reference to the xclk clock.
> -- clock-names: Should be "xclk".
> -- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
> - to the hardware pin PWDNB which is physically active low.
> -- reset-gpios: Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW. This corresponds to
> - the hardware pin RESETB.
> -- vdddo-supply: Chip digital IO regulator.
> -- vdda-supply: Chip analog regulator.
> -- vddd-supply: Chip digital core regulator.
> -
> -The device node must contain one 'port' child node for its digital output
> -video port, in accordance with the video interface bindings defined in
> -Documentation/devicetree/bindings/media/video-interfaces.txt.
> -
> -Example:
> -
> - &i2c1 {
> - ...
> -
> - ov5645: ov5645@3c {
> - compatible = "ovti,ov5645";
> - reg = <0x3c>;
> -
> - enable-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
> - reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>;
> - pinctrl-names = "default";
> - pinctrl-0 = <&camera_rear_default>;
> -
> - clocks = <&clks 200>;
> - clock-names = "xclk";
> -
> - vdddo-supply = <&camera_dovdd_1v8>;
> - vdda-supply = <&camera_avdd_2v8>;
> - vddd-supply = <&camera_dvdd_1v2>;
> -
> - port {
> - ov5645_ep: endpoint {
> - clock-lanes = <1>;
> - data-lanes = <0 2>;
> - remote-endpoint = <&csi0_ep>;
> - };
> - };
> - };
> - };
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> new file mode 100644
> index 000000000000..d5cdcf9a1c76
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> @@ -0,0 +1,126 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ov5645.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Omnivision 1/4-Inch 5MP CMOS Digital Image Sensor
> +
> +maintainers:
> + - Sakari Ailus <[email protected]>
> + - Lad Prabhakar <[email protected]>
> +
> +description: |-
> + The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
> + an active array size of 2592H x 1944V. It is programmable through a serial I2C
> + interface.
> +
> +properties:
> + compatible:
> + const: ovti,ov5645
> +
> + reg:
> + description: I2C device address
> + maxItems: 1
> +
> + clocks:
> + description: External clock frequency should range between 6MHz to 27MHz.
> + maxItems: 1
> +
> + clock-names:
> + items:
> + - const: xclk
> +
> + enable-gpios:
> + description: |-
> + Chip enable GPIO. This corresponds to the hardware pin PWDNB which is
> + physically active low.
> +
> + reset-gpios:
> + description: Chip reset GPIO. This corresponds to the hardware pin RESETB.
> +
> + vdddo-supply:
> + description: Chip digital IO regulator.
> +
> + vdda-supply:
> + description: Chip analog regulator.
> +
> + vddd-supply:
> + description: Chip digital core regulator.
> +
> + # See ../video-interfaces.txt for more details
> + port:
> + type: object
> + properties:
> + endpoint:
> + type: object
> +
> + properties:
> + data-lanes:

Don't you need

minItems: 1
maxItems: 2

here ?

> + description: |-
> + The sensor supports either one-lane, or two-lane operation.
> + For one-lane operation the property must be set to <1> and
> + for two-lane operation the property must be set to <1 2>.
> + items:
> + - const: 1
> + - const: 2
> +
> + clock-lanes:

Same here,

maxItems: 1

?

> + description:
> + should be set to <0> (clock lane on hardware lane 0).

I think you can drop the description, with the items below it's clear
that the value has to be <0>.

> + items:
> + - const: 0
> +
> + remote-endpoint: true

Should this be

remote-endpoint:
$ref: /schemas/types.yaml#/definitions/phandle

> +
> + required:
> + - data-lanes
> + - clock-lanes
> + - remote-endpoint
> +
> + additionalProperties: false
> +
> + additionalProperties: false
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - clock-names
> + - enable-gpios
> + - reset-gpios
> + - vdddo-supply
> + - vdda-supply
> + - vddd-supply
> + - port
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ov5645: sensor@3c {
> + compatible = "ovti,ov5645";
> + reg = <0x3c>;
> + clocks = <&ov5645_cl>;
> + clock-names = "xclk";
> + enable-gpios = <&gpio1 6 /* GPIO_ACTIVE_HIGH */>;
> + reset-gpios = <&gpio5 20 /* GPIO_ACTIVE_LOW */>;
> + vdddo-supply = <&camera_dovdd_1v8>;
> + vdda-supply = <&camera_avdd_2v8>;
> + vddd-supply = <&camera_dvdd_1v2>;
> +
> + port {
> + ov5645_0: endpoint {
> + remote-endpoint = <&csi1_ep>;
> + clock-lanes = <0>;
> + data-lanes = <1 2>;
> + };
> + };
> + };
> + };
> +
> +...

--
Regards,

Laurent Pinchart

2020-04-07 06:23:58

[permalink] [raw]

Subject: Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property

Hi Laurent,

On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> (CC'ing Maxime)
>
> On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote:
> > On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote:
> > > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where
> > > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to
> > > 24MHz. So instead making clock-frequency as dt-property just let the
> > > driver enforce the required clock frequency.
> >
> > Even if some current systems where the driver is used are using 24 MHz
> > clock, that doesn't mean there wouldn't be systems using another frequency
> > that the driver does not support right now.
> >
> > The driver really should not set the frequency unless it gets it from DT,
> > but I think the preferred means is to use assigned-clock-rates instead, and
> > not to involve the driver with setting the frequency.
> >
> > Otherwise we'll make it impossible to support other frequencies, at least
> > without more or less random defaults.
>
> We're running in circles here.
>
> As the driver only supports 24MHz at the moment, the frequency should be
> set by the driver, as it's a driver limitation. We can then work on
> supporting additional frequencies, which will require DT to provide a
> list of supported frequencies for the system, but that can be done on
> top.

I guess it would be possible to use different external clock frequencies on
a sensor in a given system but that seems to be a bit far fetched, to the
extent I've never seen anyone doing that in practice.

Originally, the driver set the frequency based on the clock-frequency
property. If we're removing that but use a fixed frequency instead, then
how is that going to work going forward when someone adds support for other
frequencies in the driver and has a system requiring that, while there are
some other platforms relying on the driver setting a particular frequency?

Although, if you're saying that this driver only needs to work with DT that
comes with the kernel and you don't care about DT binary compatibility,
this would be fine.

--
Regards,

Sakari Ailus

2020-04-07 07:18:02

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property

Hi Prabhakar,

On Mon, Apr 6, 2020 at 6:43 PM Lad Prabhakar
<[email protected]> wrote:
> Modes in the driver are based on xvclk frequency fixed to 24MHz, but where
> as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to
> 24MHz. So instead making clock-frequency as dt-property just let the
> driver enforce the required clock frequency.
>
> Signed-off-by: Lad Prabhakar <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

However, still wondering about the "xvclk" name above and in the definition
below. Is this the naming from the datasheet?
The DT bindings nor the driver use the "xvclk" naming.

> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -61,6 +61,8 @@
> #define OV5645_SDE_SAT_U 0x5583
> #define OV5645_SDE_SAT_V 0x5584
>
> +#define OV5645_XVCLK_FREQ 24000000
> +

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2020-04-07 07:37:29

[permalink] [raw]

Subject: Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property

Hi Laurent,

Thank you for the review.

On Mon, Apr 6, 2020 at 6:34 PM Laurent Pinchart
<[email protected]> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote:
> > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where
> > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to
> > 24MHz. So instead making clock-frequency as dt-property just let the
> > driver enforce the required clock frequency.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > ---
> > drivers/media/i2c/ov5645.c | 14 +++++---------
> > 1 file changed, 5 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > index a6c17d15d754..52848fff8a08 100644
> > --- a/drivers/media/i2c/ov5645.c
> > +++ b/drivers/media/i2c/ov5645.c
> > @@ -61,6 +61,8 @@
> > #define OV5645_SDE_SAT_U 0x5583
> > #define OV5645_SDE_SAT_V 0x5584
> >
> > +#define OV5645_XVCLK_FREQ 24000000
> > +
> > /* regulator supplies */
> > static const char * const ov5645_supply_name[] = {
> > "vdddo", /* Digital I/O (1.8V) supply */
> > @@ -1094,25 +1096,19 @@ static int ov5645_probe(struct i2c_client *client)
> > return PTR_ERR(ov5645->xclk);
> > }
> >
> > - ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq);
> > + ret = clk_set_rate(ov5645->xclk, OV5645_XVCLK_FREQ);
> > if (ret) {
> > - dev_err(dev, "could not get xclk frequency\n");
> > + dev_err(dev, "could not set xclk frequency\n");
> > return ret;
> > }
> > -
>
> I think you can keep the blank line here.
>
Oops my bad will drop that in next version.

Cheers,
--Prabhakar

> Reviewed-by: Laurent Pinchart <[email protected]>
>
> > /* external clock must be 24MHz, allow 1% tolerance */
> > + xclk_freq = clk_get_rate(ov5645->xclk);
> > if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> > dev_err(dev, "external clock frequency %u is not supported\n",
> > xclk_freq);
> > return -EINVAL;
> > }
> >
> > - ret = clk_set_rate(ov5645->xclk, xclk_freq);
> > - if (ret) {
> > - dev_err(dev, "could not set xclk frequency\n");
> > - return ret;
> > - }
> > -
> > for (i = 0; i < OV5645_NUM_SUPPLIES; i++)
> > ov5645->supplies[i].supply = ov5645_supply_name[i];
> >
>
> --
> Regards,
>
> Laurent Pinchart

2020-04-07 07:41:23

[permalink] [raw]

Subject: Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property

Hi Geert,

Thank you for the review.

On Tue, Apr 7, 2020 at 8:17 AM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Prabhakar,
>
> On Mon, Apr 6, 2020 at 6:43 PM Lad Prabhakar
> <[email protected]> wrote:
> > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where
> > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to
> > 24MHz. So instead making clock-frequency as dt-property just let the
> > driver enforce the required clock frequency.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
>
> Reviewed-by: Geert Uytterhoeven <[email protected]>
>
> However, still wondering about the "xvclk" name above and in the definition
> below. Is this the naming from the datasheet?
> The DT bindings nor the driver use the "xvclk" naming.
>
xvclk naming is from the datasheet, although the 0v5645 datasheet on
publicly available I have referred [1]/[2].
If I am not wrong all the ov sensors have the same naming convention as xvclk.

[1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
[2] https://www.ovt.com/download/sensorpdf/126/OmniVision_OV5645.pdf

Cheers,
--Prabhakar

> > --- a/drivers/media/i2c/ov5645.c
> > +++ b/drivers/media/i2c/ov5645.c
> > @@ -61,6 +61,8 @@
> > #define OV5645_SDE_SAT_U 0x5583
> > #define OV5645_SDE_SAT_V 0x5584
> >
> > +#define OV5645_XVCLK_FREQ 24000000
> > +
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds

2020-04-07 07:47:28

[permalink] [raw]

Subject: Re: [PATCH v5 5/5] media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema

Hi Laurent,

Thank you for the review.

On Mon, Apr 6, 2020 at 6:43 PM Laurent Pinchart
<[email protected]> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Mon, Apr 06, 2020 at 05:42:41PM +0100, Lad Prabhakar wrote:
> > Convert ov5645 bindings to json-schema.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > ---
> > .../devicetree/bindings/media/i2c/ov5645.txt | 52 --------
> > .../devicetree/bindings/media/i2c/ov5645.yaml | 126 ++++++++++++++++++
> > 2 files changed, 126 insertions(+), 52 deletions(-)
> > delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > deleted file mode 100644
> > index a55bb728ea48..000000000000
> > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > +++ /dev/null
> > @@ -1,52 +0,0 @@
> > -* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
> > -
> > -The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
> > -an active array size of 2592H x 1944V. It is programmable through a serial I2C
> > -interface.
> > -
> > -Required Properties:
> > -- compatible: Value should be "ovti,ov5645".
> > -- clocks: Reference to the xclk clock.
> > -- clock-names: Should be "xclk".
> > -- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
> > - to the hardware pin PWDNB which is physically active low.
> > -- reset-gpios: Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW. This corresponds to
> > - the hardware pin RESETB.
> > -- vdddo-supply: Chip digital IO regulator.
> > -- vdda-supply: Chip analog regulator.
> > -- vddd-supply: Chip digital core regulator.
> > -
> > -The device node must contain one 'port' child node for its digital output
> > -video port, in accordance with the video interface bindings defined in
> > -Documentation/devicetree/bindings/media/video-interfaces.txt.
> > -
> > -Example:
> > -
> > - &i2c1 {
> > - ...
> > -
> > - ov5645: ov5645@3c {
> > - compatible = "ovti,ov5645";
> > - reg = <0x3c>;
> > -
> > - enable-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
> > - reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>;
> > - pinctrl-names = "default";
> > - pinctrl-0 = <&camera_rear_default>;
> > -
> > - clocks = <&clks 200>;
> > - clock-names = "xclk";
> > -
> > - vdddo-supply = <&camera_dovdd_1v8>;
> > - vdda-supply = <&camera_avdd_2v8>;
> > - vddd-supply = <&camera_dvdd_1v2>;
> > -
> > - port {
> > - ov5645_ep: endpoint {
> > - clock-lanes = <1>;
> > - data-lanes = <0 2>;
> > - remote-endpoint = <&csi0_ep>;
> > - };
> > - };
> > - };
> > - };
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> > new file mode 100644
> > index 000000000000..d5cdcf9a1c76
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> > @@ -0,0 +1,126 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/ov5645.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Omnivision 1/4-Inch 5MP CMOS Digital Image Sensor
> > +
> > +maintainers:
> > + - Sakari Ailus <[email protected]>
> > + - Lad Prabhakar <[email protected]>
> > +
> > +description: |-
> > + The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
> > + an active array size of 2592H x 1944V. It is programmable through a serial I2C
> > + interface.
> > +
> > +properties:
> > + compatible:
> > + const: ovti,ov5645
> > +
> > + reg:
> > + description: I2C device address
> > + maxItems: 1
> > +
> > + clocks:
> > + description: External clock frequency should range between 6MHz to 27MHz.
> > + maxItems: 1
> > +
> > + clock-names:
> > + items:
> > + - const: xclk
> > +
> > + enable-gpios:
> > + description: |-
> > + Chip enable GPIO. This corresponds to the hardware pin PWDNB which is
> > + physically active low.
> > +
> > + reset-gpios:
> > + description: Chip reset GPIO. This corresponds to the hardware pin RESETB.
> > +
> > + vdddo-supply:
> > + description: Chip digital IO regulator.
> > +
> > + vdda-supply:
> > + description: Chip analog regulator.
> > +
> > + vddd-supply:
> > + description: Chip digital core regulator.
> > +
> > + # See ../video-interfaces.txt for more details
> > + port:
> > + type: object
> > + properties:
> > + endpoint:
> > + type: object
> > +
> > + properties:
> > + data-lanes:
>
> Don't you need
>
> minItems: 1
> maxItems: 2
>
> here ?
>
Agreed will add that.

> > + description: |-
> > + The sensor supports either one-lane, or two-lane operation.
> > + For one-lane operation the property must be set to <1> and
> > + for two-lane operation the property must be set to <1 2>.
> > + items:
> > + - const: 1
> > + - const: 2
> > +
> > + clock-lanes:
>
> Same here,
>
> maxItems: 1
>
> ?
>
As above.

> > + description:
> > + should be set to <0> (clock lane on hardware lane 0).
>
> I think you can drop the description, with the items below it's clear
> that the value has to be <0>.
>
OK

> > + items:
> > + - const: 0
> > +
> > + remote-endpoint: true
>
> Should this be
>
> remote-endpoint:
> $ref: /schemas/types.yaml#/definitions/phandle
>
OK shall replace it.

Cheers,
--Prabhakar

> > +
> > + required:
> > + - data-lanes
> > + - clock-lanes
> > + - remote-endpoint
> > +
> > + additionalProperties: false
> > +
> > + additionalProperties: false
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - clocks
> > + - clock-names
> > + - enable-gpios
> > + - reset-gpios
> > + - vdddo-supply
> > + - vdda-supply
> > + - vddd-supply
> > + - port
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + ov5645: sensor@3c {
> > + compatible = "ovti,ov5645";
> > + reg = <0x3c>;
> > + clocks = <&ov5645_cl>;
> > + clock-names = "xclk";
> > + enable-gpios = <&gpio1 6 /* GPIO_ACTIVE_HIGH */>;
> > + reset-gpios = <&gpio5 20 /* GPIO_ACTIVE_LOW */>;
> > + vdddo-supply = <&camera_dovdd_1v8>;
> > + vdda-supply = <&camera_avdd_2v8>;
> > + vddd-supply = <&camera_dvdd_1v2>;
> > +
> > + port {
> > + ov5645_0: endpoint {
> > + remote-endpoint = <&csi1_ep>;
> > + clock-lanes = <0>;
> > + data-lanes = <1 2>;
> > + };
> > + };
> > + };
> > + };
> > +
> > +...
>
> --
> Regards,
>
> Laurent Pinchart

2020-04-07 12:19:36

[permalink] [raw]

Subject: Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property

Hi Prabhakar,

On Tue, Apr 07, 2020 at 08:40:06AM +0100, Lad, Prabhakar wrote:
> On Tue, Apr 7, 2020 at 8:17 AM Geert Uytterhoeven wrote:
> > On Mon, Apr 6, 2020 at 6:43 PM Lad Prabhakar wrote:
> > > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where
> > > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to
> > > 24MHz. So instead making clock-frequency as dt-property just let the
> > > driver enforce the required clock frequency.
> > >
> > > Signed-off-by: Lad Prabhakar <[email protected]>
> >
> > Reviewed-by: Geert Uytterhoeven <[email protected]>
> >
> > However, still wondering about the "xvclk" name above and in the definition
> > below. Is this the naming from the datasheet?
> > The DT bindings nor the driver use the "xvclk" naming.
> >
> xvclk naming is from the datasheet, although the 0v5645 datasheet on
> publicly available I have referred [1]/[2].
> If I am not wrong all the ov sensors have the same naming convention as xvclk.
>
> [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
> [2] https://www.ovt.com/download/sensorpdf/126/OmniVision_OV5645.pdf

The clock in DT should really have been named xvclk, but it's too late
to change that. We can follow one of two approaches, either naming
everything xclk, and naming everything but the DT property xvclk. Both
have pros and cons, feel free to pick your preferred option, but in any
case a comment to explain the issue would be useful.

> > > --- a/drivers/media/i2c/ov5645.c
> > > +++ b/drivers/media/i2c/ov5645.c
> > > @@ -61,6 +61,8 @@
> > > #define OV5645_SDE_SAT_U 0x5583
> > > #define OV5645_SDE_SAT_V 0x5584
> > >
> > > +#define OV5645_XVCLK_FREQ 24000000
> > > +

--
Regards,

Laurent Pinchart

2020-04-07 12:23:05

[permalink] [raw]

Subject: Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property

Hi Sakari,

On Tue, Apr 07, 2020 at 09:22:41AM +0300, Sakari Ailus wrote:
> On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote:
> > On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote:
> > > On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote:
> > > > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where
> > > > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to
> > > > 24MHz. So instead making clock-frequency as dt-property just let the
> > > > driver enforce the required clock frequency.
> > >
> > > Even if some current systems where the driver is used are using 24 MHz
> > > clock, that doesn't mean there wouldn't be systems using another frequency
> > > that the driver does not support right now.
> > >
> > > The driver really should not set the frequency unless it gets it from DT,
> > > but I think the preferred means is to use assigned-clock-rates instead, and
> > > not to involve the driver with setting the frequency.
> > >
> > > Otherwise we'll make it impossible to support other frequencies, at least
> > > without more or less random defaults.
> >
> > We're running in circles here.
> >
> > As the driver only supports 24MHz at the moment, the frequency should be
> > set by the driver, as it's a driver limitation. We can then work on
> > supporting additional frequencies, which will require DT to provide a
> > list of supported frequencies for the system, but that can be done on
> > top.
>
> I guess it would be possible to use different external clock frequencies on
> a sensor in a given system but that seems to be a bit far fetched, to the
> extent I've never seen anyone doing that in practice.
>
> Originally, the driver set the frequency based on the clock-frequency
> property. If we're removing that but use a fixed frequency instead, then
> how is that going to work going forward when someone adds support for other
> frequencies in the driver and has a system requiring that, while there are
> some other platforms relying on the driver setting a particular frequency?

The standard property for this is link-frequencies, not clock-frequency.
Deprecating clock-frequency now paves the way to use the standard
property later when/if someone implements support for additional
frequencies.

> Although, if you're saying that this driver only needs to work with DT that
> comes with the kernel and you don't care about DT binary compatibility,
> this would be fine.

I believe this series to not break backward compatibility, as the driver
only works with a 24MHz clock, so I expect all DTs to specify that.

--
Regards,

Laurent Pinchart

2020-04-07 15:16:13

[permalink] [raw]

Subject: Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property

Hi Laurent,

On Tue, Apr 07, 2020 at 03:21:06PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Tue, Apr 07, 2020 at 09:22:41AM +0300, Sakari Ailus wrote:
> > On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote:
> > > On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote:
> > > > On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote:
> > > > > Modes in the driver are based on xvclk frequency fixed to 24MHz, but where
> > > > > as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to
> > > > > 24MHz. So instead making clock-frequency as dt-property just let the
> > > > > driver enforce the required clock frequency.
> > > >
> > > > Even if some current systems where the driver is used are using 24 MHz
> > > > clock, that doesn't mean there wouldn't be systems using another frequency
> > > > that the driver does not support right now.
> > > >
> > > > The driver really should not set the frequency unless it gets it from DT,
> > > > but I think the preferred means is to use assigned-clock-rates instead, and
> > > > not to involve the driver with setting the frequency.
> > > >
> > > > Otherwise we'll make it impossible to support other frequencies, at least
> > > > without more or less random defaults.
> > >
> > > We're running in circles here.
> > >
> > > As the driver only supports 24MHz at the moment, the frequency should be
> > > set by the driver, as it's a driver limitation. We can then work on
> > > supporting additional frequencies, which will require DT to provide a
> > > list of supported frequencies for the system, but that can be done on
> > > top.
> >
> > I guess it would be possible to use different external clock frequencies on
> > a sensor in a given system but that seems to be a bit far fetched, to the
> > extent I've never seen anyone doing that in practice.
> >
> > Originally, the driver set the frequency based on the clock-frequency
> > property. If we're removing that but use a fixed frequency instead, then
> > how is that going to work going forward when someone adds support for other
> > frequencies in the driver and has a system requiring that, while there are
> > some other platforms relying on the driver setting a particular frequency?
>
> The standard property for this is link-frequencies, not clock-frequency.
> Deprecating clock-frequency now paves the way to use the standard
> property later when/if someone implements support for additional
> frequencies.

The external clock frequency and link frequency are different indeed, but
they are related. The link frequency has been selected in a way that it is
possible to generate that exact frequency using the chosen external clock
frequency. If you change the external clock frequency, chances are good
there is no PLL configuration to generate that link frequency.

>
> > Although, if you're saying that this driver only needs to work with DT that
> > comes with the kernel and you don't care about DT binary compatibility,
> > this would be fine.
>
> I believe this series to not break backward compatibility, as the driver
> only works with a 24MHz clock, so I expect all DTs to specify that.

What you're still doing here is defining the DT bindings based on the
current driver implementation, not the device properties.

--
Regards,

Sakari Ailus

2020-04-15 13:41:52

[permalink] [raw]

Subject: Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property

Hi Sakari,

On Tue, Apr 14, 2020 at 11:55:54PM +0300, Laurent Pinchart wrote:
> On Tue, Apr 07, 2020 at 06:14:01PM +0300, Sakari Ailus wrote:
> > On Tue, Apr 07, 2020 at 03:21:06PM +0300, Laurent Pinchart wrote:
> >> On Tue, Apr 07, 2020 at 09:22:41AM +0300, Sakari Ailus wrote:
> >>> On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote:
> >>>> On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote:
> >>>>> On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote:
> >>>>>> Modes in the driver are based on xvclk frequency fixed to 24MHz, but where
> >>>>>> as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to
> >>>>>> 24MHz. So instead making clock-frequency as dt-property just let the
> >>>>>> driver enforce the required clock frequency.
> >>>>>
> >>>>> Even if some current systems where the driver is used are using 24 MHz
> >>>>> clock, that doesn't mean there wouldn't be systems using another frequency
> >>>>> that the driver does not support right now.
> >>>>>
> >>>>> The driver really should not set the frequency unless it gets it from DT,
> >>>>> but I think the preferred means is to use assigned-clock-rates instead, and
> >>>>> not to involve the driver with setting the frequency.
> >>>>>
> >>>>> Otherwise we'll make it impossible to support other frequencies, at least
> >>>>> without more or less random defaults.
> >>>>
> >>>> We're running in circles here.
> >>>>
> >>>> As the driver only supports 24MHz at the moment, the frequency should be
> >>>> set by the driver, as it's a driver limitation. We can then work on
> >>>> supporting additional frequencies, which will require DT to provide a
> >>>> list of supported frequencies for the system, but that can be done on
> >>>> top.
> >>>
> >>> I guess it would be possible to use different external clock frequencies on
> >>> a sensor in a given system but that seems to be a bit far fetched, to the
> >>> extent I've never seen anyone doing that in practice.
> >>>
> >>> Originally, the driver set the frequency based on the clock-frequency
> >>> property. If we're removing that but use a fixed frequency instead, then
> >>> how is that going to work going forward when someone adds support for other
> >>> frequencies in the driver and has a system requiring that, while there are
> >>> some other platforms relying on the driver setting a particular frequency?
> >>
> >> The standard property for this is link-frequencies, not clock-frequency.
> >> Deprecating clock-frequency now paves the way to use the standard
> >> property later when/if someone implements support for additional
> >> frequencies.
> >
> > The external clock frequency and link frequency are different indeed, but
> > they are related. The link frequency has been selected in a way that it is
> > possible to generate that exact frequency using the chosen external clock
> > frequency. If you change the external clock frequency, chances are good
> > there is no PLL configuration to generate that link frequency.
>
> But aren't we supposed to pick the clock frequency based on the link
> frequency specified in DT ?
>
> In any case, this policy needs to be carefully documented.

And by this I mean in a central place, not leaving it to individual
bindings.

Maxime, we've previously discussed this issue privately on IRC, what's
your opinion ?

> >>> Although, if you're saying that this driver only needs to work with DT that
> >>> comes with the kernel and you don't care about DT binary compatibility,
> >>> this would be fine.
> >>
> >> I believe this series to not break backward compatibility, as the driver
> >> only works with a 24MHz clock, so I expect all DTs to specify that.
> >
> > What you're still doing here is defining the DT bindings based on the
> > current driver implementation, not the device properties.
>
> Quite the contrary, the device doesn't require any particular input
> clock frequency, so we're removing that from DT :-) Specifying the clock
> frequency in DT is in my opinion a manual workaround for not computing
> it at runtime based on the desired link frequency, while the link
> frequency is a property of the system as it specifies the range of link
> frequencies that are safe to use from an EMC point of view.

--
Regards,

Laurent Pinchart

2020-04-15 13:42:11

[permalink] [raw]

Subject: Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property

Hi Sakari,

On Tue, Apr 07, 2020 at 06:14:01PM +0300, Sakari Ailus wrote:
> On Tue, Apr 07, 2020 at 03:21:06PM +0300, Laurent Pinchart wrote:
> > On Tue, Apr 07, 2020 at 09:22:41AM +0300, Sakari Ailus wrote:
> >> On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote:
> >>> On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote:
> >>>> On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote:
> >>>>> Modes in the driver are based on xvclk frequency fixed to 24MHz, but where
> >>>>> as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to
> >>>>> 24MHz. So instead making clock-frequency as dt-property just let the
> >>>>> driver enforce the required clock frequency.
> >>>>
> >>>> Even if some current systems where the driver is used are using 24 MHz
> >>>> clock, that doesn't mean there wouldn't be systems using another frequency
> >>>> that the driver does not support right now.
> >>>>
> >>>> The driver really should not set the frequency unless it gets it from DT,
> >>>> but I think the preferred means is to use assigned-clock-rates instead, and
> >>>> not to involve the driver with setting the frequency.
> >>>>
> >>>> Otherwise we'll make it impossible to support other frequencies, at least
> >>>> without more or less random defaults.
> >>>
> >>> We're running in circles here.
> >>>
> >>> As the driver only supports 24MHz at the moment, the frequency should be
> >>> set by the driver, as it's a driver limitation. We can then work on
> >>> supporting additional frequencies, which will require DT to provide a
> >>> list of supported frequencies for the system, but that can be done on
> >>> top.
> >>
> >> I guess it would be possible to use different external clock frequencies on
> >> a sensor in a given system but that seems to be a bit far fetched, to the
> >> extent I've never seen anyone doing that in practice.
> >>
> >> Originally, the driver set the frequency based on the clock-frequency
> >> property. If we're removing that but use a fixed frequency instead, then
> >> how is that going to work going forward when someone adds support for other
> >> frequencies in the driver and has a system requiring that, while there are
> >> some other platforms relying on the driver setting a particular frequency?
> >
> > The standard property for this is link-frequencies, not clock-frequency.
> > Deprecating clock-frequency now paves the way to use the standard
> > property later when/if someone implements support for additional
> > frequencies.
>
> The external clock frequency and link frequency are different indeed, but
> they are related. The link frequency has been selected in a way that it is
> possible to generate that exact frequency using the chosen external clock
> frequency. If you change the external clock frequency, chances are good
> there is no PLL configuration to generate that link frequency.

But aren't we supposed to pick the clock frequency based on the link
frequency specified in DT ?

In any case, this policy needs to be carefully documented.

> >> Although, if you're saying that this driver only needs to work with DT that
> >> comes with the kernel and you don't care about DT binary compatibility,
> >> this would be fine.
> >
> > I believe this series to not break backward compatibility, as the driver
> > only works with a 24MHz clock, so I expect all DTs to specify that.
>
> What you're still doing here is defining the DT bindings based on the
> current driver implementation, not the device properties.

Quite the contrary, the device doesn't require any particular input
clock frequency, so we're removing that from DT :-) Specifying the clock
frequency in DT is in my opinion a manual workaround for not computing
it at runtime based on the desired link frequency, while the link
frequency is a property of the system as it specifies the range of link
frequencies that are safe to use from an EMC point of view.

--
Regards,

Laurent Pinchart

2020-04-15 22:32:14

by Maxime Ripard

[permalink] [raw]

Subject: Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property

On Tue, Apr 14, 2020 at 11:55:52PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Tue, Apr 07, 2020 at 06:14:01PM +0300, Sakari Ailus wrote:
> > On Tue, Apr 07, 2020 at 03:21:06PM +0300, Laurent Pinchart wrote:
> > > On Tue, Apr 07, 2020 at 09:22:41AM +0300, Sakari Ailus wrote:
> > >> On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote:
> > >>> On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote:
> > >>>> On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote:
> > >>>>> Modes in the driver are based on xvclk frequency fixed to 24MHz, but where
> > >>>>> as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to
> > >>>>> 24MHz. So instead making clock-frequency as dt-property just let the
> > >>>>> driver enforce the required clock frequency.
> > >>>>
> > >>>> Even if some current systems where the driver is used are using 24 MHz
> > >>>> clock, that doesn't mean there wouldn't be systems using another frequency
> > >>>> that the driver does not support right now.
> > >>>>
> > >>>> The driver really should not set the frequency unless it gets it from DT,
> > >>>> but I think the preferred means is to use assigned-clock-rates instead, and
> > >>>> not to involve the driver with setting the frequency.
> > >>>>
> > >>>> Otherwise we'll make it impossible to support other frequencies, at least
> > >>>> without more or less random defaults.
> > >>>
> > >>> We're running in circles here.
> > >>>
> > >>> As the driver only supports 24MHz at the moment, the frequency should be
> > >>> set by the driver, as it's a driver limitation. We can then work on
> > >>> supporting additional frequencies, which will require DT to provide a
> > >>> list of supported frequencies for the system, but that can be done on
> > >>> top.
> > >>
> > >> I guess it would be possible to use different external clock frequencies on
> > >> a sensor in a given system but that seems to be a bit far fetched, to the
> > >> extent I've never seen anyone doing that in practice.
> > >>
> > >> Originally, the driver set the frequency based on the clock-frequency
> > >> property. If we're removing that but use a fixed frequency instead, then
> > >> how is that going to work going forward when someone adds support for other
> > >> frequencies in the driver and has a system requiring that, while there are
> > >> some other platforms relying on the driver setting a particular frequency?
> > >
> > > The standard property for this is link-frequencies, not clock-frequency.
> > > Deprecating clock-frequency now paves the way to use the standard
> > > property later when/if someone implements support for additional
> > > frequencies.
> >
> > The external clock frequency and link frequency are different indeed, but
> > they are related. The link frequency has been selected in a way that it is
> > possible to generate that exact frequency using the chosen external clock
> > frequency. If you change the external clock frequency, chances are good
> > there is no PLL configuration to generate that link frequency.
>
> But aren't we supposed to pick the clock frequency based on the link
> frequency specified in DT ?
>
> In any case, this policy needs to be carefully documented.
>
> > >> Although, if you're saying that this driver only needs to work with DT that
> > >> comes with the kernel and you don't care about DT binary compatibility,
> > >> this would be fine.
> > >
> > > I believe this series to not break backward compatibility, as the driver
> > > only works with a 24MHz clock, so I expect all DTs to specify that.
> >
> > What you're still doing here is defining the DT bindings based on the
> > current driver implementation, not the device properties.
>
> Quite the contrary, the device doesn't require any particular input
> clock frequency, so we're removing that from DT :-) Specifying the clock
> frequency in DT is in my opinion a manual workaround for not computing
> it at runtime based on the desired link frequency, while the link
> frequency is a property of the system as it specifies the range of link
> frequencies that are safe to use from an EMC point of view.

I 100% agree with this.

Maxime

Attachments:

(No filename) (4.15 kB)
signature.asc (235.00 B)
Download all attachments

2020-04-16 00:11:03

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH v5 5/5] media: dt-bindings: media: i2c: convert ov5645 bindings to json-schema

On Mon, Apr 06, 2020 at 08:43:46PM +0300, Laurent Pinchart wrote:
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Mon, Apr 06, 2020 at 05:42:41PM +0100, Lad Prabhakar wrote:
> > Convert ov5645 bindings to json-schema.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > ---
> > .../devicetree/bindings/media/i2c/ov5645.txt | 52 --------
> > .../devicetree/bindings/media/i2c/ov5645.yaml | 126 ++++++++++++++++++
> > 2 files changed, 126 insertions(+), 52 deletions(-)
> > delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > deleted file mode 100644
> > index a55bb728ea48..000000000000
> > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > +++ /dev/null
> > @@ -1,52 +0,0 @@
> > -* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor
> > -
> > -The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
> > -an active array size of 2592H x 1944V. It is programmable through a serial I2C
> > -interface.
> > -
> > -Required Properties:
> > -- compatible: Value should be "ovti,ov5645".
> > -- clocks: Reference to the xclk clock.
> > -- clock-names: Should be "xclk".
> > -- enable-gpios: Chip enable GPIO. Polarity is GPIO_ACTIVE_HIGH. This corresponds
> > - to the hardware pin PWDNB which is physically active low.
> > -- reset-gpios: Chip reset GPIO. Polarity is GPIO_ACTIVE_LOW. This corresponds to
> > - the hardware pin RESETB.
> > -- vdddo-supply: Chip digital IO regulator.
> > -- vdda-supply: Chip analog regulator.
> > -- vddd-supply: Chip digital core regulator.
> > -
> > -The device node must contain one 'port' child node for its digital output
> > -video port, in accordance with the video interface bindings defined in
> > -Documentation/devicetree/bindings/media/video-interfaces.txt.
> > -
> > -Example:
> > -
> > - &i2c1 {
> > - ...
> > -
> > - ov5645: ov5645@3c {
> > - compatible = "ovti,ov5645";
> > - reg = <0x3c>;
> > -
> > - enable-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
> > - reset-gpios = <&gpio5 20 GPIO_ACTIVE_LOW>;
> > - pinctrl-names = "default";
> > - pinctrl-0 = <&camera_rear_default>;
> > -
> > - clocks = <&clks 200>;
> > - clock-names = "xclk";
> > -
> > - vdddo-supply = <&camera_dovdd_1v8>;
> > - vdda-supply = <&camera_avdd_2v8>;
> > - vddd-supply = <&camera_dvdd_1v2>;
> > -
> > - port {
> > - ov5645_ep: endpoint {
> > - clock-lanes = <1>;
> > - data-lanes = <0 2>;
> > - remote-endpoint = <&csi0_ep>;
> > - };
> > - };
> > - };
> > - };
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.yaml b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> > new file mode 100644
> > index 000000000000..d5cdcf9a1c76
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.yaml
> > @@ -0,0 +1,126 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/ov5645.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Omnivision 1/4-Inch 5MP CMOS Digital Image Sensor
> > +
> > +maintainers:
> > + - Sakari Ailus <[email protected]>
> > + - Lad Prabhakar <[email protected]>
> > +
> > +description: |-
> > + The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with
> > + an active array size of 2592H x 1944V. It is programmable through a serial I2C
> > + interface.
> > +
> > +properties:
> > + compatible:
> > + const: ovti,ov5645
> > +
> > + reg:
> > + description: I2C device address
> > + maxItems: 1
> > +
> > + clocks:
> > + description: External clock frequency should range between 6MHz to 27MHz.
> > + maxItems: 1
> > +
> > + clock-names:
> > + items:
> > + - const: xclk
> > +
> > + enable-gpios:
> > + description: |-
> > + Chip enable GPIO. This corresponds to the hardware pin PWDNB which is
> > + physically active low.
> > +
> > + reset-gpios:
> > + description: Chip reset GPIO. This corresponds to the hardware pin RESETB.
> > +
> > + vdddo-supply:
> > + description: Chip digital IO regulator.
> > +
> > + vdda-supply:
> > + description: Chip analog regulator.
> > +
> > + vddd-supply:
> > + description: Chip digital core regulator.
> > +
> > + # See ../video-interfaces.txt for more details
> > + port:
> > + type: object
> > + properties:
> > + endpoint:
> > + type: object
> > +
> > + properties:
> > + data-lanes:
>
> Don't you need
>
> minItems: 1
> maxItems: 2
>
> here ?

Yes.

> > + description: |-
> > + The sensor supports either one-lane, or two-lane operation.
> > + For one-lane operation the property must be set to <1> and
> > + for two-lane operation the property must be set to <1 2>.
> > + items:
> > + - const: 1
> > + - const: 2
> > +
> > + clock-lanes:
>
> Same here,
>
> maxItems: 1
>
> ?

No, because the default is number of entries in 'items'.

>
> > + description:
> > + should be set to <0> (clock lane on hardware lane 0).
>
> I think you can drop the description, with the items below it's clear
> that the value has to be <0>.
>
> > + items:
> > + - const: 0
> > +
> > + remote-endpoint: true
>
> Should this be
>
> remote-endpoint:
> $ref: /schemas/types.yaml#/definitions/phandle

No, we can assume this has a type definition already.

Rob

2020-04-16 00:21:18

[permalink] [raw]

Subject: Re: [PATCH v5 2/5] media: i2c: ov5645: Drop reading clock-frequency dt-property

Hi Laurent,

On Tue, Apr 14, 2020 at 11:55:52PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Tue, Apr 07, 2020 at 06:14:01PM +0300, Sakari Ailus wrote:
> > On Tue, Apr 07, 2020 at 03:21:06PM +0300, Laurent Pinchart wrote:
> > > On Tue, Apr 07, 2020 at 09:22:41AM +0300, Sakari Ailus wrote:
> > >> On Mon, Apr 06, 2020 at 08:32:34PM +0300, Laurent Pinchart wrote:
> > >>> On Mon, Apr 06, 2020 at 07:51:08PM +0300, Sakari Ailus wrote:
> > >>>> On Mon, Apr 06, 2020 at 05:42:38PM +0100, Lad Prabhakar wrote:
> > >>>>> Modes in the driver are based on xvclk frequency fixed to 24MHz, but where
> > >>>>> as the OV5645 sensor can support the xvclk frequency ranging from 6MHz to
> > >>>>> 24MHz. So instead making clock-frequency as dt-property just let the
> > >>>>> driver enforce the required clock frequency.
> > >>>>
> > >>>> Even if some current systems where the driver is used are using 24 MHz
> > >>>> clock, that doesn't mean there wouldn't be systems using another frequency
> > >>>> that the driver does not support right now.
> > >>>>
> > >>>> The driver really should not set the frequency unless it gets it from DT,
> > >>>> but I think the preferred means is to use assigned-clock-rates instead, and
> > >>>> not to involve the driver with setting the frequency.
> > >>>>
> > >>>> Otherwise we'll make it impossible to support other frequencies, at least
> > >>>> without more or less random defaults.
> > >>>
> > >>> We're running in circles here.
> > >>>
> > >>> As the driver only supports 24MHz at the moment, the frequency should be
> > >>> set by the driver, as it's a driver limitation. We can then work on
> > >>> supporting additional frequencies, which will require DT to provide a
> > >>> list of supported frequencies for the system, but that can be done on
> > >>> top.
> > >>
> > >> I guess it would be possible to use different external clock frequencies on
> > >> a sensor in a given system but that seems to be a bit far fetched, to the
> > >> extent I've never seen anyone doing that in practice.
> > >>
> > >> Originally, the driver set the frequency based on the clock-frequency
> > >> property. If we're removing that but use a fixed frequency instead, then
> > >> how is that going to work going forward when someone adds support for other
> > >> frequencies in the driver and has a system requiring that, while there are
> > >> some other platforms relying on the driver setting a particular frequency?
> > >
> > > The standard property for this is link-frequencies, not clock-frequency.
> > > Deprecating clock-frequency now paves the way to use the standard
> > > property later when/if someone implements support for additional
> > > frequencies.
> >
> > The external clock frequency and link frequency are different indeed, but
> > they are related. The link frequency has been selected in a way that it is
> > possible to generate that exact frequency using the chosen external clock
> > frequency. If you change the external clock frequency, chances are good
> > there is no PLL configuration to generate that link frequency.
>
> But aren't we supposed to pick the clock frequency based on the link
> frequency specified in DT ?

No. In a general case there is no reliable way to come up with an external
clock frequency based on another, different if related, frequency.

>
> In any case, this policy needs to be carefully documented.

I thought after ten or so years this would be already an established
practice. :-)

I agree it should be documented. We don't seem to have specific
documentation for camera sensor drivers at the moment. I can submit a
patch...

>
> > >> Although, if you're saying that this driver only needs to work with DT that
> > >> comes with the kernel and you don't care about DT binary compatibility,
> > >> this would be fine.
> > >
> > > I believe this series to not break backward compatibility, as the driver
> > > only works with a 24MHz clock, so I expect all DTs to specify that.
> >
> > What you're still doing here is defining the DT bindings based on the
> > current driver implementation, not the device properties.
>
> Quite the contrary, the device doesn't require any particular input
> clock frequency, so we're removing that from DT :-) Specifying the clock
> frequency in DT is in my opinion a manual workaround for not computing
> it at runtime based on the desired link frequency, while the link
> frequency is a property of the system as it specifies the range of link
> frequencies that are safe to use from an EMC point of view.

The external clock frequency is significantly lower than the link frequency
(usually), but it still comes out of the SoC (or a PMIC chip). The clock
signal track on PCB as well as wiring may also be rather long, depending on
where the camera sensor is --- quite possibly tens of centimetres.
Therefore I wouldn't categorically rule out possible EMC issues with that
one either.

The bottom line is: use a known-good, safe frequency.

--
Regards,

Sakari Ailus

2020-04-17 02:13:54