Re: [PATCH 55/72] media: v4l2-common: Add legacy camera sensor clock helper

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

Hi Mehdi,

On Mon, Jul 14, 2025 at 03:12:28PM +0200, Mehdi Djait wrote:
> On Mon, Jul 14, 2025 at 02:55:02PM +0200, Mehdi Djait wrote:
> > On Thu, Jul 10, 2025 at 08:47:51PM +0300, Laurent Pinchart wrote:
> > > The recently introduced devm_v4l2_sensor_clk_get() helper aims at
> > > simplifying sensor drivers by centralizing clock handling code, as well
> > > as reducing cargo-cult and deprecated behaviour.
> > > 
> > > A set of drivers implement external clock handling in a non-standard
> > > way. This can't be changed as there is a high risk of breaking existing
> > > platforms, but keeping the code as-is creates a risk of new drivers
> > > copying deprecated behaviour.
> > > 
> > > To fix this, introduce a new devm_v4l2_sensor_clk_get_legacy() helper
> > > and use it in those driver. Compared to devm_v4l2_sensor_clk_get(), the
> > > new helper takes the "clock-frequency" property into account and sets
> > > the external clock rate on OF platforms, and adds the ability to specify
> > > a fixed default or fallback clock rate in case the "clock-frequency"
> > > property is not present.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-common.c | 39 +++++++++++++++++++------
> > >  include/media/v4l2-common.h           | 41 ++++++++++++++++++++++++++-
> > >  2 files changed, 70 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > > index cfc78ea6276c..6f140a78e683 100644
> > > --- a/drivers/media/v4l2-core/v4l2-common.c
> > > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > > @@ -703,24 +703,40 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
> > >  }
> > >  EXPORT_SYMBOL_GPL(v4l2_link_freq_to_bitmap);
> > >  
> > > -struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id)
> > > +struct clk *__devm_v4l2_sensor_clk_get(struct device *dev, const char *id,
> > > +				       bool legacy, bool fixed_rate,
> > > +				       unsigned long clk_rate)
> > >  {
> > > +	bool of_node = is_of_node(dev_fwnode(dev));
> > >  	const char *clk_id __free(kfree) = NULL;
> > >  	struct clk_hw *clk_hw;
> > >  	struct clk *clk;
> > > -	bool of_node;
> > > -	u32 rate;
> > > -	int ret;
> > > +	u32 rate = clk_rate;
> > 
> > [..]
> > 
> > > +	int ret = 0;
> > >  
> > >  	clk = devm_clk_get_optional(dev, id);
> > >  	if (IS_ERR(clk))
> > >  		return clk;
> > >  
> > > -	ret = device_property_read_u32(dev, "clock-frequency", &rate);
> > > -	of_node = is_of_node(dev_fwnode(dev));
> > > +	/*
> > > +	 * If the caller didn't request a fixed rate, retrieve it from the
> > > +	 * clock-frequency property. -EINVAL indicates the property is absent,
> > > +	 * and is not a failure. Other errors, or success with a clock-frequency
> > > +	 * value of 0, are hard failures.
> > > +	 */
> > > +	if (!fixed_rate || !clk_rate) {
> > > +		ret = device_property_read_u32(dev, "clock-frequency", &rate);
> > > +		if (ret != -EINVAL || !rate)
> 
> so according to the above comment, I think it should be:
> 		if (ret != -EINVAL || (!rate && !ret))

You're right, there's an issue here.

We need to return an error iif

- clock-frequency can't be read for a reason different than the property
  being absent (to support ACPI platforms where the property could be
  created dynamically at a later time) ; or

- clock-frequency can be read, and returns a 0 rate

That translates to

		if ((ret && ret != -EINVAL) || (!ret && !rate))

Omitting the initial ret test would return -EINVAL when clock-frequency
can be read correctly, which is not right. Do you agree with that ?

> > 
> > [..]
> > 
> > Let's take the case of camera sensor on a DT-system:
> > 
> > it will call the following:
> > devm_v4l2_sensor_clk_get(dev, NULL);
> > -> __devm_v4l2_sensor_clk_get(dev, id, false, false, 0);
> > --> with fixed_rate = 0 and clk_rate = 0 which will be assigned to
> > u32 rate
> > 
> > so it will go into the if statement and try to read the
> > "clock-frequency" property, which does not exist.
> > 
> > ret from device_property_read_u32() is -EINVAL and that is handled but
> > rate will still be 0 so we will return ERR_PTR(-EINVAL)
> > 
> > > +			return ERR_PTR(-EINVAL);
> > > +	}
> > >  
> > >  	if (clk) {
> > > -		if (!ret && !of_node) {
> > > +		/*
> > > +		 * On non-OF platforms, or when legacy behaviour is requested,
> > > +		 * set the clock rate if a rate has been specified by the caller
> > > +		 * of by the clock-frequency property.
> > 		   |
> > nit:		   +-> or ? 

Oops. I'll fix the typo.

> > > +		 */
> > > +		if (rate && (!of_node || legacy)) {
> > >  			ret = clk_set_rate(clk, rate);
> > >  			if (ret) {
> > >  				dev_err(dev, "Failed to set clock rate: %u\n",
> > > @@ -731,9 +747,14 @@ struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id)
> > >  		return clk;
> > >  	}
> > >  
> > > -	if (!IS_ENABLED(CONFIG_COMMON_CLK) || of_node)
> > > +	/*
> > > +	 * Register a dummy fixed clock on non-OF platforms or when legacy
> > > +	 * behaviour is requested. This required the common clock framework.
> > > +	 */
> > > +	if (!IS_ENABLED(CONFIG_COMMON_CLK) || (of_node && !legacy))
> > >  		return ERR_PTR(-ENOENT);
> > >  
> > > +	/* We need a rate to create a clock. */
> > >  	if (ret)
> > >  		return ERR_PTR(ret == -EINVAL ? -EPROBE_DEFER : ret);
> > >  
> > > @@ -750,4 +771,4 @@ struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id)
> > >  
> > >  	return clk_hw->clk;
> > >  }
> > > -EXPORT_SYMBOL_GPL(devm_v4l2_sensor_clk_get);
> > > +EXPORT_SYMBOL_GPL(__devm_v4l2_sensor_clk_get);

-- 
Regards,

Laurent Pinchart