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

[PATCH v2 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>

---

changes since v1: default to exactly 24MHz when `clock-frequency` is not present

---
 drivers/media/i2c/imx219.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index f64c0ef7a897..b6500e2ab19e 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -1443,13 +1443,28 @@ 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_warn(dev, "could not get xclk frequency\n");
+
+		/* default to 24MHz */
+		imx219->xclk_freq = 24000000;
+	}
+
+	/* 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


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

[PATCH v2 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>
---

changes since v1: none

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


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

[PATCH v2 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>
---

changes since v1: remove errorneous edit

---
 arch/arm64/boot/dts/renesas/aistarvision-mipi-adapter-2.1.dtsi | 1 +
 1 file changed, 1 insertion(+)

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


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

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

[Sascha Hauer]

Hi Michael,

On Sun, Dec 06, 2020 at 06:27:18PM +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).
> 
> Signed-off-by: Michael Srba <Michael.Srba@seznam.cz>
> 
> ---
> 
> changes since v1: default to exactly 24MHz when `clock-frequency` is not present
> 
> ---
>  drivers/media/i2c/imx219.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index f64c0ef7a897..b6500e2ab19e 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -1443,13 +1443,28 @@ 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_warn(dev, "could not get xclk frequency\n");
> +
> +		/* default to 24MHz */
> +		imx219->xclk_freq = 24000000;
> +	}
> +
> +	/* 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;
> +	}

clk_set_rate() returns successfully when the rate change has succeeded.
It tells you nothing about the actual rate that has been set. The rate
could be very different from what you want to get, depending on what the
hardware is able to archieve. There's clk_round_rate() that tells you
which rate you'll get when you call clk_set_rate() with that value.
You would have to call clk_round_rate() first and see if you are happy
with the result, afterwards set the rate. From that view it doesn't make
much sense to check the device tree if a number between 23760000 and
24240000 is specified there, the clk api will do rounding anyway.

Also there's the assigned-clocks device tree binding, see
Documentation/devicetree/bindings/clock/clock-bindings.txt. This allows
you to set the desired clock rate directly in the device tree. All
that's left to do in the driver is to replace the check for the exact
rate with a check which allows a certain tolerance.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

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

[Michael Srba]

Hi,

sorry for late reply.

I copied this approach from looking at other camera sensor drivers,
and it seemed less "ugly" to me than using assigned-rates (I will be
upstreaming required dts changes for Samsung Galaxy A3 (2015), so the
dts feeling "proper" is important to me).

I however am not qualified to make that decision, so if you believe
that the assigned-rates approach is cleaner and more suitable for mainline,
I will try to adjust my internal filter for what is "more proper" :)

As for rounding, the issue is that it seems to like to round up, instead
of trying to find the closest possible value. I *guess* trying to set
the lower barrier might work out in practice, but it seems kind of ugly.

All in all, what I did seemed like the cleanest option to me, and it was
an approach that other drivers also use. But if you believe there is
a cleaner approach, I will be more than happy to do something else,
though I would appreciate an explanation of why it is cleaner so that
I can make better decisions in the future.

Best regards,
Michael

On 07. 12. 20 6:59, Sascha Hauer wrote:
> Hi Michael,
>
> On Sun, Dec 06, 2020 at 06:27:18PM +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).
>>
>> Signed-off-by: Michael Srba <Michael.Srba@seznam.cz>
>>
>> ---
>>
>> changes since v1: default to exactly 24MHz when `clock-frequency` is not present
>>
>> ---
>>  drivers/media/i2c/imx219.c | 19 +++++++++++++++++--
>>  1 file changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
>> index f64c0ef7a897..b6500e2ab19e 100644
>> --- a/drivers/media/i2c/imx219.c
>> +++ b/drivers/media/i2c/imx219.c
>> @@ -1443,13 +1443,28 @@ 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_warn(dev, "could not get xclk frequency\n");
>> +
>> +		/* default to 24MHz */
>> +		imx219->xclk_freq = 24000000;
>> +	}
>> +
>> +	/* 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;
>> +	}
> clk_set_rate() returns successfully when the rate change has succeeded.
> It tells you nothing about the actual rate that has been set. The rate
> could be very different from what you want to get, depending on what the
> hardware is able to archieve. There's clk_round_rate() that tells you
> which rate you'll get when you call clk_set_rate() with that value.
> You would have to call clk_round_rate() first and see if you are happy
> with the result, afterwards set the rate. From that view it doesn't make
> much sense to check the device tree if a number between 23760000 and
> 24240000 is specified there, the clk api will do rounding anyway.
>
> Also there's the assigned-clocks device tree binding, see
> Documentation/devicetree/bindings/clock/clock-bindings.txt. This allows
> you to set the desired clock rate directly in the device tree. All
> that's left to do in the driver is to replace the check for the exact
> rate with a check which allows a certain tolerance.
>
> Sascha
>


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

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

[Krzysztof Kozlowski]

On Thu, 10 Dec 2020 at 21:56, Michael Srba <Michael.Srba@seznam.cz> wrote:
>
> Hi,
>
> sorry for late reply.
>
> I copied this approach from looking at other camera sensor drivers,
> and it seemed less "ugly" to me than using assigned-rates (I will be
> upstreaming required dts changes for Samsung Galaxy A3 (2015), so the
> dts feeling "proper" is important to me).

The clean solution is the assigned-clock properties. It's handled by
the core. Duplicating the same feature to every driver is not
"cleaner", but the opposite - ugly one.

> I however am not qualified to make that decision, so if you believe
> that the assigned-rates approach is cleaner and more suitable for mainline,
> I will try to adjust my internal filter for what is "more proper" :)

More proper is handling stuff in the core, not implementing the same
thing in every driver, one more time. Therefore, the assigned-clock
properties.

Best regards,
Krzysztof
Best regards,
Krzysztof

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

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

[Dave Stevenson]

Hi Michael

On Sun, 6 Dec 2020 at 17:29, <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>

As the listed maintainer of this driver I'll say that I don't have any
objections to the aim of this patch.
Those who know the clock infrastructure far better than me are
recommending alternative methods of implementing this, so I'll leave
it up to them to give a Reviewed-by. When that's happened I'll add an
ack.

  Dave

> ---
>
> changes since v1: default to exactly 24MHz when `clock-frequency` is not present
>
> ---
>  drivers/media/i2c/imx219.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index f64c0ef7a897..b6500e2ab19e 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -1443,13 +1443,28 @@ 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_warn(dev, "could not get xclk frequency\n");
> +
> +               /* default to 24MHz */
> +               imx219->xclk_freq = 24000000;
> +       }
> +
> +       /* 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
>

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