Re: [PATCH v2 8/8] iio: adc: adi-axi-adc: convert to regmap

["Nuno Sá" <noname.nuno@gmail.com>]

On Mon, 2023-12-11 at 19:16 +0000, Jonathan Cameron wrote:
> On Mon, 11 Dec 2023 14:18:43 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Thu, 2023-12-07 at 13:39 +0100, Nuno Sa via B4 Relay wrote:
> > > From: Nuno Sa <nuno.sa@analog.com>
> > > 
> > > Use MMIO regmap interface. It makes things easier for manipulating bits.
> > > 
> > > Reviewed-by: David Lechner <dlechner@baylibre.com>
> > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > ---
> > >  drivers/iio/adc/adi-axi-adc.c | 85 ++++++++++++++++++++++++++------------
> > > ----
> > > -
> > >  1 file changed, 52 insertions(+), 33 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> > > index ae83ada7f9f2..c247ff1541d2 100644
> > > --- a/drivers/iio/adc/adi-axi-adc.c
> > > +++ b/drivers/iio/adc/adi-axi-adc.c
> > > @@ -14,6 +14,7 @@
> > >  #include <linux/of.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/property.h>
> > > +#include <linux/regmap.h>
> > >  #include <linux/slab.h>
> > >  
> > >  #include <linux/iio/iio.h>
> > > @@ -62,7 +63,7 @@ struct adi_axi_adc_state {
> > >  	struct mutex				lock;
> > >  
> > >  	struct adi_axi_adc_client		*client;
> > > -	void __iomem				*regs;
> > > +	struct regmap				*regmap;
> > >  };
> > >  
> > >  struct adi_axi_adc_client {
> > > @@ -90,19 +91,6 @@ void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv
> > > *conv)
> > >  }
> > >  EXPORT_SYMBOL_NS_GPL(adi_axi_adc_conv_priv, IIO_ADI_AXI);
> > >  
> > > -static void adi_axi_adc_write(struct adi_axi_adc_state *st,
> > > -			      unsigned int reg,
> > > -			      unsigned int val)
> > > -{
> > > -	iowrite32(val, st->regs + reg);
> > > -}
> > > -
> > > -static unsigned int adi_axi_adc_read(struct adi_axi_adc_state *st,
> > > -				     unsigned int reg)
> > > -{
> > > -	return ioread32(st->regs + reg);
> > > -}
> > > -
> > >  static int adi_axi_adc_config_dma_buffer(struct device *dev,
> > >  					 struct iio_dev *indio_dev)
> > >  {
> > > @@ -163,17 +151,20 @@ static int adi_axi_adc_update_scan_mode(struct
> > > iio_dev
> > > *indio_dev,
> > >  {
> > >  	struct adi_axi_adc_state *st = iio_priv(indio_dev);
> > >  	struct adi_axi_adc_conv *conv = &st->client->conv;
> > > -	unsigned int i, ctrl;
> > > +	unsigned int i;
> > > +	int ret;
> > >  
> > >  	for (i = 0; i < conv->chip_info->num_channels; i++) {
> > > -		ctrl = adi_axi_adc_read(st, ADI_AXI_REG_CHAN_CTRL(i));
> > > -
> > >  		if (test_bit(i, scan_mask))
> > > -			ctrl |= ADI_AXI_REG_CHAN_CTRL_ENABLE;
> > > +			ret = regmap_set_bits(st->regmap,
> > > +					      ADI_AXI_REG_CHAN_CTRL(i),
> > > +					     
> > > ADI_AXI_REG_CHAN_CTRL_ENABLE);
> > >  		else
> > > -			ctrl &= ~ADI_AXI_REG_CHAN_CTRL_ENABLE;
> > > -
> > > -		adi_axi_adc_write(st, ADI_AXI_REG_CHAN_CTRL(i), ctrl);
> > > +			ret = regmap_clear_bits(st->regmap,
> > > +						ADI_AXI_REG_CHAN_CTRL(i),
> > > +						ADI_AXI_REG_CHAN_CTRL_ENA
> > > BLE)
> > > ;
> > > +		if (ret)
> > > +			return ret;
> > >  	}
> > >  
> > >  	return 0;
> > > @@ -310,21 +301,32 @@ static int adi_axi_adc_setup_channels(struct device
> > > *dev,
> > >  	}
> > >  
> > >  	for (i = 0; i < conv->chip_info->num_channels; i++) {
> > > -		adi_axi_adc_write(st, ADI_AXI_REG_CHAN_CTRL(i),
> > > -				  ADI_AXI_REG_CHAN_CTRL_DEFAULTS);
> > > +		ret = regmap_write(st->regmap, ADI_AXI_REG_CHAN_CTRL(i),
> > > +				   ADI_AXI_REG_CHAN_CTRL_DEFAULTS);
> > > +		if (ret)
> > > +			return ret;
> > >  	}
> > >  
> > >  	return 0;
> > >  }
> > >  
> > > -static void axi_adc_reset(struct adi_axi_adc_state *st)
> > > +static int axi_adc_reset(struct adi_axi_adc_state *st)
> > >  {
> > > -	adi_axi_adc_write(st, ADI_AXI_REG_RSTN, 0);
> > > +	int ret;
> > > +
> > > +	ret = regmap_write(st->regmap, ADI_AXI_REG_RSTN, 0);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	mdelay(10);
> > > -	adi_axi_adc_write(st, ADI_AXI_REG_RSTN,
> > > ADI_AXI_REG_RSTN_MMCM_RSTN);
> > > +	ret = regmap_write(st->regmap, ADI_AXI_REG_RSTN,
> > > +			   ADI_AXI_REG_RSTN_MMCM_RSTN);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	mdelay(10);
> > > -	adi_axi_adc_write(st, ADI_AXI_REG_RSTN,
> > > -			  ADI_AXI_REG_RSTN_RSTN |
> > > ADI_AXI_REG_RSTN_MMCM_RSTN);
> > > +	return regmap_write(st->regmap, ADI_AXI_REG_RSTN,
> > > +			    ADI_AXI_REG_RSTN_RSTN |
> > > ADI_AXI_REG_RSTN_MMCM_RSTN);
> > >  }
> > >  
> > >  static void adi_axi_adc_cleanup(void *data)
> > > @@ -335,12 +337,20 @@ static void adi_axi_adc_cleanup(void *data)
> > >  	module_put(cl->dev->driver->owner);
> > >  }
> > >  
> > > +static const struct regmap_config axi_adc_regmap_config = {
> > > +	.val_bits = 32,
> > > +	.reg_bits = 32,
> > > +	.reg_stride = 4,
> > > +	.max_register = 0x0800,
> > > +};
> > > +
> > >  static int adi_axi_adc_probe(struct platform_device *pdev)
> > >  {
> > >  	struct adi_axi_adc_conv *conv;
> > >  	struct iio_dev *indio_dev;
> > >  	struct adi_axi_adc_client *cl;
> > >  	struct adi_axi_adc_state *st;
> > > +	void __iomem *base;
> > >  	unsigned int ver;
> > >  	int ret;
> > >  
> > > @@ -361,15 +371,24 @@ static int adi_axi_adc_probe(struct platform_device
> > > *pdev)
> > >  	cl->state = st;
> > >  	mutex_init(&st->lock);
> > >  
> > > -	st->regs = devm_platform_ioremap_resource(pdev, 0);
> > > -	if (IS_ERR(st->regs))
> > > -		return PTR_ERR(st->regs);
> > > +	base = devm_platform_ioremap_resource(pdev, 0);
> > > +	if (IS_ERR(base))
> > > +		return PTR_ERR(base);
> > > +
> > > +	st->regmap = devm_regmap_init_mmio(&pdev->dev, base,
> > > +					   &axi_adc_regmap_config);
> > > +	if (IS_ERR(st->regmap))
> > > +		return PTR_ERR(st->regmap);
> > >  
> > >  	conv = &st->client->conv;
> > >  
> > > -	axi_adc_reset(st);
> > > +	ret = axi_adc_reset(st);
> > > +	if (ret)
> > > +		return ret;
> > >  
> > > -	ver = adi_axi_adc_read(st, ADI_AXI_REG_VERSION);
> > > +	ret = regmap_read(st->regmap, ADI_AXI_REG_VERSION, &ver);
> > > +	if (ret)
> > > +		return ret;
> > >  
> > >  	if (cl->info->version > ver) {
> > >  		dev_err(&pdev->dev,
> > >   
> > 
> > Hi Jonathan,
> > 
> > I'm not seeing this series yet applied in the togreg branch so maybe I'm
> > still
> > on time. This patch is missing the proper Kconfig change:
> 
> > 
> > 
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index af56df63beff..10e0e340cdae 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -292,7 +292,7 @@ config ADI_AXI_ADC
> >         select IIO_BUFFER
> >         select IIO_BUFFER_HW_CONSUMER
> >         select IIO_BUFFER_DMAENGINE
> > -       depends on HAS_IOMEM
> > +       select REGMAP_MMIO
> >         depends on OF
> >         help
> >           Say yes here to build support for Analog Devices Generic
> > 
> > 
> > Just realized this when working on the v3 for IIO backends. Please let me
> > know
> > if I should send a follow up patch or if I can still send a v3 on the
> > "iio: ad9467 and axi-adc cleanups" series.
> Oops. I forgot to push out yesterday.  Anyhow, would still have been fine to
> fix
> this as always about a week before I push out as togreg unless we are near
> the merge window.
> 
> Fixed up.
> Jonathan

Thanks!
- Nuno Sá