RE: [PATCH v2 2/2] iio: frequency: adf4350: add clk provider

[Hennerich, Michael]

> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Saturday, June 8, 2024 5:09 PM
> To: Nuno Sá <noname.nuno@xxxxxxxxx>
> Cc: Miclaus, Antoniu <Antoniu.Miclaus@xxxxxxxxxx>; Lars-Peter Clausen
> <lars@xxxxxxxxxx>; Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>;
> Rob Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>;
> Conor Dooley <conor+dt@xxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 2/2] iio: frequency: adf4350: add clk provider
> 
> [External]
> 
> 
> > > +static int adf4350_clk_register(struct adf4350_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;
> > > +
> > > +	init.name = devm_kasprintf(&spi->dev, GFP_KERNEL, "%s-clk",
> > > +				   fwnode_get_name(dev_fwnode(&spi-
> >dev)));
> > > +	device_property_read_string(&spi->dev, "clock-output-names",
> > > +				    &init.name);
> > > +
> > > +	parent_name = of_clk_get_parent_name(spi->dev.of_node, 0);
> > > +	if (!parent_name)
> > > +		return -EINVAL;
> > > +
> > > +	init.ops = &adf4350_clk_ops;
> > > +	init.parent_names = &parent_name;
> > > +	init.num_parents = 1;
> > > +
> > > +	st->output.hw.init = &init;
> > > +	clk = devm_clk_register(&spi->dev, &st->output.hw);
> > > +	if (IS_ERR(clk))
> > > +		return PTR_ERR(clk);
> > > +
> > > +	ret = of_clk_add_provider(spi->dev.of_node, of_clk_src_simple_get,
> > > clk);
> > > +	if (ret)
> > > +		return ret;
> > > +
> >
> > I totally agree this chip should be a clock provider (maybe it should
> > even in drivers/clk from the beginning) but there's one thing that comes to
> my mind.
> > Should we still expose the IIO userspace interface in case we register
> > it as a clock provider?
> 
> That's a reasonable suggestion.  If it's wired up as a clock we probably don't
> want to provide userspace controls via IIO.
> 
> >
> > Sure, we do have clk notifiers in the kernel but I still think it's a
> > sensible question :)
> >
> > I suspect we have another "outliers" in drivers/iio/frequency :)
> 
> We all love the blurry boundaries in the kernel. IIRC when these were
> orginally proposed, the IIO thing was mostly about how they were being
> used in Software Defined Radios where the were part of the modulator
> circuits.
> I can't remember the exact reasoning though.
> 
> As to the right answer for these today, I don't have strong feelings on it and
> almost all these parts are ADI ones.
> 
> Michael, this is one of yours, so what do you think?

Well, I think the situation hasn't changed much.
There are users which want to control the LO frequency using a user space API.
And just like you remembered, as an IF for an external mixer, etc.
Very similar to a bench top signal generator.
And there is no user space API for CCF. So IIO provides this API.
On the other side there are users who want to use such synthesizers as regular
In kernel clock providers, too

We could add a generic CCF to IIO proxy/bridge, however these
devices which sit in IIO also have additional functionality which isn't abstracted in the CCF.

It sounds sensible that when registering a clock provider, writing IIO_ALTVOLTAGE raw should return EBUSY.
Also, when it's a CCF clock consumer setting the refin_frequency should return error.
 
-Michael

> 
> On that note, given this is basically registering as a clock.I'd expect to see
> some clk related +CC
> 
> Jonathan
> 
> >
> > - Nuno Sá
> >
> >