> -----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? > > Jonathan >