Re: [PATCH 6/8] iio: adc: adi-axi-adc: support digital interface calibration

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 19 Apr 2024 17:36:49 +0200
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:

> From: Nuno Sa <nuno.sa@analog.com>
> 
> Implement the new IIO backend APIs for calibrating the data
> digital interfaces.
> 
> While at it, removed the tabs in 'struct adi_axi_adc_state' and used
> spaces for the members.

Ideally a precursor patch, but meh, it's 2 lines so I'll just moan about
it and move on.

A few minor things inline.


> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/adc/adi-axi-adc.c | 113 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 111 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> index b312369b7366..d58fa05499c4 100644
> --- a/drivers/iio/adc/adi-axi-adc.c
> +++ b/drivers/iio/adc/adi-axi-adc.c
> @@ -7,11 +7,13 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/cleanup.h>
>  #include <linux/clk.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/delay.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/property.h>
> @@ -37,6 +39,9 @@
>  #define   ADI_AXI_REG_RSTN_MMCM_RSTN		BIT(1)
>  #define   ADI_AXI_REG_RSTN_RSTN			BIT(0)
>  
> +#define ADI_AXI_ADC_REG_CTRL			0x0044
> +#define    ADI_AXI_ADC_CTRL_DDR_EDGESEL_MASK	BIT(1)
> +
>  /* ADC Channel controls */
>  
>  #define ADI_AXI_REG_CHAN_CTRL(c)		(0x0400 + (c) * 0x40)
> @@ -51,14 +56,28 @@
>  #define   ADI_AXI_REG_CHAN_CTRL_PN_TYPE_OWR	BIT(1)
>  #define   ADI_AXI_REG_CHAN_CTRL_ENABLE		BIT(0)
>  
> +#define ADI_AXI_ADC_REG_CHAN_STATUS(c)		(0x0404 + (c) * 0x40)
> +#define   ADI_AXI_ADC_CHAN_STAT_PN_MASK		GENMASK(2, 1)
> +
> +#define ADI_AXI_ADC_REG_CHAN_CTRL_3(c)		(0x0418 + (c) * 0x40)
> +#define   ADI_AXI_ADC_CHAN_PN_SEL_MASK		GENMASK(19, 16)
> +
> +/* IO Delays */
> +#define ADI_AXI_ADC_REG_DELAY(l)		(0x0800 + (l) * 0x4)
> +#define   AXI_ADC_DELAY_CTRL_MASK		GENMASK(4, 0)
> +
> +#define ADI_AXI_ADC_MAX_IO_NUM_LANES		15
> +
>  #define ADI_AXI_REG_CHAN_CTRL_DEFAULTS		\
>  	(ADI_AXI_REG_CHAN_CTRL_FMT_SIGNEXT |	\
>  	 ADI_AXI_REG_CHAN_CTRL_FMT_EN |		\
>  	 ADI_AXI_REG_CHAN_CTRL_ENABLE)
>  
>  struct adi_axi_adc_state {
> -	struct regmap				*regmap;
> -	struct device				*dev;
> +	struct regmap *regmap;
> +	struct device *dev;
> +	/* lock to protect multiple accesses to the device registers */

Why?  The locking in regmap protects register accesses in general I believe.
I guess this is to prevent accesses during that error detection routine?
Needs more detail in the comment.

> +	struct mutex lock;
>  };
>  
>  static int axi_adc_enable(struct iio_backend *back)
> @@ -104,6 +123,92 @@ static int axi_adc_data_format_set(struct iio_backend *back, unsigned int chan,
>  				  ADI_AXI_REG_CHAN_CTRL_FMT_MASK, val);
>  }
>  
> +static int axi_adc_data_sample_trigger(struct iio_backend *back,
> +				       enum iio_backend_sample_trigger trigger)
> +{
> +	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
> +
> +	if (trigger == IIO_BACKEND_SAMPLE_TRIGGER_EDGE_RISING)

It's an enum, so can't have more than one. Hence switch statement is probably
more extensible and natural to read.

> +		return regmap_clear_bits(st->regmap, ADI_AXI_ADC_REG_CTRL,
> +					 ADI_AXI_ADC_CTRL_DDR_EDGESEL_MASK);
> +	if (trigger == IIO_BACKEND_SAMPLE_TRIGGER_EDGE_FALLING)
> +		return regmap_set_bits(st->regmap, ADI_AXI_ADC_REG_CTRL,
> +				       ADI_AXI_ADC_CTRL_DDR_EDGESEL_MASK);
> +
> +	return -EINVAL;
> +}
> +


> +static int axi_adc_chan_status(struct iio_backend *back, unsigned int chan,
> +			       struct iio_backend_chan_status *status)
> +{
> +	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
> +	int ret;
> +	u32 val;
> +
> +	guard(mutex)(&st->lock);
> +	/* reset test bits by setting them */
> +	ret = regmap_write(st->regmap, ADI_AXI_ADC_REG_CHAN_STATUS(chan),
> +			   ADI_AXI_ADC_CHAN_STAT_PN_MASK);
> +	if (ret)
> +		return ret;
> +
> +	fsleep(1000);
Why this particular length sleep?  Is this a case of let it sit a while an dsee
if an error shows up?  If so a comment on that would be good.
> +
> +	ret = regmap_read(st->regmap, ADI_AXI_ADC_REG_CHAN_STATUS(chan), &val);
> +	if (ret)
> +		return ret;
> +
> +	if (ADI_AXI_ADC_CHAN_STAT_PN_MASK & val)
> +		status->errors = true;
> +
> +	return 0;
> +}
> +
>  static int axi_adc_chan_enable(struct iio_backend *back, unsigned int chan)
>  {
>  	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
> @@ -166,6 +271,10 @@ static const struct iio_backend_ops adi_axi_adc_generic = {
>  	.chan_disable = axi_adc_chan_disable,
>  	.request_buffer = axi_adc_request_buffer,
>  	.free_buffer = axi_adc_free_buffer,
> +	.data_sample_trigger = axi_adc_data_sample_trigger,
> +	.iodelay_set = axi_adc_iodelays_set,
> +	.test_pattern_set = axi_adc_test_pattern_set,
> +	.chan_status = axi_adc_chan_status,
>  };
>  
>  static int adi_axi_adc_probe(struct platform_device *pdev)
>