[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240628194546.2f608365@jic23-huawei>
Date: Fri, 28 Jun 2024 19:45:46 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: "Tinaco, Mariel" <Mariel.Tinaco@...log.com>
Cc: "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Lars-Peter
Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>, Krzysztof
Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Liam
Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
"Hennerich, Michael" <Michael.Hennerich@...log.com>, Marcelo Schmitt
<marcelo.schmitt1@...il.com>, Dimitri Fedrau <dima.fedrau@...il.com>,
Guenter Roeck <linux@...ck-us.net>
Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC
> > > +};
> > > +
> > > +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.
Jonathan
Powered by blists - more mailing lists