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

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

Hi Mehdi,

On Mon, May 12, 2025 at 10:21:21AM +0200, Mehdi Djait wrote:
> Hi Sakari,
> 
> On Sat, May 10, 2025 at 12:56:02PM +0000, Sakari Ailus wrote:
> > Hi Mehdi,
> > 
> > On Mon, Mar 10, 2025 at 01:23:05PM +0100, Mehdi Djait wrote:
> > > Introduce a helper for v4l2 sensor drivers on both DT- and ACPI-based
> > > platforms to retrieve a reference to the clock producer from firmware.
> > > 
> > > This helper behaves the same as clk_get_optional() except where there is
> > > no clock producer like ACPI-based platforms.
> > > 
> > > For ACPI-based platforms the function will read the "clock-frequency"
> > > ACPI _DSD property and register a fixed frequency clock with the frequency
> > > indicated in the property.
> > > 
> > > Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> 
> 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;
> > > +
> > > +	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));
> > > +		if (!id)
> > > +			return ERR_PTR(-ENOMEM);
> > > +	}
> > > +
> > > +	clk_hw = devm_clk_hw_register_fixed_rate(dev, id, NULL, 0, rate);
> > 
> > devm_clk_hw_register_fixed_rate() is only available when COMMON_CLK is
> > enabled. You need #ifdefs here. In practice without CCF only
> > devm_clk_get_optional() is useful I guess.
> > 
> 
> I added a call to IS_REACHABLE(CONFIG_COMMON_CLK) in the v4 of this patch:
> https://lore.kernel.org/linux-media/20250321130329.342236-1-mehdi.djait@linux.intel.com/

I wonder if this approach works. Depending on the compiler implementation,
the compiler could (or even should) still run into issues in finding an
unresolvable symbol, even if the symbol is not reachable and can be
optimised away.

> 
> > Another question is then how commonly COMMON_CLK is enabled e.g. on x86
> > systems. At least Debian kernel has it. Presumably it's common elsewhere,
> > too.
> 
> on Arch linux it is also enabled and Fedora also. I would also assume it
> is also enabled in the other linux distros.

Ack, thanks for checking.

-- 
Regards,

Sakari Ailus