On Fri, 8 Dec 2023 09:51:41 -0600
David Lechner <dlechner@xxxxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Stefan Popa <stefan.popa@xxxxxxxxxx>
> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
Hi David / Stefan,
A few minor things inline. Other than that question following through
from the bindings about 1-wire / 2-wire description it's all trivial
and some might be considered my odd tastes :)
Nice to see such a clean v1
Jonathan
> 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
> +struct ad7380_state {
> + const struct ad7380_chip_info *chip_info;
> + struct spi_device *spi;
> + struct regulator *vref;
> + struct regmap *regmap;
> + /*
> + * DMA (thus cache coherency maintenance) requires the
> + * transfer buffers to live in their own cache lines.
> + * Make the buffer large enough for 2 16-bit samples and one 64-bit
> + * aligned 64 bit timestamp.
> + */
> + struct {
> + u16 raw[2];
> + s64 ts __aligned(8);
> + } scan_data __aligned(IIO_DMA_MINALIGN);
> + u16 tx[2];
> + u16 rx[2];
> +};
...
> +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,
I'd make it explicit you are reading into st->scan_data->raw rather than using
the address of the containing structure.
> + };
> + int ret;
> +
> + ret = spi_sync_transfer(st->spi, &xfer, 1);
> +
> + if (ret == 0)
I'm not keen on this pattern. It saves a bit of text but makes things slightly less
obvious when compared to all the other error paths.
if (ret)
goto error;
iio_push_to_buffers...
error:
iio_trigger_notify_done...
Is my preference.
> + 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));
> +
Trivial, but no blank line here. It's good to keep error check and
the call in the same block.
> + 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_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;
> +
> +
Trivial: Single blank line
> + /* 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;
The local variable doesn't really add anything over putting p
in the regulator_disable() call.
> +
> + 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");
Discussion on this binding in the dt-binding patch.
As mentioned there, it feels like a place where a default would be sensible.
> +
> + 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);
> +
> + st->vref = devm_regulator_get_optional(&spi->dev, "refio");
> + if (IS_ERR(st->vref)) {
> + /*
> + * If there is no REFIO supply, then it means that we are using
> + * the internal 2.5V reference.
> + */
> + if (PTR_ERR(st->vref) == -ENODEV)
> + st->vref = NULL;
> + else
> + return dev_err_probe(&spi->dev, PTR_ERR(st->vref),
> + "Failed to get refio regulator\n");
> + }
> +
> + if (st->vref) {
> + ret = regulator_enable(st->vref);
> + if (ret)
> + return ret;
> +
> + ret = devm_add_action_or_reset(&spi->dev, ad7380_release_regulator,
Naming wise maybe ad7380_disable_regulator is less misleading that release (which in
my mind is the bit the unwind for devm_regulator_get_optional() is doing)
> + st->vref);
> + if (ret)
> + return ret;
> + }
