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: <0173eb2b-b6a5-b90a-9740-7a65f806fabc@gmail.com>
Date:   Fri, 9 Jun 2023 15:46:21 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.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>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Zhigang Shi <Zhigang.Shi@liteon.com>,
        Paul Gazzillo <paul@pgazz.com>,
        Shreeya Patel <shreeya.patel@collabora.com>,
        Dmitry Osipenko <dmitry.osipenko@collabora.com>,
        linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        "Mutanen, Mikko" <Mikko.Mutanen@fi.rohmeurope.com>
References: <cover.1683541225.git.mazziesaccount@gmail.com>
From:   Matti Vaittinen <mazziesaccount@gmail.com>
Subject: Re: [PATCH v5 0/5] Support ROHM BU27008 RGB sensor
In-Reply-To: <cover.1683541225.git.mazziesaccount@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/8/23 13:30, Matti Vaittinen wrote:
> Add support for ROHM BU27008 RGB sensor.
> 
> The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear
> and IR) with four configurable channels. Red and green being always
> available and two out of the rest three (blue, clear, IR) can be
> selected to be simultaneously measured. Typical application is adjusting
> LCD backlight of TVs, mobile phones and tablet PCs.
> 
> This series supports reading the RGBC and IR channels using IIO
> framework. However, only two of the BC+IR can be enabled at the same
> time. Series adds also support for scale and integration time
> configuration, where scale consists of impact of both the integration
> time and hardware gain. The gain and time support is backed by the newly
> introduced IIO GTS helper. This series depends on GTS helper patches
> added in BU27034 support series which is already merged in iio/togreg
> which this series is based on.

I started adding support for the BU27010 RGBC + flickering sensor to the 
BU27008 driver. While at it, I wrote some test(s) which try using also 
the 'insane' gain settings.

What I found out is that the scale setting for BU27008 is broken for 
smallest scales: 0.007812500 0.003906250 0.001953125

Reason is the accuracy.

The GTS helpers were made to use NANO scale accuracy. 999999999 is still 
fitting in an 32 bit integer after all :) This allows to handle greater 
"total gains".

The IIO scale setting interface towards the drivers seems to crop the 
val2 to micros (6 digits). This means that when user writes scale 
0.001953125 via sysfs - the driver will get val = 0, val2 = 1953. 
Currently the BU27008 driver (and probably also the BU27035 which I have 
not yet checked) will pass this value to GTS-helpers - which try to use 
it in computations where scale is tried to be converted to gain + 
integration time settings. This will fail because of rounding error this 
leads to.

Regarding the BU27* drivers I see this bug as annoying rather than 
urgent. Bug will appear only with the very smallest of scales - which 
means gains of magnitude ~1000X with the longest integration times - and 
as someone once said - 1000X gains sound pretty insane as errors will 
probably get quite big... Still, this is a bug - and it bothers me :)

What comes to fixing this - my first thought regarding "the right thing 
to do" would be improving the IIO scale setting accuracy. I wonder if 
there has been some heavy reason(s) to only provide 6 digits of val2? (I 
haven't yet looked how IIO formats the val2 from user input so I may be 
very ignorant here). For userland this fix should be relatively 
invisible - the write of for example 0.001953125 is seemingly successful 
from the user-space POV. IIO does not warn about the excess accuracy.

I am not saying this change would be risk-free. For sure there is an 
application somewhere passing this kind of 'high accuracy' scale values 
to sysfs. And it may be we have a driver which is going to have a hiccup 
is such value is passed to it - but I'd argue the driver should be fixed 
then. It's easier for a driver to drop the excess digits by a division - 
than it is to generate the missing digits...

...which leads us to the other potential way of papering over this 
issue. We could go on defining a set of "magic scale values" in the 
bu27008 driver, namely the 1953, 3906 and 7812 - and when these are used 
as val2 just assume it means 001953125, 003906250 and 007812500 
respectively. This would be quick and simple fix - but it would also 
mean this is a driver specific hack.

Finally, we could dive into GTS helpers and drop the accuracy of those 
to MIRCO scale instead of the NANO. If this was to be done it might be 
best to change the BU27008 and BU27034 intensity channel scales to start 
from bigger integers. Yes, it would potentially break any existing user 
of those intensity channels - but I suspect the amount of such users is 
still 0.

Finally, if we really want to keep the accuracy of scales in micros and 
not support nanos, then we probably should adjust the available scales 
displaying to not accept IIO_VAL_INT_PLUS_NANO type lists...

Yours,
	-- Matti

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

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