Re: [PATCH RFC 3/8] iio: backend adi-axi-dac: backend features
From: Angelo Dureghello
Date: Thu Sep 05 2024 - 08:00:16 EST
Hi,
On 05/09/24 12:49 PM, Nuno Sá wrote:
On Tue, 2024-09-03 at 20:16 +0100, Jonathan Cameron wrote:
On Mon, 2 Sep 2024 18:04:51 +0200
Angelo Dureghello <adureghello@xxxxxxxxxxxx> wrote:
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.
I doubt it will get bigger than u64. Passing void * is always
nasty if we can do something else and this is a register writing
operation. I'm yet to meet an ADC or similar with > 64 bit registers
(or even one with 64 bit ones!)
I think the original thinking was to support thinks like appending crc to the
register read/write. But even in that case, u32 for val might be enough. Not
sure. Anyways, as you often say with the backend stuff, this is all in the
kernel so I guess we can change it to unsigned int and change it in the future
if we need to.
Since you mentioned regmap, I also want to bring something that was discussed
before the RFC. Basically we talked about having the backend registering it's
own regmap_bus. Then we would either:
1) Have a specific get_regmap_bus() callback for the frontend to initialize a
regmap on;
2) Pass this bus into the core and have a new frontend API like
devm_iio_backend_regmap_init().
Then, on top of the API already provided by regmap (like _update_bit()), the
frontend could just use regmap independent of having a backend or not.
The current API is likely more generic but tbh (and David and Angelo are aware
of it) my preferred approach it to use the regmap_bus stuff. I just don't feel
that strong about it :)
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.
I would just go with unsigned int or at a push u64 *
+{
+ 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.
is QSPI actually defined anywhere? I assumed it would be like
SPI for which everything is so flexible you can build whatever you like.
QSPI has instruction, address and data.
Here we have just ADDR and DATA.
I'm not sure the instruction is really relevant for this. From a quick look, it
feels like something used for accessing external flash memory like spi-nors. So,
I would not be surprised if things are just like Jonathan said and this is just
flexible as spi (being that extra instruction field a protocol defined for flash
memory - where one typically sees this interface)
Ok, so QSPI is the hardware, and the protocol on it may vary for the target
chip/application.
Looks like DDR makes the 33MUPS rate reachable, and not all the controllers
have DDR mode. Also some controllers are supposed to work with a QSPI flash
(so with instructions), and likely this reason driven the need to use a
custom IP.
Regards,
Angelo
- Nuno Sá
--
,,, Angelo Dureghello
:: :. BayLibre -runtime team- Developer
:`___:
`____: