| Message ID | 20250321093814.18159-1-mehdi.djait@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v3] media: v4l2-common: Add a helper for obtaining the clock producer | expand |
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@linux.intel.com> > --- > Link for discussion (where this patch was proposed): https://lore.kernel.org/linux-media/20250220154909.152538-1-mehdi.djait@linux.intel.com/ > > 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@linux.intel.com > > 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) > { > /*
Hi Laurent, Mehdi, On Fri, Mar 21, 2025 at 01:30:43PM +0100, Mehdi Djait wrote: > Hi Laurent, > > thank you for the review! > > On Fri, Mar 21, 2025 at 12:11:24PM +0200, Laurent Pinchart wrote: > > Hi Mehdi, > > > > Thank you for the patch. > > > > On Fri, Mar 21, 2025 at 10:38:14AM +0100, Mehdi Djait wrote: > > SNIP > > > > + > > > +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. > > > > this is indeed way better! I will change this in the v3 This would work at the moment if devm_clk_hw_register_fixed_rate() was always available but it evaluates to __clk_hw_register_fixed_rate() that depends on COMMON_CLK. I think t he best option would be to add a stub for it for !COMMON_CLK case. It may take some time to get that merged and into the media tree. C99 also allows declaring variables midst of a basic block so declaring rate and clkhw later should address the warning. > > > > + 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; > > } > > > > Ack. >
Hi Sakari, Laurent, On Mon, Mar 24, 2025 at 07:37:57AM +0000, Sakari Ailus wrote: > Hi Laurent, Mehdi, > > On Fri, Mar 21, 2025 at 01:30:43PM +0100, Mehdi Djait wrote: > > Hi Laurent, > > > > thank you for the review! > > > > On Fri, Mar 21, 2025 at 12:11:24PM +0200, Laurent Pinchart wrote: > > > Hi Mehdi, > > > > > > Thank you for the patch. > > > > > > On Fri, Mar 21, 2025 at 10:38:14AM +0100, Mehdi Djait wrote: > > > > SNIP > > > > > > + > > > > +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. > > > > > > > this is indeed way better! I will change this in the v3 > > This would work at the moment if devm_clk_hw_register_fixed_rate() was > always available but it evaluates to __clk_hw_register_fixed_rate() that > depends on COMMON_CLK. > > I think t he best option would be to add a stub for it for !COMMON_CLK > case. It may take some time to get that merged and into the media tree. C99 > also allows declaring variables midst of a basic block so declaring rate > and clkhw later should address the warning. I don't fully understand the concern with the availability of the function. It is used by drivers in multiple subsystems and any change to it should take care of the API users. Don't you think that adding a stub for !COMMON_CLK would make the series take a much longer time to get merged and complicate the situation ? -- Kind Regards Mehdi Djait
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 + 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)); + 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) { /*
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@linux.intel.com> --- Link for discussion (where this patch was proposed): https://lore.kernel.org/linux-media/20250220154909.152538-1-mehdi.djait@linux.intel.com/ 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@linux.intel.com 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(+)