Hi Mehdi, Thank you for the patch. On Fri, Mar 21, 2025 at 10:38:14AM +0100, 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@xxxxxxxxxxxxxxx> > --- > Link for discussion (where this patch was proposed): https://lore.kernel.org/linux-media/20250220154909.152538-1-mehdi.djait@xxxxxxxxxxxxxxx/ > > v1 -> v2: > Suggested by Sakari: > - removed clk_name > - removed the IS_ERR() check > - improved the kernel-doc comment and commit msg > Link for v1: https://lore.kernel.org/linux-media/20250227092643.113939-1-mehdi.djait@xxxxxxxxxxxxxxx > > v2 -> v3: > - Added #ifdef CONFIG_COMMON_CLK for the ACPI case > > drivers/media/v4l2-core/v4l2-common.c | 39 +++++++++++++++++++++++++++ > include/media/v4l2-common.h | 18 +++++++++++++ > 2 files changed, 57 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > index 0a2f4f0d0a07..4e30f8b777b7 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,39 @@ 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) > +{ > + struct clk_hw *clk_hw; > + struct clk *clk; > + u32 rate; > + int ret; > + > + clk = devm_clk_get_optional(dev, id); > + if (clk) > + return clk; > + > +#ifdef CONFIG_COMMON_CLK This patch will cause warnings when CONFIG_COMMON_CLK is disabled. Could you use if (IS_REACHABLE(CONFIG_COMMON_CLK)) { ... } instead ? It will also ensure that all code gets compile-tested, even when CONFIG_COMMON_CLK is disabled ? If you want to minimize implementation, you could write if (!IS_REACHABLE(CONFIG_COMMON_CLK)) return ERR_PTR(-ENOENT); and keep the code below as-is. > + 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) { > + id = devm_kasprintf(dev, GFP_KERNEL, "clk-%s", dev_name(dev)); As far as I understand, the name doesn't need to stay valid after devm_clk_hw_register_fixed_rate() returns. You can call kasprintf here, and call kfree after devm_clk_hw_register_fixed_rate(). You could use __free to manage the memory life time: const char *clk_id __free(kfree) = NULL; if (!id) { clk_id = kasprintf(GFP_KERNEL, "clk-%s", dev_name(dev)); if (!clk_id) return ERR_PTR(-ENOMEM); id = clk_id; } > + if (!id) > + return ERR_PTR(-ENOMEM); > + } > + > + 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; > +#else > + return ERR_PTR(-ENOENT); > +#endif > +} > +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
