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
