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

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

Hi Mehdi, Laurent,

On Thu, May 15, 2025 at 03:51:33PM +0200, Mehdi Djait wrote:
> Hi Laurent,
> 
> On Thu, May 15, 2025 at 02:40:50PM +0200, Laurent Pinchart wrote:
> > On Thu, May 15, 2025 at 11:17:37AM +0200, Mehdi Djait wrote:
> > > On Thu, May 15, 2025 at 10:44:03AM +0200, Laurent Pinchart 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:
> > > > > 
> > > > > 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.
> > > > 
> > > > On a side note, the DT bindings for the OV8865 doesn't specify the
> > > > clock-frequency property...
> > > 
> > > Is this wrong ?
> > > 
> > > The OV8865 driver was introduced for DT-based systems, where you will
> > > get a reference to the "struct clk corresponding to the clock producer"
> > > and then get the clock-rate/frequency with a call to:
> > > 
> > > 	rate = clk_get_rate(sensor->extclk);
> > > 
> > > The patch "73dcffeb2ff9 media: i2c: Support 19.2MHz input clock in ov8865"
> > > adding support for clock-frequency came later to support ACPI-based
> > > systems (IPU3 here)
> > 
> > I'd expect all device properties to be documented in DT bindings. Is
> > that an incorrect assumption ?
> > 
> 
> I am actually genuinely asking, is the clock-frequency a device property
> of the ov8865 camera sensor or the clock source, which is a separate device ?

The sensor's.

Could we document how this is supposed to work on DT and ACPI?

I think we should also select COMMON_CLK on ACPI systems for sensor
drivers (in a separate patch maybe?), instead of relying on distributions
enabling it.

> 
> Example the imx258 with a fixed-clock, which has its own compatible
> and DT bindings under bindings/clock/fixed-clock.yaml
> 
> So when adding support for ACPI-based systems, the DT bindings should
> not be changed because getting the clock-frequency from the ACPI _DSD
> property is a workaround only needed on ACPI-based systems.

I wouldn't say it's a workaround, but something that's only needed on ACPI
systems.

> 
>     i2c {
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         sensor@6c {
>             compatible = "sony,imx258";
>             reg = <0x6c>;
>             clocks = <&imx258_clk>;
> 
>             port {
>                 endpoint {
>                     remote-endpoint = <&csi1_ep>;
>                     data-lanes = <1 2 3 4>;
>                     link-frequencies = /bits/ 64 <320000000>;
>                 };
>             };
>         };
>     };
> 
>     /* Oscillator on the camera board */
>     imx258_clk: clk {
>         compatible = "fixed-clock";
>         #clock-cells = <0>;
>         clock-frequency = <19200000>;
>     };
> 

-- 
Regards,

Sakari Ailus