All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH v2] media: v4l2-common: Add a helper for obtaining the clock producer
@ 2025-03-10 12:23 Mehdi Djait
  2025-03-11 14:51 ` kernel test robot
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Mehdi Djait @ 2025-03-10 12:23 UTC (permalink / raw)
  To: sakari.ailus, laurent.pinchart
  Cc: tomi.valkeinen, jacopo.mondi, hverkuil, kieran.bingham, naush,
	mchehab, hdegoede, dave.stevenson, linux-media, linux-kernel,
	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 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH v2] media: v4l2-common: Add a helper for obtaining the clock producer
  2025-03-10 12:23 [RFC PATCH v2] media: v4l2-common: Add a helper for obtaining the clock producer Mehdi Djait
@ 2025-03-11 14:51 ` kernel test robot
  2025-03-11 16:46 ` kernel test robot
  2025-05-10 12:56 ` Sakari Ailus
  2 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2025-03-11 14:51 UTC (permalink / raw)
  To: Mehdi Djait; +Cc: oe-kbuild-all

Hi Mehdi,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on sailus-media-tree/master v6.14-rc6 next-20250311]
[cannot apply to sailus-media-tree/streams]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mehdi-Djait/media-v4l2-common-Add-a-helper-for-obtaining-the-clock-producer/20250310-202543
base:   linus/master
patch link:    https://lore.kernel.org/r/20250310122305.209534-1-mehdi.djait%40linux.intel.com
patch subject: [RFC PATCH v2] media: v4l2-common: Add a helper for obtaining the clock producer
config: i386-randconfig-054-20250311 (https://download.01.org/0day-ci/archive/20250311/202503112248.4yP7WmdR-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250311/202503112248.4yP7WmdR-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503112248.4yP7WmdR-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: drivers/media/v4l2-core/v4l2-common.o: in function `devm_v4l2_sensor_clk_get':
>> drivers/media/v4l2-core/v4l2-common.c:667: undefined reference to `__clk_hw_register_fixed_rate'


vim +667 drivers/media/v4l2-core/v4l2-common.c

   642	
   643	struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id)
   644	{
   645		struct clk_hw *clk_hw;
   646		struct clk *clk;
   647		u32 rate;
   648		int ret;
   649	
   650		clk = devm_clk_get_optional(dev, id);
   651		if (clk)
   652			return clk;
   653	
   654		if (!is_acpi_node(dev_fwnode(dev)))
   655			return ERR_PTR(-ENOENT);
   656	
   657		ret = device_property_read_u32(dev, "clock-frequency", &rate);
   658		if (ret)
   659			return ERR_PTR(ret);
   660	
   661		if (!id) {
   662			id = devm_kasprintf(dev, GFP_KERNEL, "clk-%s", dev_name(dev));
   663			if (!id)
   664				return ERR_PTR(-ENOMEM);
   665		}
   666	
 > 667		clk_hw = devm_clk_hw_register_fixed_rate(dev, id, NULL, 0, rate);

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH v2] media: v4l2-common: Add a helper for obtaining the clock producer
  2025-03-10 12:23 [RFC PATCH v2] media: v4l2-common: Add a helper for obtaining the clock producer Mehdi Djait
  2025-03-11 14:51 ` kernel test robot
@ 2025-03-11 16:46 ` kernel test robot
  2025-05-10 12:56 ` Sakari Ailus
  2 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2025-03-11 16:46 UTC (permalink / raw)
  To: Mehdi Djait; +Cc: oe-kbuild-all

Hi Mehdi,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on sailus-media-tree/master v6.14-rc6 next-20250311]
[cannot apply to sailus-media-tree/streams]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mehdi-Djait/media-v4l2-common-Add-a-helper-for-obtaining-the-clock-producer/20250310-202543
base:   linus/master
patch link:    https://lore.kernel.org/r/20250310122305.209534-1-mehdi.djait%40linux.intel.com
patch subject: [RFC PATCH v2] media: v4l2-common: Add a helper for obtaining the clock producer
config: i386-randconfig-053-20250311 (https://download.01.org/0day-ci/archive/20250312/202503120028.JHoKfjCI-lkp@intel.com/config)
compiler: clang version 19.1.7 (https://github.com/llvm/llvm-project cd708029e0b2869e80abe31ddb175f7c35361f90)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250312/202503120028.JHoKfjCI-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503120028.JHoKfjCI-lkp@intel.com/

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: __clk_hw_register_fixed_rate
   >>> referenced by v4l2-common.c:667 (drivers/media/v4l2-core/v4l2-common.c:667)
   >>>               drivers/media/v4l2-core/v4l2-common.o:(devm_v4l2_sensor_clk_get) in archive vmlinux.a

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH v2] media: v4l2-common: Add a helper for obtaining the clock producer
  2025-03-10 12:23 [RFC PATCH v2] media: v4l2-common: Add a helper for obtaining the clock producer Mehdi Djait
  2025-03-11 14:51 ` kernel test robot
  2025-03-11 16:46 ` kernel test robot
@ 2025-05-10 12:56 ` Sakari Ailus
  2025-05-12  8:21   ` Mehdi Djait
  2 siblings, 1 reply; 7+ messages in thread
From: Sakari Ailus @ 2025-05-10 12:56 UTC (permalink / raw)
  To: Mehdi Djait
  Cc: laurent.pinchart, tomi.valkeinen, jacopo.mondi, hverkuil,
	kieran.bingham, naush, mchehab, hdegoede, dave.stevenson,
	linux-media, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH v2] media: v4l2-common: Add a helper for obtaining the clock producer
  2025-05-10 12:56 ` Sakari Ailus
@ 2025-05-12  8:21   ` Mehdi Djait
  2025-05-12 15:41     ` Sakari Ailus
  0 siblings, 1 reply; 7+ messages in thread
From: Mehdi Djait @ 2025-05-12  8:21 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: laurent.pinchart, tomi.valkeinen, jacopo.mondi, hverkuil,
	kieran.bingham, naush, mchehab, hdegoede, dave.stevenson,
	linux-media, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH v2] media: v4l2-common: Add a helper for obtaining the clock producer
  2025-05-12  8:21   ` Mehdi Djait
