Re: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Tue, Sep 02, 2025 at 03:24:31PM +0300, Matti Vaittinen wrote:
> The ROHM BD79112 is an ADC/GPIO with 32 channels. The channel inputs can
> be used as ADC or GPIO. Using the GPIOs as IRQ sources isn't supported.
> 
> The ADC is 12-bit, supporting input voltages up to 5.7V, and separate I/O
> voltage supply. Maximum SPI clock rate is 20 MHz (10 MHz with
> daisy-chain configuration) and maximum sampling rate is 1MSPS.
> 
> The IC does also support CRC but it is not implemented in the driver.

...

> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/err.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>

Incomplete list. See below (it doesn't mean I caught up all of the missing
inclusions).

...

> +struct bd79112_data {
> +	struct spi_device *spi;
> +	struct regmap *map;

> +	struct device *dev;


> +	struct gpio_chip gc;
> +	unsigned long gpio_valid_mask;
> +	unsigned int vref_mv;

Perhaps _mV to follow the actual unit spelling?
(and yes, I know that both variants are present in the kernel)

> +	struct spi_transfer read_xfer[2];
> +	struct spi_transfer write_xfer;
> +	struct spi_message read_msg;
> +	struct spi_message write_msg;
> +	/* 16-bit TX, valid data in high byte */
> +	u8 read_tx[2] __aligned(IIO_DMA_MINALIGN);

+ types.h for u8 and indirectly for __aligned.

> +	/* 8-bit address followed by 8-bit data */
> +	u8 reg_write_tx[2] __aligned(IIO_DMA_MINALIGN);
> +	/* 12-bit of ADC data or 8 bit of reg data */
> +	__be16 read_rx __aligned(IIO_DMA_MINALIGN);
> +};

...

> +#define BD79112_REG_AGIO0A		0x0

Keep it fixed-width. i.e. 0x00

> +#define BD79112_REG_AGIO15B		0x1f

...

> +#define BD79112_BIT_IO			BIT(5)

+ bits.h (but see about bitops.h below)

...

> +/*
> + * The data-sheet explains register I/O communication as follows:
> + *
> + * Read, two 16-bit sequences separated by CSB:
> + * MOSI:
> + * SCK:	| 1 | 2 | 3   | 4      | 5 .. 8 | 9 .. 16 |
> + * data:| 0 | 0 |IOSET| RW (1) | ADDR   | 8'b0    |
> + *
> + * MISO:
> + * SCK:	| 1 .. 8 | 9 .. 16 |
> + * data:| 8'b0   | data    |
> + *
> + * Note, CSB is shown to be released between writing the address (MOSI) and
> + * reading the register data (MISO).
> + *
> + * Write, single 16-bit sequence:
> + * MOSI:
> + * SCK:	| 1 | 2 | 3   | 4     | 5 .. 8 |
> + * data:| 0 | 0 |IOSET| RW(0) | ADDR   |
> + *
> + * MISO:
> + * SCK:	| 1 .. 8 |
> + * data:| data   |

> + *

Stray empty line.

> + */

...

> +static int _get_gpio_reg(int offset, unsigned int base)
> +{

Why offset is signed?

> +	int regoffset = offset / 8;
> +
> +	if (offset > 31 || offset < 0)
> +		return -EINVAL;

+ errno.h (but since you use IS_ERR(), the err.h should be)

> +	return base - regoffset;
> +}

...

> +static int bd79112_reg_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +	struct bd79112_data *data = context;
> +	int ret;
> +
> +	if (reg & BD79112_BIT_IO)
> +		reg |= BD79112_BIT_RW;
> +
> +	data->read_tx[0] = reg;
> +
> +	ret = spi_sync(data->spi, &data->read_msg);
> +	if (!ret)
> +		*val = be16_to_cpu(data->read_rx);

asm/byteorder.h

> +
> +	if (reg & BD79112_BIT_IO)
> +		if (*val & BD79112_ADC_STATUS_FLAG)
> +			dev_err(data->dev, "ADC pin configured as GPIO\n");

Missing {}, I think one needs to refresh a memory of kernel coding style.

> +	return ret;
> +}

