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?
> > + .len = reg_size + 2,
> > + .cs_change = 1,
> > + }, {
> > + .tx_buf = st->tx_data,
> > + .rx_buf = st->rx_data,
> > + .bits_per_word = 8,
> > + .len = 3,
> > + },
> > + };
> > + int ret;
> > +
> > + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > + if (ret)
> > + return ret;
> > +
> > + memcpy(val, &st->rx_data[1], val_size);
> > +
> > + return 0;
> > +}
> > [...]
> > +
> > +static int ltc2688_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan, int val,
> > + int val2, long mask)
> Using mask for the variable name is a relic from the days when it used
> to be a mask. For new drivers it is better to use `info`. Same for the
> other functions.
Got it. Probably copy paste...
> > [...]
> > +
> > +static const char * const ltc2688_dither_phase[] = {
> > + "0", "90", "180", "270",
> > +};
> Usually we use radians for phase values. Although that would make for
> a
> bit of an awkward API in this case.
> > +
> > [...]
> > +/*
> > + * For toggle mode we only expose the symbol attr (sw_toggle) in
> case a TGPx is
> > + * not provided in dts.
> > + */
> > +#define LTC2688_CHAN_TOGGLE(t, name) ({
> \
> > + static const struct iio_chan_spec_ext_info t ## _ext_info[] = {
> \
> > + LTC2688_CHAN_EXT_INFO("raw1", LTC2688_INPUT_B,
> IIO_SEPARATE), \
> > + LTC2688_CHAN_EXT_INFO("toggle_en",
> LTC2688_DITHER_TOGGLE_ENABLE, \
> > + IIO_SEPARATE),
> \
> > + LTC2688_CHAN_EXT_INFO("powerdown",
> LTC2688_POWERDOWN, IIO_SEPARATE), \
> > + LTC2688_CHAN_EXT_INFO(name,
> LTC2688_SW_TOGGLE, IIO_SEPARATE), \
> > + {}
> \
> > + };
> \
> > + t ## _ext_info;
> \
> > +})
>
> This macro is a bit strange since it declares a static, but is called in
> a function. It might be better to declare the two types of ext_infos
> statically and then reference them by name from within the function.
Yeah, I also though about that. In the end, the result is the same. I ended
up with this just to save some lines but I'm totally fine with your approach.
It actually makes the code easier to read.
> > [...]
> > +
> > +static int ltc2688_tgp_setup(struct ltc2688_state *st, long clk_mask,
> > + const struct ltc2688_dither_helper *tgp)
> > +{
> > + int ret, bit;
> > +
> > + for_each_set_bit(bit, &clk_mask, LTC2688_CHAN_TD_MAX) {
> clk_mask should be unsigned long
> > [...]
> > +
> > +static int ltc2688_span_lookup(const struct ltc2688_state *st, int
> min, int max)
> > +{
> > + u32 span;
> Nit: Why u32 and not unsigned int? The size doesn't seem to be
> important
> for the loop variable.
Shorter to type :)
> > +
> > + for (span = 0; span < ARRAY_SIZE(ltc2688_span_helper);
> span++) {
> > + if (min == ltc2688_span_helper[span][0] &&
> > + max == ltc2688_span_helper[span][1])
> > + return span;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int ltc2688_channel_config(struct ltc2688_state *st)
> > +{
> > + struct fwnode_handle *fwnode = dev_fwnode(&st->spi-
> >dev), *child;
> > + struct ltc2688_dither_helper tgp[LTC2688_CHAN_TD_MAX] =
> {0};
> > + u32 reg, clk_input, val, mask, tmp[2];
> > + unsigned long clk_msk = 0;
> > + int ret, span;
> > +
> > + fwnode_for_each_available_child_node(fwnode, child) {
> > [...]
> > + chan = &st->channels[reg];
> > + if (fwnode_property_read_bool(child, "adi,toggle-
> mode")) {
> > + chan->toggle_chan = true;
> > + /* assume sw toggle ABI */
> > + ltc2688_channels[reg].ext_info =
> LTC2688_CHAN_TOGGLE(__s, "symbol");
> Updating ltc2688_channels at runtime will break if you have multiple
> instances of the device with a different configuration. You need to
> kmemdup() the channel array.
Arghh, yeah (dummy mistake)! You're right! Will fix it in v2...
> > + }
> > +[...]
> > + return ltc2688_tgp_setup(st, clk_msk, tgp);
> > +}
> > +
> > +static int ltc2688_setup(struct ltc2688_state *st, struct regulator
> *vref)
> > +{
> > + struct gpio_desc *gpio;
> > + int ret;
> > +
> > + /*
> > + * If we have a reset pin, use that to reset the board, If not, use
> > + * the reset bit.
> > + */
> Looking at the datasheet I do not see a reset pin on the chip.
IIRC, it's called CLR... But looking at it again if feels like a reset pin but
without directly saying so in the datasheet.
> > + gpio = devm_gpiod_get_optional(&st->spi->dev, "reset",
> GPIOD_OUT_HIGH);
> Usually when we have a reset which is active low we define it in the DT
> as active low rather than doing the inversion in the driver.
And that's how I tested it in dts. The ' GPIOD_OUT_HIGH' is to request
it in the asserted state and then we just have to de-assert it to take it
out of reset. It's actually the same pattern used in the adis lib. IIRC,
you were actually the one to suggest this :)
> > + if (IS_ERR(gpio))
> > + return dev_err_probe(&st->spi->dev, PTR_ERR(gpio),
> > + "Failed to get reset gpio");
> > + if (gpio) {
> > + usleep_range(1000, 1200);
> > + /* bring device out of reset */
> > + gpiod_set_value_cansleep(gpio, 0);
> > + } else {
> > + ret = regmap_update_bits(st->regmap,
> LTC2688_CMD_CONFIG,
> > + LTC2688_CONFIG_RST,
> > + LTC2688_CONFIG_RST);
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + usleep_range(10000, 12000);
> > +
> > + ret = ltc2688_channel_config(st);
> > + if (ret)
> > + return ret;
> > +
> > + if (!vref)
> > + return 0;
> > +
> > + return regmap_update_bits(st->regmap,
> LTC2688_CMD_CONFIG,
> > + LTC2688_CONFIG_EXT_REF, BIT(1));
>
> This is a bit confusing since you are using LTC2688_CONFIG_EXT_REF
> for
> the mask and BIT(1) for the value, even though both are the same.
I tried to be more or less consistent. So, for masks I used a define and
for the actually value I used the "raw" BIT, FIELD_PREP, FIELD_GET as
I think Jonathan prefers that way. If that's also the preferred way for masks,
I'm happy to update it.
> There is a new API regmap_set_bits()/regmap_clear_bits() that allows
> you
> to write this in a more compact way. There are a few other places in
> the
> driver where they can be used as well.
Hmm, will look at the new API...
- Nuno Sá
