On Tue, 10 Dec 2024 10:46:46 +0000
Guillaume Stols <gstols@xxxxxxxxxxxx> wrote:
> Since we must access the bus parallel bus using a custom procedure,
> let's add a specialized compatible, and define specialized callbacks for
> writing the registers using the parallel interface.
>
> Signed-off-by: Guillaume Stols <gstols@xxxxxxxxxxxx>
Hi Guillaume,
A few comments inline.
Thanks,
Jonathan
> ---
> drivers/iio/adc/ad7606_bi.h | 16 +++++++
> drivers/iio/adc/adi-axi-adc.c | 100 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 116 insertions(+)
>
> diff --git a/drivers/iio/adc/ad7606_bi.h b/drivers/iio/adc/ad7606_bi.h
> new file mode 100644
> index 000000000000..9ade23ec61dd
> --- /dev/null
> +++ b/drivers/iio/adc/ad7606_bi.h
Why bi? Bus interface? I'd spell it out. Header name lengths don't
usually need to be quite this short.
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (c) 2010-2024 Analog Devices Inc.
> + * Copyright (c) 2024 Baylibre, SAS
> + */
> +#ifndef __LINUX_PLATFORM_DATA_AD7606_H__
> +#define __LINUX_PLATFORM_DATA_AD7606_H__
> +
> +#include <linux/iio/backend.h>
Only a pointer type needed, so a forward def better than an include
(which costs some trivial amount of compile time).
struct iio_backend;
> +
> +struct ad7606_platform_data {
> + int (*bus_reg_read)(struct iio_backend *back, u32 reg, u32 *val);
> + int (*bus_reg_write)(struct iio_backend *back, u32 reg, u32 val);
> +};
> +
> +#endif /* __LINUX_PLATFORM_DATA_AD7606_H__ */
> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> index 7ff636643e56..b8bcf89417b0 100644
> --- a/drivers/iio/adc/adi-axi-adc.c
> +++ b/drivers/iio/adc/adi-axi-adc.c
> @@ -27,6 +27,7 @@
> #include <linux/iio/buffer.h>
> #include <linux/iio/iio.h>
>
> +#include "ad7606_bi.h"
> /*
> * Register definitions:
> * https://wiki.analog.com/resources/fpga/docs/axi_adc_ip#register_map
> @@ -73,6 +74,12 @@
> #define ADI_AXI_ADC_REG_DELAY(l) (0x0800 + (l) * 0x4)
> #define AXI_ADC_DELAY_CTRL_MASK GENMASK(4, 0)
>
> +#define ADI_AXI_REG_CONFIG_WR 0x0080
> +#define ADI_AXI_REG_CONFIG_RD 0x0084
> +#define ADI_AXI_REG_CONFIG_CTRL 0x008c
> +#define ADI_AXI_REG_CONFIG_CTRL_READ 0x03
> +#define ADI_AXI_REG_CONFIG_CTRL_WRITE 0x01
> +
> #define ADI_AXI_ADC_MAX_IO_NUM_LANES 15
>
> #define ADI_AXI_REG_CHAN_CTRL_DEFAULTS \
> @@ -80,6 +87,11 @@
> ADI_AXI_REG_CHAN_CTRL_FMT_EN | \
> ADI_AXI_REG_CHAN_CTRL_ENABLE)
>
> +/* AD7606's specific */
> +#define AD7606_REG_READ_BIT 0x8000
> +#define AD7606_REG_ADDRESS_MASK 0xff00
> +#define AD7606_REG_VALUE_MASK 0x00ff
> +
Maybe name these to keep the ADI_AXI_ prefix as well. Otherwise it might get
a little confusing wrt to registers in teh deivce. They are nice short
names currently so that shouldn't make the code to messy.
> struct axi_adc_info {
> unsigned int version;
> const struct iio_backend_info *backend_info;
> @@ -313,6 +325,80 @@ static struct iio_buffer *axi_adc_request_buffer(struct iio_backend *back,
> return iio_dmaengine_buffer_setup(st->dev, indio_dev, dma_name);
> }
>
> +static int axi_adc_raw_write(struct iio_backend *back, void *buf, unsigned int len)
> +{
> + struct adi_axi_adc_state *st = iio_backend_get_priv(back);
> + u32 data;
> + u32 *bdata = buf;
data = *(u32)(buf);
Is fine here I think rather than the extra local variable that is just
used to do the type cast.
> +
> + data = *bdata;
> + regmap_write(st->regmap, ADI_AXI_REG_CONFIG_WR, data);
> + regmap_write(st->regmap, ADI_AXI_REG_CONFIG_CTRL,
> + ADI_AXI_REG_CONFIG_CTRL_WRITE);
> + usleep_range(50, 100);
> + regmap_write(st->regmap, ADI_AXI_REG_CONFIG_CTRL, 0x00);
> + usleep_range(50, 100);
> +
> + return 0;
> +}
...
> +
> +static int ad7606_bi_reg_write(struct iio_backend *back, u32 reg, u32 val)
> +{
> + struct adi_axi_adc_state *st = iio_backend_get_priv(back);
> + u32 buf;
> +
> + guard(mutex)(&st->lock);
> +
> + /* Read any register to switch to register mode */
What follows looks like a write...
> + buf = 0xaf00;
> + axi_adc_raw_write(back, &buf, sizeof(buf));
> +
> + buf = FIELD_PREP(AD7606_REG_ADDRESS_MASK, reg) | FIELD_PREP(AD7606_REG_VALUE_MASK, val);
> + axi_adc_raw_write(back, &buf, sizeof(buf));
> +
> + /* Write 0x0 on the bus to get back to ADC mode */
> + buf = 0;
> + axi_adc_raw_write(back, &buf, sizeof(buf));
> +
> + return 0;
> +}
