Re: [PATCH RFC 3/8] iio: backend adi-axi-dac: backend features
From: Angelo Dureghello
Date: Mon Sep 02 2024 - 12:06:15 EST
On 31/08/24 1:34 PM, Jonathan Cameron wrote:
On Thu, 29 Aug 2024 14:32:01 +0200
Angelo Dureghello <adureghello@xxxxxxxxxxxx> wrote:
From: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
Extend DAC backend with new features required for the AXI driver
version for the a3552r DAC.
Signed-off-by: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
Hi Angelo
Minor comments inline.
static int axi_dac_enable(struct iio_backend *back)
@@ -460,7 +493,13 @@ static int axi_dac_data_source_set(struct iio_backend *back, unsigned int chan,
case IIO_BACKEND_EXTERNAL:
return regmap_update_bits(st->regmap,
AXI_DAC_REG_CHAN_CNTRL_7(chan),
- AXI_DAC_DATA_SEL, AXI_DAC_DATA_DMA);
+ AXI_DAC_DATA_SEL,
+ AXI_DAC_DATA_DMA);
Unrelated change. If you want to change this, separate patch.
Thanks, fixed.
+ case IIO_BACKEND_INTERNAL_RAMP_16:
+ return regmap_update_bits(st->regmap,
+ AXI_DAC_REG_CHAN_CNTRL_7(chan),
+ AXI_DAC_DATA_SEL,
+ AXI_DAC_DATA_INTERNAL_RAMP_16);
default:
return -EINVAL;
}
@@ -518,9 +557,204 @@ static int axi_dac_reg_access(struct iio_backend *back, unsigned int reg,
return regmap_write(st->regmap, reg, writeval);
}
+
+static int axi_dac_bus_reg_write(struct iio_backend *back,
+ u32 reg, void *val, size_t size)
Maybe just pass an unsigned int for val?
So follow what regmap does? You will still need the size, but it
will just be configuration related rather than affecting the type
of val.
void * was used since data size in the future may vary depending
on the bus physical interface.
Actually, a reg bus write involves several AXI regmap operations.
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+
+ if (!st->bus_type)
+ return -EOPNOTSUPP;
+
+ if (st->bus_type == AXI_DAC_BUS_TYPE_QSPI) {
As below, I'd use a switch and factor out this block as a separate
bus specific function.
Ok, changed.
+ int ret;
+ u32 ival;
+
+ if (size != 1 && size != 2)
+ return -EINVAL;
+
+ switch (size) {
+ case 1:
+ ival = FIELD_PREP(AXI_DAC_DATA_WR_8, *(u8 *)val);
+ break;
+ case 2:
+ ival = FIELD_PREP(AXI_DAC_DATA_WR_16, *(u16 *)val);
+ break;
+ default:
+ return -EINVAL;
Hopefully compiler won't need this and the above. I'd drop the size != 1..
check in favour of just doing it in this switch.
sure, done.
+ }
+
+ 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 to the current transfer.
+ * DDR state handled separately by specific backend calls,
+ * generally all raw register writes are SDR.
+ */
+ if (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);
+ }
+
+ return -EINVAL;
+}
+
+static int axi_dac_bus_reg_read(struct iio_backend *back,
+ u32 reg, void *val, size_t size)
As for write, I'd just use an unsigned int * for val like
regmap does.
Ok, so initial choice was unsigned int, further thinking of
possible future busses drive the choice to void *.
Let me know, i can switch to unsigned int in case.
+{
+ struct axi_dac_state *st = iio_backend_get_priv(back);
+
+ if (!st->bus_type)
+ return -EOPNOTSUPP;
+
+ if (st->bus_type == AXI_DAC_BUS_TYPE_QSPI) {
It got mentioned in binding review but if this isn't QSPI, even
if similar don't call it that.
It's a bit difficult to find a different name, physically,
it is a QSPI, 4 lanes + clock + cs, and datasheet is naming it Quad SPI.
But looking the data protocol, it's a bit different.
QSPI has instruction, address and data.
Here we have just ADDR and DATA.
What about ADI_QSPI ?
Maybe use a switch from the start give it will make sense
anyway the moment there is a second bus type.
ok, used a switch in the write too.
I'd be tempted to factor the rest of this block out.
I guess expectation is we'll see more bus types so that factoring
out will be needed soon anyway.
+ int ret;
+ u32 bval;
u32 bval = 0;
+
+ if (size != 1 && size != 2)
+ return -EINVAL;
+
+ bval = 0;
+ ret = axi_dac_bus_reg_write(back,
+ AXI_DAC_RD_ADDR(reg), &bval, size);
Ugly wrap. Move more stuff on to first line.
ok done.
+ 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);
+ }
+
+ return -EINVAL;
+}
Thanks,
regards,
Angelo
--
,,, Angelo Dureghello
:: :. BayLibre -runtime team- Developer
:`___:
`____: