| Message ID | 20250310122305.209534-1-mehdi.djait@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,v2] media: v4l2-common: Add a helper for obtaining the clock producer | expand |
Hi Mehdi, On Mon, Mar 10, 2025 at 01:23:05PM +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 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 > > drivers/media/v4l2-core/v4l2-common.c | 35 +++++++++++++++++++++++++++ > include/media/v4l2-common.h | 18 ++++++++++++++ > 2 files changed, 53 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > index 0a2f4f0d0a07..99d826acb213 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,35 @@ 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; > + > + 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); devm_clk_hw_register_fixed_rate() is only available when COMMON_CLK is enabled. You need #ifdefs here. In practice without CCF only devm_clk_get_optional() is useful I guess. Another question is then how commonly COMMON_CLK is enabled e.g. on x86 systems. At least Debian kernel has it. Presumably it's common elsewhere, too. > + 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) > { > /*
Hi Sakari, On Mon, May 12, 2025 at 06:41:07PM +0300, Sakari Ailus wrote: > Hi Mehdi, > > On Mon, May 12, 2025 at 10:21:21AM +0200, Mehdi Djait wrote: > > Hi Sakari, > > > > On Sat, May 10, 2025 at 12:56:02PM +0000, Sakari Ailus wrote: > > > Hi Mehdi, > > > > > > On Mon, Mar 10, 2025 at 01:23:05PM +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 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> > > > > 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; > > > > + > > > > + 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); > > > > > > devm_clk_hw_register_fixed_rate() is only available when COMMON_CLK is > > > enabled. You need #ifdefs here. In practice without CCF only > > > devm_clk_get_optional() is useful I guess. > > > > > > > I added a call to IS_REACHABLE(CONFIG_COMMON_CLK) in the v4 of this patch: > > https://lore.kernel.org/linux-media/20250321130329.342236-1-mehdi.djait@linux.intel.com/ > > I wonder if this approach works. Depending on the compiler implementation, > the compiler could (or even should) still run into issues in finding an > unresolvable symbol, even if the symbol is not reachable and can be > optimised away. > So I discussed with Arnd about this (He introduced IS_REACHABLE()): - IS_REACHABLE() is actually discouraged [1] - COFIG_COMMON_CLK is a bool, so IS_ENABLED() will be the right solution here - usage of IS_ENABLED() is also encouraged in the coding style [2] - we will not face compiler issues because the kernel relies on dead code elimination in the compiler. (Actually I remember this is one of the reasons why you cannot compile the kernel with optimization turned off. I don't know how much this argument holds today [3] [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/Documentation/kbuild/kconfig-language.rst?h=next-20250513&id=700bd25bd4f47a0f4e02e0a25dde05f1a6b16eea [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst#n1178 [3] https://lore.kernel.org/all/20080909160452.GB30160@cs181140183.pp.htv.fi/ > > > > > Another question is then how commonly COMMON_CLK is enabled e.g. on x86 > > > systems. At least Debian kernel has it. Presumably it's common elsewhere, > > > too. > > > > on Arch linux it is also enabled and Fedora also. I would also assume it > > is also enabled in the other linux distros. > > Ack, thanks for checking. > > -- > Regards, > > Sakari Ailus -- Kind Regards Mehdi Djait
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c index 0a2f4f0d0a07..99d826acb213 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,35 @@ 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; + + 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; +} +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 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 drivers/media/v4l2-core/v4l2-common.c | 35 +++++++++++++++++++++++++++ include/media/v4l2-common.h | 18 ++++++++++++++ 2 files changed, 53 insertions(+)