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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250515125805.GB12492@pendragon.ideasonboard.com>
Date: Thu, 15 May 2025 14:58:05 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.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

On Thu, May 15, 2025 at 02:49:54PM +0300, Sakari Ailus 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...
> 
> 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.

Explicit documentation would be better.

-- 
Regards,

Laurent Pinchart