[PATCH 1/3] media: i2c: imx219: add support for specifying clock-frequencies

[PATCH 1/3] media: i2c: imx219: add support for specifying clock-frequencies

[michael.srba]

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>
---
 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;
+	}
+
+
 	ret = imx219_get_regulators(imx219);
 	if (ret) {
 		dev_err(dev, "failed to get regulators\n");
-- 
2.29.2

[PATCH 2/3] media: dt-bindings: media: i2c: imx219: document clock-frequency property

[michael.srba]

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
   - VANA-supply
   - VDIG-supply
   - VDDL-supply
@@ -95,6 +100,7 @@ examples:
             compatible = "sony,imx219";
             reg = <0x10>;
             clocks = <&imx219_clk>;
+            clock-frequency = <24000000>;
             VANA-supply = <&imx219_vana>;   /* 2.8v */
             VDIG-supply = <&imx219_vdig>;   /* 1.8v */
             VDDL-supply = <&imx219_vddl>;   /* 1.2v */
 
-- 
2.29.2

Re: [PATCH 2/3] media: dt-bindings: media: i2c: imx219: document clock-frequency property

[Krzysztof Kozlowski]

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

[PATCH 3/3] arm64: dts: update device trees to specify clock-frequency in imx219 node

[michael.srba]

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>
---
 arch/arm64/boot/dts/renesas/aistarvision-mipi-adapter-2.1.dtsi | 1 +
 arch/arm64/boot/dts/renesas/r8a774c0-ek874-mipi-2.1.dts        | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/aistarvision-mipi-adapter-2.1.dtsi b/arch/arm64/boot/dts/renesas/aistarvision-mipi-adapter-2.1.dtsi
index dac6ff49020f..986c6c1f7312 100644
--- 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>;
 			data-lanes = <1 2>;
 			link-frequencies = /bits/ 64 <456000000>;
 			/* uncomment remote-endpoint property to tie imx219 to
-- 
2.29.2

Re: [PATCH 3/3] arm64: dts: update device trees to specify clock-frequency in imx219 node

[Geert Uytterhoeven]

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

>                         data-lanes = <1 2>;
>                         link-frequencies = /bits/ 64 <456000000>;
>                         /* uncomment remote-endpoint property to tie imx219 to
-
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

Re: [PATCH 3/3] arm64: dts: update device trees to specify clock-frequency in imx219 node

[Michael Srba]

> 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

Re: [PATCH 1/3] media: i2c: imx219: add support for specifying clock-frequencies

[Geert Uytterhoeven]

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

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

Re: [PATCH 1/3] media: i2c: imx219: add support for specifying clock-frequencies

[Michael Srba]

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

Re: [PATCH 1/3] media: i2c: imx219: add support for specifying clock-frequencies

[Krzysztof Kozlowski]

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