| Message ID | 1584133954-6953-1-git-send-email-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| 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
Hi Prabhakar, Thank you for the patch. On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote: > While testing on Renesas RZ/G2E platform, noticed the clock frequency to > be 24242424 as a result the probe failed. However increasing the maximum > leverage of external clock frequency to 24480000 fixes this issue. Since > this difference is small enough and is insignificant set the same in the > driver. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > drivers/media/i2c/ov5645.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > index 4fbabf3..b49359b 100644 > --- a/drivers/media/i2c/ov5645.c > +++ b/drivers/media/i2c/ov5645.c > @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client) > } > > xclk_freq = clk_get_rate(ov5645->xclk); > - /* external clock must be 24MHz, allow 1% tolerance */ > - if (xclk_freq < 23760000 || xclk_freq > 24240000) { > + /* external clock must be 24MHz, allow a minimum 1% and a maximum of 2% > + * tolerance So where do these numbers come from ? I understand that 2% is what you need to make your clock fit in the range, but why -1%/+2% instead of -2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the range of supported xvclk frequencies to be 6MHz to 54MHz. I understand that PLL parameters depend on the clock frequency, but could they be calculated instead of hardcoded, to avoid requiring an exact 24MHz input frequency ? > + */ > + if (xclk_freq < 23760000 || xclk_freq > 24480000) { > dev_err(dev, "external clock frequency %u is not supported\n", > xclk_freq); > return -EINVAL;
Hi Prabhakar, Thank you for the patch. On Fri, Mar 13, 2020 at 09:12:34PM +0000, Lad Prabhakar wrote: > clock-frequency property is now marked as deprecated in ov5645 binding, Maybe /is now marked as/has been/ to match the recommended change to the bindings in my reply to 1/4 ? > so switch to assigned-clock-rates for specifying xclk clock frequency. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > arch/arm/boot/dts/imx6qdl-wandboard.dtsi | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi > index c070893..71f5f75 100644 > --- a/arch/arm/boot/dts/imx6qdl-wandboard.dtsi > +++ b/arch/arm/boot/dts/imx6qdl-wandboard.dtsi > @@ -126,7 +126,8 @@ > reg = <0x3c>; > clocks = <&clks IMX6QDL_CLK_CKO2>; > clock-names = "xclk"; > - clock-frequency = <24000000>; > + assigned-clocks = <&clks IMX6QDL_CLK_CKO2>; > + assigned-clock-rates = <24000000>; > vdddo-supply = <®_1p8v>; > vdda-supply = <®_2p8v>; > vddd-supply = <®_1p5v>;
Hi Laurent, Thank you for the review. > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: 13 March 2020 21:24 > 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 3/4] media: i2c: ov5645: Set maximum leverage of > external clock frequency to 24480000 > > Hi Prabhakar, > > Thank you for the patch. > > On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote: > > While testing on Renesas RZ/G2E platform, noticed the clock frequency > > to be 24242424 as a result the probe failed. However increasing the > > maximum leverage of external clock frequency to 24480000 fixes this > > issue. Since this difference is small enough and is insignificant set > > the same in the driver. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev- > lad.rj@bp.renesas.com> > > --- > > drivers/media/i2c/ov5645.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > > index 4fbabf3..b49359b 100644 > > --- a/drivers/media/i2c/ov5645.c > > +++ b/drivers/media/i2c/ov5645.c > > @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client) > > } > > > > xclk_freq = clk_get_rate(ov5645->xclk); > > -/* external clock must be 24MHz, allow 1% tolerance */ > > -if (xclk_freq < 23760000 || xclk_freq > 24240000) { > > +/* external clock must be 24MHz, allow a minimum 1% and a > maximum of 2% > > + * tolerance > > So where do these numbers come from ? I understand that 2% is what you > need to make your clock fit in the range, but why -1%/+2% instead of - > 2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the > range of supported xvclk frequencies to be 6MHz to 54MHz. I understand > that PLL parameters depend on the clock frequency, but could they be > calculated instead of hardcoded, to avoid requiring an exact 24MHz input > frequency ? > To be honest I don't have the datasheet for ov5645, the flyer says 6-54Mhz but the logs/comment says 24Mhz. Cheers, --Prabhakar > > + */ > > +if (xclk_freq < 23760000 || xclk_freq > 24480000) { > > dev_err(dev, "external clock frequency %u is not > supported\n", > > xclk_freq); > > return -EINVAL; > > -- > 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
Hi Laurent, On Fri, Mar 13, 2020 at 9:31 PM Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > Hi Laurent, > > Thank you for the review. > > > -----Original Message----- > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Sent: 13 March 2020 21:24 > > 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 3/4] media: i2c: ov5645: Set maximum leverage of > > external clock frequency to 24480000 > > > > Hi Prabhakar, > > > > Thank you for the patch. > > > > On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote: > > > While testing on Renesas RZ/G2E platform, noticed the clock frequency > > > to be 24242424 as a result the probe failed. However increasing the > > > maximum leverage of external clock frequency to 24480000 fixes this > > > issue. Since this difference is small enough and is insignificant set > > > the same in the driver. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev- > > lad.rj@bp.renesas.com> > > > --- > > > drivers/media/i2c/ov5645.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > > > index 4fbabf3..b49359b 100644 > > > --- a/drivers/media/i2c/ov5645.c > > > +++ b/drivers/media/i2c/ov5645.c > > > @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client) > > > } > > > > > > xclk_freq = clk_get_rate(ov5645->xclk); > > > -/* external clock must be 24MHz, allow 1% tolerance */ > > > -if (xclk_freq < 23760000 || xclk_freq > 24240000) { > > > +/* external clock must be 24MHz, allow a minimum 1% and a > > maximum of 2% > > > + * tolerance > > > > So where do these numbers come from ? I understand that 2% is what you > > need to make your clock fit in the range, but why -1%/+2% instead of - > > 2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the > > range of supported xvclk frequencies to be 6MHz to 54MHz. I understand > > that PLL parameters depend on the clock frequency, but could they be > > calculated instead of hardcoded, to avoid requiring an exact 24MHz input > > frequency ? > > > To be honest I don't have the datasheet for ov5645, the flyer says 6-54Mhz but the > logs/comment says 24Mhz. > Comparing to ov5640 datasheet [1] (which I am assuming might be similar to ov5645), this change should affect the driver. [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf Cheers, --Prabhakar > Cheers, > --Prabhakar > > > > + */ > > > +if (xclk_freq < 23760000 || xclk_freq > 24480000) { > > > dev_err(dev, "external clock frequency %u is not > > supported\n", > > > xclk_freq); > > > return -EINVAL; > > > > -- > > 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
Hi Prabhakar, On Fri, Mar 13, 2020 at 09:31:25PM +0000, Prabhakar Mahadev Lad wrote: > On 13 March 2020 21:24, Laurent Pinchart wrote: > > On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote: > > > While testing on Renesas RZ/G2E platform, noticed the clock frequency > > > to be 24242424 as a result the probe failed. However increasing the > > > maximum leverage of external clock frequency to 24480000 fixes this > > > issue. Since this difference is small enough and is insignificant set > > > the same in the driver. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > --- > > > drivers/media/i2c/ov5645.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > > > index 4fbabf3..b49359b 100644 > > > --- a/drivers/media/i2c/ov5645.c > > > +++ b/drivers/media/i2c/ov5645.c > > > @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client) > > > } > > > > > > xclk_freq = clk_get_rate(ov5645->xclk); > > > -/* external clock must be 24MHz, allow 1% tolerance */ > > > -if (xclk_freq < 23760000 || xclk_freq > 24240000) { > > > +/* external clock must be 24MHz, allow a minimum 1% and a maximum of 2% > > > + * tolerance > > > > So where do these numbers come from ? I understand that 2% is what you > > need to make your clock fit in the range, but why -1%/+2% instead of - > > 2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the > > range of supported xvclk frequencies to be 6MHz to 54MHz. I understand > > that PLL parameters depend on the clock frequency, but could they be > > calculated instead of hardcoded, to avoid requiring an exact 24MHz input > > frequency ? > > To be honest I don't have the datasheet for ov5645, the flyer says 6-54Mhz but the > logs/comment says 24Mhz. The OV5645 clock topology is fairly complex, with two PLLs and different set of output dividers. It however shouldn't be impossible to calculate the PLL configuration in the driver, but would require some dedication, and is probably not worth it. I've discussed the matter with Sakari, and we concluded that this is just a sanity check. We advise increasing the tolerance by a bigger amount to avoid patching this for every new board (completely arbitrarily, +/- 5%), and turning the fatal error into a dev_warn, dropping the return -EINVAL statement. > > > + */ > > > +if (xclk_freq < 23760000 || xclk_freq > 24480000) { > > > dev_err(dev, "external clock frequency %u is not supported\n", > > > xclk_freq); > > > return -EINVAL;
Hi Prabhakar, On Wed, Mar 18, 2020 at 10:41:57PM +0000, Lad, Prabhakar wrote: > On Fri, Mar 13, 2020 at 9:31 PM Prabhakar Mahadev Lad wrote: > > On 13 March 2020 21:24, Laurent Pinchart wrote: > >> On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote: > >>> While testing on Renesas RZ/G2E platform, noticed the clock frequency > >>> to be 24242424 as a result the probe failed. However increasing the > >>> maximum leverage of external clock frequency to 24480000 fixes this > >>> issue. Since this difference is small enough and is insignificant set > >>> the same in the driver. > >>> > >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > >>> --- > >>> drivers/media/i2c/ov5645.c | 6 ++++-- > >>> 1 file changed, 4 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > >>> index 4fbabf3..b49359b 100644 > >>> --- a/drivers/media/i2c/ov5645.c > >>> +++ b/drivers/media/i2c/ov5645.c > >>> @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client) > >>> } > >>> > >>> xclk_freq = clk_get_rate(ov5645->xclk); > >>> -/* external clock must be 24MHz, allow 1% tolerance */ > >>> -if (xclk_freq < 23760000 || xclk_freq > 24240000) { > >>> +/* external clock must be 24MHz, allow a minimum 1% and a > >> maximum of 2% > >>> + * tolerance > >> > >> So where do these numbers come from ? I understand that 2% is what you > >> need to make your clock fit in the range, but why -1%/+2% instead of - > >> 2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the > >> range of supported xvclk frequencies to be 6MHz to 54MHz. I understand > >> that PLL parameters depend on the clock frequency, but could they be > >> calculated instead of hardcoded, to avoid requiring an exact 24MHz input > >> frequency ? > >> > > To be honest I don't have the datasheet for ov5645, the flyer says 6-54Mhz but the > > logs/comment says 24Mhz. > > > Comparing to ov5640 datasheet [1] (which I am assuming might be > similar to ov5645), Let's assume this to be the case, I see no reason not to :-) > this change should affect the driver. How do you mean ? > [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf > > >>> + */ > >>> +if (xclk_freq < 23760000 || xclk_freq > 24480000) { > >>> dev_err(dev, "external clock frequency %u is not supported\n", > >>> xclk_freq); > >>> return -EINVAL;
Hi Laurent, On Wed, Mar 18, 2020 at 11:19 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Prabhakar, > > On Fri, Mar 13, 2020 at 09:31:25PM +0000, Prabhakar Mahadev Lad wrote: > > On 13 March 2020 21:24, Laurent Pinchart wrote: > > > On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote: > > > > While testing on Renesas RZ/G2E platform, noticed the clock frequency > > > > to be 24242424 as a result the probe failed. However increasing the > > > > maximum leverage of external clock frequency to 24480000 fixes this > > > > issue. Since this difference is small enough and is insignificant set > > > > the same in the driver. > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > --- > > > > drivers/media/i2c/ov5645.c | 6 ++++-- > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > > > > index 4fbabf3..b49359b 100644 > > > > --- a/drivers/media/i2c/ov5645.c > > > > +++ b/drivers/media/i2c/ov5645.c > > > > @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client) > > > > } > > > > > > > > xclk_freq = clk_get_rate(ov5645->xclk); > > > > -/* external clock must be 24MHz, allow 1% tolerance */ > > > > -if (xclk_freq < 23760000 || xclk_freq > 24240000) { > > > > +/* external clock must be 24MHz, allow a minimum 1% and a maximum of 2% > > > > + * tolerance > > > > > > So where do these numbers come from ? I understand that 2% is what you > > > need to make your clock fit in the range, but why -1%/+2% instead of - > > > 2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the > > > range of supported xvclk frequencies to be 6MHz to 54MHz. I understand > > > that PLL parameters depend on the clock frequency, but could they be > > > calculated instead of hardcoded, to avoid requiring an exact 24MHz input > > > frequency ? > > > > To be honest I don't have the datasheet for ov5645, the flyer says 6-54Mhz but the > > logs/comment says 24Mhz. > > The OV5645 clock topology is fairly complex, with two PLLs and different > set of output dividers. It however shouldn't be impossible to calculate > the PLL configuration in the driver, but would require some dedication, > and is probably not worth it. > > I've discussed the matter with Sakari, and we concluded that this is > just a sanity check. We advise increasing the tolerance by a bigger > amount to avoid patching this for every new board (completely > arbitrarily, +/- 5%), and turning the fatal error into a dev_warn, > dropping the return -EINVAL statement. > Thank you for looking into this. I'll update the patch accordingly. Cheers, --Prabhakar > > > > + */ > > > > +if (xclk_freq < 23760000 || xclk_freq > 24480000) { > > > > dev_err(dev, "external clock frequency %u is not supported\n", > > > > xclk_freq); > > > > return -EINVAL; > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Laurent, On Wed, Mar 18, 2020 at 11:22 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Prabhakar, > > On Wed, Mar 18, 2020 at 10:41:57PM +0000, Lad, Prabhakar wrote: > > On Fri, Mar 13, 2020 at 9:31 PM Prabhakar Mahadev Lad wrote: > > > On 13 March 2020 21:24, Laurent Pinchart wrote: > > >> On Fri, Mar 13, 2020 at 09:12:33PM +0000, Lad Prabhakar wrote: > > >>> While testing on Renesas RZ/G2E platform, noticed the clock frequency > > >>> to be 24242424 as a result the probe failed. However increasing the > > >>> maximum leverage of external clock frequency to 24480000 fixes this > > >>> issue. Since this difference is small enough and is insignificant set > > >>> the same in the driver. > > >>> > > >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > >>> --- > > >>> drivers/media/i2c/ov5645.c | 6 ++++-- > > >>> 1 file changed, 4 insertions(+), 2 deletions(-) > > >>> > > >>> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > > >>> index 4fbabf3..b49359b 100644 > > >>> --- a/drivers/media/i2c/ov5645.c > > >>> +++ b/drivers/media/i2c/ov5645.c > > >>> @@ -1107,8 +1107,10 @@ static int ov5645_probe(struct i2c_client *client) > > >>> } > > >>> > > >>> xclk_freq = clk_get_rate(ov5645->xclk); > > >>> -/* external clock must be 24MHz, allow 1% tolerance */ > > >>> -if (xclk_freq < 23760000 || xclk_freq > 24240000) { > > >>> +/* external clock must be 24MHz, allow a minimum 1% and a > > >> maximum of 2% > > >>> + * tolerance > > >> > > >> So where do these numbers come from ? I understand that 2% is what you > > >> need to make your clock fit in the range, but why -1%/+2% instead of - > > >> 2%/+2% ? And why not 2.5 or 3% ? The sensor datasheet documents the > > >> range of supported xvclk frequencies to be 6MHz to 54MHz. I understand > > >> that PLL parameters depend on the clock frequency, but could they be > > >> calculated instead of hardcoded, to avoid requiring an exact 24MHz input > > >> frequency ? > > >> > > > To be honest I don't have the datasheet for ov5645, the flyer says 6-54Mhz but the > > > logs/comment says 24Mhz. > > > > > Comparing to ov5640 datasheet [1] (which I am assuming might be > > similar to ov5645), > > Let's assume this to be the case, I see no reason not to :-) > > > this change should affect the driver. > > How do you mean ? > Oops sorry for for the typo I meant shouldn't affect the driver :) Cheers, --Prabhakar > > [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf > > > > >>> + */ > > >>> +if (xclk_freq < 23760000 || xclk_freq > 24480000) { > > >>> dev_err(dev, "external clock frequency %u is not supported\n", > > >>> xclk_freq); > > >>> return -EINVAL; > > -- > Regards, > > Laurent Pinchart
Hi All, This patch series adds support for using assigned-clock-rates for specifying clock rates for ov5645 driver Thanks, Prabhakar Changed for v3: * Dropped reading assigned-clock-rates * Increased the maximum clock frequency to 24480000 Changes for v2: * Instead of completely dropping clock-frequency property marked it as deprecated. * Split up imx6qdl-wandboard.dtsi changes as separate patch. Lad Prabhakar (4): media: dt-bindings: media: i2c: Switch to assigned-clock-rates media: i2c: ov5645: Switch to assigned-clock-rates media: i2c: ov5645: Set maximum leverage of external clock frequency to 24480000 ARM: dts: imx6qdl-wandboard: Switch to assigned-clock-rates for ov5645 node .../devicetree/bindings/media/i2c/ov5645.txt | 5 ++-- arch/arm/boot/dts/imx6qdl-wandboard.dtsi | 3 ++- drivers/media/i2c/ov5645.c | 27 ++++++++++++---------- 3 files changed, 20 insertions(+), 15 deletions(-)