RE: [PATCH v2 2/3] media: i2c: ov5645: Switch to assigned-clock-rates

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
[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 10:13 PM Lad Prabhakar <prabhakar.mahadev-
lad.rj@bp.renesas.com> 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>

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)
        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",
+                                  &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",
+                                          &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);
        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

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

quoted

        }

        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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel