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

[Jonathan Cameron]

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.

> 
> > 
> > Jonathan
> >   
> 
>