Re: [PATCH 2/2] iio: frequency: adf4377: add clk provider support

["Nuno Sá" <noname.nuno@gmail.com>]

On Sat, 2025-11-15 at 17:24 +0000, Jonathan Cameron wrote:
> On Fri, 14 Nov 2025 12:09:08 +0000
> Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> 
> > Add clk provider feature for the adf4377.
> > 
> > Even though the driver was sent as an IIO driver in most cases the
> > device is actually seen as a clock provider.
> > 
> > This patch aims to cover actual usecases requested by users in order to
> > completely control the output frequencies from userspace.
> > 
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> 
> Given this new code is basically a clock driver, I'd expect to see some
> relevant folk +CC.
> 
> Added Michael, Stephen and linux-clk.
> 
> One question from me right at the end around whether it makes sense
> to register an IIO device with no channels.  I left the rest so it was
> easy for the people added to the thread to see all the code.
> 
> Thanks,
> 
> Jonathan
> 
> 
> > ---
> >  drivers/iio/frequency/adf4377.c | 131 +++++++++++++++++++++++++++++++-
> >  1 file changed, 129 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/frequency/adf4377.c b/drivers/iio/frequency/adf4377.c
> > index 08833b7035e4..08dc2110cf8c 100644
> > --- a/drivers/iio/frequency/adf4377.c
> > +++ b/drivers/iio/frequency/adf4377.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/bitfield.h>
> >  #include <linux/bits.h>
> >  #include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> >  #include <linux/clkdev.h>
> >  #include <linux/delay.h>
> >  #include <linux/device.h>
> > @@ -435,9 +436,14 @@ struct adf4377_state {
> >  	struct gpio_desc	*gpio_ce;
> >  	struct gpio_desc	*gpio_enclk1;
> >  	struct gpio_desc	*gpio_enclk2;
> > +	struct clk		*clk;
> > +	struct clk		*clkout;
> > +	struct clk_hw		hw;
> >  	u8			buf[2] __aligned(IIO_DMA_MINALIGN);
> >  };
> >  
> > +#define to_adf4377_state(h)	container_of(h, struct adf4377_state, hw)
> > +
> >  static const char * const adf4377_muxout_modes[] = {
> >  	[ADF4377_MUXOUT_HIGH_Z] = "high_z",
> >  	[ADF4377_MUXOUT_LKDET] = "lock_detect",
> > @@ -929,6 +935,120 @@ static int adf4377_freq_change(struct notifier_block *nb,
> > unsigned long action,
> >  	return NOTIFY_OK;
> >  }
> >  
> > +static void adf4377_clk_del_provider(void *data)
> > +{
> > +	struct adf4377_state *st = data;
> > +
> > +	of_clk_del_provider(st->spi->dev.of_node);
> > +}
> > +
> > +static unsigned long adf4377_clk_recalc_rate(struct clk_hw *hw,
> > +					      unsigned long parent_rate)
> > +{
> > +	struct adf4377_state *st = to_adf4377_state(hw);
> > +	u64 freq;
> > +	int ret;
> > +
> > +	ret = adf4377_get_freq(st, &freq);
> > +	if (ret)
> > +		return 0;
> > +
> > +	return freq;
> > +}
> > +
> > +static int adf4377_clk_set_rate(struct clk_hw *hw,
> > +				unsigned long rate,
> > +				unsigned long parent_rate)
> > +{
> > +	struct adf4377_state *st = to_adf4377_state(hw);
> > +
> > +	return adf4377_set_freq(st, rate);
> > +}
> > +
> > +static int adf4377_clk_prepare(struct clk_hw *hw)
> > +{
> > +	struct adf4377_state *st = to_adf4377_state(hw);
> > +
> > +	return regmap_update_bits(st->regmap, 0x1a, ADF4377_001A_PD_CLKOUT1_MSK
> > |
> > +				  ADF4377_001A_PD_CLKOUT2_MSK,
> > +				  FIELD_PREP(ADF4377_001A_PD_CLKOUT1_MSK, 0) |
> > +				  FIELD_PREP(ADF4377_001A_PD_CLKOUT2_MSK, 0));
> > +}
> > +
> > +static void adf4377_clk_unprepare(struct clk_hw *hw)
> > +{
> > +	struct adf4377_state *st = to_adf4377_state(hw);
> > +
> > +	regmap_update_bits(st->regmap, 0x1a, ADF4377_001A_PD_CLKOUT1_MSK |
> > +			   ADF4377_001A_PD_CLKOUT2_MSK,
> > +			   FIELD_PREP(ADF4377_001A_PD_CLKOUT1_MSK, 1) |
> > +			   FIELD_PREP(ADF4377_001A_PD_CLKOUT2_MSK, 1));
> > +}
> > +
> > +static int adf4377_clk_is_enabled(struct clk_hw *hw)
> > +{
> > +	struct adf4377_state *st = to_adf4377_state(hw);
> > +	unsigned int readval;
> > +	int ret;
> > +
> > +	ret = regmap_read(st->regmap, 0x1a, &readval);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return !(readval & (ADF4377_001A_PD_CLKOUT1_MSK |
> > ADF4377_001A_PD_CLKOUT2_MSK));
> > +}
> > +
> > +static const struct clk_ops adf4377_clk_ops = {
> > +	.recalc_rate = adf4377_clk_recalc_rate,
> > +	.set_rate = adf4377_clk_set_rate,
> > +	.prepare = adf4377_clk_prepare,
> > +	.unprepare = adf4377_clk_unprepare,
> > +	.is_enabled = adf4377_clk_is_enabled,
> > +};
> > +
> > +static int adf4377_clk_register(struct adf4377_state *st)
> > +{
> > +	struct spi_device *spi = st->spi;
> > +	struct clk_init_data init;
> > +	struct clk *clk;
> > +	const char *parent_name;
> > +	int ret;
> > +
> > +	if (!device_property_present(&spi->dev, "#clock-cells"))
> > +		return 0;
> > +
> > +	if (device_property_read_string(&spi->dev, "clock-output-names",
> > &init.name)) {
> > +		init.name = devm_kasprintf(&spi->dev, GFP_KERNEL, "%s-clk",
> > +					   fwnode_get_name(dev_fwnode(&spi-
> > >dev)));
> > +		if (!init.name)
> > +			return -ENOMEM;
> > +	}
> > +
> > +	parent_name = of_clk_get_parent_name(spi->dev.of_node, 0);
> > +	if (!parent_name)
> > +		return -EINVAL;
> > +
> > +	init.ops = &adf4377_clk_ops;
> > +	init.parent_names = &parent_name;
> > +	init.num_parents = 1;
> > +	init.flags = CLK_SET_RATE_PARENT;
> > +
> > +	st->hw.init = &init;
> > +	clk = devm_clk_register(&spi->dev, &st->hw);
> > +	if (IS_ERR(clk))
> > +		return PTR_ERR(clk);
> > +
> > +	st->clk = clk;
> > +
> > +	ret = of_clk_add_provider(spi->dev.of_node, of_clk_src_simple_get, clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	st->clkout = clk;
> > +
> > +	return devm_add_action_or_reset(&spi->dev, adf4377_clk_del_provider,
> > st);
> > +}
> > +
> >  static const struct adf4377_chip_info adf4377_chip_info = {
> >  	.name = "adf4377",
> >  	.has_gpio_enclk2 = true,
> > @@ -958,8 +1078,6 @@ static int adf4377_probe(struct spi_device *spi)
> >  
> >  	indio_dev->info = &adf4377_info;
> >  	indio_dev->name = "adf4377";
> > -	indio_dev->channels = adf4377_channels;
> > -	indio_dev->num_channels = ARRAY_SIZE(adf4377_channels);
> >  
> >  	st->regmap = regmap;
> >  	st->spi = spi;
> > @@ -979,6 +1097,15 @@ static int adf4377_probe(struct spi_device *spi)
> >  	if (ret)
> >  		return ret;
> >  
> > +	ret = adf4377_clk_register(st);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!st->clkout) {
> > +		indio_dev->channels = adf4377_channels;
> > +		indio_dev->num_channels = ARRAY_SIZE(adf4377_channels);
> 
> Why register a channel free iio device? Probably better to just not register
> it at all in this path.

Maybe a sneaky way of making use of the IIO debug direct access :). But I already
asked myself the somehow related question if we should still allow IIO access even if
the clock provider is registered. I mean, for sure we would need a more "fine"
grained access but we could only forbid IIO/userspace control in case an actual
consumer of the clock asks for it.

Or maybe not worth the trouble :)

- Nuno Sá

> 
> > +	}
> > +
> >  	return devm_iio_device_register(&spi->dev, indio_dev);
> >  }
> >