Message-ID: <aCXU4rfcbBpg6p7Y@svinhufvud>
Date: Thu, 15 May 2025 14:49:54 +0300
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Hans de Goede <hdegoede@...hat.com>,
	Mehdi Djait <mehdi.djait@...ux.intel.com>,
	tomi.valkeinen@...asonboard.com, jacopo.mondi@...asonboard.com,
	hverkuil@...all.nl, kieran.bingham@...asonboard.com,
	naush@...pberrypi.com, mchehab@...nel.org,
	dave.stevenson@...pberrypi.com, linux-media@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v4] media: v4l2-common: Add a helper for obtaining
 the clock producer

Hi Laurent,

On Thu, May 15, 2025 at 10:44:03AM +0200, Laurent Pinchart wrote:
> Hi Hans,
> 
> 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@...ux.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...

And they should not.

This property is used on ACPI systems (via software nodes or otherwise) to
convey the frequency. No standard API exist for frequency control so
effectively drivers are informed of the frequency instead. Older bindings
on DT specify "clock-frequency", too, for sensors.

We could do this on ACPI only, that should be fine. Why I haven't
suggested it is because on DT you won't get as far since you always have a
clock.

> 
> > IMHO it would be good if the generic helper would handle
> > this case too and if there is both a clk-provider and
> > a clock-frequency property then try to set the clock-frequency
> > value with clk_set_rate(), arguably you could just warn on
> > a failure to set the rate though, instead of the error
> > the ov8865 driver currently throws.
> > 
> > Sakari, Laurent any opinions on adding handling this case
> > to the generic helper ?
> 
> We really need to standardize the clock-frequency property, and document
> it accordingly. Some drivers use it to set the external clock rate,
> while others use it to inform themselves about the clock rate, without
> changing it, for platforms that have no CCF clock providers. Some
> drivers also set the clock rate to a fixed value, or to a value that
> depends on the link frequency selected by userspace. I don't like this
> situation much.

I'd rather drop the clock-frequency in bindings where it's not really
needed. Drivers that have (or had) it in DT bindings need to continue to
respect it, though, but they probably won't be using this helper. There
aren't very many of these drivers and there won't be any new ones. This is
implicitly documented in driver documentation, but we could add an
explicit note to the documentation of this helper.

-- 
Regards,

Sakari Ailus

Powered by blists - more mailing lists