| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 18 Jun 2024 10:32:32 +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
> -----Original Message-----
> From: David Lechner <dlechner@...libre.com>
> Sent: Tuesday, June 4, 2024 4:43 AM
> To: Paller, Kim Seer <KimSeer.Paller@...log.com>; linux-
> kernel@...r.kernel.org; linux-iio@...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
>
> [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@...el.com>
> > Closes: https://urldefense.com/v3/__https://lore.kernel.org/oe-kbuild-
> all/202405241141.kYcxrSem-
> lkp@...el.com/__;!!A3Ni8CS0y2Y!65MPSYKyqFizjs_tSxpABl45BNKqWutx4NOBi
> EkmmmY8kJkwep-27ON2rqne-XnUId2M3KsqgGbQy7GI_aYi9Tg$
> > Co-developed-by: Michael Hennerich <michael.hennerich@...log.com>
> > Signed-off-by: Michael Hennerich <michael.hennerich@...log.com>
> > Signed-off-by: Kim Seer Paller <kimseer.paller@...log.com>
> > ---
> > 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@...log.com>
> > 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
Powered by blists - more mailing lists