| 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: <20250515084403.GQ23592@pendragon.ideasonboard.com>
Date: Thu, 15 May 2025 10:44:03 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Hans de Goede <hdegoede@...hat.com>
Cc: Mehdi Djait <mehdi.djait@...ux.intel.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
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...
> 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