Re: [PATCH v4 8/9] iio: adc: palmas: add support for iio threshold events

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Sat,  8 Apr 2023 13:48:24 +0200
Patrik Dahlström <risca@dalakolonin.se> wrote:

> The palmas gpadc block has support for monitoring up to 2 ADC channels
> and issue an interrupt if they reach past a set threshold. This change
> hooks into the IIO events system and exposes to userspace the ability to
> configure these threshold values for each channel, but only allow up to
> 2 such thresholds to be enabled at any given time. Trying to enable a
> third channel will result in an error.
> 
> Userspace is expected to input calibrated, as opposed to raw, values as
> threshold. However, it is not enough to do the opposite of what is done
> when converting the other way around. To account for tolerances in the
> ADC, the calculated raw threshold should be adjusted based on the ADC
> specifications for the device. These specifications include the integral
> nonlinearity (INL), offset, and gain error. To adjust the high
> threshold, use the following equation:
> 
>   (calibrated value + INL) * Gain error + offset = maximum value  [1]
> 
> Likewise, use the following equation for the low threshold:
> 
>   (calibrated value - INL) * Gain error - offset = minimum value
> 
> The gain error is a combination of gain error, as listed in the
> datasheet, and gain error drift due to temperature and supply. The exact
> values for these specifications vary between palmas devices. This patch
> sets the values found in TWL6035, TWL6037 datasheet.
> 
> [1] TI Application Report, SLIA087A, Guide to Using the GPADC in
>     TPS65903x, TPS65917-Q1, TPS65919-Q1, and TPS65916 Devices.
> 
> Signed-off-by: Patrik Dahlström <risca@dalakolonin.se>

0-day found some stuff we'd missed in here.

I've fixed it up and pushed out again.

> ---
...
> +static int palmas_gpadc_write_event_value(struct iio_dev *indio_dev,
> +					  const struct iio_chan_spec *chan,
> +					  enum iio_event_type type,
> +					  enum iio_event_direction dir,
> +					  enum iio_event_info info,
> +					  int val, int val2)
> +{
> +	struct palmas_gpadc *adc = iio_priv(indio_dev);
> +	int adc_chan = chan->channel;
> +	int old;
> +	int ret;
> +
> +	if (adc_chan > PALMAS_ADC_CH_MAX || type != IIO_EV_TYPE_THRESH)
> +		return -EINVAL;
> +
> +	mutex_lock(&adc->lock);
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		if (val < 0 || val > 0xFFF) {
> +			ret = -EINVAL;
> +			break;

Should be goto out_unlock; Found because old is undefined.

> +		}
> +		if (dir == IIO_EV_DIR_RISING) {
> +			old = adc->thresholds[adc_chan].high;
> +			adc->thresholds[adc_chan].high = val;
> +		}

Whilst here should be } else {

Tidied both up force an update on the testing branch.

> +		else {
> +			old = adc->thresholds[adc_chan].low;
> +			adc->thresholds[adc_chan].low = val;
> +		}
> +		ret = 0;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	if (val != old && palmas_gpadc_get_event(adc, adc_chan, dir))
> +		ret = palmas_gpadc_reconfigure_event_channels(adc);
> +
> +out_unlock:
> +	mutex_unlock(&adc->lock);
> +
> +	return ret;
> +}
> +