| 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: <aLmVzDB4bk-z5d16@smile.fi.intel.com>
Date: Thu, 4 Sep 2025 16:36:12 +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>,
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
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
Powered by blists - more mailing lists