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;
Message-ID: <1b7f1c34-1c98-be1c-5a22-e154ae5443aa@gmail.com>
Date:   Thu, 30 Mar 2023 19:59:49 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Content-Language: en-US, en-GB
To:     Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Cc:     Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Zhigang Shi <Zhigang.Shi@liteon.com>,
        Shreeya Patel <shreeya.patel@collabora.com>,
        Dmitry Osipenko <dmitry.osipenko@collabora.com>,
        Paul Gazzillo <paul@pgazz.com>, linux-kernel@vger.kernel.org,
        linux-iio@vger.kernel.org
References: <cover.1679915278.git.mazziesaccount@gmail.com>
 <1b7d3725bf5ae829b12eaf96362204edd29c6966.1679915278.git.mazziesaccount@gmail.com>
From:   Matti Vaittinen <mazziesaccount@gmail.com>
Subject: Re: [PATCH v6 6/7] iio: light: ROHM BU27034 Ambient Light Sensor
In-Reply-To: <1b7d3725bf5ae829b12eaf96362204edd29c6966.1679915278.git.mazziesaccount@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/27/23 14:34, Matti Vaittinen wrote:
> ROHM BU27034 is an ambient light sensor with 3 channels and 3 photo diodes
> capable of detecting a very wide range of illuminance. Typical application
> is adjusting LCD and backlight power of TVs and mobile phones.
> 
> Add initial  support for the ROHM BU27034 ambient light sensor.
> 
> NOTE:
> 	- Driver exposes 4 channels. One IIO_LIGHT channel providing the
> 	  calculated lux values based on measured data from diodes #0 and
> 	  #1. In addition, 3 IIO_INTENSITY channels are emitting the raw
> 	  register data from all diodes for more intense user-space
> 	  computations.
> 	- Sensor has GAIN values that can be adjusted from 1x to 4096x.
> 	- Sensor has adjustible measurement times of 5, 55, 100, 200 and
> 	  400 mS. Driver does not support 5 mS which has special
> 	  limitations.
> 	- Driver exposes standard 'scale' adjustment which is
> 	  implemented by:
> 		1) Trying to adjust only the GAIN
> 		2) If GAIN adjustment alone can't provide requested
> 		   scale, adjusting both the time and the gain is
> 		   attempted.
> 	- Driver exposes writable INT_TIME property that can be used
> 	  for adjusting the measurement time. Time adjustment will also
> 	  cause the driver to try to adjust the GAIN so that the
> 	  overall scale is kept as close to the original as possible.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> ---

...

> +/*
> + * We try to change the time in such way that the scale is maintained for
> + * given channels by adjusting gain so that it compensates the time change.
> + */
> +static int bu27034_try_set_int_time(struct bu27034_data *data, int time_us)
> +{
> +	struct bu27034_gain_check gains[] = {
> +		{ .chan = BU27034_CHAN_DATA0 },
> +		{ .chan = BU27034_CHAN_DATA1 },
> +	};
> +	int numg = ARRAY_SIZE(gains);
> +	int ret, int_time_old, i;
> +
> +	mutex_lock(&data->mutex);
> +	ret = bu27034_get_int_time(data);
> +	if (ret < 0)
> +		goto unlock_out;
> +
> +	int_time_old = ret;
> +
> +	if (!iio_gts_valid_time(&data->gts, time_us)) {
> +		dev_err(data->dev, "Unsupported integration time %u\n",
> +			time_us);
> +		ret = -EINVAL;
> +
> +		goto unlock_out;
> +	}
> +
> +	if (time_us == int_time_old) {
> +		ret = 0;
> +		goto unlock_out;
> +	}
> +
> +	for (i = 0; i < numg; i++) {
> +		ret = bu27034_get_gain(data, gains[i].chan, &gains[i].old_gain);
> +		if (ret)
> +			goto unlock_out;
> +
> +		ret = iio_gts_find_new_gain_by_old_gain_time(&data->gts,
> +							     gains[i].old_gain,
> +							     int_time_old, time_us,
> +							     &gains[i].new_gain);
> +		if (ret) {

As mentioned in my comment to the helper patch, we should have different 
returnvalue here depending on if the given times were invalid (and new 
gain was not computed) or if the new_gain was computed but not 
supported. I plan to use -ERANGE to denote "new gain computed but not 
supported" and add...

> +			int scale1, scale2;
> +			bool ok;
> +
> +			_bu27034_get_scale(data, gains[i].chan, &scale1, &scale2);
> +			dev_dbg(data->dev,
> +				"chan %u, can't support time %u with scale %u %u\n",
> +				gains[i].chan, time_us, scale1, scale2);

+                       if (ret != -ERANGE)
+                               goto unlock_out;

... here at v7.

> +
> +			/*
> +			 * If caller requests for integration time change and we
> +			 * can't support the scale - then the caller should be
> +			 * prepared to 'pick up the pieces and deal with the
> +			 * fact that the scale changed'.
> +			 */
> +			ret = iio_find_closest_gain_low(&data->gts,
> +							gains[i].new_gain, &ok);
> +
> +			if (!ok) {
> +				dev_dbg(data->dev,
> +					"optimal gain out of range for chan %u\n",
> +					gains[i].chan);
> +			}
> +			if (ret < 0) {
> +				dev_dbg(data->dev,
> +					 "Total gain increase. Risk of saturation");
> +				ret = iio_gts_get_min_gain(&data->gts);
> +				if (ret < 0)
> +					goto unlock_out;
> +			}
> +			dev_dbg(data->dev, "chan %u scale changed\n",
> +				 gains[i].chan);
> +			gains[i].new_gain = ret;
> +			dev_dbg(data->dev, "chan %u new gain %u\n",
> +				gains[i].chan, gains[i].new_gain);
> +		}
> +	}
> +
> +	for (i = 0; i < numg; i++) {
> +		ret = bu27034_set_gain(data, gains[i].chan, gains[i].new_gain);
> +		if (ret)
> +			goto unlock_out;
> +	}
> +
> +	ret = bu27034_set_int_time(data, time_us);
> +
> +unlock_out:
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +


Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~