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
[off-list ref]; Sascha Hauer [off-list ref];
Pengutronix Kernel Team [off-list ref]; Rob Herring
[off-list ref]; Mark Rutland [off-list ref]; Sakari
Ailus [off-list ref]; NXP Linux Team [off-list ref];
Magnus Damm [off-list ref]; Ezequiel Garcia
[off-list ref]; Geert Uytterhoeven [off-list ref];
devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-renesas-
soc@vger.kernel.org; Fabio Estevam [off-list ref]; 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:

quoted

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>

quoted

---
 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.

quoted

 unsigned int i;
 u32 xclk_freq;
 int ret;

@@ -1094,12 +1095,18 @@ static int ov5645_probe(struct i2c_client

*client)

quoted

 return PTR_ERR(ov5645->xclk);
 }

-ret = of_property_read_u32(dev->of_node, "clock-frequency",

&xclk_freq);

quoted

-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>

quoted

 /* 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", @@

quoted

-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