drivers/media/v4l2-core/v4l2-common.c | 40 +++++++++++++++++++++++++++ include/media/v4l2-common.h | 18 ++++++++++++ 2 files changed, 58 insertions(+)
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>
---
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)
{
/*
--
2.48.1Hi Mehdi,
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@linux.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.
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 ?
Regards,
Hans
> ---
> 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)
> {
> /*
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@linux.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
Hi Laurent, On Thu, May 15, 2025 at 10:44:03AM +0200, Laurent Pinchart wrote: > 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@linux.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... And they should not. This property is used on ACPI systems (via software nodes or otherwise) to convey the frequency. No standard API exist for frequency control so effectively drivers are informed of the frequency instead. Older bindings on DT specify "clock-frequency", too, for sensors. We could do this on ACPI only, that should be fine. Why I haven't suggested it is because on DT you won't get as far since you always have a clock. > > > 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. I'd rather drop the clock-frequency in bindings where it's not really needed. Drivers that have (or had) it in DT bindings need to continue to respect it, though, but they probably won't be using this helper. There aren't very many of these drivers and there won't be any new ones. This is implicitly documented in driver documentation, but we could add an explicit note to the documentation of this helper. -- Regards, Sakari Ailus
On Thu, May 15, 2025 at 02:49:54PM +0300, Sakari Ailus wrote: > On Thu, May 15, 2025 at 10:44:03AM +0200, Laurent Pinchart wrote: > > 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@linux.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... > > And they should not. > > This property is used on ACPI systems (via software nodes or otherwise) to > convey the frequency. No standard API exist for frequency control so > effectively drivers are informed of the frequency instead. Older bindings > on DT specify "clock-frequency", too, for sensors. > > We could do this on ACPI only, that should be fine. Why I haven't > suggested it is because on DT you won't get as far since you always have a > clock. > > > > 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. > > I'd rather drop the clock-frequency in bindings where it's not really > needed. Drivers that have (or had) it in DT bindings need to continue to > respect it, though, but they probably won't be using this helper. There > aren't very many of these drivers and there won't be any new ones. This is > implicitly documented in driver documentation, but we could add an > explicit note to the documentation of this helper. Explicit documentation would be better. -- Regards, Laurent Pinchart
Hi Laurent,
On Thu, May 15, 2025 at 10:44:03AM +0200, Laurent Pinchart wrote:
> 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@linux.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...
>
Is this wrong ?
The OV8865 driver was introduced for DT-based systems, where you will
get a reference to the "struct clk corresponding to the clock producer"
and then get the clock-rate/frequency with a call to:
rate = clk_get_rate(sensor->extclk);
The patch "73dcffeb2ff9 media: i2c: Support 19.2MHz input clock in ov8865"
adding support for clock-frequency came later to support ACPI-based
systems (IPU3 here)
> > 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
--
Kind Regards
Mehdi Djait
On Thu, May 15, 2025 at 11:17:37AM +0200, Mehdi Djait wrote:
> On Thu, May 15, 2025 at 10:44:03AM +0200, Laurent Pinchart wrote:
> > 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@linux.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...
>
> Is this wrong ?
>
> The OV8865 driver was introduced for DT-based systems, where you will
> get a reference to the "struct clk corresponding to the clock producer"
> and then get the clock-rate/frequency with a call to:
>
> rate = clk_get_rate(sensor->extclk);
>
> The patch "73dcffeb2ff9 media: i2c: Support 19.2MHz input clock in ov8865"
> adding support for clock-frequency came later to support ACPI-based
> systems (IPU3 here)
I'd expect all device properties to be documented in DT bindings. Is
that an incorrect assumption ?
> > > 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
Hi Laurent,
On Thu, May 15, 2025 at 02:40:50PM +0200, Laurent Pinchart wrote:
> On Thu, May 15, 2025 at 11:17:37AM +0200, Mehdi Djait wrote:
> > On Thu, May 15, 2025 at 10:44:03AM +0200, Laurent Pinchart wrote:
> > > 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@linux.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...
> >
> > Is this wrong ?
> >
> > The OV8865 driver was introduced for DT-based systems, where you will
> > get a reference to the "struct clk corresponding to the clock producer"
> > and then get the clock-rate/frequency with a call to:
> >
> > rate = clk_get_rate(sensor->extclk);
> >
> > The patch "73dcffeb2ff9 media: i2c: Support 19.2MHz input clock in ov8865"
> > adding support for clock-frequency came later to support ACPI-based
> > systems (IPU3 here)
>
> I'd expect all device properties to be documented in DT bindings. Is
> that an incorrect assumption ?
>
I am actually genuinely asking, is the clock-frequency a device property
of the ov8865 camera sensor or the clock source, which is a separate device ?
Example the imx258 with a fixed-clock, which has its own compatible
and DT bindings under bindings/clock/fixed-clock.yaml
So when adding support for ACPI-based systems, the DT bindings should
not be changed because getting the clock-frequency from the ACPI _DSD
property is a workaround only needed on ACPI-based systems.
i2c {
#address-cells = <1>;
#size-cells = <0>;
sensor@6c {
compatible = "sony,imx258";
reg = <0x6c>;
clocks = <&imx258_clk>;
port {
endpoint {
remote-endpoint = <&csi1_ep>;
data-lanes = <1 2 3 4>;
link-frequencies = /bits/ 64 <320000000>;
};
};
};
};
/* Oscillator on the camera board */
imx258_clk: clk {
compatible = "fixed-clock";
#clock-cells = <0>;
clock-frequency = <19200000>;
};
--
Kind Regards
Mehdi Djait
Hi Mehdi, Laurent,
On Thu, May 15, 2025 at 03:51:33PM +0200, Mehdi Djait wrote:
> Hi Laurent,
>
> On Thu, May 15, 2025 at 02:40:50PM +0200, Laurent Pinchart wrote:
> > On Thu, May 15, 2025 at 11:17:37AM +0200, Mehdi Djait wrote:
> > > On Thu, May 15, 2025 at 10:44:03AM +0200, Laurent Pinchart wrote:
> > > > 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@linux.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...
> > >
> > > Is this wrong ?
> > >
> > > The OV8865 driver was introduced for DT-based systems, where you will
> > > get a reference to the "struct clk corresponding to the clock producer"
> > > and then get the clock-rate/frequency with a call to:
> > >
> > > rate = clk_get_rate(sensor->extclk);
> > >
> > > The patch "73dcffeb2ff9 media: i2c: Support 19.2MHz input clock in ov8865"
> > > adding support for clock-frequency came later to support ACPI-based
> > > systems (IPU3 here)
> >
> > I'd expect all device properties to be documented in DT bindings. Is
> > that an incorrect assumption ?
> >
>
> I am actually genuinely asking, is the clock-frequency a device property
> of the ov8865 camera sensor or the clock source, which is a separate device ?
The sensor's.
Could we document how this is supposed to work on DT and ACPI?
I think we should also select COMMON_CLK on ACPI systems for sensor
drivers (in a separate patch maybe?), instead of relying on distributions
enabling it.
>
> Example the imx258 with a fixed-clock, which has its own compatible
> and DT bindings under bindings/clock/fixed-clock.yaml
>
> So when adding support for ACPI-based systems, the DT bindings should
> not be changed because getting the clock-frequency from the ACPI _DSD
> property is a workaround only needed on ACPI-based systems.
I wouldn't say it's a workaround, but something that's only needed on ACPI
systems.
>
> i2c {
> #address-cells = <1>;
> #size-cells = <0>;
>
> sensor@6c {
> compatible = "sony,imx258";
> reg = <0x6c>;
> clocks = <&imx258_clk>;
>
> port {
> endpoint {
> remote-endpoint = <&csi1_ep>;
> data-lanes = <1 2 3 4>;
> link-frequencies = /bits/ 64 <320000000>;
> };
> };
> };
> };
>
> /* Oscillator on the camera board */
> imx258_clk: clk {
> compatible = "fixed-clock";
> #clock-cells = <0>;
> clock-frequency = <19200000>;
> };
>
--
Regards,
Sakari Ailus
On Thu, May 15, 2025 at 11:50:19PM +0300, Sakari Ailus wrote:
> On Thu, May 15, 2025 at 03:51:33PM +0200, Mehdi Djait wrote:
> > On Thu, May 15, 2025 at 02:40:50PM +0200, Laurent Pinchart wrote:
> > > On Thu, May 15, 2025 at 11:17:37AM +0200, Mehdi Djait wrote:
> > > > On Thu, May 15, 2025 at 10:44:03AM +0200, Laurent Pinchart wrote:
> > > > > 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@linux.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...
> > > >
> > > > Is this wrong ?
> > > >
> > > > The OV8865 driver was introduced for DT-based systems, where you will
> > > > get a reference to the "struct clk corresponding to the clock producer"
> > > > and then get the clock-rate/frequency with a call to:
> > > >
> > > > rate = clk_get_rate(sensor->extclk);
> > > >
> > > > The patch "73dcffeb2ff9 media: i2c: Support 19.2MHz input clock in ov8865"
> > > > adding support for clock-frequency came later to support ACPI-based
> > > > systems (IPU3 here)
> > >
> > > I'd expect all device properties to be documented in DT bindings. Is
> > > that an incorrect assumption ?
> > >
> >
> > I am actually genuinely asking, is the clock-frequency a device property
> > of the ov8865 camera sensor or the clock source, which is a separate device ?
>
> The sensor's.
>
> Could we document how this is supposed to work on DT and ACPI?
Yes please. Would you like to send a patch ? :-)
> I think we should also select COMMON_CLK on ACPI systems for sensor
> drivers (in a separate patch maybe?), instead of relying on distributions
> enabling it.
>
> > Example the imx258 with a fixed-clock, which has its own compatible
> > and DT bindings under bindings/clock/fixed-clock.yaml
> >
> > So when adding support for ACPI-based systems, the DT bindings should
> > not be changed because getting the clock-frequency from the ACPI _DSD
> > property is a workaround only needed on ACPI-based systems.
>
> I wouldn't say it's a workaround, but something that's only needed on ACPI
> systems.
Does that mean that the clock-frequency property should be deprecated on
DT-based systems, and not used in any new sensor bindings ?
> >
> > i2c {
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > sensor@6c {
> > compatible = "sony,imx258";
> > reg = <0x6c>;
> > clocks = <&imx258_clk>;
> >
> > port {
> > endpoint {
> > remote-endpoint = <&csi1_ep>;
> > data-lanes = <1 2 3 4>;
> > link-frequencies = /bits/ 64 <320000000>;
> > };
> > };
> > };
> > };
> >
> > /* Oscillator on the camera board */
> > imx258_clk: clk {
> > compatible = "fixed-clock";
> > #clock-cells = <0>;
> > clock-frequency = <19200000>;
> > };
> >
--
Regards,
Laurent Pinchart
Hi Laurent, On Wed, May 21, 2025 at 12:51:41PM +0200, Laurent Pinchart wrote: > On Thu, May 15, 2025 at 11:50:19PM +0300, Sakari Ailus wrote: > > On Thu, May 15, 2025 at 03:51:33PM +0200, Mehdi Djait wrote: > > > On Thu, May 15, 2025 at 02:40:50PM +0200, Laurent Pinchart wrote: > > > > On Thu, May 15, 2025 at 11:17:37AM +0200, Mehdi Djait wrote: > > > > > On Thu, May 15, 2025 at 10:44:03AM +0200, Laurent Pinchart wrote: > > > > > > 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@linux.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... > > > > > > > > > > Is this wrong ? > > > > > > > > > > The OV8865 driver was introduced for DT-based systems, where you will > > > > > get a reference to the "struct clk corresponding to the clock producer" > > > > > and then get the clock-rate/frequency with a call to: > > > > > > > > > > rate = clk_get_rate(sensor->extclk); > > > > > > > > > > The patch "73dcffeb2ff9 media: i2c: Support 19.2MHz input clock in ov8865" > > > > > adding support for clock-frequency came later to support ACPI-based > > > > > systems (IPU3 here) > > > > > > > > I'd expect all device properties to be documented in DT bindings. Is > > > > that an incorrect assumption ? > > > > > > > > > > I am actually genuinely asking, is the clock-frequency a device property > > > of the ov8865 camera sensor or the clock source, which is a separate device ? > > > > The sensor's. > > > > Could we document how this is supposed to work on DT and ACPI? > > Yes please. Would you like to send a patch ? :-) I'd add this to the helper's documentation. We'll work this out with Mehdi. > > > I think we should also select COMMON_CLK on ACPI systems for sensor > > drivers (in a separate patch maybe?), instead of relying on distributions > > enabling it. > > > > > Example the imx258 with a fixed-clock, which has its own compatible > > > and DT bindings under bindings/clock/fixed-clock.yaml > > > > > > So when adding support for ACPI-based systems, the DT bindings should > > > not be changed because getting the clock-frequency from the ACPI _DSD > > > property is a workaround only needed on ACPI-based systems. > > > > I wouldn't say it's a workaround, but something that's only needed on ACPI > > systems. > > Does that mean that the clock-frequency property should be deprecated on > DT-based systems, and not used in any new sensor bindings ? I don't think we've added any "clock-frequency" properties in DT bindings for camera sensors since around 2020 or so. -- Regards, Sakari Ailus
On Wed, May 21, 2025 at 10:54:04AM +0000, Sakari Ailus wrote: > Hi Laurent, > > On Wed, May 21, 2025 at 12:51:41PM +0200, Laurent Pinchart wrote: > > On Thu, May 15, 2025 at 11:50:19PM +0300, Sakari Ailus wrote: > > > On Thu, May 15, 2025 at 03:51:33PM +0200, Mehdi Djait wrote: > > > > On Thu, May 15, 2025 at 02:40:50PM +0200, Laurent Pinchart wrote: > > > > > On Thu, May 15, 2025 at 11:17:37AM +0200, Mehdi Djait wrote: > > > > > > On Thu, May 15, 2025 at 10:44:03AM +0200, Laurent Pinchart wrote: > > > > > > > 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@linux.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... > > > > > > > > > > > > Is this wrong ? > > > > > > > > > > > > The OV8865 driver was introduced for DT-based systems, where you will > > > > > > get a reference to the "struct clk corresponding to the clock producer" > > > > > > and then get the clock-rate/frequency with a call to: > > > > > > > > > > > > rate = clk_get_rate(sensor->extclk); > > > > > > > > > > > > The patch "73dcffeb2ff9 media: i2c: Support 19.2MHz input clock in ov8865" > > > > > > adding support for clock-frequency came later to support ACPI-based > > > > > > systems (IPU3 here) > > > > > > > > > > I'd expect all device properties to be documented in DT bindings. Is > > > > > that an incorrect assumption ? > > > > > > > > > > > > > I am actually genuinely asking, is the clock-frequency a device property > > > > of the ov8865 camera sensor or the clock source, which is a separate device ? > > > > > > The sensor's. > > > > > > Could we document how this is supposed to work on DT and ACPI? > > > > Yes please. Would you like to send a patch ? :-) > > I'd add this to the helper's documentation. We'll work this out with Mehdi. > > > > I think we should also select COMMON_CLK on ACPI systems for sensor > > > drivers (in a separate patch maybe?), instead of relying on distributions > > > enabling it. > > > > > > > Example the imx258 with a fixed-clock, which has its own compatible > > > > and DT bindings under bindings/clock/fixed-clock.yaml > > > > > > > > So when adding support for ACPI-based systems, the DT bindings should > > > > not be changed because getting the clock-frequency from the ACPI _DSD > > > > property is a workaround only needed on ACPI-based systems. > > > > > > I wouldn't say it's a workaround, but something that's only needed on ACPI > > > systems. > > > > Does that mean that the clock-frequency property should be deprecated on > > DT-based systems, and not used in any new sensor bindings ? > > I don't think we've added any "clock-frequency" properties in DT bindings > for camera sensors since around 2020 or so. That's good news, but I'm not sure it's well known or well documented. On a side note, should we try to make the existing clock-frequency properties optional (or even deprecate them and drop them from bindings) when they are currently mandatory ? The following five YAML bindings require the property: - mipi-ccs.yaml - ovti,ov02a10.yaml - ovti,ov8856.yaml - sony,imx214.yaml - sony,imx290.yaml The CCS driver treats the property as optional, the imx214 driver doesn't use it at all, and the other drivers require it. There are other drivers that require the property, in particular ACPI-only drivers. -- Regards, Laurent Pinchart
On Wed, May 21, 2025 at 01:08:17PM +0200, Laurent Pinchart wrote: > On Wed, May 21, 2025 at 10:54:04AM +0000, Sakari Ailus wrote: > > Hi Laurent, > > > > On Wed, May 21, 2025 at 12:51:41PM +0200, Laurent Pinchart wrote: > > > On Thu, May 15, 2025 at 11:50:19PM +0300, Sakari Ailus wrote: > > > > On Thu, May 15, 2025 at 03:51:33PM +0200, Mehdi Djait wrote: > > > > > On Thu, May 15, 2025 at 02:40:50PM +0200, Laurent Pinchart wrote: > > > > > > On Thu, May 15, 2025 at 11:17:37AM +0200, Mehdi Djait wrote: > > > > > > > On Thu, May 15, 2025 at 10:44:03AM +0200, Laurent Pinchart wrote: > > > > > > > > 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@linux.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... > > > > > > > > > > > > > > Is this wrong ? > > > > > > > > > > > > > > The OV8865 driver was introduced for DT-based systems, where you will > > > > > > > get a reference to the "struct clk corresponding to the clock producer" > > > > > > > and then get the clock-rate/frequency with a call to: > > > > > > > > > > > > > > rate = clk_get_rate(sensor->extclk); > > > > > > > > > > > > > > The patch "73dcffeb2ff9 media: i2c: Support 19.2MHz input clock in ov8865" > > > > > > > adding support for clock-frequency came later to support ACPI-based > > > > > > > systems (IPU3 here) > > > > > > > > > > > > I'd expect all device properties to be documented in DT bindings. Is > > > > > > that an incorrect assumption ? > > > > > > > > > > > > > > > > I am actually genuinely asking, is the clock-frequency a device property > > > > > of the ov8865 camera sensor or the clock source, which is a separate device ? > > > > > > > > The sensor's. > > > > > > > > Could we document how this is supposed to work on DT and ACPI? > > > > > > Yes please. Would you like to send a patch ? :-) > > > > I'd add this to the helper's documentation. We'll work this out with Mehdi. > > > > > > I think we should also select COMMON_CLK on ACPI systems for sensor > > > > drivers (in a separate patch maybe?), instead of relying on distributions > > > > enabling it. > > > > > > > > > Example the imx258 with a fixed-clock, which has its own compatible > > > > > and DT bindings under bindings/clock/fixed-clock.yaml > > > > > > > > > > So when adding support for ACPI-based systems, the DT bindings should > > > > > not be changed because getting the clock-frequency from the ACPI _DSD > > > > > property is a workaround only needed on ACPI-based systems. > > > > > > > > I wouldn't say it's a workaround, but something that's only needed on ACPI > > > > systems. > > > > > > Does that mean that the clock-frequency property should be deprecated on > > > DT-based systems, and not used in any new sensor bindings ? > > > > I don't think we've added any "clock-frequency" properties in DT bindings > > for camera sensors since around 2020 or so. > > That's good news, but I'm not sure it's well known or well documented. > > On a side note, should we try to make the existing clock-frequency > properties optional (or even deprecate them and drop them from bindings) > when they are currently mandatory ? The following five YAML bindings > require the property: > > - mipi-ccs.yaml > - ovti,ov02a10.yaml > - ovti,ov8856.yaml > - sony,imx214.yaml > - sony,imx290.yaml > > The CCS driver treats the property as optional, the imx214 driver > doesn't use it at all, and the other drivers require it. There are other > drivers that require the property, in particular ACPI-only drivers. Given the discussion in the context of the André's imx214 patches, I think we can drop them all, both in drivers and DT. -- Sakari Ailus
Hi Sakari, On Thu, May 15, 2025 at 11:50:19PM +0300, Sakari Ailus wrote: > Hi Mehdi, Laurent, > > On Thu, May 15, 2025 at 03:51:33PM +0200, Mehdi Djait wrote: > > Hi Laurent, > > > > On Thu, May 15, 2025 at 02:40:50PM +0200, Laurent Pinchart wrote: > > > On Thu, May 15, 2025 at 11:17:37AM +0200, Mehdi Djait wrote: > > > > On Thu, May 15, 2025 at 10:44:03AM +0200, Laurent Pinchart wrote: > > > > > 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@linux.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... > > > > > > > > Is this wrong ? > > > > > > > > The OV8865 driver was introduced for DT-based systems, where you will > > > > get a reference to the "struct clk corresponding to the clock producer" > > > > and then get the clock-rate/frequency with a call to: > > > > > > > > rate = clk_get_rate(sensor->extclk); > > > > > > > > The patch "73dcffeb2ff9 media: i2c: Support 19.2MHz input clock in ov8865" > > > > adding support for clock-frequency came later to support ACPI-based > > > > systems (IPU3 here) > > > > > > I'd expect all device properties to be documented in DT bindings. Is > > > that an incorrect assumption ? > > > > > > > I am actually genuinely asking, is the clock-frequency a device property > > of the ov8865 camera sensor or the clock source, which is a separate device ? > > The sensor's. Ack. > > Could we document how this is supposed to work on DT and ACPI? I can include a documentation patch in the next version. > > I think we should also select COMMON_CLK on ACPI systems for sensor > drivers (in a separate patch maybe?), instead of relying on distributions > enabling it. > makes sense. > > > > Example the imx258 with a fixed-clock, which has its own compatible > > and DT bindings under bindings/clock/fixed-clock.yaml > > > > So when adding support for ACPI-based systems, the DT bindings should > > not be changed because getting the clock-frequency from the ACPI _DSD > > property is a workaround only needed on ACPI-based systems. > > I wouldn't say it's a workaround, but something that's only needed on ACPI > systems. > Ack. -- Kind Regards Mehdi Djait
Hello Hans, thank you for the comment. On Sat, May 10, 2025 at 04:21:09PM +0200, Hans de Goede wrote: > Hi Mehdi, > > 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@linux.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: > is it even possible to get a reference to the clock producer in ACPI systems or am I missing something here ? Here is what I gathered online for the discussion leading to this patch: ---------------------------------------------------------------------------------------------------------------------------------------------- ClockInput resource added to ACPI v6.5: https://uefi.org/specs/ACPI/6.5/19_ASL_Reference.html#clockinput-clock-input-resource-descriptor-macro - commit adding ClockInput resource to acpica: https://github.com/acpica/acpica/commit/661feab5ee01a34af95a389a18c82e79f1aba05a - commit kernel upstream: 520d4a0ee5b6d9c7a1258ace6caa13a94ac35ef8 "ACPICA: add support for ClockInput resource (v6.5)" this does not mean we can use it: I found this out-of-tree patch to supports fixed clock sources https://github.com/niyas-sait/linux-acpi/blob/main/0001-acpi-add-clock-bindings-for-fixed-clock-resources.patch it was not sent to the acpi mailing list. It was mentioned in this dicussion: https://lore.kernel.org/linux-kernel/78763d69bae04204b2af37201b09f8b5@huawei.com/ Another interesting link: https://linaro.atlassian.net/wiki/spaces/CLIENTPC/pages/28822175758/ACPI+Clock+Input+Resources ---------------------------------------------------------------------------------------------------------------------------------------------- link for the dicussion: https://lore.kernel.org/linux-media/20250220154909.152538-1-mehdi.djait@linux.intel.com/ > 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. So if I understood the above correctly: in the ov8865 IPU3/ACPI case: 1) sensor->extclk is NULL because the clock producer is not available in ACPI systems. ClockInput() ACPI resource was introduced to acpica after the ov8865 patch and the resource is not even being used in the upstream kernel. 2) the sensor->extclk_rate will be set from reading the clock-frequency _DSD property in: ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", &rate); > > 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 ? > > Regards, > > Hans -- Kind Regards Mehdi Djait
Hi Mehdi, On 14-May-25 10:25 AM, Mehdi Djait wrote: > Hello Hans, > > thank you for the comment. > > On Sat, May 10, 2025 at 04:21:09PM +0200, Hans de Goede wrote: >> Hi Mehdi, >> >> 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@linux.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: >> > > is it even possible to get a reference to the clock producer in ACPI > systems or am I missing something here ? Yes in some special cases it is possible to get a reference to a clock provider on ACPI. E.g. one is provided by: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/intel/int3472/tps68470.c on x86 ACPI systems using that sensor PMIC such as the "Microsoft Surface Go" and "Microsoft Surface Go 2" > Here is what I gathered online for the discussion leading to this patch: > ---------------------------------------------------------------------------------------------------------------------------------------------- > ClockInput resource added to ACPI v6.5: https://uefi.org/specs/ACPI/6.5/19_ASL_Reference.html#clockinput-clock-input-resource-descriptor-macro > - commit adding ClockInput resource to acpica: https://github.com/acpica/acpica/commit/661feab5ee01a34af95a389a18c82e79f1aba05a > - commit kernel upstream: 520d4a0ee5b6d9c7a1258ace6caa13a94ac35ef8 "ACPICA: add support > for ClockInput resource (v6.5)" Ah I see where the confusion is coming from, the clk-provider does not come directly from ACPI, it comes from the PMIC driver and the PMIC driver also adds a clk-lookup table entry to associate it with the PMIC. > this does not mean we can use it: I found this out-of-tree patch to supports fixed clock sources > https://github.com/niyas-sait/linux-acpi/blob/main/0001-acpi-add-clock-bindings-for-fixed-clock-resources.patch > it was not sent to the acpi mailing list. It was mentioned in this > dicussion: https://lore.kernel.org/linux-kernel/78763d69bae04204b2af37201b09f8b5@huawei.com/ > > Another interesting link: https://linaro.atlassian.net/wiki/spaces/CLIENTPC/pages/28822175758/ACPI+Clock+Input+Resources > ---------------------------------------------------------------------------------------------------------------------------------------------- > > link for the dicussion: https://lore.kernel.org/linux-media/20250220154909.152538-1-mehdi.djait@linux.intel.com/ These 2 links are not relevant, the clk-provider is not directly coming from ACPI instead the clk is registered by the PMIC driver for the clk-generator part of the PMIC. >> 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. > > So if I understood the above correctly: in the ov8865 IPU3/ACPI case: > > 1) sensor->extclk is NULL because the clock producer is not available in > ACPI systems. ClockInput() ACPI resource was introduced to acpica after > the ov8865 patch and the resource is not even being used in the upstream kernel. In this specific case it will be not NULL because the PMIC driver provides a clk-provider and creates a clk-lookup to match that to the ov8865 sensor. > 2) the sensor->extclk_rate will be set from reading > the clock-frequency _DSD property in: > > ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", &rate); This bit is correct, the special thing here is that the PMIC clk-provider is programmable so the sensor-driver needs to set it to the rate returned by reading "clock-frequency"/ So basically first call both: 1. devm_get_clk() 2. fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", &rate); and in the special case when *both* succeed do a clk_set_rate() call on the returned clk setting it to "rate". Regards, Hans
Hi Hans, On Tue, May 20, 2025 at 10:45:17AM +0200, Hans de Goede wrote: > On 14-May-25 10:25 AM, Mehdi Djait wrote: > > 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@linux.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: > > > > is it even possible to get a reference to the clock producer in ACPI > > systems or am I missing something here ? > > Yes in some special cases it is possible to get a reference to > a clock provider on ACPI. E.g. one is provided by: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/intel/int3472/tps68470.c > > on x86 ACPI systems using that sensor PMIC such as the > "Microsoft Surface Go" and "Microsoft Surface Go 2" > > > Here is what I gathered online for the discussion leading to this patch: > > ---------------------------------------------------------------------------------------------------------------------------------------------- > > ClockInput resource added to ACPI v6.5: https://uefi.org/specs/ACPI/6.5/19_ASL_Reference.html#clockinput-clock-input-resource-descriptor-macro > > - commit adding ClockInput resource to acpica: https://github.com/acpica/acpica/commit/661feab5ee01a34af95a389a18c82e79f1aba05a > > - commit kernel upstream: 520d4a0ee5b6d9c7a1258ace6caa13a94ac35ef8 "ACPICA: add support > > for ClockInput resource (v6.5)" > > Ah I see where the confusion is coming from, the clk-provider does not come > directly from ACPI, it comes from the PMIC driver and the PMIC driver also > adds a clk-lookup table entry to associate it with the PMIC. > > > this does not mean we can use it: I found this out-of-tree patch to supports fixed clock sources > > https://github.com/niyas-sait/linux-acpi/blob/main/0001-acpi-add-clock-bindings-for-fixed-clock-resources.patch > > it was not sent to the acpi mailing list. It was mentioned in this > > dicussion: https://lore.kernel.org/linux-kernel/78763d69bae04204b2af37201b09f8b5@huawei.com/ > > > > Another interesting link: https://linaro.atlassian.net/wiki/spaces/CLIENTPC/pages/28822175758/ACPI+Clock+Input+Resources > > ---------------------------------------------------------------------------------------------------------------------------------------------- > > > > link for the dicussion: https://lore.kernel.org/linux-media/20250220154909.152538-1-mehdi.djait@linux.intel.com/ > > These 2 links are not relevant, the clk-provider is not directly coming from > ACPI instead the clk is registered by the PMIC driver for the clk-generator > part of the PMIC. > > >> 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. > > > > So if I understood the above correctly: in the ov8865 IPU3/ACPI case: > > > > 1) sensor->extclk is NULL because the clock producer is not available in > > ACPI systems. ClockInput() ACPI resource was introduced to acpica after > > the ov8865 patch and the resource is not even being used in the upstream kernel. > > In this specific case it will be not NULL because the PMIC driver > provides a clk-provider and creates a clk-lookup to match that > to the ov8865 sensor. > > > 2) the sensor->extclk_rate will be set from reading > > the clock-frequency _DSD property in: > > > > ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", &rate); > > This bit is correct, the special thing here is that the PMIC > clk-provider is programmable so the sensor-driver needs to > set it to the rate returned by reading "clock-frequency"/ > > So basically first call both: > > 1. devm_get_clk() > 2. fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", &rate); > > and in the special case when *both* succeed do a clk_set_rate() > call on the returned clk setting it to "rate". An idea that struck me just now: if the ipu-bridge driver creates the clock-frequency swnode property, couldn't it also (or instead) set the clock frequency, in a similar way that the assigned-clock-rates property is handled in DT ? That would unify the ACPI and DT cases for sensor drivers. I don't know how difficult this would be to implement, but I see there's already a call to of_clk_set_defaults() in i2c_device_probe(), so maybe this could "just" be turned into a fwnode_clk_set_defaults() ? -- Regards, Laurent Pinchart
© 2016 - 2025 Red Hat, Inc.