Message-ID: <00ee1968-a471-4d2b-a024-4bee00e40513@gmail.com>
Date: Wed, 3 Sep 2025 09:52:02 +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>,
 Ramona Alexandra Nechita <ramona.nechita@...log.com>,
 Thomas Bonnefille <thomas.bonnefille@...tlin.com>,
 Hans de Goede <hansg@...nel.org>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-gpio@...r.kernel.org
Subject: Re: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO

Hi deee Ho Andy,

On 02/09/2025 17:15, Andy Shevchenko wrote:
> 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).

Thanks!

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

The 'mv' is a deliberate choise. I still remember our previous 
discussion about this same topic [1].

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

Thanks! It's super helpful to list the missing headers. (And no, there 
is no sarcasm, it really is hard to pick the right headers at times! I 
find your review _very_ helpful on that).

I won't repeat this for other header comments you made.

>> +	/* 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);
>> +};
> ...
> 
>> +static int _get_gpio_reg(int offset, unsigned int base)
>> +{
> 
> Why offset is signed?

Just out of habit. Can be unsigned.

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

...

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

Could be. I've never been overly enthusiastic what comes to memorizing 
styling rules. (I believe that if there are hard styling rules, those 
should be enforced/checked by tooling like checkpatch.) Still, I've 
myself never really been fond of having too strict set of styling rules.

Thus, I really thought useless brackets were useless.

TLDR; I can combine the conditions in one if (), thanks.

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

Hmm. Thanks. I wonder why I didn't see nagging from unhandled cases. I 
think some tooling used to emit those warnings...

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

Haven't yet checked the for_each_set_clump8() - but I assume because I 
had no memory trace about kernel having such function :) (Thanks!)

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

Because it is expected to be either GPIO_LINE_DIRECTION_IN or 
GPIO_LINE_DIRECTION_OUT, which are integers, right?

> And why in negative case or other than _IN we switch pin to
> output mode. It's dangerous default.

This function is only called with GPIO_LINE_DIRECTION_IN or 
GPIO_LINE_DIRECTION_OUT, but I agree it's better to invert the 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.

Can you please explain what you mean. I am not sure I can follow you here.

>> +}
> 
> ...
> 
>> +static int bd79112_gpio_output(struct gpio_chip *gc, unsigned int offset,
>> +			       int value)
> 
> Why value is signed?

Because the direction_output is defined as:
int                     (*direction_output)(struct gpio_chip *gc,
					unsigned int offset, int value);
in a tree this series is based on?

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

For 'i'? Because it matches with the num_channels, which is int, just as 
it is in the struct iio_dev. This is not a problem because we know the 
maximum number of channels is 32.

For 'gpio_channels'? Mostly because int works and it was handy to just 
use same type as for 'i'. It works as we just need enough memory to fit 
in 32 bits. The integer representation of the value is meaningless and 
thus the sign does not play any role here.

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

Maybe :) I would like to know why? As you know, I prefer to keep lines 
short to fit multiple terminals in parallel, so this will probably make 
the entry to consume more rows. Thus, I would like to have a solid reason.

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

Mostly just because this is not directly referred to outside the probe 
by this driver and I don't think it should be referred. OTOH, having 
this global would allow squeezing the indentiation - which would make it 
tad more compact - so I think I'll just go with your suggestion for the 
next revision :) Thanks.

..

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

I find this much more confusing than plain 1000. (I know we had this 
type of discussion before. See [1] again).

> 
>> +	ret = devm_regulator_get_enable(dev, "iovdd");
>> +	if (ret < 0)
> 
> Does it return positive or zero on success?

Zero, thanks!

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

Sorry but I am not sure I understand what you mean by amended? Can you 
please go an extra mile and explain :)

> 
>> +	if (ret < 0) {
> 
> Why ' < 0' ?

because the devm_iio_adc_device_alloc_chaninfo_se() returns number of 
found channels.

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

I suppose you mean something like:

register_gpios:
	/* We're done if all channels are reserved for ADC. */
	gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
                                           iio_dev->num_channels);
	if (!gpio_pins)
		return 0;

right?

I don't like this because now the comment suggests we do call 
bd79112_get_gpio_pins() only to see if all channels were for ADCs. This, 
however, is not THE reason for this call, only an optimization. I think: 
having:

         /* We're done if all channels are reserved for ADC. */
         if (!gpio_pins)
                 return 0;

is clearer.



Thanks (again) for the review! I always appreciate your work, even if I 
don't always agree on everything :)

[1]: https://lore.kernel.org/all/20250505173157.57aa16f9@jic23-huawei/


Yours,
	-- Matti

Powered by blists - more mailing lists