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
[off-list ref]; 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]; open list:OPEN FIRMWARE AND FLATTENED
DEVICE TREE BINDINGS [off-list ref]; Linux Kernel Mailing
List [off-list ref]; Linux-Renesas <linux-renesas-
soc@vger.kernel.org>; Fabio Estevam [off-list ref]; Linux ARM
[off-list ref]; 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:

quoted

On 12 March 2020 21:42, Geert Uytterhoeven wrote:

quoted

On Thu, Mar 12, 2020 at 10:13 PM 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
[off-list ref]

Thanks for your patch!

quoted

--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c

@@ -1055,6 +1055,7 @@ static int ov5645_probe(struct i2c_client

*client)

quoted

quoted

quoted

        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)

quoted

                return PTR_ERR(ov5645->xclk);
        }

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

&xclk_freq);

quoted

+       ret = of_property_read_u32(dev->of_node, "assigned-clock-

rates",

quoted

quoted

quoted

+                                  &xclk_freq);
        if (ret) {

I think you can just leave out the above check...

quoted

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

quoted

quoted

quoted

+                                          &xclk_freq);
+               if (ret) {

... and ignore the absence of the deprecated property.

quoted

+                       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);

quoted

quoted

        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

quoted

quoted

quoted

        }

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