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

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

On Thu, Jun 12, 2025 at 02:15:10PM +0200, Mehdi Djait wrote:
> 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.

I don't mind too much either way. If we want to make debugging easier,
may an error message would be appropriate.

-- 
Regards,

Laurent Pinchart