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

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

On Thu, May 15, 2025 at 11:50:19PM +0300, Sakari Ailus wrote:
> On Thu, May 15, 2025 at 03:51:33PM +0200, Mehdi Djait wrote:
> > 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?

Yes please. Would you like to send a patch ? :-)

> 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.

Does that mean that the clock-frequency property should be deprecated on
DT-based systems, and not used in any new sensor bindings ?

> > 
> >     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,

Laurent Pinchart