Message-ID: <20250521110817.GA4116@pendragon.ideasonboard.com>
Date: Wed, 21 May 2025 13:08:17 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: Mehdi Djait <mehdi.djait@...ux.intel.com>,
	Hans de Goede <hdegoede@...hat.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

On Wed, May 21, 2025 at 10:54:04AM +0000, Sakari Ailus wrote:
> Hi Laurent,
> 
> On Wed, May 21, 2025 at 12:51:41PM +0200, Laurent Pinchart wrote:
> > 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@...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...
> > > > > > 
> > > > > > 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'd add this to the helper's documentation. We'll work this out with Mehdi.
> 
> > > 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 ?
> 
> I don't think we've added any "clock-frequency" properties in DT bindings
> for camera sensors since around 2020 or so.

That's good news, but I'm not sure it's well known or well documented.

On a side note, should we try to make the existing clock-frequency
properties optional (or even deprecate them and drop them from bindings)
when they are currently mandatory ? The following five YAML bindings
require the property:

- mipi-ccs.yaml
- ovti,ov02a10.yaml
- ovti,ov8856.yaml
- sony,imx214.yaml
- sony,imx290.yaml

The CCS driver treats the property as optional, the imx214 driver
doesn't use it at all, and the other drivers require it. There are other
drivers that require the property, in particular ACPI-only drivers.

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists