| Message ID | 20240612104554.66851-2-antoniu.miclaus@analog.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/2] dt-bindings: iio: adf4350: add clk provider prop | expand |
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@analog.com> > --- > 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 ... > > +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; Shouldn't we set CLK_SET_RATE_PARENT in init.flags? - Nuno Sá
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@analog.com> > > --- > > 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.
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) + 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; + } ret = adf4350_set_freq(st, readin); break; case ADF4350_FREQ_REFIN: @@ -381,6 +391,110 @@ 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 adf4350_state *st = to_state(hw); + 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 adf4350_state *st = to_state(hw); + + 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 adf4350_state *st = to_state(hw); + + st->regs[ADF4350_REG2] &= ~ADF4350_REG2_POWER_DOWN_EN; + + return adf4350_sync_config(st); +} + +static void adf4350_clk_unprepare(struct clk_hw *hw) +{ + struct adf4350_state *st = to_state(hw); + + st->regs[ADF4350_REG2] |= ADF4350_REG2_POWER_DOWN_EN; + + adf4350_sync_config(st); +} + +static int adf4350_clk_is_enabled(struct clk_hw *hw) +{ + struct adf4350_state *st = to_state(hw); + + 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->hw.init = &init; + clk = devm_clk_register(&spi->dev, &st->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; @@ -551,6 +665,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 v3: - use container_of to directly access the adf4350_state structure. drivers/iio/frequency/adf4350.c | 118 ++++++++++++++++++++++++++++++++ 1 file changed, 118 insertions(+)