From: Angelo Dureghello <adureghello@baylibre.com>
Extend AXI-DAC backend with new features required to interface
to the ad3552r DAC. Mainly, a new compatible string is added to
support the ad3552r-axi DAC IP, very similar to the generic DAC
IP but with some customizations to work with the ad3552r.
Then, a serie of generic functions has been added to match with
ad3552r needs. Function names has been kept generic as much as
possible, to allow re-utilization from other frontend drivers.
Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
drivers/iio/dac/adi-axi-dac.c | 274 ++++++++++++++++++++++++++++++++++++++++--
1 file changed, 265 insertions(+), 9 deletions(-)
diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
index b8b4171b8043..3ca3a14c575b 100644
--- a/drivers/iio/dac/adi-axi-dac.c
+++ b/drivers/iio/dac/adi-axi-dac.c
@@ -44,11 +44,34 @@
#define AXI_DAC_RSTN_MMCM_RSTN BIT(1)
#define AXI_DAC_RSTN_RSTN BIT(0)
#define AXI_DAC_REG_CNTRL_1 0x0044
+#define AXI_DAC_EXT_SYNC_ARM BIT(1)
+#define AXI_DAC_EXT_SYNC_DISARM BIT(2)
#define AXI_DAC_SYNC BIT(0)
#define AXI_DAC_REG_CNTRL_2 0x0048
+#define AXI_DAC_SDR_DDR_N BIT(16)
+#define AXI_DAC_SYMB_8B BIT(14)
#define ADI_DAC_R1_MODE BIT(5)
+#define AXI_DAC_UNSIGNED_DATA BIT(4)
+#define AXI_DAC_REG_STATUS_1 0x54
+#define AXI_DAC_REG_STATUS_2 0x58
#define AXI_DAC_DRP_STATUS 0x0074
#define AXI_DAC_DRP_LOCKED BIT(17)
+#define AXI_DAC_CNTRL_DATA_RD 0x0080
+#define AXI_DAC_DATA_RD_8 GENMASK(7, 0)
+#define AXI_DAC_DATA_RD_16 GENMASK(15, 0)
+#define AXI_DAC_CNTRL_DATA_WR 0x0084
+#define AXI_DAC_DATA_WR_8 GENMASK(23, 16)
+#define AXI_DAC_DATA_WR_16 GENMASK(23, 8)
+#define AXI_DAC_UI_STATUS 0x0088
+#define AXI_DAC_BUSY BIT(4)
+#define AXI_DAC_REG_CUSTOM_CTRL 0x008C
+#define AXI_DAC_ADDRESS GENMASK(31, 24)
+#define AXI_DAC_SYNCED_TRANSFER BIT(2)
+#define AXI_DAC_STREAM BIT(1)
+#define AXI_DAC_TRANSFER_DATA BIT(0)
+
+#define AXI_DAC_STREAM_ENABLE (AXI_DAC_TRANSFER_DATA | AXI_DAC_STREAM)
+
/* DAC Channel controls */
#define AXI_DAC_REG_CHAN_CNTRL_1(c) (0x0400 + (c) * 0x40)
#define AXI_DAC_REG_CHAN_CNTRL_3(c) (0x0408 + (c) * 0x40)
@@ -62,11 +85,25 @@
#define AXI_DAC_REG_CHAN_CNTRL_7(c) (0x0418 + (c) * 0x40)
#define AXI_DAC_DATA_SEL GENMASK(3, 0)
+#define AXI_DAC_RD_ADDR(x) (BIT(7) | (x))
+
/* 360 degrees in rad */
#define AXI_DAC_2_PI_MEGA 6283190
+
enum {
AXI_DAC_DATA_INTERNAL_TONE,
AXI_DAC_DATA_DMA = 2,
+ AXI_DAC_DATA_INTERNAL_RAMP_16BIT = 11,
+};
+
+enum {
+ AXI_DAC_BUS_TYPE_NONE,
+ AXI_DAC_BUS_TYPE_DDR_QSPI,
+};
+
+struct axi_dac_info {
+ unsigned int version;
+ int bus_type;
};
struct axi_dac_state {
@@ -77,6 +114,7 @@ struct axi_dac_state {
* data/variables.
*/
struct mutex lock;
+ const struct axi_dac_info *info;
u64 dac_clk;
u32 reg_config;
bool int_tone;
@@ -461,6 +499,11 @@ static int axi_dac_data_source_set(struct iio_backend *back, unsigned int chan,
return regmap_update_bits(st->regmap,
AXI_DAC_REG_CHAN_CNTRL_7(chan),
AXI_DAC_DATA_SEL, AXI_DAC_DATA_DMA);
+ case IIO_BACKEND_INTERNAL_RAMP_16BIT:
+ return regmap_update_bits(st->regmap,
+ AXI_DAC_REG_CHAN_CNTRL_7(chan),
+ AXI_DAC_DATA_SEL,
+ AXI_DAC_DATA_INTERNAL_RAMP_16BIT);
default:
return -EINVAL;
}
@@ -518,9 +561,206 @@ static int axi_dac_reg_access(struct iio_backend *back, unsigned int reg,
return regmap_write(st->regmap, reg, writeval);
}
+static int axi_dac_ext_sync_enable(struct iio_backend *back)
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+
+ return regmap_set_bits(st->regmap, AXI_DAC_REG_CNTRL_1,
+ AXI_DAC_EXT_SYNC_ARM);
+}
+
+static int axi_dac_ext_sync_disable(struct iio_backend *back)
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+
+ return regmap_clear_bits(st->regmap, AXI_DAC_REG_CNTRL_1,
+ AXI_DAC_EXT_SYNC_DISARM);
+}
+
+static int axi_dac_ddr_enable(struct iio_backend *back)
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+
+ return regmap_clear_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
+ AXI_DAC_SDR_DDR_N);
+}
+
+static int axi_dac_ddr_disable(struct iio_backend *back)
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+
+ return regmap_set_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
+ AXI_DAC_SDR_DDR_N);
+}
+
+static int axi_dac_buffer_enable(struct iio_backend *back)
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+
+ return regmap_set_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
+ AXI_DAC_STREAM_ENABLE);
+}
+
+static int axi_dac_buffer_disable(struct iio_backend *back)
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+
+ return regmap_clear_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
+ AXI_DAC_STREAM_ENABLE);
+}
+
+static int axi_dac_data_transfer_addr(struct iio_backend *back, u32 address)
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+
+ /*
+ * Sample register address, when the DAC is configured, or stream
+ * start address when the FSM is in stream state.
+ */
+ return regmap_update_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
+ AXI_DAC_ADDRESS,
+ FIELD_PREP(AXI_DAC_ADDRESS, address));
+}
+
+static int axi_dac_data_format_set(struct iio_backend *back, unsigned int ch,
+ const struct iio_backend_data_fmt *data)
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+
+ if (data->type == IIO_BACKEND_DATA_UNSIGNED)
+ return regmap_clear_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
+ AXI_DAC_UNSIGNED_DATA);
+
+ return -EINVAL;
+}
+
+static int axi_dac_read_raw(struct iio_backend *back,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+ int err;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_FREQUENCY: {
+ int clk_in, reg;
+
+ /*
+ * As from AXI IP documentation,
+ * returning the SCLK depending on the stream mode.
+ */
+ clk_in = clk_get_rate(clk_get(st->dev, 0));
+
+ err = regmap_read(st->regmap, AXI_DAC_REG_CUSTOM_CTRL, ®);
+ if (err)
+ return err;
+
+ if (reg & AXI_DAC_STREAM)
+ *val = clk_in / 2;
+ else
+ *val = clk_in / 8;
+
+ return IIO_VAL_INT;
+ }
+ default:
+ return -EINVAL;
+ }
+}
+
+static int axi_dac_bus_reg_write(struct iio_backend *back, u32 reg,
+ unsigned int val, size_t data_size)
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+
+ switch (st->info->bus_type) {
+ case AXI_DAC_BUS_TYPE_DDR_QSPI: {
+ int ret;
+ u32 ival;
+
+ if (data_size == 2)
+ ival = FIELD_PREP(AXI_DAC_DATA_WR_16, val);
+ else
+ ival = FIELD_PREP(AXI_DAC_DATA_WR_8, val);
+
+ ret = regmap_write(st->regmap, AXI_DAC_CNTRL_DATA_WR, ival);
+ if (ret)
+ return ret;
+
+ /*
+ * Both REG_CNTRL_2 and AXI_DAC_CNTRL_DATA_WR need to know
+ * the data size. So keeping data size control here only,
+ * since data size is mandatory for the current transfer.
+ * DDR state handled separately by specific backend calls,
+ * generally all raw register writes are SDR.
+ */
+ if (data_size == 1)
+ ret = regmap_set_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
+ AXI_DAC_SYMB_8B);
+ else
+ ret = regmap_clear_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
+ AXI_DAC_SYMB_8B);
+ if (ret)
+ return ret;
+
+ ret = regmap_update_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
+ AXI_DAC_ADDRESS,
+ FIELD_PREP(AXI_DAC_ADDRESS, reg));
+ if (ret)
+ return ret;
+
+ ret = regmap_update_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
+ AXI_DAC_TRANSFER_DATA,
+ AXI_DAC_TRANSFER_DATA);
+ if (ret)
+ return ret;
+
+ ret = regmap_read_poll_timeout(st->regmap,
+ AXI_DAC_REG_CUSTOM_CTRL, ival,
+ ival & AXI_DAC_TRANSFER_DATA,
+ 10, 100 * KILO);
+ if (ret)
+ return ret;
+
+ return regmap_clear_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
+ AXI_DAC_TRANSFER_DATA);
+ }
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int axi_dac_bus_reg_read(struct iio_backend *back, u32 reg,
+ unsigned int *val, size_t data_size)
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+
+ switch (st->info->bus_type) {
+ case AXI_DAC_BUS_TYPE_DDR_QSPI: {
+ int ret;
+ u32 bval;
+
+ ret = axi_dac_bus_reg_write(back, AXI_DAC_RD_ADDR(reg), 0,
+ data_size);
+ if (ret)
+ return ret;
+
+ ret = regmap_read_poll_timeout(st->regmap, AXI_DAC_UI_STATUS,
+ bval, bval != AXI_DAC_BUSY,
+ 10, 100);
+ if (ret)
+ return ret;
+
+ return regmap_read(st->regmap, AXI_DAC_CNTRL_DATA_RD, val);
+ }
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
static const struct iio_backend_ops axi_dac_generic_ops = {
.enable = axi_dac_enable,
.disable = axi_dac_disable,
+ .read_raw = axi_dac_read_raw,
.request_buffer = axi_dac_request_buffer,
.free_buffer = axi_dac_free_buffer,
.extend_chan_spec = axi_dac_extend_chan,
@@ -528,6 +768,14 @@ static const struct iio_backend_ops axi_dac_generic_ops = {
.ext_info_get = axi_dac_ext_info_get,
.data_source_set = axi_dac_data_source_set,
.set_sample_rate = axi_dac_set_sample_rate,
+ .ext_sync_enable = axi_dac_ext_sync_enable,
+ .ext_sync_disable = axi_dac_ext_sync_disable,
+ .ddr_enable = axi_dac_ddr_enable,
+ .ddr_disable = axi_dac_ddr_disable,
+ .buffer_enable = axi_dac_buffer_enable,
+ .buffer_disable = axi_dac_buffer_disable,
+ .data_format_set = axi_dac_data_format_set,
+ .data_transfer_addr = axi_dac_data_transfer_addr,
.debugfs_reg_access = iio_backend_debugfs_ptr(axi_dac_reg_access),
};
@@ -545,7 +793,6 @@ static const struct regmap_config axi_dac_regmap_config = {
static int axi_dac_probe(struct platform_device *pdev)
{
- const unsigned int *expected_ver;
struct axi_dac_state *st;
void __iomem *base;
unsigned int ver;
@@ -556,8 +803,8 @@ static int axi_dac_probe(struct platform_device *pdev)
if (!st)
return -ENOMEM;
- expected_ver = device_get_match_data(&pdev->dev);
- if (!expected_ver)
+ st->info = device_get_match_data(&pdev->dev);
+ if (!st->info)
return -ENODEV;
clk = devm_clk_get_enabled(&pdev->dev, NULL);
@@ -588,12 +835,13 @@ static int axi_dac_probe(struct platform_device *pdev)
if (ret)
return ret;
- if (ADI_AXI_PCORE_VER_MAJOR(ver) != ADI_AXI_PCORE_VER_MAJOR(*expected_ver)) {
+ if (ADI_AXI_PCORE_VER_MAJOR(ver) !=
+ ADI_AXI_PCORE_VER_MAJOR(st->info->version)) {
dev_err(&pdev->dev,
"Major version mismatch. Expected %d.%.2d.%c, Reported %d.%.2d.%c\n",
- ADI_AXI_PCORE_VER_MAJOR(*expected_ver),
- ADI_AXI_PCORE_VER_MINOR(*expected_ver),
- ADI_AXI_PCORE_VER_PATCH(*expected_ver),
+ ADI_AXI_PCORE_VER_MAJOR(st->info->version),
+ ADI_AXI_PCORE_VER_MINOR(st->info->version),
+ ADI_AXI_PCORE_VER_PATCH(st->info->version),
ADI_AXI_PCORE_VER_MAJOR(ver),
ADI_AXI_PCORE_VER_MINOR(ver),
ADI_AXI_PCORE_VER_PATCH(ver));
@@ -631,10 +879,18 @@ static int axi_dac_probe(struct platform_device *pdev)
return 0;
}
-static unsigned int axi_dac_9_1_b_info = ADI_AXI_PCORE_VER(9, 1, 'b');
+static const struct axi_dac_info dac_generic = {
+ .version = ADI_AXI_PCORE_VER(9, 1, 'b'),
+};
+
+static const struct axi_dac_info dac_ad3552r = {
+ .version = ADI_AXI_PCORE_VER(9, 1, 'b'),
+ .bus_type = AXI_DAC_BUS_TYPE_DDR_QSPI,
+};
static const struct of_device_id axi_dac_of_match[] = {
- { .compatible = "adi,axi-dac-9.1.b", .data = &axi_dac_9_1_b_info },
+ { .compatible = "adi,axi-dac-9.1.b", .data = &dac_generic },
+ { .compatible = "adi,axi-ad3552r", .data = &dac_ad3552r },
{}
};
MODULE_DEVICE_TABLE(of, axi_dac_of_match);
--
2.45.0.rc1On Thu, 19 Sep 2024 11:20:02 +0200
Angelo Dureghello <adureghello@baylibre.com> wrote:
> From: Angelo Dureghello <adureghello@baylibre.com>
>
> Extend AXI-DAC backend with new features required to interface
> to the ad3552r DAC. Mainly, a new compatible string is added to
> support the ad3552r-axi DAC IP, very similar to the generic DAC
> IP but with some customizations to work with the ad3552r.
>
> Then, a serie of generic functions has been added to match with
> ad3552r needs. Function names has been kept generic as much as
> possible, to allow re-utilization from other frontend drivers.
>
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
Hi Angelo,
A few things in addition to Nuno's review.
> ---
> drivers/iio/dac/adi-axi-dac.c | 274 ++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 265 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
> index b8b4171b8043..3ca3a14c575b 100644
> --- a/drivers/iio/dac/adi-axi-dac.c
> +++ b/drivers/iio/dac/adi-axi-dac.c
> @@ -44,11 +44,34 @@
> #define AXI_DAC_RSTN_MMCM_RSTN BIT(1)
> #define AXI_DAC_RSTN_RSTN BIT(0)
> #define AXI_DAC_REG_CNTRL_1 0x0044
> +#define AXI_DAC_EXT_SYNC_ARM BIT(1)
> +#define AXI_DAC_EXT_SYNC_DISARM BIT(2)
Probably want some sort of order in here! Either do highest
to lowest bit or the other way around. Not a mixture.
I should probably have moaned about this earlier, but naming these
defines to make it clear what is the register address and which
fields belong to that register. Right now that's not easy to tell
for most of these and means that more effort is needed in review
to check values are written to the relevant register and not some
other one.
Perhaps a precursor patch to clean that up would be a good idea.
(or do it right for the new stuff and chase this with a patch tidying
up the existing registers).
The exisint defines seem to have _REG_ in the middle which sort
of does this role but then the fields are not associated with the
register.
E.g. this one should be something like
#define AXI_DAC_FIELD_CNTRL_1_EXT_SYNC_ARM BIT(1)
More common would be to move _REG_ to the end and then just
have fields as the ones that aren't _REG
> #define AXI_DAC_SYNC BIT(0)
> #define AXI_DAC_REG_CNTRL_2 0x0048
> +#define AXI_DAC_SDR_DDR_N BIT(16)
> +#define AXI_DAC_SYMB_8B BIT(14)
> #define ADI_DAC_R1_MODE BIT(5)
Oddity of tabs vs spaces here that should be consistent.
> +#define AXI_DAC_UNSIGNED_DATA BIT(4)
> +#define AXI_DAC_REG_STATUS_1 0x54
> +#define AXI_DAC_REG_STATUS_2 0x58
> #define AXI_DAC_DRP_STATUS 0x0074
> #define AXI_DAC_DRP_LOCKED BIT(17)
> +#define AXI_DAC_CNTRL_DATA_RD 0x0080
Prior cases have _REG_ for the registers, this one doesn't.
> +#define AXI_DAC_DATA_RD_8 GENMASK(7, 0)
> +#define AXI_DAC_DATA_RD_16 GENMASK(15, 0)
> +#define AXI_DAC_CNTRL_DATA_WR 0x0084
> +#define AXI_DAC_DATA_WR_8 GENMASK(23, 16)
> +#define AXI_DAC_DATA_WR_16 GENMASK(23, 8)
Huh. interesting IP, it's aligned one way for read path and the opposite
for write. I bet that never introduces any bugs :)
I'd started commenting the DATA_RD cases seemed obvious enough not
to need defines before I read on a few lines!
> +#define AXI_DAC_UI_STATUS 0x0088
> +#define AXI_DAC_BUSY BIT(4)
> +#define AXI_DAC_REG_CUSTOM_CTRL 0x008C
> +#define AXI_DAC_ADDRESS GENMASK(31, 24)
> +#define AXI_DAC_SYNCED_TRANSFER BIT(2)
> +#define AXI_DAC_STREAM BIT(1)
> +#define AXI_DAC_TRANSFER_DATA BIT(0)
> +static int axi_dac_bus_reg_write(struct iio_backend *back, u32 reg,
> + unsigned int val, size_t data_size)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + switch (st->info->bus_type) {
> + case AXI_DAC_BUS_TYPE_DDR_QSPI: {
> + int ret;
> + u32 ival;
> +
> + if (data_size == 2)
> + ival = FIELD_PREP(AXI_DAC_DATA_WR_16, val);
> + else
> + ival = FIELD_PREP(AXI_DAC_DATA_WR_8, val);
> +
> + ret = regmap_write(st->regmap, AXI_DAC_CNTRL_DATA_WR, ival);
> + if (ret)
> + return ret;
> +
> + /*
> + * Both REG_CNTRL_2 and AXI_DAC_CNTRL_DATA_WR need to know
> + * the data size. So keeping data size control here only,
> + * since data size is mandatory for the current transfer.
> + * DDR state handled separately by specific backend calls,
> + * generally all raw register writes are SDR.
> + */
> + if (data_size == 1)
> + ret = regmap_set_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
> + AXI_DAC_SYMB_8B);
> + else
> + ret = regmap_clear_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
> + AXI_DAC_SYMB_8B);
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
> + AXI_DAC_ADDRESS,
> + FIELD_PREP(AXI_DAC_ADDRESS, reg));
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
> + AXI_DAC_TRANSFER_DATA,
> + AXI_DAC_TRANSFER_DATA);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read_poll_timeout(st->regmap,
> + AXI_DAC_REG_CUSTOM_CTRL, ival,
> + ival & AXI_DAC_TRANSFER_DATA,
> + 10, 100 * KILO);
> + if (ret)
> + return ret;
> +
> + return regmap_clear_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
> + AXI_DAC_TRANSFER_DATA);
> + }
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int axi_dac_bus_reg_read(struct iio_backend *back, u32 reg,
> + unsigned int *val, size_t data_size)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + switch (st->info->bus_type) {
> + case AXI_DAC_BUS_TYPE_DDR_QSPI: {
> + int ret;
> + u32 bval;
> +
> + ret = axi_dac_bus_reg_write(back, AXI_DAC_RD_ADDR(reg), 0,
> + data_size);
> + if (ret)
So the data size is in the write of the register not the read. That seems odd.
I guess this is about setting the bit that says how bit the read is going to be?
Maybe a comment would help here.
> + return ret;
> +
> + ret = regmap_read_poll_timeout(st->regmap, AXI_DAC_UI_STATUS,
> + bval, bval != AXI_DAC_BUSY,
> + 10, 100);
> + if (ret)
> + return ret;
> +
> + return regmap_read(st->regmap, AXI_DAC_CNTRL_DATA_RD, val);
I supposed datasize doesn't strictly matter here, but for ease of readability
I'd do FIELD_GET() to pull out only what was requested.
> + }
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> static const struct iio_backend_ops axi_dac_generic_ops = {
> .enable = axi_dac_enable,
> .disable = axi_dac_disable,
> + .read_raw = axi_dac_read_raw,
> .request_buffer = axi_dac_request_buffer,
> .free_buffer = axi_dac_free_buffer,
> .extend_chan_spec = axi_dac_extend_chan,
> @@ -528,6 +768,14 @@ static const struct iio_backend_ops axi_dac_generic_ops = {
> .ext_info_get = axi_dac_ext_info_get,
> .data_source_set = axi_dac_data_source_set,
> .set_sample_rate = axi_dac_set_sample_rate,
> + .ext_sync_enable = axi_dac_ext_sync_enable,
> + .ext_sync_disable = axi_dac_ext_sync_disable,
> + .ddr_enable = axi_dac_ddr_enable,
> + .ddr_disable = axi_dac_ddr_disable,
> + .buffer_enable = axi_dac_buffer_enable,
> + .buffer_disable = axi_dac_buffer_disable,
> + .data_format_set = axi_dac_data_format_set,
> + .data_transfer_addr = axi_dac_data_transfer_addr,
> .debugfs_reg_access = iio_backend_debugfs_ptr(axi_dac_reg_access),
> };
>
> @@ -545,7 +793,6 @@ static const struct regmap_config axi_dac_regmap_config = {
>
> static int axi_dac_probe(struct platform_device *pdev)
> {
> - const unsigned int *expected_ver;
> struct axi_dac_state *st;
> void __iomem *base;
> unsigned int ver;
> @@ -556,8 +803,8 @@ static int axi_dac_probe(struct platform_device *pdev)
> if (!st)
> return -ENOMEM;
>
> - expected_ver = device_get_match_data(&pdev->dev);
> - if (!expected_ver)
> + st->info = device_get_match_data(&pdev->dev);
> + if (!st->info)
> return -ENODEV;
>
> clk = devm_clk_get_enabled(&pdev->dev, NULL);
> @@ -588,12 +835,13 @@ static int axi_dac_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> - if (ADI_AXI_PCORE_VER_MAJOR(ver) != ADI_AXI_PCORE_VER_MAJOR(*expected_ver)) {
> + if (ADI_AXI_PCORE_VER_MAJOR(ver) !=
> + ADI_AXI_PCORE_VER_MAJOR(st->info->version)) {
> dev_err(&pdev->dev,
> "Major version mismatch. Expected %d.%.2d.%c, Reported %d.%.2d.%c\n",
> - ADI_AXI_PCORE_VER_MAJOR(*expected_ver),
> - ADI_AXI_PCORE_VER_MINOR(*expected_ver),
> - ADI_AXI_PCORE_VER_PATCH(*expected_ver),
> + ADI_AXI_PCORE_VER_MAJOR(st->info->version),
> + ADI_AXI_PCORE_VER_MINOR(st->info->version),
> + ADI_AXI_PCORE_VER_PATCH(st->info->version),
> ADI_AXI_PCORE_VER_MAJOR(ver),
> ADI_AXI_PCORE_VER_MINOR(ver),
> ADI_AXI_PCORE_VER_PATCH(ver));
> @@ -631,10 +879,18 @@ static int axi_dac_probe(struct platform_device *pdev)
> return 0;
> }
>
> -static unsigned int axi_dac_9_1_b_info = ADI_AXI_PCORE_VER(9, 1, 'b');
> +static const struct axi_dac_info dac_generic = {
> + .version = ADI_AXI_PCORE_VER(9, 1, 'b'),
> +};
> +
> +static const struct axi_dac_info dac_ad3552r = {
> + .version = ADI_AXI_PCORE_VER(9, 1, 'b'),
> + .bus_type = AXI_DAC_BUS_TYPE_DDR_QSPI,
> +};
>
> static const struct of_device_id axi_dac_of_match[] = {
> - { .compatible = "adi,axi-dac-9.1.b", .data = &axi_dac_9_1_b_info },
> + { .compatible = "adi,axi-dac-9.1.b", .data = &dac_generic },
> + { .compatible = "adi,axi-ad3552r", .data = &dac_ad3552r },
> {}
> };
> MODULE_DEVICE_TABLE(of, axi_dac_of_match);
>
On Thu, 2024-09-19 at 11:20 +0200, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@baylibre.com>
>
> Extend AXI-DAC backend with new features required to interface
> to the ad3552r DAC. Mainly, a new compatible string is added to
> support the ad3552r-axi DAC IP, very similar to the generic DAC
> IP but with some customizations to work with the ad3552r.
>
> Then, a serie of generic functions has been added to match with
> ad3552r needs. Function names has been kept generic as much as
> possible, to allow re-utilization from other frontend drivers.
>
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
The commit title is not ok... iio: dac: adi-axi-dac: ...
> drivers/iio/dac/adi-axi-dac.c | 274 ++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 265 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
> index b8b4171b8043..3ca3a14c575b 100644
> --- a/drivers/iio/dac/adi-axi-dac.c
> +++ b/drivers/iio/dac/adi-axi-dac.c
> @@ -44,11 +44,34 @@
> #define AXI_DAC_RSTN_MMCM_RSTN BIT(1)
> #define AXI_DAC_RSTN_RSTN BIT(0)
> #define AXI_DAC_REG_CNTRL_1 0x0044
> +#define AXI_DAC_EXT_SYNC_ARM BIT(1)
> +#define AXI_DAC_EXT_SYNC_DISARM BIT(2)
> #define AXI_DAC_SYNC BIT(0)
> #define AXI_DAC_REG_CNTRL_2 0x0048
> +#define AXI_DAC_SDR_DDR_N BIT(16)
> +#define AXI_DAC_SYMB_8B BIT(14)
> #define ADI_DAC_R1_MODE BIT(5)
> +#define AXI_DAC_UNSIGNED_DATA BIT(4)
> +#define AXI_DAC_REG_STATUS_1 0x54
> +#define AXI_DAC_REG_STATUS_2 0x58
> #define AXI_DAC_DRP_STATUS 0x0074
> #define AXI_DAC_DRP_LOCKED BIT(17)
> +#define AXI_DAC_CNTRL_DATA_RD 0x0080
> +#define AXI_DAC_DATA_RD_8 GENMASK(7, 0)
> +#define AXI_DAC_DATA_RD_16 GENMASK(15, 0)
> +#define AXI_DAC_CNTRL_DATA_WR 0x0084
> +#define AXI_DAC_DATA_WR_8 GENMASK(23, 16)
> +#define AXI_DAC_DATA_WR_16 GENMASK(23, 8)
> +#define AXI_DAC_UI_STATUS 0x0088
> +#define AXI_DAC_BUSY BIT(4)
> +#define AXI_DAC_REG_CUSTOM_CTRL 0x008C
> +#define AXI_DAC_ADDRESS GENMASK(31, 24)
> +#define AXI_DAC_SYNCED_TRANSFER BIT(2)
> +#define AXI_DAC_STREAM BIT(1)
> +#define AXI_DAC_TRANSFER_DATA BIT(0)
> +
> +#define AXI_DAC_STREAM_ENABLE (AXI_DAC_TRANSFER_DATA | AXI_DAC_STREAM)
> +
> /* DAC Channel controls */
> #define AXI_DAC_REG_CHAN_CNTRL_1(c) (0x0400 + (c) * 0x40)
> #define AXI_DAC_REG_CHAN_CNTRL_3(c) (0x0408 + (c) * 0x40)
> @@ -62,11 +85,25 @@
> #define AXI_DAC_REG_CHAN_CNTRL_7(c) (0x0418 + (c) * 0x40)
> #define AXI_DAC_DATA_SEL GENMASK(3, 0)
>
> +#define AXI_DAC_RD_ADDR(x) (BIT(7) | (x))
> +
> /* 360 degrees in rad */
> #define AXI_DAC_2_PI_MEGA 6283190
> +
> enum {
> AXI_DAC_DATA_INTERNAL_TONE,
> AXI_DAC_DATA_DMA = 2,
> + AXI_DAC_DATA_INTERNAL_RAMP_16BIT = 11,
> +};
> +
> +enum {
> + AXI_DAC_BUS_TYPE_NONE,
> + AXI_DAC_BUS_TYPE_DDR_QSPI,
> +};
> +
> +struct axi_dac_info {
> + unsigned int version;
> + int bus_type;
Remove the bus_type... For now, let's just assume it's this QSPI implementation.
Let's worry with different buses when we actually have them. For now, let's implement
this one but only for the new compatible. Maybe have a boolean in here like
`bus_controller`. You'll also need a backend info structure per dac_info structure.
More on that below...
Also it makes sense to have a const char *name variable. If it has a different
compatible, we should name it accordingly.
> };
>
> struct axi_dac_state {
> @@ -77,6 +114,7 @@ struct axi_dac_state {
> * data/variables.
> */
> struct mutex lock;
> + const struct axi_dac_info *info;
> u64 dac_clk;
> u32 reg_config;
> bool int_tone;
> @@ -461,6 +499,11 @@ static int axi_dac_data_source_set(struct iio_backend *back,
> unsigned int chan,
> return regmap_update_bits(st->regmap,
> AXI_DAC_REG_CHAN_CNTRL_7(chan),
> AXI_DAC_DATA_SEL, AXI_DAC_DATA_DMA);
> + case IIO_BACKEND_INTERNAL_RAMP_16BIT:
> + return regmap_update_bits(st->regmap,
> + AXI_DAC_REG_CHAN_CNTRL_7(chan),
> + AXI_DAC_DATA_SEL,
> + AXI_DAC_DATA_INTERNAL_RAMP_16BIT);
> default:
> return -EINVAL;
> }
> @@ -518,9 +561,206 @@ static int axi_dac_reg_access(struct iio_backend *back,
> unsigned int reg,
> return regmap_write(st->regmap, reg, writeval);
> }
>
> +static int axi_dac_ext_sync_enable(struct iio_backend *back)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + return regmap_set_bits(st->regmap, AXI_DAC_REG_CNTRL_1,
> + AXI_DAC_EXT_SYNC_ARM);
> +}
> +
> +static int axi_dac_ext_sync_disable(struct iio_backend *back)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + return regmap_clear_bits(st->regmap, AXI_DAC_REG_CNTRL_1,
> + AXI_DAC_EXT_SYNC_DISARM);
> +}
> +
> +static int axi_dac_ddr_enable(struct iio_backend *back)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + return regmap_clear_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
> + AXI_DAC_SDR_DDR_N);
> +}
> +
> +static int axi_dac_ddr_disable(struct iio_backend *back)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + return regmap_set_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
> + AXI_DAC_SDR_DDR_N);
> +}
> +
> +static int axi_dac_buffer_enable(struct iio_backend *back)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + return regmap_set_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
> + AXI_DAC_STREAM_ENABLE);
> +}
> +
> +static int axi_dac_buffer_disable(struct iio_backend *back)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + return regmap_clear_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
> + AXI_DAC_STREAM_ENABLE);
> +}
> +
> +static int axi_dac_data_transfer_addr(struct iio_backend *back, u32 address)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + /*
> + * Sample register address, when the DAC is configured, or stream
> + * start address when the FSM is in stream state.
> + */
> + return regmap_update_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
> + AXI_DAC_ADDRESS,
> + FIELD_PREP(AXI_DAC_ADDRESS, address));
> +}
> +
> +static int axi_dac_data_format_set(struct iio_backend *back, unsigned int ch,
> + const struct iio_backend_data_fmt *data)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + if (data->type == IIO_BACKEND_DATA_UNSIGNED)
> + return regmap_clear_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
> + AXI_DAC_UNSIGNED_DATA);
> +
> + return -EINVAL;
nit: I would prefer error handling. Or assuming we might have additional types in the
future, we can also make it a switch() case...
> +}
> +
> +static int axi_dac_read_raw(struct iio_backend *back,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> + int err;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_FREQUENCY: {
> + int clk_in, reg;
> +
> + /*
> + * As from AXI IP documentation,
> + * returning the SCLK depending on the stream mode.
> + */
This comment is very specific to the implementation. Make sure to refer to the actual
ip on it (axi-ad3552r).
> + clk_in = clk_get_rate(clk_get(st->dev, 0));
> +
Nope.. You have several problems in here:
1) clk_get() every time the function get's called. This has leaks;
2) AFAIR, this IP already get's the AXI bus clock to enable it. You'll have to name
your clocks;
3) The clk_get() needs to be done on probe() (use the devm_ variant). And it is only
mandatory for the axi_ad3552r implementation.
4) You're not enabling the clock (you can have it done in one call).
5) You need to return -EOPNOTSUPP or similar in case there's no support for this. We
have a dedicated compatible so we can make use of it (maybe for now the
bus_controller boolean is enough - again, let's assume it's qspi).
6) I don't think the refclk is expected to change so you can likely get on probe().
> + err = regmap_read(st->regmap, AXI_DAC_REG_CUSTOM_CTRL, ®);
> + if (err)
> + return err;
> +
> + if (reg & AXI_DAC_STREAM)
> + *val = clk_in / 2;
> + else
> + *val = clk_in / 8;
> +
> + return IIO_VAL_INT;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int axi_dac_bus_reg_write(struct iio_backend *back, u32 reg,
> + unsigned int val, size_t data_size)
> +{
Be consistent... either two `u32` or two `unsigned int`
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + switch (st->info->bus_type) {
> + case AXI_DAC_BUS_TYPE_DDR_QSPI: {
> + int ret;
> + u32 ival;
> +
> + if (data_size == 2)
> + ival = FIELD_PREP(AXI_DAC_DATA_WR_16, val);
> + else
> + ival = FIELD_PREP(AXI_DAC_DATA_WR_8, val);
> +
> + ret = regmap_write(st->regmap, AXI_DAC_CNTRL_DATA_WR, ival);
> + if (ret)
> + return ret;
> +
> + /*
> + * Both REG_CNTRL_2 and AXI_DAC_CNTRL_DATA_WR need to know
> + * the data size. So keeping data size control here only,
> + * since data size is mandatory for the current transfer.
> + * DDR state handled separately by specific backend calls,
> + * generally all raw register writes are SDR.
> + */
> + if (data_size == 1)
> + ret = regmap_set_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
> + AXI_DAC_SYMB_8B);
> + else
> + ret = regmap_clear_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
> + AXI_DAC_SYMB_8B);
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
> + AXI_DAC_ADDRESS,
> + FIELD_PREP(AXI_DAC_ADDRESS, reg));
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
> + AXI_DAC_TRANSFER_DATA,
> + AXI_DAC_TRANSFER_DATA);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read_poll_timeout(st->regmap,
> + AXI_DAC_REG_CUSTOM_CTRL, ival,
> + ival & AXI_DAC_TRANSFER_DATA,
> + 10, 100 * KILO);
> + if (ret)
> + return ret;
> +
> + return regmap_clear_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
> + AXI_DAC_TRANSFER_DATA);
> + }
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int axi_dac_bus_reg_read(struct iio_backend *back, u32 reg,
> + unsigned int *val, size_t data_size)
> +{
ditto. Not sure if the backend ops are like this. If yes, same comment.
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + switch (st->info->bus_type) {
> + case AXI_DAC_BUS_TYPE_DDR_QSPI: {
> + int ret;
> + u32 bval;
> +
> + ret = axi_dac_bus_reg_write(back, AXI_DAC_RD_ADDR(reg), 0,
> + data_size);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read_poll_timeout(st->regmap, AXI_DAC_UI_STATUS,
> + bval, bval != AXI_DAC_BUSY,
> + 10, 100);
> + if (ret)
> + return ret;
> +
> + return regmap_read(st->regmap, AXI_DAC_CNTRL_DATA_RD, val);
> + }
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
Both the above look mostly good... Just get rid of the bus_type id :)
> static const struct iio_backend_ops axi_dac_generic_ops = {
> .enable = axi_dac_enable,
> .disable = axi_dac_disable,
> + .read_raw = axi_dac_read_raw,
> .request_buffer = axi_dac_request_buffer,
> .free_buffer = axi_dac_free_buffer,
> .extend_chan_spec = axi_dac_extend_chan,
> @@ -528,6 +768,14 @@ static const struct iio_backend_ops axi_dac_generic_ops = {
> .ext_info_get = axi_dac_ext_info_get,
> .data_source_set = axi_dac_data_source_set,
> .set_sample_rate = axi_dac_set_sample_rate,
> + .ext_sync_enable = axi_dac_ext_sync_enable,
> + .ext_sync_disable = axi_dac_ext_sync_disable,
> + .ddr_enable = axi_dac_ddr_enable,
> + .ddr_disable = axi_dac_ddr_disable,
> + .buffer_enable = axi_dac_buffer_enable,
> + .buffer_disable = axi_dac_buffer_disable,
> + .data_format_set = axi_dac_data_format_set,
> + .data_transfer_addr = axi_dac_data_transfer_addr,
> .debugfs_reg_access = iio_backend_debugfs_ptr(axi_dac_reg_access),
> };
Make sure to define a new struct iio_backend_ops with the ops this design needs and
leave the above untouched.
- Nuno Sá
© 2016 - 2024 Red Hat, Inc.