| Message ID | 20240829-wip-bl-ad3552r-axi-v0-v1-3-b6da6015327a@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: dac: introducing ad3552r-axi | expand |
On Thu, 29 Aug 2024 14:32:01 +0200 Angelo Dureghello <adureghello@baylibre.com> wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > Extend DAC backend with new features required for the AXI driver > version for the a3552r DAC. > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> 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. > + 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. > +{ > + 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. > + 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. > + } > + > + 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. > +{ > + 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. Maybe use a switch from the start give it will make sense anyway the moment there is a second bus type. 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. > + 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; > +}
On 31/08/24 1:34 PM, Jonathan Cameron wrote: > On Thu, 29 Aug 2024 14:32:01 +0200 > Angelo Dureghello <adureghello@baylibre.com> wrote: > >> From: Angelo Dureghello <adureghello@baylibre.com> >> >> Extend DAC backend with new features required for the AXI driver >> version for the a3552r DAC. >> >> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > 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
On Mon, 2 Sep 2024 18:04:51 +0200 Angelo Dureghello <adureghello@baylibre.com> wrote: > On 31/08/24 1:34 PM, Jonathan Cameron wrote: > > On Thu, 29 Aug 2024 14:32:01 +0200 > > Angelo Dureghello <adureghello@baylibre.com> wrote: > > > >> From: Angelo Dureghello <adureghello@baylibre.com> > >> > >> Extend DAC backend with new features required for the AXI driver > >> version for the a3552r DAC. > >> > >> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > 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
On Tue, 2024-09-03 at 20:16 +0100, Jonathan Cameron wrote: > On Mon, 2 Sep 2024 18:04:51 +0200 > Angelo Dureghello <adureghello@baylibre.com> wrote: > > > On 31/08/24 1:34 PM, Jonathan Cameron wrote: > > > On Thu, 29 Aug 2024 14:32:01 +0200 > > > Angelo Dureghello <adureghello@baylibre.com> wrote: > > > > > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > > > Extend DAC backend with new features required for the AXI driver > > > > version for the a3552r DAC. > > > > > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > > 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) - Nuno Sá >
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@baylibre.com> wrote: >> >>> On 31/08/24 1:34 PM, Jonathan Cameron wrote: >>>> On Thu, 29 Aug 2024 14:32:01 +0200 >>>> Angelo Dureghello <adureghello@baylibre.com> wrote: >>>> >>>>> From: Angelo Dureghello <adureghello@baylibre.com> >>>>> >>>>> Extend DAC backend with new features required for the AXI driver >>>>> version for the a3552r DAC. >>>>> >>>>> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> >>>> 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á
Hi, sorry forgot to reply about the regmap, 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@baylibre.com> wrote: >> >>> On 31/08/24 1:34 PM, Jonathan Cameron wrote: >>>> On Thu, 29 Aug 2024 14:32:01 +0200 >>>> Angelo Dureghello <adureghello@baylibre.com> wrote: >>>> >>>>> From: Angelo Dureghello <adureghello@baylibre.com> >>>>> >>>>> Extend DAC backend with new features required for the AXI driver >>>>> version for the a3552r DAC. >>>>> >>>>> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> >>>> 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 :) regmap idea seems really nice and a better style. Honestly, if possible, would not go for it right now. The main reason is that i am on this work from months and it would require a quite big rework (also rearranging more common code, retest, etc) while i am trying to finalize a first driver. If you agree, this could come in a second "cleanup" patchset, but at least i can provide an initial support for ad3552r. >>> 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) > > - Nuno Sá regards, Angelo
On Thu, 2024-09-05 at 14:11 +0200, Angelo Dureghello wrote: > Hi, > > sorry forgot to reply about the regmap, > > 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@baylibre.com> wrote: > > > > > > > On 31/08/24 1:34 PM, Jonathan Cameron wrote: > > > > > On Thu, 29 Aug 2024 14:32:01 +0200 > > > > > Angelo Dureghello <adureghello@baylibre.com> wrote: > > > > > > > > > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > > > > > > > Extend DAC backend with new features required for the AXI driver > > > > > > version for the a3552r DAC. > > > > > > > > > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > > > > 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 :) > > regmap idea seems really nice and a better style. > > Honestly, if possible, would not go for it right now. > The main reason is that i am on this work from months and it would > require a quite > big rework (also rearranging more common code, retest, etc) while i am > trying to > finalize a first driver. > While I understand your reasoning, I can't really agree with it if we feel regmap is the better solution. It makes no sense to add something knowing that it will removed in the next couple of weeks. Actually (and I'm guilty of that too :)), when we say things like that, odds are we're just leaving things like this. > If you agree, this could come in a second "cleanup" patchset, but at > least i can > provide an initial support for ad3552r. > Having said the above, I'm not going to NAK this approach even if it's not my favorite one :) - Nuno Sá > > >
On Thu, 2024-09-05 at 13:58 +0200, Angelo Dureghello wrote: > 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@baylibre.com> wrote: > > > > > > > On 31/08/24 1:34 PM, Jonathan Cameron wrote: > > > > > On Thu, 29 Aug 2024 14:32:01 +0200 > > > > > Angelo Dureghello <adureghello@baylibre.com> wrote: > > > > > > > > > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > > > > > > > Extend DAC backend with new features required for the AXI driver > > > > > > version for the a3552r DAC. > > > > > > > > > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > > > > 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. > I do understand the custom IP, I just don't understand why not using the spi_engine IP. Indeed maybe because of DDR (as that is already supported on axi-dac). - Nuno Sá
diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c index 0cb00f3bec04..395f222e254d 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 ADI_DAC_R1_MODE BIT(4) +#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,20 @@ #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_16 = 11, +}; + +enum { + AXI_DAC_BUS_TYPE_NONE, + AXI_DAC_BUS_TYPE_QSPI, }; struct axi_dac_state { @@ -80,6 +112,7 @@ struct axi_dac_state { u64 dac_clk; u32 reg_config; bool int_tone; + int bus_type; }; 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); + 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_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); + + switch (mask) { + case IIO_CHAN_INFO_FREQUENCY: + *val = clk_get_rate(devm_clk_get(st->dev, 0)); + + return IIO_VAL_INT; + default: + return -EINVAL; + } +} + +static int axi_dac_bus_reg_write(struct iio_backend *back, + u32 reg, void *val, size_t size) +{ + 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) { + 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; + } + + 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) +{ + 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) { + int ret; + u32 bval; + + if (size != 1 && size != 2) + return -EINVAL; + + bval = 0; + ret = axi_dac_bus_reg_write(back, + AXI_DAC_RD_ADDR(reg), &bval, 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); + } + + return -EINVAL; +} + 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 +762,16 @@ 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, + .bus_reg_read = axi_dac_bus_reg_read, + .bus_reg_write = axi_dac_bus_reg_write, .debugfs_reg_access = iio_backend_debugfs_ptr(axi_dac_reg_access), }; @@ -576,6 +820,8 @@ static int axi_dac_probe(struct platform_device *pdev) return dev_err_probe(&pdev->dev, PTR_ERR(st->regmap), "failed to init register map\n"); + device_property_read_u32(st->dev, "bus-type", &st->bus_type); + /* * Force disable the core. Up to the frontend to enable us. And we can * still read/write registers...