* [PATCH v2 0/3] ov5645: Switch to assigned-clock-rates
@ 2020-03-12 21:12 Lad Prabhakar
2020-03-12 21:12 ` [PATCH v2 1/3] media: dt-bindings: media: i2c: " Lad Prabhakar
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Lad Prabhakar @ 2020-03-12 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
Cc: devicetree, linux-kernel, linux-renesas-soc, Lad Prabhakar,
Fabio Estevam, linux-media, linux-arm-kernel
Hi All,
This patch series adds support for using assigned-clock-rates for
specifying clock rates for ov5645 driver
Thanks,
Prabhakar
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 (3):
media: dt-bindings: media: i2c: Switch to assigned-clock-rates
media: i2c: ov5645: Switch to assigned-clock-rates
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 | 24 +++++++++++++++-------
3 files changed, 22 insertions(+), 10 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v2 1/3] media: dt-bindings: media: i2c: Switch to assigned-clock-rates 2020-03-12 21:12 [PATCH v2 0/3] ov5645: Switch to assigned-clock-rates Lad Prabhakar @ 2020-03-12 21:12 ` Lad Prabhakar 2020-03-12 21:12 ` [PATCH v2 2/3] media: i2c: ov5645: " Lad Prabhakar 2020-03-12 21:12 ` [PATCH v2 3/3] ARM: dts: imx6qdl-wandboard: Switch to assigned-clock-rates for ov5645 node Lad Prabhakar 2 siblings, 0 replies; 9+ messages in thread From: Lad Prabhakar @ 2020-03-12 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 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] 9+ messages in thread
* [PATCH v2 2/3] media: i2c: ov5645: Switch to assigned-clock-rates 2020-03-12 21:12 [PATCH v2 0/3] ov5645: Switch to assigned-clock-rates Lad Prabhakar 2020-03-12 21:12 ` [PATCH v2 1/3] media: dt-bindings: media: i2c: " Lad Prabhakar @ 2020-03-12 21:12 ` Lad Prabhakar 2020-03-12 21:41 ` Geert Uytterhoeven 2020-03-12 23:00 ` Laurent Pinchart 2020-03-12 21:12 ` [PATCH v2 3/3] ARM: dts: imx6qdl-wandboard: Switch to assigned-clock-rates for ov5645 node Lad Prabhakar 2 siblings, 2 replies; 9+ messages in thread From: Lad Prabhakar @ 2020-03-12 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 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 | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c index a6c17d1..6cd16c5 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,10 +1095,17 @@ static int ov5645_probe(struct i2c_client *client) return PTR_ERR(ov5645->xclk); } - ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq); + ret = of_property_read_u32(dev->of_node, "assigned-clock-rates", + &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) { + dev_err(dev, "could not get xclk frequency\n"); + return ret; + } + set_clk = true; } /* external clock must be 24MHz, allow 1% tolerance */ @@ -1107,10 +1115,12 @@ 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; + if (set_clk) { + 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++) -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] media: i2c: ov5645: Switch to assigned-clock-rates 2020-03-12 21:12 ` [PATCH v2 2/3] media: i2c: ov5645: " Lad Prabhakar @ 2020-03-12 21:41 ` Geert Uytterhoeven 2020-03-12 21:52 ` Prabhakar Mahadev Lad 2020-03-12 23:00 ` Laurent Pinchart 1 sibling, 1 reply; 9+ messages in thread From: Geert Uytterhoeven @ 2020-03-12 21:41 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, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List, Linux-Renesas, Fabio Estevam, Linux ARM, Linux Media Mailing List Hi Prabhakar, On Thu, Mar 12, 2020 at 10:13 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> 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> Thanks for your patch! > --- 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,10 +1095,17 @@ static int ov5645_probe(struct i2c_client *client) > return PTR_ERR(ov5645->xclk); > } > > - ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq); > + ret = of_property_read_u32(dev->of_node, "assigned-clock-rates", > + &xclk_freq); > if (ret) { I think you can just leave out the above check... > - 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) { ... and ignore the absence of the deprecated property. > + dev_err(dev, "could not get xclk frequency\n"); > + return ret; > + } > + set_clk = true; I.e. just /* check if deprecated property clock-frequency is defined */ xclk_freq = 0; of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq); if (xclk_freq) { ret = clk_set_rate(ov5645->xclk, xclk_freq); if (ret) { dev_err(dev, "could not set xclk frequency\n"); return ret; } } else { xclk_freq = clk_get_rate(ov5645->xclk, xclk_freq); } > } > > /* external clock must be 24MHz, allow 1% tolerance */ > @@ -1107,10 +1115,12 @@ 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; > + if (set_clk) { > + 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++) > -- > 2.7.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v2 2/3] media: i2c: ov5645: Switch to assigned-clock-rates 2020-03-12 21:41 ` Geert Uytterhoeven @ 2020-03-12 21:52 ` Prabhakar Mahadev Lad 2020-03-12 23:02 ` Laurent Pinchart 0 siblings, 1 reply; 9+ messages in thread From: Prabhakar Mahadev Lad @ 2020-03-12 21:52 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus, NXP Linux Team, Magnus Damm, Ezequiel Garcia, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List, Linux-Renesas, Fabio Estevam, Linux ARM, Linux Media Mailing List Hi Geert, Thank you for the quick review. > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: 12 March 2020 21:42 > 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>; open list:OPEN FIRMWARE AND FLATTENED > DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; Linux Kernel Mailing > List <linux-kernel@vger.kernel.org>; Linux-Renesas <linux-renesas- > soc@vger.kernel.org>; Fabio Estevam <festevam@gmail.com>; Linux ARM > <linux-arm-kernel@lists.infradead.org>; Linux Media Mailing List <linux- > media@vger.kernel.org> > Subject: Re: [PATCH v2 2/3] media: i2c: ov5645: Switch to assigned-clock- > rates > > Hi Prabhakar, > > On Thu, Mar 12, 2020 at 10:13 PM Lad Prabhakar <prabhakar.mahadev- > lad.rj@bp.renesas.com> 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> > > Thanks for your patch! > > > --- 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,10 +1095,17 @@ static int ov5645_probe(struct i2c_client > *client) > > return PTR_ERR(ov5645->xclk); > > } > > > > - ret = of_property_read_u32(dev->of_node, "clock-frequency", > &xclk_freq); > > + ret = of_property_read_u32(dev->of_node, "assigned-clock-rates", > > + &xclk_freq); > > if (ret) { > > I think you can just leave out the above check... > > > - 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) { > > ... and ignore the absence of the deprecated property. > > > + dev_err(dev, "could not get xclk frequency\n"); > > + return ret; > > + } > > + set_clk = true; > > I.e. just > > /* check if deprecated property clock-frequency is defined */ > xclk_freq = 0; > of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq); > if (xclk_freq) { > ret = clk_set_rate(ov5645->xclk, xclk_freq); > if (ret) { > dev_err(dev, "could not set xclk frequency\n"); > return ret; > } > } else { > xclk_freq = clk_get_rate(ov5645->xclk, xclk_freq); > } > I did think about it initially, but realized the clk_get_rate may vary platform to platform for example in G2E clk_get_rate() returns a frequency of 24242424 with assigned-clock-rates set to 24000000 and probe would fail due to a check for external clock must be 24MHz, with 1% tolerance. Cheers, --Prabhakar > > } > > > > /* external clock must be 24MHz, allow 1% tolerance */ @@ > > -1107,10 +1115,12 @@ 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; > > + if (set_clk) { > > + 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++) > > -- > > 2.7.4 > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > -- > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux- > m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds 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] 9+ messages in thread
* Re: [PATCH v2 2/3] media: i2c: ov5645: Switch to assigned-clock-rates 2020-03-12 21:52 ` Prabhakar Mahadev Lad @ 2020-03-12 23:02 ` Laurent Pinchart 2020-03-13 8:02 ` Prabhakar Mahadev Lad 0 siblings, 1 reply; 9+ messages in thread From: Laurent Pinchart @ 2020-03-12 23:02 UTC (permalink / raw) To: Prabhakar Mahadev Lad Cc: Geert Uytterhoeven, Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus, NXP Linux Team, Magnus Damm, Ezequiel Garcia, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List, Linux-Renesas, Fabio Estevam, Linux ARM, Linux Media Mailing List Hi Prabhakar, On Thu, Mar 12, 2020 at 09:52:38PM +0000, Prabhakar Mahadev Lad wrote: > On 12 March 2020 21:42, Geert Uytterhoeven wrote: > > On Thu, Mar 12, 2020 at 10:13 PM 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> > > > > Thanks for your patch! > > > > > --- 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,10 +1095,17 @@ static int ov5645_probe(struct i2c_client > > *client) > > > return PTR_ERR(ov5645->xclk); > > > } > > > > > > - ret = of_property_read_u32(dev->of_node, "clock-frequency", > > &xclk_freq); > > > + ret = of_property_read_u32(dev->of_node, "assigned-clock-rates", > > > + &xclk_freq); > > > if (ret) { > > > > I think you can just leave out the above check... > > > > > - 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) { > > > > ... and ignore the absence of the deprecated property. > > > > > + dev_err(dev, "could not get xclk frequency\n"); > > > + return ret; > > > + } > > > + set_clk = true; > > > > I.e. just > > > > /* check if deprecated property clock-frequency is defined */ > > xclk_freq = 0; > > of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq); > > if (xclk_freq) { > > ret = clk_set_rate(ov5645->xclk, xclk_freq); > > if (ret) { > > dev_err(dev, "could not set xclk frequency\n"); > > return ret; > > } > > } else { > > xclk_freq = clk_get_rate(ov5645->xclk, xclk_freq); > > } > > > I did think about it initially, but realized the clk_get_rate may vary platform to platform > for example in G2E clk_get_rate() returns a frequency of 24242424 with > assigned-clock-rates set to 24000000 and probe would fail due to a check for > external clock must be 24MHz, with 1% tolerance. Then you need to handle the tolerance in this driver :-) > > > } > > > > > > /* external clock must be 24MHz, allow 1% tolerance */ @@ > > > -1107,10 +1115,12 @@ 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; > > > + if (set_clk) { > > > + 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++) -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v2 2/3] media: i2c: ov5645: Switch to assigned-clock-rates 2020-03-12 23:02 ` Laurent Pinchart @ 2020-03-13 8:02 ` Prabhakar Mahadev Lad 0 siblings, 0 replies; 9+ messages in thread From: Prabhakar Mahadev Lad @ 2020-03-13 8:02 UTC (permalink / raw) To: Laurent Pinchart Cc: Geert Uytterhoeven, Mauro Carvalho Chehab, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Rob Herring, Mark Rutland, Sakari Ailus, NXP Linux Team, Magnus Damm, Ezequiel Garcia, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List, Linux-Renesas, Fabio Estevam, Linux ARM, Linux Media Mailing List Hi Laurent, Thank you for the review. > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: 12 March 2020 23:03 > To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>; 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>; open list:OPEN FIRMWARE AND FLATTENED > DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; Linux Kernel Mailing > List <linux-kernel@vger.kernel.org>; Linux-Renesas <linux-renesas- > soc@vger.kernel.org>; Fabio Estevam <festevam@gmail.com>; Linux ARM > <linux-arm-kernel@lists.infradead.org>; Linux Media Mailing List <linux- > media@vger.kernel.org> > Subject: Re: [PATCH v2 2/3] media: i2c: ov5645: Switch to assigned-clock- > rates > > Hi Prabhakar, > > On Thu, Mar 12, 2020 at 09:52:38PM +0000, Prabhakar Mahadev Lad wrote: > > On 12 March 2020 21:42, Geert Uytterhoeven wrote: > > > On Thu, Mar 12, 2020 at 10:13 PM 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> > > > > > > Thanks for your patch! > > > > > > > --- 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,10 +1095,17 @@ static int ov5645_probe(struct i2c_client > > > *client) > > > > return PTR_ERR(ov5645->xclk); > > > > } > > > > > > > > - ret = of_property_read_u32(dev->of_node, "clock-frequency", > > > &xclk_freq); > > > > + ret = of_property_read_u32(dev->of_node, "assigned-clock- > rates", > > > > + &xclk_freq); > > > > if (ret) { > > > > > > I think you can just leave out the above check... > > > > > > > - 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) { > > > > > > ... and ignore the absence of the deprecated property. > > > > > > > + dev_err(dev, "could not get xclk frequency\n"); > > > > + return ret; > > > > + } > > > > + set_clk = true; > > > > > > I.e. just > > > > > > /* check if deprecated property clock-frequency is defined */ > > > xclk_freq = 0; > > > of_property_read_u32(dev->of_node, "clock-frequency", > &xclk_freq); > > > if (xclk_freq) { > > > ret = clk_set_rate(ov5645->xclk, xclk_freq); > > > if (ret) { > > > dev_err(dev, "could not set xclk frequency\n"); > > > return ret; > > > } > > > } else { > > > xclk_freq = clk_get_rate(ov5645->xclk, xclk_freq); > > > } > > > > > I did think about it initially, but realized the clk_get_rate may vary > > platform to platform for example in G2E clk_get_rate() returns a > > frequency of 24242424 with assigned-clock-rates set to 24000000 and > > probe would fail due to a check for external clock must be 24MHz, with 1% > tolerance. > > Then you need to handle the tolerance in this driver :-) > Sure I will increase the tolerance here. Cheers, --Prabhakar > > > > } > > > > > > > > /* external clock must be 24MHz, allow 1% tolerance */ @@ > > > > -1107,10 +1115,12 @@ 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; > > > > + if (set_clk) { > > > > + 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++) > > -- > 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] 9+ messages in thread
* Re: [PATCH v2 2/3] media: i2c: ov5645: Switch to assigned-clock-rates 2020-03-12 21:12 ` [PATCH v2 2/3] media: i2c: ov5645: " Lad Prabhakar 2020-03-12 21:41 ` Geert Uytterhoeven @ 2020-03-12 23:00 ` Laurent Pinchart 1 sibling, 0 replies; 9+ messages in thread From: Laurent Pinchart @ 2020-03-12 23:00 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, devicetree, linux-kernel, linux-renesas-soc, Fabio Estevam, linux-media, linux-arm-kernel Hi Prabakhar, Thank you for the patch. On Thu, Mar 12, 2020 at 09:12:31PM +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 | 24 +++++++++++++++++------- > 1 file changed, 17 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > index a6c17d1..6cd16c5 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,10 +1095,17 @@ static int ov5645_probe(struct i2c_client *client) > return PTR_ERR(ov5645->xclk); > } > > - ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq); > + ret = of_property_read_u32(dev->of_node, "assigned-clock-rates", > + &xclk_freq); You shouldn't read the assigned-clock-rates property, you should instead get the rate from the clock with clk_get_rate(). > 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) { > + dev_err(dev, "could not get xclk frequency\n"); > + return ret; > + } > + set_clk = true; > } > > /* external clock must be 24MHz, allow 1% tolerance */ > @@ -1107,10 +1115,12 @@ 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; > + if (set_clk) { > + 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++) -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 3/3] ARM: dts: imx6qdl-wandboard: Switch to assigned-clock-rates for ov5645 node 2020-03-12 21:12 [PATCH v2 0/3] ov5645: Switch to assigned-clock-rates Lad Prabhakar 2020-03-12 21:12 ` [PATCH v2 1/3] media: dt-bindings: media: i2c: " Lad Prabhakar 2020-03-12 21:12 ` [PATCH v2 2/3] media: i2c: ov5645: " Lad Prabhakar @ 2020-03-12 21:12 ` Lad Prabhakar 2 siblings, 0 replies; 9+ messages in thread From: Lad Prabhakar @ 2020-03-12 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 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] 9+ messages in thread
end of thread, other threads:[~2020-03-13 8:02 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-12 21:12 [PATCH v2 0/3] ov5645: Switch to assigned-clock-rates Lad Prabhakar 2020-03-12 21:12 ` [PATCH v2 1/3] media: dt-bindings: media: i2c: " Lad Prabhakar 2020-03-12 21:12 ` [PATCH v2 2/3] media: i2c: ov5645: " Lad Prabhakar 2020-03-12 21:41 ` Geert Uytterhoeven 2020-03-12 21:52 ` Prabhakar Mahadev Lad 2020-03-12 23:02 ` Laurent Pinchart 2020-03-13 8:02 ` Prabhakar Mahadev Lad 2020-03-12 23:00 ` Laurent Pinchart 2020-03-12 21:12 ` [PATCH v2 3/3] ARM: dts: imx6qdl-wandboard: Switch to assigned-clock-rates for ov5645 node Lad Prabhakar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).