On Fri, 19 Apr 2024 17:36:49 +0200
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@xxxxxxxxxx> wrote:
> From: Nuno Sa <nuno.sa@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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)
>
