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

[David Lechner]

On 6/16/26 2:32 AM, Andy Shevchenko wrote:
> 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.
> 
> ...


>> +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?

Then this would have to be split into two functions. One to return
it directly and one to do the sysfs_emit(). I don't think it is
worth it.

> 
>> +	/* 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);
>> +}
> 


>> +	/*
>> +	 * 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;

We should not return early here. We just continue with the rest
of the function as normal. So I think the way I had it was
simplest. Otherwise we would need a goto or something like that.


> 	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");
>> +