RE: [PATCH v3 5/5] iio: dac: ltc2664: Add driver for LTC2664 and LTC2672

From: Paller, Kim Seer
Date: Thu Jun 06 2024 - 12:06:33 EST

Next message: Stefan Schaeckeler: "Re: [PATCH] igb: Fix igb_down hung on surprise removal"
Previous message: Dave Jiang: "Re: [PATCH] acpi,hmat: Use ACCESS_COORDINATE_CPU when appropriate"
In reply to: David Lechner: "Re: [PATCH v3 5/5] iio: dac: ltc2664: Add driver for LTC2664 and LTC2672"
Next in thread: David Lechner: "Re: [PATCH v3 5/5] iio: dac: ltc2664: Add driver for LTC2664 and LTC2672"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> > +struct ltc2664_state {
> > + struct spi_device *spi;
> > + struct regmap *regmap;
> > + struct ltc2664_chan channels[LTC2672_MAX_CHANNEL];
> > + /* lock to protect against multiple access to the device and shared data
> */
> > + struct mutex lock;
> > + const struct ltc2664_chip_info *chip_info;
> > + struct iio_chan_spec *iio_channels;
> > + int vref;
>
> vref_mv
>
> Always nice to have the units since regulators use µV and IIO uses mV.
> Otherwise we have to guess.
>
> > + u32 toggle_sel;
> > + u32 global_toggle;
>
> Should this be bool?
>
> > + u32 rfsadj;
>
> rfsadj_ohms
>
> > +};
> > +
> > +static const int ltc2664_span_helper[][2] = {
> > + { 0, 5000 },
> > + { 0, 10000 },
> > + { -5000, 5000 },
> > + { -10000, 10000 },
> > + { -2500, 2500 },
> > +};
> > +
> > +static const int ltc2672_span_helper[][2] = {
> > + { 0, 3125 },
> > + { 0, 6250 },
> > + { 0, 12500 },
> > + { 0, 25000 },
> > + { 0, 50000 },
> > + { 0, 100000 },
> > + { 0, 200000 },
> > + { 0, 300000 },
> > +};
> > +
> > +static int ltc2664_scale_get(const struct ltc2664_state *st, int c)
> > +{
> > + const struct ltc2664_chan *chan = &st->channels[c];
> > + const int (*span_helper)[2] = st->chip_info->span_helper;
> > + int span, fs;
> > +
> > + span = chan->span;
> > + if (span < 0)
> > + return span;
> > +
> > + fs = span_helper[span][1] - span_helper[span][0];
> > +
> > + return (fs / 2500) * st->vref;
>
> Should we multiply first and then divide? 3125 isn't divisible by 2500
> so there may be unwanted rounding otherwise.

Yes that would make sense, should perform the multiplication first,
then the division.

> > +}
> > +
> > +static int ltc2672_scale_get(const struct ltc2664_state *st, int c)
> > +{
> > + const struct ltc2664_chan *chan = &st->channels[c];
> > + int span, fs;
> > +
> > + span = chan->span;
> > + if (span < 0)
> > + return span;
> > +
> > + fs = 1000 * st->vref / st->rfsadj;
> > +
> > + if (span == LTC2672_MAX_SPAN)
> > + return 4800 * fs;
> > +
> > + return LTC2672_SCALE_MULTIPLIER(span) * fs;
>
> Are we losing accuracy by multiplying after dividing here as well?
>
> > +}
> > +
> > +static int ltc2664_offset_get(const struct ltc2664_state *st, int c)
> > +{
> > + const struct ltc2664_chan *chan = &st->channels[c];
> > + int span;
> > +
> > + span = chan->span;
> > + if (span < 0)
> > + return span;
> > +
> > + if (st->chip_info->span_helper[span][0] < 0)
> > + return -32768;
> > +
> > + return 0;
> > +}
> > +
> > +static int ltc2672_offset_get(const struct ltc2664_state *st, int c)
> > +{
> > + const struct ltc2664_chan *chan = &st->channels[c];
> > + int span;
> > +
> > + span = chan->span;
> > + if (span < 0)
> > + return span;
> > +
> > + if (st->chip_info->span_helper[span][1] < 0)
>
> Should this be span_helper[span][0]? [span][1] is always > 0.
>
> And for that matter, [span][0] is always 0 for ltc2672, so
> maybe we don't need this check at all?
>
> > + return -32768;
> > +
> > + return st->chip_info->span_helper[span][1] / 250;
>
> Why is this one not return 0 like the other chip?
>
> Figure 24 and 25 in the datasheet don't show an offset in
> the tranfer function.

I think I misinterpret page 25 of the datasheet about the offset. We
can make it return 0.

> > +}
> > +
> > +static int ltc2664_dac_code_write(struct ltc2664_state *st, u32 chan, u32
> input,
> > + u16 code)
> > +{
> > + struct ltc2664_chan *c = &st->channels[chan];
> > + int ret, reg;
> > +
> > + guard(mutex)(&st->lock);
> > + /* select the correct input register to write to */
> > + if (c->toggle_chan) {
> > + ret = regmap_write(st->regmap, LTC2664_CMD_TOGGLE_SEL,
> > + input << chan);
> > + if (ret)
> > + return ret;
> > + }
> > + /*
> > + * If in toggle mode the dac should be updated by an
> > + * external signal (or sw toggle) and not here.
> > + */
> > + if (st->toggle_sel & BIT(chan))
> > + reg = LTC2664_CMD_WRITE_N(chan);
> > + else
> > + reg = LTC2664_CMD_WRITE_N_UPDATE_N(chan);
> > +
> > + ret = regmap_write(st->regmap, reg, code);
> > + if (ret)
> > + return ret;
> > +
> > + c->raw[input] = code;
> > +
> > + if (c->toggle_chan) {
> > + ret = regmap_write(st->regmap, LTC2664_CMD_TOGGLE_SEL,
> > + st->toggle_sel);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int ltc2664_dac_code_read(struct ltc2664_state *st, u32 chan, u32
> input,
> > + u32 *code)
> > +{
> > + guard(mutex)(&st->lock);
> > + *code = st->channels[chan].raw[input];
> > +
> > + return 0;
> > +}
> > +
> > +static const int ltc2664_raw_range[] = {0, 1, U16_MAX};
> > +
> > +static int ltc2664_read_avail(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + const int **vals, int *type, int *length,
> > + long info)
> > +{
> > + switch (info) {
> > + case IIO_CHAN_INFO_RAW:
> > + *vals = ltc2664_raw_range;
> > + *type = IIO_VAL_INT;
> > +
> > + return IIO_AVAIL_RANGE;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int ltc2664_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan, int *val,
> > + int *val2, long info)
> > +{
> > + struct ltc2664_state *st = iio_priv(indio_dev);
> > + int ret;
> > +
> > + switch (info) {
> > + case IIO_CHAN_INFO_RAW:
> > + ret = ltc2664_dac_code_read(st, chan->channel,
> > + LTC2664_INPUT_A, val);
> > + if (ret)
> > + return ret;
> > +
> > + return IIO_VAL_INT;
> > + case IIO_CHAN_INFO_OFFSET:
> > + *val = st->chip_info->offset_get(st, chan->channel);
> > +
> > + return IIO_VAL_INT;
> > + case IIO_CHAN_INFO_SCALE:
> > + *val = st->chip_info->scale_get(st, chan->channel);
> > +
> > + *val2 = 16;
> > + return IIO_VAL_FRACTIONAL_LOG2;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int ltc2664_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan, int val,
> > + int val2, long info)
> > +{
> > + struct ltc2664_state *st = iio_priv(indio_dev);
> > +
> > + switch (info) {
> > + case IIO_CHAN_INFO_RAW:
> > + if (val > U16_MAX || val < 0)
> > + return -EINVAL;
> > +
> > + return ltc2664_dac_code_write(st, chan->channel,
> > + LTC2664_INPUT_A, val);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static ssize_t ltc2664_reg_bool_get(struct iio_dev *indio_dev,
> > + uintptr_t private,
> > + const struct iio_chan_spec *chan,
> > + char *buf)
> > +{
> > + struct ltc2664_state *st = iio_priv(indio_dev);
> > + u32 val;
> > +
> > + guard(mutex)(&st->lock);
> > + switch (private) {
> > + case LTC2664_POWERDOWN:
> > + val = st->channels[chan->channel].powerdown;
> > +
> > + return sysfs_emit(buf, "%u\n", val);
> > + case LTC2664_POWERDOWN_MODE:
> > + return sysfs_emit(buf, "42kohm_to_gnd\n");> + case
> LTC2664_TOGGLE_EN:
> > + val = !!(st->toggle_sel & BIT(chan->channel));
> > +
> > + return sysfs_emit(buf, "%u\n", val);
> > + case LTC2664_GLOBAL_TOGGLE:
> > + val = st->global_toggle;
> > +
> > + return sysfs_emit(buf, "%u\n", val);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static ssize_t ltc2664_reg_bool_set(struct iio_dev *indio_dev,
> > + uintptr_t private,
> > + const struct iio_chan_spec *chan,
> > + const char *buf, size_t len)
> > +{
> > + struct ltc2664_state *st = iio_priv(indio_dev);
> > + int ret;
> > + bool en;
> > +
> > + ret = kstrtobool(buf, &en);
> > + if (ret)
> > + return ret;
> > +
> > + guard(mutex)(&st->lock);
> > + switch (private) {
> > + case LTC2664_POWERDOWN:
> > + ret = regmap_write(st->regmap,
> > + en ?
> LTC2664_CMD_POWER_DOWN_N(chan->channel) :
> > + LTC2664_CMD_UPDATE_N(chan->channel),
> en);
> > + if (ret)
> > + return ret;
> > +
> > + st->channels[chan->channel].powerdown = en;
> > +
> > + return len;
> > + case LTC2664_TOGGLE_EN:
> > + if (en)
> > + st->toggle_sel |= BIT(chan->channel);
> > + else
> > + st->toggle_sel &= ~BIT(chan->channel);
> > +
> > + ret = regmap_write(st->regmap, LTC2664_CMD_TOGGLE_SEL,
> > + st->toggle_sel);
> > + if (ret)
> > + return ret;
> > +
> > + return len;
> > + case LTC2664_GLOBAL_TOGGLE:
> > + ret = regmap_write(st->regmap,
> LTC2664_CMD_GLOBAL_TOGGLE, en);
> > + if (ret)
> > + return ret;
> > +
> > + st->global_toggle = en;
> > +
> > + return len;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static ssize_t ltc2664_dac_input_read(struct iio_dev *indio_dev,
> > + uintptr_t private,
> > + const struct iio_chan_spec *chan,
> > + char *buf)
> > +{
> > + struct ltc2664_state *st = iio_priv(indio_dev);
> > + int ret;
> > + u32 val;
> > +
> > + if (private == LTC2664_INPUT_B_AVAIL)
> > + return sysfs_emit(buf, "[%u %u %u]\n", ltc2664_raw_range[0],
> > + ltc2664_raw_range[1],
> > + ltc2664_raw_range[2] / 4);
> > +
> > + ret = ltc2664_dac_code_read(st, chan->channel, private, &val);
> > + if (ret)
> > + return ret;
> > +
> > + return sysfs_emit(buf, "%u\n", val);
> > +}
> > +
> > +static ssize_t ltc2664_dac_input_write(struct iio_dev *indio_dev,
> > + uintptr_t private,
> > + const struct iio_chan_spec *chan,
> > + const char *buf, size_t len)
> > +{
> > + struct ltc2664_state *st = iio_priv(indio_dev);
> > + int ret;
> > + u16 val;
> > +
> > + if (private == LTC2664_INPUT_B_AVAIL)
> > + return -EINVAL;
> > +
> > + ret = kstrtou16(buf, 10, &val);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ltc2664_dac_code_write(st, chan->channel, private, val);
> > + if (ret)
> > + return ret;
> > +
> > + return len;
> > +}
> > +
> > +static int ltc2664_reg_access(struct iio_dev *indio_dev,
> > + unsigned int reg,
> > + unsigned int writeval,
> > + unsigned int *readval)
> > +{
> > + struct ltc2664_state *st = iio_priv(indio_dev);
> > +
> > + if (readval)
> > + return -EOPNOTSUPP;
> > +
> > + return regmap_write(st->regmap, reg, writeval);
> > +}
> > +
> > +#define LTC2664_CHAN_EXT_INFO(_name, _what, _shared, _read, _write) {
> \
> > + .name = _name, \
> > + .read = (_read), \
> > + .write = (_write), \
> > + .private = (_what), \
> > + .shared = (_shared), \
> > +}
> > +
> > +/*
> > + * For toggle mode we only expose the symbol attr (sw_toggle) in case a
> TGPx is
> > + * not provided in dts.
> > + */
> > +static const struct iio_chan_spec_ext_info ltc2664_toggle_sym_ext_info[] = {
> > + LTC2664_CHAN_EXT_INFO("raw0", LTC2664_INPUT_A, IIO_SEPARATE,
> > + ltc2664_dac_input_read,
> ltc2664_dac_input_write),
> > + LTC2664_CHAN_EXT_INFO("raw1", LTC2664_INPUT_B, IIO_SEPARATE,
> > + ltc2664_dac_input_read,
> ltc2664_dac_input_write),
> > + LTC2664_CHAN_EXT_INFO("powerdown", LTC2664_POWERDOWN,
> IIO_SEPARATE,
> > + ltc2664_reg_bool_get, ltc2664_reg_bool_set),
> > + LTC2664_CHAN_EXT_INFO("powerdown_mode",
> LTC2664_POWERDOWN_MODE,
> > + IIO_SEPARATE, ltc2664_reg_bool_get, NULL),
> > + LTC2664_CHAN_EXT_INFO("symbol", LTC2664_GLOBAL_TOGGLE,
> IIO_SEPARATE,
> > + ltc2664_reg_bool_get, ltc2664_reg_bool_set),
> > + LTC2664_CHAN_EXT_INFO("toggle_en", LTC2664_TOGGLE_EN,
> > + IIO_SEPARATE, ltc2664_reg_bool_get,
> > + ltc2664_reg_bool_set),
> > + { }
> > +};
> > +
> > +static const struct iio_chan_spec_ext_info ltc2664_ext_info[] = {
> > + LTC2664_CHAN_EXT_INFO("powerdown", LTC2664_POWERDOWN,
> IIO_SEPARATE,
> > + ltc2664_reg_bool_get, ltc2664_reg_bool_set),
> > + LTC2664_CHAN_EXT_INFO("powerdown_mode",
> LTC2664_POWERDOWN_MODE,
> > + IIO_SEPARATE, ltc2664_reg_bool_get, NULL),
> > + { }
> > +};
> > +
> > +#define LTC2664_CHANNEL(_chan) { \
> > + .indexed = 1, \
> > + .output = 1, \
> > + .channel = (_chan), \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_SCALE) | \
> > + BIT(IIO_CHAN_INFO_OFFSET) | BIT(IIO_CHAN_INFO_RAW),
> \
> > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW),
> \
> > + .ext_info = ltc2664_ext_info, \
> > +}
> > +
> > +static const struct iio_chan_spec ltc2664_channels[] = {
> > + LTC2664_CHANNEL(0),
> > + LTC2664_CHANNEL(1),
> > + LTC2664_CHANNEL(2),
> > + LTC2664_CHANNEL(3),
> > +};
> > +
> > +static const struct iio_chan_spec ltc2672_channels[] = {
> > + LTC2664_CHANNEL(0),
> > + LTC2664_CHANNEL(1),
> > + LTC2664_CHANNEL(2),
> > + LTC2664_CHANNEL(3),
> > + LTC2664_CHANNEL(4),
> > +};
>
> Do we really need these since they are only used as a template anyway?
> We could just have a single template for one channel and copy it as
> manay times as needed.

Yes, from what I can see we need separate channel specs for both devices
since they have a differing number of channels. As for your suggestion about
having a single template for one channel and copying it as many times as
needed, I'm not entirely sure how to implement it in this context. Could you
provide something like a code snippet to illustrate this?

> > +
> > +static const struct ltc2664_chip_info ltc2664_chip = {
> > + .id = LTC2664,
> > + .name = "ltc2664",
> > + .scale_get = ltc2664_scale_get,
> > + .offset_get = ltc2664_offset_get,
> > + .measurement_type = IIO_VOLTAGE,
> > + .num_channels = ARRAY_SIZE(ltc2664_channels),
> > + .iio_chan = ltc2664_channels,
> > + .span_helper = ltc2664_span_helper,
> > + .num_span = ARRAY_SIZE(ltc2664_span_helper),
> > + .internal_vref = 2500,
> > + .manual_span_support = true,
> > + .rfsadj_support = false,
> > +};
> > +

Next message: Stefan Schaeckeler: "Re: [PATCH] igb: Fix igb_down hung on surprise removal"
Previous message: Dave Jiang: "Re: [PATCH] acpi,hmat: Use ACCESS_COORDINATE_CPU when appropriate"
In reply to: David Lechner: "Re: [PATCH v3 5/5] iio: dac: ltc2664: Add driver for LTC2664 and LTC2672"
Next in thread: David Lechner: "Re: [PATCH v3 5/5] iio: dac: ltc2664: Add driver for LTC2664 and LTC2672"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]