linux-kernel - Re: [RFC PATCH v5] media: v4l2-common: Add a helper for obtaining the clock producer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <mwh7xx675kulx6tdebuvqtdjfa4ih3ehi2brrcdxfemfnvxsrs@i5nxkvfskfhe>
Date: Wed, 21 May 2025 12:52:08 +0200
From: Mehdi Djait <mehdi.djait@...ux.intel.com>
To: sakari.ailus@...ux.intel.com, laurent.pinchart@...asonboard.com
Cc: tomi.valkeinen@...asonboard.com, jacopo.mondi@...asonboard.com, 
	hverkuil@...all.nl, kieran.bingham@...asonboard.com, naush@...pberrypi.com, 
	mchehab@...nel.org, hdegoede@...hat.com, dave.stevenson@...pberrypi.com, 
	arnd@...db.de, linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v5] media: v4l2-common: Add a helper for obtaining
 the clock producer

Hi everyone,

On Wed, May 21, 2025 at 12:41:15PM +0200, Mehdi Djait wrote:
>  drivers/media/v4l2-core/v4l2-common.c | 46 +++++++++++++++++++++++++++
>  include/media/v4l2-common.h           | 25 +++++++++++++++
>  2 files changed, 71 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 4ee4aa19efe6..6099acd339ad 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>
> @@ -665,3 +668,46 @@ 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;
> +	u32 rate;
> +	int ret;
> +
> +	clk = devm_clk_get_optional(dev, id);
> +	ret = device_property_read_u32(dev, "clock-frequency", &rate);
> +
> +	if (clk) {
> +		if (!ret) {
> +			ret = clk_set_rate(clk, rate);
> +			if (ret)
> +				dev_warn(dev, "Failed to set clock rate: %u\n",
> +					 rate);
> +		}
> +
> +		return clk;
> +	}
> +
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	if (!IS_ENABLED(CONFIG_COMMON_CLK) || !is_acpi_node(dev_fwnode(dev)))
> +		return ERR_PTR(-ENOENT);
> +
> +	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);

I sent this as an RFC because I am still unsure and need comments on two
things. After they are addressed, I plan to send a patch, documentation
patch (what Sakari proposed in the RFC V4 discussion) and convert the
camera sensors using devm_clk_get()

1. Should the case where both the clock and the clock-frequency are
present be reserved just for ACPI systems ? In other words if a DT
system provides both, should we also attempt to set the provided clock
rate ?

If the former makes more sense, maybe add this:

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 6099acd339ad..3dfbbd699c67 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -674,14 +674,16 @@ 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 acpi_node;
        u32 rate;
        int ret;
 
        clk = devm_clk_get_optional(dev, id);
        ret = device_property_read_u32(dev, "clock-frequency", &rate);
+       acpi_node = is_acpi_node(dev_fwnode(dev));
 
        if (clk) {
-               if (!ret) {
+               if (!ret && acpi_node) {
                        ret = clk_set_rate(clk, rate);
                        if (ret)
                                dev_warn(dev, "Failed to set clock rate: %u\n",
@@ -694,7 +696,7 @@ struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id)
        if (ret)
                return ERR_PTR(ret);
 
-       if (!IS_ENABLED(CONFIG_COMMON_CLK) || !is_acpi_node(dev_fwnode(dev)))
+       if (!IS_ENABLED(CONFIG_COMMON_CLK) || !acpi_node)
                return ERR_PTR(-ENOENT);

2. Should we just warn when the clk_set_rate() fails or return err code
and exit ?