On Sat, 2024-04-20 at 16:19 +0100, Jonathan Cameron wrote: > On Fri, 19 Apr 2024 17:36:50 +0200 > Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@xxxxxxxxxx> wrote: > > > From: Nuno Sa <nuno.sa@xxxxxxxxxx> > > > > Since we allow to change the sampling frequency and do it with > > clk_round_rate(), we can cache it and use on the read_raw() interface. > > This will also be useful in a following patch supporting interface > > calibration. > > > > Signed-off-by: Nuno Sa <nuno.sa@xxxxxxxxxx> > > The clk subsystem caches the clock rate in most cases anyway, so > I'm not sure why we need this. Or it the point that you are going > to temporarily change it in the next patch? > The idea is that in the next patch I want to bail out early if we're not changing the rate (after clk_round_rate()) because I also don't want to re-run tuning in that case. Since the rate is not guaranteed to be cached (even though I think most clock providers don't set the NO_CACHE flag), I went this way. Anyways, this is minor so I can stick to clk_get_rate() if you prefer. > Patch looks fine, but I think a clearer requirements statement is > needed. > > Jonathan > > > > --- > > drivers/iio/adc/ad9467.c | 13 +++++++++++-- > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c > > index 7475ec2a56c72..7db87ccc1ea4b 100644 > > --- a/drivers/iio/adc/ad9467.c > > +++ b/drivers/iio/adc/ad9467.c > > @@ -117,6 +117,7 @@ struct ad9467_state { > > struct iio_backend *back; > > struct spi_device *spi; > > struct clk *clk; > > + unsigned long sample_rate; > > unsigned int output_mode; > > unsigned int (*scales)[2]; > > > > @@ -331,7 +332,7 @@ static int ad9467_read_raw(struct iio_dev *indio_dev, > > case IIO_CHAN_INFO_SCALE: > > return ad9467_get_scale(st, val, val2); > > case IIO_CHAN_INFO_SAMP_FREQ: > > - *val = clk_get_rate(st->clk); > > + *val = st->sample_rate; > > > > return IIO_VAL_INT; > > default: > > @@ -346,6 +347,7 @@ static int ad9467_write_raw(struct iio_dev *indio_dev, > > struct ad9467_state *st = iio_priv(indio_dev); > > const struct ad9467_chip_info *info = st->info; > > long r_clk; > > + int ret; > > > > switch (mask) { > > case IIO_CHAN_INFO_SCALE: > > @@ -358,7 +360,12 @@ static int ad9467_write_raw(struct iio_dev *indio_dev, > > return -EINVAL; > > } > > > > - return clk_set_rate(st->clk, r_clk); > > + ret = clk_set_rate(st->clk, r_clk); > > + if (ret) > > + return ret; > > + > > + st->sample_rate = r_clk; > > + return 0; > > default: > > return -EINVAL; > > } > > @@ -543,6 +550,8 @@ static int ad9467_probe(struct spi_device *spi) > > if (IS_ERR(st->clk)) > > return PTR_ERR(st->clk); > > > > + st->sample_rate = clk_get_rate(st->clk); > > + > > st->pwrdown_gpio = devm_gpiod_get_optional(&spi->dev, "powerdown", > > GPIOD_OUT_LOW); > > if (IS_ERR(st->pwrdown_gpio)) > > >