Re: [PATCH 2/4] iio: adc: add ti-ads112c14 driver

[Andy Shevchenko]

On Mon, Jun 15, 2026 at 05:00:00PM -0500, David Lechner (TI) wrote:
> Add a new driver for the TI ADS112C14/ADS122C14 ADC chips.
> 
> This first step is adding a very basic driver that only supports power
> on/reset and reading the system monitor channels.
> 
> ADS112C14_SYS_MON_CHANNEL_SHORT is the last channel rather than being in
> logical order by address to keep the voltage channels together and in
> case we find we need to add variants of this channel with different
> voltage reference later.

...

> +#include <linux/bitfield.h>

+ bitops.h // BIT(), GENMASK(), sign_extend32()

> +#include <linux/delay.h>
> +#include <linux/dev_printk.h>
> +#include <linux/i2c.h>

> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>

I believe we discussed that (with Jonathan) already and seems the trend is to
imply that iio/types.h is always included whenever iio/iio.h is included.

> +#include <linux/math64.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/sysfs.h>
> +#include <linux/time64.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>

...

> +/* Datasheet t_d(RST) - time to wait after reset before next I2C use. */
> +#define ADS112C14_DELAY_RESET_us 500

For µs (and second related units) we traditionally use _US suffix
(yeah, I know...). Of course, we can start a SI schema in new code
but it will look quite strange and probably more confusing.

...

> +struct ads112c14_data {
> +	const struct ads112c14_chip_info *chip_info;

> +	struct i2c_client *client;

No need to keep it, it's one time use and can be derived from regmap.

regmap --> dev --> i2c_client.

> +	struct regmap *regmap;
> +};

...

> +static int ads112c14_read_label(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan, char *label)
> +{
> +	const char *label_source;

I don't see the need of having this. Can't be returned directly?

> +	/* System monitor channels. */
> +	switch (chan->channel) {
> +	case ADS112C14_SYS_MON_CHANNEL_TEMP:
> +		label_source = "Internal temperature sensor";
> +		break;
> +	case ADS112C14_SYS_MON_CHANNEL_EXT_REF:
> +		label_source = "External reference";
> +		break;
> +	case ADS112C14_SYS_MON_CHANNEL_AVDD:
> +		label_source = "AVDD";
> +		break;
> +	case ADS112C14_SYS_MON_CHANNEL_DVDD:
> +		label_source = "DVDD";
> +		break;
> +	case ADS112C14_SYS_MON_CHANNEL_SHORT:
> +		label_source = "Internal short";
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return sysfs_emit(label, "%s\n", label_source);
> +}

...

> +static int ads112c14_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	const struct ads112c14_chip_info *info;
> +	struct iio_dev *indio_dev;
> +	struct ads112c14_data *data;
> +	u32 reg_val;
> +	int ret;
> +
> +	info = i2c_get_match_data(client);

NULL check (yeah) due to driver_override issue.

> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));

Here, and below, use 'dev'.

> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->chip_info = info;
> +	data->client = client;
> +
> +	ret = devm_regulator_get_enable(&client->dev, "dvdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get dvdd regulator\n");
> +
> +	ret = devm_regulator_get_enable(&client->dev, "avdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get avdd regulator\n");
> +
> +	data->regmap = devm_regmap_init_i2c(client, &ads112c14_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(dev, PTR_ERR(data->regmap),
> +				     "failed to init regmap\n");
> +
> +	/* Write magic reset value (0x16) to ensure known state.*/
> +	ret = regmap_write(data->regmap, ADS112C14_REG_CONVERSION_CTRL,
> +			   FIELD_PREP(ADS112C14_CONVERSION_CTRL_RESET, 0x16));
> +	/*
> +	 * The reset may cause an -EREMOTEIO error because of failing to get the
> +	 * I2C ACK at the end of the message. The device still gets reset.
> +	 */
> +	if (ret != -EREMOTEIO)
> +		return ret;

I would do it separately as

	if (ret == -EREMOTEIO)
		/* ...big comment here... */
		return 0;
	if (ret) // which is regular pattern and doesn't need any comment.
		return ret;

> +	fsleep(ADS112C14_DELAY_RESET_us);
> +
> +	ret = regmap_read(data->regmap, ADS112C14_REG_STATUS_MSB, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	if (FIELD_GET(ADS112C14_STATUS_MSB_RESETN, reg_val))
> +		return dev_err_probe(dev, -EIO, "reset failed\n");
> +
> +	/*
> +	 * Clear reset bit to prepare for next probe. And clear AVDD fault since
> +	 * that happens on every reset.
> +	 */
> +	ret = regmap_write(data->regmap, ADS112C14_REG_STATUS_MSB,
> +			   ADS112C14_STATUS_MSB_RESETN |
> +			   ADS112C14_STATUS_MSB_AVDD_UVN);
> +	if (ret)
> +		return ret;
> +
> +	/* Place in single-shot conversion mode to make ready for raw read. */
> +	ret = regmap_set_bits(data->regmap, ADS112C14_REG_DEVICE_CFG,
> +			      ADS112C14_DEVICE_CFG_CONV_MODE);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = info->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ads112c14_sys_mon_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ads112c14_sys_mon_channels);
> +	indio_dev->info = &ads112c14_info;
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}

-- 
With Best Regards,
Andy Shevchenko