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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: David Lechner <dlechner@xxxxxxxxxxxx>
> Sent: Tuesday, June 4, 2024 4:43 AM
> To: Paller, Kim Seer <KimSeer.Paller@xxxxxxxxxx>; linux-
> kernel@xxxxxxxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Cc: Jonathan Cameron <jic23@xxxxxxxxxx>; Lars-Peter Clausen
> <lars@xxxxxxxxxx>; Liam Girdwood <lgirdwood@xxxxxxxxx>; Mark Brown
> <broonie@xxxxxxxxxx>; Dimitri Fedrau <dima.fedrau@xxxxxxxxx>; Krzysztof
> Kozlowski <krzk+dt@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Conor
> Dooley <conor+dt@xxxxxxxxxx>; Hennerich, Michael
> <Michael.Hennerich@xxxxxxxxxx>; Nuno Sá <noname.nuno@xxxxxxxxx>;
> kernel test robot <lkp@xxxxxxxxx>
> Subject: Re: [PATCH v3 5/5] iio: dac: ltc2664: Add driver for LTC2664 and
> LTC2672
> 
> [External]
> 
> On 6/2/24 8:22 PM, Kim Seer Paller wrote:
> > LTC2664 4 channel, 16 bit Voltage Output SoftSpan DAC
> > LTC2672 5 channel, 16 bit Current Output Softspan DAC
> >
> > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > Closes: https://urldefense.com/v3/__https://lore.kernel.org/oe-kbuild-
> all/202405241141.kYcxrSem-
> lkp@xxxxxxxxx/__;!!A3Ni8CS0y2Y!65MPSYKyqFizjs_tSxpABl45BNKqWutx4NOBi
> EkmmmY8kJkwep-27ON2rqne-XnUId2M3KsqgGbQy7GI_aYi9Tg$
> > Co-developed-by: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
> > Signed-off-by: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
> > Signed-off-by: Kim Seer Paller <kimseer.paller@xxxxxxxxxx>
> > ---
> >  MAINTAINERS               |   1 +
> >  drivers/iio/dac/Kconfig   |  11 +
> >  drivers/iio/dac/Makefile  |   1 +
> >  drivers/iio/dac/ltc2664.c | 806 ++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 819 insertions(+)
> >  create mode 100644 drivers/iio/dac/ltc2664.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ac1e29e26f31..1262e1231923 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -13071,6 +13071,7 @@ S:	Supported
> >  W:	https://ez.analog.com/linux-software-drivers
> >  F:	Documentation/devicetree/bindings/iio/dac/adi,ltc2664.yaml
> >  F:	Documentation/devicetree/bindings/iio/dac/adi,ltc2672.yaml
> > +F:	drivers/iio/dac/ltc2664.c
> >
> >  LTC2688 IIO DAC DRIVER
> >  M:	Nuno Sá <nuno.sa@xxxxxxxxxx>
> > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> > index 3c2bf620f00f..3d065c157605 100644
> > --- a/drivers/iio/dac/Kconfig
> > +++ b/drivers/iio/dac/Kconfig
> > @@ -370,6 +370,17 @@ config LTC2632
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called ltc2632.
> >
> > +config LTC2664
> > +	tristate "Analog Devices LTC2664 and LTC2672 DAC SPI driver"
> > +	depends on SPI
> > +	select REGMAP
> > +	help
> > +	  Say yes here to build support for Analog Devices
> > +	  LTC2664 and LTC2672 converters (DAC).
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called ltc2664.
> > +
> >  config M62332
> >  	tristate "Mitsubishi M62332 DAC driver"
> >  	depends on I2C
> > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> > index 8432a81a19dc..2cf148f16306 100644
> > --- a/drivers/iio/dac/Makefile
> > +++ b/drivers/iio/dac/Makefile
> > @@ -37,6 +37,7 @@ obj-$(CONFIG_DS4424) += ds4424.o
> >  obj-$(CONFIG_LPC18XX_DAC) += lpc18xx_dac.o
> >  obj-$(CONFIG_LTC1660) += ltc1660.o
> >  obj-$(CONFIG_LTC2632) += ltc2632.o
> > +obj-$(CONFIG_LTC2664) += ltc2664.o
> >  obj-$(CONFIG_LTC2688) += ltc2688.o
> >  obj-$(CONFIG_M62332) += m62332.o
> >  obj-$(CONFIG_MAX517) += max517.o
> > diff --git a/drivers/iio/dac/ltc2664.c b/drivers/iio/dac/ltc2664.c
> > new file mode 100644
> > index 000000000000..ef5d7d6fec5a
> > --- /dev/null
> > +++ b/drivers/iio/dac/ltc2664.c
> > @@ -0,0 +1,806 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * LTC2664 4 channel, 16 bit Voltage Output SoftSpan DAC driver
> > + * LTC2672 5 channel, 16 bit Current Output Softspan DAC driver
> > + *
> > + * Copyright 2024 Analog Devices Inc.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/device.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/mutex.h>
> > +#include <linux/property.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#define LTC2664_CMD_WRITE_N(n)		(0x00 + (n))
> > +#define LTC2664_CMD_UPDATE_N(n)		(0x10 + (n))
> > +#define LTC2664_CMD_WRITE_N_UPDATE_ALL	0x20
> > +#define LTC2664_CMD_WRITE_N_UPDATE_N(n)	(0x30 + (n))
> > +#define LTC2664_CMD_POWER_DOWN_N(n)	(0x40 + (n))
> > +#define LTC2664_CMD_POWER_DOWN_ALL	0x50
> > +#define LTC2664_CMD_SPAN_N(n)		(0x60 + (n))
> > +#define LTC2664_CMD_CONFIG		0x70
> > +#define LTC2664_CMD_MUX			0xB0
> > +#define LTC2664_CMD_TOGGLE_SEL		0xC0
> > +#define LTC2664_CMD_GLOBAL_TOGGLE	0xD0
> > +#define LTC2664_CMD_NO_OPERATION	0xF0
> > +#define LTC2664_REF_DISABLE		0x0001
> > +#define LTC2664_MSPAN_SOFTSPAN		7
> > +
> > +#define LTC2672_MAX_CHANNEL		5
> > +#define LTC2672_MAX_SPAN		7
> > +#define LTC2672_SCALE_MULTIPLIER(n)	(50 * BIT(n))
> > +
> > +enum ltc2664_ids {
> > +	LTC2664,
> > +	LTC2672,
> > +};
> > +
> > +enum {
> > +	LTC2664_SPAN_RANGE_0V_5V,
> > +	LTC2664_SPAN_RANGE_0V_10V,
> > +	LTC2664_SPAN_RANGE_M5V_5V,
> > +	LTC2664_SPAN_RANGE_M10V_10V,
> > +	LTC2664_SPAN_RANGE_M2V5_2V5,
> > +};
> > +
> > +enum {
> > +	LTC2664_INPUT_A,
> > +	LTC2664_INPUT_B,
> > +	LTC2664_INPUT_B_AVAIL,
> > +	LTC2664_POWERDOWN,
> > +	LTC2664_POWERDOWN_MODE,
> > +	LTC2664_TOGGLE_EN,
> > +	LTC2664_GLOBAL_TOGGLE,
> > +};
> > +
> > +static const u16 ltc2664_mspan_lut[8][2] = {
> > +	{ LTC2664_SPAN_RANGE_M10V_10V, 32768 }, /* MPS2=0, MPS1=0,
> MSP0=0 (0)*/
> > +	{ LTC2664_SPAN_RANGE_M5V_5V, 32768 }, /* MPS2=0, MPS1=0,
> MSP0=1 (1)*/
> > +	{ LTC2664_SPAN_RANGE_M2V5_2V5, 32768 }, /* MPS2=0, MPS1=1,
> MSP0=0 (2)*/
> > +	{ LTC2664_SPAN_RANGE_0V_10V, 0 }, /* MPS2=0, MPS1=1, MSP0=1
> (3)*/
> > +	{ LTC2664_SPAN_RANGE_0V_10V, 32768 }, /* MPS2=1, MPS1=0,
> MSP0=0 (4)*/
> > +	{ LTC2664_SPAN_RANGE_0V_5V, 0 }, /* MPS2=1, MPS1=0, MSP0=1
> (5)*/
> > +	{ LTC2664_SPAN_RANGE_0V_5V, 32768 }, /* MPS2=1, MPS1=1,
> MSP0=0 (6)*/
> > +	{ LTC2664_SPAN_RANGE_0V_5V, 0 } /* MPS2=1, MPS1=1, MSP0=1
> (7)*/
> > +};
> > +
> > +struct ltc2664_state;
> > +
> > +struct ltc2664_chip_info {
> > +	enum ltc2664_ids id;
> > +	const char *name;
> > +	int (*scale_get)(const struct ltc2664_state *st, int c);
> > +	int (*offset_get)(const struct ltc2664_state *st, int c);
> > +	int measurement_type;
> > +	unsigned int num_channels;
> > +	const struct iio_chan_spec *iio_chan;
> > +	const int (*span_helper)[2];
> > +	unsigned int num_span;
> > +	unsigned int internal_vref;
> > +	bool manual_span_support;
> > +	bool rfsadj_support;
> > +};
> > +
> > +struct ltc2664_chan {
> > +	bool toggle_chan;
> > +	bool powerdown;
> > +	u8 span;
> > +	u16 raw[2]; /* A/B */
> > +};
> 
> I would find it helpful to have more comments explainging what the various
> fields are for. For example, raw to be used to supply data to a SPI xfer
> but actually it is just a shadow copy of the current state of the chip
> registers.
> 
> > +
> > +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.
> 
> > +}
> > +
> > +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?

Hi,

In the case of max span for ltc2672, I found that performing multiplication
before division causes an integer overflow during testing. I was wondering
how the upstream handles this case. Could you provide some advice?

Thanks,
Kim






[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux