media: Add a helper for obtaining the clock producer

Introduce a helper for v4l2 sensor drivers on both DT- and ACPI-based platforms to retrieve a reference to the clock producer from firmware.

This helper behaves the same as devm_clk_get() except where there is no clock producer like in ACPI-based platforms.

For ACPI-based platforms the function will read the "clock-frequency" ACPI _DSD property and register a fixed frequency clock with the frequency indicated in the property.

This function also handles the special ACPI-based system case where:
The clock-frequency _DSD property is present.
A reference to the clock producer is present, where the clock is provided by a camera sensor PMIC driver (e.g. int3472/tps68470.c)

In this case try to set the clock-frequency value to the provided clock.

Reviewed-by Laurent Pinchart <[email protected]>

Reviewed-by Lad Prabhakar <[email protected]>

Signed-off-by Mehdi Djait <[email protected]>

---
 drivers/media/v4l2-core/v4l2-common.c | 52 +++++++++++++++++++++++++++
 include/media/v4l2-common.h           | 27 ++++++++++++++
 2 files changed, 79 insertions(+)

drivers/media/v4l2-core/v4l2-common.c +52 -0

index bd160a8c9efe..ac98895b0394 100644
@@ -34,6 +34,9 @@
  * Added Gerd Knorrs v4l1 enhancements (Justin Schoeman)
  */
 
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/kernel.h>

@@ -673,3 +676,52 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_link_freq_to_bitmap);
+
+struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id)
+{
+	const char *clk_id __free(kfree) = NULL;
+	struct clk_hw *clk_hw;
+	struct clk *clk;
+	bool of_node;
+	u32 rate;
+	int ret;
+
+	clk = devm_clk_get_optional(dev, id);
+	ret = device_property_read_u32(dev, "clock-frequency", &rate);
+	of_node = is_of_node(dev_fwnode(dev));
+
+	if (clk) {
+		if (!ret && !of_node) {
+			ret = clk_set_rate(clk, rate);
+			if (ret) {
+				dev_err(dev, "Failed to set clock rate: %u\n",
+					rate);
+				return ERR_PTR(ret);
+			}
+		}
+		return clk;
+	}
+
+	if (PTR_ERR(clk) == -EPROBE_DEFER)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	if (!IS_ENABLED(CONFIG_COMMON_CLK) || of_node)
+		return ERR_PTR(-ENOENT);
+
+	if (ret)
+		return ERR_PTR(ret == -EINVAL ? -EPROBE_DEFER : ret);
+
+	if (!id) {
+		clk_id = kasprintf(GFP_KERNEL, "clk-%s", dev_name(dev));
+		if (!clk_id)
+			return ERR_PTR(-ENOMEM);
+		id = clk_id;
+	}
+
+	clk_hw = devm_clk_hw_register_fixed_rate(dev, id, NULL, 0, rate);
+	if (IS_ERR(clk_hw))
+		return ERR_CAST(clk_hw);
+
+	return clk_hw->clk;
+}
+EXPORT_SYMBOL_GPL(devm_v4l2_sensor_clk_get);

include/media/v4l2-common.h +27 -0

index 0a43f56578bc..1c79ca4d5c73 100644
@@ -100,6 +100,7 @@ int v4l2_ctrl_query_fill(struct v4l2_queryctrl *qctrl,
 struct v4l2_device;
 struct v4l2_subdev;
 struct v4l2_subdev_ops;
+struct clk;
 
 /* I2C Helper functions */
 #include <linux/i2c.h>

@@ -620,6 +621,32 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
 			     unsigned int num_of_driver_link_freqs,
 			     unsigned long *bitmap);
 
+/**
+ * devm_v4l2_sensor_clk_get - lookup and obtain a reference to a clock producer
+ *	for a camera sensor.
+ *
+ * @dev: device for v4l2 sensor clock "consumer"
+ * @id: clock consumer ID
+ *
+ * This function behaves the same way as devm_clk_get() except where there
+ * is no clock producer like in ACPI-based platforms.
+ *
+ * For ACPI-based platforms, the function will read the "clock-frequency"
+ * ACPI _DSD property and register a fixed-clock with the frequency indicated
+ * in the property.
+ *
+ * This function also handles the special ACPI-based system case where:
+ *
+ * * The clock-frequency _DSD property is present.
+ * * A reference to the clock producer is present, where the clock is provided
+ *   by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
+ *
+ * In this case try to set the clock-frequency value to the provided clock.
+ *
+ * Returns a pointer to a struct clk on success or an error pointer on failure.
+ */
+struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id);
+
 static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf)
 {
 	/*

Hi Mehdi,

Quoted 6 lines · Mehdi Djait

Introduce a helper for v4l2 sensor drivers on both DT- and ACPI-based platforms to retrieve a reference to the clock producer from firmware.

This helper behaves the same as devm_clk_get() except where there is no clock producer like in ACPI-based platforms.

Quoted 8 lines

For ACPI-based platforms the function will read the "clock-frequency" ACPI _DSD property and register a fixed frequency clock with the frequency indicated in the property.

This function also handles the special ACPI-based system case where:
The clock-frequency _DSD property is present.
A reference to the clock producer is present, where the clock is provided by a camera sensor PMIC driver (e.g. int3472/tps68470.c)

Missing leading dot. You could also rewrap this paragraph, using longer lines up to 75 characters.

If there's not going to be further versions of the patch, I'll just rewrap this while applying.

Hi Sakari,

Quoted 21 lines · Sakari Ailus

Hi Mehdi,

Quoted 6 lines · Mehdi Djait

Introduce a helper for v4l2 sensor drivers on both DT- and ACPI-based platforms to retrieve a reference to the clock producer from firmware.

This helper behaves the same as devm_clk_get() except where there is no clock producer like in ACPI-based platforms.

Quoted 8 lines

For ACPI-based platforms the function will read the "clock-frequency" ACPI _DSD property and register a fixed frequency clock with the frequency indicated in the property.

This function also handles the special ACPI-based system case where:
The clock-frequency _DSD property is present.
A reference to the clock producer is present, where the clock is provided by a camera sensor PMIC driver (e.g. int3472/tps68470.c)

Missing leading dot. You could also rewrap this paragraph, using longer lines up to 75 characters.

Quoted 2 lines

If there's not going to be further versions of the patch, I'll just rewrap this while applying.

Two things before applying:

- A missing Signed-off-by: Laurent Pinchart <[email protected]> for the Documentation patch -> [PATCH 02/48]

- We still need to agree on how to proceed for the sensor drivers I mentioned in the cover Letter:

1) drivers/media/i2c/s5k5baf.c
Always returns -EPROBE_DEFER if getting the clock fails ?!

2) drivers/media/i2c/mt9t112.c
This drivers seems to be implementing the behaviour of devm_clk_get_optional() while using devm_clk_get(): remove it from the list of changed drivers ?

3) drivers/media/i2c/ov8856.c
Getting the clock, setting the rate, getting the optional gpio and the regulator_bulk is only when the fwnode is NOT acpi.

Hi Mehdi,

Quoted 24 lines · Mehdi Djait

Hi Sakari,

Quoted 21 lines · Sakari Ailus

Hi Mehdi,

Quoted 6 lines · Mehdi Djait

Introduce a helper for v4l2 sensor drivers on both DT- and ACPI-based platforms to retrieve a reference to the clock producer from firmware.

This helper behaves the same as devm_clk_get() except where there is no clock producer like in ACPI-based platforms.

Quoted 8 lines

For ACPI-based platforms the function will read the "clock-frequency" ACPI _DSD property and register a fixed frequency clock with the frequency indicated in the property.

This function also handles the special ACPI-based system case where:
The clock-frequency _DSD property is present.
A reference to the clock producer is present, where the clock is provided by a camera sensor PMIC driver (e.g. int3472/tps68470.c)

Missing leading dot. You could also rewrap this paragraph, using longer lines up to 75 characters.

Quoted 7 lines

Quoted 2 lines

If there's not going to be further versions of the patch, I'll just rewrap this while applying.

Two things before applying:

- A missing Signed-off-by: Laurent Pinchart <[email protected]> for the Documentation patch -> [PATCH 02/48]

Is Co-developed-by: also relevant?

Quoted 6 lines

- We still need to agree on how to proceed for the sensor drivers I mentioned in the cover Letter:

1) drivers/media/i2c/s5k5baf.c
Always returns -EPROBE_DEFER if getting the clock fails ?!

This can be fixed later on IMO.

Quoted 1 line

Quoted 4 lines

2) drivers/media/i2c/mt9t112.c
This drivers seems to be implementing the behaviour of devm_clk_get_optional() while using devm_clk_get(): remove it from the list of changed drivers ?

There are no DT bindings. I wonder if this is still relevant. Maybe Jacopo has an idea?

Quoted 5 lines

3) drivers/media/i2c/ov8856.c
Getting the clock, setting the rate, getting the optional gpio and the regulator_bulk is only when the fwnode is NOT acpi.

Isn't this a similar case than the other sensor (which I can't remember anymore), where effectively the rate set couldn't be different than the clock already had?

I think this also could be handled after this set.

--
Kind regards,

Sakari Ailus

Quoted 21 lines · Sakari Ailus

Quoted 20 lines · Mehdi Djait

Quoted 19 lines · Sakari Ailus

Quoted 6 lines · Mehdi Djait

Introduce a helper for v4l2 sensor drivers on both DT- and ACPI-based platforms to retrieve a reference to the clock producer from firmware.

This helper behaves the same as devm_clk_get() except where there is no clock producer like in ACPI-based platforms.

Quoted 8 lines

For ACPI-based platforms the function will read the "clock-frequency" ACPI _DSD property and register a fixed frequency clock with the frequency indicated in the property.

This function also handles the special ACPI-based system case where:
The clock-frequency _DSD property is present.
A reference to the clock producer is present, where the clock is provided by a camera sensor PMIC driver (e.g. int3472/tps68470.c)

Missing leading dot. You could also rewrap this paragraph, using longer lines up to 75 characters.

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

If there's not going to be further versions of the patch, I'll just rewrap this while applying.

Two things before applying:

- A missing Signed-off-by: Laurent Pinchart <[email protected]> for the Documentation patch -> [PATCH 02/48]

Quoted 9 lines

Is Co-developed-by: also relevant?

Quoted 5 lines

- We still need to agree on how to proceed for the sensor drivers I mentioned in the cover Letter:

1) drivers/media/i2c/s5k5baf.c
Always returns -EPROBE_DEFER if getting the clock fails ?!

This can be fixed later on IMO.

Quoted 8 lines

Quoted 5 lines

2) drivers/media/i2c/mt9t112.c
This drivers seems to be implementing the behaviour of devm_clk_get_optional() while using devm_clk_get(): remove it from the list of changed drivers ?

There are no DT bindings. I wonder if this is still relevant. Maybe Jacopo has an idea?

Not really, the driver has been ported to be a regular i2c driver from its soc-camera-based legacy implementation but has seen basically no development since then, if not for sub-system wide changes or odd fixes.

Looking at the implementatio, the driver indeed seems to implement what _optional() does already, so it might be more opportune to convert it to _optional() first maybe ? However in my understanding your patch doesn't change the current behaviour, doesn't it ? In this case I think you can go ahead.

Quoted 9 lines

Quoted 3 lines

3) drivers/media/i2c/ov8856.c
Getting the clock, setting the rate, getting the optional gpio and the regulator_bulk is only when the fwnode is NOT acpi.

Isn't this a similar case than the other sensor (which I can't remember anymore), where effectively the rate set couldn't be different than the clock already had?

I think this also could be handled after this set.

Quoted 25 lines · Jacopo Mondi

Quoted 24 lines · Sakari Ailus

Quoted 23 lines · Mehdi Djait

Quoted 21 lines · Sakari Ailus

Quoted 6 lines · Mehdi Djait

Introduce a helper for v4l2 sensor drivers on both DT- and ACPI-based platforms to retrieve a reference to the clock producer from firmware.

This helper behaves the same as devm_clk_get() except where there is no clock producer like in ACPI-based platforms.

Quoted 8 lines

For ACPI-based platforms the function will read the "clock-frequency" ACPI _DSD property and register a fixed frequency clock with the frequency indicated in the property.

This function also handles the special ACPI-based system case where:
The clock-frequency _DSD property is present.
A reference to the clock producer is present, where the clock is provided by a camera sensor PMIC driver (e.g. int3472/tps68470.c)

Missing leading dot. You could also rewrap this paragraph, using longer lines up to 75 characters.

If there's not going to be further versions of the patch, I'll just rewrap this while applying.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Two things before applying:

- A missing Signed-off-by: Laurent Pinchart <[email protected]> for the Documentation patch -> [PATCH 02/48]

Fixed.

Quoted 3 lines

Quoted 3 lines

Is Co-developed-by: also relevant?

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

- We still need to agree on how to proceed for the sensor drivers I mentioned in the cover Letter:

1) drivers/media/i2c/s5k5baf.c
Always returns -EPROBE_DEFER if getting the clock fails ?!

This can be fixed later on IMO.

Agreed.

Quoted 13 lines

Quoted 8 lines

Quoted 5 lines

2) drivers/media/i2c/mt9t112.c
This drivers seems to be implementing the behaviour of devm_clk_get_optional() while using devm_clk_get(): remove it from the list of changed drivers ?

There are no DT bindings. I wonder if this is still relevant. Maybe Jacopo has an idea?

Not really, the driver has been ported to be a regular i2c driver from its soc-camera-based legacy implementation but has seen basically no development since then, if not for sub-system wide changes or odd fixes.

Quoted 5 lines

Looking at the implementatio, the driver indeed seems to implement what _optional() does already, so it might be more opportune to convert it to _optional() first maybe ? However in my understanding your patch doesn't change the current behaviour, doesn't it ? In this case I think you can go ahead.

Agreed too.

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

3) drivers/media/i2c/ov8856.c
Getting the clock, setting the rate, getting the optional gpio and the regulator_bulk is only when the fwnode is NOT acpi.

Isn't this a similar case than the other sensor (which I can't remember anymore), where effectively the rate set couldn't be different than the clock already had?

Quoted 1 line

Quoted 1 line

I think this also could be handled after this set.

Ideally we should stop setting the rate and simplify the driver, but we have to be careful about regressions. I agree with Sakari, we can address this later.

Introduce a helper for v4l2 sensor drivers on both DT- and ACPI-based platforms to retrieve a reference to the clock producer from firmware.

This helper behaves the same as devm_clk_get() except where there is no clock producer like in ACPI-based platforms.

For ACPI-based platforms the function will read the "clock-frequency" ACPI _DSD property and register a fixed frequency clock with the frequency indicated in the property.

This function also handles the special ACPI-based system case where:
The clock-frequency _DSD property is present.
A reference to the clock producer is present, where the clock is provided by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
In this case try to set the clock-frequency value to the provided clock.

Reviewed-by Laurent Pinchart <[email protected]>

Reviewed-by Lad Prabhakar <[email protected]>

Signed-off-by Mehdi Djait <[email protected]>

---
 drivers/media/v4l2-core/v4l2-common.c | 52 +++++++++++++++++++++++++++
 include/media/v4l2-common.h           | 27 ++++++++++++++
 2 files changed, 79 insertions(+)

drivers/media/v4l2-core/v4l2-common.c +4 -0

index bd160a8c9efe..ac98895b0394 100644
@@ -34,6 +34,9 @@
  * Added Gerd Knorrs v4l1 enhancements (Justin Schoeman)
  */
 
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/kernel.h>

@@ -673,3 +676,52 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_link_freq_to_bitmap);
+

drivers/media/v4l2-core/v4l2-common.c +21 -0

+
+	if (!IS_ENABLED(CONFIG_COMMON_CLK) || of_node)
+		return ERR_PTR(-ENOENT);
+
+	if (ret)
+		return ERR_PTR(ret == -EINVAL ? -EPROBE_DEFER : ret);
+
+	if (!id) {
+		clk_id = kasprintf(GFP_KERNEL, "clk-%s", dev_name(dev));
+		if (!clk_id)
+			return ERR_PTR(-ENOMEM);
+		id = clk_id;
+	}
+
+	clk_hw = devm_clk_hw_register_fixed_rate(dev, id, NULL, 0, rate);
+	if (IS_ERR(clk_hw))
+		return ERR_CAST(clk_hw);
+
+	return clk_hw->clk;
+}
+EXPORT_SYMBOL_GPL(devm_v4l2_sensor_clk_get);

include/media/v4l2-common.h +0 -0

index 0a43f56578bc..1c79ca4d5c73 100644

include/media/v4l2-common.h +26 -0

 /* I2C Helper functions */
 #include <linux/i2c.h>

@@ -620,6 +621,32 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
 			     unsigned int num_of_driver_link_freqs,
 			     unsigned long *bitmap);
 
+/**
+ * devm_v4l2_sensor_clk_get - lookup and obtain a reference to a clock producer
+ *	for a camera sensor.
+ *
+ * @dev: device for v4l2 sensor clock "consumer"
+ * @id: clock consumer ID
+ *
+ * This function behaves the same way as devm_clk_get() except where there
+ * is no clock producer like in ACPI-based platforms.
+ *
+ * For ACPI-based platforms, the function will read the "clock-frequency"
+ * ACPI _DSD property and register a fixed-clock with the frequency indicated
+ * in the property.
+ *
+ * This function also handles the special ACPI-based system case where:
+ *
+ * * The clock-frequency _DSD property is present.
+ * * A reference to the clock producer is present, where the clock is provided
+ *   by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
+ *
+ * In this case try to set the clock-frequency value to the provided clock.
+ *
+ * Returns a pointer to a struct clk on success or an error pointer on failure.
+ */
+struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id);
+
 static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf)
 {
 	/*

Hi Mehdi,

One more comment.

Quoted 43 lines · Mehdi Djait

Introduce a helper for v4l2 sensor drivers on both DT- and ACPI-based platforms to retrieve a reference to the clock producer from firmware.

This helper behaves the same as devm_clk_get() except where there is no clock producer like in ACPI-based platforms.

For ACPI-based platforms the function will read the "clock-frequency" ACPI _DSD property and register a fixed frequency clock with the frequency indicated in the property.

This function also handles the special ACPI-based system case where:
The clock-frequency _DSD property is present.
A reference to the clock producer is present, where the clock is provided by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
In this case try to set the clock-frequency value to the provided clock.

Reviewed-by Laurent Pinchart <[email protected]>

Reviewed-by Lad Prabhakar <[email protected]>

Signed-off-by Mehdi Djait <[email protected]>

---
 drivers/media/v4l2-core/v4l2-common.c | 52 +++++++++++++++++++++++++++
 include/media/v4l2-common.h           | 27 ++++++++++++++
 2 files changed, 79 insertions(+)

drivers/media/v4l2-core/v4l2-common.c +4 -0

index bd160a8c9efe..ac98895b0394 100644
@@ -34,6 +34,9 @@
  * Added Gerd Knorrs v4l1 enhancements (Justin Schoeman)
  */
 
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/kernel.h>

@@ -673,3 +676,52 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_link_freq_to_bitmap);
+

drivers/media/v4l2-core/v4l2-common.c int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs, · line 689

Quoted 10 lines

drivers/media/v4l2-core/v4l2-common.c +10 -0

+struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id)
+{
+	const char *clk_id __free(kfree) = NULL;
+	struct clk_hw *clk_hw;
+	struct clk *clk;
+	bool of_node;
+	u32 rate;
+	int ret;
+
+	clk = devm_clk_get_optional(dev, id);

Shouldn't your return here if there's an error ?

	if (IS_ERR(clk))
		return clk;

because ...

drivers/media/v4l2-core/v4l2-common.c int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs, · line 695

Quoted 6 lines

drivers/media/v4l2-core/v4l2-common.c +6 -0

+	ret = device_property_read_u32(dev, "clock-frequency", &rate);
+	of_node = is_of_node(dev_fwnode(dev));
+
+	if (clk) {
+		if (!ret && !of_node) {
+			ret = clk_set_rate(clk, rate);

... here clk_set_rate() will dereference clk and crash if it's an error pointer ...

Quoted 5 lines

drivers/media/v4l2-core/v4l2-common.c +5 -0

+			if (ret) {
+				dev_err(dev, "Failed to set clock rate: %u\n",
+					rate);
+				return ERR_PTR(ret);
+			}

drivers/media/v4l2-core/v4l2-common.c int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs, · line 706

Quoted 6 lines

drivers/media/v4l2-core/v4l2-common.c +6 -0

+		}
+		return clk;
+	}
+
+	if (PTR_ERR(clk) == -EPROBE_DEFER)
+		return ERR_PTR(-EPROBE_DEFER);

... and this will never be reached.

Quoted 26 lines

drivers/media/v4l2-core/v4l2-common.c +21 -0

+
+	if (!IS_ENABLED(CONFIG_COMMON_CLK) || of_node)
+		return ERR_PTR(-ENOENT);
+
+	if (ret)
+		return ERR_PTR(ret == -EINVAL ? -EPROBE_DEFER : ret);
+
+	if (!id) {
+		clk_id = kasprintf(GFP_KERNEL, "clk-%s", dev_name(dev));
+		if (!clk_id)
+			return ERR_PTR(-ENOMEM);
+		id = clk_id;
+	}
+
+	clk_hw = devm_clk_hw_register_fixed_rate(dev, id, NULL, 0, rate);
+	if (IS_ERR(clk_hw))
+		return ERR_CAST(clk_hw);
+
+	return clk_hw->clk;
+}
+EXPORT_SYMBOL_GPL(devm_v4l2_sensor_clk_get);

include/media/v4l2-common.h +0 -0

index 0a43f56578bc..1c79ca4d5c73 100644

include/media/v4l2-common.h int v4l2_ctrl_query_fill(struct v4l2_queryctrl *qctrl, · line 103

Quoted 4 lines

include/media/v4l2-common.h +1 -0

@@ -100,6 +100,7 @@ int v4l2_ctrl_query_fill(struct v4l2_queryctrl *qctrl,
 struct v4l2_device;
 struct v4l2_subdev;
 struct v4l2_subdev_ops;
+struct clk;

Please keep forward declarations sorted.

You don't need to resubmit the whole series for this, but it would be useful if you could retest it, and then send a new version of just this patch.

Introduce a helper for v4l2 sensor drivers on both DT- and ACPI-based platforms to retrieve a reference to the clock producer from firmware.

This helper behaves the same as devm_clk_get() except where there is no clock producer like in ACPI-based platforms.

For ACPI-based platforms the function will read the "clock-frequency" ACPI _DSD property and register a fixed frequency clock with the frequency indicated in the property.

This function also handles the special ACPI-based system case where:
. The clock-frequency _DSD property is present.
. A reference to the clock producer is present, where the clock is provided
by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
In this case try to set the clock-frequency value to the provided clock.

Introduce a helper for v4l2 sensor drivers on both DT- and ACPI-based platforms to retrieve a reference to the clock producer from firmware.

This helper behaves the same as devm_clk_get() except where there is no clock producer like in ACPI-based platforms.

For ACPI-based platforms the function will read the "clock-frequency" ACPI _DSD property and register a fixed frequency clock with the frequency indicated in the property.

This function also handles the special ACPI-based system case where:
The clock-frequency _DSD property is present.
A reference to the clock producer is present, where the clock is provided by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
In this case try to set the clock-frequency value to the provided clock.

Reviewed-by Laurent Pinchart <[email protected]>

Reviewed-by Lad Prabhakar <[email protected]>

Signed-off-by Mehdi Djait <[email protected]>

---
 drivers/media/v4l2-core/v4l2-common.c | 52 +++++++++++++++++++++++++++
 include/media/v4l2-common.h           | 27 ++++++++++++++
 2 files changed, 79 insertions(+)

drivers/media/v4l2-core/v4l2-common.c +0 -0

include/media/v4l2-common.h +12 -0

index 0a43f56578bc..1c79ca4d5c73 100644
@@ -100,6 +100,7 @@ int v4l2_ctrl_query_fill(struct v4l2_queryctrl *qctrl,
 struct v4l2_device;
 struct v4l2_subdev;
 struct v4l2_subdev_ops;
+struct clk;
 
 /* I2C Helper functions */
 #include <linux/i2c.h>

@@ -620,6 +621,32 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
 			     unsigned int num_of_driver_link_freqs,
 			     unsigned long *bitmap);
 
+/**
+ * devm_v4l2_sensor_clk_get - lookup and obtain a reference to a clock producer
+ *	for a camera sensor.
+ *
+ * @dev: device for v4l2 sensor clock "consumer"
+ * @id: clock consumer ID
+ *
+ * This function behaves the same way as devm_clk_get() except where there
+ * is no clock producer like in ACPI-based platforms.
+ *
+ * For ACPI-based platforms, the function will read the "clock-frequency"

include/media/v4l2-common.h +5 -0

+ *
+ * Returns a pointer to a struct clk on success or an error pointer on failure.
+ */
+struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id);
+
 static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf)
 {
 	/*

Hi Mehdi, Sakari, Laurent,

Quoted 10 lines · Mehdi Djait

Introduce a helper for v4l2 sensor drivers on both DT- and ACPI-based platforms to retrieve a reference to the clock producer from firmware.

This helper behaves the same as devm_clk_get() except where there is no clock producer like in ACPI-based platforms.

For ACPI-based platforms the function will read the "clock-frequency" ACPI _DSD property and register a fixed frequency clock with the frequency indicated in the property.

Quoted 5 lines

This function also handles the special ACPI-based system case where:
The clock-frequency _DSD property is present.
A reference to the clock producer is present, where the clock is provided by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
In this case try to set the clock-frequency value to the provided clock.

On irc I wondered about the name of the new function since it is sensor specific, and if it can also be used for e.g. video receivers, then it should be renamed to something more generic.

According to Laurent there is no ACPI standard for video receivers, so that's the reason this is sensor specific.

I'd like to see that documented in this patch. Either in the commit log, or, preferred, in the header in the function description.

I made a suggestion below.

Quoted 10 lines

Reviewed-by Laurent Pinchart <[email protected]>

Reviewed-by Lad Prabhakar <[email protected]>

Signed-off-by Mehdi Djait <[email protected]>

---
 drivers/media/v4l2-core/v4l2-common.c | 52 +++++++++++++++++++++++++++
 include/media/v4l2-common.h           | 27 ++++++++++++++
 2 files changed, 79 insertions(+)

drivers/media/v4l2-core/v4l2-common.c +0 -0

<snip>

Quoted 27 lines

include/media/v4l2-common.h +12 -0

index 0a43f56578bc..1c79ca4d5c73 100644
@@ -100,6 +100,7 @@ int v4l2_ctrl_query_fill(struct v4l2_queryctrl *qctrl,
 struct v4l2_device;
 struct v4l2_subdev;
 struct v4l2_subdev_ops;
+struct clk;
 
 /* I2C Helper functions */
 #include <linux/i2c.h>

@@ -620,6 +621,32 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
 			     unsigned int num_of_driver_link_freqs,
 			     unsigned long *bitmap);
 
+/**
+ * devm_v4l2_sensor_clk_get - lookup and obtain a reference to a clock producer
+ *	for a camera sensor.
+ *
+ * @dev: device for v4l2 sensor clock "consumer"
+ * @id: clock consumer ID
+ *
+ * This function behaves the same way as devm_clk_get() except where there
+ * is no clock producer like in ACPI-based platforms.
+ *
+ * For ACPI-based platforms, the function will read the "clock-frequency"

include/media/v4l2-common.h int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs, · line 644

Quoted 10 lines

include/media/v4l2-common.h +10 -0

+ * ACPI _DSD property and register a fixed-clock with the frequency indicated
+ * in the property.
+ *
+ * This function also handles the special ACPI-based system case where:
+ *
+ * * The clock-frequency _DSD property is present.
+ * * A reference to the clock producer is present, where the clock is provided
+ *   by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
+ *
+ * In this case try to set the clock-frequency value to the provided clock.

*
* While ACPI has standardized sensor support, there is no standard support for
* e.g. video receivers. So this function is specific to sensors, hence the
* chosen function name.

Better suggestions are welcome.

If it is just adding a paragraph, then I can just add that manually. If the change is more involved, then a v2.1 for just this patch should be posted.

Other than this, the series looks good.

Quoted 24 lines · Hans Verkuil

Quoted 10 lines · Mehdi Djait

Introduce a helper for v4l2 sensor drivers on both DT- and ACPI-based platforms to retrieve a reference to the clock producer from firmware.

This helper behaves the same as devm_clk_get() except where there is no clock producer like in ACPI-based platforms.

For ACPI-based platforms the function will read the "clock-frequency" ACPI _DSD property and register a fixed frequency clock with the frequency indicated in the property.

Quoted 5 lines

This function also handles the special ACPI-based system case where:
The clock-frequency _DSD property is present.
A reference to the clock producer is present, where the clock is provided by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
In this case try to set the clock-frequency value to the provided clock.

On irc I wondered about the name of the new function since it is sensor specific, and if it can also be used for e.g. video receivers, then it should be renamed to something more generic.

According to Laurent there is no ACPI standard for video receivers, so that's the reason this is sensor specific.

Quoted 2 lines

I'd like to see that documented in this patch. Either in the commit log, or, preferred, in the header in the function description.

Given this patch has been around for quite some time and I've sent a PR on it, I'd prefer to proceed with the current PR. I'm fine with adding more documentation though if you think we should do that.

Quoted 57 lines

I made a suggestion below.

Quoted 9 lines

Reviewed-by Laurent Pinchart <[email protected]>

Reviewed-by Lad Prabhakar <[email protected]>

Signed-off-by Mehdi Djait <[email protected]>

---
 drivers/media/v4l2-core/v4l2-common.c | 52 +++++++++++++++++++++++++++
 include/media/v4l2-common.h           | 27 ++++++++++++++
 2 files changed, 79 insertions(+)

drivers/media/v4l2-core/v4l2-common.c +0 -0

<snip>

Quoted 27 lines

include/media/v4l2-common.h +12 -0

index 0a43f56578bc..1c79ca4d5c73 100644
@@ -100,6 +100,7 @@ int v4l2_ctrl_query_fill(struct v4l2_queryctrl *qctrl,
 struct v4l2_device;
 struct v4l2_subdev;
 struct v4l2_subdev_ops;
+struct clk;
 
 /* I2C Helper functions */
 #include <linux/i2c.h>

@@ -620,6 +621,32 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
 			     unsigned int num_of_driver_link_freqs,
 			     unsigned long *bitmap);
 
+/**
+ * devm_v4l2_sensor_clk_get - lookup and obtain a reference to a clock producer
+ *	for a camera sensor.
+ *
+ * @dev: device for v4l2 sensor clock "consumer"
+ * @id: clock consumer ID
+ *
+ * This function behaves the same way as devm_clk_get() except where there
+ * is no clock producer like in ACPI-based platforms.
+ *
+ * For ACPI-based platforms, the function will read the "clock-frequency"

include/media/v4l2-common.h int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs, · line 644

Quoted 10 lines

include/media/v4l2-common.h +10 -0

+ * ACPI _DSD property and register a fixed-clock with the frequency indicated
+ * in the property.
+ *
+ * This function also handles the special ACPI-based system case where:
+ *
+ * * The clock-frequency _DSD property is present.
+ * * A reference to the clock producer is present, where the clock is provided
+ *   by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
+ *
+ * In this case try to set the clock-frequency value to the provided clock.

*
* While ACPI has standardized sensor support, there is no standard support for
* e.g. video receivers. So this function is specific to sensors, hence the
* chosen function name.

Quoted 1 line

Better suggestions are welcome.

ACPI itself does not standardise camera sensor support. What driver authors

should know indeed is that this function provides a clock that can be at least queried for frequency, independently of how that clock is controlled or how its frequency is configured.

How about:

* This function may be used in camera sensor drivers only.

Quoted 4 lines

If it is just adding a paragraph, then I can just add that manually. If the change is more involved, then a v2.1 for just this patch should be posted.

Other than this, the series looks good.

Quoted 28 lines · Sakari Ailus

Quoted 26 lines · Hans Verkuil

Quoted 10 lines · Mehdi Djait

Introduce a helper for v4l2 sensor drivers on both DT- and ACPI-based platforms to retrieve a reference to the clock producer from firmware.

This helper behaves the same as devm_clk_get() except where there is no clock producer like in ACPI-based platforms.

For ACPI-based platforms the function will read the "clock-frequency" ACPI _DSD property and register a fixed frequency clock with the frequency indicated in the property.

Quoted 5 lines

This function also handles the special ACPI-based system case where:
The clock-frequency _DSD property is present.
A reference to the clock producer is present, where the clock is provided by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
In this case try to set the clock-frequency value to the provided clock.

On irc I wondered about the name of the new function since it is sensor specific, and if it can also be used for e.g. video receivers, then it should be renamed to something more generic.

According to Laurent there is no ACPI standard for video receivers, so that's the reason this is sensor specific.

I'd like to see that documented in this patch. Either in the commit log, or, preferred, in the header in the function description.

Quoted 3 lines

Given this patch has been around for quite some time and I've sent a PR on it, I'd prefer to proceed with the current PR. I'm fine with adding more documentation though if you think we should do that.

How about a separate patch that Hans can merge just on top of your PR ? Could you send one ?

Quoted 58 lines

Quoted 58 lines

I made a suggestion below.

Quoted 9 lines

Reviewed-by Laurent Pinchart <[email protected]>

Reviewed-by Lad Prabhakar <[email protected]>

Signed-off-by Mehdi Djait <[email protected]>

---
 drivers/media/v4l2-core/v4l2-common.c | 52 +++++++++++++++++++++++++++
 include/media/v4l2-common.h           | 27 ++++++++++++++
 2 files changed, 79 insertions(+)

drivers/media/v4l2-core/v4l2-common.c +0 -0

<snip>

Quoted 27 lines

include/media/v4l2-common.h +12 -0

index 0a43f56578bc..1c79ca4d5c73 100644
@@ -100,6 +100,7 @@ int v4l2_ctrl_query_fill(struct v4l2_queryctrl *qctrl,
 struct v4l2_device;
 struct v4l2_subdev;
 struct v4l2_subdev_ops;
+struct clk;
 
 /* I2C Helper functions */
 #include <linux/i2c.h>

@@ -620,6 +621,32 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
 			     unsigned int num_of_driver_link_freqs,
 			     unsigned long *bitmap);
 
+/**
+ * devm_v4l2_sensor_clk_get - lookup and obtain a reference to a clock producer
+ *	for a camera sensor.
+ *
+ * @dev: device for v4l2 sensor clock "consumer"
+ * @id: clock consumer ID
+ *
+ * This function behaves the same way as devm_clk_get() except where there
+ * is no clock producer like in ACPI-based platforms.
+ *
+ * For ACPI-based platforms, the function will read the "clock-frequency"

include/media/v4l2-common.h int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs, · line 644

Quoted 10 lines

include/media/v4l2-common.h +10 -0

+ * ACPI _DSD property and register a fixed-clock with the frequency indicated
+ * in the property.
+ *
+ * This function also handles the special ACPI-based system case where:
+ *
+ * * The clock-frequency _DSD property is present.
+ * * A reference to the clock producer is present, where the clock is provided
+ *   by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
+ *
+ * In this case try to set the clock-frequency value to the provided clock.

*
* While ACPI has standardized sensor support, there is no standard support for
* e.g. video receivers. So this function is specific to sensors, hence the
* chosen function name.

Quoted 3 lines

Quoted 1 line

Better suggestions are welcome.

ACPI itself does not standardise camera sensor support. What driver authors

But there's a de facto standard that makes this helper function suitable, isn't there ?

Quoted 7 lines

should know indeed is that this function provides a clock that can be at least queried for frequency, independently of how that clock is controlled or how its frequency is configured.

How about:

* This function may be used in camera sensor drivers only.

Quoted 5 lines

Quoted 5 lines

If it is just adding a paragraph, then I can just add that manually. If the change is more involved, then a v2.1 for just this patch should be posted.

Other than this, the series looks good.

Quoted 29 lines · Laurent Pinchart

Quoted 28 lines · Sakari Ailus

Quoted 26 lines · Hans Verkuil

Quoted 10 lines · Mehdi Djait

Introduce a helper for v4l2 sensor drivers on both DT- and ACPI-based platforms to retrieve a reference to the clock producer from firmware.

This helper behaves the same as devm_clk_get() except where there is no clock producer like in ACPI-based platforms.

For ACPI-based platforms the function will read the "clock-frequency" ACPI _DSD property and register a fixed frequency clock with the frequency indicated in the property.

Quoted 5 lines

This function also handles the special ACPI-based system case where:
The clock-frequency _DSD property is present.
A reference to the clock producer is present, where the clock is provided by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
In this case try to set the clock-frequency value to the provided clock.

On irc I wondered about the name of the new function since it is sensor specific, and if it can also be used for e.g. video receivers, then it should be renamed to something more generic.

According to Laurent there is no ACPI standard for video receivers, so that's the reason this is sensor specific.

I'd like to see that documented in this patch. Either in the commit log, or, preferred, in the header in the function description.

Quoted 6 lines

Quoted 3 lines

Given this patch has been around for quite some time and I've sent a PR on it, I'd prefer to proceed with the current PR. I'm fine with adding more documentation though if you think we should do that.

How about a separate patch that Hans can merge just on top of your PR ? Could you send one ?

I'd like to have the rationale of why this is a sensor-only function documented. If I wondered about that, then someone else will likely have the same question.

Quoted 69 lines

Quoted 58 lines

Quoted 58 lines

I made a suggestion below.

Quoted 9 lines

Reviewed-by Laurent Pinchart <[email protected]>

Reviewed-by Lad Prabhakar <[email protected]>

Signed-off-by Mehdi Djait <[email protected]>

---
 drivers/media/v4l2-core/v4l2-common.c | 52 +++++++++++++++++++++++++++
 include/media/v4l2-common.h           | 27 ++++++++++++++
 2 files changed, 79 insertions(+)

drivers/media/v4l2-core/v4l2-common.c +0 -0

<snip>

Quoted 27 lines

include/media/v4l2-common.h +12 -0

index 0a43f56578bc..1c79ca4d5c73 100644
@@ -100,6 +100,7 @@ int v4l2_ctrl_query_fill(struct v4l2_queryctrl *qctrl,
 struct v4l2_device;
 struct v4l2_subdev;
 struct v4l2_subdev_ops;
+struct clk;
 
 /* I2C Helper functions */
 #include <linux/i2c.h>

@@ -620,6 +621,32 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
 			     unsigned int num_of_driver_link_freqs,
 			     unsigned long *bitmap);
 
+/**
+ * devm_v4l2_sensor_clk_get - lookup and obtain a reference to a clock producer
+ *	for a camera sensor.
+ *
+ * @dev: device for v4l2 sensor clock "consumer"
+ * @id: clock consumer ID
+ *
+ * This function behaves the same way as devm_clk_get() except where there
+ * is no clock producer like in ACPI-based platforms.
+ *
+ * For ACPI-based platforms, the function will read the "clock-frequency"

include/media/v4l2-common.h int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs, · line 644

Quoted 10 lines

include/media/v4l2-common.h +10 -0

+ * ACPI _DSD property and register a fixed-clock with the frequency indicated
+ * in the property.
+ *
+ * This function also handles the special ACPI-based system case where:
+ *
+ * * The clock-frequency _DSD property is present.
+ * * A reference to the clock producer is present, where the clock is provided
+ *   by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
+ *
+ * In this case try to set the clock-frequency value to the provided clock.

*
* While ACPI has standardized sensor support, there is no standard support for
* e.g. video receivers. So this function is specific to sensors, hence the
* chosen function name.

Quoted 3 lines

Quoted 1 line

Better suggestions are welcome.

ACPI itself does not standardise camera sensor support. What driver authors

But there's a de facto standard that makes this helper function suitable, isn't there ?

Quoted 4 lines

should know indeed is that this function provides a clock that can be at least queried for frequency, independently of how that clock is controlled or how its frequency is configured.

Quoted 3 lines

Quoted 3 lines

How about:

* This function may be used in camera sensor drivers only.

That just restates what is also in the function name. It's the 'why' that I'd like to know. Even if you simply don't know if this will work for non-sensor driver, just stating that will help.

Hi Hans,

Quoted 110 lines · Hans Verkuil

Quoted 29 lines · Laurent Pinchart

Quoted 28 lines · Sakari Ailus

Quoted 26 lines · Hans Verkuil

Quoted 10 lines · Mehdi Djait

Introduce a helper for v4l2 sensor drivers on both DT- and ACPI-based platforms to retrieve a reference to the clock producer from firmware.

This helper behaves the same as devm_clk_get() except where there is no clock producer like in ACPI-based platforms.

For ACPI-based platforms the function will read the "clock-frequency" ACPI _DSD property and register a fixed frequency clock with the frequency indicated in the property.

Quoted 5 lines

This function also handles the special ACPI-based system case where:
The clock-frequency _DSD property is present.
A reference to the clock producer is present, where the clock is provided by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
In this case try to set the clock-frequency value to the provided clock.

On irc I wondered about the name of the new function since it is sensor specific, and if it can also be used for e.g. video receivers, then it should be renamed to something more generic.

According to Laurent there is no ACPI standard for video receivers, so that's the reason this is sensor specific.

I'd like to see that documented in this patch. Either in the commit log, or, preferred, in the header in the function description.

Quoted 6 lines

Quoted 3 lines

Given this patch has been around for quite some time and I've sent a PR on it, I'd prefer to proceed with the current PR. I'm fine with adding more documentation though if you think we should do that.

How about a separate patch that Hans can merge just on top of your PR ? Could you send one ?

I'd like to have the rationale of why this is a sensor-only function documented. If I wondered about that, then someone else will likely have the same question.

Quoted 69 lines

Quoted 58 lines

Quoted 58 lines

I made a suggestion below.

Quoted 9 lines

Reviewed-by Laurent Pinchart <[email protected]>

Reviewed-by Lad Prabhakar <[email protected]>

Signed-off-by Mehdi Djait <[email protected]>

---
 drivers/media/v4l2-core/v4l2-common.c | 52 +++++++++++++++++++++++++++
 include/media/v4l2-common.h           | 27 ++++++++++++++
 2 files changed, 79 insertions(+)

drivers/media/v4l2-core/v4l2-common.c +0 -0

<snip>

Quoted 27 lines

include/media/v4l2-common.h +12 -0

index 0a43f56578bc..1c79ca4d5c73 100644
@@ -100,6 +100,7 @@ int v4l2_ctrl_query_fill(struct v4l2_queryctrl *qctrl,
 struct v4l2_device;
 struct v4l2_subdev;
 struct v4l2_subdev_ops;
+struct clk;
 
 /* I2C Helper functions */
 #include <linux/i2c.h>

@@ -620,6 +621,32 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
 			     unsigned int num_of_driver_link_freqs,
 			     unsigned long *bitmap);
 
+/**
+ * devm_v4l2_sensor_clk_get - lookup and obtain a reference to a clock producer
+ *	for a camera sensor.
+ *
+ * @dev: device for v4l2 sensor clock "consumer"
+ * @id: clock consumer ID
+ *
+ * This function behaves the same way as devm_clk_get() except where there
+ * is no clock producer like in ACPI-based platforms.
+ *
+ * For ACPI-based platforms, the function will read the "clock-frequency"

include/media/v4l2-common.h int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs, · line 644

Quoted 10 lines

include/media/v4l2-common.h +10 -0

+ * ACPI _DSD property and register a fixed-clock with the frequency indicated
+ * in the property.
+ *
+ * This function also handles the special ACPI-based system case where:
+ *
+ * * The clock-frequency _DSD property is present.
+ * * A reference to the clock producer is present, where the clock is provided
+ *   by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
+ *
+ * In this case try to set the clock-frequency value to the provided clock.

*
* While ACPI has standardized sensor support, there is no standard support for
* e.g. video receivers. So this function is specific to sensors, hence the
* chosen function name.

Quoted 3 lines

Quoted 1 line

Better suggestions are welcome.

ACPI itself does not standardise camera sensor support. What driver authors

But there's a de facto standard that makes this helper function suitable, isn't there ?

Quoted 4 lines

should know indeed is that this function provides a clock that can be at least queried for frequency, independently of how that clock is controlled or how its frequency is configured.

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

How about:

* This function may be used in camera sensor drivers only.

That just restates what is also in the function name. It's the 'why' that I'd like to know. Even if you simply don't know if this will work for non-sensor driver, just stating that will help.

As I replied on #linux-media:

devm_v4l2_sensor_clk_get() in practice provides a clock to camera sensors which is what they need; other types of devices generally don't care much about clocks. Why camera sensors are special is that they have a PLL and a CSI-2 RX the configuration of which depend on the external clock.

Is something like this what you're looking for, or something more elaborate? There are different ways the clock can be obtained but I'm not sure we'll need to go to that in the function documentation.

Quoted 128 lines · Sakari Ailus

Hi Hans,

Quoted 110 lines · Hans Verkuil

Quoted 29 lines · Laurent Pinchart

Quoted 28 lines · Sakari Ailus

Quoted 26 lines · Hans Verkuil

Quoted 10 lines · Mehdi Djait

Introduce a helper for v4l2 sensor drivers on both DT- and ACPI-based platforms to retrieve a reference to the clock producer from firmware.

This helper behaves the same as devm_clk_get() except where there is no clock producer like in ACPI-based platforms.

For ACPI-based platforms the function will read the "clock-frequency" ACPI _DSD property and register a fixed frequency clock with the frequency indicated in the property.

Quoted 5 lines

This function also handles the special ACPI-based system case where:
The clock-frequency _DSD property is present.
A reference to the clock producer is present, where the clock is provided by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
In this case try to set the clock-frequency value to the provided clock.

On irc I wondered about the name of the new function since it is sensor specific, and if it can also be used for e.g. video receivers, then it should be renamed to something more generic.

According to Laurent there is no ACPI standard for video receivers, so that's the reason this is sensor specific.

I'd like to see that documented in this patch. Either in the commit log, or, preferred, in the header in the function description.

Quoted 6 lines

Quoted 3 lines

Given this patch has been around for quite some time and I've sent a PR on it, I'd prefer to proceed with the current PR. I'm fine with adding more documentation though if you think we should do that.

How about a separate patch that Hans can merge just on top of your PR ? Could you send one ?

I'd like to have the rationale of why this is a sensor-only function documented. If I wondered about that, then someone else will likely have the same question.

Quoted 69 lines

Quoted 58 lines

Quoted 58 lines

I made a suggestion below.

Quoted 9 lines

Reviewed-by Laurent Pinchart <[email protected]>

Reviewed-by Lad Prabhakar <[email protected]>

Signed-off-by Mehdi Djait <[email protected]>

---
 drivers/media/v4l2-core/v4l2-common.c | 52 +++++++++++++++++++++++++++
 include/media/v4l2-common.h           | 27 ++++++++++++++
 2 files changed, 79 insertions(+)

drivers/media/v4l2-core/v4l2-common.c +0 -0

<snip>

Quoted 27 lines

include/media/v4l2-common.h +12 -0

index 0a43f56578bc..1c79ca4d5c73 100644
@@ -100,6 +100,7 @@ int v4l2_ctrl_query_fill(struct v4l2_queryctrl *qctrl,
 struct v4l2_device;
 struct v4l2_subdev;
 struct v4l2_subdev_ops;
+struct clk;
 
 /* I2C Helper functions */
 #include <linux/i2c.h>

@@ -620,6 +621,32 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
 			     unsigned int num_of_driver_link_freqs,
 			     unsigned long *bitmap);
 
+/**
+ * devm_v4l2_sensor_clk_get - lookup and obtain a reference to a clock producer
+ *	for a camera sensor.
+ *
+ * @dev: device for v4l2 sensor clock "consumer"
+ * @id: clock consumer ID
+ *
+ * This function behaves the same way as devm_clk_get() except where there
+ * is no clock producer like in ACPI-based platforms.
+ *
+ * For ACPI-based platforms, the function will read the "clock-frequency"

include/media/v4l2-common.h int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs, · line 644

Quoted 10 lines

include/media/v4l2-common.h +10 -0

+ * ACPI _DSD property and register a fixed-clock with the frequency indicated
+ * in the property.
+ *
+ * This function also handles the special ACPI-based system case where:
+ *
+ * * The clock-frequency _DSD property is present.
+ * * A reference to the clock producer is present, where the clock is provided
+ *   by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
+ *
+ * In this case try to set the clock-frequency value to the provided clock.

*
* While ACPI has standardized sensor support, there is no standard support for
* e.g. video receivers. So this function is specific to sensors, hence the
* chosen function name.

Quoted 3 lines

Quoted 1 line

Better suggestions are welcome.

ACPI itself does not standardise camera sensor support. What driver authors

But there's a de facto standard that makes this helper function suitable, isn't there ?

Quoted 4 lines

should know indeed is that this function provides a clock that can be at least queried for frequency, independently of how that clock is controlled or how its frequency is configured.

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

How about:

* This function may be used in camera sensor drivers only.

That just restates what is also in the function name. It's the 'why' that I'd like to know. Even if you simply don't know if this will work for non-sensor driver, just stating that will help.

As I replied on #linux-media:

devm_v4l2_sensor_clk_get() in practice provides a clock to camera sensors which is what they need; other types of devices generally don't care much about clocks. Why camera sensors are special is that they have a PLL and a CSI-2 RX the configuration of which depend on the external clock.

Quoted 4 lines

Is something like this what you're looking for, or something more elaborate? There are different ways the clock can be obtained but I'm not sure we'll need to go to that in the function documentation.

Yes, that's sufficient. If you can make a patch adding that, then I'll add it to the PR. No need to make a new PR for this, I can just add it myself.

Remove the extra leading period and provide more elaborate explanation for why devm_v4l2_sensor_clk_get() is only allowed to be used on camera sensor devices.

Signed-off-by Sakari Ailus <[email protected]>

---
 include/media/v4l2-common.h | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

include/media/v4l2-common.h +1 -1

index 9d6c236e8f14..39dd0c78d70f 100644
@@ -623,7 +623,7 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
 
 /**
  * devm_v4l2_sensor_clk_get - lookup and obtain a reference to a clock producer
- *	for a camera sensor.
+ *	for a camera sensor
  *
  * @dev: device for v4l2 sensor clock "consumer"
  * @id: clock consumer ID

@@ -643,6 +643,14 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
  *

Hi Sakari,

Thank you for the patch!

Quoted 29 lines · Sakari Ailus

Remove the extra leading period and provide more elaborate explanation for why devm_v4l2_sensor_clk_get() is only allowed to be used on camera sensor devices.

Signed-off-by Sakari Ailus <[email protected]>

---
 include/media/v4l2-common.h | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

include/media/v4l2-common.h +4 -1

index 9d6c236e8f14..39dd0c78d70f 100644
@@ -623,7 +623,7 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
 
 /**
  * devm_v4l2_sensor_clk_get - lookup and obtain a reference to a clock producer
- *	for a camera sensor.
+ *	for a camera sensor
  *
  * @dev: device for v4l2 sensor clock "consumer"
  * @id: clock consumer ID

@@ -643,6 +643,14 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
  *
  * In this case try to set the clock-frequency value to the provided clock.
  *
+ * As the name indicates, this function may only be used on camera sensor
+ * devices. This is because generally only camera sensors do need a clock to
+ * query the frequency from, due to the requirement to configure the PLL for a

include/media/v4l2-common.h int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs, · line 653

Quoted 5 lines

include/media/v4l2-common.h +5 -0

+ * given CSI-2 interface frequency where the sensor's external clock frequency
+ * is a factor. Additionally, the clock frequency tends to be available on ACPI
+ * firmware based systems for camera sensors specifically (if e.g. DisCo for
+ * Imaging compliant).
+ *

I don't know about the last sentence about ACPI. Does it help explain why this helper is intended only for camera sensors ?

Quoted 28 lines · Sakari Ailus

Quoted 26 lines · Hans Verkuil

Quoted 10 lines · Mehdi Djait

Introduce a helper for v4l2 sensor drivers on both DT- and ACPI-based platforms to retrieve a reference to the clock producer from firmware.

This helper behaves the same as devm_clk_get() except where there is no clock producer like in ACPI-based platforms.

For ACPI-based platforms the function will read the "clock-frequency" ACPI _DSD property and register a fixed frequency clock with the frequency indicated in the property.

Quoted 5 lines

This function also handles the special ACPI-based system case where:
The clock-frequency _DSD property is present.
A reference to the clock producer is present, where the clock is provided by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
In this case try to set the clock-frequency value to the provided clock.

On irc I wondered about the name of the new function since it is sensor specific, and if it can also be used for e.g. video receivers, then it should be renamed to something more generic.

According to Laurent there is no ACPI standard for video receivers, so that's the reason this is sensor specific.

I'd like to see that documented in this patch. Either in the commit log, or, preferred, in the header in the function description.

Quoted 3 lines

Given this patch has been around for quite some time and I've sent a PR on it, I'd prefer to proceed with the current PR. I'm fine with adding more documentation though if you think we should do that.

How about a separate patch that Hans can merge just on top of your PR ? Could you send one ?

Quoted 58 lines

Quoted 58 lines

I made a suggestion below.

Quoted 9 lines

Reviewed-by Laurent Pinchart <[email protected]>

Reviewed-by Lad Prabhakar <[email protected]>

Signed-off-by Mehdi Djait <[email protected]>

---
 drivers/media/v4l2-core/v4l2-common.c | 52 +++++++++++++++++++++++++++
 include/media/v4l2-common.h           | 27 ++++++++++++++
 2 files changed, 79 insertions(+)

drivers/media/v4l2-core/v4l2-common.c +0 -0

<snip>

Quoted 27 lines

include/media/v4l2-common.h +12 -0

index 0a43f56578bc..1c79ca4d5c73 100644
@@ -100,6 +100,7 @@ int v4l2_ctrl_query_fill(struct v4l2_queryctrl *qctrl,
 struct v4l2_device;
 struct v4l2_subdev;
 struct v4l2_subdev_ops;
+struct clk;
 
 /* I2C Helper functions */
 #include <linux/i2c.h>

