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

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

Hi Hans,

On Tue, May 20, 2025 at 10:45:17AM +0200, Hans de Goede wrote:
> On 14-May-25 10:25 AM, Mehdi Djait wrote:
> > On Sat, May 10, 2025 at 04:21:09PM +0200, Hans de Goede wrote:
> >> On 21-Mar-25 2:03 PM, 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 in 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>
> >>
> >> This certainly looks quite useful, thank you for working
> >> on this.
> >>
> >> Note on some IPU3 platforms where the clk is provided by
> >> a clk-generator which is part of a special sensor-PMIC
> >> the situation is a bit more complicated.
> >>
> >> Basically if there is both a clk provider and a clock-frequency
> >> property then the clock-frequency value should be set as freq
> >> to the clk-provider, see:
> > 
> > is it even possible to get a reference to the clock producer in ACPI
> > systems or am I missing something here ?
> 
> Yes in some special cases it is possible to get a reference to
> a clock provider on ACPI. E.g. one is provided by:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/intel/int3472/tps68470.c
> 
> on x86 ACPI systems using that sensor PMIC such as the
> "Microsoft Surface Go" and "Microsoft Surface Go 2"
> 
> > Here is what I gathered online for the discussion leading to this patch:
> > ----------------------------------------------------------------------------------------------------------------------------------------------
> > ClockInput resource added to ACPI v6.5: https://uefi.org/specs/ACPI/6.5/19_ASL_Reference.html#clockinput-clock-input-resource-descriptor-macro
> > - commit adding ClockInput resource to acpica: https://github.com/acpica/acpica/commit/661feab5ee01a34af95a389a18c82e79f1aba05a
> > - commit kernel upstream: 520d4a0ee5b6d9c7a1258ace6caa13a94ac35ef8 "ACPICA: add support
> >   for ClockInput resource (v6.5)"
> 
> Ah I see where the confusion is coming from, the clk-provider does not come
> directly from ACPI, it comes from the PMIC driver and the PMIC driver also
> adds a clk-lookup table entry to associate it with the PMIC.
> 
> > this does not mean we can use it: I found this out-of-tree patch to supports fixed clock sources
> > https://github.com/niyas-sait/linux-acpi/blob/main/0001-acpi-add-clock-bindings-for-fixed-clock-resources.patch
> > it was not sent to the acpi mailing list. It was mentioned in this
> > dicussion: https://lore.kernel.org/linux-kernel/78763d69bae04204b2af37201b09f8b5@huawei.com/
> >
> > Another interesting link: https://linaro.atlassian.net/wiki/spaces/CLIENTPC/pages/28822175758/ACPI+Clock+Input+Resources
> > ----------------------------------------------------------------------------------------------------------------------------------------------
> > 
> > link for the dicussion: https://lore.kernel.org/linux-media/20250220154909.152538-1-mehdi.djait@linux.intel.com/
> 
> These 2 links are not relevant, the clk-provider is not directly coming from
> ACPI instead the clk is registered by the PMIC driver for the clk-generator
> part of the PMIC.
> 
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/i2c/ov8865.c#n3020
> >>
> >> for an example of a driver which handles this case.
> > 
> > So if I understood the above correctly: in the ov8865 IPU3/ACPI case:
> > 
> > 1) sensor->extclk is NULL because the clock producer is not available in
> > ACPI systems. ClockInput() ACPI resource was introduced to acpica after
> > the ov8865 patch and the resource is not even being used in the upstream kernel.
> 
> In this specific case it will be not NULL because the PMIC driver
> provides a clk-provider and creates a clk-lookup to match that
> to the ov8865 sensor.
> 
> > 2) the sensor->extclk_rate will be set from reading
> > the clock-frequency _DSD property in:
> > 
> > 	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", &rate);
> 
> This bit is correct, the special thing here is that the PMIC
> clk-provider is programmable so the sensor-driver needs to
> set it to the rate returned by reading "clock-frequency"/
> 
> So basically first call both:
> 
> 1. devm_get_clk()
> 2. fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", &rate);
> 
> and in the special case when *both* succeed do a clk_set_rate()
> call on the returned clk setting it to "rate".

An idea that struck me just now: if the ipu-bridge driver creates the
clock-frequency swnode property, couldn't it also (or instead) set the
clock frequency, in a similar way that the assigned-clock-rates property
is handled in DT ? That would unify the ACPI and DT cases for sensor
drivers.

I don't know how difficult this would be to implement, but I see there's
already a call to of_clk_set_defaults() in i2c_device_probe(), so maybe
this could "just" be turned into a fwnode_clk_set_defaults() ?

-- 
Regards,

Laurent Pinchart