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 clk_get_optional() except where there is
no clock producer like 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.

Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
---
Link for discussion (where this patch was proposed): https://lore.kernel.org/linux-media/20250220154909.152538-1-mehdi.djait@linux.intel.com/

v1 -> v2:
Suggested by Sakari:
    - removed clk_name
    - removed the IS_ERR() check
    - improved the kernel-doc comment and commit msg
Link for v1: https://lore.kernel.org/linux-media/20250227092643.113939-1-mehdi.djait@linux.intel.com

 drivers/media/v4l2-core/v4l2-common.c | 35 +++++++++++++++++++++++++++
 include/media/v4l2-common.h           | 18 ++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 0a2f4f0d0a07..99d826acb213 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -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>
@@ -636,3 +639,35 @@ 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)
+{
+	struct clk_hw *clk_hw;
+	struct clk *clk;
+	u32 rate;
+	int ret;
+
+	clk = devm_clk_get_optional(dev, id);
+	if (clk)
+		return clk;
+
+	if (!is_acpi_node(dev_fwnode(dev)))
+		return ERR_PTR(-ENOENT);
+
+	ret = device_property_read_u32(dev, "clock-frequency", &rate);
+	if (ret)
+		return ERR_PTR(ret);
+
+	if (!id) {
+		id = devm_kasprintf(dev, GFP_KERNEL, "clk-%s", dev_name(dev));
+		if (!id)
+			return ERR_PTR(-ENOMEM);
+	}
+
+	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);
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index 63ad36f04f72..35b9ac698e8a 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -573,6 +573,24 @@ 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 an optional clock
+ *			      producer for a camera sensor.
+ *
+ * @dev: device for v4l2 sensor clock "consumer"
+ * @id: clock consumer ID
+ *
+ * This function behaves the same way as clk_get_optional() 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.
+ *
+ * Return:
+ * * pointer to a struct clk on success or an error code 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)
 {
 	/*
-- 
2.48.1

Hi Mehdi,

On Mon, Mar 10, 2025 at 01:23:05PM +0100, Mehdi Djait wrote:
> 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 clk_get_optional() except where there is
> no clock producer like 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.
> 
> Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> ---
> Link for discussion (where this patch was proposed): https://lore.kernel.org/linux-media/20250220154909.152538-1-mehdi.djait@linux.intel.com/
> 
> v1 -> v2:
> Suggested by Sakari:
>     - removed clk_name
>     - removed the IS_ERR() check
>     - improved the kernel-doc comment and commit msg
> Link for v1: https://lore.kernel.org/linux-media/20250227092643.113939-1-mehdi.djait@linux.intel.com
> 
>  drivers/media/v4l2-core/v4l2-common.c | 35 +++++++++++++++++++++++++++
>  include/media/v4l2-common.h           | 18 ++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 0a2f4f0d0a07..99d826acb213 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -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>
> @@ -636,3 +639,35 @@ 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)
> +{
> +	struct clk_hw *clk_hw;
> +	struct clk *clk;
> +	u32 rate;
> +	int ret;
> +
> +	clk = devm_clk_get_optional(dev, id);
> +	if (clk)
> +		return clk;
> +
> +	if (!is_acpi_node(dev_fwnode(dev)))
> +		return ERR_PTR(-ENOENT);
> +
> +	ret = device_property_read_u32(dev, "clock-frequency", &rate);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	if (!id) {
> +		id = devm_kasprintf(dev, GFP_KERNEL, "clk-%s", dev_name(dev));
> +		if (!id)
> +			return ERR_PTR(-ENOMEM);
> +	}
> +
> +	clk_hw = devm_clk_hw_register_fixed_rate(dev, id, NULL, 0, rate);

devm_clk_hw_register_fixed_rate() is only available when COMMON_CLK is
enabled. You need #ifdefs here. In practice without CCF only
devm_clk_get_optional() is useful I guess.

Another question is then how commonly COMMON_CLK is enabled e.g. on x86
systems. At least Debian kernel has it. Presumably it's common elsewhere,
too.

> +	if (IS_ERR(clk_hw))
> +		return ERR_CAST(clk_hw);
> +
> +	return clk_hw->clk;
> +}
> +EXPORT_SYMBOL_GPL(devm_v4l2_sensor_clk_get);
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index 63ad36f04f72..35b9ac698e8a 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -573,6 +573,24 @@ 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 an optional clock
> + *			      producer for a camera sensor.
> + *
> + * @dev: device for v4l2 sensor clock "consumer"
> + * @id: clock consumer ID
> + *
> + * This function behaves the same way as clk_get_optional() 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.
> + *
> + * Return:
> + * * pointer to a struct clk on success or an error code 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)
>  {
>  	/*

-- 
Regards,

Sakari Ailus

Hi Sakari,

On Sat, May 10, 2025 at 12:56:02PM +0000, Sakari Ailus wrote:
> Hi Mehdi,
> 
> On Mon, Mar 10, 2025 at 01:23:05PM +0100, Mehdi Djait wrote:
> > 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 clk_get_optional() except where there is
> > no clock producer like 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.
> > 
> > Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>

SNIP

> > +struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id)
> > +{
> > +	struct clk_hw *clk_hw;
> > +	struct clk *clk;
> > +	u32 rate;
> > +	int ret;
> > +
> > +	clk = devm_clk_get_optional(dev, id);
> > +	if (clk)
> > +		return clk;
> > +
> > +	if (!is_acpi_node(dev_fwnode(dev)))
> > +		return ERR_PTR(-ENOENT);
> > +
> > +	ret = device_property_read_u32(dev, "clock-frequency", &rate);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	if (!id) {
> > +		id = devm_kasprintf(dev, GFP_KERNEL, "clk-%s", dev_name(dev));
> > +		if (!id)
> > +			return ERR_PTR(-ENOMEM);
> > +	}
> > +
> > +	clk_hw = devm_clk_hw_register_fixed_rate(dev, id, NULL, 0, rate);
> 
> devm_clk_hw_register_fixed_rate() is only available when COMMON_CLK is
> enabled. You need #ifdefs here. In practice without CCF only
> devm_clk_get_optional() is useful I guess.
> 

I added a call to IS_REACHABLE(CONFIG_COMMON_CLK) in the v4 of this patch:
https://lore.kernel.org/linux-media/20250321130329.342236-1-mehdi.djait@linux.intel.com/

> Another question is then how commonly COMMON_CLK is enabled e.g. on x86
> systems. At least Debian kernel has it. Presumably it's common elsewhere,
> too.

on Arch linux it is also enabled and Fedora also. I would also assume it
is also enabled in the other linux distros.

--
Kind Regards
Mehdi Djait

Hi Mehdi,

On Mon, May 12, 2025 at 10:21:21AM +0200, Mehdi Djait wrote:
> Hi Sakari,
> 
> On Sat, May 10, 2025 at 12:56:02PM +0000, Sakari Ailus wrote:
> > Hi Mehdi,
> > 
> > On Mon, Mar 10, 2025 at 01:23:05PM +0100, Mehdi Djait wrote:
> > > 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 clk_get_optional() except where there is
> > > no clock producer like 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.
> > > 
> > > Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> 
> SNIP
> 
> > > +struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id)
> > > +{
> > > +	struct clk_hw *clk_hw;
> > > +	struct clk *clk;
> > > +	u32 rate;
> > > +	int ret;
> > > +
> > > +	clk = devm_clk_get_optional(dev, id);
> > > +	if (clk)
> > > +		return clk;
> > > +
> > > +	if (!is_acpi_node(dev_fwnode(dev)))
> > > +		return ERR_PTR(-ENOENT);
> > > +
> > > +	ret = device_property_read_u32(dev, "clock-frequency", &rate);
> > > +	if (ret)
> > > +		return ERR_PTR(ret);
> > > +
> > > +	if (!id) {
> > > +		id = devm_kasprintf(dev, GFP_KERNEL, "clk-%s", dev_name(dev));
> > > +		if (!id)
> > > +			return ERR_PTR(-ENOMEM);
> > > +	}
> > > +
> > > +	clk_hw = devm_clk_hw_register_fixed_rate(dev, id, NULL, 0, rate);
> > 
> > devm_clk_hw_register_fixed_rate() is only available when COMMON_CLK is
> > enabled. You need #ifdefs here. In practice without CCF only
> > devm_clk_get_optional() is useful I guess.
> > 
> 
> I added a call to IS_REACHABLE(CONFIG_COMMON_CLK) in the v4 of this patch:
> https://lore.kernel.org/linux-media/20250321130329.342236-1-mehdi.djait@linux.intel.com/

I wonder if this approach works. Depending on the compiler implementation,
the compiler could (or even should) still run into issues in finding an
unresolvable symbol, even if the symbol is not reachable and can be
optimised away.

> 
> > Another question is then how commonly COMMON_CLK is enabled e.g. on x86
> > systems. At least Debian kernel has it. Presumably it's common elsewhere,
> > too.
> 
> on Arch linux it is also enabled and Fedora also. I would also assume it
> is also enabled in the other linux distros.

Ack, thanks for checking.

-- 
Regards,

Sakari Ailus

Hi Sakari,

On Mon, May 12, 2025 at 06:41:07PM +0300, Sakari Ailus wrote:
> Hi Mehdi,
> 
> On Mon, May 12, 2025 at 10:21:21AM +0200, Mehdi Djait wrote:
> > Hi Sakari,
> > 
> > On Sat, May 10, 2025 at 12:56:02PM +0000, Sakari Ailus wrote:
> > > Hi Mehdi,
> > > 
> > > On Mon, Mar 10, 2025 at 01:23:05PM +0100, Mehdi Djait wrote:
> > > > 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 clk_get_optional() except where there is
> > > > no clock producer like 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.
> > > > 
> > > > Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> > 
> > SNIP
> > 
> > > > +struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id)
> > > > +{
> > > > +	struct clk_hw *clk_hw;
> > > > +	struct clk *clk;
> > > > +	u32 rate;
> > > > +	int ret;
> > > > +
> > > > +	clk = devm_clk_get_optional(dev, id);
> > > > +	if (clk)
> > > > +		return clk;
> > > > +
> > > > +	if (!is_acpi_node(dev_fwnode(dev)))
> > > > +		return ERR_PTR(-ENOENT);
> > > > +
> > > > +	ret = device_property_read_u32(dev, "clock-frequency", &rate);
> > > > +	if (ret)
> > > > +		return ERR_PTR(ret);
> > > > +
> > > > +	if (!id) {
> > > > +		id = devm_kasprintf(dev, GFP_KERNEL, "clk-%s", dev_name(dev));
> > > > +		if (!id)
> > > > +			return ERR_PTR(-ENOMEM);
> > > > +	}
> > > > +
> > > > +	clk_hw = devm_clk_hw_register_fixed_rate(dev, id, NULL, 0, rate);
> > > 
> > > devm_clk_hw_register_fixed_rate() is only available when COMMON_CLK is
> > > enabled. You need #ifdefs here. In practice without CCF only
> > > devm_clk_get_optional() is useful I guess.
> > > 
> > 
> > I added a call to IS_REACHABLE(CONFIG_COMMON_CLK) in the v4 of this patch:
> > https://lore.kernel.org/linux-media/20250321130329.342236-1-mehdi.djait@linux.intel.com/
> 
> I wonder if this approach works. Depending on the compiler implementation,
> the compiler could (or even should) still run into issues in finding an
> unresolvable symbol, even if the symbol is not reachable and can be
> optimised away.
> 

So I discussed with Arnd about this (He introduced IS_REACHABLE()):

- IS_REACHABLE() is actually discouraged [1]

- COFIG_COMMON_CLK is a bool, so IS_ENABLED() will be the right solution here

- usage of IS_ENABLED() is also encouraged in the coding style [2]

- we will not face compiler issues because the kernel relies on
  dead code elimination in the compiler. (Actually I remember this is
  one of the reasons why you cannot compile the kernel with optimization
  turned off. I don't know how much this argument holds today [3]

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/Documentation/kbuild/kconfig-language.rst?h=next-20250513&id=700bd25bd4f47a0f4e02e0a25dde05f1a6b16eea
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst#n1178
[3] https://lore.kernel.org/all/20080909160452.GB30160@cs181140183.pp.htv.fi/

> > 
> > > Another question is then how commonly COMMON_CLK is enabled e.g. on x86
> > > systems. At least Debian kernel has it. Presumably it's common elsewhere,
> > > too.
> > 
> > on Arch linux it is also enabled and Fedora also. I would also assume it
> > is also enabled in the other linux distros.
> 
> Ack, thanks for checking.
> 
> -- 
> Regards,
> 
> Sakari Ailus

--
Kind Regards
Mehdi Djait