@@ -620,6 +621,32 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
 			     unsigned int num_of_driver_link_freqs,
 			     unsigned long *bitmap);
 
+/**
+ * devm_v4l2_sensor_clk_get - lookup and obtain a reference to a clock producer
+ *	for a camera sensor.
+ *
+ * @dev: device for v4l2 sensor clock "consumer"
+ * @id: clock consumer ID
+ *
+ * This function behaves the same way as devm_clk_get() except where there
+ * is no clock producer like in ACPI-based platforms.
+ *
+ * For ACPI-based platforms, the function will read the "clock-frequency"

include/media/v4l2-common.h int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs, · line 644

Quoted 10 lines

include/media/v4l2-common.h +10 -0

+ * ACPI _DSD property and register a fixed-clock with the frequency indicated
+ * in the property.
+ *
+ * This function also handles the special ACPI-based system case where:
+ *
+ * * The clock-frequency _DSD property is present.
+ * * A reference to the clock producer is present, where the clock is provided
+ *   by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
+ *
+ * In this case try to set the clock-frequency value to the provided clock.

*
* While ACPI has standardized sensor support, there is no standard support for
* e.g. video receivers. So this function is specific to sensors, hence the
* chosen function name.

Quoted 3 lines

Quoted 1 line

Better suggestions are welcome.

ACPI itself does not standardise camera sensor support. What driver authors

But there's a de facto standard that makes this helper function suitable, isn't there ?

Quoted 13 lines

should know indeed is that this function provides a clock that can be at least queried for frequency, independently of how that clock is controlled or how its frequency is configured.

How about:

* This function may be used in camera sensor drivers only.

Quoted 4 lines

If it is just adding a paragraph, then I can just add that manually. If the change is more involved, then a v2.1 for just this patch should be posted.

Other than this, the series looks good.

Add the new v4l2 helper devm_v4l2_sensor_clk_get() to Documentation. the helper works on both DT- and ACPI-based platforms to retrieve a reference to the clock producer from firmware.

Reviewed-by Lad Prabhakar <[email protected]>

Reviewed-by Laurent Pinchart <[email protected]>

Co-developed-by Laurent Pinchart <[email protected]>

Signed-off-by Mehdi Djait <[email protected]>

---
 .../driver-api/media/camera-sensor.rst        | 24 +++++++++++++------
 1 file changed, 17 insertions(+), 7 deletions(-)

Documentation/driver-api/media/camera-sensor.rst +17 -7

index c290833165e6..94bd1dae82d5 100644
@@ -29,21 +29,31 @@ used in the system. Using another frequency may cause harmful effects
 elsewhere. Therefore only the pre-determined frequencies are configurable by the
 user.
 
+The external clock frequency shall be retrieved by obtaining the external clock
+using the ``devm_v4l2_sensor_clk_get()`` helper function, and then getting its
+frequency with ``clk_get_rate()``. Usage of the helper function guarantees
+correct behaviour regardless of whether the sensor is integrated in a DT-based
+or ACPI-based system.
+
 ACPI
 ~~~~
 
-Read the ``clock-frequency`` _DSD property to denote the frequency. The driver
-can rely on this frequency being used.
+ACPI-based systems typically don't register the sensor external clock with the
+kernel, but specify the external clock frequency in the ``clock-frequency``
+_DSD property. The ``devm_v4l2_sensor_clk_get()`` helper creates and returns a
+fixed clock set at that rate.
 
 Devicetree
 ~~~~~~~~~~
 
-The preferred way to achieve this is using ``assigned-clocks``,
-``assigned-clock-parents`` and ``assigned-clock-rates`` properties. See the
-`clock device tree bindings
+Devicetree-based systems declare the sensor external clock in the device tree
+and reference it from the sensor node. The preferred way to select the external
+clock frequency is to use the ``assigned-clocks``, ``assigned-clock-parents``
+and ``assigned-clock-rates`` properties in the sensor node to set the clock
+rate. See the `clock device tree bindings
 <https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/clock/clock.yaml>`_
-for more information. The driver then gets the frequency using
-``clk_get_rate()``.
+for more information. The ``devm_v4l2_sensor_clk_get()`` helper retrieves and
+returns that clock.
 
 This approach has the drawback that there's no guarantee that the frequency
 hasn't been modified directly or indirectly by another driver, or supported by

devm_clk_get() fails on ACPI-based platforms, where firmware does not provide a direct reference to the clock producer.

Replace devm_clk_get() with the new v4l2 helper devm_v4l2_sensor_clk_get() that works on both DT- and ACPI-based platforms to retrieve a reference to the clock producer from firmware.

Reviewed-by Lad Prabhakar <[email protected]>

Reviewed-by Laurent Pinchart <[email protected]>

Signed-off-by Mehdi Djait <[email protected]>

---
 drivers/media/i2c/imx415.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/media/i2c/imx415.c +0 -0

index 9f37779bd611..2e206a671ed3 100644