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:

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

Then you need to handle the tolerance in this driver :-)

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel