RE: [PATCH 1/3] iio: dac: add support for ltc2688

["Sa, Nuno" <Nuno.Sa@analog.com>]

> -----Original Message-----
> From: Sa, Nuno <Nuno.Sa@analog.com>
> Sent: Wednesday, December 15, 2021 2:41 PM
> To: Lars-Peter Clausen <lars@metafoo.de>; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org
> Cc: Jonathan Cameron <jic23@kernel.org>; Rob Herring
> <robh+dt@kernel.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>
> Subject: RE: [PATCH 1/3] iio: dac: add support for ltc2688
> 
> [External]
> 
> Hi Lars,
> 
> Thanks for the review!
> 
> > From: Lars-Peter Clausen <lars@metafoo.de>
> > Sent: Wednesday, December 15, 2021 11:24 AM
> > To: Sa, Nuno <Nuno.Sa@analog.com>; linux-iio@vger.kernel.org;
> > devicetree@vger.kernel.org
> > Cc: Jonathan Cameron <jic23@kernel.org>; Rob Herring
> > <robh+dt@kernel.org>; Hennerich, Michael
> > <Michael.Hennerich@analog.com>
> > Subject: Re: [PATCH 1/3] iio: dac: add support for ltc2688
> > >
> > On 12/14/21 5:56 PM, Nuno Sá wrote:
> > > The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated
> > > precision reference. It is guaranteed monotonic and has built in
> > > rail-to-rail output buffers that can source or sink up to 20 mA.
> >
> > Looks very good!
> >
> > Although I'm not sure what to make of the `raw1` API. Maybe it
> makes
> > sense to submit an initial version of this driver without the toggle
> > API. And then have a follow up discussion how to define the API for
> > this. This will not be the only DAC that has this feature so it would be
> > a good idea to come up with a common API.
> 
> This was discussed in the RFC. The raw1 refers to the second code
> on the DAC output path so we can toggle between raw and raw1. The
> feeling I got from HW guys internally is that this is an important feature
> to support. That said, I understand that this being ABI is always
> sensitive
> stuff but ultimately, I honestly think that raw1 fits this feature.
> 
> The thing that is making more reluctant about toggle mode is not so
> much the ABI but forcing a clock to be used when a toggle channel is
> mapped to a TGPx pin...
> 
> >
> > >
> > > [...]
> > > +
> > > +static int ltc2688_spi_read(void *context, const void *reg, size_t
> > reg_size,
> > > +			    void *val, size_t val_size)
> > > +{
> > > +	struct ltc2688_state *st = context;
> > > +	struct spi_transfer xfers[] = {
> > > +		{
> > > +			.tx_buf = reg,
> > > +			.bits_per_word = 8,
> > > +			/*
> > > +			 * This means that @val will also be part of the
> > > +			 * transfer as there's no pad bits. That's fine as
> > these
> > > +			 * bits are don't care for the device and we fill
> > > +			 * @val with the proper value afterwards. Using
> > regmap
> > > +			 * pad bits to get reg_size right would just make
> > the
> > > +			 * write part more cumbersome than this...
> > > +			 */
> > This is making assumptions about the memory layout in the regmap
> > core.
> > This could change in the future and then this driver breaks. It is
> > better to not assume that reg is part of a larger buffer.
> 
> True, but I think reg_size should not really change as we define it in
> our regmap_config. Are you suggesting to just use plain 3 here?

Ahhh I get what you mean. So you are saying to just use a bounce buffer
(that actually holds space for 3 bytes) in our state struct and just copy reg
onto it? That makes sense...

- Nuno Sá