On 4/10/23 19:16, Jonathan Cameron wrote:
On Tue, 4 Apr 2023 15:04:38 +0300
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
Hi deeee Ho peeps,
I have following questions:
1) I have no good knowledge as to what units the register values
represent. I know the greater value informs greater color intensity -
but that's about it. I currently just send out the raw register values
via IIO_INTENSITY raw channel - but I don't know if this is usual or if
typical user-space would expect the values to be some how 'normalized'?
Given these colour curves are normally devices specific, it's often very hard
to give them any units that can be compared across devices. There are standard
colour spaces, (CIE 1931 RGB for example) but basic sensors tend not to be matched
to these. Sometimes there are documented approximate conversion calculations
(similar to the ones you get for illuminance calculations). In some cases
these are highly non linear with different approximations for different parts
of the colour space.
Okay. In that case I'll probably just go with the raw register values,
thanks!
With the current setup user-space needs to either just compare the
different channel values to each other to decide which colour dominates
- or perform some manual calibration using known light sources. I have
no idea if this is usual approach with RGB sensors?
It seems to me that
for example the adjd_s311.c just returns raw register values - but I
don't know what the format is. Any insight on if the values should
represent some 'units' or if they can really just be 'register values
proportional to intensity of measured colour'.
There isn't usually a way of measuring which colour dominates. That depends
on all sorts of things that affect human perception of colour. All you can
measure is that the intensity of a particular colour is itself increasing
or decreasing.
2) The gain setting is once again ... eh ... complicated. The RGB and C
channels are sharing gain setting. There are a few supported gain values
- ranging from 1X to 1024X. The IR channel _shares_ again the high bits
of GAIN setting with the RGBC channels. Two lowest bits can be set
independently - but again, quite a few gain 'selector' field values are
marked as forbidden.
To make it worse, the IR gain values matching the selector field are
same as for RGBC - except the first selector. The sel 0 equals to gain
1X on RGBC, but gain 2X for IR. (1X is not supported for IR). So,
changing gain selector from 0 => 1 will cause gain to jump from 1X => 4X
for RGBC but from 2X => 4X for IR channel.
That's "novel". Feel free to tell the Rohm hardware folk that I think they
are crazy in a really unhelpful way.
As a ROHM employee, I should probably be a bit taken aback - but this
actually made me laugh. Well, I was on a vacation when I read this so
maybe it is Ok ;)
Well, ROHM hardware folks have a long history of making hardware, and
typically hardware for a specific customer and for a specific need.
They're actually truly novel at times - and they tend to solve pretty
impossible problems with pretty "out of the box" solutions (at times).
Unfortunately, these novel solutions which solved a specific problem for
a specific customer tend to be inherited to next products where these
'hacks' may no longer have a purpose. Furthermore, (as far as I know)
for a long period of time, the software for ROHM ICs was also written
either by a customer, or by a subcontractor. The (driver) SW was written
for a specific project and it was not necessary to make it fit for
generic use-cases - hence, things like this 1X Vs 2X gain did not really
matter as SW could be tailored to just do the right thing for a specific
product. When the product was out the project was done.
This means that there has been no "feed-back" loop from SW engineers to
many of the ROHM HW colleagues. So, even though you hit to the spot with
the comment - there is a very understandable reasons behind things being
somewhat ... peculiar ;)
And yes, I have been and continue to be giving feed-back to hardware
colleagues. And even though I am very much tempted of using your quote
as my email signature, I guess it is more productive to try a bit more
gentle approach with the feed-back. XD
I see two options:
1) Use fixed high bits which means supporting only 4X and 16X gains -
for which changing the low selector bits is enough. In this case the
RGBC would have own gain setting, IR would have own and there would be
no shared bits.
That's pretty nasty as I suspect the high bits are the useful ones.
I kind of agree. So, while that would be the simplest thing, it would
also drop most of the gains the HW is capable of.
2) Allow full range of supported gains to be set for RGBC - and disallow
setting gain for IR. However, change the IR gain to have same selector
as RGBC gain when RGBC gain is changed. (This prevents IR gain selector
from changing to an unsupported value when RGBC gain is changed). This
means that if user-space changes the gain for RGBC, it should also
read-back the gain for IR to detect the change. I have no idea if
existing user-space apps do this.
We've had other cases where setting a gain affects a subset of channels
(shared gains for pairs of channels for instance). In those cases userspace
has to read back the values. I suspect there are no userspace programs
that do anything more complex than setting gain to a value in a config
file. So if that's valid then all is good.
Oh, interesting. I don't know how userspace apps utilize these sensors.
Nor do I know what these RGB sensors are mostly used for. I however
thought userspace apps would be detecting saturation / measurement
values going close to zero, and did dynamically change scale based on
this. Config file sounds like most of the apps just use a static
settings - which indeed simplifies things.
I think that no matter if we select option 1) or option 2) - we must
have own scale entries for all channels. This is needed for option 2)
because of the 1X vs 2X difference mentioned above.
I have currently implemented the option 2) because it supports wider
variety of gains - but I am unsure if this is "the right thing to do".
Any insight is appreciated!
Hmm. Given the pairwise case I mention above requires that any write to
gain can change any other and there is no strong reason it must change it
to the same value as the one written. So setting channel 1 scale to x2 might
result in channel 2 scale doubling from x2 to x4 (think of a fixed x2 multiplier
on channel 2 with a pga in front of it (this craziness happens on SoC ADCs
sometimes) I think you can be more generic about this.
Have all channels have their own scale + scale_available.
Ok. My current draft did not have 'scale_available' for IR because
setting scale for IR was forbidden.
A write to any of them changes the value with exception of when it affects only
the lower bits where you can separate off the IR channel. If the change requires
changing the upper bits than fine it changes the IR gain as well.
Thanks for the suggestion. In my head this felt too complex but now that
you wrote it down, it does not really be that complex. We can simply
compare the old and new values for high bits and set the lower bits for
other channels only when high bits change. (I wrote it like this because
I am at the same time working with yet another RGB sensor model, with
yet another messy 'shared bits' gain setting. The other sensor has
individual 'low bits' for all channels, while having shared high bits...)
Another option comes to mind. Just have one scale value and don't allow the
lowest gain value. That way you can always program the scales to the same value
by setting both registers. So basically hid the oddity of that different
1x vs 2x initial scale by not supporting it.
Yes. That would be an option. Not supporting 1X makes the 'saturation
point' for RGBC to jump 4X lower though... I will see how the code looks
like when implementing the 'check if high bits changed' logic you
suggested. Well, thanks a LOT for the help! This means I will soon(ish)
pour some more patches to your review queue :) (Might be I do some PMIC
work before that though).
Thanks,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
