> -----Original Message-----
> From: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> Sent: Wednesday, December 15, 2021 2:41 PM
> To: Lars-Peter Clausen <lars@xxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx
> Cc: Jonathan Cameron <jic23@xxxxxxxxxx>; Rob Herring
> <robh+dt@xxxxxxxxxx>; Hennerich, Michael
> <Michael.Hennerich@xxxxxxxxxx>
> Subject: RE: [PATCH 1/3] iio: dac: add support for ltc2688
>
> [External]
>
> Hi Lars,
>
> Thanks for the review!
>
> > From: Lars-Peter Clausen <lars@xxxxxxxxxx>
> > Sent: Wednesday, December 15, 2021 11:24 AM
> > To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx
> > Cc: Jonathan Cameron <jic23@xxxxxxxxxx>; Rob Herring
> > <robh+dt@xxxxxxxxxx>; Hennerich, Michael
> > <Michael.Hennerich@xxxxxxxxxx>
> > 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á
