| Message ID | 20201205183355.6488-1-michael.srba@seznam.cz |
|---|---|
| State | New |
| 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
> 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 the clock-frequency property to all device trees that use >> the imx219 binding, with the value of exactly 24Mhz which was previously >> implicitly assumed. >> >> Signed-off-by: Michael Srba <Michael.Srba@seznam.cz> > Thanks for your patch! > >> --- a/arch/arm64/boot/dts/renesas/aistarvision-mipi-adapter-2.1.dtsi >> +++ b/arch/arm64/boot/dts/renesas/aistarvision-mipi-adapter-2.1.dtsi >> @@ -82,6 +82,7 @@ imx219: imx219@10 { >> compatible = "sony,imx219"; >> reg = <0x10>; >> clocks = <&osc25250_clk>; >> + clock-frequency = <24000000>; >> VANA-supply = <&imx219_vana_2v8>; >> VDIG-supply = <&imx219_vdig_1v8>; >> VDDL-supply = <&imx219_vddl_1v2>; >> diff --git a/arch/arm64/boot/dts/renesas/r8a774c0-ek874-mipi-2.1.dts b/arch/arm64/boot/dts/renesas/r8a774c0-ek874-mipi-2.1.dts >> index f0829e905506..db4b801b17b5 100644 >> --- a/arch/arm64/boot/dts/renesas/r8a774c0-ek874-mipi-2.1.dts >> +++ b/arch/arm64/boot/dts/renesas/r8a774c0-ek874-mipi-2.1.dts >> @@ -59,6 +59,7 @@ &imx219 { >> port { >> imx219_ep: endpoint { >> clock-lanes = <0>; >> + clock-frequency = <24000000>; > Why is this change needed? This is not the imx219 node, but its endpoint > subnode (the imx219 is imported from aistarvision-mipi-adapter-2.1.dtsi). My bad, I must have been really tired. will rectify this >> data-lanes = <1 2>; >> link-frequencies = /bits/ 64 <456000000>; >> /* uncomment remote-endpoint property to tie imx219 to > - > 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
On Sat, Dec 05, 2020 at 07:33:54PM +0100, michael.srba@seznam.cz wrote: > From: Michael Srba <Michael.Srba@seznam.cz> > > This patch documents the clock-frequency property, which allows the driver > to change the clock frequency from it's default value. > > Signed-off-by: Michael Srba <Michael.Srba@seznam.cz> > --- > Documentation/devicetree/bindings/media/i2c/imx219.yaml | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/media/i2c/imx219.yaml b/Documentation/devicetree/bindings/media/i2c/imx219.yaml > index dfc4d29a4f04..666b8a9da5be 100644 > --- a/Documentation/devicetree/bindings/media/i2c/imx219.yaml > +++ b/Documentation/devicetree/bindings/media/i2c/imx219.yaml > @@ -27,6 +27,10 @@ properties: > clocks: > maxItems: 1 > > + clock-frequency: > + description: > + Frequency of the input clock in Hertz. > + > VDIG-supply: > description: > Digital I/O voltage supply, 1.8 volts > @@ -78,6 +82,7 @@ required: > - compatible > - reg > - clocks > + - clock-frequency Although you can make the field required in bindings, your driver implementation must support older DTBs. 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");