| 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
| ||
|
Message-ID: <b2d9ff438948fdc6f52029d25bb03d1e02874fb6.camel@gmail.com>
Date: Wed, 13 Dec 2023 08:13:05 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: David Lechner <dlechner@...libre.com>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Jonathan Cameron <jic23@...nel.org>,
Michael Hennerich <michael.hennerich@...log.com>,
Nuno Sá <nuno.sa@...log.com>,
Alexandru Ardelean <alexandru.ardelean@...log.com>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>, linux-kernel@...r.kernel.org,
Stefan Popa <stefan.popa@...log.com>
Subject: Re: [PATCH 2/2] iio: adc: ad7380: new driver for AD7380 ADCs
On Tue, 2023-12-12 at 16:42 +0100, David Lechner wrote:
> On Tue, Dec 12, 2023 at 4:16 PM Nuno Sá <noname.nuno@...il.com> wrote:
> >
> > Hi David,
> >
> > minor stuff from me...
> >
> > On Fri, 2023-12-08 at 09:51 -0600, David Lechner wrote:
> > > This adds a new driver for the AD7380 family ADCs.
> > >
> > > The driver currently implements basic support for the AD7380, AD7381,
> > > AD7383, and AD7384 2-channel differential ADCs. Support for additional
> > > single-ended and 4-channel chips that use the same register map as well
> > > as additional features of the chip will be added in future patches.
> > >
> > > Co-developed-by: Stefan Popa <stefan.popa@...log.com>
> > > Signed-off-by: Stefan Popa <stefan.popa@...log.com>
> > > Signed-off-by: David Lechner <dlechner@...libre.com>
> > > ---
> > > MAINTAINERS | 1 +
> > > drivers/iio/adc/Kconfig | 16 ++
> > > drivers/iio/adc/Makefile | 1 +
> > > drivers/iio/adc/ad7380.c | 467
> > > +++++++++++++++++++++++++++++++++++++++++++++++
> > > 4 files changed, 485 insertions(+)
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index e2a998be5879..5a54620a31b8 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -438,6 +438,7 @@ S: Supported
> > > W:
> > > https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad738x
> > > W: https://ez.analog.com/linux-software-drivers
> > > F: Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
> > > +F: drivers/iio/adc/ad7380.c
> > >
> > > AD7877 TOUCHSCREEN DRIVER
> > > M: Michael Hennerich <michael.hennerich@...log.com>
> > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > index 35f9867da12c..cbfd626712e3 100644
> > > --- a/drivers/iio/adc/Kconfig
> > > +++ b/drivers/iio/adc/Kconfig
> > > @@ -122,6 +122,22 @@ config AD7298
> > > To compile this driver as a module, choose M here: the
> > > module will be called ad7298.
> > >
> > > +config AD7380
> > > + tristate "Analog Devices AD7380 ADC driver"
> > > + depends on SPI_MASTER
> > > + select IIO_BUFFER
> > > + select IIO_TRIGGER
> > > + select IIO_TRIGGERED_BUFFER
> > > + help
> > > + AD7380 is a family of simultaneous sampling ADCs that share the
> > > same
> > > + SPI register map and have similar pinouts.
> > > +
> > > + Say yes here to build support for Analog Devices AD7380 ADC and
> > > + similar chips.
> > > +
> > > + To compile this driver as a module, choose M here: the module will
> > > be
> > > + called ad7380.
> > > +
> > > config AD7476
> > > tristate "Analog Devices AD7476 1-channel ADCs driver and other
> > > similar devices from AD and TI"
> > > depends on SPI
> > > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > > index bee11d442af4..e046d8004f41 100644
> > > --- a/drivers/iio/adc/Makefile
> > > +++ b/drivers/iio/adc/Makefile
> > > @@ -16,6 +16,7 @@ obj-$(CONFIG_AD7291) += ad7291.o
> > > obj-$(CONFIG_AD7292) += ad7292.o
> > > obj-$(CONFIG_AD7298) += ad7298.o
> > > obj-$(CONFIG_AD7923) += ad7923.o
> > > +obj-$(CONFIG_AD7476) += ad7380.o
> > > obj-$(CONFIG_AD7476) += ad7476.o
> > > obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
> > > obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
> > > diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> > > new file mode 100644
> > > index 000000000000..6a5ec59bd1fd
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/ad7380.c
> > > @@ -0,0 +1,467 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Analog Devices AD738x Simultaneous Sampling SAR ADCs
> > > + *
> > > + * Copyright 2017 Analog Devices Inc.
> > > + * Copyright 2023 BayLibre, SAS
> > > + */
> > > +
> > > +#include <linux/bitfield.h>
> > > +#include <linux/bitops.h>
> > > +#include <linux/cleanup.h>
> > > +#include <linux/device.h>
> > > +#include <linux/err.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/spi/spi.h>
> > > +#include <linux/sysfs.h>
> > > +
> > > +#include <linux/iio/buffer.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/sysfs.h>
> > > +#include <linux/iio/trigger_consumer.h>
> > > +#include <linux/iio/triggered_buffer.h>
> > > +
> > >
> >
> > ...
> >
> > > +
> > > +static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
> > > + u32 writeval, u32 *readval)
> > > +{
> > > + struct ad7380_state *st = iio_priv(indio_dev);
> > > + int ret;
> > > +
> > > + ret = iio_device_claim_direct_mode(indio_dev);
> > > + if (ret)
> > > + return ret;
> > > +
> >
> > potential controversial take: do we really need locking in here? regmap already
> > has it's own lock (I think you're not disabling it) and if someone plays with
> > registers it shouldn't while buffering, well... This is a debug interface so I
> > would probably not worry much. One could anyways for write stuff by going
> > directly to regmap :)
> >
> > That said, fine to be as-is from my side (just wanted to take it out of my
> > system :))...
>
> I put it in because we often run `iio_info` which reads register 0 via
> debugfs. So even though we "shouldn't" be using debug while buffer is
> enabled, it is easy to do unintentionally. (This was particularly
> problem while working on offload support.)
>
fair enough...
> >
> > > + if (readval)
> > > + ret = regmap_read(st->regmap, reg, readval);
> > > + else
> > > + ret = regmap_write(st->regmap, reg, writeval);
> > > +
> > > + iio_device_release_direct_mode(indio_dev);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static irqreturn_t ad7380_trigger_handler(int irq, void *p)
> > > +{
> > > + struct iio_poll_func *pf = p;
> > > + struct iio_dev *indio_dev = pf->indio_dev;
> > > + struct ad7380_state *st = iio_priv(indio_dev);
> > > + struct spi_transfer xfer = {
> > > + .bits_per_word = st->chip_info-
> > > > channels[0].scan_type.realbits,
> > > + .len = 4,
> > > + .rx_buf = &st->scan_data,
> > > + };
> > > + int ret;
> > > +
> > > + ret = spi_sync_transfer(st->spi, &xfer, 1);
> > > +
> > > + if (ret == 0)
> > > + iio_push_to_buffers_with_timestamp(indio_dev, &st->scan_data,
> > > + pf->timestamp);
> > > +
> > > + iio_trigger_notify_done(indio_dev->trig);
> > > +
> > > + return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int ad7380_read_direct(struct ad7380_state *st,
> > > + struct iio_chan_spec const *chan, int *val)
> > > +{
> > > + struct spi_transfer xfers[] = {
> > > + /* toggle CS (no data xfer) to trigger a conversion */
> > > + {
> > > + .speed_hz = AD7380_REG_WR_SPEED_HZ,
> > > + .bits_per_word = chan->scan_type.realbits,
> > > + .delay = {
> > > + .value = 190, /* t[CONVERT] */
> > > + .unit = SPI_DELAY_UNIT_NSECS,
> > > + },
> > > + .cs_change = 1,
> > > + .cs_change_delay = {
> > > + .value = 10, /* t[CSH] */
> > > + .unit = SPI_DELAY_UNIT_NSECS,
> > > + },
> > > + },
> > > + /* then read both channels */
> > > + {
> > > + .speed_hz = AD7380_REG_WR_SPEED_HZ,
> > > + .bits_per_word = chan->scan_type.realbits,
> > > + .rx_buf = &st->rx[0],
> > > + .len = 4,
> > > + },
> > > + };
> > > + int ret;
> > > +
> > > + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > > +
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + *val = sign_extend32(st->rx[chan->scan_index],
> > > + chan->scan_type.realbits - 1);
> > > +
> > > + return IIO_VAL_INT;
> > > +}
> > > +
> > > +static int ad7380_read_raw(struct iio_dev *indio_dev,
> > > + struct iio_chan_spec const *chan,
> > > + int *val, int *val2, long info)
> > > +{
> > > + struct ad7380_state *st = iio_priv(indio_dev);
> > > + int ret;
> > > +
> > > + switch (info) {
> > > + case IIO_CHAN_INFO_RAW:
> > > + ret = iio_device_claim_direct_mode(indio_dev);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = ad7380_read_direct(st, chan, val);
> > > + iio_device_release_direct_mode(indio_dev);
> > > +
> > > + return ret;
> > > + case IIO_CHAN_INFO_SCALE:
> > > + if (st->vref) {
> > > + ret = regulator_get_voltage(st->vref);
> >
> > nit: I wonder how likely it is for vref to change at runtime...
> >
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + *val = ret / 1000;
> > > + } else {
> > > + *val = AD7380_INTERNAL_REF_MV;
> > > + }
> > > +
> > > + *val2 = chan->scan_type.realbits;
> > > +
> > > + return IIO_VAL_FRACTIONAL_LOG2;
> > > + }
> > > +
> > > + return -EINVAL;
> > > +}
> > > +
> > > +static const struct iio_info ad7380_info = {
> > > + .read_raw = &ad7380_read_raw,
> > > + .debugfs_reg_access = &ad7380_debugfs_reg_access,
> > > +};
> > > +
> > > +static int ad7380_init(struct ad7380_state *st)
> > > +{
> > > + int ret;
> > > +
> > > + /* perform hard reset */
> > > + ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
> > > + AD7380_CONFIG2_RESET,
> > > + FIELD_PREP(AD7380_CONFIG2_RESET,
> > > + AD7380_CONFIG2_RESET_HARD));
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > +
> > > + /* select internal or external reference voltage */
> > > + ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1,
> > > + AD7380_CONFIG1_REFSEL,
> > > + FIELD_PREP(AD7380_CONFIG1_REFSEL, !!st-
> > > > vref));
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + /* SPI 1-wire mode */
> > > + return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
> > > + AD7380_CONFIG2_SDO,
> > > + FIELD_PREP(AD7380_CONFIG2_SDO, 1));
> > > +}
> > > +
> > > +static void ad7380_release_regulator(void *p)
> > > +{
> > > + struct regulator *reg = p;
> > > +
> > > + regulator_disable(reg);
> > > +}
> > > +
> > > +static int ad7380_probe(struct spi_device *spi)
> > > +{
> > > + struct iio_dev *indio_dev;
> > > + struct ad7380_state *st;
> > > + const char *str_val;
> > > + int ret;
> > > +
> > > + ret = device_property_read_string(&spi->dev, "adi,sdo-mode",
> > > &str_val);
> > > + if (ret < 0)
> > > + return dev_err_probe(&spi->dev, ret,
> > > + "Failed to read adi,sdo-mode
> > > property\n");
> > > +
> > > + if (strcmp(str_val, "1-wire"))
> > > + return dev_err_probe(&spi->dev, -EINVAL,
> > > + "Only 1-wire SDO is supported\n");
> > > +
> > > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > > + if (!indio_dev)
> > > + return -ENOMEM;
> > > +
> > > + st = iio_priv(indio_dev);
> > > + st->spi = spi;
> > > + st->chip_info = spi_get_device_match_data(spi);
> > > +
> >
> > if (!st->chip_info) ?
>
> It looks like quite a few drivers don't check this, but I guess safer
> to check anyway.
>
Don't make it right :). I mean, FWIW, I'm not quite sure if you can really get NULL
in here but since it's a possibility (from the API point of view), better to do it
properly.
- Nuno Sá
Powered by blists - more mailing lists