On Fri, 12 Jul 2024 08:57:00 +0200 Nuno Sá <noname.nuno@xxxxxxxxx> 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@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. 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á >