Re: [PATCH RFC 3/8] iio: backend adi-axi-dac: backend features

[Jonathan Cameron <jic23@kernel.org>]

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