* [PATCH v3 0/4] ov5645: Switch to assigned-clock-rates
@ 2020-03-13 21:12 ` Lad Prabhakar
0 siblings, 0 replies; 58+ messages in thread
From: Lad Prabhakar @ 2020-03-13 21:12 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus,
NXP Linux Team, Magnus Damm, Ezequiel Garcia, Laurent Pinchart,
Geert Uytterhoeven
Cc: devicetree, linux-kernel, Lad Prabhakar, linux-renesas-soc,
Fabio Estevam, linux-arm-kernel, linux-media
Hi All,
This patch series adds support for using assigned-clock-rates for
specifying clock rates for ov5645 driver
Thanks,
Prabhakar
Changed for v3:
* Dropped reading assigned-clock-rates
* Increased the maximum clock frequency to 24480000
Changes for v2:
* Instead of completely dropping clock-frequency property marked it as
deprecated.
* Split up imx6qdl-wandboard.dtsi changes as separate patch.
Lad Prabhakar (4):
media: dt-bindings: media: i2c: Switch to assigned-clock-rates
media: i2c: ov5645: Switch to assigned-clock-rates
media: i2c: ov5645: Set maximum leverage of external clock frequency
to 24480000
ARM: dts: imx6qdl-wandboard: Switch to assigned-clock-rates for ov5645
node
.../devicetree/bindings/media/i2c/ov5645.txt | 5 ++--
arch/arm/boot/dts/imx6qdl-wandboard.dtsi | 3 ++-
drivers/media/i2c/ov5645.c | 27 ++++++++++++----------
3 files changed, 20 insertions(+), 15 deletions(-)
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread* [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
2020-03-13 21:12 ` Lad Prabhakar
@ 2020-03-13 21:12 ` Lad Prabhakar
-1 siblings, 0 replies; 58+ messages in thread
From: Lad Prabhakar @ 2020-03-13 21:12 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus,
NXP Linux Team, Magnus Damm, Ezequiel Garcia, Laurent Pinchart,
Geert Uytterhoeven
Cc: devicetree, linux-kernel, linux-renesas-soc, Lad Prabhakar,
Fabio Estevam, linux-media, linux-arm-kernel
Use assigned-clock-rates to specify the clock rate. Also mark
clock-frequency property as deprecated.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
index 72ad992..e62fe82 100644
--- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
+++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
@@ -8,7 +8,7 @@ Required Properties:
- compatible: Value should be "ovti,ov5645".
- clocks: Reference to the xclk clock.
- clock-names: Should be "xclk".
-- clock-frequency: Frequency of the xclk clock.
+- clock-frequency (deprecated): Frequency of the xclk clock.
- 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
@@ -37,7 +37,8 @@ Example:
clocks = <&clks 200>;
clock-names = "xclk";
- clock-frequency = <24000000>;
+ assigned-clocks = <&clks 200>;
+ assigned-clock-rates = <24000000>;
vdddo-supply = <&camera_dovdd_1v8>;
vdda-supply = <&camera_avdd_2v8>;
--
2.7.4
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
@ 2020-03-13 21:12 ` Lad Prabhakar
0 siblings, 0 replies; 58+ messages in thread
From: Lad Prabhakar @ 2020-03-13 21:12 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus,
NXP Linux Team, Magnus Damm, Ezequiel Garcia, Laurent Pinchart,
Geert Uytterhoeven
Cc: devicetree, linux-kernel, Lad Prabhakar, linux-renesas-soc,
Fabio Estevam, linux-arm-kernel, linux-media
Use assigned-clock-rates to specify the clock rate. Also mark
clock-frequency property as deprecated.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
index 72ad992..e62fe82 100644
--- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
+++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
@@ -8,7 +8,7 @@ Required Properties:
- compatible: Value should be "ovti,ov5645".
- clocks: Reference to the xclk clock.
- clock-names: Should be "xclk".
-- clock-frequency: Frequency of the xclk clock.
+- clock-frequency (deprecated): Frequency of the xclk clock.
- 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
@@ -37,7 +37,8 @@ Example:
clocks = <&clks 200>;
clock-names = "xclk";
- clock-frequency = <24000000>;
+ assigned-clocks = <&clks 200>;
+ assigned-clock-rates = <24000000>;
vdddo-supply = <&camera_dovdd_1v8>;
vdda-supply = <&camera_avdd_2v8>;
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
2020-03-13 21:12 ` Lad Prabhakar
@ 2020-03-13 21:20 ` Laurent Pinchart
-1 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-03-13 21:20 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus,
NXP Linux Team, Magnus Damm, Ezequiel Garcia, Geert Uytterhoeven,
devicetree, linux-kernel, linux-renesas-soc, Fabio Estevam,
linux-media, linux-arm-kernel
Hi Prabhakar,
Thank you for the patch.
On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> Use assigned-clock-rates to specify the clock rate. Also mark
> clock-frequency property as deprecated.
I would phrase it the other way around, this patch mainly deprecates
clock-frequency, and as a side effect recommends usage of
assigned-clock-rates.
"Deprecate usage of the clock-frequency propertly. The preferred method
to set clock rates is to use assigned-clock-rates."
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> index 72ad992..e62fe82 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> @@ -8,7 +8,7 @@ Required Properties:
> - compatible: Value should be "ovti,ov5645".
> - clocks: Reference to the xclk clock.
> - clock-names: Should be "xclk".
> -- clock-frequency: Frequency of the xclk clock.
> +- clock-frequency (deprecated): Frequency of the xclk clock.
I would drop this completely. Drivers need to ensure backward
compatibility, but DT bindings should only document the latest version,
the history is available in git.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
While at it, can I enlist you to convert these bindings to yaml ? :-)
> - 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
> @@ -37,7 +37,8 @@ Example:
>
> clocks = <&clks 200>;
> clock-names = "xclk";
> - clock-frequency = <24000000>;
> + assigned-clocks = <&clks 200>;
> + assigned-clock-rates = <24000000>;
>
> vdddo-supply = <&camera_dovdd_1v8>;
> vdda-supply = <&camera_avdd_2v8>;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
@ 2020-03-13 21:20 ` Laurent Pinchart
0 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-03-13 21:20 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Mark Rutland, devicetree, Fabio Estevam, Sakari Ailus,
Ezequiel Garcia, Sascha Hauer, Magnus Damm, linux-kernel,
linux-renesas-soc, Rob Herring, Geert Uytterhoeven,
NXP Linux Team, Pengutronix Kernel Team, Mauro Carvalho Chehab,
Shawn Guo, linux-arm-kernel, linux-media
Hi Prabhakar,
Thank you for the patch.
On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> Use assigned-clock-rates to specify the clock rate. Also mark
> clock-frequency property as deprecated.
I would phrase it the other way around, this patch mainly deprecates
clock-frequency, and as a side effect recommends usage of
assigned-clock-rates.
"Deprecate usage of the clock-frequency propertly. The preferred method
to set clock rates is to use assigned-clock-rates."
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> index 72ad992..e62fe82 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> @@ -8,7 +8,7 @@ Required Properties:
> - compatible: Value should be "ovti,ov5645".
> - clocks: Reference to the xclk clock.
> - clock-names: Should be "xclk".
> -- clock-frequency: Frequency of the xclk clock.
> +- clock-frequency (deprecated): Frequency of the xclk clock.
I would drop this completely. Drivers need to ensure backward
compatibility, but DT bindings should only document the latest version,
the history is available in git.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
While at it, can I enlist you to convert these bindings to yaml ? :-)
> - 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
> @@ -37,7 +37,8 @@ Example:
>
> clocks = <&clks 200>;
> clock-names = "xclk";
> - clock-frequency = <24000000>;
> + assigned-clocks = <&clks 200>;
> + assigned-clock-rates = <24000000>;
>
> vdddo-supply = <&camera_dovdd_1v8>;
> vdda-supply = <&camera_avdd_2v8>;
--
Regards,
Laurent Pinchart
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* RE: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
2020-03-13 21:20 ` Laurent Pinchart
@ 2020-03-13 21:25 ` Prabhakar Mahadev Lad
-1 siblings, 0 replies; 58+ messages in thread
From: Prabhakar Mahadev Lad @ 2020-03-13 21:25 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus,
NXP Linux Team, Magnus Damm, Ezequiel Garcia, Geert Uytterhoeven,
devicetree, linux-kernel, linux-renesas-soc, Fabio Estevam,
linux-media, linux-arm-kernel
Hi Laurent,
Thank you for the quick review.
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 13 March 2020 21:20
> To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Shawn Guo
> <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Rob Herring
> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Sakari
> Ailus <sakari.ailus@linux.intel.com>; NXP Linux Team <linux-imx@nxp.com>;
> Magnus Damm <magnus.damm@gmail.com>; Ezequiel Garcia
> <ezequiel@collabora.com>; Geert Uytterhoeven <geert@linux-m68k.org>;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-renesas-
> soc@vger.kernel.org; Fabio Estevam <festevam@gmail.com>; linux-
> media@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to
> assigned-clock-rates
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > Use assigned-clock-rates to specify the clock rate. Also mark
> > clock-frequency property as deprecated.
>
> I would phrase it the other way around, this patch mainly deprecates clock-
> frequency, and as a side effect recommends usage of assigned-clock-rates.
>
> "Deprecate usage of the clock-frequency propertly. The preferred method
> to set clock rates is to use assigned-clock-rates."
>
Agreed will do that.
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> lad.rj@bp.renesas.com>
> > ---
> > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > index 72ad992..e62fe82 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > @@ -8,7 +8,7 @@ Required Properties:
> > - compatible: Value should be "ovti,ov5645".
> > - clocks: Reference to the xclk clock.
> > - clock-names: Should be "xclk".
> > -- clock-frequency: Frequency of the xclk clock.
> > +- clock-frequency (deprecated): Frequency of the xclk clock.
>
> I would drop this completely. Drivers need to ensure backward compatibility,
> but DT bindings should only document the latest version, the history is
> available in git.
>
Sure will drop it.
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> While at it, can I enlist you to convert these bindings to yaml ? :-)
>
Sure will do the honours 😊, will make sure yaml patch is ontop of this patch too.
Cheers,
--Prabhakar
> > - 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 @@ -37,7 +37,8 @@ Example:
> >
> > clocks = <&clks 200>;
> > clock-names = "xclk";
> > -clock-frequency = <24000000>;
> > +assigned-clocks = <&clks 200>;
> > +assigned-clock-rates = <24000000>;
> >
> > vdddo-supply = <&camera_dovdd_1v8>;
> > vdda-supply = <&camera_avdd_2v8>;
>
> --
> Regards,
>
> Laurent Pinchart
Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
^ permalink raw reply [flat|nested] 58+ messages in thread
* RE: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
@ 2020-03-13 21:25 ` Prabhakar Mahadev Lad
0 siblings, 0 replies; 58+ messages in thread
From: Prabhakar Mahadev Lad @ 2020-03-13 21:25 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mark Rutland, devicetree, Fabio Estevam, Sakari Ailus,
Ezequiel Garcia, Sascha Hauer, Magnus Damm, linux-kernel,
linux-renesas-soc, Rob Herring, Geert Uytterhoeven,
NXP Linux Team, Pengutronix Kernel Team, Mauro Carvalho Chehab,
Shawn Guo, linux-arm-kernel, linux-media
Hi Laurent,
Thank you for the quick review.
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 13 March 2020 21:20
> To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Shawn Guo
> <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Rob Herring
> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Sakari
> Ailus <sakari.ailus@linux.intel.com>; NXP Linux Team <linux-imx@nxp.com>;
> Magnus Damm <magnus.damm@gmail.com>; Ezequiel Garcia
> <ezequiel@collabora.com>; Geert Uytterhoeven <geert@linux-m68k.org>;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-renesas-
> soc@vger.kernel.org; Fabio Estevam <festevam@gmail.com>; linux-
> media@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to
> assigned-clock-rates
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > Use assigned-clock-rates to specify the clock rate. Also mark
> > clock-frequency property as deprecated.
>
> I would phrase it the other way around, this patch mainly deprecates clock-
> frequency, and as a side effect recommends usage of assigned-clock-rates.
>
> "Deprecate usage of the clock-frequency propertly. The preferred method
> to set clock rates is to use assigned-clock-rates."
>
Agreed will do that.
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> lad.rj@bp.renesas.com>
> > ---
> > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > index 72ad992..e62fe82 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > @@ -8,7 +8,7 @@ Required Properties:
> > - compatible: Value should be "ovti,ov5645".
> > - clocks: Reference to the xclk clock.
> > - clock-names: Should be "xclk".
> > -- clock-frequency: Frequency of the xclk clock.
> > +- clock-frequency (deprecated): Frequency of the xclk clock.
>
> I would drop this completely. Drivers need to ensure backward compatibility,
> but DT bindings should only document the latest version, the history is
> available in git.
>
Sure will drop it.
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> While at it, can I enlist you to convert these bindings to yaml ? :-)
>
Sure will do the honours 😊, will make sure yaml patch is ontop of this patch too.
Cheers,
--Prabhakar
> > - 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 @@ -37,7 +37,8 @@ Example:
> >
> > clocks = <&clks 200>;
> > clock-names = "xclk";
> > -clock-frequency = <24000000>;
> > +assigned-clocks = <&clks 200>;
> > +assigned-clock-rates = <24000000>;
> >
> > vdddo-supply = <&camera_dovdd_1v8>;
> > vdda-supply = <&camera_avdd_2v8>;
>
> --
> Regards,
>
> Laurent Pinchart
Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
2020-03-13 21:25 ` Prabhakar Mahadev Lad
@ 2020-03-13 21:27 ` Laurent Pinchart
-1 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-03-13 21:27 UTC (permalink / raw)
To: Prabhakar Mahadev Lad
Cc: Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus,
NXP Linux Team, Magnus Damm, Ezequiel Garcia, Geert Uytterhoeven,
devicetree, linux-kernel, linux-renesas-soc, Fabio Estevam,
linux-media, linux-arm-kernel
Hi Prabhakar,
On Fri, Mar 13, 2020 at 09:25:01PM +0000, Prabhakar Mahadev Lad wrote:
> On Sent: 13 March 2020 21:20, Laurent Pinchart wrote:
> > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > > Use assigned-clock-rates to specify the clock rate. Also mark
> > > clock-frequency property as deprecated.
> >
> > I would phrase it the other way around, this patch mainly deprecates clock-
> > frequency, and as a side effect recommends usage of assigned-clock-rates.
> >
> > "Deprecate usage of the clock-frequency propertly. The preferred method
> > to set clock rates is to use assigned-clock-rates."
>
> Agreed will do that.
>
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > index 72ad992..e62fe82 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > @@ -8,7 +8,7 @@ Required Properties:
> > > - compatible: Value should be "ovti,ov5645".
> > > - clocks: Reference to the xclk clock.
> > > - clock-names: Should be "xclk".
> > > -- clock-frequency: Frequency of the xclk clock.
> > > +- clock-frequency (deprecated): Frequency of the xclk clock.
> >
> > I would drop this completely. Drivers need to ensure backward compatibility,
> > but DT bindings should only document the latest version, the history is
> > available in git.
> >
> Sure will drop it.
>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > While at it, can I enlist you to convert these bindings to yaml ? :-)
> >
> Sure will do the honours 😊, will make sure yaml patch is ontop of this patch too.
Thank you :-)
> > > - 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
> > > @@ -37,7 +37,8 @@ Example:
> > >
> > > clocks = <&clks 200>;
> > > clock-names = "xclk";
> > > -clock-frequency = <24000000>;
> > > +assigned-clocks = <&clks 200>;
> > > +assigned-clock-rates = <24000000>;
> > >
> > > vdddo-supply = <&camera_dovdd_1v8>;
> > > vdda-supply = <&camera_avdd_2v8>;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
@ 2020-03-13 21:27 ` Laurent Pinchart
0 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-03-13 21:27 UTC (permalink / raw)
To: Prabhakar Mahadev Lad
Cc: Mark Rutland, devicetree, Fabio Estevam, Sakari Ailus,
Ezequiel Garcia, Sascha Hauer, Magnus Damm, linux-kernel,
linux-renesas-soc, Rob Herring, Geert Uytterhoeven,
NXP Linux Team, Pengutronix Kernel Team, Mauro Carvalho Chehab,
Shawn Guo, linux-arm-kernel, linux-media
Hi Prabhakar,
On Fri, Mar 13, 2020 at 09:25:01PM +0000, Prabhakar Mahadev Lad wrote:
> On Sent: 13 March 2020 21:20, Laurent Pinchart wrote:
> > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > > Use assigned-clock-rates to specify the clock rate. Also mark
> > > clock-frequency property as deprecated.
> >
> > I would phrase it the other way around, this patch mainly deprecates clock-
> > frequency, and as a side effect recommends usage of assigned-clock-rates.
> >
> > "Deprecate usage of the clock-frequency propertly. The preferred method
> > to set clock rates is to use assigned-clock-rates."
>
> Agreed will do that.
>
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > index 72ad992..e62fe82 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > @@ -8,7 +8,7 @@ Required Properties:
> > > - compatible: Value should be "ovti,ov5645".
> > > - clocks: Reference to the xclk clock.
> > > - clock-names: Should be "xclk".
> > > -- clock-frequency: Frequency of the xclk clock.
> > > +- clock-frequency (deprecated): Frequency of the xclk clock.
> >
> > I would drop this completely. Drivers need to ensure backward compatibility,
> > but DT bindings should only document the latest version, the history is
> > available in git.
> >
> Sure will drop it.
>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > While at it, can I enlist you to convert these bindings to yaml ? :-)
> >
> Sure will do the honours 😊, will make sure yaml patch is ontop of this patch too.
Thank you :-)
> > > - 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
> > > @@ -37,7 +37,8 @@ Example:
> > >
> > > clocks = <&clks 200>;
> > > clock-names = "xclk";
> > > -clock-frequency = <24000000>;
> > > +assigned-clocks = <&clks 200>;
> > > +assigned-clock-rates = <24000000>;
> > >
> > > vdddo-supply = <&camera_dovdd_1v8>;
> > > vdda-supply = <&camera_avdd_2v8>;
--
Regards,
Laurent Pinchart
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
2020-03-13 21:27 ` Laurent Pinchart
@ 2020-03-18 20:13 ` Lad, Prabhakar
-1 siblings, 0 replies; 58+ messages in thread
From: Lad, Prabhakar @ 2020-03-18 20:13 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Prabhakar Mahadev Lad, Mark Rutland, devicetree, Fabio Estevam,
Sakari Ailus, Ezequiel Garcia, Sascha Hauer, Magnus Damm,
linux-kernel, linux-renesas-soc, Rob Herring, Geert Uytterhoeven,
NXP Linux Team, Pengutronix Kernel Team, Mauro Carvalho Chehab,
Shawn Guo, linux-arm-kernel, linux-media
Hi Laurent,
On Fri, Mar 13, 2020 at 9:27 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> On Fri, Mar 13, 2020 at 09:25:01PM +0000, Prabhakar Mahadev Lad wrote:
> > On Sent: 13 March 2020 21:20, Laurent Pinchart wrote:
> > > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > > > Use assigned-clock-rates to specify the clock rate. Also mark
> > > > clock-frequency property as deprecated.
> > >
> > > I would phrase it the other way around, this patch mainly deprecates clock-
> > > frequency, and as a side effect recommends usage of assigned-clock-rates.
> > >
> > > "Deprecate usage of the clock-frequency propertly. The preferred method
> > > to set clock rates is to use assigned-clock-rates."
> >
> > Agreed will do that.
> >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > index 72ad992..e62fe82 100644
> > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > @@ -8,7 +8,7 @@ Required Properties:
> > > > - compatible: Value should be "ovti,ov5645".
> > > > - clocks: Reference to the xclk clock.
> > > > - clock-names: Should be "xclk".
> > > > -- clock-frequency: Frequency of the xclk clock.
> > > > +- clock-frequency (deprecated): Frequency of the xclk clock.
> > >
> > > I would drop this completely. Drivers need to ensure backward compatibility,
> > > but DT bindings should only document the latest version, the history is
> > > available in git.
> > >
> > Sure will drop it.
> >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > While at it, can I enlist you to convert these bindings to yaml ? :-)
> > >
> > Sure will do the honours , will make sure yaml patch is ontop of this patch too.
>
Shall I enlist you as the maintainer in the json-schema ?
dt_binding_check says 'maintainers' is a required property.
Cheers,
--Prabhakar Lad
> Thank you :-)
>
> > > > - 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
> > > > @@ -37,7 +37,8 @@ Example:
> > > >
> > > > clocks = <&clks 200>;
> > > > clock-names = "xclk";
> > > > -clock-frequency = <24000000>;
> > > > +assigned-clocks = <&clks 200>;
> > > > +assigned-clock-rates = <24000000>;
> > > >
> > > > vdddo-supply = <&camera_dovdd_1v8>;
> > > > vdda-supply = <&camera_avdd_2v8>;
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
@ 2020-03-18 20:13 ` Lad, Prabhakar
0 siblings, 0 replies; 58+ messages in thread
From: Lad, Prabhakar @ 2020-03-18 20:13 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mark Rutland, devicetree, Pengutronix Kernel Team,
Ezequiel Garcia, Sascha Hauer, Magnus Damm,
Prabhakar Mahadev Lad, linux-kernel, linux-renesas-soc,
Rob Herring, Geert Uytterhoeven, NXP Linux Team, Sakari Ailus,
Shawn Guo, Mauro Carvalho Chehab, Fabio Estevam,
linux-arm-kernel, linux-media
Hi Laurent,
On Fri, Mar 13, 2020 at 9:27 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> On Fri, Mar 13, 2020 at 09:25:01PM +0000, Prabhakar Mahadev Lad wrote:
> > On Sent: 13 March 2020 21:20, Laurent Pinchart wrote:
> > > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > > > Use assigned-clock-rates to specify the clock rate. Also mark
> > > > clock-frequency property as deprecated.
> > >
> > > I would phrase it the other way around, this patch mainly deprecates clock-
> > > frequency, and as a side effect recommends usage of assigned-clock-rates.
> > >
> > > "Deprecate usage of the clock-frequency propertly. The preferred method
> > > to set clock rates is to use assigned-clock-rates."
> >
> > Agreed will do that.
> >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > index 72ad992..e62fe82 100644
> > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > @@ -8,7 +8,7 @@ Required Properties:
> > > > - compatible: Value should be "ovti,ov5645".
> > > > - clocks: Reference to the xclk clock.
> > > > - clock-names: Should be "xclk".
> > > > -- clock-frequency: Frequency of the xclk clock.
> > > > +- clock-frequency (deprecated): Frequency of the xclk clock.
> > >
> > > I would drop this completely. Drivers need to ensure backward compatibility,
> > > but DT bindings should only document the latest version, the history is
> > > available in git.
> > >
> > Sure will drop it.
> >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > While at it, can I enlist you to convert these bindings to yaml ? :-)
> > >
> > Sure will do the honours , will make sure yaml patch is ontop of this patch too.
>
Shall I enlist you as the maintainer in the json-schema ?
dt_binding_check says 'maintainers' is a required property.
Cheers,
--Prabhakar Lad
> Thank you :-)
>
> > > > - 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
> > > > @@ -37,7 +37,8 @@ Example:
> > > >
> > > > clocks = <&clks 200>;
> > > > clock-names = "xclk";
> > > > -clock-frequency = <24000000>;
> > > > +assigned-clocks = <&clks 200>;
> > > > +assigned-clock-rates = <24000000>;
> > > >
> > > > vdddo-supply = <&camera_dovdd_1v8>;
> > > > vdda-supply = <&camera_avdd_2v8>;
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
2020-03-18 20:13 ` Lad, Prabhakar
@ 2020-03-18 22:33 ` Laurent Pinchart
-1 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-03-18 22:33 UTC (permalink / raw)
To: Lad, Prabhakar
Cc: Prabhakar Mahadev Lad, Mark Rutland, devicetree, Fabio Estevam,
Sakari Ailus, Ezequiel Garcia, Sascha Hauer, Magnus Damm,
linux-kernel, linux-renesas-soc, Rob Herring, Geert Uytterhoeven,
NXP Linux Team, Pengutronix Kernel Team, Mauro Carvalho Chehab,
Shawn Guo, linux-arm-kernel, linux-media
Hi Prabhakar,
On Wed, Mar 18, 2020 at 08:13:00PM +0000, Lad, Prabhakar wrote:
> On Fri, Mar 13, 2020 at 9:27 PM Laurent Pinchart wrote:
> > On Fri, Mar 13, 2020 at 09:25:01PM +0000, Prabhakar Mahadev Lad wrote:
> >> On Sent: 13 March 2020 21:20, Laurent Pinchart wrote:
> >>> On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> >>>> Use assigned-clock-rates to specify the clock rate. Also mark
> >>>> clock-frequency property as deprecated.
> >>>
> >>> I would phrase it the other way around, this patch mainly deprecates clock-
> >>> frequency, and as a side effect recommends usage of assigned-clock-rates.
> >>>
> >>> "Deprecate usage of the clock-frequency propertly. The preferred method
> >>> to set clock rates is to use assigned-clock-rates."
> >>
> >> Agreed will do that.
> >>
> >>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>>> ---
> >>>> Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> >>>> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> >>>> b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> >>>> index 72ad992..e62fe82 100644
> >>>> --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> >>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> >>>> @@ -8,7 +8,7 @@ Required Properties:
> >>>> - compatible: Value should be "ovti,ov5645".
> >>>> - clocks: Reference to the xclk clock.
> >>>> - clock-names: Should be "xclk".
> >>>> -- clock-frequency: Frequency of the xclk clock.
> >>>> +- clock-frequency (deprecated): Frequency of the xclk clock.
> >>>
> >>> I would drop this completely. Drivers need to ensure backward compatibility,
> >>> but DT bindings should only document the latest version, the history is
> >>> available in git.
> >>
> >> Sure will drop it.
> >>
> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>
> >>> While at it, can I enlist you to convert these bindings to yaml ? :-)
> >>>
> >> Sure will do the honours , will make sure yaml patch is ontop of this patch too.
>
> Shall I enlist you as the maintainer in the json-schema ?
> dt_binding_check says 'maintainers' is a required property.
Do you want to be the new maintainer ? :-) Sakari is maintaining sensor
drivers (in his spare time though) so maybe he could be listed in the DT
bindings too if he wants. Otherwise, I could do it.
> > Thank you :-)
> >
> >>>> - 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
> >>>> @@ -37,7 +37,8 @@ Example:
> >>>>
> >>>> clocks = <&clks 200>;
> >>>> clock-names = "xclk";
> >>>> -clock-frequency = <24000000>;
> >>>> +assigned-clocks = <&clks 200>;
> >>>> +assigned-clock-rates = <24000000>;
> >>>>
> >>>> vdddo-supply = <&camera_dovdd_1v8>;
> >>>> vdda-supply = <&camera_avdd_2v8>;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
@ 2020-03-18 22:33 ` Laurent Pinchart
0 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-03-18 22:33 UTC (permalink / raw)
To: Lad, Prabhakar
Cc: Mark Rutland, devicetree, Pengutronix Kernel Team,
Ezequiel Garcia, Sascha Hauer, Magnus Damm,
Prabhakar Mahadev Lad, linux-kernel, linux-renesas-soc,
Rob Herring, Geert Uytterhoeven, NXP Linux Team, Sakari Ailus,
Shawn Guo, Mauro Carvalho Chehab, Fabio Estevam,
linux-arm-kernel, linux-media
Hi Prabhakar,
On Wed, Mar 18, 2020 at 08:13:00PM +0000, Lad, Prabhakar wrote:
> On Fri, Mar 13, 2020 at 9:27 PM Laurent Pinchart wrote:
> > On Fri, Mar 13, 2020 at 09:25:01PM +0000, Prabhakar Mahadev Lad wrote:
> >> On Sent: 13 March 2020 21:20, Laurent Pinchart wrote:
> >>> On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> >>>> Use assigned-clock-rates to specify the clock rate. Also mark
> >>>> clock-frequency property as deprecated.
> >>>
> >>> I would phrase it the other way around, this patch mainly deprecates clock-
> >>> frequency, and as a side effect recommends usage of assigned-clock-rates.
> >>>
> >>> "Deprecate usage of the clock-frequency propertly. The preferred method
> >>> to set clock rates is to use assigned-clock-rates."
> >>
> >> Agreed will do that.
> >>
> >>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>>> ---
> >>>> Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> >>>> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> >>>> b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> >>>> index 72ad992..e62fe82 100644
> >>>> --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> >>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> >>>> @@ -8,7 +8,7 @@ Required Properties:
> >>>> - compatible: Value should be "ovti,ov5645".
> >>>> - clocks: Reference to the xclk clock.
> >>>> - clock-names: Should be "xclk".
> >>>> -- clock-frequency: Frequency of the xclk clock.
> >>>> +- clock-frequency (deprecated): Frequency of the xclk clock.
> >>>
> >>> I would drop this completely. Drivers need to ensure backward compatibility,
> >>> but DT bindings should only document the latest version, the history is
> >>> available in git.
> >>
> >> Sure will drop it.
> >>
> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>
> >>> While at it, can I enlist you to convert these bindings to yaml ? :-)
> >>>
> >> Sure will do the honours , will make sure yaml patch is ontop of this patch too.
>
> Shall I enlist you as the maintainer in the json-schema ?
> dt_binding_check says 'maintainers' is a required property.
Do you want to be the new maintainer ? :-) Sakari is maintaining sensor
drivers (in his spare time though) so maybe he could be listed in the DT
bindings too if he wants. Otherwise, I could do it.
> > Thank you :-)
> >
> >>>> - 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
> >>>> @@ -37,7 +37,8 @@ Example:
> >>>>
> >>>> clocks = <&clks 200>;
> >>>> clock-names = "xclk";
> >>>> -clock-frequency = <24000000>;
> >>>> +assigned-clocks = <&clks 200>;
> >>>> +assigned-clock-rates = <24000000>;
> >>>>
> >>>> vdddo-supply = <&camera_dovdd_1v8>;
> >>>> vdda-supply = <&camera_avdd_2v8>;
--
Regards,
Laurent Pinchart
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
2020-03-18 22:33 ` Laurent Pinchart
@ 2020-03-18 22:37 ` Lad, Prabhakar
-1 siblings, 0 replies; 58+ messages in thread
From: Lad, Prabhakar @ 2020-03-18 22:37 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Prabhakar Mahadev Lad, Mark Rutland, devicetree, Fabio Estevam,
Sakari Ailus, Ezequiel Garcia, Sascha Hauer, Magnus Damm,
linux-kernel, linux-renesas-soc, Rob Herring, Geert Uytterhoeven,
NXP Linux Team, Pengutronix Kernel Team, Mauro Carvalho Chehab,
Shawn Guo, linux-arm-kernel, linux-media
Hi Laurent,
On Wed, Mar 18, 2020 at 10:33 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> On Wed, Mar 18, 2020 at 08:13:00PM +0000, Lad, Prabhakar wrote:
> > On Fri, Mar 13, 2020 at 9:27 PM Laurent Pinchart wrote:
> > > On Fri, Mar 13, 2020 at 09:25:01PM +0000, Prabhakar Mahadev Lad wrote:
> > >> On Sent: 13 March 2020 21:20, Laurent Pinchart wrote:
> > >>> On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > >>>> Use assigned-clock-rates to specify the clock rate. Also mark
> > >>>> clock-frequency property as deprecated.
> > >>>
> > >>> I would phrase it the other way around, this patch mainly deprecates clock-
> > >>> frequency, and as a side effect recommends usage of assigned-clock-rates.
> > >>>
> > >>> "Deprecate usage of the clock-frequency propertly. The preferred method
> > >>> to set clock rates is to use assigned-clock-rates."
> > >>
> > >> Agreed will do that.
> > >>
> > >>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >>>> ---
> > >>>> Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > >>>> 1 file changed, 3 insertions(+), 2 deletions(-)
> > >>>>
> > >>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > >>>> b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > >>>> index 72ad992..e62fe82 100644
> > >>>> --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > >>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > >>>> @@ -8,7 +8,7 @@ Required Properties:
> > >>>> - compatible: Value should be "ovti,ov5645".
> > >>>> - clocks: Reference to the xclk clock.
> > >>>> - clock-names: Should be "xclk".
> > >>>> -- clock-frequency: Frequency of the xclk clock.
> > >>>> +- clock-frequency (deprecated): Frequency of the xclk clock.
> > >>>
> > >>> I would drop this completely. Drivers need to ensure backward compatibility,
> > >>> but DT bindings should only document the latest version, the history is
> > >>> available in git.
> > >>
> > >> Sure will drop it.
> > >>
> > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >>>
> > >>> While at it, can I enlist you to convert these bindings to yaml ? :-)
> > >>>
> > >> Sure will do the honours , will make sure yaml patch is ontop of this patch too.
> >
> > Shall I enlist you as the maintainer in the json-schema ?
> > dt_binding_check says 'maintainers' is a required property.
>
> Do you want to be the new maintainer ? :-) Sakari is maintaining sensor
> drivers (in his spare time though) so maybe he could be listed in the DT
> bindings too if he wants. Otherwise, I could do it.
>
OK I will add myself and Sakari for now and post a v4.
Cheers,
--Prabhakar
> > > Thank you :-)
> > >
> > >>>> - 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
> > >>>> @@ -37,7 +37,8 @@ Example:
> > >>>>
> > >>>> clocks = <&clks 200>;
> > >>>> clock-names = "xclk";
> > >>>> -clock-frequency = <24000000>;
> > >>>> +assigned-clocks = <&clks 200>;
> > >>>> +assigned-clock-rates = <24000000>;
> > >>>>
> > >>>> vdddo-supply = <&camera_dovdd_1v8>;
> > >>>> vdda-supply = <&camera_avdd_2v8>;
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
@ 2020-03-18 22:37 ` Lad, Prabhakar
0 siblings, 0 replies; 58+ messages in thread
From: Lad, Prabhakar @ 2020-03-18 22:37 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mark Rutland, devicetree, Pengutronix Kernel Team,
Ezequiel Garcia, Sascha Hauer, Magnus Damm,
Prabhakar Mahadev Lad, linux-kernel, linux-renesas-soc,
Rob Herring, Geert Uytterhoeven, NXP Linux Team, Sakari Ailus,
Shawn Guo, Mauro Carvalho Chehab, Fabio Estevam,
linux-arm-kernel, linux-media
Hi Laurent,
On Wed, Mar 18, 2020 at 10:33 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> On Wed, Mar 18, 2020 at 08:13:00PM +0000, Lad, Prabhakar wrote:
> > On Fri, Mar 13, 2020 at 9:27 PM Laurent Pinchart wrote:
> > > On Fri, Mar 13, 2020 at 09:25:01PM +0000, Prabhakar Mahadev Lad wrote:
> > >> On Sent: 13 March 2020 21:20, Laurent Pinchart wrote:
> > >>> On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > >>>> Use assigned-clock-rates to specify the clock rate. Also mark
> > >>>> clock-frequency property as deprecated.
> > >>>
> > >>> I would phrase it the other way around, this patch mainly deprecates clock-
> > >>> frequency, and as a side effect recommends usage of assigned-clock-rates.
> > >>>
> > >>> "Deprecate usage of the clock-frequency propertly. The preferred method
> > >>> to set clock rates is to use assigned-clock-rates."
> > >>
> > >> Agreed will do that.
> > >>
> > >>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >>>> ---
> > >>>> Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > >>>> 1 file changed, 3 insertions(+), 2 deletions(-)
> > >>>>
> > >>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > >>>> b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > >>>> index 72ad992..e62fe82 100644
> > >>>> --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > >>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > >>>> @@ -8,7 +8,7 @@ Required Properties:
> > >>>> - compatible: Value should be "ovti,ov5645".
> > >>>> - clocks: Reference to the xclk clock.
> > >>>> - clock-names: Should be "xclk".
> > >>>> -- clock-frequency: Frequency of the xclk clock.
> > >>>> +- clock-frequency (deprecated): Frequency of the xclk clock.
> > >>>
> > >>> I would drop this completely. Drivers need to ensure backward compatibility,
> > >>> but DT bindings should only document the latest version, the history is
> > >>> available in git.
> > >>
> > >> Sure will drop it.
> > >>
> > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >>>
> > >>> While at it, can I enlist you to convert these bindings to yaml ? :-)
> > >>>
> > >> Sure will do the honours , will make sure yaml patch is ontop of this patch too.
> >
> > Shall I enlist you as the maintainer in the json-schema ?
> > dt_binding_check says 'maintainers' is a required property.
>
> Do you want to be the new maintainer ? :-) Sakari is maintaining sensor
> drivers (in his spare time though) so maybe he could be listed in the DT
> bindings too if he wants. Otherwise, I could do it.
>
OK I will add myself and Sakari for now and post a v4.
Cheers,
--Prabhakar
> > > Thank you :-)
> > >
> > >>>> - 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
> > >>>> @@ -37,7 +37,8 @@ Example:
> > >>>>
> > >>>> clocks = <&clks 200>;
> > >>>> clock-names = "xclk";
> > >>>> -clock-frequency = <24000000>;
> > >>>> +assigned-clocks = <&clks 200>;
> > >>>> +assigned-clock-rates = <24000000>;
> > >>>>
> > >>>> vdddo-supply = <&camera_dovdd_1v8>;
> > >>>> vdda-supply = <&camera_avdd_2v8>;
>
> --
> Regards,
>
> Laurent Pinchart
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
2020-03-13 21:12 ` Lad Prabhakar
@ 2020-03-19 12:44 ` Maxime Ripard
-1 siblings, 0 replies; 58+ messages in thread
From: Maxime Ripard @ 2020-03-19 12:44 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus,
NXP Linux Team, Magnus Damm, Ezequiel Garcia, Laurent Pinchart,
Geert Uytterhoeven, devicetree, linux-kernel, linux-renesas-soc,
Fabio Estevam, linux-media, linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1913 bytes --]
Hi,
On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> Use assigned-clock-rates to specify the clock rate. Also mark
> clock-frequency property as deprecated.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> index 72ad992..e62fe82 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> @@ -8,7 +8,7 @@ Required Properties:
> - compatible: Value should be "ovti,ov5645".
> - clocks: Reference to the xclk clock.
> - clock-names: Should be "xclk".
> -- clock-frequency: Frequency of the xclk clock.
> +- clock-frequency (deprecated): Frequency of the xclk clock.
> - 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
> @@ -37,7 +37,8 @@ Example:
>
> clocks = <&clks 200>;
> clock-names = "xclk";
> - clock-frequency = <24000000>;
> + assigned-clocks = <&clks 200>;
> + assigned-clock-rates = <24000000>;
>
> vdddo-supply = <&camera_dovdd_1v8>;
> vdda-supply = <&camera_avdd_2v8>;
clock-frequency is quite different from assigned-clock-rates though,
semantically speaking. clock-frequency is only about what the clock
frequency is, while assigned-clock-rates will change the rate as well,
and you have no idea how long it will last.
If you want to retrieve that through the clock framework, then just
making clock-frequency optional is enough and falling back to
clk_get_rate on the clocks property already provided is enough.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
@ 2020-03-19 12:44 ` Maxime Ripard
0 siblings, 0 replies; 58+ messages in thread
From: Maxime Ripard @ 2020-03-19 12:44 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Mark Rutland, devicetree, Fabio Estevam, Sakari Ailus,
Ezequiel Garcia, Sascha Hauer, Magnus Damm, linux-kernel,
linux-renesas-soc, Rob Herring, Geert Uytterhoeven,
NXP Linux Team, Pengutronix Kernel Team, Mauro Carvalho Chehab,
Shawn Guo, linux-media, linux-arm-kernel, Laurent Pinchart
[-- Attachment #1.1: Type: text/plain, Size: 1913 bytes --]
Hi,
On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> Use assigned-clock-rates to specify the clock rate. Also mark
> clock-frequency property as deprecated.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> index 72ad992..e62fe82 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> @@ -8,7 +8,7 @@ Required Properties:
> - compatible: Value should be "ovti,ov5645".
> - clocks: Reference to the xclk clock.
> - clock-names: Should be "xclk".
> -- clock-frequency: Frequency of the xclk clock.
> +- clock-frequency (deprecated): Frequency of the xclk clock.
> - 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
> @@ -37,7 +37,8 @@ Example:
>
> clocks = <&clks 200>;
> clock-names = "xclk";
> - clock-frequency = <24000000>;
> + assigned-clocks = <&clks 200>;
> + assigned-clock-rates = <24000000>;
>
> vdddo-supply = <&camera_dovdd_1v8>;
> vdda-supply = <&camera_avdd_2v8>;
clock-frequency is quite different from assigned-clock-rates though,
semantically speaking. clock-frequency is only about what the clock
frequency is, while assigned-clock-rates will change the rate as well,
and you have no idea how long it will last.
If you want to retrieve that through the clock framework, then just
making clock-frequency optional is enough and falling back to
clk_get_rate on the clocks property already provided is enough.
Maxime
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
2020-03-19 12:44 ` Maxime Ripard
@ 2020-03-19 13:03 ` Laurent Pinchart
-1 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-03-19 13:03 UTC (permalink / raw)
To: Maxime Ripard
Cc: Lad Prabhakar, Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus,
NXP Linux Team, Magnus Damm, Ezequiel Garcia, Geert Uytterhoeven,
devicetree, linux-kernel, linux-renesas-soc, Fabio Estevam,
linux-media, linux-arm-kernel
Hi Maxime,
On Thu, Mar 19, 2020 at 01:44:52PM +0100, Maxime Ripard wrote:
> On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > Use assigned-clock-rates to specify the clock rate. Also mark
> > clock-frequency property as deprecated.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > index 72ad992..e62fe82 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > @@ -8,7 +8,7 @@ Required Properties:
> > - compatible: Value should be "ovti,ov5645".
> > - clocks: Reference to the xclk clock.
> > - clock-names: Should be "xclk".
> > -- clock-frequency: Frequency of the xclk clock.
> > +- clock-frequency (deprecated): Frequency of the xclk clock.
> > - 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
> > @@ -37,7 +37,8 @@ Example:
> >
> > clocks = <&clks 200>;
> > clock-names = "xclk";
> > - clock-frequency = <24000000>;
> > + assigned-clocks = <&clks 200>;
> > + assigned-clock-rates = <24000000>;
> >
> > vdddo-supply = <&camera_dovdd_1v8>;
> > vdda-supply = <&camera_avdd_2v8>;
>
> clock-frequency is quite different from assigned-clock-rates though,
> semantically speaking. clock-frequency is only about what the clock
> frequency is, while assigned-clock-rates will change the rate as well,
> and you have no idea how long it will last.
The driver currently reads the clock-frequency property and then calls
clk_set_rate(). I agree tht assigned-clock-rates isn't a panacea, but I
think it's less of a hack than what we currently have.
As discussed on IRC, maybe the best option in this specific case is to
drop clock-frequency and assigned-clok-rates, and call clk_set_rate()
with a hardcoded frequency of 24MHz in the driver, as that's the only
frequency the driver supports.
> If you want to retrieve that through the clock framework, then just
> making clock-frequency optional is enough and falling back to
> clk_get_rate on the clocks property already provided is enough.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
@ 2020-03-19 13:03 ` Laurent Pinchart
0 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-03-19 13:03 UTC (permalink / raw)
To: Maxime Ripard
Cc: Mark Rutland, devicetree, Fabio Estevam, Sakari Ailus,
Ezequiel Garcia, Sascha Hauer, Magnus Damm, Lad Prabhakar,
linux-kernel, linux-renesas-soc, Rob Herring, Geert Uytterhoeven,
NXP Linux Team, Pengutronix Kernel Team, Mauro Carvalho Chehab,
Shawn Guo, linux-arm-kernel, linux-media
Hi Maxime,
On Thu, Mar 19, 2020 at 01:44:52PM +0100, Maxime Ripard wrote:
> On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > Use assigned-clock-rates to specify the clock rate. Also mark
> > clock-frequency property as deprecated.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > index 72ad992..e62fe82 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > @@ -8,7 +8,7 @@ Required Properties:
> > - compatible: Value should be "ovti,ov5645".
> > - clocks: Reference to the xclk clock.
> > - clock-names: Should be "xclk".
> > -- clock-frequency: Frequency of the xclk clock.
> > +- clock-frequency (deprecated): Frequency of the xclk clock.
> > - 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
> > @@ -37,7 +37,8 @@ Example:
> >
> > clocks = <&clks 200>;
> > clock-names = "xclk";
> > - clock-frequency = <24000000>;
> > + assigned-clocks = <&clks 200>;
> > + assigned-clock-rates = <24000000>;
> >
> > vdddo-supply = <&camera_dovdd_1v8>;
> > vdda-supply = <&camera_avdd_2v8>;
>
> clock-frequency is quite different from assigned-clock-rates though,
> semantically speaking. clock-frequency is only about what the clock
> frequency is, while assigned-clock-rates will change the rate as well,
> and you have no idea how long it will last.
The driver currently reads the clock-frequency property and then calls
clk_set_rate(). I agree tht assigned-clock-rates isn't a panacea, but I
think it's less of a hack than what we currently have.
As discussed on IRC, maybe the best option in this specific case is to
drop clock-frequency and assigned-clok-rates, and call clk_set_rate()
with a hardcoded frequency of 24MHz in the driver, as that's the only
frequency the driver supports.
> If you want to retrieve that through the clock framework, then just
> making clock-frequency optional is enough and falling back to
> clk_get_rate on the clocks property already provided is enough.
--
Regards,
Laurent Pinchart
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
2020-03-19 13:03 ` Laurent Pinchart
@ 2020-03-19 13:17 ` Lad, Prabhakar
-1 siblings, 0 replies; 58+ messages in thread
From: Lad, Prabhakar @ 2020-03-19 13:17 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Maxime Ripard, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Fabio Estevam, Sakari Ailus, Ezequiel Garcia, Sascha Hauer,
Magnus Damm, Lad Prabhakar, LKML, Linux-Renesas, Rob Herring,
Geert Uytterhoeven, NXP Linux Team, Pengutronix Kernel Team,
Mauro Carvalho Chehab, Shawn Guo, LAK, linux-media
Hi Laurent,
On Thu, Mar 19, 2020 at 1:04 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Maxime,
>
> On Thu, Mar 19, 2020 at 01:44:52PM +0100, Maxime Ripard wrote:
> > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > > Use assigned-clock-rates to specify the clock rate. Also mark
> > > clock-frequency property as deprecated.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > index 72ad992..e62fe82 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > @@ -8,7 +8,7 @@ Required Properties:
> > > - compatible: Value should be "ovti,ov5645".
> > > - clocks: Reference to the xclk clock.
> > > - clock-names: Should be "xclk".
> > > -- clock-frequency: Frequency of the xclk clock.
> > > +- clock-frequency (deprecated): Frequency of the xclk clock.
> > > - 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
> > > @@ -37,7 +37,8 @@ Example:
> > >
> > > clocks = <&clks 200>;
> > > clock-names = "xclk";
> > > - clock-frequency = <24000000>;
> > > + assigned-clocks = <&clks 200>;
> > > + assigned-clock-rates = <24000000>;
> > >
> > > vdddo-supply = <&camera_dovdd_1v8>;
> > > vdda-supply = <&camera_avdd_2v8>;
> >
> > clock-frequency is quite different from assigned-clock-rates though,
> > semantically speaking. clock-frequency is only about what the clock
> > frequency is, while assigned-clock-rates will change the rate as well,
> > and you have no idea how long it will last.
>
> The driver currently reads the clock-frequency property and then calls
> clk_set_rate(). I agree tht assigned-clock-rates isn't a panacea, but I
> think it's less of a hack than what we currently have.
>
> As discussed on IRC, maybe the best option in this specific case is to
> drop clock-frequency and assigned-clok-rates, and call clk_set_rate()
> with a hardcoded frequency of 24MHz in the driver, as that's the only
> frequency the driver supports.
>
Does this mean any driver which has a fixed clock requirement shouldn't be a
DT property and should be just handled by the drivers internally ?
Cheers,
--Prabhakar
> > If you want to retrieve that through the clock framework, then just
> > making clock-frequency optional is enough and falling back to
> > clk_get_rate on the clocks property already provided is enough.
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
@ 2020-03-19 13:17 ` Lad, Prabhakar
0 siblings, 0 replies; 58+ messages in thread
From: Lad, Prabhakar @ 2020-03-19 13:17 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Pengutronix Kernel Team, Ezequiel Garcia, Sascha Hauer,
Magnus Damm, Lad Prabhakar, LKML, Linux-Renesas, Rob Herring,
Geert Uytterhoeven, Maxime Ripard, Sakari Ailus, Shawn Guo,
Mauro Carvalho Chehab, Fabio Estevam, linux-media, LAK,
NXP Linux Team
Hi Laurent,
On Thu, Mar 19, 2020 at 1:04 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Maxime,
>
> On Thu, Mar 19, 2020 at 01:44:52PM +0100, Maxime Ripard wrote:
> > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > > Use assigned-clock-rates to specify the clock rate. Also mark
> > > clock-frequency property as deprecated.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > index 72ad992..e62fe82 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > @@ -8,7 +8,7 @@ Required Properties:
> > > - compatible: Value should be "ovti,ov5645".
> > > - clocks: Reference to the xclk clock.
> > > - clock-names: Should be "xclk".
> > > -- clock-frequency: Frequency of the xclk clock.
> > > +- clock-frequency (deprecated): Frequency of the xclk clock.
> > > - 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
> > > @@ -37,7 +37,8 @@ Example:
> > >
> > > clocks = <&clks 200>;
> > > clock-names = "xclk";
> > > - clock-frequency = <24000000>;
> > > + assigned-clocks = <&clks 200>;
> > > + assigned-clock-rates = <24000000>;
> > >
> > > vdddo-supply = <&camera_dovdd_1v8>;
> > > vdda-supply = <&camera_avdd_2v8>;
> >
> > clock-frequency is quite different from assigned-clock-rates though,
> > semantically speaking. clock-frequency is only about what the clock
> > frequency is, while assigned-clock-rates will change the rate as well,
> > and you have no idea how long it will last.
>
> The driver currently reads the clock-frequency property and then calls
> clk_set_rate(). I agree tht assigned-clock-rates isn't a panacea, but I
> think it's less of a hack than what we currently have.
>
> As discussed on IRC, maybe the best option in this specific case is to
> drop clock-frequency and assigned-clok-rates, and call clk_set_rate()
> with a hardcoded frequency of 24MHz in the driver, as that's the only
> frequency the driver supports.
>
Does this mean any driver which has a fixed clock requirement shouldn't be a
DT property and should be just handled by the drivers internally ?
Cheers,
--Prabhakar
> > If you want to retrieve that through the clock framework, then just
> > making clock-frequency optional is enough and falling back to
> > clk_get_rate on the clocks property already provided is enough.
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
2020-03-19 13:17 ` Lad, Prabhakar
@ 2020-03-24 15:40 ` Maxime Ripard
-1 siblings, 0 replies; 58+ messages in thread
From: Maxime Ripard @ 2020-03-24 15:40 UTC (permalink / raw)
To: Lad, Prabhakar
Cc: Laurent Pinchart, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Fabio Estevam, Sakari Ailus, Ezequiel Garcia, Sascha Hauer,
Magnus Damm, Lad Prabhakar, LKML, Linux-Renesas, Rob Herring,
Geert Uytterhoeven, NXP Linux Team, Pengutronix Kernel Team,
Mauro Carvalho Chehab, Shawn Guo, LAK, linux-media
[-- Attachment #1: Type: text/plain, Size: 3288 bytes --]
On Thu, Mar 19, 2020 at 01:17:51PM +0000, Lad, Prabhakar wrote:
> Hi Laurent,
>
> On Thu, Mar 19, 2020 at 1:04 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Maxime,
> >
> > On Thu, Mar 19, 2020 at 01:44:52PM +0100, Maxime Ripard wrote:
> > > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > > > Use assigned-clock-rates to specify the clock rate. Also mark
> > > > clock-frequency property as deprecated.
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > index 72ad992..e62fe82 100644
> > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > @@ -8,7 +8,7 @@ Required Properties:
> > > > - compatible: Value should be "ovti,ov5645".
> > > > - clocks: Reference to the xclk clock.
> > > > - clock-names: Should be "xclk".
> > > > -- clock-frequency: Frequency of the xclk clock.
> > > > +- clock-frequency (deprecated): Frequency of the xclk clock.
> > > > - 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
> > > > @@ -37,7 +37,8 @@ Example:
> > > >
> > > > clocks = <&clks 200>;
> > > > clock-names = "xclk";
> > > > - clock-frequency = <24000000>;
> > > > + assigned-clocks = <&clks 200>;
> > > > + assigned-clock-rates = <24000000>;
> > > >
> > > > vdddo-supply = <&camera_dovdd_1v8>;
> > > > vdda-supply = <&camera_avdd_2v8>;
> > >
> > > clock-frequency is quite different from assigned-clock-rates though,
> > > semantically speaking. clock-frequency is only about what the clock
> > > frequency is, while assigned-clock-rates will change the rate as well,
> > > and you have no idea how long it will last.
> >
> > The driver currently reads the clock-frequency property and then calls
> > clk_set_rate(). I agree tht assigned-clock-rates isn't a panacea, but I
> > think it's less of a hack than what we currently have.
> >
> > As discussed on IRC, maybe the best option in this specific case is to
> > drop clock-frequency and assigned-clok-rates, and call clk_set_rate()
> > with a hardcoded frequency of 24MHz in the driver, as that's the only
> > frequency the driver supports.
> >
> Does this mean any driver which has a fixed clock requirement shouldn't be a
> DT property and should be just handled by the drivers internally ?
It's hard to give a generic policy, but here, the hardware is pretty
flexible since it can deal with anything between 6MHz to 50-something
MHz, it's the driver that chooses to enforce a 24MHz and be pedantic
about it, so it's up to the driver to enforce that policy, not to the
DT since it's essentially a software limitation, not a hardware one.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
@ 2020-03-24 15:40 ` Maxime Ripard
0 siblings, 0 replies; 58+ messages in thread
From: Maxime Ripard @ 2020-03-24 15:40 UTC (permalink / raw)
To: Lad, Prabhakar
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Pengutronix Kernel Team, Ezequiel Garcia, Sascha Hauer,
Magnus Damm, Lad Prabhakar, LKML, Linux-Renesas, Rob Herring,
Geert Uytterhoeven, Laurent Pinchart, Sakari Ailus, Shawn Guo,
Mauro Carvalho Chehab, Fabio Estevam, linux-media, LAK,
NXP Linux Team
[-- Attachment #1.1: Type: text/plain, Size: 3288 bytes --]
On Thu, Mar 19, 2020 at 01:17:51PM +0000, Lad, Prabhakar wrote:
> Hi Laurent,
>
> On Thu, Mar 19, 2020 at 1:04 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Maxime,
> >
> > On Thu, Mar 19, 2020 at 01:44:52PM +0100, Maxime Ripard wrote:
> > > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > > > Use assigned-clock-rates to specify the clock rate. Also mark
> > > > clock-frequency property as deprecated.
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > index 72ad992..e62fe82 100644
> > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > @@ -8,7 +8,7 @@ Required Properties:
> > > > - compatible: Value should be "ovti,ov5645".
> > > > - clocks: Reference to the xclk clock.
> > > > - clock-names: Should be "xclk".
> > > > -- clock-frequency: Frequency of the xclk clock.
> > > > +- clock-frequency (deprecated): Frequency of the xclk clock.
> > > > - 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
> > > > @@ -37,7 +37,8 @@ Example:
> > > >
> > > > clocks = <&clks 200>;
> > > > clock-names = "xclk";
> > > > - clock-frequency = <24000000>;
> > > > + assigned-clocks = <&clks 200>;
> > > > + assigned-clock-rates = <24000000>;
> > > >
> > > > vdddo-supply = <&camera_dovdd_1v8>;
> > > > vdda-supply = <&camera_avdd_2v8>;
> > >
> > > clock-frequency is quite different from assigned-clock-rates though,
> > > semantically speaking. clock-frequency is only about what the clock
> > > frequency is, while assigned-clock-rates will change the rate as well,
> > > and you have no idea how long it will last.
> >
> > The driver currently reads the clock-frequency property and then calls
> > clk_set_rate(). I agree tht assigned-clock-rates isn't a panacea, but I
> > think it's less of a hack than what we currently have.
> >
> > As discussed on IRC, maybe the best option in this specific case is to
> > drop clock-frequency and assigned-clok-rates, and call clk_set_rate()
> > with a hardcoded frequency of 24MHz in the driver, as that's the only
> > frequency the driver supports.
> >
> Does this mean any driver which has a fixed clock requirement shouldn't be a
> DT property and should be just handled by the drivers internally ?
It's hard to give a generic policy, but here, the hardware is pretty
flexible since it can deal with anything between 6MHz to 50-something
MHz, it's the driver that chooses to enforce a 24MHz and be pedantic
about it, so it's up to the driver to enforce that policy, not to the
DT since it's essentially a software limitation, not a hardware one.
Maxime
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
2020-03-24 15:40 ` Maxime Ripard
@ 2020-03-24 16:04 ` Lad, Prabhakar
-1 siblings, 0 replies; 58+ messages in thread
From: Lad, Prabhakar @ 2020-03-24 16:04 UTC (permalink / raw)
To: Maxime Ripard
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Pengutronix Kernel Team, Ezequiel Garcia, Sascha Hauer,
Magnus Damm, Lad Prabhakar, LKML, Linux-Renesas, Rob Herring,
Geert Uytterhoeven, Laurent Pinchart, Sakari Ailus, Shawn Guo,
Mauro Carvalho Chehab, Fabio Estevam, linux-media, LAK,
NXP Linux Team
Hi Maxime,
On Tue, Mar 24, 2020 at 3:40 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Thu, Mar 19, 2020 at 01:17:51PM +0000, Lad, Prabhakar wrote:
> > Hi Laurent,
> >
> > On Thu, Mar 19, 2020 at 1:04 PM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > Hi Maxime,
> > >
> > > On Thu, Mar 19, 2020 at 01:44:52PM +0100, Maxime Ripard wrote:
> > > > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > > > > Use assigned-clock-rates to specify the clock rate. Also mark
> > > > > clock-frequency property as deprecated.
> > > > >
> > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > ---
> > > > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > > index 72ad992..e62fe82 100644
> > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > > @@ -8,7 +8,7 @@ Required Properties:
> > > > > - compatible: Value should be "ovti,ov5645".
> > > > > - clocks: Reference to the xclk clock.
> > > > > - clock-names: Should be "xclk".
> > > > > -- clock-frequency: Frequency of the xclk clock.
> > > > > +- clock-frequency (deprecated): Frequency of the xclk clock.
> > > > > - 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
> > > > > @@ -37,7 +37,8 @@ Example:
> > > > >
> > > > > clocks = <&clks 200>;
> > > > > clock-names = "xclk";
> > > > > - clock-frequency = <24000000>;
> > > > > + assigned-clocks = <&clks 200>;
> > > > > + assigned-clock-rates = <24000000>;
> > > > >
> > > > > vdddo-supply = <&camera_dovdd_1v8>;
> > > > > vdda-supply = <&camera_avdd_2v8>;
> > > >
> > > > clock-frequency is quite different from assigned-clock-rates though,
> > > > semantically speaking. clock-frequency is only about what the clock
> > > > frequency is, while assigned-clock-rates will change the rate as well,
> > > > and you have no idea how long it will last.
> > >
> > > The driver currently reads the clock-frequency property and then calls
> > > clk_set_rate(). I agree tht assigned-clock-rates isn't a panacea, but I
> > > think it's less of a hack than what we currently have.
> > >
> > > As discussed on IRC, maybe the best option in this specific case is to
> > > drop clock-frequency and assigned-clok-rates, and call clk_set_rate()
> > > with a hardcoded frequency of 24MHz in the driver, as that's the only
> > > frequency the driver supports.
> > >
> > Does this mean any driver which has a fixed clock requirement shouldn't be a
> > DT property and should be just handled by the drivers internally ?
>
> It's hard to give a generic policy, but here, the hardware is pretty
> flexible since it can deal with anything between 6MHz to 50-something
> MHz, it's the driver that chooses to enforce a 24MHz and be pedantic
> about it, so it's up to the driver to enforce that policy, not to the
> DT since it's essentially a software limitation, not a hardware one.
>
Thank you for the clarification, Ill drop patches 1-4 from this series.
Cheers,
--Prabhakar
> Maxime
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
@ 2020-03-24 16:04 ` Lad, Prabhakar
0 siblings, 0 replies; 58+ messages in thread
From: Lad, Prabhakar @ 2020-03-24 16:04 UTC (permalink / raw)
To: Maxime Ripard
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Fabio Estevam, Sakari Ailus, Shawn Guo, Sascha Hauer,
Magnus Damm, Lad Prabhakar, LKML, Linux-Renesas, Rob Herring,
Geert Uytterhoeven, Laurent Pinchart, Pengutronix Kernel Team,
NXP Linux Team, Mauro Carvalho Chehab, Ezequiel Garcia, LAK,
linux-media
Hi Maxime,
On Tue, Mar 24, 2020 at 3:40 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Thu, Mar 19, 2020 at 01:17:51PM +0000, Lad, Prabhakar wrote:
> > Hi Laurent,
> >
> > On Thu, Mar 19, 2020 at 1:04 PM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > Hi Maxime,
> > >
> > > On Thu, Mar 19, 2020 at 01:44:52PM +0100, Maxime Ripard wrote:
> > > > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > > > > Use assigned-clock-rates to specify the clock rate. Also mark
> > > > > clock-frequency property as deprecated.
> > > > >
> > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > ---
> > > > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > > index 72ad992..e62fe82 100644
> > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > > @@ -8,7 +8,7 @@ Required Properties:
> > > > > - compatible: Value should be "ovti,ov5645".
> > > > > - clocks: Reference to the xclk clock.
> > > > > - clock-names: Should be "xclk".
> > > > > -- clock-frequency: Frequency of the xclk clock.
> > > > > +- clock-frequency (deprecated): Frequency of the xclk clock.
> > > > > - 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
> > > > > @@ -37,7 +37,8 @@ Example:
> > > > >
> > > > > clocks = <&clks 200>;
> > > > > clock-names = "xclk";
> > > > > - clock-frequency = <24000000>;
> > > > > + assigned-clocks = <&clks 200>;
> > > > > + assigned-clock-rates = <24000000>;
> > > > >
> > > > > vdddo-supply = <&camera_dovdd_1v8>;
> > > > > vdda-supply = <&camera_avdd_2v8>;
> > > >
> > > > clock-frequency is quite different from assigned-clock-rates though,
> > > > semantically speaking. clock-frequency is only about what the clock
> > > > frequency is, while assigned-clock-rates will change the rate as well,
> > > > and you have no idea how long it will last.
> > >
> > > The driver currently reads the clock-frequency property and then calls
> > > clk_set_rate(). I agree tht assigned-clock-rates isn't a panacea, but I
> > > think it's less of a hack than what we currently have.
> > >
> > > As discussed on IRC, maybe the best option in this specific case is to
> > > drop clock-frequency and assigned-clok-rates, and call clk_set_rate()
> > > with a hardcoded frequency of 24MHz in the driver, as that's the only
> > > frequency the driver supports.
> > >
> > Does this mean any driver which has a fixed clock requirement shouldn't be a
> > DT property and should be just handled by the drivers internally ?
>
> It's hard to give a generic policy, but here, the hardware is pretty
> flexible since it can deal with anything between 6MHz to 50-something
> MHz, it's the driver that chooses to enforce a 24MHz and be pedantic
> about it, so it's up to the driver to enforce that policy, not to the
> DT since it's essentially a software limitation, not a hardware one.
>
Thank you for the clarification, Ill drop patches 1-4 from this series.
Cheers,
--Prabhakar
> Maxime
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
2020-03-24 16:04 ` Lad, Prabhakar
@ 2020-03-24 16:12 ` Laurent Pinchart
-1 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-03-24 16:12 UTC (permalink / raw)
To: Lad, Prabhakar
Cc: Maxime Ripard, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Pengutronix Kernel Team, Ezequiel Garcia, Sascha Hauer,
Magnus Damm, Lad Prabhakar, LKML, Linux-Renesas, Rob Herring,
Geert Uytterhoeven, Sakari Ailus, Shawn Guo,
Mauro Carvalho Chehab, Fabio Estevam, linux-media, LAK,
NXP Linux Team
Hi Prabhakar,
On Tue, Mar 24, 2020 at 04:04:43PM +0000, Lad, Prabhakar wrote:
> On Tue, Mar 24, 2020 at 3:40 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > On Thu, Mar 19, 2020 at 01:17:51PM +0000, Lad, Prabhakar wrote:
> > > On Thu, Mar 19, 2020 at 1:04 PM Laurent Pinchart wrote:
> > > > On Thu, Mar 19, 2020 at 01:44:52PM +0100, Maxime Ripard wrote:
> > > > > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > > > > > Use assigned-clock-rates to specify the clock rate. Also mark
> > > > > > clock-frequency property as deprecated.
> > > > > >
> > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > ---
> > > > > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > > > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > > > index 72ad992..e62fe82 100644
> > > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > > > @@ -8,7 +8,7 @@ Required Properties:
> > > > > > - compatible: Value should be "ovti,ov5645".
> > > > > > - clocks: Reference to the xclk clock.
> > > > > > - clock-names: Should be "xclk".
> > > > > > -- clock-frequency: Frequency of the xclk clock.
> > > > > > +- clock-frequency (deprecated): Frequency of the xclk clock.
> > > > > > - 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
> > > > > > @@ -37,7 +37,8 @@ Example:
> > > > > >
> > > > > > clocks = <&clks 200>;
> > > > > > clock-names = "xclk";
> > > > > > - clock-frequency = <24000000>;
> > > > > > + assigned-clocks = <&clks 200>;
> > > > > > + assigned-clock-rates = <24000000>;
> > > > > >
> > > > > > vdddo-supply = <&camera_dovdd_1v8>;
> > > > > > vdda-supply = <&camera_avdd_2v8>;
> > > > >
> > > > > clock-frequency is quite different from assigned-clock-rates though,
> > > > > semantically speaking. clock-frequency is only about what the clock
> > > > > frequency is, while assigned-clock-rates will change the rate as well,
> > > > > and you have no idea how long it will last.
> > > >
> > > > The driver currently reads the clock-frequency property and then calls
> > > > clk_set_rate(). I agree tht assigned-clock-rates isn't a panacea, but I
> > > > think it's less of a hack than what we currently have.
> > > >
> > > > As discussed on IRC, maybe the best option in this specific case is to
> > > > drop clock-frequency and assigned-clok-rates, and call clk_set_rate()
> > > > with a hardcoded frequency of 24MHz in the driver, as that's the only
> > > > frequency the driver supports.
> > > >
> > > Does this mean any driver which has a fixed clock requirement shouldn't be a
> > > DT property and should be just handled by the drivers internally ?
> >
> > It's hard to give a generic policy, but here, the hardware is pretty
> > flexible since it can deal with anything between 6MHz to 50-something
> > MHz, it's the driver that chooses to enforce a 24MHz and be pedantic
> > about it, so it's up to the driver to enforce that policy, not to the
> > DT since it's essentially a software limitation, not a hardware one.
>
> Thank you for the clarification, Ill drop patches 1-4 from this series.
That's the whole series... :-) I think you should keep patch 1/4 but
just remove the clock-frequency from the bindings, then remove it from
the DT files, and patch the driver to set the clock rate to 24MHz
unconditionally in patch 4/4.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
@ 2020-03-24 16:12 ` Laurent Pinchart
0 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-03-24 16:12 UTC (permalink / raw)
To: Lad, Prabhakar
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Fabio Estevam, Sakari Ailus, Shawn Guo, Sascha Hauer,
Magnus Damm, Lad Prabhakar, LKML, Linux-Renesas, Rob Herring,
Geert Uytterhoeven, Maxime Ripard, Pengutronix Kernel Team,
NXP Linux Team, Mauro Carvalho Chehab, Ezequiel Garcia, LAK,
linux-media
Hi Prabhakar,
On Tue, Mar 24, 2020 at 04:04:43PM +0000, Lad, Prabhakar wrote:
> On Tue, Mar 24, 2020 at 3:40 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > On Thu, Mar 19, 2020 at 01:17:51PM +0000, Lad, Prabhakar wrote:
> > > On Thu, Mar 19, 2020 at 1:04 PM Laurent Pinchart wrote:
> > > > On Thu, Mar 19, 2020 at 01:44:52PM +0100, Maxime Ripard wrote:
> > > > > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > > > > > Use assigned-clock-rates to specify the clock rate. Also mark
> > > > > > clock-frequency property as deprecated.
> > > > > >
> > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > ---
> > > > > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > > > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > > > index 72ad992..e62fe82 100644
> > > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > > > @@ -8,7 +8,7 @@ Required Properties:
> > > > > > - compatible: Value should be "ovti,ov5645".
> > > > > > - clocks: Reference to the xclk clock.
> > > > > > - clock-names: Should be "xclk".
> > > > > > -- clock-frequency: Frequency of the xclk clock.
> > > > > > +- clock-frequency (deprecated): Frequency of the xclk clock.
> > > > > > - 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
> > > > > > @@ -37,7 +37,8 @@ Example:
> > > > > >
> > > > > > clocks = <&clks 200>;
> > > > > > clock-names = "xclk";
> > > > > > - clock-frequency = <24000000>;
> > > > > > + assigned-clocks = <&clks 200>;
> > > > > > + assigned-clock-rates = <24000000>;
> > > > > >
> > > > > > vdddo-supply = <&camera_dovdd_1v8>;
> > > > > > vdda-supply = <&camera_avdd_2v8>;
> > > > >
> > > > > clock-frequency is quite different from assigned-clock-rates though,
> > > > > semantically speaking. clock-frequency is only about what the clock
> > > > > frequency is, while assigned-clock-rates will change the rate as well,
> > > > > and you have no idea how long it will last.
> > > >
> > > > The driver currently reads the clock-frequency property and then calls
> > > > clk_set_rate(). I agree tht assigned-clock-rates isn't a panacea, but I
> > > > think it's less of a hack than what we currently have.
> > > >
> > > > As discussed on IRC, maybe the best option in this specific case is to
> > > > drop clock-frequency and assigned-clok-rates, and call clk_set_rate()
> > > > with a hardcoded frequency of 24MHz in the driver, as that's the only
> > > > frequency the driver supports.
> > > >
> > > Does this mean any driver which has a fixed clock requirement shouldn't be a
> > > DT property and should be just handled by the drivers internally ?
> >
> > It's hard to give a generic policy, but here, the hardware is pretty
> > flexible since it can deal with anything between 6MHz to 50-something
> > MHz, it's the driver that chooses to enforce a 24MHz and be pedantic
> > about it, so it's up to the driver to enforce that policy, not to the
> > DT since it's essentially a software limitation, not a hardware one.
>
> Thank you for the clarification, Ill drop patches 1-4 from this series.
That's the whole series... :-) I think you should keep patch 1/4 but
just remove the clock-frequency from the bindings, then remove it from
the DT files, and patch the driver to set the clock rate to 24MHz
unconditionally in patch 4/4.
--
Regards,
Laurent Pinchart
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
2020-03-24 16:12 ` Laurent Pinchart
@ 2020-03-25 21:52 ` Lad, Prabhakar
-1 siblings, 0 replies; 58+ messages in thread
From: Lad, Prabhakar @ 2020-03-25 21:52 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Maxime Ripard, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Pengutronix Kernel Team, Ezequiel Garcia, Sascha Hauer,
Magnus Damm, Lad Prabhakar, LKML, Linux-Renesas, Rob Herring,
Geert Uytterhoeven, Sakari Ailus, Shawn Guo,
Mauro Carvalho Chehab, Fabio Estevam, linux-media, LAK,
NXP Linux Team
Hi Laurent,
On Tue, Mar 24, 2020 at 4:12 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> On Tue, Mar 24, 2020 at 04:04:43PM +0000, Lad, Prabhakar wrote:
> > On Tue, Mar 24, 2020 at 3:40 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > On Thu, Mar 19, 2020 at 01:17:51PM +0000, Lad, Prabhakar wrote:
> > > > On Thu, Mar 19, 2020 at 1:04 PM Laurent Pinchart wrote:
> > > > > On Thu, Mar 19, 2020 at 01:44:52PM +0100, Maxime Ripard wrote:
> > > > > > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > > > > > > Use assigned-clock-rates to specify the clock rate. Also mark
> > > > > > > clock-frequency property as deprecated.
> > > > > > >
> > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > > ---
> > > > > > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > > > > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > > > > index 72ad992..e62fe82 100644
> > > > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > > > > @@ -8,7 +8,7 @@ Required Properties:
> > > > > > > - compatible: Value should be "ovti,ov5645".
> > > > > > > - clocks: Reference to the xclk clock.
> > > > > > > - clock-names: Should be "xclk".
> > > > > > > -- clock-frequency: Frequency of the xclk clock.
> > > > > > > +- clock-frequency (deprecated): Frequency of the xclk clock.
> > > > > > > - 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
> > > > > > > @@ -37,7 +37,8 @@ Example:
> > > > > > >
> > > > > > > clocks = <&clks 200>;
> > > > > > > clock-names = "xclk";
> > > > > > > - clock-frequency = <24000000>;
> > > > > > > + assigned-clocks = <&clks 200>;
> > > > > > > + assigned-clock-rates = <24000000>;
> > > > > > >
> > > > > > > vdddo-supply = <&camera_dovdd_1v8>;
> > > > > > > vdda-supply = <&camera_avdd_2v8>;
> > > > > >
> > > > > > clock-frequency is quite different from assigned-clock-rates though,
> > > > > > semantically speaking. clock-frequency is only about what the clock
> > > > > > frequency is, while assigned-clock-rates will change the rate as well,
> > > > > > and you have no idea how long it will last.
> > > > >
> > > > > The driver currently reads the clock-frequency property and then calls
> > > > > clk_set_rate(). I agree tht assigned-clock-rates isn't a panacea, but I
> > > > > think it's less of a hack than what we currently have.
> > > > >
> > > > > As discussed on IRC, maybe the best option in this specific case is to
> > > > > drop clock-frequency and assigned-clok-rates, and call clk_set_rate()
> > > > > with a hardcoded frequency of 24MHz in the driver, as that's the only
> > > > > frequency the driver supports.
> > > > >
> > > > Does this mean any driver which has a fixed clock requirement shouldn't be a
> > > > DT property and should be just handled by the drivers internally ?
> > >
> > > It's hard to give a generic policy, but here, the hardware is pretty
> > > flexible since it can deal with anything between 6MHz to 50-something
> > > MHz, it's the driver that chooses to enforce a 24MHz and be pedantic
> > > about it, so it's up to the driver to enforce that policy, not to the
> > > DT since it's essentially a software limitation, not a hardware one.
> >
> > Thank you for the clarification, Ill drop patches 1-4 from this series.
>
> That's the whole series... :-) I think you should keep patch 1/4 but
> just remove the clock-frequency from the bindings, then remove it from
> the DT files, and patch the driver to set the clock rate to 24MHz
> unconditionally in patch 4/4.
>
My bad I was referring to v4 series patch 5/5 which converts dt
bindings to json schema.
I'll shall post a v5 as suggested above.
Cheers,
--Prabhakar
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
@ 2020-03-25 21:52 ` Lad, Prabhakar
0 siblings, 0 replies; 58+ messages in thread
From: Lad, Prabhakar @ 2020-03-25 21:52 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Fabio Estevam, Sakari Ailus, Shawn Guo, Sascha Hauer,
Magnus Damm, Lad Prabhakar, LKML, Linux-Renesas, Rob Herring,
Geert Uytterhoeven, Maxime Ripard, Pengutronix Kernel Team,
NXP Linux Team, Mauro Carvalho Chehab, Ezequiel Garcia, LAK,
linux-media
Hi Laurent,
On Tue, Mar 24, 2020 at 4:12 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> On Tue, Mar 24, 2020 at 04:04:43PM +0000, Lad, Prabhakar wrote:
> > On Tue, Mar 24, 2020 at 3:40 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > On Thu, Mar 19, 2020 at 01:17:51PM +0000, Lad, Prabhakar wrote:
> > > > On Thu, Mar 19, 2020 at 1:04 PM Laurent Pinchart wrote:
> > > > > On Thu, Mar 19, 2020 at 01:44:52PM +0100, Maxime Ripard wrote:
> > > > > > On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > > > > > > Use assigned-clock-rates to specify the clock rate. Also mark
> > > > > > > clock-frequency property as deprecated.
> > > > > > >
> > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > > ---
> > > > > > > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > > > > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > > > > index 72ad992..e62fe82 100644
> > > > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > > > > > > @@ -8,7 +8,7 @@ Required Properties:
> > > > > > > - compatible: Value should be "ovti,ov5645".
> > > > > > > - clocks: Reference to the xclk clock.
> > > > > > > - clock-names: Should be "xclk".
> > > > > > > -- clock-frequency: Frequency of the xclk clock.
> > > > > > > +- clock-frequency (deprecated): Frequency of the xclk clock.
> > > > > > > - 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
> > > > > > > @@ -37,7 +37,8 @@ Example:
> > > > > > >
> > > > > > > clocks = <&clks 200>;
> > > > > > > clock-names = "xclk";
> > > > > > > - clock-frequency = <24000000>;
> > > > > > > + assigned-clocks = <&clks 200>;
> > > > > > > + assigned-clock-rates = <24000000>;
> > > > > > >
> > > > > > > vdddo-supply = <&camera_dovdd_1v8>;
> > > > > > > vdda-supply = <&camera_avdd_2v8>;
> > > > > >
> > > > > > clock-frequency is quite different from assigned-clock-rates though,
> > > > > > semantically speaking. clock-frequency is only about what the clock
> > > > > > frequency is, while assigned-clock-rates will change the rate as well,
> > > > > > and you have no idea how long it will last.
> > > > >
> > > > > The driver currently reads the clock-frequency property and then calls
> > > > > clk_set_rate(). I agree tht assigned-clock-rates isn't a panacea, but I
> > > > > think it's less of a hack than what we currently have.
> > > > >
> > > > > As discussed on IRC, maybe the best option in this specific case is to
> > > > > drop clock-frequency and assigned-clok-rates, and call clk_set_rate()
> > > > > with a hardcoded frequency of 24MHz in the driver, as that's the only
> > > > > frequency the driver supports.
> > > > >
> > > > Does this mean any driver which has a fixed clock requirement shouldn't be a
> > > > DT property and should be just handled by the drivers internally ?
> > >
> > > It's hard to give a generic policy, but here, the hardware is pretty
> > > flexible since it can deal with anything between 6MHz to 50-something
> > > MHz, it's the driver that chooses to enforce a 24MHz and be pedantic
> > > about it, so it's up to the driver to enforce that policy, not to the
> > > DT since it's essentially a software limitation, not a hardware one.
> >
> > Thank you for the clarification, Ill drop patches 1-4 from this series.
>
> That's the whole series... :-) I think you should keep patch 1/4 but
> just remove the clock-frequency from the bindings, then remove it from
> the DT files, and patch the driver to set the clock rate to 24MHz
> unconditionally in patch 4/4.
>
My bad I was referring to v4 series patch 5/5 which converts dt
bindings to json schema.
I'll shall post a v5 as suggested above.
Cheers,
--Prabhakar
> --
> Regards,
>
> Laurent Pinchart
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
2020-03-19 12:44 ` Maxime Ripard
@ 2020-03-19 13:05 ` Lad, Prabhakar
-1 siblings, 0 replies; 58+ messages in thread
From: Lad, Prabhakar @ 2020-03-19 13:05 UTC (permalink / raw)
To: Maxime Ripard, Geert Uytterhoeven
Cc: Lad Prabhakar, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Fabio Estevam, Sakari Ailus, Ezequiel Garcia, Sascha Hauer,
Magnus Damm, LKML, Linux-Renesas, Rob Herring,
Geert Uytterhoeven, NXP Linux Team, Pengutronix Kernel Team,
Mauro Carvalho Chehab, Shawn Guo, linux-media, LAK,
Laurent Pinchart
Hi Maxime,
Thank you for the review.
On Thu, Mar 19, 2020 at 12:45 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi,
>
> On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > Use assigned-clock-rates to specify the clock rate. Also mark
> > clock-frequency property as deprecated.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > index 72ad992..e62fe82 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > @@ -8,7 +8,7 @@ Required Properties:
> > - compatible: Value should be "ovti,ov5645".
> > - clocks: Reference to the xclk clock.
> > - clock-names: Should be "xclk".
> > -- clock-frequency: Frequency of the xclk clock.
> > +- clock-frequency (deprecated): Frequency of the xclk clock.
> > - 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
> > @@ -37,7 +37,8 @@ Example:
> >
> > clocks = <&clks 200>;
> > clock-names = "xclk";
> > - clock-frequency = <24000000>;
> > + assigned-clocks = <&clks 200>;
> > + assigned-clock-rates = <24000000>;
> >
> > vdddo-supply = <&camera_dovdd_1v8>;
> > vdda-supply = <&camera_avdd_2v8>;
>
> clock-frequency is quite different from assigned-clock-rates though,
> semantically speaking. clock-frequency is only about what the clock
> frequency is, while assigned-clock-rates will change the rate as well,
> and you have no idea how long it will last.
>
Agreed clock-frequency tells whats the clock frequency, wrt ov5645 driver
this property was read and and the clock rate was changed accordingly as per
the value being passed. So switching to assigned-clock-rates does bypass
of clock rate being set in the ov5645 driver [1] as the framework does it.
> If you want to retrieve that through the clock framework, then just
> making clock-frequency optional is enough and falling back to
> clk_get_rate on the clocks property already provided is enough.
>
As done in patch [1] ?
Fyi I have posted a v4 [2] to ML.
[1] https://patchwork.linuxtv.org/patch/62378/
[2] https://patchwork.linuxtv.org/project/linux-media/list/?series=1990
Cheers,
--Prabhakar
> Maxime
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 1/4] media: dt-bindings: media: i2c: Switch to assigned-clock-rates
@ 2020-03-19 13:05 ` Lad, Prabhakar
0 siblings, 0 replies; 58+ messages in thread
From: Lad, Prabhakar @ 2020-03-19 13:05 UTC (permalink / raw)
To: Maxime Ripard, Geert Uytterhoeven
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Laurent Pinchart, Pengutronix Kernel Team, Ezequiel Garcia,
Sascha Hauer, Magnus Damm, Lad Prabhakar, LKML, Linux-Renesas,
Rob Herring, Geert Uytterhoeven, NXP Linux Team, Sakari Ailus,
Shawn Guo, Mauro Carvalho Chehab, Fabio Estevam, LAK,
linux-media
Hi Maxime,
Thank you for the review.
On Thu, Mar 19, 2020 at 12:45 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi,
>
> On Fri, Mar 13, 2020 at 09:12:31PM +0000, Lad Prabhakar wrote:
> > Use assigned-clock-rates to specify the clock rate. Also mark
> > clock-frequency property as deprecated.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > Documentation/devicetree/bindings/media/i2c/ov5645.txt | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > index 72ad992..e62fe82 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt
> > @@ -8,7 +8,7 @@ Required Properties:
> > - compatible: Value should be "ovti,ov5645".
> > - clocks: Reference to the xclk clock.
> > - clock-names: Should be "xclk".
> > -- clock-frequency: Frequency of the xclk clock.
> > +- clock-frequency (deprecated): Frequency of the xclk clock.
> > - 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
> > @@ -37,7 +37,8 @@ Example:
> >
> > clocks = <&clks 200>;
> > clock-names = "xclk";
> > - clock-frequency = <24000000>;
> > + assigned-clocks = <&clks 200>;
> > + assigned-clock-rates = <24000000>;
> >
> > vdddo-supply = <&camera_dovdd_1v8>;
> > vdda-supply = <&camera_avdd_2v8>;
>
> clock-frequency is quite different from assigned-clock-rates though,
> semantically speaking. clock-frequency is only about what the clock
> frequency is, while assigned-clock-rates will change the rate as well,
> and you have no idea how long it will last.
>
Agreed clock-frequency tells whats the clock frequency, wrt ov5645 driver
this property was read and and the clock rate was changed accordingly as per
the value being passed. So switching to assigned-clock-rates does bypass
of clock rate being set in the ov5645 driver [1] as the framework does it.
> If you want to retrieve that through the clock framework, then just
> making clock-frequency optional is enough and falling back to
> clk_get_rate on the clocks property already provided is enough.
>
As done in patch [1] ?
Fyi I have posted a v4 [2] to ML.
[1] https://patchwork.linuxtv.org/patch/62378/
[2] https://patchwork.linuxtv.org/project/linux-media/list/?series=1990
Cheers,
--Prabhakar
> Maxime
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v3 2/4] media: i2c: ov5645: Switch to assigned-clock-rates
2020-03-13 21:12 ` Lad Prabhakar
@ 2020-03-13 21:12 ` Lad Prabhakar
-1 siblings, 0 replies; 58+ messages in thread
From: Lad Prabhakar @ 2020-03-13 21:12 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus,
NXP Linux Team, Magnus Damm, Ezequiel Garcia, Laurent Pinchart,
Geert Uytterhoeven
Cc: devicetree, linux-kernel, linux-renesas-soc, Lad Prabhakar,
Fabio Estevam, linux-media, linux-arm-kernel
This patch switches to assigned-clock-rates for specifying the clock rate.
The clk-conf.c internally handles setting the clock rate when
assigned-clock-rates is passed.
The driver now sets the clock frequency only if the deprecated property
clock-frequency is defined instead assigned-clock-rates, this is to avoid
breakage with existing DT binaries.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
drivers/media/i2c/ov5645.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index a6c17d1..4fbabf3 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -1055,6 +1055,7 @@ static int ov5645_probe(struct i2c_client *client)
struct device_node *endpoint;
struct ov5645 *ov5645;
u8 chip_id_high, chip_id_low;
+ bool set_clk = false;
unsigned int i;
u32 xclk_freq;
int ret;
@@ -1094,12 +1095,18 @@ 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);
- if (ret) {
- dev_err(dev, "could not get xclk frequency\n");
- return ret;
+ /* check if deprecated property clock-frequency is defined */
+ ret = of_property_read_u32(dev->of_node, "clock-frequency",
+ &xclk_freq);
+ if (!ret) {
+ ret = clk_set_rate(ov5645->xclk, xclk_freq);
+ if (ret) {
+ dev_err(dev, "could not set xclk frequency\n");
+ return ret;
+ }
}
+ xclk_freq = clk_get_rate(ov5645->xclk);
/* external clock must be 24MHz, allow 1% tolerance */
if (xclk_freq < 23760000 || xclk_freq > 24240000) {
dev_err(dev, "external clock frequency %u is not supported\n",
@@ -1107,12 +1114,6 @@ static int ov5645_probe(struct i2c_client *client)
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];
--
2.7.4
^ permalink raw reply related [flat|nested] 58+ messages in thread* [PATCH v3 2/4] media: i2c: ov5645: Switch to assigned-clock-rates
@ 2020-03-13 21:12 ` Lad Prabhakar
0 siblings, 0 replies; 58+ messages in thread
From: Lad Prabhakar @ 2020-03-13 21:12 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus,
NXP Linux Team, Magnus Damm, Ezequiel Garcia, Laurent Pinchart,
Geert Uytterhoeven
Cc: devicetree, linux-kernel, Lad Prabhakar, linux-renesas-soc,
Fabio Estevam, linux-arm-kernel, linux-media
This patch switches to assigned-clock-rates for specifying the clock rate.
The clk-conf.c internally handles setting the clock rate when
assigned-clock-rates is passed.
The driver now sets the clock frequency only if the deprecated property
clock-frequency is defined instead assigned-clock-rates, this is to avoid
breakage with existing DT binaries.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
drivers/media/i2c/ov5645.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index a6c17d1..4fbabf3 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -1055,6 +1055,7 @@ static int ov5645_probe(struct i2c_client *client)
struct device_node *endpoint;
struct ov5645 *ov5645;
u8 chip_id_high, chip_id_low;
+ bool set_clk = false;
unsigned int i;
u32 xclk_freq;
int ret;
@@ -1094,12 +1095,18 @@ 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);
- if (ret) {
- dev_err(dev, "could not get xclk frequency\n");
- return ret;
+ /* check if deprecated property clock-frequency is defined */
+ ret = of_property_read_u32(dev->of_node, "clock-frequency",
+ &xclk_freq);
+ if (!ret) {
+ ret = clk_set_rate(ov5645->xclk, xclk_freq);
+ if (ret) {
+ dev_err(dev, "could not set xclk frequency\n");
+ return ret;
+ }
}
+ xclk_freq = clk_get_rate(ov5645->xclk);
/* external clock must be 24MHz, allow 1% tolerance */
if (xclk_freq < 23760000 || xclk_freq > 24240000) {
dev_err(dev, "external clock frequency %u is not supported\n",
@@ -1107,12 +1114,6 @@ static int ov5645_probe(struct i2c_client *client)
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];
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 58+ messages in thread* Re: [PATCH v3 2/4] media: i2c: ov5645: Switch to assigned-clock-rates
2020-03-13 21:12 ` Lad Prabhakar
@ 2020-03-13 21:16 ` Laurent Pinchart
-1 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-03-13 21:16 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus,
NXP Linux Team, Magnus Damm, Ezequiel Garcia, Geert Uytterhoeven,
devicetree, linux-kernel, linux-renesas-soc, Fabio Estevam,
linux-media, linux-arm-kernel
Hi Prabakhar,
Thank you for the patch.
On Fri, Mar 13, 2020 at 09:12:32PM +0000, Lad Prabhakar wrote:
> This patch switches to assigned-clock-rates for specifying the clock rate.
> The clk-conf.c internally handles setting the clock rate when
> assigned-clock-rates is passed.
>
> The driver now sets the clock frequency only if the deprecated property
> clock-frequency is defined instead assigned-clock-rates, this is to avoid
> breakage with existing DT binaries.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> drivers/media/i2c/ov5645.c | 21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index a6c17d1..4fbabf3 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -1055,6 +1055,7 @@ static int ov5645_probe(struct i2c_client *client)
> struct device_node *endpoint;
> struct ov5645 *ov5645;
> u8 chip_id_high, chip_id_low;
> + bool set_clk = false;
This isn't used.
> unsigned int i;
> u32 xclk_freq;
> int ret;
> @@ -1094,12 +1095,18 @@ 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);
> - if (ret) {
> - dev_err(dev, "could not get xclk frequency\n");
> - return ret;
> + /* check if deprecated property clock-frequency is defined */
> + ret = of_property_read_u32(dev->of_node, "clock-frequency",
> + &xclk_freq);
> + if (!ret) {
> + ret = clk_set_rate(ov5645->xclk, xclk_freq);
> + if (ret) {
> + dev_err(dev, "could not set xclk frequency\n");
> + return ret;
> + }
> }
>
> + xclk_freq = clk_get_rate(ov5645->xclk);
I would move this line below the comment.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> /* external clock must be 24MHz, allow 1% tolerance */
> if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> dev_err(dev, "external clock frequency %u is not supported\n",
> @@ -1107,12 +1114,6 @@ static int ov5645_probe(struct i2c_client *client)
> 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
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v3 2/4] media: i2c: ov5645: Switch to assigned-clock-rates
@ 2020-03-13 21:16 ` Laurent Pinchart
0 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-03-13 21:16 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Mark Rutland, devicetree, Fabio Estevam, Sakari Ailus,
Ezequiel Garcia, Sascha Hauer, Magnus Damm, linux-kernel,
linux-renesas-soc, Rob Herring, Geert Uytterhoeven,
NXP Linux Team, Pengutronix Kernel Team, Mauro Carvalho Chehab,
Shawn Guo, linux-arm-kernel, linux-media
Hi Prabakhar,
Thank you for the patch.
On Fri, Mar 13, 2020 at 09:12:32PM +0000, Lad Prabhakar wrote:
> This patch switches to assigned-clock-rates for specifying the clock rate.
> The clk-conf.c internally handles setting the clock rate when
> assigned-clock-rates is passed.
>
> The driver now sets the clock frequency only if the deprecated property
> clock-frequency is defined instead assigned-clock-rates, this is to avoid
> breakage with existing DT binaries.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> drivers/media/i2c/ov5645.c | 21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index a6c17d1..4fbabf3 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -1055,6 +1055,7 @@ static int ov5645_probe(struct i2c_client *client)
> struct device_node *endpoint;
> struct ov5645 *ov5645;
> u8 chip_id_high, chip_id_low;
> + bool set_clk = false;
This isn't used.
> unsigned int i;
> u32 xclk_freq;
> int ret;
> @@ -1094,12 +1095,18 @@ 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);
> - if (ret) {
> - dev_err(dev, "could not get xclk frequency\n");
> - return ret;
> + /* check if deprecated property clock-frequency is defined */
> + ret = of_property_read_u32(dev->of_node, "clock-frequency",
> + &xclk_freq);
> + if (!ret) {
> + ret = clk_set_rate(ov5645->xclk, xclk_freq);
> + if (ret) {
> + dev_err(dev, "could not set xclk frequency\n");
> + return ret;
> + }
> }
>
> + xclk_freq = clk_get_rate(ov5645->xclk);
I would move this line below the comment.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> /* external clock must be 24MHz, allow 1% tolerance */
> if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> dev_err(dev, "external clock frequency %u is not supported\n",
> @@ -1107,12 +1114,6 @@ static int ov5645_probe(struct i2c_client *client)
> 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread* RE: [PATCH v3 2/4] media: i2c: ov5645: Switch to assigned-clock-rates
2020-03-13 21:16 ` Laurent Pinchart
@ 2020-03-13 21:19 ` Prabhakar Mahadev Lad
-1 siblings, 0 replies; 58+ messages in thread
From: Prabhakar Mahadev Lad @ 2020-03-13 21:19 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus,
NXP Linux Team, Magnus Damm, Ezequiel Garcia, Geert Uytterhoeven,
devicetree, linux-kernel, linux-renesas-soc, Fabio Estevam,
linux-media, linux-arm-kernel
Hi Laurent,
Thank you for the quick review.
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 13 March 2020 21:17
> To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Shawn Guo
> <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Rob Herring
> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Sakari
> Ailus <sakari.ailus@linux.intel.com>; NXP Linux Team <linux-imx@nxp.com>;
> Magnus Damm <magnus.damm@gmail.com>; Ezequiel Garcia
> <ezequiel@collabora.com>; Geert Uytterhoeven <geert@linux-m68k.org>;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-renesas-
> soc@vger.kernel.org; Fabio Estevam <festevam@gmail.com>; linux-
> media@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v3 2/4] media: i2c: ov5645: Switch to assigned-clock-
> rates
>
> Hi Prabakhar,
>
> Thank you for the patch.
>
> On Fri, Mar 13, 2020 at 09:12:32PM +0000, Lad Prabhakar wrote:
> > This patch switches to assigned-clock-rates for specifying the clock rate.
> > The clk-conf.c internally handles setting the clock rate when
> > assigned-clock-rates is passed.
> >
> > The driver now sets the clock frequency only if the deprecated
> > property clock-frequency is defined instead assigned-clock-rates, this
> > is to avoid breakage with existing DT binaries.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> lad.rj@bp.renesas.com>
> > ---
> > drivers/media/i2c/ov5645.c | 21 +++++++++++----------
> > 1 file changed, 11 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > index a6c17d1..4fbabf3 100644
> > --- a/drivers/media/i2c/ov5645.c
> > +++ b/drivers/media/i2c/ov5645.c
> > @@ -1055,6 +1055,7 @@ static int ov5645_probe(struct i2c_client *client)
> > struct device_node *endpoint;
> > struct ov5645 *ov5645;
> > u8 chip_id_high, chip_id_low;
> > +bool set_clk = false;
>
> This isn't used.
>
Argh! missed it.
> > unsigned int i;
> > u32 xclk_freq;
> > int ret;
> > @@ -1094,12 +1095,18 @@ 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);
> > -if (ret) {
> > -dev_err(dev, "could not get xclk frequency\n");
> > -return ret;
> > +/* check if deprecated property clock-frequency is defined */
> > +ret = of_property_read_u32(dev->of_node, "clock-frequency",
> > + &xclk_freq);
> > +if (!ret) {
> > +ret = clk_set_rate(ov5645->xclk, xclk_freq);
> > +if (ret) {
> > +dev_err(dev, "could not set xclk frequency\n");
> > +return ret;
> > +}
> > }
> >
> > +xclk_freq = clk_get_rate(ov5645->xclk);
>
> I would move this line below the comment.
>
Sure will do that.
Cheers,
--Prabhakar
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > /* external clock must be 24MHz, allow 1% tolerance */
> > if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> > dev_err(dev, "external clock frequency %u is not
> supported\n", @@
> > -1107,12 +1114,6 @@ static int ov5645_probe(struct i2c_client *client)
> > 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
Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
^ permalink raw reply [flat|nested] 58+ messages in thread* RE: [PATCH v3 2/4] media: i2c: ov5645: Switch to assigned-clock-rates
@ 2020-03-13 21:19 ` Prabhakar Mahadev Lad
0 siblings, 0 replies; 58+ messages in thread
From: Prabhakar Mahadev Lad @ 2020-03-13 21:19 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mark Rutland, devicetree, Fabio Estevam, Sakari Ailus,
Ezequiel Garcia, Sascha Hauer, Magnus Damm, linux-kernel,
linux-renesas-soc, Rob Herring, Geert Uytterhoeven,
NXP Linux Team, Pengutronix Kernel Team, Mauro Carvalho Chehab,
Shawn Guo, linux-arm-kernel, linux-media
Hi Laurent,
Thank you for the quick review.
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 13 March 2020 21:17
> To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Shawn Guo
> <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Rob Herring
> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Sakari
> Ailus <sakari.ailus@linux.intel.com>; NXP Linux Team <linux-imx@nxp.com>;
> Magnus Damm <magnus.damm@gmail.com>; Ezequiel Garcia
> <ezequiel@collabora.com>; Geert Uytterhoeven <geert@linux-m68k.org>;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-renesas-
> soc@vger.kernel.org; Fabio Estevam <festevam@gmail.com>; linux-
> media@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v3 2/4] media: i2c: ov5645: Switch to assigned-clock-
> rates
>
> Hi Prabakhar,
>
> Thank you for the patch.
>
> On Fri, Mar 13, 2020 at 09:12:32PM +0000, Lad Prabhakar wrote:
> > This patch switches to assigned-clock-rates for specifying the clock rate.
> > The clk-conf.c internally handles setting the clock rate when
> > assigned-clock-rates is passed.
> >
> > The driver now sets the clock frequency only if the deprecated
> > property clock-frequency is defined instead assigned-clock-rates, this
> > is to avoid breakage with existing DT binaries.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> lad.rj@bp.renesas.com>
> > ---
> > drivers/media/i2c/ov5645.c | 21 +++++++++++----------
> > 1 file changed, 11 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > index a6c17d1..4fbabf3 100644
> > --- a/drivers/media/i2c/ov5645.c
> > +++ b/drivers/media/i2c/ov5645.c
> > @@ -1055,6 +1055,7 @@ static int ov5645_probe(struct i2c_client *client)
> > struct device_node *endpoint;
> > struct ov5645 *ov5645;
> > u8 chip_id_high, chip_id_low;
> > +bool set_clk = false;
>
> This isn't used.
>
Argh! missed it.
> > unsigned int i;
> > u32 xclk_freq;
> > int ret;
> > @@ -1094,12 +1095,18 @@ 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);
> > -if (ret) {
> > -dev_err(dev, "could not get xclk frequency\n");
> > -return ret;
> > +/* check if deprecated property clock-frequency is defined */
> > +ret = of_property_read_u32(dev->of_node, "clock-frequency",
> > + &xclk_freq);
> > +if (!ret) {
> > +ret = clk_set_rate(ov5645->xclk, xclk_freq);
> > +if (ret) {
> > +dev_err(dev, "could not set xclk frequency\n");
> > +return ret;
> > +}
> > }
> >
> > +xclk_freq = clk_get_rate(ov5645->xclk);
>
> I would move this line below the comment.
>
Sure will do that.
Cheers,
--Prabhakar
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > /* external clock must be 24MHz, allow 1% tolerance */
> > if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> > dev_err(dev, "external clock frequency %u is not
> supported\n", @@
> > -1107,12 +1114,6 @@ static int ov5645_probe(struct i2c_client *client)
> > 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
Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v3 3/4] media: i2c: ov5645: Set maximum leverage of external clock frequency to 24480000
2020-03-13 21:12 ` Lad Prabhakar
@ 2020-03-13 21:12 ` Lad Prabhakar
-1 siblings, 0 replies; 58+ messages in thread
From: Lad Prabhakar @ 2020-03-13 21:12 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus,
NXP Linux Team, Magnus Damm, Ezequiel Garcia, Laurent Pinchart,
Geert Uytterhoeven
Cc: devicetree, linux-kernel, linux-renesas-soc, Lad Prabhakar,
Fabio Estevam, linux-media, linux-arm-kernel
While testing on Renesas RZ/G2E platform, noticed the clock frequency to
be 24242424 as a result the probe failed. However increasing the maximum
leverage of external clock frequency to 24480000 fixes this issue. Since
this difference is small enough and is insignificant set the same in the
driver.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
drivers/media/i2c/ov5645.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 4fbabf3..b49359b 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client)
}
xclk_freq = clk_get_rate(ov5645->xclk);
- /* external clock must be 24MHz, allow 1% tolerance */
- if (xclk_freq < 23760000 || xclk_freq > 24240000) {
+ /* external clock must be 24MHz, allow a minimum 1% and a maximum of 2%
+ * tolerance
+ */
+ if (xclk_freq < 23760000 || xclk_freq > 24480000) {
dev_err(dev, "external clock frequency %u is not supported\n",
xclk_freq);
return -EINVAL;
--
2.7.4
^ permalink raw reply related [flat|nested] 58+ messages in thread* [PATCH v3 3/4] media: i2c: ov5645: Set maximum leverage of external clock frequency to 24480000
@ 2020-03-13 21:12 ` Lad Prabhakar
0 siblings, 0 replies; 58+ messages in thread
From: Lad Prabhakar @ 2020-03-13 21:12 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus,
NXP Linux Team, Magnus Damm, Ezequiel Garcia, Laurent Pinchart,
Geert Uytterhoeven
Cc: devicetree, linux-kernel, Lad Prabhakar, linux-renesas-soc,
Fabio Estevam, linux-arm-kernel, linux-media
While testing on Renesas RZ/G2E platform, noticed the clock frequency to
be 24242424 as a result the probe failed. However increasing the maximum
leverage of external clock frequency to 24480000 fixes this issue. Since
this difference is small enough and is insignificant set the same in the
driver.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
drivers/media/i2c/ov5645.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 4fbabf3..b49359b 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client)
}
xclk_freq = clk_get_rate(ov5645->xclk);
- /* external clock must be 24MHz, allow 1% tolerance */
- if (xclk_freq < 23760000 || xclk_freq > 24240000) {
+ /* external clock must be 24MHz, allow a minimum 1% and a maximum of 2%
+ * tolerance
+ */
+ if (xclk_freq < 23760000 || xclk_freq > 24480000) {
dev_err(dev, "external clock frequency %u is not supported\n",
xclk_freq);
return -EINVAL;
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 58+ messages in thread* Re: [PATCH v3 3/4] media: i2c: ov5645: Set maximum leverage of external clock frequency to 24480000
2020-03-13 21:12 ` Lad Prabhakar
@ 2020-03-13 21:23 ` Laurent Pinchart
-1 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-03-13 21:23 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus,
NXP Linux Team, Magnus Damm, Ezequiel Garcia, Geert Uytterhoeven,
devicetree, linux-kernel, linux-renesas-soc, Fabio Estevam,
linux-media, linux-arm-kernel
Hi Prabhakar,
Thank you for the patch.
On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote:
> While testing on Renesas RZ/G2E platform, noticed the clock frequency to
> be 24242424 as a result the probe failed. However increasing the maximum
> leverage of external clock frequency to 24480000 fixes this issue. Since
> this difference is small enough and is insignificant set the same in the
> driver.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> drivers/media/i2c/ov5645.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index 4fbabf3..b49359b 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client)
> }
>
> xclk_freq = clk_get_rate(ov5645->xclk);
> - /* external clock must be 24MHz, allow 1% tolerance */
> - if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> + /* external clock must be 24MHz, allow a minimum 1% and a maximum of 2%
> + * tolerance
So where do these numbers come from ? I understand that 2% is what you
need to make your clock fit in the range, but why -1%/+2% instead of
-2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the
range of supported xvclk frequencies to be 6MHz to 54MHz. I understand
that PLL parameters depend on the clock frequency, but could they be
calculated instead of hardcoded, to avoid requiring an exact 24MHz input
frequency ?
> + */
> + if (xclk_freq < 23760000 || xclk_freq > 24480000) {
> dev_err(dev, "external clock frequency %u is not supported\n",
> xclk_freq);
> return -EINVAL;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v3 3/4] media: i2c: ov5645: Set maximum leverage of external clock frequency to 24480000
@ 2020-03-13 21:23 ` Laurent Pinchart
0 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-03-13 21:23 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Mark Rutland, devicetree, Fabio Estevam, Sakari Ailus,
Ezequiel Garcia, Sascha Hauer, Magnus Damm, linux-kernel,
linux-renesas-soc, Rob Herring, Geert Uytterhoeven,
NXP Linux Team, Pengutronix Kernel Team, Mauro Carvalho Chehab,
Shawn Guo, linux-arm-kernel, linux-media
Hi Prabhakar,
Thank you for the patch.
On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote:
> While testing on Renesas RZ/G2E platform, noticed the clock frequency to
> be 24242424 as a result the probe failed. However increasing the maximum
> leverage of external clock frequency to 24480000 fixes this issue. Since
> this difference is small enough and is insignificant set the same in the
> driver.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> drivers/media/i2c/ov5645.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index 4fbabf3..b49359b 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client)
> }
>
> xclk_freq = clk_get_rate(ov5645->xclk);
> - /* external clock must be 24MHz, allow 1% tolerance */
> - if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> + /* external clock must be 24MHz, allow a minimum 1% and a maximum of 2%
> + * tolerance
So where do these numbers come from ? I understand that 2% is what you
need to make your clock fit in the range, but why -1%/+2% instead of
-2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the
range of supported xvclk frequencies to be 6MHz to 54MHz. I understand
that PLL parameters depend on the clock frequency, but could they be
calculated instead of hardcoded, to avoid requiring an exact 24MHz input
frequency ?
> + */
> + if (xclk_freq < 23760000 || xclk_freq > 24480000) {
> dev_err(dev, "external clock frequency %u is not supported\n",
> xclk_freq);
> return -EINVAL;
--
Regards,
Laurent Pinchart
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread* RE: [PATCH v3 3/4] media: i2c: ov5645: Set maximum leverage of external clock frequency to 24480000
2020-03-13 21:23 ` Laurent Pinchart
@ 2020-03-13 21:31 ` Prabhakar Mahadev Lad
-1 siblings, 0 replies; 58+ messages in thread
From: Prabhakar Mahadev Lad @ 2020-03-13 21:31 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus,
NXP Linux Team, Magnus Damm, Ezequiel Garcia, Geert Uytterhoeven,
devicetree, linux-kernel, linux-renesas-soc, Fabio Estevam,
linux-media, linux-arm-kernel
Hi Laurent,
Thank you for the review.
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 13 March 2020 21:24
> To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Shawn Guo
> <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Rob Herring
> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Sakari
> Ailus <sakari.ailus@linux.intel.com>; NXP Linux Team <linux-imx@nxp.com>;
> Magnus Damm <magnus.damm@gmail.com>; Ezequiel Garcia
> <ezequiel@collabora.com>; Geert Uytterhoeven <geert@linux-m68k.org>;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-renesas-
> soc@vger.kernel.org; Fabio Estevam <festevam@gmail.com>; linux-
> media@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v3 3/4] media: i2c: ov5645: Set maximum leverage of
> external clock frequency to 24480000
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote:
> > While testing on Renesas RZ/G2E platform, noticed the clock frequency
> > to be 24242424 as a result the probe failed. However increasing the
> > maximum leverage of external clock frequency to 24480000 fixes this
> > issue. Since this difference is small enough and is insignificant set
> > the same in the driver.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> lad.rj@bp.renesas.com>
> > ---
> > drivers/media/i2c/ov5645.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > index 4fbabf3..b49359b 100644
> > --- a/drivers/media/i2c/ov5645.c
> > +++ b/drivers/media/i2c/ov5645.c
> > @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client)
> > }
> >
> > xclk_freq = clk_get_rate(ov5645->xclk);
> > -/* external clock must be 24MHz, allow 1% tolerance */
> > -if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> > +/* external clock must be 24MHz, allow a minimum 1% and a
> maximum of 2%
> > + * tolerance
>
> So where do these numbers come from ? I understand that 2% is what you
> need to make your clock fit in the range, but why -1%/+2% instead of -
> 2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the
> range of supported xvclk frequencies to be 6MHz to 54MHz. I understand
> that PLL parameters depend on the clock frequency, but could they be
> calculated instead of hardcoded, to avoid requiring an exact 24MHz input
> frequency ?
>
To be honest I don't have the datasheet for ov5645, the flyer says 6-54Mhz but the
logs/comment says 24Mhz.
Cheers,
--Prabhakar
> > + */
> > +if (xclk_freq < 23760000 || xclk_freq > 24480000) {
> > dev_err(dev, "external clock frequency %u is not
> supported\n",
> > xclk_freq);
> > return -EINVAL;
>
> --
> Regards,
>
> Laurent Pinchart
Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
^ permalink raw reply [flat|nested] 58+ messages in thread* RE: [PATCH v3 3/4] media: i2c: ov5645: Set maximum leverage of external clock frequency to 24480000
@ 2020-03-13 21:31 ` Prabhakar Mahadev Lad
0 siblings, 0 replies; 58+ messages in thread
From: Prabhakar Mahadev Lad @ 2020-03-13 21:31 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mark Rutland, devicetree, Fabio Estevam, Sakari Ailus,
Ezequiel Garcia, Sascha Hauer, Magnus Damm, linux-kernel,
linux-renesas-soc, Rob Herring, Geert Uytterhoeven,
NXP Linux Team, Pengutronix Kernel Team, Mauro Carvalho Chehab,
Shawn Guo, linux-arm-kernel, linux-media
Hi Laurent,
Thank you for the review.
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 13 March 2020 21:24
> To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Shawn Guo
> <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Rob Herring
> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Sakari
> Ailus <sakari.ailus@linux.intel.com>; NXP Linux Team <linux-imx@nxp.com>;
> Magnus Damm <magnus.damm@gmail.com>; Ezequiel Garcia
> <ezequiel@collabora.com>; Geert Uytterhoeven <geert@linux-m68k.org>;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-renesas-
> soc@vger.kernel.org; Fabio Estevam <festevam@gmail.com>; linux-
> media@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v3 3/4] media: i2c: ov5645: Set maximum leverage of
> external clock frequency to 24480000
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote:
> > While testing on Renesas RZ/G2E platform, noticed the clock frequency
> > to be 24242424 as a result the probe failed. However increasing the
> > maximum leverage of external clock frequency to 24480000 fixes this
> > issue. Since this difference is small enough and is insignificant set
> > the same in the driver.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> lad.rj@bp.renesas.com>
> > ---
> > drivers/media/i2c/ov5645.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > index 4fbabf3..b49359b 100644
> > --- a/drivers/media/i2c/ov5645.c
> > +++ b/drivers/media/i2c/ov5645.c
> > @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client)
> > }
> >
> > xclk_freq = clk_get_rate(ov5645->xclk);
> > -/* external clock must be 24MHz, allow 1% tolerance */
> > -if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> > +/* external clock must be 24MHz, allow a minimum 1% and a
> maximum of 2%
> > + * tolerance
>
> So where do these numbers come from ? I understand that 2% is what you
> need to make your clock fit in the range, but why -1%/+2% instead of -
> 2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the
> range of supported xvclk frequencies to be 6MHz to 54MHz. I understand
> that PLL parameters depend on the clock frequency, but could they be
> calculated instead of hardcoded, to avoid requiring an exact 24MHz input
> frequency ?
>
To be honest I don't have the datasheet for ov5645, the flyer says 6-54Mhz but the
logs/comment says 24Mhz.
Cheers,
--Prabhakar
> > + */
> > +if (xclk_freq < 23760000 || xclk_freq > 24480000) {
> > dev_err(dev, "external clock frequency %u is not
> supported\n",
> > xclk_freq);
> > return -EINVAL;
>
> --
> Regards,
>
> Laurent Pinchart
Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v3 3/4] media: i2c: ov5645: Set maximum leverage of external clock frequency to 24480000
2020-03-13 21:31 ` Prabhakar Mahadev Lad
@ 2020-03-18 22:41 ` Lad, Prabhakar
-1 siblings, 0 replies; 58+ messages in thread
From: Lad, Prabhakar @ 2020-03-18 22:41 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mark Rutland, devicetree, Fabio Estevam, Sakari Ailus,
Ezequiel Garcia, Prabhakar Mahadev Lad, Sascha Hauer,
Magnus Damm, linux-kernel, linux-renesas-soc, Rob Herring,
Geert Uytterhoeven, NXP Linux Team, Pengutronix Kernel Team,
Mauro Carvalho Chehab, Shawn Guo, linux-arm-kernel, linux-media
Hi Laurent,
On Fri, Mar 13, 2020 at 9:31 PM Prabhakar Mahadev Lad
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
>
> Hi Laurent,
>
> Thank you for the review.
>
> > -----Original Message-----
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Sent: 13 March 2020 21:24
> > To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Shawn Guo
> > <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>;
> > Pengutronix Kernel Team <kernel@pengutronix.de>; Rob Herring
> > <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Sakari
> > Ailus <sakari.ailus@linux.intel.com>; NXP Linux Team <linux-imx@nxp.com>;
> > Magnus Damm <magnus.damm@gmail.com>; Ezequiel Garcia
> > <ezequiel@collabora.com>; Geert Uytterhoeven <geert@linux-m68k.org>;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-renesas-
> > soc@vger.kernel.org; Fabio Estevam <festevam@gmail.com>; linux-
> > media@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> > Subject: Re: [PATCH v3 3/4] media: i2c: ov5645: Set maximum leverage of
> > external clock frequency to 24480000
> >
> > Hi Prabhakar,
> >
> > Thank you for the patch.
> >
> > On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote:
> > > While testing on Renesas RZ/G2E platform, noticed the clock frequency
> > > to be 24242424 as a result the probe failed. However increasing the
> > > maximum leverage of external clock frequency to 24480000 fixes this
> > > issue. Since this difference is small enough and is insignificant set
> > > the same in the driver.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> > lad.rj@bp.renesas.com>
> > > ---
> > > drivers/media/i2c/ov5645.c | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > > index 4fbabf3..b49359b 100644
> > > --- a/drivers/media/i2c/ov5645.c
> > > +++ b/drivers/media/i2c/ov5645.c
> > > @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client)
> > > }
> > >
> > > xclk_freq = clk_get_rate(ov5645->xclk);
> > > -/* external clock must be 24MHz, allow 1% tolerance */
> > > -if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> > > +/* external clock must be 24MHz, allow a minimum 1% and a
> > maximum of 2%
> > > + * tolerance
> >
> > So where do these numbers come from ? I understand that 2% is what you
> > need to make your clock fit in the range, but why -1%/+2% instead of -
> > 2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the
> > range of supported xvclk frequencies to be 6MHz to 54MHz. I understand
> > that PLL parameters depend on the clock frequency, but could they be
> > calculated instead of hardcoded, to avoid requiring an exact 24MHz input
> > frequency ?
> >
> To be honest I don't have the datasheet for ov5645, the flyer says 6-54Mhz but the
> logs/comment says 24Mhz.
>
Comparing to ov5640 datasheet [1] (which I am assuming might be
similar to ov5645), this change should affect the driver.
[1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
Cheers,
--Prabhakar
> Cheers,
> --Prabhakar
>
> > > + */
> > > +if (xclk_freq < 23760000 || xclk_freq > 24480000) {
> > > dev_err(dev, "external clock frequency %u is not
> > supported\n",
> > > xclk_freq);
> > > return -EINVAL;
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
>
> Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v3 3/4] media: i2c: ov5645: Set maximum leverage of external clock frequency to 24480000
@ 2020-03-18 22:41 ` Lad, Prabhakar
0 siblings, 0 replies; 58+ messages in thread
From: Lad, Prabhakar @ 2020-03-18 22:41 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mark Rutland, devicetree, Pengutronix Kernel Team,
Ezequiel Garcia, Sascha Hauer, Magnus Damm,
Prabhakar Mahadev Lad, linux-kernel, linux-renesas-soc,
Rob Herring, Geert Uytterhoeven, NXP Linux Team, Sakari Ailus,
Shawn Guo, Mauro Carvalho Chehab, Fabio Estevam,
linux-arm-kernel, linux-media
Hi Laurent,
On Fri, Mar 13, 2020 at 9:31 PM Prabhakar Mahadev Lad
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
>
> Hi Laurent,
>
> Thank you for the review.
>
> > -----Original Message-----
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Sent: 13 March 2020 21:24
> > To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Shawn Guo
> > <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>;
> > Pengutronix Kernel Team <kernel@pengutronix.de>; Rob Herring
> > <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Sakari
> > Ailus <sakari.ailus@linux.intel.com>; NXP Linux Team <linux-imx@nxp.com>;
> > Magnus Damm <magnus.damm@gmail.com>; Ezequiel Garcia
> > <ezequiel@collabora.com>; Geert Uytterhoeven <geert@linux-m68k.org>;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-renesas-
> > soc@vger.kernel.org; Fabio Estevam <festevam@gmail.com>; linux-
> > media@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> > Subject: Re: [PATCH v3 3/4] media: i2c: ov5645: Set maximum leverage of
> > external clock frequency to 24480000
> >
> > Hi Prabhakar,
> >
> > Thank you for the patch.
> >
> > On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote:
> > > While testing on Renesas RZ/G2E platform, noticed the clock frequency
> > > to be 24242424 as a result the probe failed. However increasing the
> > > maximum leverage of external clock frequency to 24480000 fixes this
> > > issue. Since this difference is small enough and is insignificant set
> > > the same in the driver.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> > lad.rj@bp.renesas.com>
> > > ---
> > > drivers/media/i2c/ov5645.c | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > > index 4fbabf3..b49359b 100644
> > > --- a/drivers/media/i2c/ov5645.c
> > > +++ b/drivers/media/i2c/ov5645.c
> > > @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client)
> > > }
> > >
> > > xclk_freq = clk_get_rate(ov5645->xclk);
> > > -/* external clock must be 24MHz, allow 1% tolerance */
> > > -if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> > > +/* external clock must be 24MHz, allow a minimum 1% and a
> > maximum of 2%
> > > + * tolerance
> >
> > So where do these numbers come from ? I understand that 2% is what you
> > need to make your clock fit in the range, but why -1%/+2% instead of -
> > 2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the
> > range of supported xvclk frequencies to be 6MHz to 54MHz. I understand
> > that PLL parameters depend on the clock frequency, but could they be
> > calculated instead of hardcoded, to avoid requiring an exact 24MHz input
> > frequency ?
> >
> To be honest I don't have the datasheet for ov5645, the flyer says 6-54Mhz but the
> logs/comment says 24Mhz.
>
Comparing to ov5640 datasheet [1] (which I am assuming might be
similar to ov5645), this change should affect the driver.
[1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
Cheers,
--Prabhakar
> Cheers,
> --Prabhakar
>
> > > + */
> > > +if (xclk_freq < 23760000 || xclk_freq > 24480000) {
> > > dev_err(dev, "external clock frequency %u is not
> > supported\n",
> > > xclk_freq);
> > > return -EINVAL;
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
>
> Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v3 3/4] media: i2c: ov5645: Set maximum leverage of external clock frequency to 24480000
2020-03-18 22:41 ` Lad, Prabhakar
@ 2020-03-18 23:22 ` Laurent Pinchart
-1 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-03-18 23:22 UTC (permalink / raw)
To: Lad, Prabhakar
Cc: Mark Rutland, devicetree, Fabio Estevam, Sakari Ailus,
Ezequiel Garcia, Prabhakar Mahadev Lad, Sascha Hauer,
Magnus Damm, linux-kernel, linux-renesas-soc, Rob Herring,
Geert Uytterhoeven, NXP Linux Team, Pengutronix Kernel Team,
Mauro Carvalho Chehab, Shawn Guo, linux-arm-kernel, linux-media
Hi Prabhakar,
On Wed, Mar 18, 2020 at 10:41:57PM +0000, Lad, Prabhakar wrote:
> On Fri, Mar 13, 2020 at 9:31 PM Prabhakar Mahadev Lad wrote:
> > On 13 March 2020 21:24, Laurent Pinchart wrote:
> >> On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote:
> >>> While testing on Renesas RZ/G2E platform, noticed the clock frequency
> >>> to be 24242424 as a result the probe failed. However increasing the
> >>> maximum leverage of external clock frequency to 24480000 fixes this
> >>> issue. Since this difference is small enough and is insignificant set
> >>> the same in the driver.
> >>>
> >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>> ---
> >>> drivers/media/i2c/ov5645.c | 6 ++++--
> >>> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> >>> index 4fbabf3..b49359b 100644
> >>> --- a/drivers/media/i2c/ov5645.c
> >>> +++ b/drivers/media/i2c/ov5645.c
> >>> @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client)
> >>> }
> >>>
> >>> xclk_freq = clk_get_rate(ov5645->xclk);
> >>> -/* external clock must be 24MHz, allow 1% tolerance */
> >>> -if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> >>> +/* external clock must be 24MHz, allow a minimum 1% and a
> >> maximum of 2%
> >>> + * tolerance
> >>
> >> So where do these numbers come from ? I understand that 2% is what you
> >> need to make your clock fit in the range, but why -1%/+2% instead of -
> >> 2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the
> >> range of supported xvclk frequencies to be 6MHz to 54MHz. I understand
> >> that PLL parameters depend on the clock frequency, but could they be
> >> calculated instead of hardcoded, to avoid requiring an exact 24MHz input
> >> frequency ?
> >>
> > To be honest I don't have the datasheet for ov5645, the flyer says 6-54Mhz but the
> > logs/comment says 24Mhz.
> >
> Comparing to ov5640 datasheet [1] (which I am assuming might be
> similar to ov5645),
Let's assume this to be the case, I see no reason not to :-)
> this change should affect the driver.
How do you mean ?
> [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
>
> >>> + */
> >>> +if (xclk_freq < 23760000 || xclk_freq > 24480000) {
> >>> dev_err(dev, "external clock frequency %u is not supported\n",
> >>> xclk_freq);
> >>> return -EINVAL;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v3 3/4] media: i2c: ov5645: Set maximum leverage of external clock frequency to 24480000
@ 2020-03-18 23:22 ` Laurent Pinchart
0 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-03-18 23:22 UTC (permalink / raw)
To: Lad, Prabhakar
Cc: Mark Rutland, devicetree, Pengutronix Kernel Team,
Ezequiel Garcia, Sascha Hauer, Magnus Damm,
Prabhakar Mahadev Lad, linux-kernel, linux-renesas-soc,
Rob Herring, Geert Uytterhoeven, NXP Linux Team, Sakari Ailus,
Shawn Guo, Mauro Carvalho Chehab, Fabio Estevam,
linux-arm-kernel, linux-media
Hi Prabhakar,
On Wed, Mar 18, 2020 at 10:41:57PM +0000, Lad, Prabhakar wrote:
> On Fri, Mar 13, 2020 at 9:31 PM Prabhakar Mahadev Lad wrote:
> > On 13 March 2020 21:24, Laurent Pinchart wrote:
> >> On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote:
> >>> While testing on Renesas RZ/G2E platform, noticed the clock frequency
> >>> to be 24242424 as a result the probe failed. However increasing the
> >>> maximum leverage of external clock frequency to 24480000 fixes this
> >>> issue. Since this difference is small enough and is insignificant set
> >>> the same in the driver.
> >>>
> >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>> ---
> >>> drivers/media/i2c/ov5645.c | 6 ++++--
> >>> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> >>> index 4fbabf3..b49359b 100644
> >>> --- a/drivers/media/i2c/ov5645.c
> >>> +++ b/drivers/media/i2c/ov5645.c
> >>> @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client)
> >>> }
> >>>
> >>> xclk_freq = clk_get_rate(ov5645->xclk);
> >>> -/* external clock must be 24MHz, allow 1% tolerance */
> >>> -if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> >>> +/* external clock must be 24MHz, allow a minimum 1% and a
> >> maximum of 2%
> >>> + * tolerance
> >>
> >> So where do these numbers come from ? I understand that 2% is what you
> >> need to make your clock fit in the range, but why -1%/+2% instead of -
> >> 2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the
> >> range of supported xvclk frequencies to be 6MHz to 54MHz. I understand
> >> that PLL parameters depend on the clock frequency, but could they be
> >> calculated instead of hardcoded, to avoid requiring an exact 24MHz input
> >> frequency ?
> >>
> > To be honest I don't have the datasheet for ov5645, the flyer says 6-54Mhz but the
> > logs/comment says 24Mhz.
> >
> Comparing to ov5640 datasheet [1] (which I am assuming might be
> similar to ov5645),
Let's assume this to be the case, I see no reason not to :-)
> this change should affect the driver.
How do you mean ?
> [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
>
> >>> + */
> >>> +if (xclk_freq < 23760000 || xclk_freq > 24480000) {
> >>> dev_err(dev, "external clock frequency %u is not supported\n",
> >>> xclk_freq);
> >>> return -EINVAL;
--
Regards,
Laurent Pinchart
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v3 3/4] media: i2c: ov5645: Set maximum leverage of external clock frequency to 24480000
2020-03-18 23:22 ` Laurent Pinchart
@ 2020-03-19 8:00 ` Lad, Prabhakar
-1 siblings, 0 replies; 58+ messages in thread
From: Lad, Prabhakar @ 2020-03-19 8:00 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mark Rutland, devicetree, Fabio Estevam, Sakari Ailus,
Ezequiel Garcia, Prabhakar Mahadev Lad, Sascha Hauer,
Magnus Damm, linux-kernel, linux-renesas-soc, Rob Herring,
Geert Uytterhoeven, NXP Linux Team, Pengutronix Kernel Team,
Mauro Carvalho Chehab, Shawn Guo, linux-arm-kernel, linux-media
Hi Laurent,
On Wed, Mar 18, 2020 at 11:22 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> On Wed, Mar 18, 2020 at 10:41:57PM +0000, Lad, Prabhakar wrote:
> > On Fri, Mar 13, 2020 at 9:31 PM Prabhakar Mahadev Lad wrote:
> > > On 13 March 2020 21:24, Laurent Pinchart wrote:
> > >> On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote:
> > >>> While testing on Renesas RZ/G2E platform, noticed the clock frequency
> > >>> to be 24242424 as a result the probe failed. However increasing the
> > >>> maximum leverage of external clock frequency to 24480000 fixes this
> > >>> issue. Since this difference is small enough and is insignificant set
> > >>> the same in the driver.
> > >>>
> > >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >>> ---
> > >>> drivers/media/i2c/ov5645.c | 6 ++++--
> > >>> 1 file changed, 4 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > >>> index 4fbabf3..b49359b 100644
> > >>> --- a/drivers/media/i2c/ov5645.c
> > >>> +++ b/drivers/media/i2c/ov5645.c
> > >>> @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client)
> > >>> }
> > >>>
> > >>> xclk_freq = clk_get_rate(ov5645->xclk);
> > >>> -/* external clock must be 24MHz, allow 1% tolerance */
> > >>> -if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> > >>> +/* external clock must be 24MHz, allow a minimum 1% and a
> > >> maximum of 2%
> > >>> + * tolerance
> > >>
> > >> So where do these numbers come from ? I understand that 2% is what you
> > >> need to make your clock fit in the range, but why -1%/+2% instead of -
> > >> 2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the
> > >> range of supported xvclk frequencies to be 6MHz to 54MHz. I understand
> > >> that PLL parameters depend on the clock frequency, but could they be
> > >> calculated instead of hardcoded, to avoid requiring an exact 24MHz input
> > >> frequency ?
> > >>
> > > To be honest I don't have the datasheet for ov5645, the flyer says 6-54Mhz but the
> > > logs/comment says 24Mhz.
> > >
> > Comparing to ov5640 datasheet [1] (which I am assuming might be
> > similar to ov5645),
>
> Let's assume this to be the case, I see no reason not to :-)
>
> > this change should affect the driver.
>
> How do you mean ?
>
Oops sorry for for the typo I meant shouldn't affect the driver :)
Cheers,
--Prabhakar
> > [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
> >
> > >>> + */
> > >>> +if (xclk_freq < 23760000 || xclk_freq > 24480000) {
> > >>> dev_err(dev, "external clock frequency %u is not supported\n",
> > >>> xclk_freq);
> > >>> return -EINVAL;
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v3 3/4] media: i2c: ov5645: Set maximum leverage of external clock frequency to 24480000
@ 2020-03-19 8:00 ` Lad, Prabhakar
0 siblings, 0 replies; 58+ messages in thread
From: Lad, Prabhakar @ 2020-03-19 8:00 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mark Rutland, devicetree, Pengutronix Kernel Team,
Ezequiel Garcia, Sascha Hauer, Magnus Damm,
Prabhakar Mahadev Lad, linux-kernel, linux-renesas-soc,
Rob Herring, Geert Uytterhoeven, NXP Linux Team, Sakari Ailus,
Shawn Guo, Mauro Carvalho Chehab, Fabio Estevam,
linux-arm-kernel, linux-media
Hi Laurent,
On Wed, Mar 18, 2020 at 11:22 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> On Wed, Mar 18, 2020 at 10:41:57PM +0000, Lad, Prabhakar wrote:
> > On Fri, Mar 13, 2020 at 9:31 PM Prabhakar Mahadev Lad wrote:
> > > On 13 March 2020 21:24, Laurent Pinchart wrote:
> > >> On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote:
> > >>> While testing on Renesas RZ/G2E platform, noticed the clock frequency
> > >>> to be 24242424 as a result the probe failed. However increasing the
> > >>> maximum leverage of external clock frequency to 24480000 fixes this
> > >>> issue. Since this difference is small enough and is insignificant set
> > >>> the same in the driver.
> > >>>
> > >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >>> ---
> > >>> drivers/media/i2c/ov5645.c | 6 ++++--
> > >>> 1 file changed, 4 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > >>> index 4fbabf3..b49359b 100644
> > >>> --- a/drivers/media/i2c/ov5645.c
> > >>> +++ b/drivers/media/i2c/ov5645.c
> > >>> @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client)
> > >>> }
> > >>>
> > >>> xclk_freq = clk_get_rate(ov5645->xclk);
> > >>> -/* external clock must be 24MHz, allow 1% tolerance */
> > >>> -if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> > >>> +/* external clock must be 24MHz, allow a minimum 1% and a
> > >> maximum of 2%
> > >>> + * tolerance
> > >>
> > >> So where do these numbers come from ? I understand that 2% is what you
> > >> need to make your clock fit in the range, but why -1%/+2% instead of -
> > >> 2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the
> > >> range of supported xvclk frequencies to be 6MHz to 54MHz. I understand
> > >> that PLL parameters depend on the clock frequency, but could they be
> > >> calculated instead of hardcoded, to avoid requiring an exact 24MHz input
> > >> frequency ?
> > >>
> > > To be honest I don't have the datasheet for ov5645, the flyer says 6-54Mhz but the
> > > logs/comment says 24Mhz.
> > >
> > Comparing to ov5640 datasheet [1] (which I am assuming might be
> > similar to ov5645),
>
> Let's assume this to be the case, I see no reason not to :-)
>
> > this change should affect the driver.
>
> How do you mean ?
>
Oops sorry for for the typo I meant shouldn't affect the driver :)
Cheers,
--Prabhakar
> > [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
> >
> > >>> + */
> > >>> +if (xclk_freq < 23760000 || xclk_freq > 24480000) {
> > >>> dev_err(dev, "external clock frequency %u is not supported\n",
> > >>> xclk_freq);
> > >>> return -EINVAL;
>
> --
> Regards,
>
> Laurent Pinchart
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 3/4] media: i2c: ov5645: Set maximum leverage of external clock frequency to 24480000
2020-03-13 21:31 ` Prabhakar Mahadev Lad
@ 2020-03-18 23:18 ` Laurent Pinchart
-1 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-03-18 23:18 UTC (permalink / raw)
To: Prabhakar Mahadev Lad
Cc: Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus,
NXP Linux Team, Magnus Damm, Ezequiel Garcia, Geert Uytterhoeven,
devicetree, linux-kernel, linux-renesas-soc, Fabio Estevam,
linux-media, linux-arm-kernel
Hi Prabhakar,
On Fri, Mar 13, 2020 at 09:31:25PM +0000, Prabhakar Mahadev Lad wrote:
> On 13 March 2020 21:24, Laurent Pinchart wrote:
> > On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote:
> > > While testing on Renesas RZ/G2E platform, noticed the clock frequency
> > > to be 24242424 as a result the probe failed. However increasing the
> > > maximum leverage of external clock frequency to 24480000 fixes this
> > > issue. Since this difference is small enough and is insignificant set
> > > the same in the driver.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > > drivers/media/i2c/ov5645.c | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > > index 4fbabf3..b49359b 100644
> > > --- a/drivers/media/i2c/ov5645.c
> > > +++ b/drivers/media/i2c/ov5645.c
> > > @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client)
> > > }
> > >
> > > xclk_freq = clk_get_rate(ov5645->xclk);
> > > -/* external clock must be 24MHz, allow 1% tolerance */
> > > -if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> > > +/* external clock must be 24MHz, allow a minimum 1% and a maximum of 2%
> > > + * tolerance
> >
> > So where do these numbers come from ? I understand that 2% is what you
> > need to make your clock fit in the range, but why -1%/+2% instead of -
> > 2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the
> > range of supported xvclk frequencies to be 6MHz to 54MHz. I understand
> > that PLL parameters depend on the clock frequency, but could they be
> > calculated instead of hardcoded, to avoid requiring an exact 24MHz input
> > frequency ?
>
> To be honest I don't have the datasheet for ov5645, the flyer says 6-54Mhz but the
> logs/comment says 24Mhz.
The OV5645 clock topology is fairly complex, with two PLLs and different
set of output dividers. It however shouldn't be impossible to calculate
the PLL configuration in the driver, but would require some dedication,
and is probably not worth it.
I've discussed the matter with Sakari, and we concluded that this is
just a sanity check. We advise increasing the tolerance by a bigger
amount to avoid patching this for every new board (completely
arbitrarily, +/- 5%), and turning the fatal error into a dev_warn,
dropping the return -EINVAL statement.
> > > + */
> > > +if (xclk_freq < 23760000 || xclk_freq > 24480000) {
> > > dev_err(dev, "external clock frequency %u is not supported\n",
> > > xclk_freq);
> > > return -EINVAL;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v3 3/4] media: i2c: ov5645: Set maximum leverage of external clock frequency to 24480000
@ 2020-03-18 23:18 ` Laurent Pinchart
0 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-03-18 23:18 UTC (permalink / raw)
To: Prabhakar Mahadev Lad
Cc: Mark Rutland, devicetree, Fabio Estevam, Sakari Ailus,
Ezequiel Garcia, Sascha Hauer, Magnus Damm, linux-kernel,
linux-renesas-soc, Rob Herring, Geert Uytterhoeven,
NXP Linux Team, Pengutronix Kernel Team, Mauro Carvalho Chehab,
Shawn Guo, linux-arm-kernel, linux-media
Hi Prabhakar,
On Fri, Mar 13, 2020 at 09:31:25PM +0000, Prabhakar Mahadev Lad wrote:
> On 13 March 2020 21:24, Laurent Pinchart wrote:
> > On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote:
> > > While testing on Renesas RZ/G2E platform, noticed the clock frequency
> > > to be 24242424 as a result the probe failed. However increasing the
> > > maximum leverage of external clock frequency to 24480000 fixes this
> > > issue. Since this difference is small enough and is insignificant set
> > > the same in the driver.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > > drivers/media/i2c/ov5645.c | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > > index 4fbabf3..b49359b 100644
> > > --- a/drivers/media/i2c/ov5645.c
> > > +++ b/drivers/media/i2c/ov5645.c
> > > @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client)
> > > }
> > >
> > > xclk_freq = clk_get_rate(ov5645->xclk);
> > > -/* external clock must be 24MHz, allow 1% tolerance */
> > > -if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> > > +/* external clock must be 24MHz, allow a minimum 1% and a maximum of 2%
> > > + * tolerance
> >
> > So where do these numbers come from ? I understand that 2% is what you
> > need to make your clock fit in the range, but why -1%/+2% instead of -
> > 2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the
> > range of supported xvclk frequencies to be 6MHz to 54MHz. I understand
> > that PLL parameters depend on the clock frequency, but could they be
> > calculated instead of hardcoded, to avoid requiring an exact 24MHz input
> > frequency ?
>
> To be honest I don't have the datasheet for ov5645, the flyer says 6-54Mhz but the
> logs/comment says 24Mhz.
The OV5645 clock topology is fairly complex, with two PLLs and different
set of output dividers. It however shouldn't be impossible to calculate
the PLL configuration in the driver, but would require some dedication,
and is probably not worth it.
I've discussed the matter with Sakari, and we concluded that this is
just a sanity check. We advise increasing the tolerance by a bigger
amount to avoid patching this for every new board (completely
arbitrarily, +/- 5%), and turning the fatal error into a dev_warn,
dropping the return -EINVAL statement.
> > > + */
> > > +if (xclk_freq < 23760000 || xclk_freq > 24480000) {
> > > dev_err(dev, "external clock frequency %u is not supported\n",
> > > xclk_freq);
> > > return -EINVAL;
--
Regards,
Laurent Pinchart
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v3 3/4] media: i2c: ov5645: Set maximum leverage of external clock frequency to 24480000
2020-03-18 23:18 ` Laurent Pinchart
@ 2020-03-19 7:59 ` Lad, Prabhakar
-1 siblings, 0 replies; 58+ messages in thread
From: Lad, Prabhakar @ 2020-03-19 7:59 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Prabhakar Mahadev Lad, Mark Rutland, devicetree, Fabio Estevam,
Sakari Ailus, Ezequiel Garcia, Sascha Hauer, Magnus Damm,
linux-kernel, linux-renesas-soc, Rob Herring, Geert Uytterhoeven,
NXP Linux Team, Pengutronix Kernel Team, Mauro Carvalho Chehab,
Shawn Guo, linux-arm-kernel, linux-media
Hi Laurent,
On Wed, Mar 18, 2020 at 11:19 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> On Fri, Mar 13, 2020 at 09:31:25PM +0000, Prabhakar Mahadev Lad wrote:
> > On 13 March 2020 21:24, Laurent Pinchart wrote:
> > > On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote:
> > > > While testing on Renesas RZ/G2E platform, noticed the clock frequency
> > > > to be 24242424 as a result the probe failed. However increasing the
> > > > maximum leverage of external clock frequency to 24480000 fixes this
> > > > issue. Since this difference is small enough and is insignificant set
> > > > the same in the driver.
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > > drivers/media/i2c/ov5645.c | 6 ++++--
> > > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > > > index 4fbabf3..b49359b 100644
> > > > --- a/drivers/media/i2c/ov5645.c
> > > > +++ b/drivers/media/i2c/ov5645.c
> > > > @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client)
> > > > }
> > > >
> > > > xclk_freq = clk_get_rate(ov5645->xclk);
> > > > -/* external clock must be 24MHz, allow 1% tolerance */
> > > > -if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> > > > +/* external clock must be 24MHz, allow a minimum 1% and a maximum of 2%
> > > > + * tolerance
> > >
> > > So where do these numbers come from ? I understand that 2% is what you
> > > need to make your clock fit in the range, but why -1%/+2% instead of -
> > > 2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the
> > > range of supported xvclk frequencies to be 6MHz to 54MHz. I understand
> > > that PLL parameters depend on the clock frequency, but could they be
> > > calculated instead of hardcoded, to avoid requiring an exact 24MHz input
> > > frequency ?
> >
> > To be honest I don't have the datasheet for ov5645, the flyer says 6-54Mhz but the
> > logs/comment says 24Mhz.
>
> The OV5645 clock topology is fairly complex, with two PLLs and different
> set of output dividers. It however shouldn't be impossible to calculate
> the PLL configuration in the driver, but would require some dedication,
> and is probably not worth it.
>
> I've discussed the matter with Sakari, and we concluded that this is
> just a sanity check. We advise increasing the tolerance by a bigger
> amount to avoid patching this for every new board (completely
> arbitrarily, +/- 5%), and turning the fatal error into a dev_warn,
> dropping the return -EINVAL statement.
>
Thank you for looking into this. I'll update the patch accordingly.
Cheers,
--Prabhakar
> > > > + */
> > > > +if (xclk_freq < 23760000 || xclk_freq > 24480000) {
> > > > dev_err(dev, "external clock frequency %u is not supported\n",
> > > > xclk_freq);
> > > > return -EINVAL;
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v3 3/4] media: i2c: ov5645: Set maximum leverage of external clock frequency to 24480000
@ 2020-03-19 7:59 ` Lad, Prabhakar
0 siblings, 0 replies; 58+ messages in thread
From: Lad, Prabhakar @ 2020-03-19 7:59 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mark Rutland, devicetree, Pengutronix Kernel Team,
Ezequiel Garcia, Sascha Hauer, Magnus Damm,
Prabhakar Mahadev Lad, linux-kernel, linux-renesas-soc,
Rob Herring, Geert Uytterhoeven, NXP Linux Team, Sakari Ailus,
Shawn Guo, Mauro Carvalho Chehab, Fabio Estevam,
linux-arm-kernel, linux-media
Hi Laurent,
On Wed, Mar 18, 2020 at 11:19 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> On Fri, Mar 13, 2020 at 09:31:25PM +0000, Prabhakar Mahadev Lad wrote:
> > On 13 March 2020 21:24, Laurent Pinchart wrote:
> > > On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote:
> > > > While testing on Renesas RZ/G2E platform, noticed the clock frequency
> > > > to be 24242424 as a result the probe failed. However increasing the
> > > > maximum leverage of external clock frequency to 24480000 fixes this
> > > > issue. Since this difference is small enough and is insignificant set
> > > > the same in the driver.
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > > drivers/media/i2c/ov5645.c | 6 ++++--
> > > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > > > index 4fbabf3..b49359b 100644
> > > > --- a/drivers/media/i2c/ov5645.c
> > > > +++ b/drivers/media/i2c/ov5645.c
> > > > @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client)
> > > > }
> > > >
> > > > xclk_freq = clk_get_rate(ov5645->xclk);
> > > > -/* external clock must be 24MHz, allow 1% tolerance */
> > > > -if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> > > > +/* external clock must be 24MHz, allow a minimum 1% and a maximum of 2%
> > > > + * tolerance
> > >
> > > So where do these numbers come from ? I understand that 2% is what you
> > > need to make your clock fit in the range, but why -1%/+2% instead of -
> > > 2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the
> > > range of supported xvclk frequencies to be 6MHz to 54MHz. I understand
> > > that PLL parameters depend on the clock frequency, but could they be
> > > calculated instead of hardcoded, to avoid requiring an exact 24MHz input
> > > frequency ?
> >
> > To be honest I don't have the datasheet for ov5645, the flyer says 6-54Mhz but the
> > logs/comment says 24Mhz.
>
> The OV5645 clock topology is fairly complex, with two PLLs and different
> set of output dividers. It however shouldn't be impossible to calculate
> the PLL configuration in the driver, but would require some dedication,
> and is probably not worth it.
>
> I've discussed the matter with Sakari, and we concluded that this is
> just a sanity check. We advise increasing the tolerance by a bigger
> amount to avoid patching this for every new board (completely
> arbitrarily, +/- 5%), and turning the fatal error into a dev_warn,
> dropping the return -EINVAL statement.
>
Thank you for looking into this. I'll update the patch accordingly.
Cheers,
--Prabhakar
> > > > + */
> > > > +if (xclk_freq < 23760000 || xclk_freq > 24480000) {
> > > > dev_err(dev, "external clock frequency %u is not supported\n",
> > > > xclk_freq);
> > > > return -EINVAL;
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v3 4/4] ARM: dts: imx6qdl-wandboard: Switch to assigned-clock-rates for ov5645 node
2020-03-13 21:12 ` Lad Prabhakar
@ 2020-03-13 21:12 ` Lad Prabhakar
-1 siblings, 0 replies; 58+ messages in thread
From: Lad Prabhakar @ 2020-03-13 21:12 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus,
NXP Linux Team, Magnus Damm, Ezequiel Garcia, Laurent Pinchart,
Geert Uytterhoeven
Cc: devicetree, linux-kernel, linux-renesas-soc, Lad Prabhakar,
Fabio Estevam, linux-media, linux-arm-kernel
clock-frequency property is now marked as deprecated in ov5645 binding,
so switch to assigned-clock-rates for specifying xclk clock frequency.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
arch/arm/boot/dts/imx6qdl-wandboard.dtsi | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
index c070893..71f5f75 100644
--- a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
@@ -126,7 +126,8 @@
reg = <0x3c>;
clocks = <&clks IMX6QDL_CLK_CKO2>;
clock-names = "xclk";
- clock-frequency = <24000000>;
+ assigned-clocks = <&clks IMX6QDL_CLK_CKO2>;
+ assigned-clock-rates = <24000000>;
vdddo-supply = <®_1p8v>;
vdda-supply = <®_2p8v>;
vddd-supply = <®_1p5v>;
--
2.7.4
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH v3 4/4] ARM: dts: imx6qdl-wandboard: Switch to assigned-clock-rates for ov5645 node
@ 2020-03-13 21:12 ` Lad Prabhakar
0 siblings, 0 replies; 58+ messages in thread
From: Lad Prabhakar @ 2020-03-13 21:12 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus,
NXP Linux Team, Magnus Damm, Ezequiel Garcia, Laurent Pinchart,
Geert Uytterhoeven
Cc: devicetree, linux-kernel, Lad Prabhakar, linux-renesas-soc,
Fabio Estevam, linux-arm-kernel, linux-media
clock-frequency property is now marked as deprecated in ov5645 binding,
so switch to assigned-clock-rates for specifying xclk clock frequency.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
arch/arm/boot/dts/imx6qdl-wandboard.dtsi | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
index c070893..71f5f75 100644
--- a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
@@ -126,7 +126,8 @@
reg = <0x3c>;
clocks = <&clks IMX6QDL_CLK_CKO2>;
clock-names = "xclk";
- clock-frequency = <24000000>;
+ assigned-clocks = <&clks IMX6QDL_CLK_CKO2>;
+ assigned-clock-rates = <24000000>;
vdddo-supply = <®_1p8v>;
vdda-supply = <®_2p8v>;
vddd-supply = <®_1p5v>;
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH v3 4/4] ARM: dts: imx6qdl-wandboard: Switch to assigned-clock-rates for ov5645 node
2020-03-13 21:12 ` Lad Prabhakar
@ 2020-03-13 21:25 ` Laurent Pinchart
-1 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-03-13 21:25 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus,
NXP Linux Team, Magnus Damm, Ezequiel Garcia, Geert Uytterhoeven,
devicetree, linux-kernel, linux-renesas-soc, Fabio Estevam,
linux-media, linux-arm-kernel
Hi Prabhakar,
Thank you for the patch.
On Fri, Mar 13, 2020 at 09:12:34PM +0000, Lad Prabhakar wrote:
> clock-frequency property is now marked as deprecated in ov5645 binding,
Maybe /is now marked as/has been/ to match the recommended change to the
bindings in my reply to 1/4 ?
> so switch to assigned-clock-rates for specifying xclk clock frequency.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> arch/arm/boot/dts/imx6qdl-wandboard.dtsi | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
> index c070893..71f5f75 100644
> --- a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
> @@ -126,7 +126,8 @@
> reg = <0x3c>;
> clocks = <&clks IMX6QDL_CLK_CKO2>;
> clock-names = "xclk";
> - clock-frequency = <24000000>;
> + assigned-clocks = <&clks IMX6QDL_CLK_CKO2>;
> + assigned-clock-rates = <24000000>;
> vdddo-supply = <®_1p8v>;
> vdda-supply = <®_2p8v>;
> vddd-supply = <®_1p5v>;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3 4/4] ARM: dts: imx6qdl-wandboard: Switch to assigned-clock-rates for ov5645 node
@ 2020-03-13 21:25 ` Laurent Pinchart
0 siblings, 0 replies; 58+ messages in thread
From: Laurent Pinchart @ 2020-03-13 21:25 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Mark Rutland, devicetree, Fabio Estevam, Sakari Ailus,
Ezequiel Garcia, Sascha Hauer, Magnus Damm, linux-kernel,
linux-renesas-soc, Rob Herring, Geert Uytterhoeven,
NXP Linux Team, Pengutronix Kernel Team, Mauro Carvalho Chehab,
Shawn Guo, linux-arm-kernel, linux-media
Hi Prabhakar,
Thank you for the patch.
On Fri, Mar 13, 2020 at 09:12:34PM +0000, Lad Prabhakar wrote:
> clock-frequency property is now marked as deprecated in ov5645 binding,
Maybe /is now marked as/has been/ to match the recommended change to the
bindings in my reply to 1/4 ?
> so switch to assigned-clock-rates for specifying xclk clock frequency.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> arch/arm/boot/dts/imx6qdl-wandboard.dtsi | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
> index c070893..71f5f75 100644
> --- a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi
> @@ -126,7 +126,8 @@
> reg = <0x3c>;
> clocks = <&clks IMX6QDL_CLK_CKO2>;
> clock-names = "xclk";
> - clock-frequency = <24000000>;
> + assigned-clocks = <&clks IMX6QDL_CLK_CKO2>;
> + assigned-clock-rates = <24000000>;
> vdddo-supply = <®_1p8v>;
> vdda-supply = <®_2p8v>;
> vddd-supply = <®_1p5v>;
--
Regards,
Laurent Pinchart
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 58+ messages in thread