Support ROHM BU27008 RGB sensor
Subsystems (3)
Matti Vaittinen
maintainerv2 cover Support ROHM BU27008 RGB sensor
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
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 | 964 ++++++++++++++++++ include/linux/iio/trigger.h | 1 + 8 files changed, 1053 insertions(+), 14 deletions(-) create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bu27008.yaml create mode 100644 drivers/iio/light/rohm-bu27008.c
base-commit: 52cc189b4fc6af6accc45fe7b7053d76d8724059
Signature · 14 lines
2.40.0 -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~ Thanks to Simon Glass for the translation =]
Matti Vaittinen
maintainerPATCH v2 1/5 dt-bindings: iio: light: ROHM BU27008
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.
Add BU27008 dt-bindings.
---
Revision history:
v1 => v2:
- fix binding file name
- fix binding id
- drop unnecessary '|' from description
--- .../bindings/iio/light/rohm,bu27008.yaml | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bu27008.yaml
Documentation/devicetree/bindings/iio/light/rohm,bu27008.yaml new file +49 -0
@@ -0,0 +1,49 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/light/rohm,bu27008.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ROHM BU27008 color sensor + +maintainers: + - Matti Vaittinen <[email protected]> + +description: + 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. + +properties: + compatible: + const: rohm,bu27008 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + vdd-supply: true + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + light-sensor@38 { + compatible = "rohm,bu27008"; + reg = <0x38>; + }; + }; + +...
Signature · 14 lines
2.40.0 -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~ Thanks to Simon Glass for the translation =]
Matti Vaittinen
maintainerPATCH v2 2/5 iio:trigger: Add simple trigger_validation helper
In reply to Matti VaittinenSome triggers can only be attached to the IIO device that corresponds to the same physical device. Implement generic helper which can be used as a validate_trigger callback for such devices.
---
Revision history
v2: New patch
--- drivers/iio/industrialio-trigger.c | 22 +++++++++++++++++++++- include/linux/iio/trigger.h | 1 + 2 files changed, 22 insertions(+), 1 deletion(-)
drivers/iio/industrialio-trigger.c +21 -1
@@ -322,7 +322,7 @@ int iio_trigger_attach_poll_func(struct iio_trigger *trig,* this is the case if the IIO device and the trigger device share the * same parent device. */ -if (pf->indio_dev->dev.parent == trig->dev.parent) +if (iio_validate_own_trigger (pf->indio_dev, trig)) trig->attached_own_device = true;return ret;
@@ -728,6 +728,26 @@ bool iio_trigger_using_own(struct iio_dev *indio_dev) }EXPORT_SYMBOL (iio_trigger_using_own); +/** +* iio_validate_own_trigger - Check if a trigger and IIO device belong to +* the same device +* @idev: the IIO device to check +* @trig: The IIO trigger to check +* +* This function can be used as the validate_trigger callback for triggers that +* can only be attached to their own device. +* +* Return: 0 if both the trigger and the IIO device belong to the same +* device, -EINVAL otherwise. +*/ +int iio_validate_own_trigger (struct iio_dev *idev,struct iio_trigger *trig) +{ +if (idev->dev.parent != trig->dev.parent) +return -EINVAL ; +return 0 ; +} +EXPORT_SYMBOL_GPL (iio_validate_own_trigger); +/** * iio_trigger_validate_own_device - Check if a trigger and IIO device belong to * the same device
include/linux/iio/trigger.h +1 -0
@@ -171,6 +171,7 @@ void iio_trigger_free(struct iio_trigger *trig);*/ bool iio_trigger_using_own (struct iio_dev *indio_dev); +int iio_validate_own_trigger (struct iio_dev *idev,struct iio_trigger *trig);int iio_trigger_validate_own_device (struct iio_trigger *trig,struct iio_dev *indio_dev);
Signature · 14 lines
2.40.0 -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~ Thanks to Simon Glass for the translation =]
Matti Vaittinen
maintainerPATCH v2 3/5 iio: kx022a: Use new iio_validate_own_trigger()
In reply to Matti VaittinenThe new generic iio_validate_own_trigger() can be used as validate_trigger callback for verifying the used trigger belongs to same device as the iio_dev.
Use the generic function instead of rolling own one.
---
Revision history
v2: New patch
--- drivers/iio/accel/kionix-kx022a.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-)
drivers/iio/accel/kionix-kx022a.c +1 -12
@@ -516,17 +516,6 @@ static int kx022a_read_raw(struct iio_dev *idev,return -EINVAL ; }; -static int kx022a_validate_trigger (struct iio_dev *idev, -struct iio_trigger *trig) -{ -struct kx022a_data *data =iio_priv (idev); - -if (data->trig != trig) -return -EINVAL ; - -return 0 ; -} -static int kx022a_set_watermark (struct iio_dev *idev,unsigned int val) {struct kx022a_data *data =iio_priv (idev);
@@ -725,7 +714,7 @@ static const struct iio_info kx022a_info = { .write_raw = &kx022a_write_raw, .read_avail = &kx022a_read_avail, - .validate_trigger = kx022a_validate_trigger, + .validate_trigger = iio_validate_own_trigger, .hwfifo_set_watermark = kx022a_set_watermark, .hwfifo_flush_to_buffer = kx022a_fifo_flush, };
Signature · 14 lines
2.40.0 -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~ Thanks to Simon Glass for the translation =]
Matti Vaittinen
maintainerPATCH v2 4/5 iio: light: ROHM BU27008 color sensor
In reply to Matti VaittinenThe 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.
Add initial support for the ROHM BU27008 color sensor.
- raw_read() of RGB and clear channels
- triggered buffer w/ DRDY interrtupt
---
Revision history
v1 => v2:
drivers/iio/light/rohm-bu27008.c
- Fix buffered data demuxing
- Use generic trigger functions instead of rolling own ones
- Drop unnecessary locking
- Use generic iio_validate_own_trigger()
- Some other more trivial fixes for review comments
- use defines for [enable/disable] [measurement/data-ready IRQ] reg values and use regmap_update_bits() directly instead of regamap_[set/clear]_bits()
--- drivers/iio/light/Kconfig | 14 + drivers/iio/light/Makefile | 1 + drivers/iio/light/rohm-bu27008.c | 964 +++++++++++++++++++++++++++++++ 3 files changed, 979 insertions(+) create mode 100644 drivers/iio/light/rohm-bu27008.c
drivers/iio/light/Kconfig +14 -0
@@ -289,6 +289,20 @@ config JSA1212 To compile this driver as a module, choose M here: the module will be called jsa1212. +config ROHM_BU27008 + tristate "ROHM BU27008 color (RGB+C/IR) sensor" + depends on I2C + select REGMAP_I2C + select IIO_GTS_HELPER + help + Enable support for the ROHM BU27008 color 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. + config ROHM_BU27034 tristate "ROHM BU27034 ambient light sensor" depends on I2C
drivers/iio/light/Makefile +1 -0
@@ -39,6 +39,7 @@ obj-$(CONFIG_NOA1305) += noa1305.o obj-$(CONFIG_OPT3001) += opt3001.o obj-$(CONFIG_PA12203001) += pa12203001.o obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o +obj-$(CONFIG_ROHM_BU27008) += rohm-bu27008.o obj-$(CONFIG_RPR0521) += rpr0521.o obj-$(CONFIG_SI1133) += si1133.o obj-$(CONFIG_SI1145) += si1145.o
drivers/iio/light/rohm-bu27008.c new file +964 -0
964 lines not on this page · raw message
Signature · 14 lines
2.40.0 -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~ Thanks to Simon Glass for the translation =]
Matti Vaittinen
maintainerPATCH v2 5/5 MAINTAINERS: Add ROHM BU27008
In reply to Matti VaittinenAdd myself as a maintainer for ROHM BU27008 color sensor driver.
---
Revision history:
No changes since v1
--- MAINTAINERS | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
MAINTAINERS +2 -1
@@ -18099,10 +18099,11 @@ S: Maintained F: Documentation/devicetree/bindings/iio/light/bh1750.yaml F: drivers/iio/light/bh1750.c -ROHM BU27034 AMBIENT LIGHT SENSOR DRIVER +ROHM BU270xx LIGHT SENSOR DRIVERs M: Matti Vaittinen <[email protected]> L: [email protected] S: Supported +F: drivers/iio/light/rohm-bu27008.c F: drivers/iio/light/rohm-bu27034.c ROHM MULTIFUNCTION BD9571MWV-M PMIC DEVICE DRIVERS
Signature · 14 lines
2.40.0 -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~ Thanks to Simon Glass for the translation =]
Re: [PATCH v2 1/5] dt-bindings: iio: light: ROHM BU27008
In reply to Matti VaittinenQuoted 9 lines · Matti Vaittinen
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.
Add BU27008 dt-bindings.
Signed-off-by Matti Vaittinen <[email protected]>
Best regards,
Krzysztof
Re: [PATCH v2 2/5] iio:trigger: Add simple trigger_validation helper
In reply to Matti VaittinenQuoted 3 lines · Matti Vaittinen
Some triggers can only be attached to the IIO device that corresponds to the same physical device. Implement generic helper which can be used as a
validate_triggercallback for such devices.
Missing space in the Subject?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v2 4/5] iio: light: ROHM BU27008 color sensor
In reply to Matti VaittinenQuoted 9 lines · Matti Vaittinen
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.
Add initial support for the ROHM BU27008 color sensor.
-raw_read()of RGB and clear channels
- triggered buffer w/ DRDY interrtupt
...
This...
Quoted 1 line
+#include <
linux/bits.h>
...is guaranteed to be included by this.
Quoted 1 line
+#include <
linux/bitops.h>
...
drivers/iio/light/rohm-bu27008.c line 947
Quoted 2 lines
drivers/iio/light/rohm-bu27008.c new file +2
-0+static const struct of_device_id bu27008_of_match[] = { + { .compatible ="rohm,bu27008" , },
Inner comma is not necessary to have.
Quoted 2 lines
(quoted diff) +2
-0+ { } +};
Signature · 2 lines
With Best Regards, Andy Shevchenko
Matti Vaittinen
maintainerRe: [PATCH v2 2/5] iio:trigger: Add simple trigger_validation helper
In reply to Andy ShevchenkoQuoted 7 lines · Andy Shevchenko
Quoted 3 lines · Matti Vaittinen
Some triggers can only be attached to the IIO device that corresponds to the same physical device. Implement generic helper which can be used as a
validate_triggercallback for such devices.Missing space in the Subject?
After iio:? Yep, thanks!
Yours,
-- Matti
Signature · 5 lines
Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~
Matti Vaittinen
maintainerRe: [PATCH v2 4/5] iio: light: ROHM BU27008 color sensor
In reply to Andy ShevchenkoQuoted 11 lines · Andy Shevchenko
Quoted 9 lines · Matti Vaittinen
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.
Add initial support for the ROHM BU27008 color sensor.
-raw_read()of RGB and clear channels
- triggered buffer w/ DRDY interrtupt
Quoted 9 lines
...
This...
Quoted 1 line
+#include <
linux/bits.h>...is guaranteed to be included by this.
Quoted 1 line
+#include <
linux/bitops.h>
Out of the curiosity - do we have a rule and rationale for explicitly including headers with 'stuff' we use Vs. trusting some header being included by another one? I've not thought much of this so I don't know if there are any pros/cons?
Quoted 6 lines
...
drivers/iio/light/rohm-bu27008.c line 947
Quoted 2 lines
drivers/iio/light/rohm-bu27008.c new file +2
-0+static const struct of_device_id bu27008_of_match[] = { + { .compatible ="rohm,bu27008" , },Inner comma is not necessary to have.
Thanks.
Yours,
-- Matti
Signature · 5 lines
Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~
Re: [PATCH v2 4/5] iio: light: ROHM BU27008 color sensor
Quoted 2 lines · Matti Vaittinen
Quoted 1 line · Andy Shevchenko
On Mon, Apr 24, 2023 at 04:10:09PM +0300, Matti Vaittinen wrote:
...
Quoted 4 lines
Quoted 4 lines
This...
Quoted 1 line
+#include <
linux/bits.h>
Quoted 8 lines
Quoted 3 lines
...is guaranteed to be included by this.
Quoted 1 line
+#include <
linux/bitops.h>Out of the curiosity - do we have a rule and rationale for explicitly including headers with 'stuff' we use Vs. trusting some header being included by another one? I've not thought much of this so I don't know if there are any pros/cons?
That's what we are starving for actually. Currently this is a tribe knowledge which one gets while being involved into Linux kernel development for a long time and being capable of keeping an eye on tree wide, library or similar changes.
I would love to see some (preferably generated) list of the header dependencies. Yet, the header dependency hell should be solved meanwhile (see Ingo's 2k+ patch series).
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v2 4/5] iio: light: ROHM BU27008 color sensor
In reply to Matti VaittinenHi Matti,
[...]
Quoted 1 line
+static int
bu27008_get_int_time(struct bu27008_data *data)
this is providing the time in 'us' if I understood correctly.
Can you add it at the end of the function to make it clear? bu27008_get_int_time_us().
No need to resend it just for this.
Quoted 10 lines
drivers/iio/light/rohm-bu27008.c new file +10
-0+{ +int ret, sel; + + ret =bu27008_get_int_time_sel (data, &sel); +if (ret) +return ret; + +return iio_gts_find_int_time_by_sel (&data->gts, + sel &BU27008_MASK_MEAS_MODE ); +}
[...]
Quoted 10 lines
drivers/iio/light/rohm-bu27008.c new file +10
-0+static int bu27008_try_set_int_time (struct bu27008_data *data,int int_time_new) +{ +int ret, old_time_sel, new_time_sel, old_gain, new_gain; + +mutex_lock (&data->mutex); + + ret =bu27008_get_int_time_sel (data, &old_time_sel); +if (ret <0 ) + goto unlock_out; +
drivers/iio/light/rohm-bu27008.c line 446
Quoted 10 lines
drivers/iio/light/rohm-bu27008.c new file +10
-0+if (!iio_gts_valid_time (&data->gts, int_time_new)) { +dev_dbg (data->dev,"Unsupported integration time %u\n" , + int_time_new); + + ret = -EINVAL ; + goto unlock_out; + } + new_time_sel =iio_gts_find_sel_by_int_time (&data->gts, int_time_new); +if (new_time_sel == old_time_sel) { + ret =0 ;
ret is already '0' here.
Quoted 2 lines
(quoted diff) +2
-0+ goto unlock_out; + }
[...]
Quoted 8 lines
drivers/iio/light/rohm-bu27008.c new file +8
-0+static int bu27008_read_one (struct bu27008_data *data,struct iio_dev *idev, +struct iio_chan_spec const *chan,int *val,int *val2) +{ +int ret, int_time; + + ret =bu27008_chan_cfg (data, chan); +if (ret) +return ret;
drivers/iio/light/rohm-bu27008.c line 540
Quoted 10 lines
drivers/iio/light/rohm-bu27008.c new file +10
-0+ + ret =bu27008_meas_set (data,BU27008_MEAS_EN ); +if (ret) +return ret; + + int_time =bu27008_get_int_time (data); +if (int_time <0 ) + int_time =400000 ; + +msleep ((int_time +500 ) /1000 );
What is this 500 doing? Is it making a real difference? it's 0.5ms.
What's the minimum int_time? Can we set a minimum, as well, just for the sake of the msleep?
Quoted 3 lines
drivers/iio/light/rohm-bu27008.c new file +3
-0+ ret =bu27008_chan_read_data (data, chan->address, val); +if (!ret) + ret =IIO_VAL_INT ;
[...]
Quoted 6 lines
drivers/iio/light/rohm-bu27008.c new file +6
-0+static int bu27008_set_scale (struct bu27008_data *data, +struct iio_chan_spec const *chan, +int val,int val2) +{ +int ret, gain_sel, time_sel, i; +
drivers/iio/light/rohm-bu27008.c line 612
Quoted 10 lines
drivers/iio/light/rohm-bu27008.c new file +10
-0+if (chan->scan_index ==BU27008_IR ) +return -EINVAL ; + +mutex_lock (&data->mutex); + + ret =bu27008_get_int_time_sel (data, &time_sel); +if (ret <0 ) + goto unlock_out; + +
extra line here.
Quoted 16 lines
drivers/iio/light/rohm-bu27008.c new file +16
-0+ ret =iio_gts_find_gain_sel_for_scale_using_time (&data->gts, time_sel, + val, val2 *1000 , &gain_sel); +if (ret) { +/* Could not support new scale with existing int-time */ +int new_time_sel; + +for (i =0 ; i < data->gts.num_itime; i++) { + new_time_sel = data->gts.itime_table[i].sel; + ret =iio_gts_find_gain_sel_for_scale_using_time ( + &data->gts, new_time_sel, val, val2 *1000 , + &gain_sel); +if (!ret) +break ; + } +if (i == data->gts.num_itime) { + dev_err(data->dev, "Can't support scale %u %u\n", val,
drivers/iio/light/rohm-bu27008.c line 638
Quoted 10 lines
drivers/iio/light/rohm-bu27008.c new file +10
-0+ val2); + + ret = -EINVAL ; + goto unlock_out; + } + + ret =bu27008_set_int_time_sel (data, new_time_sel); +if (ret) + goto unlock_out; + }
just a nit: I see you got tight here and goto's are not made only for error handling. You could:
if (!ret) goto something; /* * everything that you have inside the 'if (ret)' with * one level of indentation less */
something:
ret = bu27008_write_gain_sel(data, gain_sel); /* ... */
free to ignore this comment, though, I just saw that there are a few cases where you can save some indentation above.
Quoted 8 lines
drivers/iio/light/rohm-bu27008.c new file +8
-0+ + ret =bu27008_write_gain_sel (data, gain_sel); + +unlock_out: +mutex_unlock (&data->mutex); + +return ret; +}
[...]
Quoted 6 lines
drivers/iio/light/rohm-bu27008.c new file +6
-0+static int bu27008_chip_init (struct bu27008_data *data) +{ +int ret; + + ret =regmap_update_bits (data->regmap,BU27008_REG_SYSTEM_CONTROL , +BU27008_MASK_SW_RESET ,BU27008_MASK_SW_RESET );
drivers/iio/light/rohm-bu27008.c line 723
Quoted 10 lines
drivers/iio/light/rohm-bu27008.c new file +10
-0+if (ret) +return dev_err_probe (data->dev, ret,"Sensor reset failed\n" ); + +/* +* The data-sheet does not tell how long performing the IC reset takes. +* However, the data-sheet says the minimum time it takes the IC to be +* able to take inputs after power is applied, is 100 uS. I'd assume +* > 1 mS is enough. +*/ +msleep (1 );
please use usleep_range().
Quoted 3 lines
drivers/iio/light/rohm-bu27008.c new file +3
-0+ +return ret; +}
[...]
Quoted 1 line
+static
irqreturn_tbu27008_trigger_handler(int irq, void *p)
Do we really need to be in atomic context here? Can this be handled from a thread?
Quoted 31 lines
drivers/iio/light/rohm-bu27008.c new file +31
-0+{ +struct iio_poll_func *pf = p; +struct iio_dev *idev = pf->indio_dev; +struct bu27008_data *data =iio_priv (idev); +struct { +__le16 chan[BU27008_NUM_HW_CHANS ]; +s64 ts__aligned(8) ; + } raw; +int ret, dummy; + +memset (&raw,0 ,sizeof (raw)); + +/* +* After some measurements, it seems reading the +* BU27008_REG_MODE_CONTROL3 debounces the IRQ line +*/ + ret =regmap_read (data->regmap,BU27008_REG_MODE_CONTROL3 , &dummy); +if (ret <0 ) + goto err_read; + + ret =regmap_bulk_read (data->regmap,BU27008_REG_DATA0_LO , &raw.chan, +sizeof (raw.chan)); +if (ret <0 ) + goto err_read; + +iio_push_to_buffers_with_timestamp (idev, &raw, pf->timestamp); +err_read: +iio_trigger_notify_done (idev->trig); + +return IRQ_HANDLED ; +}
[...]
Quoted 13 lines
drivers/iio/light/rohm-bu27008.c new file +13
-0+static int bu27008_probe (struct i2c_client *i2c) +{ +struct device *dev = &i2c->dev; +struct iio_trigger *itrig; +struct bu27008_data *data; +struct regmap *regmap; +unsigned int part_id, reg; +struct iio_dev *idev; +char *name; +int ret; + +if (!i2c->irq) +dev_warn (dev,"No IRQ configured\n" );
[...]
drivers/iio/light/rohm-bu27008.c line 901
Quoted 1 line
drivers/iio/light/rohm-bu27008.c new file +1
-0+if (i2c->irq) {
e.g.:
if (!i2c->irq) {
dev_warn(dev, "No IRQ configured\n");
goto no_irq;
}
/* ... */or, if you don't like the goto used like this...
Quoted 25 lines
drivers/iio/light/rohm-bu27008.c new file +25
-0+ ret =devm_iio_triggered_buffer_setup (dev, idev, + &iio_pollfunc_store_time, + bu27008_trigger_handler, + &bu27008_buffer_ops); +if (ret) +return dev_err_probe (dev, ret, +"iio_triggered_buffer_setup_ext FAIL\n" ); + + itrig =devm_iio_trigger_alloc (dev,"%sdata-rdy-dev%d" , + idev->name,iio_device_id (idev)); +if (!itrig) +return -ENOMEM ; + + data->trig = itrig; + + itrig->ops = &bu27008_trigger_ops; +iio_trigger_set_drvdata (itrig, data); + + name =devm_kasprintf (dev,GFP_KERNEL ,"%s-bu27008" , +dev_name (dev)); + + ret =devm_request_threaded_irq (dev, i2c->irq, + iio_pollfunc_store_time, + &bu27008_irq_thread_handler, +IRQF_ONESHOT , name, idev->pollfunc);
drivers/iio/light/rohm-bu27008.c line 936
Quoted 10 lines
drivers/iio/light/rohm-bu27008.c new file +10
-0+if (ret) +return dev_err_probe (dev, ret, +"Could not request IRQ\n" ); + + + ret =devm_iio_trigger_register (dev, itrig); +if (ret) +return dev_err_probe (dev, ret, +"Trigger registration failed\n" ); + }
} else {
dev_warn(dev, "No IRQ configured\n");
}and save the first 'if' of this function.
drivers/iio/light/rohm-bu27008.c line 944
Quoted 8 lines
drivers/iio/light/rohm-bu27008.c new file +8
-0+ + ret =devm_iio_device_register (dev, idev); +if (ret) +return dev_err_probe (dev, ret, +"Unable to register iio device\n" ); + +return ret; +}
[...]
Andi
Matti Vaittinen
maintainerRe: [PATCH v2 4/5] iio: light: ROHM BU27008 color sensor
Hi Andi,
Thanks for the review! It's nice to see you're still keeping an eye on ROHM / Kionix senor drivers ;)
Quoted 4 lines · Andi Shyti
Hi Matti,
[...]
Quoted 8 lines
Quoted 1 line
+static int
bu27008_get_int_time(struct bu27008_data *data)this is providing the time in 'us' if I understood correctly.
Can you add it at the end of the function to make it clear?
bu27008_get_int_time_us().No need to resend it just for this.
I like the idea.
Quoted 28 lines
Quoted 11 lines
drivers/iio/light/rohm-bu27008.c new file +10
-0+{ +int ret, sel; + + ret =bu27008_get_int_time_sel (data, &sel); +if (ret) +return ret; + +return iio_gts_find_int_time_by_sel (&data->gts, + sel &BU27008_MASK_MEAS_MODE ); +}[...]
Quoted 14 lines
drivers/iio/light/rohm-bu27008.c new file +14
-0+static int bu27008_try_set_int_time (struct bu27008_data *data,int int_time_new) +{ +int ret, old_time_sel, new_time_sel, old_gain, new_gain; + +mutex_lock (&data->mutex); + + ret =bu27008_get_int_time_sel (data, &old_time_sel); +if (ret <0 ) + goto unlock_out; + +if (!iio_gts_valid_time (&data->gts, int_time_new)) { +dev_dbg (data->dev,"Unsupported integration time %u\n" , + int_time_new); +
Quoted 8 lines
drivers/iio/light/rohm-bu27008.c line 446
Quoted 6 lines
drivers/iio/light/rohm-bu27008.c new file +6
-0+ ret = -EINVAL ; + goto unlock_out; + } + new_time_sel =iio_gts_find_sel_by_int_time (&data->gts, int_time_new); +if (new_time_sel == old_time_sel) { + ret =0 ;ret is already '0' here.
Good point! Kills the ugly brackets just nice :)
Quoted 19 lines
Quoted 3 lines
(quoted diff) +2
-0+ goto unlock_out; + }[...]
Quoted 13 lines
drivers/iio/light/rohm-bu27008.c new file +13
-0+static int bu27008_read_one (struct bu27008_data *data,struct iio_dev *idev, +struct iio_chan_spec const *chan,int *val,int *val2) +{ +int ret, int_time; + + ret =bu27008_chan_cfg (data, chan); +if (ret) +return ret; + + ret =bu27008_meas_set (data,BU27008_MEAS_EN ); +if (ret) +return ret; +
Quoted 8 lines
drivers/iio/light/rohm-bu27008.c line 540
Quoted 5 lines
drivers/iio/light/rohm-bu27008.c new file +5
-0+ int_time =bu27008_get_int_time (data); +if (int_time <0 ) + int_time =400000 ; + +msleep ((int_time +500 ) /1000 );What is this 500 doing? Is it making a real difference? it's 0.5ms.
Thanks for the question, having extra pairs of eyes helps spotting brainfarts :)
The 500 here is half of the value of the divider - idea was to do rounding correctly upwards to prevent premature wake-up. Well, this is incorrect because we should always round up the sleep time, not just 'mathematically correctly' (Eg, not only upwards when value >= 0.5 but upwards always when the division is not even).
After this being said, integration times for this device are full milli seconds so they can all be divided by 1000 uS.
Nevertheless, it's good to note that the sensor is definitely not being clocked by the same clock as CPU and I assume the timing for it will be drifting quite a bit from the CPU clock. This means some sensors will for sure complete the measurement later than this wake-up. In order to tackle this we have the valid-bit polling in bu27008_chan_read_data(). So, at the end of the day, this rounding correction is lkely to be just some unnecessary noise.
Quoted 2 lines
What's the minimum
int_time? Can we set a minimum, as well, just for the sake of the msleep?
Can you please elaborate what you mean by this? The minimum integration time for bu27008 is 55 mS and this is set in the time tables for the gts-helpers. The bu27008_get_int_time() should never return valid time smaller than that.
Quoted 68 lines
Quoted 3 lines
drivers/iio/light/rohm-bu27008.c new file +3
-0+ ret =bu27008_chan_read_data (data, chan->address, val); +if (!ret) + ret =IIO_VAL_INT ;[...]
Quoted 6 lines
drivers/iio/light/rohm-bu27008.c new file +6
-0+static int bu27008_set_scale (struct bu27008_data *data, +struct iio_chan_spec const *chan, +int val,int val2) +{ +int ret, gain_sel, time_sel, i; +drivers/iio/light/rohm-bu27008.c line 612
Quoted 10 lines
drivers/iio/light/rohm-bu27008.c new file +10
-0+if (chan->scan_index ==BU27008_IR ) +return -EINVAL ; + +mutex_lock (&data->mutex); + + ret =bu27008_get_int_time_sel (data, &time_sel); +if (ret <0 ) + goto unlock_out; + +extra line here.
Quoted 16 lines
drivers/iio/light/rohm-bu27008.c new file +16
-0+ ret =iio_gts_find_gain_sel_for_scale_using_time (&data->gts, time_sel, + val, val2 *1000 , &gain_sel); +if (ret) { +/* Could not support new scale with existing int-time */ +int new_time_sel; + +for (i =0 ; i < data->gts.num_itime; i++) { + new_time_sel = data->gts.itime_table[i].sel; + ret =iio_gts_find_gain_sel_for_scale_using_time ( + &data->gts, new_time_sel, val, val2 *1000 , + &gain_sel); +if (!ret) +break ; + } +if (i == data->gts.num_itime) { + dev_err(data->dev, "Can't support scale %u %u\n", val,drivers/iio/light/rohm-bu27008.c line 638
Quoted 10 lines
drivers/iio/light/rohm-bu27008.c new file +10
-0+ val2); + + ret = -EINVAL ; + goto unlock_out; + } + + ret =bu27008_set_int_time_sel (data, new_time_sel); +if (ret) + goto unlock_out; + }just a nit: I see you got tight here and goto's are not made only for error handling. You could:
if (!ret) goto something; /* * everything that you have inside the 'if (ret)' with * one level of indentation less */ something: ret = bu27008_write_gain_sel(data, gain_sel); /* ... */
Quoted 3 lines
free to ignore this comment, though, I just saw that there are a few cases where you can save some indentation above.
Thanks for the suggestion. I am not really sure which way I like better. I think the logic is tiny bit easier to follow when the success case is not handled with goto. OTOH, with a proper descriptive goto label (definitely not 'something' ;) ) this "tiny bit" gets, well, tiny.
Yet, the indentation is not _that_ deep here - logic does not (in my opinion) get messy because of nesting but iio_gts_find_gain_sel_for_scale_using_time - is a long function name and the call looks kind of ugly.
Thanks for the suggestion - I'll see how this looks like and decide when I open the file next time :)
Quoted 21 lines
Quoted 8 lines
drivers/iio/light/rohm-bu27008.c new file +8
-0+ + ret =bu27008_write_gain_sel (data, gain_sel); + +unlock_out: +mutex_unlock (&data->mutex); + +return ret; +}[...]
Quoted 10 lines
drivers/iio/light/rohm-bu27008.c new file +10
-0+static int bu27008_chip_init (struct bu27008_data *data) +{ +int ret; + + ret =regmap_update_bits (data->regmap,BU27008_REG_SYSTEM_CONTROL , +BU27008_MASK_SW_RESET ,BU27008_MASK_SW_RESET ); +if (ret) +return dev_err_probe (data->dev, ret,"Sensor reset failed\n" ); + +/*
Quoted 8 lines
drivers/iio/light/rohm-bu27008.c line 723
Quoted 6 lines
drivers/iio/light/rohm-bu27008.c new file +6
-0+* The data-sheet does not tell how long performing the IC reset takes. +* However, the data-sheet says the minimum time it takes the IC to be +* able to take inputs after power is applied, is 100 uS. I'd assume +* > 1 mS is enough. +*/ +msleep (1 );please use
usleep_range().
I prefer to not require setting up hrtimers as we have no real requirements for the duration of this sleep. I know the msleep() is likely to exceed the 1 mS, potentially a lot if there is things to do - but we don't really care at this point. The main thing is to give the HW time to reset while allowing other things to be scheduled.
Quoted 11 lines
Quoted 4 lines
drivers/iio/light/rohm-bu27008.c new file +3
-0+ +return ret; +}[...]
Quoted 1 line
+static
irqreturn_tbu27008_trigger_handler(int irq, void *p)Do we really need to be in atomic context here? Can this be handled from a thread?
As far as I understand, this is handled from a process context.
Quoted 105 lines
Quoted 32 lines
drivers/iio/light/rohm-bu27008.c new file +31
-0+{ +struct iio_poll_func *pf = p; +struct iio_dev *idev = pf->indio_dev; +struct bu27008_data *data =iio_priv (idev); +struct { +__le16 chan[BU27008_NUM_HW_CHANS ]; +s64 ts__aligned(8) ; + } raw; +int ret, dummy; + +memset (&raw,0 ,sizeof (raw)); + +/* +* After some measurements, it seems reading the +* BU27008_REG_MODE_CONTROL3 debounces the IRQ line +*/ + ret =regmap_read (data->regmap,BU27008_REG_MODE_CONTROL3 , &dummy); +if (ret <0 ) + goto err_read; + + ret =regmap_bulk_read (data->regmap,BU27008_REG_DATA0_LO , &raw.chan, +sizeof (raw.chan)); +if (ret <0 ) + goto err_read; + +iio_push_to_buffers_with_timestamp (idev, &raw, pf->timestamp); +err_read: +iio_trigger_notify_done (idev->trig); + +return IRQ_HANDLED ; +}[...]
Quoted 13 lines
drivers/iio/light/rohm-bu27008.c new file +13
-0+static int bu27008_probe (struct i2c_client *i2c) +{ +struct device *dev = &i2c->dev; +struct iio_trigger *itrig; +struct bu27008_data *data; +struct regmap *regmap; +unsigned int part_id, reg; +struct iio_dev *idev; +char *name; +int ret; + +if (!i2c->irq) +dev_warn (dev,"No IRQ configured\n" );[...]
drivers/iio/light/rohm-bu27008.c line 901
Quoted 1 line
drivers/iio/light/rohm-bu27008.c new file +1
-0+if (i2c->irq) {e.g.:
if (!i2c->irq) { dev_warn(dev, "No IRQ configured\n"); goto no_irq; } /* ... */or, if you don't like the goto used like this...
Quoted 25 lines
drivers/iio/light/rohm-bu27008.c new file +25
-0+ ret =devm_iio_triggered_buffer_setup (dev, idev, + &iio_pollfunc_store_time, + bu27008_trigger_handler, + &bu27008_buffer_ops); +if (ret) +return dev_err_probe (dev, ret, +"iio_triggered_buffer_setup_ext FAIL\n" ); + + itrig =devm_iio_trigger_alloc (dev,"%sdata-rdy-dev%d" , + idev->name,iio_device_id (idev)); +if (!itrig) +return -ENOMEM ; + + data->trig = itrig; + + itrig->ops = &bu27008_trigger_ops; +iio_trigger_set_drvdata (itrig, data); + + name =devm_kasprintf (dev,GFP_KERNEL ,"%s-bu27008" , +dev_name (dev)); + + ret =devm_request_threaded_irq (dev, i2c->irq, + iio_pollfunc_store_time, + &bu27008_irq_thread_handler, +IRQF_ONESHOT , name, idev->pollfunc);drivers/iio/light/rohm-bu27008.c line 936
Quoted 10 lines
drivers/iio/light/rohm-bu27008.c new file +10
-0+if (ret) +return dev_err_probe (dev, ret, +"Could not request IRQ\n" ); + + + ret =devm_iio_trigger_register (dev, itrig); +if (ret) +return dev_err_probe (dev, ret, +"Trigger registration failed\n" ); + }} else { dev_warn(dev, "No IRQ configured\n"); }
Quoted 1 line
and save the first 'if' of this function.
Thanks. I like this suggestion. I should've done it like this in the first place.
Quoted 9 lines
drivers/iio/light/rohm-bu27008.c line 944
Quoted 9 lines
drivers/iio/light/rohm-bu27008.c new file +8
-0+ + ret =devm_iio_device_register (dev, idev); +if (ret) +return dev_err_probe (dev, ret, +"Unable to register iio device\n" ); + +return ret; +}
Yours,
-- Matti
Signature · 5 lines
Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~
Re: [PATCH v2 4/5] iio: light: ROHM BU27008 color sensor
Hi Matti,
Quoted 2 lines
Thanks for the review! It's nice to see you're still keeping an eye on ROHM / Kionix senor drivers ;)
yeah... this is fun... if I just had a bit more time :)
Quoted 34 lines
Quoted 13 lines
Quoted 13 lines
drivers/iio/light/rohm-bu27008.c new file +13
-0+static int bu27008_read_one (struct bu27008_data *data,struct iio_dev *idev, +struct iio_chan_spec const *chan,int *val,int *val2) +{ +int ret, int_time; + + ret =bu27008_chan_cfg (data, chan); +if (ret) +return ret; + + ret =bu27008_meas_set (data,BU27008_MEAS_EN ); +if (ret) +return ret; +Quoted 8 lines
drivers/iio/light/rohm-bu27008.c line 540
Quoted 5 lines
drivers/iio/light/rohm-bu27008.c new file +5
-0+ int_time =bu27008_get_int_time (data); +if (int_time <0 ) + int_time =400000 ; + +msleep ((int_time +500 ) /1000 );What is this 500 doing? Is it making a real difference? it's 0.5ms.
Thanks for the question, having extra pairs of eyes helps spotting brainfarts :)
The 500 here is half of the value of the divider - idea was to do rounding correctly upwards to prevent premature wake-up. Well, this is incorrect because we should always round up the sleep time, not just 'mathematically correctly' (Eg, not only upwards when value >= 0.5 but upwards always when the division is not even).
After this being said, integration times for this device are full milli seconds so they can all be divided by 1000 uS.
Quoted 7 lines
Nevertheless, it's good to note that the sensor is definitely not being clocked by the same clock as CPU and I assume the timing for it will be drifting quite a bit from the CPU clock. This means some sensors will for sure complete the measurement later than this wake-up. In order to tackle this we have the valid-bit polling in
bu27008_chan_read_data(). So, at the end of the day, this rounding correction is lkely to be just some unnecessary noise.
I understand the logic of the waiting, but msleep is not the right function as waiting with msleep is always very approximate, that's why it's recommended to use it for a large waiting period, where the error is smaller.
If int_time is 1ms, waiting 1.5 or 2 or 1, is the same thing, most probably you will end up waiting more.
Quoted 6 lines
Quoted 2 lines
What's the minimum
int_time? Can we set a minimum, as well, just for the sake of the msleep?Can you please elaborate what you mean by this? The minimum integration time for bu27008 is 55 mS and this is set in the time tables for the gts-helpers. The
bu27008_get_int_time()should never return valid time smaller than that.
Witha minimum i mean a minimum value for the msleep to start working decently. E.g. what if int_time is lower than 1ms? Can we have msleep(0)?
[...]
Quoted 19 lines
Quoted 10 lines
Quoted 10 lines
drivers/iio/light/rohm-bu27008.c new file +10
-0+static int bu27008_chip_init (struct bu27008_data *data) +{ +int ret; + + ret =regmap_update_bits (data->regmap,BU27008_REG_SYSTEM_CONTROL , +BU27008_MASK_SW_RESET ,BU27008_MASK_SW_RESET ); +if (ret) +return dev_err_probe (data->dev, ret,"Sensor reset failed\n" ); + +/* Quoted 8 lines
drivers/iio/light/rohm-bu27008.c line 723
Quoted 6 lines
drivers/iio/light/rohm-bu27008.c new file +6
-0+* The data-sheet does not tell how long performing the IC reset takes. +* However, the data-sheet says the minimum time it takes the IC to be +* able to take inputs after power is applied, is 100 uS. I'd assume +* > 1 mS is enough. +*/ +msleep (1 );please use
usleep_range().
Quoted 5 lines
I prefer to not require setting up hrtimers as we have no real requirements for the duration of this sleep. I know the
msleep()is likely to exceed the 1 mS, potentially a lot if there is things to do - but we don't really care at this point. The main thing is to give the HW time to reset while allowing other things to be scheduled.
For the reason above, msleep(1) is quite a meaningless instruction. If you need to wait around 1ms, then usleep_range is the function to be used.
Refer, also, to the Documentation/timers/timers-howto.rst
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/iio/light/rohm-bu27008.c new file +3
-0+ +return ret; +}
[...]
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
+static
irqreturn_tbu27008_trigger_handler(int irq, void *p)Do we really need to be in atomic context here? Can this be handled from a thread?
As far as I understand, this is handled from a process context.
Sorry... I misread it... I thought you used request_irq() for this and request_threaded_irq() for bu27008_irq_thread_handler().
Ignore :)
Andi
Matti Vaittinen
maintainerRe: [PATCH v2 4/5] iio: light: ROHM BU27008 color sensor
Quoted 49 lines · Andi Shyti
Hi Matti,
Quoted 2 lines
Thanks for the review! It's nice to see you're still keeping an eye on ROHM / Kionix senor drivers ;)
yeah... this is fun... if I just had a bit more time :)
Quoted 34 lines
Quoted 13 lines
Quoted 13 lines
drivers/iio/light/rohm-bu27008.c new file +13
-0+static int bu27008_read_one (struct bu27008_data *data,struct iio_dev *idev, +struct iio_chan_spec const *chan,int *val,int *val2) +{ +int ret, int_time; + + ret =bu27008_chan_cfg (data, chan); +if (ret) +return ret; + + ret =bu27008_meas_set (data,BU27008_MEAS_EN ); +if (ret) +return ret; +Quoted 8 lines
drivers/iio/light/rohm-bu27008.c line 540
Quoted 5 lines
drivers/iio/light/rohm-bu27008.c new file +5
-0+ int_time =bu27008_get_int_time (data); +if (int_time <0 ) + int_time =400000 ; + +msleep ((int_time +500 ) /1000 );What is this 500 doing? Is it making a real difference? it's 0.5ms.
Thanks for the question, having extra pairs of eyes helps spotting brainfarts :)
The 500 here is half of the value of the divider - idea was to do rounding correctly upwards to prevent premature wake-up. Well, this is incorrect because we should always round up the sleep time, not just 'mathematically correctly' (Eg, not only upwards when value >= 0.5 but upwards always when the division is not even).
After this being said, integration times for this device are full milli seconds so they can all be divided by 1000 uS.
Quoted 7 lines
Nevertheless, it's good to note that the sensor is definitely not being clocked by the same clock as CPU and I assume the timing for it will be drifting quite a bit from the CPU clock. This means some sensors will for sure complete the measurement later than this wake-up. In order to tackle this we have the valid-bit polling in
bu27008_chan_read_data(). So, at the end of the day, this rounding correction is lkely to be just some unnecessary noise.
Quoted 7 lines
I understand the logic of the waiting, but msleep is not the right function as waiting with msleep is always very approximate, that's why it's recommended to use it for a large waiting period, where the error is smaller.
If
int_timeis 1ms, waiting 1.5 or 2 or 1, is the same thing, most probably you will end up waiting more.
Well, the light sensors are slow. The shortest time with BU27008 is 55 mS, longest being 400 mS. I think msleep fits Ok for this purpose.
Quoted 10 lines
Quoted 6 lines
Quoted 2 lines
What's the minimum
int_time? Can we set a minimum, as well, just for the sake of the msleep?Can you please elaborate what you mean by this? The minimum integration time for bu27008 is 55 mS and this is set in the time tables for the gts-helpers. The
bu27008_get_int_time()should never return valid time smaller than that.Witha minimum i mean a minimum value for the msleep to start working decently. E.g. what if
int_timeis lower than 1ms? Can we have msleep(0)?
As mentioned, int_time for bu27008 will be in a range of 55 ... 400 mS.
Quoted 28 lines
[...]
Quoted 19 lines
Quoted 10 lines
Quoted 10 lines
drivers/iio/light/rohm-bu27008.c new file +10
-0+static int bu27008_chip_init (struct bu27008_data *data) +{ +int ret; + + ret =regmap_update_bits (data->regmap,BU27008_REG_SYSTEM_CONTROL , +BU27008_MASK_SW_RESET ,BU27008_MASK_SW_RESET ); +if (ret) +return dev_err_probe (data->dev, ret,"Sensor reset failed\n" ); + +/* Quoted 8 lines
drivers/iio/light/rohm-bu27008.c line 723
Quoted 6 lines
drivers/iio/light/rohm-bu27008.c new file +6
-0+* The data-sheet does not tell how long performing the IC reset takes. +* However, the data-sheet says the minimum time it takes the IC to be +* able to take inputs after power is applied, is 100 uS. I'd assume +* > 1 mS is enough. +*/ +msleep (1 );please use
usleep_range().Quoted 5 lines
I prefer to not require setting up hrtimers as we have no real requirements for the duration of this sleep. I know the
msleep()is likely to exceed the 1 mS, potentially a lot if there is things to do - but we don't really care at this point. The main thing is to give the HW time to reset while allowing other things to be scheduled.
Quoted 5 lines
For the reason above, msleep(1) is quite a meaningless instruction. If you need to wait around 1ms, then
usleep_rangeis the function to be used.Refer, also, to the
Documentation/timers/timers-howto.rst
I know the msleep() may sleep much longer. It still is not meaningless. Here we use the msleep() in a purpose:
"Sleep at least 1 mS, but actually I don't care if that is 20 mS or more - as long as you allow scheduling other things with as little overhead as possible".
For that purpose msleep() works just perfectly :)
I actually had a comment clarifying this in previous IIO driver I wrote (just to avoid confusing reviewers) but Jonathan asked me to remove the comment ;)
Yours,
-- Matti
Signature · 5 lines
Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~
Re: [PATCH v2 4/5] iio: light: ROHM BU27008 color sensor
Hi Matti,
Quoted 38 lines
Quoted 25 lines
Quoted 19 lines
Quoted 10 lines
Quoted 10 lines
drivers/iio/light/rohm-bu27008.c new file +10
-0+static int bu27008_chip_init (struct bu27008_data *data) +{ +int ret; + + ret =regmap_update_bits (data->regmap,BU27008_REG_SYSTEM_CONTROL , +BU27008_MASK_SW_RESET ,BU27008_MASK_SW_RESET ); +if (ret) +return dev_err_probe (data->dev, ret,"Sensor reset failed\n" ); + +/* Quoted 8 lines
drivers/iio/light/rohm-bu27008.c line 723
Quoted 6 lines
drivers/iio/light/rohm-bu27008.c new file +6
-0+* The data-sheet does not tell how long performing the IC reset takes. +* However, the data-sheet says the minimum time it takes the IC to be +* able to take inputs after power is applied, is 100 uS. I'd assume +* > 1 mS is enough. +*/ +msleep (1 );please use
usleep_range().Quoted 5 lines
I prefer to not require setting up hrtimers as we have no real requirements for the duration of this sleep. I know the
msleep()is likely to exceed the 1 mS, potentially a lot if there is things to do - but we don't really care at this point. The main thing is to give the HW time to reset while allowing other things to be scheduled.Quoted 5 lines
For the reason above, msleep(1) is quite a meaningless instruction. If you need to wait around 1ms, then
usleep_rangeis the function to be used.Refer, also, to the
Documentation/timers/timers-howto.rstI know the
msleep()may sleep much longer. It still is not meaningless. Here we use themsleep()in a purpose:"Sleep at least 1 mS, but actually I don't care if that is 20 mS or more - as long as you allow scheduling other things with as little overhead as possible".
Quoted 5 lines
For that purpose
msleep()works just perfectly :)I actually had a comment clarifying this in previous IIO driver I wrote (just to avoid confusing reviewers) but Jonathan asked me to remove the comment ;)
OK... ok... you convinced me :)
Thanks, will check soon your v3.
Andi