Re: [PATCH v1 02/55] Documentation: media: camera-sensor: Mention v4l2_devm_sensor_clk_get() for obtaining the clock

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

Hi Mehdi, Laurent,

On Fri, Jun 20, 2025 at 12:47:05AM +0300, Laurent Pinchart wrote:
> Hi Mehdi,
> 
> Thank you for the patch.
> 
> On Thu, Jun 19, 2025 at 07:58:55PM +0200, Mehdi Djait wrote:
> > Add the new v4l2 helper devm_v4l2_sensor_clk_get() to Documentation. the
> > helper works on both DT- and ACPI-based platforms to retrieve a
> > reference to the clock producer from firmware.
> > 
> > Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> > 
> > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > index c290833165e6..c82c018a5f40 100644
> > --- a/Documentation/driver-api/media/camera-sensor.rst
> > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > @@ -32,7 +32,8 @@ user.
> >  ACPI
> >  ~~~~
> >  
> > -Read the ``clock-frequency`` _DSD property to denote the frequency. The driver
> > +Use ``devm_v4l2_sensor_clk_get()`` to get the clock. The function will read the
> > +``clock-frequency`` _DSD property to denote the frequency. The driver
> >  can rely on this frequency being used.
> 
> This isn't specific to ACPI anymore. I think the documentation should be
> refactored further. Here's a proposal.
> 
> 
> Handling clocks
> ---------------
> 
> Camera sensors have an internal clock tree including a PLL and a number of
> divisors. The clock tree is generally configured by the driver based on a few
> input parameters that are specific to the hardware: the external clock frequency
> and the link frequency. The two parameters generally are specified by system
> firmware. **No other frequencies should be used in any circumstances.**
> 
> The reason why the clock frequencies are so important is that the system is
> usually designed to use specific external clock and link frequencies to ensure
> electro-magnetic compatibility. Using other frequencies may cause harmful
> effects elsewhere. Therefore only the pre-determined frequencies are
> configurable by the user.
> 
> The external clock frequency shall be retrieved by obtaining the external clock
> using the ``devm_v4l2_sensor_clk_get()`` helper function, and then getting its
> frequency with ``clk_get_rate()``. Usage of the helper function guarantees
> correct behaviour regardless of whether the sensor is integrated in a DT-based
> or ACPI-based system.
> 
> ACPI
> ~~~~
> 
> ACPI-based systems typically don't register the sensor external clock with the
> kernel, but specify the external clock frequency in the ``clock-frequency``
> _DSD property. The ``devm_v4l2_sensor_clk_get()`` helper creates and returns a
> fixed clock set at that rate.
> 
> Devicetree
> ~~~~~~~~~~
> 
> Devicetree-based systems declare the sensor external clock in the device tree
> and reference it from the sensor node. The preferred way to select the external
> clock frequency is to use the ``assigned-clocks``, ``assigned-clock-parents``
> and ``assigned-clock-rates`` properties in the sensor node to set the clock
> rate. See the `clock device tree bindings
> <https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/clock/clock.yaml>`_
> for more information. The ``devm_v4l2_sensor_clk_get()`` helper retrieves and
> returns that clock.
> 
> This approach has the drawback that there's no guarantee that the frequency
> hasn't been modified directly or indirectly by another driver, or supported by
> the board's clock tree to begin with. Changes to the Common Clock Framework API
> are required to ensure reliability.

This looks good to me.

> 
> 
> >  
> >  Devicetree
> 

-- 
Regards,

Sakari Ailus