Re: [PATCH 7/8] iio: adc: ad9467: cache the sample rate

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 22 Apr 2024 17:46:02 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> 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@kernel.org> wrote:
> >   
> > > From: Nuno Sa <nuno.sa@analog.com>
> > > 
> > > 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@analog.com>  
> > 
> > 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.

I'd make it local by retrieving current clock just before you consider writing
it.  We can optimize any need to cache it later.

Jonathan

> 
> > 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))
> > >   
> >   
>