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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 12 Jul 2024 08:57:00 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> 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@analog.com> wrote:
> > > >   
> > > > > > -----Original Message-----
> > > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > > Sent: Saturday, June 29, 2024 2:46 AM
> > > > > > To: Tinaco, Mariel <Mariel.Tinaco@analog.com>
> > > > > > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > > > > > kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob
> > > > > > Herring
> > > > > > <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor
> > > > > > Dooley
> > > > > > <conor+dt@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown
> > > > > > <broonie@kernel.org>; Hennerich, Michael
> > > > > > <Michael.Hennerich@analog.com>;
> > > > > > Marcelo Schmitt <marcelo.schmitt1@gmail.com>; Dimitri Fedrau
> > > > > > <dima.fedrau@gmail.com>; Guenter Roeck <linux@roeck-us.net>
> > > > > > 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.

ok. So short term, DT bindings that document the dma-channel
and see what the DT maintainers think. Longer term, if things get more
complex, that can become optional and a backend added.

Jonathan

> 
> - Nuno Sá
>