@ 2025-05-12 15:41     ` Sakari Ailus
  2025-05-14 11:43       ` Mehdi Djait
  0 siblings, 1 reply; 7+ messages in thread
From: Sakari Ailus @ 2025-05-12 15:41 UTC (permalink / raw)
  To: Mehdi Djait
  Cc: laurent.pinchart, tomi.valkeinen, jacopo.mondi, hverkuil,
	kieran.bingham, naush, mchehab, hdegoede, dave.stevenson,
	linux-media, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH v2] media: v4l2-common: Add a helper for obtaining the clock producer
  2025-05-12 15:41     ` Sakari Ailus
@ 2025-05-14 11:43       ` Mehdi Djait
  0 siblings, 0 replies; 7+ messages in thread
From: Mehdi Djait @ 2025-05-14 11:43 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: laurent.pinchart, tomi.valkeinen, jacopo.mondi, hverkuil,
	kieran.bingham, naush, mchehab, hdegoede, dave.stevenson,
	linux-media, linux-kernel, Arnd Bergmann

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2025-05-14 11:43 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-10 12:23 [RFC PATCH v2] media: v4l2-common: Add a helper for obtaining the clock producer Mehdi Djait
2025-03-11 14:51 ` kernel test robot
2025-03-11 16:46 ` kernel test robot
2025-05-10 12:56 ` Sakari Ailus
2025-05-12  8:21   ` Mehdi Djait
2025-05-12 15:41     ` Sakari Ailus
2025-05-14 11:43       ` Mehdi Djait

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.