| Message ID | 20201205183355.6488-1-michael.srba@seznam.cz (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | [1/3] media: i2c: imx219: add support for specifying clock-frequencies | expand |
Hi Michael, On Sat, Dec 5, 2020 at 7:36 PM <michael.srba@seznam.cz> wrote: > From: Michael Srba <Michael.Srba@seznam.cz> > > This patch adds 1% tolerance on input clock, similar to other camera sensor > drivers. It also allows for specifying the actual clock in the device tree, > instead of relying on it being already set to the right frequency (which is > often not the case). > > Signed-off-by: Michael Srba <Michael.Srba@seznam.cz> Thanks for your patch! > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -1443,13 +1443,26 @@ static int imx219_probe(struct i2c_client *client) > return PTR_ERR(imx219->xclk); > } > > - imx219->xclk_freq = clk_get_rate(imx219->xclk); > - if (imx219->xclk_freq != IMX219_XCLK_FREQ) { > + ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", &imx219->xclk_freq); > + if (ret) { > + dev_err(dev, "could not get xclk frequency\n"); > + return ret; This breaks compatibility with existing DTBs, which do not have the clock-frequency property. For backwards compatibility, you should assume the default 24 MHz instead of returning an error. > + } > + > + /* this driver currently expects 24MHz; allow 1% tolerance */ > + if (imx219->xclk_freq < 23760000 || imx219->xclk_freq > 24240000) { > dev_err(dev, "xclk frequency not supported: %d Hz\n", > imx219->xclk_freq); > return -EINVAL; > } > > + ret = clk_set_rate(imx219->xclk, imx219->xclk_freq); > + if (ret) { > + dev_err(dev, "could not set xclk frequency\n"); > + return ret; > + } > + > + > ret = imx219_get_regulators(imx219); > if (ret) { > dev_err(dev, "failed to get regulators\n"); Gr{oetje,eeting}s, Geert
On 05. 12. 20 19:54, Geert Uytterhoeven wrote: > Hi Michael, > > On Sat, Dec 5, 2020 at 7:36 PM <michael.srba@seznam.cz> wrote: >> From: Michael Srba <Michael.Srba@seznam.cz> >> >> This patch adds 1% tolerance on input clock, similar to other camera sensor >> drivers. It also allows for specifying the actual clock in the device tree, >> instead of relying on it being already set to the right frequency (which is >> often not the case). >> >> Signed-off-by: Michael Srba <Michael.Srba@seznam.cz> > Thanks for your patch! > >> --- a/drivers/media/i2c/imx219.c >> +++ b/drivers/media/i2c/imx219.c >> @@ -1443,13 +1443,26 @@ static int imx219_probe(struct i2c_client *client) >> return PTR_ERR(imx219->xclk); >> } >> >> - imx219->xclk_freq = clk_get_rate(imx219->xclk); >> - if (imx219->xclk_freq != IMX219_XCLK_FREQ) { >> + ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", &imx219->xclk_freq); >> + if (ret) { >> + dev_err(dev, "could not get xclk frequency\n"); >> + return ret; > This breaks compatibility with existing DTBs, which do not have the > clock-frequency property. > For backwards compatibility, you should assume the default 24 MHz > instead of returning an error. Good point, will do. >> + } >> + >> + /* this driver currently expects 24MHz; allow 1% tolerance */ >> + if (imx219->xclk_freq < 23760000 || imx219->xclk_freq > 24240000) { >> dev_err(dev, "xclk frequency not supported: %d Hz\n", >> imx219->xclk_freq); >> return -EINVAL; >> } >> >> + ret = clk_set_rate(imx219->xclk, imx219->xclk_freq); >> + if (ret) { >> + dev_err(dev, "could not set xclk frequency\n"); >> + return ret; >> + } >> + >> + >> ret = imx219_get_regulators(imx219); >> if (ret) { >> dev_err(dev, "failed to get regulators\n"); > Gr{oetje,eeting}s, > > Geert > Michael
On Sat, Dec 05, 2020 at 07:33:53PM +0100, michael.srba@seznam.cz wrote: > From: Michael Srba <Michael.Srba@seznam.cz> > > This patch adds 1% tolerance on input clock, similar to other camera sensor > drivers. It also allows for specifying the actual clock in the device tree, > instead of relying on it being already set to the right frequency (which is > often not the case). All this can be achieved with assigned-clocks-rate and basically you do not add here value. At least not for DT-based systems. The supported clock rates will be the same. The method of choosing frequency is over-complicated comparing to simple assigned-clocks. If this is for ACPI systems, please document in commit msg why you cannot used assigned-clocks and choose this solution. > > Signed-off-by: Michael Srba <Michael.Srba@seznam.cz> > --- > drivers/media/i2c/imx219.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index f64c0ef7a897..a8f05562d0af 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -1443,13 +1443,26 @@ static int imx219_probe(struct i2c_client *client) > return PTR_ERR(imx219->xclk); > } > > - imx219->xclk_freq = clk_get_rate(imx219->xclk); > - if (imx219->xclk_freq != IMX219_XCLK_FREQ) { > + ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", &imx219->xclk_freq); > + if (ret) { > + dev_err(dev, "could not get xclk frequency\n"); > + return ret; > + } > + > + /* this driver currently expects 24MHz; allow 1% tolerance */ > + if (imx219->xclk_freq < 23760000 || imx219->xclk_freq > 24240000) { > dev_err(dev, "xclk frequency not supported: %d Hz\n", > imx219->xclk_freq); > return -EINVAL; > } > > + ret = clk_set_rate(imx219->xclk, imx219->xclk_freq); > + if (ret) { > + dev_err(dev, "could not set xclk frequency\n"); > + return ret; > + } > + > + No need for double line break. Best regards, Krzysztof
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c index f64c0ef7a897..a8f05562d0af 100644 --- a/drivers/media/i2c/imx219.c +++ b/drivers/media/i2c/imx219.c @@ -1443,13 +1443,26 @@ static int imx219_probe(struct i2c_client *client) return PTR_ERR(imx219->xclk); } - imx219->xclk_freq = clk_get_rate(imx219->xclk); - if (imx219->xclk_freq != IMX219_XCLK_FREQ) { + ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", &imx219->xclk_freq); + if (ret) { + dev_err(dev, "could not get xclk frequency\n"); + return ret; + } + + /* this driver currently expects 24MHz; allow 1% tolerance */ + if (imx219->xclk_freq < 23760000 || imx219->xclk_freq > 24240000) { dev_err(dev, "xclk frequency not supported: %d Hz\n", imx219->xclk_freq); return -EINVAL; } + ret = clk_set_rate(imx219->xclk, imx219->xclk_freq); + if (ret) { + dev_err(dev, "could not set xclk frequency\n"); + return ret; + } + + ret = imx219_get_regulators(imx219); if (ret) { dev_err(dev, "failed to get regulators\n");