On Wed, Jun 12, 2024 at 01:54:07PM +0200, Nuno Sá wrote:
> Hi Antonium
>
> On Wed, 2024-06-12 at 13:45 +0300, Antoniu Miclaus wrote:
> > Add clk provider feature for the adf4350.
> >
> > 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@xxxxxxxxxx>
> > ---
> > changes in v3:
> > - use container_of to directly access the adf4350_state structure.
> > drivers/iio/frequency/adf4350.c | 118 ++++++++++++++++++++++++++++++++
> > 1 file changed, 118 insertions(+)
> >
> > diff --git a/drivers/iio/frequency/adf4350.c b/drivers/iio/frequency/adf4350.c
> > index 4abf80f75ef5..f716f744baa9 100644
> > --- a/drivers/iio/frequency/adf4350.c
> > +++ b/drivers/iio/frequency/adf4350.c
> > @@ -19,6 +19,7 @@
> > #include <linux/gpio/consumer.h>
> > #include <asm/div64.h>
> > #include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> >
> > #include <linux/iio/iio.h>
> > #include <linux/iio/sysfs.h>
> > @@ -36,6 +37,9 @@ struct adf4350_state {
> > struct gpio_desc *lock_detect_gpiod;
> > struct adf4350_platform_data *pdata;
> > struct clk *clk;
> > + struct clk *clkout;
> > + const char *clk_out_name;
> > + struct clk_hw hw;
> > unsigned long clkin;
> > unsigned long chspc; /* Channel Spacing */
> > unsigned long fpfd; /* Phase Frequency Detector */
> > @@ -61,6 +65,8 @@ struct adf4350_state {
> > __be32 val __aligned(IIO_DMA_MINALIGN);
> > };
> >
> > +#define to_state(_hw) container_of(_hw, struct adf4350_state, hw)
> > +
>
> nit: to_adf4350_state() would be neater...
>
> > static struct adf4350_platform_data default_pdata = {
> > .channel_spacing = 10000,
> > .r2_user_settings = ADF4350_REG2_PD_POLARITY_POS |
> > @@ -264,6 +270,10 @@ static ssize_t adf4350_write(struct iio_dev *indio_dev,
> > mutex_lock(&st->lock);
> > switch ((u32)private) {
> > case ADF4350_FREQ:
> > + if (st->clkout) {
> > + ret = clk_set_rate(st->clkout, readin);
> > + break;
> > + }
>
> So, apparently you forgot or decided otherwise to not go with the suggestion of
> not including the IIO interface (at least he channel one - debugfs could be
> maintained I guess) or with the more in the middle approach Michael suggested.
> Just not allowing ADF4350_FREQ and ADF4350_FREQ_REFIN.
>
> Hence, I would expect at least some justification to keep the above in your v3
> changelog. Also note that keeping ADF4350_FREQ_REFIN while being a clock
> provider seems pointless and maybe even be wrong (as the clock framework should
> take care of the parent clock). This also brings another question... see below
>
I think also there was a request to include the clk list and
maintainers. It's possible that Stephen might want something like the
auxbus to be used here so that the clock code ends up in the clock
subsystem.
Thanks,
Conor.
Attachment:
signature.asc
Description: PGP signature
