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

[Nuno Sá]

On Thu, 2024-07-11 at 16:31 -0500, David Lechner wrote:
> On 7/11/24 4:20 AM, Nuno Sá wrote:
> > 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á
> > 
> > 
> 
> I'm a bit inclined to agree with Jonathan here. I could see someone in the
> future,
> wanting to, e.g., use DMA + a GPIO controller for the parallel interface if
> they
> didn't have an FPGA. So it seems a bit more future-proof to just always use
> the
> IIO backend framework for the parallel interface.

I do agree it's more future but guessing usecases is not something I tend to
like much (often just results in code that never gets __really__ used). We can
very well take care of it when a usecase pops up and we have an actual device
that can be represented by a backend :).

> 
> FWIW, I don't think it would be "lying" since the io-backend DT node would be
> representing physical parallel bus between the DMA controller and the ADC
> chip.

To me, it's really a stretch having a backend with the only reason (op) of
requesting the DMA channel. I still think you're pushing to much and going
around with wording to justify for the DMA property :). The parallel bus is part
of the DAC and directly connects to the DMA data lines so it really looks to me
the dma is a property of the actual DAC.

That said and Mariel can help here, I did not really looked into the design
myself and I'm just stating (or what I understood) what Mariel told me. But if
there's some other piece of HW sitting between the DMA and the bus then it would
be easier for me to agree even if we don't have any real control over that
device.

> But if DT maintainers are OK with the idea that a DMA channel can be directly
> wired to an external chip, I guess I won't complain. :-)

That's the case in here so I don't see why it should be a problem :). It's the
same with the axi-dac/adc. It's all inside the FPGA but different cores/IPS.

FWIW, I'm ok if we go the backend direction even if I don't fully agree with it
(at least with the understanding I have so far about the design). I definitely
want to see more users of it but I also don't think it should be a rule for any
fairly high speed converter to have a backend associated.

- Nuno Sá