> -----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
