On Fri, 9 Jun 2023 15:46:21 +0300 Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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 > > 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. It's common for drivers to round to nearest to allow for subtle differences due to precision. That may make sense here. > > 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... 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. But I may be miss understanding. Jonathan > > Yours, > -- Matti >
