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 <mazziesaccount@gmail.com>
---
Revision history:
v3 => v4:
- Fix Kconfig dependency (I2C => SPI)
- Styling as suggested by Andy and Jonathan
- Moved I/O documentation comment and read/write functions next to each
other and tried clarifying the comment
v2 => v3: (mainly based on review by Andy, thanks!)
- fix broken indentiation
- re-order includes
- drop useless < 0 check for an unsigned offset
- use gc->ngpios instead of hard coded pincount in
for_each_set_clump8()
- drop useless check for zero mask inside for_each_set_clump8() body
- reorder return value checks for the
devm_iio_adc_device_alloc_chaninfo_se() as suggested by Andy. (Well,
I am not 100% happy with it as it results an extra check in the
hopefully most common 'success' -case. But yeah, probe is not exactly
a fast path).
v1 => v2 (mainly based on reviews by Andy and David, thanks!)
- Fix Kconfig dependency to REGMAP_SPI instead of REGMAP_I2C
- Add a few header includes
- Drop unnecessary alignments
- plenty of styling
- use for_each_set_clump8 instead of open-coding it
- change order of direction setting writes to avoid receiving 'event'
when direction is changed from input to output.
- fix data-sheet names and assigning of them to iio_dev
---
drivers/iio/adc/Kconfig | 10 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/rohm-bd79112.c | 553 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 564 insertions(+)
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index e3d3826c335714652726f012018aa5ecb9124a6d..984246b6f57f34e8c097fb92b59df158dd2a14a4 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -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 SPI && GPIOLIB
+ select REGMAP_SPI
+ 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. Analog inputs
+ can also be used for GPIO.
+
config ROHM_BD79124
tristate "Rohm BD79124 ADC driver"
depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 89d72bf9ce70ac1e225fbef83a83dd1a13aef27c..34b40c34cf7191c3367c6c57dcad6d1dbe374824 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -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
diff --git a/drivers/iio/adc/rohm-bd79112.c b/drivers/iio/adc/rohm-bd79112.c
new file mode 100644
index 0000000000000000000000000000000000000000..a2a3affe2c6dc86a237a164139c27ec66dc9d131
--- /dev/null
+++ b/drivers/iio/adc/rohm-bd79112.c
@@ -0,0 +1,553 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ROHM ADC driver for BD79112 signal monitoring hub.
+ * Copyright (C) 2025, ROHM Semiconductor.
+ *
+ * SPI communication derived from ad7923.c and ti-ads7950.c
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/bits.h>
+#include <linux/dev_printk.h>
+#include <linux/err.h>
+#include <linux/errno.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>
+#include <linux/types.h>
+#include <asm/byteorder.h>
+
+#include <linux/iio/adc-helpers.h>
+#include <linux/iio/iio.h>
+
+#define BD79112_MAX_NUM_CHANNELS 32
+
+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];
+ /* 12-bit of ADC data or 8 bit of reg data */
+ __be16 read_rx;
+};
+
+/*
+ * The ADC data is read issuing SPI-command matching the channel number.
+ * We treat this as a register address.
+ */
+#define BD79112_REG_AGIO0A 0x00
+#define BD79112_REG_AGIO15B 0x1f
+
+/*
+ * ADC STATUS_FLAG appended to ADC data will be set, if the ADC result is being
+ * read for a channel, which input pin is muxed to be a GPIO.
+ */
+#define BD79112_ADC_STATUS_FLAG BIT(14)
+
+/*
+ * The BD79112 requires "R/W bit" to be set for SPI register (not ADC data)
+ * reads and an "IOSET bit" to be set for read/write operations (which aren't
+ * reading the ADC data).
+ */
+#define BD79112_BIT_RW BIT(4)
+#define BD79112_BIT_IO BIT(5)
+
+#define BD79112_REG_GPI_VALUE_B8_15 (BD79112_BIT_IO | 0x0)
+#define BD79112_REG_GPI_VALUE_B0_B7 (BD79112_BIT_IO | 0x1)
+#define BD79112_REG_GPI_VALUE_A8_15 (BD79112_BIT_IO | 0x2)
+#define BD79112_REG_GPI_VALUE_A0_A7 (BD79112_BIT_IO | 0x3)
+
+#define BD79112_REG_GPI_EN_B7_B15 (BD79112_BIT_IO | 0x4)
+#define BD79112_REG_GPI_EN_B0_B7 (BD79112_BIT_IO | 0x5)
+#define BD79112_REG_GPI_EN_A8_A15 (BD79112_BIT_IO | 0x6)
+#define BD79112_REG_GPI_EN_A0_A7 (BD79112_BIT_IO | 0x7)
+
+#define BD79112_REG_GPO_EN_B7_B15 (BD79112_BIT_IO | 0x8)
+#define BD79112_REG_GPO_EN_B0_B7 (BD79112_BIT_IO | 0x9)
+#define BD79112_REG_GPO_EN_A8_A15 (BD79112_BIT_IO | 0xa)
+#define BD79112_REG_GPO_EN_A0_A7 (BD79112_BIT_IO | 0xb)
+
+#define BD79112_NUM_GPIO_EN_REGS 8
+#define BD79112_FIRST_GPIO_EN_REG BD79112_REG_GPI_EN_B7_B15
+
+#define BD79112_REG_GPO_VALUE_B8_15 (BD79112_BIT_IO | 0xc)
+#define BD79112_REG_GPO_VALUE_B0_B7 (BD79112_BIT_IO | 0xd)
+#define BD79112_REG_GPO_VALUE_A8_15 (BD79112_BIT_IO | 0xe)
+#define BD79112_REG_GPO_VALUE_A0_A7 (BD79112_BIT_IO | 0xf)
+
+#define BD79112_REG_MAX BD79112_REG_GPO_VALUE_A0_A7
+
+/*
+ * Read transaction consists of two 16-bit sequences separated by CSB.
+ * For register read, 'IOSET' bit must be set. For ADC read, IOSET is cleared
+ * and ADDR equals the channel number (0 ... 31).
+ *
+ * First 16-bit sequence, MOSI as below, MISO data ignored:
+ * - SCK: | 1 | 2 | 3 | 4 | 5 .. 8 | 9 .. 16 |
+ * - MOSI:| 0 | 0 | IOSET | RW (1) | ADDR | 8'b0 |
+ *
+ * CSB released and re-acquired between these sequences
+ *
+ * Second 16-bit sequence, MISO as below, MOSI data ignored:
+ * For Register read data is 8 bits:
+ * - SCK: | 1 .. 8 | 9 .. 16 |
+ * - MISO:| 8'b0 | 8-bit data |
+ *
+ * For ADC read data is 12 bits:
+ * - SCK: | 1 .. 4 | 4 .. 16 |
+ * - MISO:| 4'b0 | 12-bit data |
+ */
+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);
+
+ if (reg & BD79112_BIT_IO && *val & BD79112_ADC_STATUS_FLAG)
+ dev_err(data->dev, "ADC pin configured as GPIO\n");
+
+ return ret;
+}
+
+/*
+ * Write, single 16-bit sequence (broken down below):
+ *
+ * First 8-bit, MOSI as below, MISO data ignored:
+ * - SCK: | 1 | 2 | 3 | 4 | 5 .. 8 |
+ * - MOSI:| 0 | 0 |IOSET| RW(0) | ADDR |
+ *
+ * Last 8 SCK cycles (b8 ... b15), MISO contains register data, MOSI ignored.
+ * - SCK: | 9 .. 16 |
+ * - MISO:| data |
+ */
+static int bd79112_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+ struct bd79112_data *data = context;
+
+ data->reg_write_tx[0] = reg;
+ data->reg_write_tx[1] = val;
+
+ return spi_sync(data->spi, &data->write_msg);
+}
+
+static int _get_gpio_reg(unsigned int offset, unsigned int base)
+{
+ int regoffset = offset / 8;
+
+ if (offset > 31)
+ return -EINVAL;
+
+ return base - regoffset;
+}
+
+#define GET_GPIO_BIT(offset) BIT((offset) % 8)
+#define GET_GPO_EN_REG(offset) _get_gpio_reg((offset), BD79112_REG_GPO_EN_A0_A7)
+#define GET_GPI_EN_REG(offset) _get_gpio_reg((offset), BD79112_REG_GPI_EN_A0_A7)
+#define GET_GPO_VAL_REG(offset) _get_gpio_reg((offset), BD79112_REG_GPO_VALUE_A0_A7)
+#define GET_GPI_VAL_REG(offset) _get_gpio_reg((offset), BD79112_REG_GPI_VALUE_A0_A7)
+
+static const struct regmap_range bd71815_volatile_ro_ranges[] = {
+ {
+ /* Read ADC data */
+ .range_min = BD79112_REG_AGIO0A,
+ .range_max = BD79112_REG_AGIO15B,
+ }, {
+ /* GPI state */
+ .range_min = BD79112_REG_GPI_VALUE_B8_15,
+ .range_max = BD79112_REG_GPI_VALUE_A0_A7,
+ },
+};
+
+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),
+};
+
+static const struct regmap_access_table bd79112_ro_regs = {
+ .no_ranges = &bd71815_volatile_ro_ranges[0],
+ .n_no_ranges = ARRAY_SIZE(bd71815_volatile_ro_ranges),
+};
+
+static const struct regmap_config bd79112_regmap = {
+ .reg_read = bd79112_reg_read,
+ .reg_write = bd79112_reg_write,
+ .volatile_table = &bd79112_volatile_regs,
+ .wr_table = &bd79112_ro_regs,
+ .cache_type = REGCACHE_MAPLE,
+ .max_register = BD79112_REG_MAX,
+};
+
+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;
+ }
+}
+
+static const struct iio_info bd79112_info = {
+ .read_raw = bd79112_read_raw,
+};
+
+static const struct iio_chan_spec bd79112_chan_template = {
+ .type = IIO_VOLTAGE,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+ .indexed = 1,
+};
+
+static int bd79112_gpio_init_valid_mask(struct gpio_chip *gc,
+ unsigned long *valid_mask,
+ unsigned int ngpios)
+{
+ struct bd79112_data *data = gpiochip_get_data(gc);
+
+ *valid_mask = data->gpio_valid_mask;
+
+ return 0;
+}
+
+static int bd79112_gpio_dir_get(struct gpio_chip *gc, unsigned int offset)
+{
+ struct bd79112_data *data = gpiochip_get_data(gc);
+ unsigned int reg, bit, val;
+ int ret;
+
+ bit = GET_GPIO_BIT(offset);
+ reg = GET_GPO_EN_REG(offset);
+
+ ret = regmap_read(data->map, reg, &val);
+ if (ret)
+ return ret;
+
+ if (bit & val)
+ return GPIO_LINE_DIRECTION_OUT;
+
+ reg = GET_GPI_EN_REG(offset);
+ ret = regmap_read(data->map, reg, &val);
+ if (ret)
+ return ret;
+
+ if (bit & val)
+ return GPIO_LINE_DIRECTION_IN;
+
+ /*
+ * Ouch. Seems the pin is ADC input - shouldn't happen as changing mux
+ * at runtime is not supported and non GPIO pins should be invalidated
+ * by the valid_mask at probe. Maybe someone wrote a register bypassing
+ * the driver?
+ */
+ dev_err(data->dev, "Pin not a GPIO\n");
+
+ return -EINVAL;
+}
+
+static int bd79112_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+ struct bd79112_data *data = gpiochip_get_data(gc);
+ unsigned int reg, bit, val;
+ int ret;
+
+ bit = GET_GPIO_BIT(offset);
+ reg = GET_GPI_VAL_REG(offset);
+
+ ret = regmap_read(data->map, reg, &val);
+ if (ret)
+ return ret;
+
+ return !!(val & bit);
+}
+
+static int bd79112_gpio_set(struct gpio_chip *gc, unsigned int offset,
+ int value)
+{
+ struct bd79112_data *data = gpiochip_get_data(gc);
+ unsigned int reg, bit;
+
+ bit = GET_GPIO_BIT(offset);
+ reg = GET_GPO_VAL_REG(offset);
+
+ return regmap_assign_bits(data->map, reg, bit, value);
+}
+
+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) {
+ unsigned long bank_bits;
+ unsigned int reg;
+ int ret;
+
+ 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_gpio_dir_set(struct bd79112_data *data, unsigned int offset,
+ int dir)
+{
+ unsigned int gpi_reg, gpo_reg, bit;
+ int ret;
+
+ bit = GET_GPIO_BIT(offset);
+ gpi_reg = GET_GPI_EN_REG(offset);
+ gpo_reg = GET_GPO_EN_REG(offset);
+
+ if (dir == GPIO_LINE_DIRECTION_OUT) {
+ ret = regmap_clear_bits(data->map, gpi_reg, bit);
+ if (ret)
+ return ret;
+
+ return regmap_set_bits(data->map, gpo_reg, bit);
+ }
+
+ ret = regmap_set_bits(data->map, gpi_reg, bit);
+ if (ret)
+ return ret;
+
+ return regmap_clear_bits(data->map, gpo_reg, bit);
+}
+
+static int bd79112_gpio_input(struct gpio_chip *gc, unsigned int offset)
+{
+ struct bd79112_data *data = gpiochip_get_data(gc);
+
+ return bd79112_gpio_dir_set(data, offset, GPIO_LINE_DIRECTION_IN);
+}
+
+static int bd79112_gpio_output(struct gpio_chip *gc, unsigned int offset,
+ int value)
+{
+ struct bd79112_data *data = gpiochip_get_data(gc);
+ int ret;
+
+ ret = bd79112_gpio_set(gc, offset, value);
+ if (ret)
+ return ret;
+
+ return bd79112_gpio_dir_set(data, offset, GPIO_LINE_DIRECTION_OUT);
+}
+
+static const struct gpio_chip bd79112_gpio_chip = {
+ .label = "bd79112-gpio",
+ .get_direction = bd79112_gpio_dir_get,
+ .direction_input = bd79112_gpio_input,
+ .direction_output = bd79112_gpio_output,
+ .get = bd79112_gpio_get,
+ .set = bd79112_gpio_set,
+ .set_multiple = bd79112_gpio_set_multiple,
+ .init_valid_mask = bd79112_gpio_init_valid_mask,
+ .can_sleep = true,
+ .ngpio = 32,
+ .base = -1,
+};
+
+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);
+ 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", /* 0 - 3 */
+ "AGIO4A", "AGIO5A", "AGIO6A", "AGIO7A", /* 4 - 7 */
+ "AGIO8A", "AGIO9A", "AGIO10A", "AGIO11A", /* 8 - 11 */
+ "AGIO12A", "AGIO13A", "AGIO14A", "AGIO15A", /* 12 - 15 */
+ "AGIO0B", "AGIO1B", "AGIO2B", "AGIO3B", /* 16 - 19 */
+ "AGIO4B", "AGIO5B", "AGIO6B", "AGIO7B", /* 20 - 23 */
+ "AGIO8B", "AGIO9B", "AGIO10B", "AGIO11B", /* 24 - 27 */
+ "AGIO12B", "AGIO13B", "AGIO14B", "AGIO15B", /* 28 - 31 */
+};
+
+static int bd79112_probe(struct spi_device *spi)
+{
+ 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);
+ if (IS_ERR(data->map))
+ return dev_err_probe(dev, PTR_ERR(data->map),
+ "Failed to initialize Regmap\n");
+
+ 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;
+
+ ret = devm_regulator_get_enable(dev, "iovdd");
+ if (ret < 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);
+
+ 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);
+
+ /* Register all pins as GPIOs if there are no ADC channels */
+ if (ret == -ENOENT)
+ goto register_gpios;
+
+ if (ret < 0)
+ return ret;
+
+ iio_dev->num_channels = ret;
+ iio_dev->channels = cs;
+
+ for (i = 0; i < iio_dev->num_channels; i++) {
+ unsigned int ch = cs[i].channel;
+
+ cs[i].datasheet_name = bd79112_chan_names[ch];
+ }
+
+ 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);
+
+ /* If all channels are reserved for ADC, then we're done. */
+ if (!gpio_pins)
+ return 0;
+
+ /* Default all the GPIO pins to GPI */
+ for_each_set_bit(pin, &gpio_pins, BD79112_MAX_NUM_CHANNELS) {
+ 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);
+}
+
+static const struct of_device_id bd79112_of_match[] = {
+ { .compatible = "rohm,bd79112" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, bd79112_of_match);
+
+static const struct spi_device_id bd79112_id[] = {
+ { "bd79112" },
+ { }
+};
+MODULE_DEVICE_TABLE(spi, bd79112_id);
+
+static struct spi_driver bd79112_driver = {
+ .driver = {
+ .name = "bd79112",
+ .of_match_table = bd79112_of_match,
+ },
+ .probe = bd79112_probe,
+ .id_table = bd79112_id,
+};
+module_spi_driver(bd79112_driver);
+
+MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>");
+MODULE_DESCRIPTION("Driver for ROHM BD79112 ADC/GPIO");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("IIO_DRIVER");
--
2.51.0
On 9/10/25 6:24 AM, Matti Vaittinen wrote:
...
> diff --git a/drivers/iio/adc/rohm-bd79112.c b/drivers/iio/adc/rohm-bd79112.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..a2a3affe2c6dc86a237a164139c27ec66dc9d131
> --- /dev/null
> +++ b/drivers/iio/adc/rohm-bd79112.c
> @@ -0,0 +1,553 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ROHM ADC driver for BD79112 signal monitoring hub.
> + * Copyright (C) 2025, ROHM Semiconductor.
> + *
> + * SPI communication derived from ad7923.c and ti-ads7950.c
Really? I wrote the ti-ads7950 driver and I can't say I see the
resemblance. ;-)
> + */
> +
...
> +static int bd79112_get_gpio_pins(const struct iio_chan_spec *cs, int num_channels)
u32 would make more sense when dealing with bit flags.
> +{
> + int i, gpio_channels;
same for the local variable.
...
> +static int bd79112_probe(struct spi_device *spi)
> +{
...
> +
> + 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);
If these messages never change (other than the data in the buffers), you can
call devm_spi_optimize_message() here on each message to get reduced CPU usage
on every SPI message for free.
On 12/09/2025 00:20, David Lechner wrote:
> On 9/10/25 6:24 AM, Matti Vaittinen wrote:
>
> ...
>
>> diff --git a/drivers/iio/adc/rohm-bd79112.c b/drivers/iio/adc/rohm-bd79112.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..a2a3affe2c6dc86a237a164139c27ec66dc9d131
>> --- /dev/null
>> +++ b/drivers/iio/adc/rohm-bd79112.c
>> @@ -0,0 +1,553 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * ROHM ADC driver for BD79112 signal monitoring hub.
>> + * Copyright (C) 2025, ROHM Semiconductor.
>> + *
>> + * SPI communication derived from ad7923.c and ti-ads7950.c
>
> Really? I wrote the ti-ads7950 driver and I can't say I see the
> resemblance. ;-)
Really. :) I picked the idea of populating the transfers in the probe
and using buffers from the driver-data, from these drivers :) Well, I
admit it ended up being a bit different - but the starting point was
those drivers ;)
>
>> + */
>> +
>
> ...
>
>> +static int bd79112_get_gpio_pins(const struct iio_chan_spec *cs, int num_channels)
>
> u32 would make more sense when dealing with bit flags.
>
>> +{
>> + int i, gpio_channels;
>
> same for the local variable.
Meh. Ok :)
> ...
>
>> +static int bd79112_probe(struct spi_device *spi)
>> +{
>
> ...
>
>> +
>> + 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);
>
> If these messages never change (other than the data in the buffers), you can
> call devm_spi_optimize_message() here on each message to get reduced CPU usage
> on every SPI message for free.
>
Thanks!
Yours,
-- Matti
On Wed, 10 Sep 2025 14:24:35 +0300
Matti Vaittinen <mazziesaccount@gmail.com> 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.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Hi Matti,
A few trivial things that I'll tidy up if nothing else comes up (I might not
bother given how trivial they are!)
Also one question. I couldn't immediately follow why any random register
read is sanity checking if an ADC pin is configured as GPIO.
Jonathan
> diff --git a/drivers/iio/adc/rohm-bd79112.c b/drivers/iio/adc/rohm-bd79112.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..a2a3affe2c6dc86a237a164139c27ec66dc9d131
> --- /dev/null
> +++ b/drivers/iio/adc/rohm-bd79112.c
> @@ -0,0 +1,553 @@
> +/*
> + * The BD79112 requires "R/W bit" to be set for SPI register (not ADC data)
> + * reads and an "IOSET bit" to be set for read/write operations (which aren't
> + * reading the ADC data).
> + */
> +/*
> + * Read transaction consists of two 16-bit sequences separated by CSB.
> + * For register read, 'IOSET' bit must be set. For ADC read, IOSET is cleared
> + * and ADDR equals the channel number (0 ... 31).
> + *
> + * First 16-bit sequence, MOSI as below, MISO data ignored:
> + * - SCK: | 1 | 2 | 3 | 4 | 5 .. 8 | 9 .. 16 |
> + * - MOSI:| 0 | 0 | IOSET | RW (1) | ADDR | 8'b0 |
> + *
> + * CSB released and re-acquired between these sequences
> + *
> + * Second 16-bit sequence, MISO as below, MOSI data ignored:
> + * For Register read data is 8 bits:
> + * - SCK: | 1 .. 8 | 9 .. 16 |
> + * - MISO:| 8'b0 | 8-bit data |
> + *
> + * For ADC read data is 12 bits:
> + * - SCK: | 1 .. 4 | 4 .. 16 |
> + * - MISO:| 4'b0 | 12-bit data |
> + */
> +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);
> +
> + if (reg & BD79112_BIT_IO && *val & BD79112_ADC_STATUS_FLAG)
> + dev_err(data->dev, "ADC pin configured as GPIO\n");
Why are we checking this in a regmap callback?
Maybe it needs rewording and the point is some missmatch in what we
can read vs the state?
> +
> + return ret;
> +}
> +
> +static int bd79112_probe(struct spi_device *spi)
> +{
> + 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);
data->mpa = devm_regmap_init(dev, ...
> + if (IS_ERR(data->map))
> + return dev_err_probe(dev, PTR_ERR(data->map),
> + "Failed to initialize Regmap\n");
> +
> + 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;
> +
> + ret = devm_regulator_get_enable(dev, "iovdd");
> + if (ret < 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);
> +
> + 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);
> +
> + /* Register all pins as GPIOs if there are no ADC channels */
> + if (ret == -ENOENT)
> + goto register_gpios;
> +
> + if (ret < 0)
> + return ret;
> +
> + iio_dev->num_channels = ret;
> + iio_dev->channels = cs;
> +
> + for (i = 0; i < iio_dev->num_channels; i++) {
> + unsigned int ch = cs[i].channel;
> +
> + cs[i].datasheet_name = bd79112_chan_names[ch];
Could have done
cs[i].datasheet_name = bd79112_chan_names[cs[i].channel];
and I don't think it makes it harder to read, but doesn't matter enough to respin.
> + }
Morning Jonathan,
On 10/09/2025 20:46, Jonathan Cameron wrote:
> On Wed, 10 Sep 2025 14:24:35 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> 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.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>
> Hi Matti,
>
> A few trivial things that I'll tidy up if nothing else comes up (I might not
> bother given how trivial they are!)
Thanks again!
> Also one question. I couldn't immediately follow why any random register
> read is sanity checking if an ADC pin is configured as GPIO.
>
Ah. Valid question! I see my comment below is partially wrong.
>> +/*
>> + * Read transaction consists of two 16-bit sequences separated by CSB.
>> + * For register read, 'IOSET' bit must be set. For ADC read, IOSET is cleared
>> + * and ADDR equals the channel number (0 ... 31).
>> + *
>> + * First 16-bit sequence, MOSI as below, MISO data ignored:
>> + * - SCK: | 1 | 2 | 3 | 4 | 5 .. 8 | 9 .. 16 |
>> + * - MOSI:| 0 | 0 | IOSET | RW (1) | ADDR | 8'b0 |
>> + *
>> + * CSB released and re-acquired between these sequences
>> + *
>> + * Second 16-bit sequence, MISO as below, MOSI data ignored:
>> + * For Register read data is 8 bits:
>> + * - SCK: | 1 .. 8 | 9 .. 16 |
>> + * - MISO:| 8'b0 | 8-bit data |
>> + *
>> + * For ADC read data is 12 bits:
>> + * - SCK: | 1 .. 4 | 4 .. 16 |
>> + * - MISO:| 4'b0 | 12-bit data |
This is not 100% true. I overlooked the ADC read "status flag" when
adding this comment for the ADC data reading.
This should be:
* For ADC, read data is 12 bits prepended with a status flag:
* - SCK: | 1 | 2 | 3 4 | 4 .. 16 |
* - MISO:| 0 | STATUS_FLAG | 2'b0 | 12-bit data |
The 'STATUS_FLAG' is set if the input pin is configured as a GPIO.
>> + */
>> +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);
>> +
>> + if (reg & BD79112_BIT_IO && *val & BD79112_ADC_STATUS_FLAG)
>> + dev_err(data->dev, "ADC pin configured as GPIO\n");
>
> Why are we checking this in a regmap callback?
> Maybe it needs rewording and the point is some missmatch in what we
> can read vs the state?
>
>> +
>> + return ret;
>> +}
>
>> +
>> +static int bd79112_probe(struct spi_device *spi)
>> +{
>> + 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);
>
> data->mpa = devm_regmap_init(dev, ...
>
>
>> + if (IS_ERR(data->map))
>> + return dev_err_probe(dev, PTR_ERR(data->map),
>> + "Failed to initialize Regmap\n");
>> +
>> + 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;
>> +
>> + ret = devm_regulator_get_enable(dev, "iovdd");
>> + if (ret < 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);
>> +
>> + 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);
>> +
>> + /* Register all pins as GPIOs if there are no ADC channels */
>> + if (ret == -ENOENT)
>> + goto register_gpios;
>> +
>> + if (ret < 0)
>> + return ret;
>> +
>> + iio_dev->num_channels = ret;
>> + iio_dev->channels = cs;
>> +
>> + for (i = 0; i < iio_dev->num_channels; i++) {
>> + unsigned int ch = cs[i].channel;
>> +
>> + cs[i].datasheet_name = bd79112_chan_names[ch];
>
> Could have done
>
> cs[i].datasheet_name = bd79112_chan_names[cs[i].channel];
>
> and I don't think it makes it harder to read, but doesn't matter enough to respin.
I kind of agree. It stays short and allows us to get rid of the
brackets. Thanks!
I can still re-spin this later this week, assuming you rather not fix
the data-format comment while applying :)
Thanks for pointing this out!
Yours,
-- Matti
On Thu, 11 Sep 2025 08:13:03 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > Morning Jonathan, > > On 10/09/2025 20:46, Jonathan Cameron wrote: > > On Wed, 10 Sep 2025 14:24:35 +0300 > > Matti Vaittinen <mazziesaccount@gmail.com> 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. > >> > >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > > > > Hi Matti, > > > > A few trivial things that I'll tidy up if nothing else comes up (I might not > > bother given how trivial they are!) > > Thanks again! > > > Also one question. I couldn't immediately follow why any random register > > read is sanity checking if an ADC pin is configured as GPIO. > > > > Ah. Valid question! I see my comment below is partially wrong. > > > >> +/* > >> + * Read transaction consists of two 16-bit sequences separated by CSB. > >> + * For register read, 'IOSET' bit must be set. For ADC read, IOSET is cleared > >> + * and ADDR equals the channel number (0 ... 31). > >> + * > >> + * First 16-bit sequence, MOSI as below, MISO data ignored: > >> + * - SCK: | 1 | 2 | 3 | 4 | 5 .. 8 | 9 .. 16 | > >> + * - MOSI:| 0 | 0 | IOSET | RW (1) | ADDR | 8'b0 | > >> + * > >> + * CSB released and re-acquired between these sequences > >> + * > >> + * Second 16-bit sequence, MISO as below, MOSI data ignored: > >> + * For Register read data is 8 bits: > >> + * - SCK: | 1 .. 8 | 9 .. 16 | > >> + * - MISO:| 8'b0 | 8-bit data | > >> + * > >> + * For ADC read data is 12 bits: > >> + * - SCK: | 1 .. 4 | 4 .. 16 | > >> + * - MISO:| 4'b0 | 12-bit data | > > This is not 100% true. I overlooked the ADC read "status flag" when > adding this comment for the ADC data reading. > > This should be: > > * For ADC, read data is 12 bits prepended with a status flag: > * - SCK: | 1 | 2 | 3 4 | 4 .. 16 | > * - MISO:| 0 | STATUS_FLAG | 2'b0 | 12-bit data | > > The 'STATUS_FLAG' is set if the input pin is configured as a GPIO. That's good additional info, but I'm still struggling on why we are effectively providing a 'debug' check in ever register read. My assumption is that it should never fire unless you have a driver bug? Jonathan
On 13/09/2025 15:24, Jonathan Cameron wrote: > On Thu, 11 Sep 2025 08:13:03 +0300 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> Morning Jonathan, >> >> On 10/09/2025 20:46, Jonathan Cameron wrote: >>> On Wed, 10 Sep 2025 14:24:35 +0300 >>> Matti Vaittinen <mazziesaccount@gmail.com> 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. >>>> >>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> >>> >>> Hi Matti, >>> >>> A few trivial things that I'll tidy up if nothing else comes up (I might not >>> bother given how trivial they are!) >> >> Thanks again! >> >>> Also one question. I couldn't immediately follow why any random register >>> read is sanity checking if an ADC pin is configured as GPIO. >>> >> >> Ah. Valid question! I see my comment below is partially wrong. >> >> >>>> +/* >>>> + * Read transaction consists of two 16-bit sequences separated by CSB. >>>> + * For register read, 'IOSET' bit must be set. For ADC read, IOSET is cleared >>>> + * and ADDR equals the channel number (0 ... 31). >>>> + * >>>> + * First 16-bit sequence, MOSI as below, MISO data ignored: >>>> + * - SCK: | 1 | 2 | 3 | 4 | 5 .. 8 | 9 .. 16 | >>>> + * - MOSI:| 0 | 0 | IOSET | RW (1) | ADDR | 8'b0 | >>>> + * >>>> + * CSB released and re-acquired between these sequences >>>> + * >>>> + * Second 16-bit sequence, MISO as below, MOSI data ignored: >>>> + * For Register read data is 8 bits: >>>> + * - SCK: | 1 .. 8 | 9 .. 16 | >>>> + * - MISO:| 8'b0 | 8-bit data | >>>> + * >>>> + * For ADC read data is 12 bits: >>>> + * - SCK: | 1 .. 4 | 4 .. 16 | >>>> + * - MISO:| 4'b0 | 12-bit data | >> >> This is not 100% true. I overlooked the ADC read "status flag" when >> adding this comment for the ADC data reading. >> >> This should be: >> >> * For ADC, read data is 12 bits prepended with a status flag: >> * - SCK: | 1 | 2 | 3 4 | 4 .. 16 | >> * - MISO:| 0 | STATUS_FLAG | 2'b0 | 12-bit data | >> >> The 'STATUS_FLAG' is set if the input pin is configured as a GPIO. > > That's good additional info, but I'm still struggling on why > we are effectively providing a 'debug' check in ever register > read. My assumption is that it should never fire unless you have > a driver bug? Yes, a driver bug or someone accessing the ADC outside the driver. I kind of agree the check shouldn't be needed - but I've seen quite a few driver bugs during my career. XD The check is _very_ light weight compared to the SPI access time - but you're right that it is done at every ADC data read - which is 'hot path'. As a result, I am not sure whether to leave or drop it. Yours, -- Matti > > Jonathan
On Sun, 14 Sep 2025 12:25:06 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 13/09/2025 15:24, Jonathan Cameron wrote: > > On Thu, 11 Sep 2025 08:13:03 +0300 > > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > > > >> Morning Jonathan, > >> > >> On 10/09/2025 20:46, Jonathan Cameron wrote: > >>> On Wed, 10 Sep 2025 14:24:35 +0300 > >>> Matti Vaittinen <mazziesaccount@gmail.com> 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. > >>>> > >>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > >>> > >>> Hi Matti, > >>> > >>> A few trivial things that I'll tidy up if nothing else comes up (I might not > >>> bother given how trivial they are!) > >> > >> Thanks again! > >> > >>> Also one question. I couldn't immediately follow why any random register > >>> read is sanity checking if an ADC pin is configured as GPIO. > >>> > >> > >> Ah. Valid question! I see my comment below is partially wrong. > >> > >> > >>>> +/* > >>>> + * Read transaction consists of two 16-bit sequences separated by CSB. > >>>> + * For register read, 'IOSET' bit must be set. For ADC read, IOSET is cleared > >>>> + * and ADDR equals the channel number (0 ... 31). > >>>> + * > >>>> + * First 16-bit sequence, MOSI as below, MISO data ignored: > >>>> + * - SCK: | 1 | 2 | 3 | 4 | 5 .. 8 | 9 .. 16 | > >>>> + * - MOSI:| 0 | 0 | IOSET | RW (1) | ADDR | 8'b0 | > >>>> + * > >>>> + * CSB released and re-acquired between these sequences > >>>> + * > >>>> + * Second 16-bit sequence, MISO as below, MOSI data ignored: > >>>> + * For Register read data is 8 bits: > >>>> + * - SCK: | 1 .. 8 | 9 .. 16 | > >>>> + * - MISO:| 8'b0 | 8-bit data | > >>>> + * > >>>> + * For ADC read data is 12 bits: > >>>> + * - SCK: | 1 .. 4 | 4 .. 16 | > >>>> + * - MISO:| 4'b0 | 12-bit data | > >> > >> This is not 100% true. I overlooked the ADC read "status flag" when > >> adding this comment for the ADC data reading. > >> > >> This should be: > >> > >> * For ADC, read data is 12 bits prepended with a status flag: > >> * - SCK: | 1 | 2 | 3 4 | 4 .. 16 | > >> * - MISO:| 0 | STATUS_FLAG | 2'b0 | 12-bit data | > >> > >> The 'STATUS_FLAG' is set if the input pin is configured as a GPIO. > > > > That's good additional info, but I'm still struggling on why > > we are effectively providing a 'debug' check in ever register > > read. My assumption is that it should never fire unless you have > > a driver bug? > > Yes, a driver bug or someone accessing the ADC outside the driver. > > I kind of agree the check shouldn't be needed - but I've seen quite a > few driver bugs during my career. XD The check is _very_ light weight > compared to the SPI access time - but you're right that it is done at > every ADC data read - which is 'hot path'. As a result, I am not sure > whether to leave or drop it. Maybe just add a comment along the lines of /* Lets check this whilst here, but should never happen! */ > > Yours, > -- Matti > > > > > Jonathan > >
© 2016 - 2025 Red Hat, Inc.