| Message ID | 20240607095806.3299-2-antoniu.miclaus@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,1/2] dt-bindings: iio: adf4350: add clk provider prop | expand |
On Fri, 2024-06-07 at 12:57 +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@analog.com> > --- > changes in v2: > - rework commit title > drivers/iio/frequency/adf4350.c | 129 ++++++++++++++++++++++++++++++++ > 1 file changed, 129 insertions(+) > > diff --git a/drivers/iio/frequency/adf4350.c b/drivers/iio/frequency/adf4350.c > index 4abf80f75ef5..1eb8bce71fe1 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> > @@ -31,11 +32,21 @@ enum { > ADF4350_PWRDOWN, > }; > > +struct adf4350_output { > + struct clk_hw hw; > + struct iio_dev *indio_dev; > +}; > + > +#define to_output(_hw) container_of(_hw, struct adf4350_output, hw) > + > struct adf4350_state { > struct spi_device *spi; > struct gpio_desc *lock_detect_gpiod; > struct adf4350_platform_data *pdata; > struct clk *clk; > + struct clk *clkout; > + const char *clk_out_name; > + struct adf4350_output output; > unsigned long clkin; > unsigned long chspc; /* Channel Spacing */ > unsigned long fpfd; /* Phase Frequency Detector */ > @@ -264,6 +275,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; > + } > ret = adf4350_set_freq(st, readin); > break; > case ADF4350_FREQ_REFIN: > @@ -381,6 +396,115 @@ static const struct iio_info adf4350_info = { > .debugfs_reg_access = &adf4350_reg_access, > }; > > +static void adf4350_clk_del_provider(void *data) > +{ > + struct adf4350_state *st = data; > + > + of_clk_del_provider(st->spi->dev.of_node); > +} > + > +static unsigned long adf4350_clk_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct iio_dev *indio_dev = to_output(hw)->indio_dev; > + struct adf4350_state *st = iio_priv(indio_dev); > + unsigned long long tmp; > + > + tmp = (u64)(st->r0_int * st->r1_mod + st->r0_fract) * st->fpfd; > + do_div(tmp, st->r1_mod * (1 << st->r4_rf_div_sel)); > + > + return tmp; > +} > + > +static int adf4350_clk_set_rate(struct clk_hw *hw, > + unsigned long rate, > + unsigned long parent_rate) > +{ > + struct iio_dev *indio_dev = to_output(hw)->indio_dev; > + struct adf4350_state *st = iio_priv(indio_dev); > + > + if (parent_rate == 0 || parent_rate > ADF4350_MAX_FREQ_REFIN) > + return -EINVAL; > + > + st->clkin = parent_rate; > + > + return adf4350_set_freq(st, rate); > +} > + > +static int adf4350_clk_prepare(struct clk_hw *hw) > +{ > + struct iio_dev *indio_dev = to_output(hw)->indio_dev; > + struct adf4350_state *st = iio_priv(indio_dev); > + > + st->regs[ADF4350_REG2] &= ~ADF4350_REG2_POWER_DOWN_EN; > + > + return adf4350_sync_config(st); > +} > + > +static void adf4350_clk_unprepare(struct clk_hw *hw) > +{ > + struct iio_dev *indio_dev = to_output(hw)->indio_dev; > + struct adf4350_state *st = iio_priv(indio_dev); > + > + st->regs[ADF4350_REG2] |= ADF4350_REG2_POWER_DOWN_EN; > + > + adf4350_sync_config(st); > +} > + > +static int adf4350_clk_is_enabled(struct clk_hw *hw) > +{ > + struct iio_dev *indio_dev = to_output(hw)->indio_dev; > + struct adf4350_state *st = iio_priv(indio_dev); > + > + return (st->regs[ADF4350_REG2] & ADF4350_REG2_POWER_DOWN_EN); > +} > + > +static const struct clk_ops adf4350_clk_ops = { > + .recalc_rate = adf4350_clk_recalc_rate, > + .set_rate = adf4350_clk_set_rate, > + .prepare = adf4350_clk_prepare, > + .unprepare = adf4350_clk_unprepare, > + .is_enabled = adf4350_clk_is_enabled, > +}; > + > +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? 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 :) - Nuno Sá
> > +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? On that note, given this is basically registering as a clock.I'd expect to see some clk related +CC Jonathan > > - Nuno Sá > >
On Fri, 7 Jun 2024 12:57:53 +0300 Antoniu Miclaus <antoniu.miclaus@analog.com> 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@analog.com> > --- > changes in v2: > - rework commit title > drivers/iio/frequency/adf4350.c | 129 ++++++++++++++++++++++++++++++++ > 1 file changed, 129 insertions(+) > > diff --git a/drivers/iio/frequency/adf4350.c b/drivers/iio/frequency/adf4350.c > index 4abf80f75ef5..1eb8bce71fe1 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> > @@ -31,11 +32,21 @@ enum { > ADF4350_PWRDOWN, > }; > > +struct adf4350_output { I can't see why this is needed. Looks to me like you only ever go from hw here to the state. Use container_of to do that. > + struct clk_hw hw; > + struct iio_dev *indio_dev; I wouldn't expect this clk_hw path to need the generic iio structure. I think you only did this to make the lookup of adf4350_state possible? If so, as above, container_of() is your friend. > +}; > + > +#define to_output(_hw) container_of(_hw, struct adf4350_output, hw) > + > struct adf4350_state { > struct spi_device *spi; > struct gpio_desc *lock_detect_gpiod; > struct adf4350_platform_data *pdata; > struct clk *clk; > + struct clk *clkout; > + const char *clk_out_name; > + struct adf4350_output output; > unsigned long clkin; > unsigned long chspc; /* Channel Spacing */ > unsigned long fpfd; /* Phase Frequency Detector */ > @@ -264,6 +275,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; > + } > ret = adf4350_set_freq(st, readin); > break; > case ADF4350_FREQ_REFIN: > @@ -381,6 +396,115 @@ static const struct iio_info adf4350_info = { > .debugfs_reg_access = &adf4350_reg_access, > }; > > +static void adf4350_clk_del_provider(void *data) > +{ > + struct adf4350_state *st = data; > + > + of_clk_del_provider(st->spi->dev.of_node); > +} > + > +static unsigned long adf4350_clk_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct iio_dev *indio_dev = to_output(hw)->indio_dev; You have the output in the state, so use container_of() to get to the containing structure. > + struct adf4350_state *st = iio_priv(indio_dev); > + unsigned long long tmp; > + > + tmp = (u64)(st->r0_int * st->r1_mod + st->r0_fract) * st->fpfd; > + do_div(tmp, st->r1_mod * (1 << st->r4_rf_div_sel)); > + > + return tmp; > +}
> -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Saturday, June 8, 2024 5:09 PM > To: Nuno Sá <noname.nuno@gmail.com> > Cc: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>; Lars-Peter Clausen > <lars@metafoo.de>; Hennerich, Michael <Michael.Hennerich@analog.com>; > Rob Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; > Conor Dooley <conor+dt@kernel.org>; linux-iio@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org > 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á > > > >
diff --git a/drivers/iio/frequency/adf4350.c b/drivers/iio/frequency/adf4350.c index 4abf80f75ef5..1eb8bce71fe1 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> @@ -31,11 +32,21 @@ enum { ADF4350_PWRDOWN, }; +struct adf4350_output { + struct clk_hw hw; + struct iio_dev *indio_dev; +}; + +#define to_output(_hw) container_of(_hw, struct adf4350_output, hw) + struct adf4350_state { struct spi_device *spi; struct gpio_desc *lock_detect_gpiod; struct adf4350_platform_data *pdata; struct clk *clk; + struct clk *clkout; + const char *clk_out_name; + struct adf4350_output output; unsigned long clkin; unsigned long chspc; /* Channel Spacing */ unsigned long fpfd; /* Phase Frequency Detector */ @@ -264,6 +275,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; + } ret = adf4350_set_freq(st, readin); break; case ADF4350_FREQ_REFIN: @@ -381,6 +396,115 @@ static const struct iio_info adf4350_info = { .debugfs_reg_access = &adf4350_reg_access, }; +static void adf4350_clk_del_provider(void *data) +{ + struct adf4350_state *st = data; + + of_clk_del_provider(st->spi->dev.of_node); +} + +static unsigned long adf4350_clk_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + struct iio_dev *indio_dev = to_output(hw)->indio_dev; + struct adf4350_state *st = iio_priv(indio_dev); + unsigned long long tmp; + + tmp = (u64)(st->r0_int * st->r1_mod + st->r0_fract) * st->fpfd; + do_div(tmp, st->r1_mod * (1 << st->r4_rf_div_sel)); + + return tmp; +} + +static int adf4350_clk_set_rate(struct clk_hw *hw, + unsigned long rate, + unsigned long parent_rate) +{ + struct iio_dev *indio_dev = to_output(hw)->indio_dev; + struct adf4350_state *st = iio_priv(indio_dev); + + if (parent_rate == 0 || parent_rate > ADF4350_MAX_FREQ_REFIN) + return -EINVAL; + + st->clkin = parent_rate; + + return adf4350_set_freq(st, rate); +} + +static int adf4350_clk_prepare(struct clk_hw *hw) +{ + struct iio_dev *indio_dev = to_output(hw)->indio_dev; + struct adf4350_state *st = iio_priv(indio_dev); + + st->regs[ADF4350_REG2] &= ~ADF4350_REG2_POWER_DOWN_EN; + + return adf4350_sync_config(st); +} + +static void adf4350_clk_unprepare(struct clk_hw *hw) +{ + struct iio_dev *indio_dev = to_output(hw)->indio_dev; + struct adf4350_state *st = iio_priv(indio_dev); + + st->regs[ADF4350_REG2] |= ADF4350_REG2_POWER_DOWN_EN; + + adf4350_sync_config(st); +} + +static int adf4350_clk_is_enabled(struct clk_hw *hw) +{ + struct iio_dev *indio_dev = to_output(hw)->indio_dev; + struct adf4350_state *st = iio_priv(indio_dev); + + return (st->regs[ADF4350_REG2] & ADF4350_REG2_POWER_DOWN_EN); +} + +static const struct clk_ops adf4350_clk_ops = { + .recalc_rate = adf4350_clk_recalc_rate, + .set_rate = adf4350_clk_set_rate, + .prepare = adf4350_clk_prepare, + .unprepare = adf4350_clk_unprepare, + .is_enabled = adf4350_clk_is_enabled, +}; + +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; + + st->clkout = clk; + + return devm_add_action_or_reset(&spi->dev, adf4350_clk_del_provider, st); +} + static struct adf4350_platform_data *adf4350_parse_dt(struct device *dev) { struct adf4350_platform_data *pdata; @@ -524,6 +648,7 @@ static int adf4350_probe(struct spi_device *spi) indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->channels = &adf4350_chan; indio_dev->num_channels = 1; + st->output.indio_dev = indio_dev; mutex_init(&st->lock); @@ -551,6 +676,10 @@ static int adf4350_probe(struct spi_device *spi) return ret; } + ret = adf4350_clk_register(st); + if (ret) + return ret; + ret = devm_add_action_or_reset(&spi->dev, adf4350_power_down, indio_dev); if (ret) return dev_err_probe(&spi->dev, ret,
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@analog.com> --- changes in v2: - rework commit title drivers/iio/frequency/adf4350.c | 129 ++++++++++++++++++++++++++++++++ 1 file changed, 129 insertions(+)