| 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
| ||
|
Message-ID: <20250515124050.GA12492@pendragon.ideasonboard.com>
Date: Thu, 15 May 2025 14:40:50 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Mehdi Djait <mehdi.djait@...ux.intel.com>
Cc: Hans de Goede <hdegoede@...hat.com>, sakari.ailus@...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 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 ?
> > > 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.
> >
> > > > ---
> > > > v1 -> v2:
> > > > Suggested by Sakari:
> > > > - removed clk_name
> > > > - removed the IS_ERR() check
> > > > - improved the kernel-doc comment and commit msg
> > > > Link v1: https://lore.kernel.org/linux-media/20250227092643.113939-1-mehdi.djait@linux.intel.com
> > > >
> > > > v2 -> v3:
> > > > - Added #ifdef CONFIG_COMMON_CLK for the ACPI case
> > > > Link v2: https://lore.kernel.org/linux-media/20250310122305.209534-1-mehdi.djait@linux.intel.com/
> > > >
> > > > v3 -> v4:
> > > > Suggested by Laurent:
> > > > - removed the #ifdef to use IS_REACHABLE(CONFIG_COMMON_CLK)
> > > > - changed to kasprintf() to allocate the clk name when id is NULL and
> > > > used the __free(kfree) scope-based cleanup helper when
> > > > defining the variable to hold the allocated name
> > > > Link v3: https://lore.kernel.org/linux-media/20250321093814.18159-1-mehdi.djait@linux.intel.com/
> > > >
> > > >
> > > > drivers/media/v4l2-core/v4l2-common.c | 40 +++++++++++++++++++++++++++
> > > > include/media/v4l2-common.h | 18 ++++++++++++
> > > > 2 files changed, 58 insertions(+)
> > > >
> > > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > > > index 0a2f4f0d0a07..b33152e2c3af 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-common.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > > > @@ -34,6 +34,9 @@
> > > > * Added Gerd Knorrs v4l1 enhancements (Justin Schoeman)
> > > > */
> > > >
> > > > +#include <linux/clk.h>
> > > > +#include <linux/clkdev.h>
> > > > +#include <linux/clk-provider.h>
> > > > #include <linux/module.h>
> > > > #include <linux/types.h>
> > > > #include <linux/kernel.h>
> > > > @@ -636,3 +639,40 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
> > > > return 0;
> > > > }
> > > > EXPORT_SYMBOL_GPL(v4l2_link_freq_to_bitmap);
> > > > +
> > > > +struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id)
> > > > +{
> > > > + const char *clk_id __free(kfree) = NULL;
> > > > + struct clk_hw *clk_hw;
> > > > + struct clk *clk;
> > > > + u32 rate;
> > > > + int ret;
> > > > +
> > > > + clk = devm_clk_get_optional(dev, id);
> > > > + if (clk)
> > > > + return clk;
> > > > +
> > > > + if (!IS_REACHABLE(CONFIG_COMMON_CLK))
> > > > + return ERR_PTR(-ENOENT);
> > > > +
> > > > + if (!is_acpi_node(dev_fwnode(dev)))
> > > > + return ERR_PTR(-ENOENT);
> > > > +
> > > > + ret = device_property_read_u32(dev, "clock-frequency", &rate);
> > > > + if (ret)
> > > > + return ERR_PTR(ret);
> > > > +
> > > > + if (!id) {
> > > > + clk_id = kasprintf(GFP_KERNEL, "clk-%s", dev_name(dev));
> > > > + if (!clk_id)
> > > > + return ERR_PTR(-ENOMEM);
> > > > + id = clk_id;
> > > > + }
> > > > +
> > > > + clk_hw = devm_clk_hw_register_fixed_rate(dev, id, NULL, 0, rate);
> > > > + if (IS_ERR(clk_hw))
> > > > + return ERR_CAST(clk_hw);
> > > > +
> > > > + return clk_hw->clk;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(devm_v4l2_sensor_clk_get);
> > > > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> > > > index 63ad36f04f72..35b9ac698e8a 100644
> > > > --- a/include/media/v4l2-common.h
> > > > +++ b/include/media/v4l2-common.h
> > > > @@ -573,6 +573,24 @@ 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 an optional clock
> > > > + * producer for a camera sensor.
> > > > + *
> > > > + * @dev: device for v4l2 sensor clock "consumer"
> > > > + * @id: clock consumer ID
> > > > + *
> > > > + * This function behaves the same way 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-clock with the frequency indicated
> > > > + * in the property.
> > > > + *
> > > > + * Return:
> > > > + * * pointer to a struct clk on success or an error code 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)
> > > > {
> > > > /*
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists