| Message ID | 1584133954-6953-3-git-send-email-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ov5645: Switch to assigned-clock-rates | expand |
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]; >
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
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];
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(-)