| Message ID | 1584047552-20166-3-git-send-email-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | ov5645: Switch to assigned-clock-rates | expand |
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
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
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++)
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++)
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
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++)
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(-)