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:   Sun, 14 Aug 2022 17:22:32 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Shreeya Patel <shreeya.patel@collabora.com>
Cc:     lars@metafoo.de, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org, krisman@collabora.com,
        kernel@collabora.com, alvaro.soliverez@collabora.com
Subject: Re: [PATCH v2] iio: light: ltrf216a: Add raw attribute
Message-ID: <20220814172232.4caeaf1c@jic23-huawei>
In-Reply-To: <20220812100424.529425-1-shreeya.patel@collabora.com>
References: <20220812100424.529425-1-shreeya.patel@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri, 12 Aug 2022 15:34:24 +0530
Shreeya Patel <shreeya.patel@collabora.com> wrote:

> Add IIO_CHAN_INFO_RAW to the mask to be able to read raw values
> from the light sensor.
> 
> The userspace code for brightness control in steam deck uses the
> in_illuminance_input value through sysfs and multiplies it
> with a constant stored in BIOS at factory calibration time.
> 
> The downstream driver for LTRF216A that we have been using
> has incorrect formula for LUX calculation which we corrected
> in the upstreamed driver.
> 
> Now to be able to use the upstreamed driver, we need to add some
> magic in userspace so that the brightness control works like before
> even with the updated LUX formula.
> 
> Hence, we need the raw data to calculate a constant that can be
> added in userspace code.
> 
> Downstream driver LUX formula :-
> (greendata*8*LTRF216A_WIN_FAC) / (data->als_gain_fac*data->int_time_fac*10)
> 
> Upstreamed driver LUX formula :-
> (greendata*45*LTRF216A_WIN_FAC) / (data->als_gain_fac*data->int_time_fac)
> 
> greendata is the ALS_DATA which we would like to get through sysfs using
> the raw attribute.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>

Hi Shreeya.

Your description above makes me wonder though if we should support
this as an intensity channel as we did for many of the early Ambient light
sensors.  Not sure why it's called 'greendata' btw!
For those early tsl2583 IIRC and similar, we had two sensors with infrared vs
visible+infrared (which is basically what clear is here).
The readings given were of those two sensors then we did a bunch of maths
to convert those to LUX (in simplest drivers we simply subtracted
the infrared part from visible and applied a scale factor)

That lead to IIO_TYPE_BOTH though we later added IIO_TYPE_CLEAR which is
subtly different as that was for color sensors with RGB and clearish
filters.  The value you want here doesn't really correspond to any of
those modifiers

I guess that brings us back around to LIGHT(illuminance) + raw as you have it.
or adding a 'visible' modifier which is also rather ugly and hard
to define.

Let's leave this on list a while longer to see if others comment.
For now I'm inclined to just accept this as a dirty hack needed for this
corner case.

Jonathan

> ---
> 
> Changes in v2
>   - Add a better commit message explaining why we want this change.
>   - Call ltrf216a_set_power_state(data, false) before return.
> 
> 
>  drivers/iio/light/ltrf216a.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
> index e6e24e70d2b9..4b8ef36b6912 100644
> --- a/drivers/iio/light/ltrf216a.c
> +++ b/drivers/iio/light/ltrf216a.c
> @@ -93,6 +93,7 @@ static const struct iio_chan_spec ltrf216a_channels[] = {
>  	{
>  		.type = IIO_LIGHT,
>  		.info_mask_separate =
> +			BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_PROCESSED) |
>  			BIT(IIO_CHAN_INFO_INT_TIME),
>  		.info_mask_separate_available =
> @@ -259,6 +260,18 @@ static int ltrf216a_read_raw(struct iio_dev *indio_dev,
>  	int ret;
>  
>  	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = ltrf216a_set_power_state(data, true);
> +		if (ret)
> +			return ret;
> +		mutex_lock(&data->lock);
> +		ret = ltrf216a_read_data(data, LTRF216A_ALS_DATA_0);
> +		mutex_unlock(&data->lock);
> +		ltrf216a_set_power_state(data, false);
> +		if (ret < 0)
> +			return ret;
> +		*val = ret;
> +		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_PROCESSED:
>  		mutex_lock(&data->lock);
>  		ret = ltrf216a_get_lux(data);