Message-ID: <796291b5-f61a-4d68-9cbb-ae099dbb93d8@gmail.com>
Date: Fri, 5 Sep 2025 08:41:26 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
 Jonathan Cameron <jic23@...nel.org>, David Lechner <dlechner@...libre.com>,
 Nuno Sá <nuno.sa@...log.com>,
 Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Linus Walleij <linus.walleij@...aro.org>,
 Bartosz Golaszewski <brgl@...ev.pl>,
 Marcelo Schmitt <marcelo.schmitt@...log.com>,
 Javier Carrasco <javier.carrasco.cruz@...il.com>,
 Tobias Sperling <tobias.sperling@...ting.com>,
 Antoniu Miclaus <antoniu.miclaus@...log.com>,
 Trevor Gamblin <tgamblin@...libre.com>, Esteban Blanc <eblanc@...libre.com>,
 Herve Codina <herve.codina@...tlin.com>,
 Ramona Alexandra Nechita <ramona.nechita@...log.com>,
 Eason Yang <j2anfernee@...il.com>,
 Pop Ioan Daniel <pop.ioan-daniel@...log.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-gpio@...r.kernel.org
Subject: Re: [PATCH v2 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO

Hi dee Ho Andy!

Thanks again. I really appreciate the effort you put in these reviews! :)

On 04/09/2025 16:36, Andy Shevchenko wrote:
> 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.

What do you mean by a mess? Don't you have the '|' -characters aligned? 
That's very odd because they are aligned for me. Or, is this otherwise 
unclear?

> 
> ...
> 
>> +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'.

Ah, thanks for pointing out the pointlessness of the < 0! Will fix this.

I still prefer checking that the offset doesn't exceed the pin count. 
The amount of pins is clear for anyone using the device, where as amount 
of 'banks' or even 'registers' isn't quite as obvious but requires one 
to know the internals of the IC.

> 
>> +		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.

Interesting micro optimization. I had no idea about this - thanks for 
sharing it :) It's always good to learn something!

I don't like it here though. There are a few call sites where we only 
need to get the offset once, but more than one register. (Like the 
direction setting). Hence, I like to keep these operations decoupled. I 
think the benefit from this optimization is quite small. Furthermore, 
architecture specific optimizations aren't really something we should 
add in generic drivers. It may be this will never get used on the 
architecture we are optimizing for. [Sure it can make sense for 
something like regmap, which is heavily used (probably) on all 
architectures, especially if cost is just placing some lines closer to 
each others. So, nice change there!]

> 
> ...
> 
>> +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).

Thanks!

>> +};
> 
> ...
> 
>> +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?

Oh, well spotted! That's just a leftover from my testing where I called 
this without the gpio-chip! It should indeed be the gc->ngpio! Thanks!

> 
>> +		unsigned long bank_bits;
>> +		unsigned int reg;
>> +		int ret;
> 
>> +		if (bank_mask) {
> 
> This is a duplication, the iterator only gives non-zero "clumps".

Indeed. Thanks!

>> +			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?

I think I already explained why it is signed. I don't see how this bites 
us here? Can you please elaborate what's the problem _here_?

> 
>> +	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.)

I just filled the rows to the maximum width, while keeping the item 
count same for each row and adding the comment.

I'm not really sure having 4 items / row, and adding one row more would 
be much better, but not much worse either. I can do this if you think 
it's better. (No need to even fight for that).

>> +};
> 
> ...
> 
>> +	data->vref_mv = ret / 1000;
> 
> Yeah, mV, (MICRO / MILLI) and other things I leave to other people to discuss.

Thanks Andy. I've a feeling we were somewhat stuck on this discussion 
anyways. I suppose it's nice to hear other's opinions if someone else 
really cares. This was approaching bikeshedding.

> ...
> 
>> +	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.

Sorry Andy. I think I didn't consider your suggestion thoroughly as I 
just assumed it was related to the question if the value returned by the 
devm_iio_adc_device_alloc_chaninfo_se() can be positive. I'll re-read 
your suggestion, thanks for pinging me on this.

>> +		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.

I do also still think placing the comment before assignment can give 
reader an idea that the amount of pins is fetched for this check, which 
it isn't. We fetch it for other purposes, but optimize things bailing 
out if there are no GPIOs. So, we comment to explain early exit. But 
yeah, let's see what others think of it as well.

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

Yours,
	-- Matti

Powered by blists - more mailing lists