Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 13 Apr 2023 11:28:30 +0100
From:   Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To:     Patrik =?ISO-8859-1?Q?Dahlstr=F6m?= <risca@dalakolonin.se>
CC:     <linux-iio@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <letux-kernel@openphoenux.org>, <kernel@pyra-handheld.com>,
        <pgoudagunta@nvidia.com>, <hns@goldelico.com>, <jic23@kernel.org>,
        <lars@metafoo.de>, <linux-omap@vger.kernel.org>
Subject: Re: [PATCH v4 8/9] iio: adc: palmas: add support for iio threshold
 events
Message-ID: <20230413112830.00006279@Huawei.com>
In-Reply-To: <20230408114825.824505-9-risca@dalakolonin.se>
References: <20230408114825.824505-1-risca@dalakolonin.se>
        <20230408114825.824505-9-risca@dalakolonin.se>
Organization: Huawei Technologies Research and Development (UK) Ltd.
MIME-Version: 1.0
Content-Type: text/plain; charset="ISO-8859-1"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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;
> +}
> +