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

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

On Thu, Sep 04, 2025 at 03:36:46PM +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.

...

> +/*
> + * 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   |
> + */

I don't know how to read this comment. In the monospace font the whole block
looks like a mess.

...

> +static int _get_gpio_reg(unsigned int offset, unsigned int base)
> +{
> +	int regoffset = offset / 8;
> +
> +	if (offset > 31 || offset < 0)

So, < 0 is now unneeded and offset > 31 can be rewritten as

	if (regoffset >= 4)

which is more clear to me (like we have 4 banks and here is the check for
the bank. Maybe you can even call the variable 'bank'.

> +		return -EINVAL;
> +
> +	return base - regoffset;
> +}

...

> +#define GET_GPIO_BIT(offset) BIT((offset) % 8)

I suggest to make it to be a returned parameter of _get_gpio_reg(). This will
give better code generation on some architectures, see, for example, this
commit: 9b3cd5c7099f regmap: place foo / 8 and foo % 8 closer to each other.

...

> +static const struct regmap_access_table bd79112_volatile_regs = {
> +	.yes_ranges = &bd71815_volatile_ro_ranges[0],
> +	.n_yes_ranges = ARRAY_SIZE(bd71815_volatile_ro_ranges),

+ array_size.h
(and btw we put generic asm/* _after_ generic linux/*, just noticed that).

> +};

...

> +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;
> +	default:
> +		return -EINVAL;
> +	}

> +

Unneeded blank line.

> +}

...

> +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 long i, bank_mask;
> +
> +	for_each_set_clump8(i, bank_mask, mask, /* gc->ngpio */ 32) {

Hmm... Why constant and not gc->ngpio?

> +		unsigned long bank_bits;
> +		unsigned int reg;
> +		int ret;

> +		if (bank_mask) {

This is a duplication, the iterator only gives non-zero "clumps".

> +			bank_bits = bitmap_get_value8(bits, i);
> +			reg = BD79112_REG_GPO_VALUE_A0_A7 - i / 8;
> +			ret = regmap_update_bits(data->map, reg, bank_mask,
> +						 bank_bits);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	return 0;
> +}

...

> +static int bd79112_get_gpio_pins(const struct iio_chan_spec *cs, int num_channels)
> +{
> +	int i, gpio_channels;
> +
> +	/*
> +	 * Let's initialize the mux config to say that all 32 channels are
> +	 * GPIOs. Then we can just loop through the iio_chan_spec and clear the
> +	 * bits for found ADC channels.
> +	 */
> +	gpio_channels = GENMASK(31, 0);

This is negative number, it might bait one at a surprising time. Hence once
again, why not make them unsigned?

> +	for (i = 0; i < num_channels; i++)
> +		gpio_channels &= ~BIT(cs[i].channel);
> +
> +	return gpio_channels;
> +}

...

> +/* ADC channels as named in the data-sheet */
> +static const char * const bd79112_chan_names[] = {
> +	"AGIO0A", "AGIO1A", "AGIO2A", "AGIO3A", "AGIO4A",	/* 0 - 4 */
> +	"AGIO5A", "AGIO6A", "AGIO7A", "AGIO8A", "AGIO9A",	/* 5 - 9 */
> +	"AGIO10A", "AGIO11A", "AGIO12A", "AGIO13A", "AGIO14A",	/* 10 - 14 */
> +	"AGIO15A", "AGIO0B", "AGIO1B", "AGIO2B", "AGIO3B",	/* 15 - 19 */
> +	"AGIO4B", "AGIO5B", "AGIO6B", "AGIO7B", "AGIO8B",	/* 20 - 24 */
> +	"AGIO9B", "AGIO10B", "AGIO11B", "AGIO12B", "AGIO13B",	/* 25 - 29 */
> +	"AGIO14B", "AGIO15B",					/* 30 - 31 */

O-o-key, but why not power-of-two per line (esp. taking into account
the whole size)? (Whatever, it's not something I would fight for.)

> +};

...

> +	data->vref_mv = ret / 1000;

Yeah, mV, (MICRO / MILLI) and other things I leave to other people to discuss.


...

> +	ret = devm_iio_adc_device_alloc_chaninfo_se(dev, &bd79112_chan_template,
> +						    BD79112_MAX_NUM_CHANNELS - 1,
> +						    &cs);
> +	if (ret < 0) {

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

As I showed this can be checked before other case, but I kinda have an idea why
you are liking to do it this way.

> +		return ret;
> +	}

...

> +register_gpios:
> +	gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
> +					  iio_dev->num_channels);
> +
> +	/* If all channels are reserved for ADC, then we're done. */

I still consider the assignment to be located here is a better place,
but I leave it to maintainers.

> +	if (!gpio_pins)
> +		return 0;

-- 
With Best Regards,
Andy Shevchenko