...

> +static int bd79112_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int *val,
> +			    int *val2, long m)
> +{
> +	struct bd79112_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_read(data->map, chan->channel, val);
> +		if (ret < 0)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		 *val = data->vref_mv;
> +		 *val2 = 12;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	}

> +	return -EINVAL;

Why not making it default case? This is how most of the IIO drivers do.

> +}

...

> +static int bd79112_gpio_set_multiple(struct gpio_chip *gc, unsigned long *mask,
> +				     unsigned long *bits)
> +{
> +	struct bd79112_data *data = gpiochip_get_data(gc);
> +	unsigned int i;
> +
> +	for (i = 0; i < 4; i++) {
> +		unsigned int bank_mask, reg, regval, regmask;
> +		int ret;
> +
> +		bank_mask = 0xff << 8 * i;
> +		regmask = (*mask & bank_mask) << 8 * i;

Why all this?

We have for_each_set_clump8().

> +		if (!regmask)
> +			continue;
> +
> +		reg = BD79112_REG_GPO_VALUE_A0_A7 - i;
> +		regval = (*bits & bank_mask) >> 8 * i;
> +		ret = regmap_update_bits(data->map, reg, regmask, regval);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

...

> +static int bd79112_gpio_dir_set(struct bd79112_data *data, unsigned int offset,
> +				int dir)

Why dir is int? And why in negative case or other than _IN we switch pin to
output mode. It's dangerous default.

> +{
> +	unsigned int set_reg, clear_reg, bit;
> +	int ret;
> +
> +	bit = GET_GPIO_BIT(offset);
> +
> +	if (dir == GPIO_LINE_DIRECTION_IN) {
> +		set_reg = GET_GPI_EN_REG(offset);
> +		clear_reg = GET_GPO_EN_REG(offset);
> +	} else {
> +		set_reg = GET_GPO_EN_REG(offset);
> +		clear_reg = GET_GPI_EN_REG(offset);
> +	}

> +	ret = regmap_set_bits(data->map, set_reg, bit);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_clear_bits(data->map, clear_reg, bit);

I believe the order depends on the out-in or in-out switch.
Otherwise it might be potential glitches on input (hw) buffer.
Right now when it's not an interrupt it may be okay to don't
bother, but in general I see a potential issues with that.

> +}

...

> +static int bd79112_gpio_output(struct gpio_chip *gc, unsigned int offset,
> +			       int value)

Why value is signed?

...

> +static int bd79112_get_gpio_pins(const struct iio_chan_spec *cs, int num_channels)
> +{
> +	int i, gpio_channels;

Why signed?

...

> +static int bd79112_probe(struct spi_device *spi)
> +{
> +	/* ADC channels as named in the data-sheet */
> +	static const char * const chan_names[] = {
> +		"AGIO0A", "AGIO1A", "AGIO2A", "AGIO3A", "AGIO4A", "AGIO5A",
> +		"AGIO6A", "AGIO7A", "AGIO8A", "AGIO9A", "AGIO10A", "AGIO11A",
> +		"AGIO11A", "AGIO12A", "AGIO13A", "AGIO14A", "AGIO15A",
> +		"AGIO0B", "AGIO1B", "AGIO2B", "AGIO3B", "AGIO4B", "AGIO5B",
> +		"AGIO6B", "AGIO7B", "AGIO8B", "AGIO9B", "AGIO10B", "AGIO11B",
> +		"AGIO11B", "AGIO12B", "AGIO13B", "AGIO14B", "AGIO15B",

Can you make all of the lines to be the same in terms of amount of entries?

> +	};

This seems to be hidden in the function while it's used for the whole life time
f the device. Why not move it outside of the function?

> +	struct bd79112_data *data;
> +	struct iio_dev *iio_dev;
> +	struct iio_chan_spec *cs;
> +	struct device *dev = &spi->dev;
> +	unsigned long gpio_pins, pin;
> +	unsigned int i;
> +	int ret;
> +
> +	iio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!iio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(iio_dev);
> +	data->spi = spi;
> +	data->dev = dev;
> +	data->map = devm_regmap_init(&spi->dev, NULL, data, &bd79112_regmap);

You have dev, use it.

> +	if (IS_ERR(data->map))
> +		return dev_err_probe(dev, PTR_ERR(data->map),
> +				     "Failed to initialize Regmap\n");

+ dev_printk.h

> +	ret = devm_regulator_get_enable_read_voltage(dev, "vdd");
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to get the Vdd\n");

> +	data->vref_mv = ret / 1000;

(MICRO / MILLI)

> +	ret = devm_regulator_get_enable(dev, "iovdd");
> +	if (ret < 0)

Does it return positive or zero on success?

> +		return dev_err_probe(dev, ret, "Failed to enable I/O voltage\n");
> +
> +	data->read_xfer[0].tx_buf = &data->read_tx[0];
> +	data->read_xfer[0].len = sizeof(data->read_tx);
> +	data->read_xfer[0].cs_change = 1;
> +	data->read_xfer[1].rx_buf = &data->read_rx;
> +	data->read_xfer[1].len = sizeof(data->read_rx);
> +	spi_message_init_with_transfers(&data->read_msg, data->read_xfer, 2);
> +
> +	data->write_xfer.tx_buf = &data->reg_write_tx[0];
> +	data->write_xfer.len = sizeof(data->reg_write_tx);
> +	spi_message_init_with_transfers(&data->write_msg, &data->write_xfer, 1);
> +
> +	ret = devm_iio_adc_device_alloc_chaninfo_se(dev, &bd79112_chan_template,
> +		BD79112_MAX_NUM_CHANNELS - 1, &cs);

Hmm... Indentation can be amended.

> +	if (ret < 0) {

Why ' < 0' ?

> +		/* Register all pins as GPIOs if there are no ADC channels */
> +		if (ret == -ENOENT)
> +			goto register_gpios;
> +
> +		return ret;
> +	}

Assuming ret can't be positive this can be refactored as:

	/* Register all pins as GPIOs if there are no ADC channels */
	if (ret == -ENOENT)
		goto register_gpios;
	else if (ret)
		return ret;

I find it easier to follow.

> +	/* Let's assign data-sheet names to channels */
> +	for (i = 0; i < iio_dev->num_channels; i++) {
> +		unsigned int ch = cs[i].channel;
> +
> +		cs[i].datasheet_name = chan_names[ch];
> +	}
> +
> +	iio_dev->channels = cs;
> +	iio_dev->num_channels = ret;
> +	iio_dev->info = &bd79112_info;
> +	iio_dev->name = "bd79112";
> +	iio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	/*
> +	 * Ensure all channels are ADCs. This allows us to register the IIO
> +	 * device early (before checking which pins are to be used for GPIO)
> +	 * without having to worry about some pins being initially used for
> +	 * GPIO.
> +	 */
> +	for (i = 0; i < BD79112_NUM_GPIO_EN_REGS; i++) {
> +		ret = regmap_write(data->map, BD79112_FIRST_GPIO_EN_REG + i, 0);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to initialize channels\n");
> +	}
> +
> +	ret = devm_iio_device_register(data->dev, iio_dev);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret, "Failed to register ADC\n");
> +
> +register_gpios:
> +	gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
> +					  iio_dev->num_channels);

> +

Instead of leaving this rather unneeded blank line I would move above...

> +	/* We're done if all channels are reserved for ADC. */

...to be here

	gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
					  iio_dev->num_channels);

> +	if (!gpio_pins)
> +		return 0;
> +
> +	/* Default all the GPIO pins to GPI */
> +	for_each_set_bit(pin, &gpio_pins, BD79112_MAX_NUM_CHANNELS) {

+ bitops.h

> +		ret = bd79112_gpio_dir_set(data, pin, GPIO_LINE_DIRECTION_IN);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to mark pin as GPI\n");
> +	}
> +
> +	data->gpio_valid_mask = gpio_pins;
> +	data->gc = bd79112_gpio_chip;
> +	data->gc.parent = dev;
> +
> +	return devm_gpiochip_add_data(dev, &data->gc, data);
> +}

-- 
With Best Regards,
Andy Shevchenko