| Message ID | 20250321093814.18159-1-mehdi.djait@linux.intel.com (mailing list archive) |
|---|---|
| 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, 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 > > + 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. -- Kind Regards Mehdi Djait
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. >
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(+)