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

[Hans Verkuil <hverkuil+cisco@kernel.org>]

Hi Mehdi, Sakari, Laurent,

On 26/06/2025 15:33, 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 devm_clk_get() 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.
> 
> This function also handles the special ACPI-based system case where:
> The clock-frequency _DSD property is present.
> A reference to the clock producer is present, where the clock is provided
> by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
> In this case try to set the clock-frequency value to the provided clock.

On irc I wondered about the name of the new function since it is sensor
specific, and if it can also be used for e.g. video receivers, then it
should be renamed to something more generic.

According to Laurent there is no ACPI standard for video receivers, so
that's the reason this is sensor specific.

I'd like to see that documented in this patch. Either in the commit log,
or, preferred, in the header in the function description.

I made a suggestion below.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-common.c | 52 +++++++++++++++++++++++++++
>  include/media/v4l2-common.h           | 27 ++++++++++++++
>  2 files changed, 79 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c

<snip>

> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index 0a43f56578bc..1c79ca4d5c73 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -100,6 +100,7 @@ int v4l2_ctrl_query_fill(struct v4l2_queryctrl *qctrl,
>  struct v4l2_device;
>  struct v4l2_subdev;
>  struct v4l2_subdev_ops;
> +struct clk;
>  
>  /* I2C Helper functions */
>  #include <linux/i2c.h>
> @@ -620,6 +621,32 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
>  			     unsigned int num_of_driver_link_freqs,
>  			     unsigned long *bitmap);
>  
> +/**
> + * devm_v4l2_sensor_clk_get - lookup and obtain a reference to a clock producer
> + *	for a camera sensor.
> + *
> + * @dev: device for v4l2 sensor clock "consumer"
> + * @id: clock consumer ID
> + *
> + * This function behaves the same way as devm_clk_get() 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-clock with the frequency indicated
> + * in the property.
> + *
> + * This function also handles the special ACPI-based system case where:
> + *
> + * * The clock-frequency _DSD property is present.
> + * * A reference to the clock producer is present, where the clock is provided
> + *   by a camera sensor PMIC driver (e.g. int3472/tps68470.c)
> + *
> + * In this case try to set the clock-frequency value to the provided clock.

    *
    * While ACPI has standardized sensor support, there is no standard support for
    * e.g. video receivers. So this function is specific to sensors, hence the
    * chosen function name.

Better suggestions are welcome.

If it is just adding a paragraph, then I can just add that manually. If the change
is more involved, then a v2.1 for just this patch should be posted.

Other than this, the series looks good.

Regards,

	Hans

> + *
> + * Returns a pointer to a struct clk on success or an error pointer on failure.
> + */
> +struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id);
> +
>  static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf)
>  {
>  	/*