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!) > 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. > > What about ADI_QSPI ? Sure, that is fine if we worry about differences from qspi (which depends on there being a reference spec!) Jonathan