Re: [PATCH] iio: adc: Add Intel Dollar Cove TI PMIC ADC driver

[Hans de Goede <hansg@kernel.org>]

Hi,

On 18-Jul-25 12:56 PM, Hans de Goede wrote:
> Hi Jonathan,
> 
> Thank you for the review and sorry for being quite slow in
> working on the next revision.
> 
> Note this is really part of a fuel-gauge setup series,
> so for the next revision I'll include this driver in
> the upcoming v3 posting of that series (it can still be
> merged independently from the rest of the series).

<snip>

>>> +static int dc_ti_adc_read_raw(struct iio_dev *indio_dev,
>>> +			      struct iio_chan_spec const *chan,
>>> +			      int *val, int *val2, long mask)
>>> +{
>>> +	struct dc_ti_adc_info *info = iio_priv(indio_dev);
>>> +	int ret, ch = chan->channel;
>>> +	unsigned int lsb, msb;
>>> +
>>> +	if (mask != IIO_CHAN_INFO_RAW)
>>> +		return -EINVAL;
>>> +
>>> +	guard(mutex)(&info->lock);
>>> +
>>> +	info->conversion_done = false;
>>> +
>>> +	/*
>>> +	 * If channel BPTHERM has been selected, first enable the BPTHERM BIAS
>>> +	 * which provides the VREFT Voltage reference to convert BPTHERM Input
>>> +	 * voltage to temperature.
>>> +	 * As per PMIC Vendor specifications, BPTHERM BIAS should be enabled
>>> +	 * 35 ms before ADC_EN command.
>>> +	 */
>>> +	if (ch == DC_TI_ADC_BATTEMP) {
>>> +		ret = regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
>>> +					 DC_TI_ADC_EN_EXT_BPTH_BIAS,
>>> +					 DC_TI_ADC_EN_EXT_BPTH_BIAS);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +		msleep(35);
>>> +	}
>>> +
>>> +	/*
>>> +	 * As per TI (PMIC Vendor), the ADC enable and ADC start commands should
>>> +	 * not be sent together. Hence send the commands separately
>>> +	 */
>>> +	ret = regmap_update_bits(info->regmap, DC_TI_ADC_CNTL_REG,
>>> +				 DC_TI_ADC_EN, DC_TI_ADC_EN);
>>> +	if (ret < 0)
>>> +		goto disable_adc;
>> Always a corner case of what to do about disabling when an enable fail.
>> We'd hope it never happens but in general I'd assume no side effects occured
>> and return here rather than the goto.
> 
> Ok.

Correction, the goto is needed to undo the setting of
DC_TI_ADC_EN_EXT_BPTH_BIAS done above, so I'm keeping this as is.

(although it is ikely that once we start getting register update
errors doing more register updates will likely also fail...)

Regards,

Hans