Message-ID: 
 <PH0PR03MB7141405D9A40A18E1C90FF79F9FA2@PH0PR03MB7141.namprd03.prod.outlook.com>
Date: Thu, 6 Jun 2024 15:49:00 +0000
From: "Paller, Kim Seer" <KimSeer.Paller@...log.com>
To: David Lechner <dlechner@...libre.com>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org"
	<linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>
CC: Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>,
        Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
        Dimitri
 Fedrau <dima.fedrau@...il.com>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Rob
 Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
        "Hennerich,
 Michael" <Michael.Hennerich@...log.com>,
        Nuno Sá
	<noname.nuno@...il.com>,
        kernel test robot <lkp@...el.com>
Subject: RE: [PATCH v3 5/5] iio: dac: ltc2664: Add driver for LTC2664 and
 LTC2672


> > +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,
> > +};
> > +

Powered by blists - more mailing lists