Re: [PATCH v5 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 15 Sep 2025 17:12:34 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Mon, Sep 15, 2025 at 10:12:43AM +0300, Matti Vaittinen wrote:
> > The ROHM BD79112 is an ADC/GPIO with 32 channels. The channel inputs can
> > be used as ADC or GPIO. Using the GPIOs as IRQ sources isn't supported.
> > 
> > The ADC is 12-bit, supporting input voltages up to 5.7V, and separate I/O
> > voltage supply. Maximum SPI clock rate is 20 MHz (10 MHz with
> > daisy-chain configuration) and maximum sampling rate is 1MSPS.
> > 
> > The IC does also support CRC but it is not implemented in the driver.  
> 
> ...
> 
> > +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(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;  
> 
> I still think moving to _mV is the right thing to do.
> There is no 'mv' in the physics for Volts.

I'm not disagreeing with this review but I'm also not going to hold a
driver back for that given timing is pretty much such that I merge it
today or it sits a cycle and this one is very near...
I'll get fussier on this once we have written up some guidance and may
well send a patch to modify existing recent cases like this one!

> 
> > +	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);  
> 
> > +	devm_spi_optimize_message(dev, spi, &data->read_msg);  
> 
> And if it fails?..
I've added the following and applied the series.

Note I'm cutting this fine so if we get any build issues or similar
it might well get pushed back to next cycle yet!

diff --git a/drivers/iio/adc/rohm-bd79112.c b/drivers/iio/adc/rohm-bd79112.c
index b406d4ee5411..d15e06c8b94d 100644
--- a/drivers/iio/adc/rohm-bd79112.c
+++ b/drivers/iio/adc/rohm-bd79112.c
@@ -454,12 +454,18 @@ static int bd79112_probe(struct spi_device *spi)
        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);
-       devm_spi_optimize_message(dev, spi, &data->read_msg);
+       ret = devm_spi_optimize_message(dev, spi, &data->read_msg);
+       if (ret < 0)
+               return dev_err_probe(dev, ret,
+                                    "Failed to optimize SPI read message\n");
 
        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);
-       devm_spi_optimize_message(dev, spi, &data->write_msg);
+       ret = devm_spi_optimize_message(dev, spi, &data->write_msg);
+       if (ret < 0)
+               return dev_err_probe(dev, ret,
+                                    "Failed to optimize SPI write message\n");
 
        ret = devm_iio_adc_device_alloc_chaninfo_se(dev, &bd79112_chan_template,
                                                    BD79112_MAX_NUM_CHANNELS - 1,
> 
> > +	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);
> > +	devm_spi_optimize_message(dev, spi, &data->write_msg);
> > +
> > +	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++)
> > +		cs[i].datasheet_name = bd79112_chan_names[cs[i].channel];
> > +
> > +	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);
> > +}  
>