Support ROHM BD79112 ADC
Subsystems (4)
Matti Vaittinen
maintainercover Support ROHM BD79112 ADC
Support ROHM BD79112 ADC/GPIO
The ROHM BD79112 is a 12-bit, 32 channel SAR ADC / GPIO IC. Or, a "Signal Monitor Hub IC" as data-sheet describes it.
Data sheet states the maximum sampling rate to be 1 MSPS, but achieving this would probably require the SPI and samples to be processed by something else but the CPU running Linux. This could work with the "SPI offloading" which has recently landed upstream - but I have no HW to test this so nothing fancy is implemented here. It's still worth mentioning if someone needs the speed and wants to try implementing it :)
The SPI protocol is slightly peculiar. Accesses are done in 16-bit sequences, separated by releasing and re-aquiring the chip-select.
Register write takes 1 such sequence. The 8-bit register data to write, is stored in the last 8 bits. The high 8 bits contain register address and an I/O-bit which needs to be set for register accesses.
Register read consists of two 16-bit sequences (separated by chip-select). First sequence has again the register address and an IO bit in the high byte. Additionally, reads must have a 'read bit' set. The last 8 bits must be zero. The register data will be carried in the last 8 bits of the next 16-bit sequence while high bits in reply are zero.
ADC data reading is similar to register reading except:
- No R/W bit or I/O bit should be set.
- Register address is replaced by channel number (0 - 31).
- Reply data is carried in the 12 low bits (instead of 8 bits) of the reply sequence.
The protocol is implemented using custom regmap read() and write() operations.
Other than that, pretty standard device and driver.
Matti Vaittinen (3):
dt-bindings: iio: adc: ROHM BD79112 ADC/GPIO
iio: adc: Support ROHM BD79112 ADC/GPIO
MAINTAINERS: Support ROHM BD79112 ADC
.../bindings/iio/adc/rohm,bd79112.yaml | 118 ++++ MAINTAINERS | 3 +- drivers/iio/adc/Kconfig | 10 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/rohm-bd79112.c | 542 ++++++++++++++++++ 5 files changed, 673 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/iio/adc/rohm,bd79112.yaml create mode 100644 drivers/iio/adc/rohm-bd79112.c
base-commit: d1487b0b78720b86ec2a2ac7acc683ec90627e5b
Signature · 1 line
2.51.0
Matti Vaittinen
maintainerPATCH 1/3 dt-bindings: iio: adc: ROHM BD79112 ADC/GPIO
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.
Add a device tree binding document for the ROHM BD79112.
--- .../bindings/iio/adc/rohm,bd79112.yaml | 118 ++++++++++++++++++ 1 file changed, 118 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/rohm,bd79112.yaml
Documentation/devicetree/bindings/iio/adc/rohm,bd79112.yaml new file +118 -0
@@ -0,0 +1,118 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/rohm,bd79112.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ROHM BD79112 ADC/GPO + +maintainers: + - Matti Vaittinen <[email protected]> + +description: | + The ROHM BD79112 is a 12-bit, 32-channel, SAR ADC. ADC input pins can be + also configured as general purpose inputs/outputs. SPI should use MODE 3. + +properties: + compatible: + const: rohm,bd79112 + + reg: + maxItems: 1 + + spi-cpha: true + spi-cpol: true + + gpio-controller: true + + "#gpio-cells": + const: 1 + description: + The pin number. + + vdd-supply: true + + iovdd-supply: true + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + +patternProperties: + "^channel@([0-9]|[12][0-9]|3[01])$": + type: object + $ref: /schemas/iio/adc/adc.yaml# + description: Represents ADC channel. Omitted channels' inputs are GPIOs. + + properties: + reg: + description: AIN pin number + minimum: 0 + maximum: 31 + + required: + - reg + + additionalProperties: false + +required: + - compatible + - reg + - iovdd-supply + - vdd-supply + - spi-cpha + - spi-cpol + +additionalProperties: false + +examples: + - | + spi { + #address-cells = <1>; + #size-cells = <0>; + adc: adc@0 { + compatible = "rohm,bd79112"; + reg = <0x0>; + + spi-cpha; + spi-cpol; + + vdd-supply = <&dummyreg>; + iovdd-supply = <&dummyreg>; + + #address-cells = <1>; + #size-cells = <0>; + + gpio-controller; + + channel@0 { + reg = <0>; + }; + channel@1 { + reg = <1>; + }; + channel@2 { + reg = <2>; + }; + channel@3 { + reg = <3>; + }; + channel@4 { + reg = <4>; + }; + channel@5 { + reg = <5>; + }; + channel@6 { + reg = <6>; + }; + channel@16 { + reg = <16>; + }; + channel@20 { + reg = <20>; + }; + }; + };
Signature · 1 line
2.51.0
Rob Herring (Arm)
maintainerRe: [PATCH 1/3] dt-bindings: iio: adc: ROHM BD79112 ADC/GPIO
Quoted 7 lines · Matti Vaittinen
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.
Quoted 8 lines
Add a device tree binding document for the ROHM BD79112.
Signed-off-by Matti Vaittinen <[email protected]>--- .../bindings/iio/adc/rohm,bd79112.yaml | 118 ++++++++++++++++++ 1 file changed, 118 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/rohm,bd79112.yaml
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/rohm,bd79112.example.dtb: adc@0 (rohm,bd79112): '#gpio-cells' is a dependency of 'gpio-controller'
from schema $id: http://devicetree.org/schemas/gpio/gpio.yaml#
doc reference errors (make refcheckdocs):
The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
Matti Vaittinen
maintainerPATCH 2/3 iio: adc: Support ROHM BD79112 ADC/GPIO
In reply to Matti VaittinenThe 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.
--- drivers/iio/adc/Kconfig | 10 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/rohm-bd79112.c | 542 +++++++++++++++++++++++++++++++++ 3 files changed, 553 insertions(+) create mode 100644 drivers/iio/adc/rohm-bd79112.c
drivers/iio/adc/Kconfig +10 -0
@@ -1309,6 +1309,16 @@ config RN5T618_ADC This driver can also be built as a module. If so, the module will be called rn5t618-adc. +config ROHM_BD79112 + tristate "Rohm BD79112 ADC driver" + depends on I2C && GPIOLIB + select REGMAP_I2C + select IIO_ADC_HELPER + help + Say yes here to build support for the ROHM BD79112 ADC. The + ROHM BD79112 is a 12-bit, 32-channel, SAR ADC, which analog + inputs can also be used for GPIO. + config ROHM_BD79124 tristate "Rohm BD79124 ADC driver" depends on I2C
drivers/iio/adc/Makefile +1 -0
@@ -117,6 +117,7 @@ obj-$(CONFIG_QCOM_VADC_COMMON) += qcom-vadc-common.o obj-$(CONFIG_RCAR_GYRO_ADC) += rcar-gyroadc.o obj-$(CONFIG_RICHTEK_RTQ6056) += rtq6056.o obj-$(CONFIG_RN5T618_ADC) += rn5t618-adc.o +obj-$(CONFIG_ROHM_BD79112) += rohm-bd79112.o obj-$(CONFIG_ROHM_BD79124) += rohm-bd79124.o obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
drivers/iio/adc/rohm-bd79112.c new file +542 -0
542 lines not on this page · raw message
Signature · 1 line
2.51.0
Re: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
Quoted 8 lines · Matti Vaittinen
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.
...
drivers/iio/adc/rohm-bd79112.c line 17
Quoted 9 lines
drivers/iio/adc/rohm-bd79112.c new file +9
-0+#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).
...
drivers/iio/adc/rohm-bd79112.c line 30
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c new file +7
-0+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)
drivers/iio/adc/rohm-bd79112.c line 36
Quoted 6 lines
drivers/iio/adc/rohm-bd79112.c new file +6
-0+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.
Quoted 5 lines
drivers/iio/adc/rohm-bd79112.c new file +5
-0+/* 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) ; +};
...
Quoted 1 line
+#define
BD79112_REG_AGIO0A0x0
Keep it fixed-width. i.e. 0x00
Quoted 1 line
+#define
BD79112_REG_AGIO15B0x1f
...
Quoted 1 line
+#define
BD79112_BIT_IOBIT(5)
+ bits.h (but see about bitops.h below)
...
Quoted 17 lines
+/*
+ * 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:
Quoted 8 lines
+ * SCK: | 1 | 2 | 3 | 4 | 5 .. 8 | + * data:| 0 | 0 |IOSET| RW(0) | ADDR | + * + * MISO: + * SCK: | 1 .. 8 | + * data:| data |+ *
Stray empty line.
Quoted 1 line
+ */
...
Review
Quoted 2 lines
(quoted diff) +2
-0+static int _get_gpio_reg(int offset, unsigned int base) +{
Why offset is signed?
drivers/iio/adc/rohm-bd79112.c line 120
Quoted 4 lines
drivers/iio/adc/rohm-bd79112.c new file +4
-0+int regoffset = offset /8 ; + +if (offset >31 || offset <0 ) +return -EINVAL ;
+ errno.h (but since you use IS_ERR(), the err.h should be)
Quoted 2 lines
(quoted diff) +2
-0+ return base - regoffset; +}
...
drivers/iio/adc/rohm-bd79112.c line 165
Quoted 13 lines
drivers/iio/adc/rohm-bd79112.c new file +13
-0+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
drivers/iio/adc/rohm-bd79112.c line 169
Quoted 4 lines
drivers/iio/adc/rohm-bd79112.c new file +4
-0+ +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.
Quoted 2 lines
(quoted diff) +2
-0+ return ret; +}
...
Quoted 13 lines
drivers/iio/adc/rohm-bd79112.c new file +13
-0+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; +
drivers/iio/adc/rohm-bd79112.c line 215
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c new file +9
-0+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.
Quoted 1 line
+}
...
drivers/iio/adc/rohm-bd79112.c line 314
Quoted 12 lines
drivers/iio/adc/rohm-bd79112.c new file +12
-0+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().
Quoted 12 lines
drivers/iio/adc/rohm-bd79112.c new file +12
-0+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 ; +}
...
drivers/iio/adc/rohm-bd79112.c line 330
Quoted 2 lines
drivers/iio/adc/rohm-bd79112.c new file +2
-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.
Quoted 9 lines
drivers/iio/adc/rohm-bd79112.c new file +9
-0+{ +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);
drivers/iio/adc/rohm-bd79112.c line 349
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c new file +9
-0+ }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.
Quoted 1 line
+}
...
drivers/iio/adc/rohm-bd79112.c line 360
Quoted 2 lines
drivers/iio/adc/rohm-bd79112.c new file +2
-0+static int bd79112_gpio_output (struct gpio_chip *gc,unsigned int offset, +int value)
Why value is signed?
...
drivers/iio/adc/rohm-bd79112.c line 388
Quoted 3 lines
drivers/iio/adc/rohm-bd79112.c new file +3
-0+static int bd79112_get_gpio_pins (const struct iio_chan_spec *cs,int num_channels) +{ +int i, gpio_channels;
Why signed?
...
drivers/iio/adc/rohm-bd79112.c line 411
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c new file +10
-0+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?
Review
Quoted 1 line
(quoted diff) +1
-0+ };
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?
Quoted 6 lines
drivers/iio/adc/rohm-bd79112.c new file +6
-0+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;
drivers/iio/adc/rohm-bd79112.c line 428
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c new file +10
-0+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.
drivers/iio/adc/rohm-bd79112.c line 431
Quoted 3 lines
drivers/iio/adc/rohm-bd79112.c new file +3
-0+if (IS_ERR (data->map)) +return dev_err_probe (dev,PTR_ERR (data->map), +"Failed to initialize Regmap\n" );
+ dev_printk.h
drivers/iio/adc/rohm-bd79112.c line 437
Quoted 5 lines
drivers/iio/adc/rohm-bd79112.c new file +4
-0+ 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)
drivers/iio/adc/rohm-bd79112.c line 440
Quoted 2 lines
drivers/iio/adc/rohm-bd79112.c new file +2
-0+ ret =devm_regulator_get_enable (dev,"iovdd" ); +if (ret <0 )
Does it return positive or zero on success?
Quoted 5 lines
drivers/iio/adc/rohm-bd79112.c new file +5
-0+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 ;
drivers/iio/adc/rohm-bd79112.c line 455
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c new file +10
-0+ 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.
drivers/iio/adc/rohm-bd79112.c line 456
Quoted 1 line
drivers/iio/adc/rohm-bd79112.c new file +1
-0+if (ret <0 ) {
Why ' < 0' ?
drivers/iio/adc/rohm-bd79112.c line 462
Quoted 6 lines
drivers/iio/adc/rohm-bd79112.c new file +6
-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.
Quoted 25 lines
drivers/iio/adc/rohm-bd79112.c new file +25
-0+/* 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" ); + }
drivers/iio/adc/rohm-bd79112.c line 497
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c new file +9
-0+ + 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...
drivers/iio/adc/rohm-bd79112.c line 498
Quoted 1 line
drivers/iio/adc/rohm-bd79112.c new file +1
-0+/* 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);
drivers/iio/adc/rohm-bd79112.c line 503
Quoted 5 lines
drivers/iio/adc/rohm-bd79112.c new file +5
-0+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
Quoted 12 lines
drivers/iio/adc/rohm-bd79112.c new file +12
-0+ 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); +}
Signature · 2 lines
With Best Regards, Andy Shevchenko
Matti Vaittinen
maintainerRe: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
Hi deee Ho Andy,
Quoted 12 lines · Andy Shevchenko
Quoted 8 lines · Matti Vaittinen
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.
...
Quoted 12 lines
drivers/iio/adc/rohm-bd79112.c line 17
Quoted 9 lines
drivers/iio/adc/rohm-bd79112.c new file +9
-0+#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!
Quoted 9 lines
...
Quoted 5 lines
drivers/iio/adc/rohm-bd79112.c new file +4
-0+struct bd79112_data { +struct spi_device *spi; +struct regmap *map; +struct device *dev;
Quoted 6 lines
drivers/iio/adc/rohm-bd79112.c line 30
Quoted 3 lines
drivers/iio/adc/rohm-bd79112.c new file +3
-0+struct gpio_chip gc; +unsigned long gpio_valid_mask; +unsigned int vref_mv;Perhaps
_mVto 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].
Quoted 8 lines
drivers/iio/adc/rohm-bd79112.c line 36
Quoted 6 lines
drivers/iio/adc/rohm-bd79112.c new file +6
-0+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.hfor 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.
Quoted 11 lines
Quoted 5 lines
drivers/iio/adc/rohm-bd79112.c new file +5
-0+/* 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) ; +};...
Review
Quoted 2 lines
(quoted diff) +2
-0+static int _get_gpio_reg(int offset, unsigned int base) +{Why offset is signed?
Just out of habit. Can be unsigned.
Quoted 4 lines
Quoted 4 lines
drivers/iio/adc/rohm-bd79112.c new file +4
-0+int regoffset = offset /8 ; + +if (offset >31 || offset <0 ) +return -EINVAL ;
...
Quoted 6 lines
drivers/iio/adc/rohm-bd79112.c line 169
Quoted 4 lines
drivers/iio/adc/rohm-bd79112.c new file +4
-0+ +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.
Quoted 28 lines
Quoted 3 lines
(quoted diff) +2
-0+ return ret; +}...
Quoted 21 lines
drivers/iio/adc/rohm-bd79112.c new file +21
-0+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 ; + }
Quoted 3 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ 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...
Quoted 20 lines
Quoted 2 lines
+}
...
drivers/iio/adc/rohm-bd79112.c line 314
Quoted 12 lines
drivers/iio/adc/rohm-bd79112.c new file +12
-0+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?
Quoted 1 line
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!)
Quoted 15 lines
Quoted 12 lines
drivers/iio/adc/rohm-bd79112.c new file +12
-0+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 ; +}...
Quoted 4 lines
drivers/iio/adc/rohm-bd79112.c line 330
Quoted 2 lines
drivers/iio/adc/rohm-bd79112.c new file +2
-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?
Quoted 2 lines
And why in negative case or other than
_INwe 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.
Quoted 14 lines
Quoted 13 lines
drivers/iio/adc/rohm-bd79112.c new file +13
-0+{ +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); + }
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c line 349
Quoted 5 lines
drivers/iio/adc/rohm-bd79112.c new file +5
-0+ 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.
Quoted 8 lines
Quoted 1 line
+}
...
drivers/iio/adc/rohm-bd79112.c line 360
Quoted 2 lines
drivers/iio/adc/rohm-bd79112.c new file +2
-0+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?
Quoted 8 lines
...
drivers/iio/adc/rohm-bd79112.c line 388
Quoted 3 lines
drivers/iio/adc/rohm-bd79112.c new file +3
-0+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.
Quoted 14 lines
...
drivers/iio/adc/rohm-bd79112.c line 411
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c new file +10
-0+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.
Quoted 4 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ };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.
..
Quoted 8 lines
drivers/iio/adc/rohm-bd79112.c line 437
Quoted 6 lines
drivers/iio/adc/rohm-bd79112.c new file +4
-0+ 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).
Quoted 5 lines
drivers/iio/adc/rohm-bd79112.c line 440
Quoted 3 lines
drivers/iio/adc/rohm-bd79112.c new file +2
-0+ ret =devm_regulator_get_enable (dev,"iovdd" ); +if (ret <0 )Does it return positive or zero on success?
Zero, thanks!
Quoted 9 lines
Quoted 9 lines
drivers/iio/adc/rohm-bd79112.c new file +9
-0+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 ); +
Quoted 8 lines
drivers/iio/adc/rohm-bd79112.c line 455
Quoted 6 lines
drivers/iio/adc/rohm-bd79112.c new file +6
-0+ 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 :)
Quoted 4 lines
drivers/iio/adc/rohm-bd79112.c line 456
Quoted 2 lines
drivers/iio/adc/rohm-bd79112.c new file +1
-0+if (ret <0 ) {Why ' < 0' ?
because the devm_iio_adc_device_alloc_chaninfo_se() returns number of found channels.
Quoted 36 lines
Quoted 25 lines
drivers/iio/adc/rohm-bd79112.c new file +25
-0+/* 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" ); + }drivers/iio/adc/rohm-bd79112.c line 497
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c new file +9
-0+ + 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); +
Quoted 8 lines
Instead of leaving this rather unneeded blank line I would move above...
drivers/iio/adc/rohm-bd79112.c line 498
Quoted 1 line
drivers/iio/adc/rohm-bd79112.c new file +1
-0+/* 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
Re: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
Quoted 2 lines · Matti Vaittinen
Quoted 1 line · Andy Shevchenko
On Tue, Sep 02, 2025 at 03:24:31PM +0300, Matti Vaittinen wrote:
...
Quoted 7 lines
Quoted 4 lines
drivers/iio/adc/rohm-bd79112.c line 30
Quoted 1 line
drivers/iio/adc/rohm-bd79112.c new file +1
-0+unsigned int vref_mv;Perhaps
_mVto 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].
Rings a bell. And I still think the proper spelling is more important when we talk about physics.
...
Quoted 1 line
TLDR; I can combine the conditions in one if (), thanks.
Works for me.
...
Quoted 27 lines
Quoted 16 lines
Quoted 15 lines
drivers/iio/adc/rohm-bd79112.c new file +15
-0+static int bd79112_gpio_dir_set (struct bd79112_data *data,unsigned int offset, +int dir) +{ +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); + }Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c line 349
Quoted 5 lines
drivers/iio/adc/rohm-bd79112.c new file +5
-0+ 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.
Quoted 1 line
Can you please explain what you mean. I am not sure I can follow you here.
If we set out first with enabled input, input can get a value from the output. If the value before was different, it will be an event condition (of course it all depends on the HW and I haven't read datasheet for this one). So when
1) turn out on
2) ...possible event condition... (if external bias, or similar)
3) turn in off
Compare to
1) turn in off
2) ...possible glitch on the wire... (if no external bias, or similar)
3) turn out on
Quoted 1 line
Quoted 1 line
Quoted 1 line
+}
...
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
drivers/iio/adc/rohm-bd79112.c new file +4
-0+static int bd79112_probe (struct spi_device *spi) +{ +/* ADC channels as named in the data-sheet */ +static const char *const chan_names[] = {Quoted 8 lines
drivers/iio/adc/rohm-bd79112.c line 411
Quoted 6 lines
drivers/iio/adc/rohm-bd79112.c new file +6
-0+"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?
Quoted 3 lines
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.
Sure, the array above is unindexed. It's prone to errors and typos. Moreover, it's really hard to follow in case one needs to debug such a typo and see which value needs to be fixed (imagine you typed twice the same name).
Recommended way is to use power-of-two per line (and even add a comment at the end), like
static const char * const chan_names[] = {
"AGIO0A", "AGIO1A", "AGIO2A", "AGIO3A", /* 0 - 3 */
"AGIO4A", "AGIO5A", "AGIO6A", "AGIO7A", /* 4 - 7 */
"AGIO8A", "AGIO9A", "AGIO10A", "AGIO11A", /* 8 - 11 */
...(or hexadecimal offsets, whatever is better and more in accordance with
the SW / data sheet).
Quoted 1 line
Quoted 1 line
Quoted 1 line
(quoted diff) +1
-0+ };
...
Quoted 6 lines
Quoted 3 lines
drivers/iio/adc/rohm-bd79112.c line 437
Quoted 1 line
drivers/iio/adc/rohm-bd79112.c new file +1
-0+ 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).
Rings a bell, but that's what IIO reviewers suggest to do nowadays as a compromise between creating a new bunch of unit (V) related definitions.
Quoted 7 lines
Quoted 4 lines
drivers/iio/adc/rohm-bd79112.c line 455
Quoted 2 lines
drivers/iio/adc/rohm-bd79112.c new file +2
-0+ 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 :)
ret = devm_iio_adc_device_alloc_chaninfo_se(dev, &bd79112_chan_template, BD79112_MAX_NUM_CHANNELS - 1, &cs);
...
Quoted 16 lines
Quoted 5 lines
drivers/iio/adc/rohm-bd79112.c line 497
Quoted 4 lines
drivers/iio/adc/rohm-bd79112.c new file +3
-0+ gpio_pins =bd79112_get_gpio_pins (iio_dev->channels, + iio_dev->num_channels); +Quoted 8 lines
Instead of leaving this rather unneeded blank line I would move above...
drivers/iio/adc/rohm-bd79112.c line 498
Quoted 1 line
drivers/iio/adc/rohm-bd79112.c new file +1
-0+/* 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:
Quoted 8 lines
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?
Yes.
Quoted 6 lines
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. */
Then you can amend the comment
/* If all channels are reserved for ADC, we are done. */
Quoted 4 lines
if (!gpio_pins) return 0;is clearer.
Which makes my approach sustainable.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Matti Vaittinen
maintainerRe: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
Quoted 52 lines · Andy Shevchenko
Quoted 2 lines · Matti Vaittinen
Quoted 1 line · Andy Shevchenko
On Tue, Sep 02, 2025 at 03:24:31PM +0300, Matti Vaittinen wrote:
...
Quoted 7 lines
Quoted 4 lines
drivers/iio/adc/rohm-bd79112.c line 30
Quoted 1 line
drivers/iio/adc/rohm-bd79112.c new file +1
-0+unsigned int vref_mv;Perhaps
_mVto 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].
Rings a bell. And I still think the proper spelling is more important when we talk about physics.
...
Quoted 1 line
TLDR; I can combine the conditions in one if (), thanks.
Works for me.
...
Quoted 27 lines
Quoted 26 lines
Quoted 11 lines
drivers/iio/adc/rohm-bd79112.c new file +11
-0+static int bd79112_gpio_dir_set (struct bd79112_data *data,unsigned int offset, +int dir) +{ +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);drivers/iio/adc/rohm-bd79112.c line 349
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c new file +9
-0+ }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.
Quoted 5 lines
Quoted 1 line
Can you please explain what you mean. I am not sure I can follow you here.
If we set out first with enabled input, input can get a value from the output. If the value before was different, it will be an event condition (of course it all depends on the HW and I haven't read datasheet for this one).
Ah. I guess I see what you mean. I have a feeling there is a problem at the GPIO user side, if it keeps listening for inputs after it calls direction to be switched to output.
But yes. I believe turning off the 'input listening' first, and toggling direction to output only afterwards makes sense. So, thanks. And thanks for explaining this further.
Quoted 15 lines
...
Quoted 13 lines
Quoted 12 lines
drivers/iio/adc/rohm-bd79112.c line 411
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c new file +10
-0+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?
Quoted 5 lines
Quoted 3 lines
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.
Sure, the array above is unindexed. It's prone to errors and typos.
Ha. Thanks :) I see it now when I counted the entries :) Should be 32, was 34. I agree this would have been easier to spot!
Quoted 3 lines
Moreover, it's really hard to follow in case one needs to debug such a typo and see which value needs to be fixed (imagine you typed twice the same name).
Or, if I typed twice the same name twice ;) Thanks!
Quoted 4 lines
Recommended way is to use power-of-two per line (and even add a comment at the end), like
Quoted 8 lines
static const char * const chan_names[] = { "AGIO0A", "AGIO1A", "AGIO2A", "AGIO3A", /* 0 - 3 */ "AGIO4A", "AGIO5A", "AGIO6A", "AGIO7A", /* 4 - 7 */ "AGIO8A", "AGIO9A", "AGIO10A", "AGIO11A", /* 8 - 11 */ ...(or hexadecimal offsets, whatever is better and more in accordance with
the SW / data sheet).
Ok, This makes sense now.
Quoted 9 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
(quoted diff) +1
-0+ };...
Quoted 4 lines
Quoted 3 lines
drivers/iio/adc/rohm-bd79112.c line 437
Quoted 1 line
drivers/iio/adc/rohm-bd79112.c new file +1
-0+ data->vref_mv = ret /1000 ;(MICRO / MILLI)
Quoted 5 lines
Quoted 2 lines
I find this much more confusing than plain 1000. (I know we had this type of discussion before. See [1] again).
Rings a bell, but that's what IIO reviewers suggest to do nowadays as a compromise between creating a new bunch of unit (V) related definitions.
I am sorry, but this just seems stupid to me. I'd say that it is very obvious for most of the readers dividing microvolts by 1000 results millivolts. And if it is not, then having this MICRO / MILLI is likely to just cause more confusion.
I _really_ dislike these defines. Why is MILLI 1000? Why it isn't 0.001? It makes no sense that KILO and MILLI are the same. Especially not when we are dealing with physics.
This is just an obfuscation compared to using plain 1000. (I kind of understand having a define for a value like 100000 - where counting the zeros gets cumbersome, although 100 * 1000 would be equally clear. But 1000 _is_ really 100% clear, whereas MICRO / MILLI is not).
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
drivers/iio/adc/rohm-bd79112.c line 455
Quoted 2 lines
drivers/iio/adc/rohm-bd79112.c new file +2
-0+ ret =devm_iio_adc_device_alloc_chaninfo_se (dev, &bd79112_chan_template, +BD79112_MAX_NUM_CHANNELS -1 , &cs);Hmm... Indentation can be amended.
Quoted 8 lines
Quoted 2 lines
Sorry but I am not sure I understand what you mean by amended? Can you please go an extra mile and explain :)
ret = devm_iio_adc_device_alloc_chaninfo_se(dev, &bd79112_chan_template, BD79112_MAX_NUM_CHANNELS - 1, &cs);
Ah, Ok. I'll do this, thanks.
Quoted 38 lines
...
Quoted 16 lines
Quoted 5 lines
drivers/iio/adc/rohm-bd79112.c line 497
Quoted 4 lines
drivers/iio/adc/rohm-bd79112.c new file +3
-0+ gpio_pins =bd79112_get_gpio_pins (iio_dev->channels, + iio_dev->num_channels); +Quoted 8 lines
Instead of leaving this rather unneeded blank line I would move above...
drivers/iio/adc/rohm-bd79112.c line 498
Quoted 1 line
drivers/iio/adc/rohm-bd79112.c new file +1
-0+/* 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:
Quoted 8 lines
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?
Yes.
Quoted 6 lines
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. */Then you can amend the comment
Quoted 8 lines
/* If all channels are reserved for ADC, we are done. */Quoted 4 lines
if (!gpio_pins) return 0;is clearer.
Which makes my approach sustainable.
I like your wording better, but placing this comment before the call to bd79112_get_gpio_pins() is still more confusing that placing it before the actual check:
if (!gpio_pins)
is still misleading. Comment applies to the check, not the retrieval.
Yours,
-- Matti
Re: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
Quoted 4 lines · Matti Vaittinen
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Matti Vaittinen
Quoted 1 line · Andy Shevchenko
On Tue, Sep 02, 2025 at 03:24:31PM +0300, Matti Vaittinen wrote:
...
Quoted 19 lines
Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
drivers/iio/adc/rohm-bd79112.c line 411
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c new file +10
-0+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?
Quoted 5 lines
Quoted 3 lines
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.
Sure, the array above is unindexed. It's prone to errors and typos.
Quoted 8 lines
Ha. Thanks :) I see it now when I count the entries :) Should be 32, was 34. I agree this would have been easier to spot!
Quoted 3 lines
Moreover, it's really hard to follow in case one needs to debug such a typo and see which value needs to be fixed (imagine you typed twice the same name).
Or, if I typed twice the same name twice ;) Thanks!
TBH, I even haven't noticed that the array _has_ already mistakes :-)
Quoted 15 lines
Quoted 11 lines
Recommended way is to use power-of-two per line (and even add a comment at the end), like
static const char * const chan_names[] = { "AGIO0A", "AGIO1A", "AGIO2A", "AGIO3A", /* 0 - 3 */ "AGIO4A", "AGIO5A", "AGIO6A", "AGIO7A", /* 4 - 7 */ "AGIO8A", "AGIO9A", "AGIO10A", "AGIO11A", /* 8 - 11 */ ...(or hexadecimal offsets, whatever is better and more in accordance with
the SW / data sheet).Ok, This makes sense now.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
+ };
...
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
+
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).
Quoted 7 lines
Quoted 2 lines
Rings a bell, but that's what IIO reviewers suggest to do nowadays as a compromise between creating a new bunch of unit (V) related definitions.
I am sorry, but this just seems stupid to me. I'd say that it is very obvious for most of the readers dividing microvolts by 1000 results millivolts. And if it is not, then having this MICRO / MILLI is likely to just cause more confusion.
No, it tells that we have a value in microSOMETHING that is converted to MILLIsomething.
Quoted 1 line
I _really_ dislike these defines. Why is MILLI 1000? Why it isn't 0.001?
You know exactly a few reasons why it's not.
Quoted 2 lines
It makes no sense that KILO and MILLI are the same. Especially not when we are dealing with physics.
Yes, this is the limitation of computers and particularly of _a_ kernel.
Quoted 4 lines
This is just an obfuscation compared to using plain 1000. (I kind of understand having a define for a value like 100000 - where counting the zeros gets cumbersome, although 100 * 1000 would be equally clear. But 1000 _is_ really 100% clear, whereas MICRO / MILLI is not).
See above why this way.
...
Quoted 43 lines
Quoted 43 lines
Quoted 16 lines
Quoted 5 lines
drivers/iio/adc/rohm-bd79112.c line 497
Quoted 4 lines
drivers/iio/adc/rohm-bd79112.c new file +3
-0+ gpio_pins =bd79112_get_gpio_pins (iio_dev->channels, + iio_dev->num_channels); +Quoted 8 lines
Instead of leaving this rather unneeded blank line I would move above...
Quoted 1 line
+ /* 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:
Quoted 8 lines
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?
Yes.
Quoted 6 lines
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. */Then you can amend the comment
/* If all channels are reserved for ADC, we are done. */Quoted 4 lines
if (!gpio_pins) return 0;is clearer.
Quoted 7 lines
Quoted 1 line
Which makes my approach sustainable.
I like your wording better, but placing this comment before the call to
bd79112_get_gpio_pins()is still more confusing that placing it before the actual check:if (!gpio_pins)is still misleading. Comment applies to the check, not the retrieval.
The variable assignment, or i.o.w. the source of the value we are testing is also part of the equation.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Matti Vaittinen
maintainerRe: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
Quoted 18 lines · Andy Shevchenko
Quoted 4 lines · Matti Vaittinen
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Matti Vaittinen
Quoted 1 line · Andy Shevchenko
On Tue, Sep 02, 2025 at 03:24:31PM +0300, Matti Vaittinen wrote:
...
Quoted 10 lines
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
+
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).
Rings a bell, but that's what IIO reviewers suggest to do nowadays as a compromise between creating a new bunch of unit (V) related definitions.
Quoted 7 lines
Quoted 4 lines
I am sorry, but this just seems stupid to me. I'd say that it is very obvious for most of the readers dividing microvolts by 1000 results millivolts. And if it is not, then having this MICRO / MILLI is likely to just cause more confusion.
No, it tells that we have a value in microSOMETHING that is converted to MILLIsomething.
No. I disagree. This tells that 'ret' from the regulator API is divided by some unknown value, which is a result of division of two odd defines. Especially odd because one would intuitively think MICRO is smaller than MILLI. You need to look up the definitions to understand WTF is really going on. I think this is plain terrible.
The fact that we store value in vref_mv should be enough of hint that idea is to have value in millivolts. Dividing by 1000 before assigning makes it 100% clear the ret is in microvolts even if you didn't know the regulator API to return micro volts.
Quoted 8 lines
Quoted 1 line
I _really_ dislike these defines. Why is MILLI 1000? Why it isn't 0.001?
You know exactly a few reasons why it's not.
Quoted 2 lines
It makes no sense that KILO and MILLI are the same. Especially not when we are dealing with physics.
Yes, this is the limitation of computers and particularly of _a_ kernel.
No. In my opinion, this is an example of, hopefully unintentional, obfuscation where blindly following some paradigm like 'avoid plain numbers and always use named defines' just results things getting worse. That combined with bad naming. If KILO is 1000, then MILLI can't be 1000. That's 1 per milli.
And still, the original "mv = uv / 1000" is superior in clarity. Using (MICRO / MILLI) there to avoid plain number is just a sign of blindly and religiously following some 'golden rule', even when it results worse code.
Quoted 6 lines
Quoted 4 lines
This is just an obfuscation compared to using plain 1000. (I kind of understand having a define for a value like 100000 - where counting the zeros gets cumbersome, although 100 * 1000 would be equally clear. But 1000 _is_ really 100% clear, whereas MICRO / MILLI is not).
See above why this way.
I see no real justification to degrade this - other than "because I say so". Sorry but that's not really a good reason to me.
Quoted 47 lines
...
Quoted 45 lines
Quoted 44 lines
Quoted 16 lines
Quoted 5 lines
drivers/iio/adc/rohm-bd79112.c line 497
Quoted 4 lines
drivers/iio/adc/rohm-bd79112.c new file +3
-0+ gpio_pins =bd79112_get_gpio_pins (iio_dev->channels, + iio_dev->num_channels); +Quoted 8 lines
Instead of leaving this rather unneeded blank line I would move above...
Quoted 1 line
+ /* 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:
Quoted 8 lines
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?
Yes.
Quoted 6 lines
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. */Then you can amend the comment
/* If all channels are reserved for ADC, we are done. */Quoted 4 lines
if (!gpio_pins) return 0;is clearer.
Which makes my approach sustainable.
Quoted 8 lines
Quoted 5 lines
I like your wording better, but placing this comment before the call to
bd79112_get_gpio_pins()is still more confusing that placing it before the actual check:if (!gpio_pins)is still misleading. Comment applies to the check, not the retrieval.
The variable assignment, or i.o.w. the source of the value we are testing is also part of the equation.
The comment explains why the check, not why the value is obtained.
Yours,
-- Matti
Re: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
Quoted 6 lines · Matti Vaittinen
Quoted 5 lines · Andy Shevchenko
Quoted 4 lines · Matti Vaittinen
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Matti Vaittinen
Quoted 1 line · Andy Shevchenko
On Tue, Sep 02, 2025 at 03:24:31PM +0300, Matti Vaittinen wrote:
...
Quoted 24 lines
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
+
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).
Rings a bell, but that's what IIO reviewers suggest to do nowadays as a compromise between creating a new bunch of unit (V) related definitions.
Quoted 7 lines
Quoted 4 lines
I am sorry, but this just seems stupid to me. I'd say that it is very obvious for most of the readers dividing microvolts by 1000 results millivolts. And if it is not, then having this MICRO / MILLI is likely to just cause more confusion.
No, it tells that we have a value in microSOMETHING that is converted to MILLIsomething.
No. I disagree. This tells that 'ret' from the regulator API is divided by some unknown value, which is a result of division of two odd defines. Especially odd because one would intuitively think MICRO is smaller than MILLI. You need to look up the definitions to understand WTF is really going on. I think this is plain terrible.
Quoted 4 lines
The fact that we store value in
vref_mvshould be enough of hint that idea is to have value in millivolts. Dividing by 1000 before assigning makes it 100% clear the ret is in microvolts even if you didn't know the regulator API to return micro volts.
"Dividing by 1000... makes 100% clear...". I disagree on this. It's not clear and 1000 needs to be counted (it's harder to read, than reader needs to think what mv and not mV (!) means) and so on. For bare minimum make it mV to understand the semantics.
So, since it's one of the principal disagreements between us on this particular issue, I leave it other reviewers and maintainers.
Quoted 7 lines
Quoted 7 lines
Quoted 1 line
I _really_ dislike these defines. Why is MILLI 1000? Why it isn't 0.001?
You know exactly a few reasons why it's not.
Quoted 2 lines
It makes no sense that KILO and MILLI are the same. Especially not when we are dealing with physics.
Quoted 7 lines
Quoted 1 line
Yes, this is the limitation of computers and particularly of _a_ kernel.
No. In my opinion, this is an example of, hopefully unintentional, obfuscation where blindly following some paradigm like 'avoid plain numbers and always use named defines' just results things getting worse. That combined with bad naming. If KILO is 1000, then MILLI can't be 1000. That's 1 per milli.
You have a point, send a patch!
Quoted 3 lines
And still, the original "mv = uv / 1000" is superior in clarity. Using (MICRO / MILLI) there to avoid plain number is just a sign of blindly and religiously following some 'golden rule', even when it results worse code.
No, it's not.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
This is just an obfuscation compared to using plain 1000. (I kind of understand having a define for a value like 100000 - where counting the zeros gets cumbersome, although 100 * 1000 would be equally clear. But 1000 _is_ really 100% clear, whereas MICRO / MILLI is not).
See above why this way.
I see no real justification to degrade this - other than "because I say so".
You may (mis)interpret this, up to you.
Quoted 1 line
Sorry but that's not really a good reason to me.
OK.
...
Quoted 51 lines
Quoted 51 lines
Quoted 50 lines
Quoted 36 lines
Quoted 16 lines
Quoted 5 lines
Review
Quoted 4 lines
(quoted diff) +3
-0+ gpio_pins = bd79112_get_gpio_pins(iio_dev->channels, + iio_dev->num_channels); +Quoted 8 lines
Instead of leaving this rather unneeded blank line I would move above...
Quoted 1 line
+ /* 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:
Quoted 8 lines
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?
Yes.
Quoted 6 lines
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. */Then you can amend the comment
Quoted 8 lines
/* If all channels are reserved for ADC, we are done. */Quoted 4 lines
if (!gpio_pins) return 0;is clearer.
Which makes my approach sustainable.
I like your wording better, but placing this comment before the call to
bd79112_get_gpio_pins()is still more confusing that placing it before the actual check:if (!gpio_pins)is still misleading. Comment applies to the check, not the retrieval.
Quoted 4 lines
Quoted 2 lines
The variable assignment, or i.o.w. the source of the value we are testing is also part of the equation.
The comment explains why the check, not why the value is obtained.
Yes, but value is not taken from out-of-nowhere. It belongs to that check.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Matti Vaittinen
maintainerRe: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
In reply to Andy ShevchenkoQuoted 9 lines · Andy Shevchenko
drivers/iio/adc/rohm-bd79112.c line 455
Quoted 2 lines
drivers/iio/adc/rohm-bd79112.c new file +2
-0+ ret =devm_iio_adc_device_alloc_chaninfo_se (dev, &bd79112_chan_template, +BD79112_MAX_NUM_CHANNELS -1 , &cs);Hmm... Indentation can be amended.
drivers/iio/adc/rohm-bd79112.c line 456
Quoted 1 line
drivers/iio/adc/rohm-bd79112.c new file +1
-0+if (ret <0 ) {Why ' < 0' ?
Quoted 8 lines
drivers/iio/adc/rohm-bd79112.c line 462
Quoted 6 lines
drivers/iio/adc/rohm-bd79112.c new file +6
-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:
I returned back to this after reading your comment to v2.
ret can be positive.
Quoted 8 lines
/* 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.
I still kinda like your suggestion. I'll change this to something like below for v3:
/* Register all pins as GPIOs if there are no ADC channels */
if (ret == -ENOENT)
goto register_gpios;
if (ret < 0)
return ret;Thanks again!
Yours,
-- Matti
David Lechner
reviewerRe: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
In reply to Matti VaittinenQuoted 22 lines · Matti Vaittinen
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.
Signed-off-by Matti Vaittinen <[email protected]>--- drivers/iio/adc/Kconfig | 10 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/rohm-bd79112.c | 542 +++++++++++++++++++++++++++++++++ 3 files changed, 553 insertions(+) create mode 100644 drivers/iio/adc/rohm-bd79112.cdrivers/iio/adc/Kconfig +0
-0
drivers/iio/adc/Kconfig config RN5T618_ADC · line 1315
Quoted 7 lines
drivers/iio/adc/Kconfig +4
-0@@ -1309,6 +1309,16 @@ config RN5T618_ADC This driver can also be built as a module. If so, the module will be called rn5t618-adc. +config ROHM_BD79112 + tristate "Rohm BD79112 ADC driver" + depends on I2C && GPIOLIB + select REGMAP_I2C
I think you want SPI rather than I2C. :-)
Quoted 16 lines
drivers/iio/adc/Kconfig +13
-0+ select IIO_ADC_HELPER + help + Say yes here to build support for the ROHM BD79112 ADC. The + ROHM BD79112 is a 12-bit, 32-channel, SAR ADC, which analog + inputs can also be used for GPIO. + +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;
drivers/iio/adc/Kconfig config RN5T618_ADC
Quoted 10 lines
drivers/iio/adc/Kconfig +10
-0+ 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); + /* 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);
Usually, we only need one __aligned(IIO_DMA_MINALIGN) (on the first field). Since these are only used for SPI messages and we can only send one message at a time, there isn't a way for there to be a problem that would require them to each need to be in their own cache line.
Quoted 7 lines
drivers/iio/adc/Kconfig +4
-0+}; + +static int bd79112_probe(struct spi_device *spi) +{
...
drivers/iio/adc/Kconfig
Quoted 2 lines
drivers/iio/adc/Kconfig +2
-0+ iio_dev->channels = cs; + iio_dev->num_channels = ret;
This is quite far from where it is assigned. Better to have a dedicated local variable for this.
Quoted 4 lines
drivers/iio/adc/Kconfig +4
-0+ iio_dev->info = &bd79112_info; + iio_dev->name = "bd79112"; + iio_dev->modes = INDIO_DIRECT_MODE; +
Matti Vaittinen
maintainerRe: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
Quoted 26 lines · David Lechner
Quoted 25 lines · Matti Vaittinen
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.
Signed-off-by Matti Vaittinen <[email protected]>--- drivers/iio/adc/Kconfig | 10 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/rohm-bd79112.c | 542 +++++++++++++++++++++++++++++++++ 3 files changed, 553 insertions(+)create mode 100644
drivers/iio/adc/rohm-bd79112.cdrivers/iio/adc/Kconfig +0
-0@@ -1309,6 +1309,16 @@ config RN5T618_ADC This driver can also be built as a module. If so, the module will be called rn5t618-adc.
Quoted 6 lines
drivers/iio/adc/Kconfig config RN5T618_ADC · line 1315
Quoted 4 lines
drivers/iio/adc/Kconfig +4
-0+config ROHM_BD79112 + tristate "Rohm BD79112 ADC driver" + depends on I2C && GPIOLIB + select REGMAP_I2CI think you want SPI rather than I2C. :-)
Ouch! :) Well spotted! Thanks!
Quoted 25 lines
Quoted 25 lines
drivers/iio/adc/Kconfig +21
-0+ select IIO_ADC_HELPER + help + Say yes here to build support for the ROHM BD79112 ADC. The + ROHM BD79112 is a 12-bit, 32-channel, SAR ADC, which analog + inputs can also be used for GPIO. + +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; + 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); + /* 8-bit address followed by 8-bit data */ + u8 reg_write_tx[2] __aligned(IIO_DMA_MINALIGN);
Quoted 8 lines
drivers/iio/adc/Kconfig
Quoted 2 lines
drivers/iio/adc/Kconfig +2
-0+ /* 12-bit of ADC data or 8 bit of reg data */ + __be16 read_rx __aligned(IIO_DMA_MINALIGN);Usually, we only need one
__aligned(IIO_DMA_MINALIGN)(on the first field). Since these are only used for SPI messages and we can only send one message at a time, there isn't a way for there to be a problem that would require them to each need to be in their own cache line.
I was wondering about this and hoping to get a good comment explaining it :) I noticed I don't really know how different SPI controllers handle DMA or cache. Hence I just went with what felt like safest option - and hoped to get a comment like yours if it wasn't needed ;) So, Thanks!
Quoted 11 lines
Quoted 8 lines
drivers/iio/adc/Kconfig +4
-0+}; + +static int bd79112_probe(struct spi_device *spi) +{...
Quoted 5 lines
drivers/iio/adc/Kconfig
Quoted 2 lines
drivers/iio/adc/Kconfig +2
-0+ iio_dev->channels = cs; + iio_dev->num_channels = ret;This is quite far from where it is assigned. Better to have a dedicated local variable for this.
Gah! I agree. Actually there is now a bug where the iio_dev->num_channels is used before it is set. So, datasheet names won't be assigned correctly. (I did some re-ordering of stuff in probe to cover the 'all ADCs and all GPIOs cases. I must've messed this at that point!).
Thanks! I feel like I owe you a beer :] Just remind me if we meet! ;)
Yours,
-- Matti
Re: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
In reply to Matti VaittinenHi Matti,
Quoted 12 lines · Matti Vaittinen
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.
Signed-off-by Matti Vaittinen <[email protected]>drivers/iio/adc/rohm-bd79112.c new file +1
-0+static int bd79112_gpio_dir_get (struct gpio_chip *gc,unsigned int offset)
drivers/iio/adc/rohm-bd79112.c line 360
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c new file +10
-0+static int bd79112_gpio_get(struct gpio_chip *gc, unsigned int offset) +static int bd79112_gpio_set(struct gpio_chip *gc, unsigned int offset, + int value) +static int bd79112_gpio_set_multiple(struct gpio_chip *gc, unsigned long *mask, + unsigned long *bits) +static int bd79112_gpio_dir_set(struct bd79112_data *data, unsigned int offset, + int dir) +static int bd79112_gpio_input(struct gpio_chip *gc, unsigned int offset) +static int bd79112_gpio_output(struct gpio_chip *gc, unsigned int offset, + int value)
This looks like it could use
select GPIO_REGMAP
#include <linux/gpio/regmap.h>
struct gpio_regmap_config config = {};
etc. Did you check out the GPIO_REGMAP
helper library? It can work wonders to reduce code footprint, just git grep and look a bit at what the drivers using it does.
Yours,
Linus Walleij
Matti Vaittinen
maintainerRe: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
Hi deee Ho Linus,
Long time no chat. Thanks for the review!
Quoted 29 lines · Linus Walleij
Hi Matti,
Quoted 12 lines · Matti Vaittinen
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.
Signed-off-by Matti Vaittinen <[email protected]>drivers/iio/adc/rohm-bd79112.c new file +1
-0+static int bd79112_gpio_dir_get (struct gpio_chip *gc,unsigned int offset)drivers/iio/adc/rohm-bd79112.c line 360
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c new file +10
-0+static int bd79112_gpio_get(struct gpio_chip *gc, unsigned int offset) +static int bd79112_gpio_set(struct gpio_chip *gc, unsigned int offset, + int value) +static int bd79112_gpio_set_multiple(struct gpio_chip *gc, unsigned long *mask, + unsigned long *bits) +static int bd79112_gpio_dir_set(struct bd79112_data *data, unsigned int offset, + int dir) +static int bd79112_gpio_input(struct gpio_chip *gc, unsigned int offset) +static int bd79112_gpio_output(struct gpio_chip *gc, unsigned int offset, + int value)This looks like it could use
Quoted 8 lines
select
GPIO_REGMAP#include <
linux/gpio/regmap.h>struct
gpio_regmap_configconfig = {};etc. Did you check out the
GPIO_REGMAP
helper library?
I did - but that was couple of years ago :) I was very excited about it back then. I actually tried (tried hard, fingers almost bleeding) to write a patch to make it cover all of the ROHM PMIC GPIOs which I had to deal with. I thought it won't get it's full potential due to it's somewhat inflexible design. (And to tell the truth, I still believe so).
Anyways, fast-forward to this day, I don't see it handling valid_mask. I think it is a must for this device/driver, where pins can be either GPIOs or ADC inputs.
Yours,
-- Matti
Re: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
Quoted 3 lines · Matti Vaittinen
Anyways, fast-forward to this day, I don't see it handling
valid_mask. I think it is a must for this device/driver, where pins can be either GPIOs or ADC inputs.
Why not just add a .init_valid_mask() to struct gpio_regmap_config so it can just pass that down to its gpio_chip?
OK I don't want to load you with too much extra work for the driver, but it seems such a small thing for a blocker, and Michael who wrote the library is really helpful with extending the code, so consider it!
Yours,
Linus Walleij
Matti Vaittinen
maintainerRe: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
Quoted 9 lines · Linus Walleij
Quoted 3 lines · Matti Vaittinen
Anyways, fast-forward to this day, I don't see it handling
valid_mask. I think it is a must for this device/driver, where pins can be either GPIOs or ADC inputs.Why not just add a .init_valid_mask() to struct
gpio_regmap_configso it can just pass that down to itsgpio_chip?
Sigh. I suppose that would technically make sense. (So would allowing other IC-specific callbacks ;) ).
Quoted 4 lines
OK I don't want to load you with too much extra work for the driver, but it seems such a small thing for a blocker, and Michael who wrote the library is really helpful with extending the code, so consider it!
I suppose I can see how that works out. I am not a fan of maintaining the extra code. Thanks for the suggestion.
Yours,
-- Matti
Matti Vaittinen
maintainerRe: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
In reply to Linus WalleijQuoted 9 lines · Linus Walleij
Quoted 3 lines · Matti Vaittinen
Anyways, fast-forward to this day, I don't see it handling
valid_mask. I think it is a must for this device/driver, where pins can be either GPIOs or ADC inputs.Why not just add a .init_valid_mask() to struct
gpio_regmap_configso it can just pass that down to itsgpio_chip?
Sigh. I suppose that would technically make sense. (So would allowing other IC-specific callbacks... ;) ).
Quoted 4 lines
OK I don't want to load you with too much extra work for the driver, but it seems such a small thing for a blocker, and Michael who wrote the library is really helpful with extending the code, so consider it!
I suppose I can see how that works out. I am not a fan of maintaining the extra code. Thanks for the suggestion.
Yours,
-- Matti
Matti Vaittinen
maintainerRe: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
Quoted 11 lines · Matti Vaittinen
Quoted 9 lines · Linus Walleij
Quoted 3 lines · Matti Vaittinen
Anyways, fast-forward to this day, I don't see it handling
valid_mask. I think it is a must for this device/driver, where pins can be either GPIOs or ADC inputs.Why not just add a .init_valid_mask() to struct
gpio_regmap_configso it can just pass that down to itsgpio_chip?
Quoted 8 lines
Sigh. I suppose that would technically make sense. (So would allowing other IC-specific callbacks... ;) ).
Quoted 4 lines
OK I don't want to load you with too much extra work for the driver, but it seems such a small thing for a blocker, and Michael who wrote the library is really helpful with extending the code, so consider it!
I suppose I can see how that works out. I am not a fan of maintaining the extra code. Thanks for the suggestion.
After more thorough look, I don't think the gpio-regmap is flexible enough for this IC (either). The BD79112 direction setting requires accessing two registers (one for input, other for output), and I don't see how this could be done with the current gpio-regmap.
...If only I was able to override the direction setting callback with IC specific one, then I could use the regular gpio-regmap for all the rest...
Yours,
-- Matti
Re: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
In reply to Linus WalleijQuoted 11 lines · Linus Walleij
Quoted 3 lines · Matti Vaittinen
Anyways, fast-forward to this day, I don't see it handling
valid_mask. I think it is a must for this device/driver, where pins can be either GPIOs or ADC inputs.Why not just add a .init_valid_mask() to struct
gpio_regmap_configso it can just pass that down to itsgpio_chip?
Hmm I guess I'll get a similar comment on the series I just sent [1] :)
[1]: https://lore.kernel.org/linux-hwmon/[email protected]/
- Nuno Sá
Quoted 7 lines
OK I don't want to load you with too much extra work for the driver, but it seems such a small thing for a blocker, and Michael who wrote the library is really helpful with extending the code, so consider it!
Yours,
Linus Walleij
Re: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
Quoted 3 lines · Nuno Sá
Quoted 2 lines · Linus Walleij
On Wed, Sep 3, 2025 at 7:23 AM Matti Vaittinen <[email protected]> wrote:
...
Quoted 9 lines
Quoted 7 lines
Quoted 3 lines
Anyways, fast-forward to this day, I don't see it handling
valid_mask. I think it is a must for this device/driver, where pins can be either GPIOs or ADC inputs.Why not just add a .init_valid_mask() to struct
gpio_regmap_configso it can just pass that down to itsgpio_chip?Hmm I guess I'll get a similar comment on the series I just sent [1] :)
Right, you have the mask but you don't use gpio-regmap!
Quoted 1 line
Signature · 2 lines
With Best Regards, Andy Shevchenko
kernel test robot
build botRe: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
In reply to Matti Vaittinenkernel test robot noticed the following build warnings:
Hi Matti,
kernel test robot noticed the following build warnings:
[auto build test WARNING on d1487b0b78720b86ec2a2ac7acc683ec90627e5b]
url: https://github.com/intel-lab-lkp/linux/commits/Matti-Vaittinen/dt-bindings-iio-adc-ROHM-BD79112-ADC-GPIO/20250902-203558 base: d1487b0b78720b86ec2a2ac7acc683ec90627e5b patch link: https://lore.kernel.org/r/08929460fe11dd0b749c50a72a634423f13f4104.1756813980.git.mazziesaccount%40gmail.com patch subject: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO config: sparc-randconfig-r071-20250904 (https://download.01.org/0day-ci/archive/20250904/[email protected]/config) compiler: sparc-linux-gcc (GCC) 14.3.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
smatch warnings:drivers/iio/adc/rohm-bd79112.c:212 bd79112_read_raw() warn: inconsistent indenting
vim +212 drivers/iio/adc/rohm-bd79112.c
192
193 static int bd79112_read_raw(struct iio_dev *indio_dev,
194 struct iio_chan_spec const *chan, int *val,
195 int *val2, long m)
196 {
197 struct bd79112_data *data = iio_priv(indio_dev);
198 int ret;
199
200 switch (m) {
201 case IIO_CHAN_INFO_RAW:
202 ret = regmap_read(data->map, chan->channel, val);
203 if (ret < 0)
204 return ret;
205
206 return IIO_VAL_INT;
207
208 case IIO_CHAN_INFO_SCALE:
209 *val = data->vref_mv;
210 *val2 = 12;
211
> 212 return IIO_VAL_FRACTIONAL_LOG2;
213 }
214
215 return -EINVAL;
216 }
217
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Matti Vaittinen
maintainerPATCH 3/3 MAINTAINERS: Support ROHM BD79112 ADC
In reply to Matti VaittinenAdd the ROHM BD79112 ADC in the list of the BD791xx ADC drivers which are maintained by undersigned.
---
This patch got some last minute changes after other patches were already sent. I hope I got the message-IDs right - but if I didin't - I'm sorry. In that case I'll re-spin series after some delay :)
MAINTAINERS | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
MAINTAINERS +2 -1
@@ -21864,9 +21864,10 @@ S: Supported F: drivers/power/supply/bd99954-charger.c F: drivers/power/supply/bd99954-charger.h -ROHM BD79124 ADC / GPO IC +ROHM BD791xx ADC / GPO IC M: Matti Vaittinen <[email protected]> S: Supported +F: drivers/iio/adc/rohm-bd79112.c F: drivers/iio/adc/rohm-bd79124.c ROHM BH1745 COLOUR SENSOR
Signature · 1 line
2.51.0