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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Wed, May 21, 2025 at 12:52:08PM +0200, Mehdi Djait wrote:
> 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);

I would return ERR_PTR(ret) here.

> > +		}
> > +
> > +		return clk;
> > +	}
> > +
> > +	if (ret)
> > +		return ERR_PTR(ret);

And here, return a fixed error code, maybe -ENOENT, as propagating the
device_property_read_u32() error could result in strange error code for
the user.

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

I would very much like to reserve this case for ACPI, yes.

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

Looks good to me.

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

I'd make it a dev_err() and return an error. We can then relax this
check later if there's a need to.

-- 
Regards,

Laurent Pinchart