| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aLb8HuIG0XXLu653@smile.fi.intel.com>
Date: Tue, 2 Sep 2025 17:15:58 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Matti Vaittinen <mazziesaccount@...il.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
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
Powered by blists - more mailing lists