| Message ID | cover.1683541225.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Support ROHM BU27008 RGB sensor | expand |
On Mon, 8 May 2023 13:30:28 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > Add support for ROHM BU27008 RGB sensor. Series applied to the togreg branch of iio.git and pushed out initially as testing for all the normal reasons. Thanks, Jonathan > > 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. > > The hardware allows configuring gain setting by writing a 5-bit gain > selector value to a register. Part of the gain setting is common for all > channels (RGBC + IR) but part of the selector value can be set > separately for RGBC and IR: > > MODE_CONTROL2 REG: > bit 7 6 5 4 3 2 1 0 > ----------------------------------------------------------------- > | RGB selector | > +---------------------------------------+ > ----------------------------------------------------------------- > | high bits IR | | low bits IR selector | > +---------------+ +-----------------------+ > > In theory it would be possible to set certain separate gain values for > RGBC and IR channels, but this gets pretty confusing because there are a > few 'unsupported' selector values. If only RGBC or IR was set, some > extra handling should be done to prevent the other channel from getting > unsupported value due to change in high-bits. Furthermore, allowing the > channels to be set different gain values (in some cases when gains are > such the HW supports it) would make the cases where also integration > time is changed to achieve correct scale ... interesting. It might also > be confusing for user to try predicting when setting different scales > succeeds and when it does not. Furthermore, if for example the scale > setting for RGBC caused IR selector to be invalid - it could also cause > the IR scale to "jump" very far from previous value. > > To make the code simpler and more predictable for users, the current > logic is as follows: > > 1. Prevent setting IR scale. (My assumption is IR is less used than > RGBC) > 2. When RGBC scale is set, set also the IR-selector to the same value. > This prevents unsupported selector values and makes the IR scale changes > predictable. > > The 2) could mean we effectively have the same scale for all channels. > Unfortunately, the HW design is slightly peculiar and selector 0 means > gain 1X on RGBC but gain 2X on IR. Rest of the selectors equal same gain > values on RGBC and IR. The result is that while changin selector from 0 > => 1 causes RGBC gain to go from 1X => 4X, it causes IR gain to go from > 2X => 4X. > > So, the driver provides separate scale entries for all channels (also > RGB and C will have separate gain entries because these channels are of > same type as IR channel). This makes it possible for user applications > to go read the scales for all channels after setting scale for one (in > order to detect the IR scale difference). > > Having the separate IR scale entry which applications can read to detect > "arbitrary scale changes" makes it possible for applications to be > written so they can cope if we need to implement the 'allow setting some > different gains for IR and RGBC' - later. > > Finally, the scales_available is also provided for all other channels > except the IR channel, which does not allow the scale to be changed. > > The sensor provides a data-ready IRQ and the driver implements a > triggered buffer mode using this IRQ as a trigger. > > Finally, the series introduces generic iio_validate_own_trigger() helper > which can be used as a validate_trigger callback for drivers which > require the trigger and iio-device to be parented by same device. The > KX022A driver is converted to use this new callback instead of rolling > it's own function. The new helper and KX022A can be merged in as > independent changes if need be. > > > Revision history > v3 => v4: > iio_validate_own_trigger: > - kernel-doc fix > bu27008 driver fixes: > - re-enable IRQ in trigger renable-callback > - do trigger setup on own function > - drop regmap lock > - configure channels in appropriate callback > v3 => v4: > bu27008 driver fixes: > - Drop thread from device IRQ handler > - Styling and some minor improvements > - Use kernel-doc for enums > - Correctly order entries in Makefile > v2 => v3: > dt-bindings: > - No changes > iio_validate_own_trigger: > - subject fix > bu27008: > - Mostly styling based on comments from Andy and Andi > > More accurate changelog in individual patches > > v1 => v2: > dt-bindings: > - Fix issues pointed by Krzysztof. > bu27008 driver: > - Fix issues pointed by Jonathan > Add new helper for validating own trigger > > More accurate changelog in individual patches > > --- > > Matti Vaittinen (5): > dt-bindings: iio: light: ROHM BU27008 > iio: trigger: Add simple trigger_validation helper > iio: kx022a: Use new iio_validate_own_trigger() > iio: light: ROHM BU27008 color sensor > MAINTAINERS: Add ROHM BU27008 > > .../bindings/iio/light/rohm,bu27008.yaml | 49 + > MAINTAINERS | 3 +- > drivers/iio/accel/kionix-kx022a.c | 13 +- > drivers/iio/industrialio-trigger.c | 22 +- > drivers/iio/light/Kconfig | 14 + > drivers/iio/light/Makefile | 1 + > drivers/iio/light/rohm-bu27008.c | 1026 +++++++++++++++++ > include/linux/iio/trigger.h | 1 + > 8 files changed, 1115 insertions(+), 14 deletions(-) > create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bu27008.yaml > create mode 100644 drivers/iio/light/rohm-bu27008.c >
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
On Fri, 9 Jun 2023 15:46:21 +0300 Matti Vaittinen <mazziesaccount@gmail.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 > > 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 >
On 6/9/23 20:19, Jonathan Cameron wrote: > On Fri, 9 Jun 2023 15:46:21 +0300 > Matti Vaittinen <mazziesaccount@gmail.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
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. The hardware allows configuring gain setting by writing a 5-bit gain selector value to a register. Part of the gain setting is common for all channels (RGBC + IR) but part of the selector value can be set separately for RGBC and IR: MODE_CONTROL2 REG: bit 7 6 5 4 3 2 1 0 ----------------------------------------------------------------- | RGB selector | +---------------------------------------+ ----------------------------------------------------------------- | high bits IR | | low bits IR selector | +---------------+ +-----------------------+ In theory it would be possible to set certain separate gain values for RGBC and IR channels, but this gets pretty confusing because there are a few 'unsupported' selector values. If only RGBC or IR was set, some extra handling should be done to prevent the other channel from getting unsupported value due to change in high-bits. Furthermore, allowing the channels to be set different gain values (in some cases when gains are such the HW supports it) would make the cases where also integration time is changed to achieve correct scale ... interesting. It might also be confusing for user to try predicting when setting different scales succeeds and when it does not. Furthermore, if for example the scale setting for RGBC caused IR selector to be invalid - it could also cause the IR scale to "jump" very far from previous value. To make the code simpler and more predictable for users, the current logic is as follows: 1. Prevent setting IR scale. (My assumption is IR is less used than RGBC) 2. When RGBC scale is set, set also the IR-selector to the same value. This prevents unsupported selector values and makes the IR scale changes predictable. The 2) could mean we effectively have the same scale for all channels. Unfortunately, the HW design is slightly peculiar and selector 0 means gain 1X on RGBC but gain 2X on IR. Rest of the selectors equal same gain values on RGBC and IR. The result is that while changin selector from 0 => 1 causes RGBC gain to go from 1X => 4X, it causes IR gain to go from 2X => 4X. So, the driver provides separate scale entries for all channels (also RGB and C will have separate gain entries because these channels are of same type as IR channel). This makes it possible for user applications to go read the scales for all channels after setting scale for one (in order to detect the IR scale difference). Having the separate IR scale entry which applications can read to detect "arbitrary scale changes" makes it possible for applications to be written so they can cope if we need to implement the 'allow setting some different gains for IR and RGBC' - later. Finally, the scales_available is also provided for all other channels except the IR channel, which does not allow the scale to be changed. The sensor provides a data-ready IRQ and the driver implements a triggered buffer mode using this IRQ as a trigger. Finally, the series introduces generic iio_validate_own_trigger() helper which can be used as a validate_trigger callback for drivers which require the trigger and iio-device to be parented by same device. The KX022A driver is converted to use this new callback instead of rolling it's own function. The new helper and KX022A can be merged in as independent changes if need be. Revision history v3 => v4: iio_validate_own_trigger: - kernel-doc fix bu27008 driver fixes: - re-enable IRQ in trigger renable-callback - do trigger setup on own function - drop regmap lock - configure channels in appropriate callback v3 => v4: bu27008 driver fixes: - Drop thread from device IRQ handler - Styling and some minor improvements - Use kernel-doc for enums - Correctly order entries in Makefile v2 => v3: dt-bindings: - No changes iio_validate_own_trigger: - subject fix bu27008: - Mostly styling based on comments from Andy and Andi More accurate changelog in individual patches v1 => v2: dt-bindings: - Fix issues pointed by Krzysztof. bu27008 driver: - Fix issues pointed by Jonathan Add new helper for validating own trigger More accurate changelog in individual patches --- Matti Vaittinen (5): dt-bindings: iio: light: ROHM BU27008 iio: trigger: Add simple trigger_validation helper iio: kx022a: Use new iio_validate_own_trigger() iio: light: ROHM BU27008 color sensor MAINTAINERS: Add ROHM BU27008 .../bindings/iio/light/rohm,bu27008.yaml | 49 + MAINTAINERS | 3 +- drivers/iio/accel/kionix-kx022a.c | 13 +- drivers/iio/industrialio-trigger.c | 22 +- drivers/iio/light/Kconfig | 14 + drivers/iio/light/Makefile | 1 + drivers/iio/light/rohm-bu27008.c | 1026 +++++++++++++++++ include/linux/iio/trigger.h | 1 + 8 files changed, 1115 insertions(+), 14 deletions(-) create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bu27008.yaml create mode 100644 drivers/iio/light/rohm-bu27008.c