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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Laurent, Mehdi,

On Fri, Mar 21, 2025 at 01:30:43PM +0100, Mehdi Djait wrote:
> Hi Laurent,
> 
> thank you for the review!
> 
> On Fri, Mar 21, 2025 at 12:11:24PM +0200, Laurent Pinchart wrote:
> > Hi Mehdi,
> > 
> > Thank you for the patch.
> > 
> > On Fri, Mar 21, 2025 at 10:38:14AM +0100, Mehdi Djait wrote:
> 
> SNIP
> 
> > > +
> > > +struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id)
> > > +{
> > > +	struct clk_hw *clk_hw;
> > > +	struct clk *clk;
> > > +	u32 rate;
> > > +	int ret;
> > > +
> > > +	clk = devm_clk_get_optional(dev, id);
> > > +	if (clk)
> > > +		return clk;
> > > +
> > > +#ifdef CONFIG_COMMON_CLK
> > 
> > This patch will cause warnings when CONFIG_COMMON_CLK is disabled. Could
> > you use
> > 
> > 	if (IS_REACHABLE(CONFIG_COMMON_CLK)) {
> > 		...
> > 	}
> > 
> > instead ? It will also ensure that all code gets compile-tested, even
> > when CONFIG_COMMON_CLK is disabled ?
> > 
> > If you want to minimize implementation, you could write
> > 
> > 	if (!IS_REACHABLE(CONFIG_COMMON_CLK))
> > 		return ERR_PTR(-ENOENT);
> > 
> > and keep the code below as-is.
> > 
> 
> this is indeed way better! I will change this in the v3

This would work at the moment if devm_clk_hw_register_fixed_rate() was
always available but it evaluates to __clk_hw_register_fixed_rate() that
depends on COMMON_CLK.

I think t he best option would be to add a stub for it for !COMMON_CLK
case. It may take some time to get that merged and into the media tree. C99
also allows declaring variables midst of a basic block so declaring rate
and clkhw later should address the warning.

> 
> > > +	if (!is_acpi_node(dev_fwnode(dev)))
> > > +		return ERR_PTR(-ENOENT);
> > > +
> > > +	ret = device_property_read_u32(dev, "clock-frequency", &rate);
> > > +	if (ret)
> > > +		return ERR_PTR(ret);
> > > +
> > > +	if (!id) {
> > > +		id = devm_kasprintf(dev, GFP_KERNEL, "clk-%s", dev_name(dev));
> > 
> > As far as I understand, the name doesn't need to stay valid after
> > devm_clk_hw_register_fixed_rate() returns. You can call kasprintf here,
> > and call kfree after devm_clk_hw_register_fixed_rate(). You could use
> > __free to manage the memory life time:
> > 
> > 	const char *clk_id __free(kfree) = NULL;
> > 
> > 	if (!id) {
> > 		clk_id = kasprintf(GFP_KERNEL, "clk-%s", dev_name(dev));
> > 		if (!clk_id)
> > 			return ERR_PTR(-ENOMEM);
> > 		id = clk_id;
> > 	}
> > 
> 
> Ack.
> 

-- 
Kind regards,

Sakari Ailus