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

[Mehdi Djait]

Hi Laurent,

Thank you for the review!

A very small question below.

On Wed, May 21, 2025 at 01:09:44PM +0200, Laurent Pinchart wrote:
> On Wed, May 21, 2025 at 12:52:08PM +0200, Mehdi Djait wrote:

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

device_property_read_u32() returns the following:

Return: number of values if @val was %NULL,
        %0 if the property was found (success),
          %-EINVAL if given arguments are not valid,
          %-ENODATA if the property does not have a value,
          %-EPROTO if the property is not an array of numbers,
          %-EOVERFLOW if the size of the property is not as expected.
          %-ENXIO if no suitable firmware interface is present.

Don't you think it is better to keep the return value and not overshadow
it ? The function is well documented and this may help understand where
the problem comes from if getting the clk fails.

--
Kind Regards
Mehdi Djait