Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC

[Nuno Sá]

On Mon, 2024-07-08 at 17:05 +0100, Jonathan Cameron wrote:
> On Mon, 8 Jul 2024 05:17:55 +0000
> "Tinaco, Mariel" <Mariel.Tinaco@xxxxxxxxxx> wrote:
> 
> > > -----Original Message-----
> > > From: Jonathan Cameron <jic23@xxxxxxxxxx>
> > > Sent: Saturday, June 29, 2024 2:46 AM
> > > To: Tinaco, Mariel <Mariel.Tinaco@xxxxxxxxxx>
> > > Cc: linux-iio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> > > kernel@xxxxxxxxxxxxxxx; Lars-Peter Clausen <lars@xxxxxxxxxx>; Rob Herring
> > > <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> > > <conor+dt@xxxxxxxxxx>; Liam Girdwood <lgirdwood@xxxxxxxxx>; Mark Brown
> > > <broonie@xxxxxxxxxx>; Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>;
> > > Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx>; Dimitri Fedrau
> > > <dima.fedrau@xxxxxxxxx>; Guenter Roeck <linux@xxxxxxxxxxxx>
> > > Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC
> > > 
> > > [External]
> > >   
> > > > > > +};
> > > > > > +
> > > > > > +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev,
> > > > > > +				     const struct iio_chan_spec *chan) {
> > > > > > +	return 0;  
> > > > > 
> > > > > Why have the stubs in here?  
> > > > 
> > > > Should I move the stubs to a different place in the code or remove
> > > > them altogether since there is only a single powerdown mode available  
> > > Ah. I'd not really understood what was going on here.  This is fine as is.
> > >   
> > > > > AD8460_HVDAC_DATA_WORD_HIGH(index),  
> > > > > > +			    ((val >> 8) & 0xFF));  
> > > > > 
> > > > > bulk write? or do these need to be ordered?  
> > > > 
> > > > For this I used bulk read/write this way.
> > > > 
> > > > static int ad8460_set_hvdac_word(struct ad8460_state *state,
> > > > 				 int index,
> > > > 				 int val)
> > > > {
> > > > 	u8 regvals[AD8460_DATA_BYTE_WORD_LENGTH];  
> > > regmap bulk accesses (when spi anyway) should be provided with DMA safe
> > > buffers.
> > > Easiest way to do that is add one with __aligned(IIO_DMA_MINALIGN) to the
> > > end of the ad8460_state structure.  Possibly you'll need a lock to protect it -
> > > I
> > > haven't checked.  
> > > > 
> > > > 	regvals[0] = val & 0xFF;
> > > > 	regvals[1] = (val >> 8) & 0xFF;  
> > > 
> > > That is an endian conversion so use appropriate endian function to fill it
> > > efficiently and document clearly what is going on.
> > > 
> > > 
> > > 	put_unaligned_le16()
> > >   
> > > > 
> > > > 	return regmap_bulk_write(state->regmap,  
> > > AD8460_HVDAC_DATA_WORD_LOW(index),  
> > > > 				 regvals,  
> > > AD8460_DATA_BYTE_WORD_LENGTH); }  
> > > > 
> > > >  
> > > > > > +}  
> > >   
> > > > > > +	state->regmap = devm_regmap_init_spi(spi,
> > > > > > &ad8460_regmap_config);
> > > > > > +	if (IS_ERR(state->regmap))
> > > > > > +		return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
> > > > > > +				     "Failed to initialize regmap");
> > > > > > +
> > > > > > +	ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev,
> > > > > > +"tx",
> > > > > > +  
> > > > > IIO_BUFFER_DIRECTION_OUT);
> > > > > 
> > > > > Ah. I take back my binding comment. I assume this is mapping some
> > > > > non standard interface for the parallel data flow?  
> > > > 
> > > > Yes, the HDL side doesn't follow yet the standard IIO backend from
> > > > which this driver was tested  
> > > 
> > > Hmm. I'd like to see this brought inline with the other iio backend drivers if
> > > possible.  
> > 
> > Does this mean that we would need to implement an AXI IP core on the
> > FPGA side to be able to test this?
> 
> Don't think so.  That framework is meant to support any equivalent IP.
> So whatever you have should be supportable. Maybe it's somewhat of a stub
> driver though if there isn't anything controllable.
> 
> It's Nuno's area of expertise though +CC.
> 

Hi Jonathan,

Yeah, I did reply David (IIRC) about the very same question. In the design/HW Mariel
is working on the DAC is directly connected to the DMA core which is handled already
by a proper dma controller driver. So in this case I'm really not seeing the backend
need right now (maybe in the future we may have another design for this device that
could justify for a backend device but no idea on that).

As you mention, we could very well do a stub platform driver so we can use the
backend framework (like dma-backend or something) that could pretty much be a stub
for the DMA controller. But is it worth it though? We'd actually be "lying" in terms
of HW description as the DMA is a property of the actual converter.

- Nuno Sá