Add support for the AD7124's internal clock output. If the #clock-cells
property is present, turn on the internal clock output during probe.
If both the clocks and #clock-names properties are present (not allowed
by devicetree bindings), assume that an external clock is being used so
that we don't accidentally have two outputs fighting each other.
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
We could make this fancier and only turn on the output on demand of a
clock consumer, but then we have to deal with locking of the SPI bus
to be able to write to the register. So I opted for the simpler
solution of always turning it on during probe. This would only be used
for synchronizing with other similar ADCs, so implementing the functions
for a more general-purpose clock seems a bit overkill.
---
drivers/iio/adc/ad7124.c | 37 ++++++++++++++++++++++++++++++++++---
1 file changed, 34 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 4112c484c2371cfa6f26acb0d7c5b2a308a5fb35..a1c5f059b284c4f8797986628b92b70fd84e90f4 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -7,6 +7,7 @@
#include <linux/bitfield.h>
#include <linux/bitops.h>
#include <linux/clk.h>
+#include <linux/clk-provider.h>
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/err.h>
@@ -18,6 +19,7 @@
#include <linux/property.h>
#include <linux/regulator/consumer.h>
#include <linux/spi/spi.h>
+#include <linux/sprintf.h>
#include <linux/units.h>
#include <linux/iio/iio.h>
@@ -125,10 +127,12 @@ static const unsigned int ad7124_reg_size[] = {
3, 3, 3, 3, 3
};
+#define AD7124_INT_CLK_HZ 614400
+
static const int ad7124_master_clk_freq_hz[3] = {
- [AD7124_LOW_POWER] = 76800,
- [AD7124_MID_POWER] = 153600,
- [AD7124_FULL_POWER] = 614400,
+ [AD7124_LOW_POWER] = AD7124_INT_CLK_HZ / 8,
+ [AD7124_MID_POWER] = AD7124_INT_CLK_HZ / 4,
+ [AD7124_FULL_POWER] = AD7124_INT_CLK_HZ,
};
static const char * const ad7124_ref_names[] = {
@@ -1164,6 +1168,33 @@ static int ad7124_setup(struct ad7124_state *st)
}
clk_sel = AD7124_ADC_CONTROL_CLK_SEL_INT;
+ } else if (!device_property_present(dev, "clocks") &&
+ device_property_present(dev, "clock-names")) {
+ struct clk_hw *clk_hw;
+
+ const char *name __free(kfree) = kasprintf(GFP_KERNEL, "%s-clk",
+ fwnode_get_name(dev_fwnode(dev)));
+ if (!name)
+ return -ENOMEM;
+
+ clk_hw = devm_clk_hw_register_fixed_rate(dev, name, NULL, 0,
+ AD7124_INT_CLK_HZ);
+ if (IS_ERR(clk_hw))
+ return dev_err_probe(dev, PTR_ERR(clk_hw),
+ "Failed to register clock provider\n");
+
+ ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
+ clk_hw);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to add clock provider\n");
+
+ /*
+ * Treat the clock as always on. This way we don't have to deal
+ * with someone trying to enable/disable the clock while we are
+ * reading samples.
+ */
+ clk_sel = AD7124_ADC_CONTROL_CLK_SEL_INT_OUT;
} else {
struct clk *clk;
--
2.43.0On 8/25/25 5:55 PM, David Lechner wrote:
> Add support for the AD7124's internal clock output. If the #clock-cells
> property is present, turn on the internal clock output during probe.
>
...
> @@ -1164,6 +1168,33 @@ static int ad7124_setup(struct ad7124_state *st)
> }
>
> clk_sel = AD7124_ADC_CONTROL_CLK_SEL_INT;
> + } else if (!device_property_present(dev, "clocks") &&
> + device_property_present(dev, "clock-names")) {
Found a mistake here. This should be #clock-cells rather than
clock-names.
Hi David, kernel test robot noticed the following build errors: [auto build test ERROR on 91812d3843409c235f336f32f1c37ddc790f1e03] url: https://github.com/intel-lab-lkp/linux/commits/David-Lechner/dt-bindings-iio-adc-adi-ad7124-fix-clocks-properties/20250826-065924 base: 91812d3843409c235f336f32f1c37ddc790f1e03 patch link: https://lore.kernel.org/r/20250825-iio-adc-ad7124-proper-clock-support-v2-4-4dcff9db6b35%40baylibre.com patch subject: [PATCH v2 4/4] iio: adc: ad7124: add clock output support config: sparc-randconfig-002-20250826 (https://download.01.org/0day-ci/archive/20250826/202508261731.33GQLUPx-lkp@intel.com/config) compiler: sparc64-linux-gcc (GCC) 8.5.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250826/202508261731.33GQLUPx-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202508261731.33GQLUPx-lkp@intel.com/ All errors (new ones prefixed by >>, old ones prefixed by <<): >> ERROR: modpost: "__clk_hw_register_fixed_rate" [drivers/iio/adc/ad7124.ko] undefined! >> ERROR: modpost: "of_clk_hw_simple_get" [drivers/iio/adc/ad7124.ko] undefined! >> ERROR: modpost: "devm_of_clk_add_hw_provider" [drivers/iio/adc/ad7124.ko] undefined! -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
On Tue, Aug 26, 2025 at 1:55 AM David Lechner <dlechner@baylibre.com> wrote:
>
> Add support for the AD7124's internal clock output. If the #clock-cells
> property is present, turn on the internal clock output during probe.
>
> If both the clocks and #clock-names properties are present (not allowed
> by devicetree bindings), assume that an external clock is being used so
> that we don't accidentally have two outputs fighting each other.
...
> static const int ad7124_master_clk_freq_hz[3] = {
> - [AD7124_LOW_POWER] = 76800,
> - [AD7124_MID_POWER] = 153600,
> - [AD7124_FULL_POWER] = 614400,
> + [AD7124_LOW_POWER] = AD7124_INT_CLK_HZ / 8,
> + [AD7124_MID_POWER] = AD7124_INT_CLK_HZ / 4,
> + [AD7124_FULL_POWER] = AD7124_INT_CLK_HZ,
Perhaps / 1 ?
> };
...
> + const char *name __free(kfree) = kasprintf(GFP_KERNEL, "%s-clk",
> + fwnode_get_name(dev_fwnode(dev)));
What's wrong with the %pfwP specifier?
> + if (!name)
> + return -ENOMEM;
--
With Best Regards,
Andy Shevchenko
On 8/26/25 3:13 AM, Andy Shevchenko wrote:
> On Tue, Aug 26, 2025 at 1:55 AM David Lechner <dlechner@baylibre.com> wrote:
>>
>> Add support for the AD7124's internal clock output. If the #clock-cells
>> property is present, turn on the internal clock output during probe.
>>
>> If both the clocks and #clock-names properties are present (not allowed
>> by devicetree bindings), assume that an external clock is being used so
>> that we don't accidentally have two outputs fighting each other.
>
> ...
>
>> static const int ad7124_master_clk_freq_hz[3] = {
>> - [AD7124_LOW_POWER] = 76800,
>> - [AD7124_MID_POWER] = 153600,
>> - [AD7124_FULL_POWER] = 614400,
>> + [AD7124_LOW_POWER] = AD7124_INT_CLK_HZ / 8,
>> + [AD7124_MID_POWER] = AD7124_INT_CLK_HZ / 4,
>> + [AD7124_FULL_POWER] = AD7124_INT_CLK_HZ,
>
> Perhaps / 1 ?
Seems redundant.
>
>> };
>
> ...
>
>> + const char *name __free(kfree) = kasprintf(GFP_KERNEL, "%s-clk",
>> + fwnode_get_name(dev_fwnode(dev)));
>
> What's wrong with the %pfwP specifier?
I didn't know about it.
>
>> + if (!name)
>> + return -ENOMEM;
>
On Tue, Aug 26, 2025 at 10:11:09AM -0500, David Lechner wrote:
> On 8/26/25 3:13 AM, Andy Shevchenko wrote:
> > On Tue, Aug 26, 2025 at 1:55 AM David Lechner <dlechner@baylibre.com> wrote:
...
> >> static const int ad7124_master_clk_freq_hz[3] = {
> >> - [AD7124_LOW_POWER] = 76800,
> >> - [AD7124_MID_POWER] = 153600,
> >> - [AD7124_FULL_POWER] = 614400,
> >> + [AD7124_LOW_POWER] = AD7124_INT_CLK_HZ / 8,
> >> + [AD7124_MID_POWER] = AD7124_INT_CLK_HZ / 4,
> >> + [AD7124_FULL_POWER] = AD7124_INT_CLK_HZ,
> >
> > Perhaps / 1 ?
>
> Seems redundant.
Yes and no. I think that it makes sense to put for the consistency as it makes
reader aware of the divisors. 1 is also valid divisor after all.
--
With Best Regards,
Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.