Message-ID: <ca1999c9-910b-558f-d4ca-267809db8cfb@fi.rohmeurope.com>
Date:   Mon, 12 Jun 2023 06:36:38 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Matti Vaittinen <mazziesaccount@...il.com>
CC:     Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Zhigang Shi <Zhigang.Shi@...eon.com>,
        Paul Gazzillo <paul@...zz.com>,
        Shreeya Patel <shreeya.patel@...labora.com>,
        Dmitry Osipenko <dmitry.osipenko@...labora.com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Mutanen, Mikko" <Mikko.Mutanen@...rohmeurope.com>
Subject: Re: [PATCH v5 0/5] Support ROHM BU27008 RGB sensor

On 6/9/23 20:19, Jonathan Cameron wrote:
> On Fri, 9 Jun 2023 15:46:21 +0300
> Matti Vaittinen <mazziesaccount@...il.com> wrote:
> 
>> 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?
> 
> History...
> 
>> (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.
> 
> IIO_VAL_INTO_PLUS_NANO might solve this
> and you'll need to provide the callback write_raw_get_fmt() if you aren't
> already so that the conversion from string to val and val2 takes into
> account that the driver expects val2 to be *10^-9
> 

It seems the biggest problem (once again) was my ignorance :) I didn't 
know of write_raw_get_fmt(). I think You just really saved my day! After 
a quick glance it seems to me that all I need to do is indeed just to 
implement the write_raw_get_fmt() callback in BU27xxx drivers.

I was already getting a bit uncomfortable as I have promised to do few 
other things and I just hit this issue - which seemed much bigger that 
it now is. On top of this I'll be in Embedded Linux Conf for the last 
week of June (I hope to meet some of you guys there!) - and after that I 
plan to take a month off... So, things started to pile up once again. 
Now, Jonathan just lifted off a rock from my shoulders - big thanks!

I hope I'll be able to cook some patches Today or tomorrow.

> 
> Given that I think you just need to have the driver tell the core it wants
> IIO_VAL_INT_PLUS_NANO.  Problem still occurs, but several orders of magnitude
> smaller.

Several magnitudes indeed!

> But I may be miss understanding.

No :) It was me who didn't (once again) see all the cool things provided 
by IIO :)

Yours,
	-- Matti

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

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

Powered by blists - more mailing lists