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. 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. 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. :-)