* [PATCH v4] iio: light: new driver for the ROHM BH1780
@ 2016-04-06 13:03 Linus Walleij
2016-04-06 13:32 ` Ulf Hansson
2016-04-10 13:42 ` Jonathan Cameron
0 siblings, 2 replies; 6+ messages in thread
From: Linus Walleij @ 2016-04-06 13:03 UTC (permalink / raw)
To: Jonathan Cameron, linux-iio
Cc: Linus Walleij, Arnd Bergmann, Ulf Hansson, Daniel Mack,
Peter Meerwald-Stadler
This is a reimplementation of the old misc device driver for the
ROHM BH1780 ambient light sensor (drivers/misc/bh1780gli.c).
Differences from the old driver:
- Uses the IIO framework
- Uses runtime PM to idle the hardware after 5 seconds
- No weird custom power management from userspace
- No homebrewn values in sysfs
This uses the same (undocumented) device tree compatible-string
as the old driver ("rohm,bh1780gli").
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Daniel Mack <daniel@caiaq.de>
Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v3->v4:
- Set the system suspend PM ops to pm_force_[suspend|resume]
so we are in low power also in system suspend.
- Error prints for failed I2C transactions in runtime
suspend/resume
ChangeLog v2->v3:
- Dropped the _scale information just returning "1" from
sysfs, userspace should assume scale by 1 if not present.
- Dropped CC to maintainers who seemingly fell off the planet.
ChangeLog v1->v2:
- Use <linux/bitops.h>, BIT(), GENMASK()
- Do not read state first when suspending.
- Do not cast debug reg access return value.
- Whitespace, comments.
I want to phase out the old misc driver, but don't know how that
should properly be handled. The driver has an old (totally
undocumented) sysfs ABI for raw measurements, should I add
this as backward-compatibility and symlink the device to the old
location?
---
drivers/iio/light/Kconfig | 11 ++
drivers/iio/light/Makefile | 1 +
drivers/iio/light/bh1780.c | 283 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 295 insertions(+)
create mode 100644 drivers/iio/light/bh1780.c
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index cfd3df8416bb..5ee1d453b3d8 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -73,6 +73,17 @@ config BH1750
To compile this driver as a module, choose M here: the module will
be called bh1750.
+config BH1780
+ tristate "ROHM BH1780 ambient light sensor"
+ depends on I2C
+ depends on !SENSORS_BH1780
+ help
+ Say Y here to build support for the ROHM BH1780GLI ambient
+ light sensor.
+
+ To compile this driver as a module, choose M here: the module will
+ be called bh1780.
+
config CM32181
depends on I2C
tristate "CM32181 driver"
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index b2c31053db0c..4aeee2bd8f49 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_AL3320A) += al3320a.o
obj-$(CONFIG_APDS9300) += apds9300.o
obj-$(CONFIG_APDS9960) += apds9960.o
obj-$(CONFIG_BH1750) += bh1750.o
+obj-$(CONFIG_BH1780) += bh1780.o
obj-$(CONFIG_CM32181) += cm32181.o
obj-$(CONFIG_CM3232) += cm3232.o
obj-$(CONFIG_CM3323) += cm3323.o
diff --git a/drivers/iio/light/bh1780.c b/drivers/iio/light/bh1780.c
new file mode 100644
index 000000000000..3947aeecd058
--- /dev/null
+++ b/drivers/iio/light/bh1780.c
@@ -0,0 +1,283 @@
+/*
+ * ROHM 1780GLI Ambient Light Sensor Driver
+ *
+ * Copyright (C) 2016 Linaro Ltd.
+ * Author: Linus Walleij <linus.walleij@linaro.org>
+ * Loosely based on the previous BH1780 ALS misc driver
+ * Copyright (C) 2010 Texas Instruments
+ * Author: Hemanth V <hemanthv@ti.com>
+ */
+#include <linux/i2c.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pm_runtime.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/bitops.h>
+
+#define BH1780_CMD_BIT BIT(7)
+#define BH1780_REG_CONTROL 0x00
+#define BH1780_REG_PARTID 0x0A
+#define BH1780_REG_MANFID 0x0B
+#define BH1780_REG_DLOW 0x0C
+#define BH1780_REG_DHIGH 0x0D
+
+#define BH1780_REVMASK GENMASK(3,0)
+#define BH1780_POWMASK GENMASK(1,0)
+#define BH1780_POFF (0x0)
+#define BH1780_PON (0x3)
+
+/* power on settling time in ms */
+#define BH1780_PON_DELAY 2
+/* max time before value available in ms */
+#define BH1780_INTERVAL 250
+
+struct bh1780_data {
+ struct i2c_client *client;
+};
+
+static int bh1780_write(struct bh1780_data *bh1780, u8 reg, u8 val)
+{
+ int ret = i2c_smbus_write_byte_data(bh1780->client,
+ BH1780_CMD_BIT | reg,
+ val);
+ if (ret < 0)
+ dev_err(&bh1780->client->dev,
+ "i2c_smbus_write_byte_data failed error "
+ "%d, register %01x\n",
+ ret, reg);
+ return ret;
+}
+
+static int bh1780_read(struct bh1780_data *bh1780, u8 reg)
+{
+ int ret = i2c_smbus_read_byte_data(bh1780->client,
+ BH1780_CMD_BIT | reg);
+ if (ret < 0)
+ dev_err(&bh1780->client->dev,
+ "i2c_smbus_read_byte_data failed error "
+ "%d, register %01x\n",
+ ret, reg);
+ return ret;
+}
+
+int bh1780_debugfs_reg_access(struct iio_dev *indio_dev,
+ unsigned int reg, unsigned int writeval,
+ unsigned int *readval)
+{
+ struct bh1780_data *bh1780 = iio_priv(indio_dev);
+ int ret;
+
+ if (!readval)
+ bh1780_write(bh1780, (u8)reg, (u8)writeval);
+
+ ret = bh1780_read(bh1780, (u8)reg);
+ if (ret < 0)
+ return ret;
+
+ *readval = ret;
+
+ return 0;
+}
+
+static int bh1780_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct bh1780_data *bh1780 = iio_priv(indio_dev);
+ int lsb, msb;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ switch (chan->type) {
+ case IIO_LIGHT:
+ pm_runtime_get_sync(&bh1780->client->dev);
+ lsb = bh1780_read(bh1780, BH1780_REG_DLOW);
+ if (lsb < 0)
+ return lsb;
+ msb = bh1780_read(bh1780, BH1780_REG_DHIGH);
+ if (msb < 0)
+ return msb;
+ pm_runtime_mark_last_busy(&bh1780->client->dev);
+ pm_runtime_put_autosuspend(&bh1780->client->dev);
+ *val = (msb << 8 | lsb);
+
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+ case IIO_CHAN_INFO_INT_TIME:
+ *val = 0;
+ *val2 = BH1780_INTERVAL * 1000;
+ return IIO_VAL_INT_PLUS_MICRO;
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_info bh1780_info = {
+ .driver_module = THIS_MODULE,
+ .read_raw = bh1780_read_raw,
+ .debugfs_reg_access = bh1780_debugfs_reg_access,
+};
+
+static const struct iio_chan_spec bh1780_channels[] = {
+ {
+ .type = IIO_LIGHT,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_INT_TIME)
+ }
+};
+
+static int bh1780_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ int ret;
+ struct bh1780_data *bh1780;
+ struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
+ struct iio_dev *indio_dev;
+
+ if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
+ return -EIO;
+
+ indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*bh1780));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ bh1780 = iio_priv(indio_dev);
+ bh1780->client = client;
+ i2c_set_clientdata(client, bh1780);
+
+ /* Power up the device */
+ ret = bh1780_write(bh1780, BH1780_REG_CONTROL, BH1780_PON);
+ if (ret < 0)
+ return ret;
+ msleep(BH1780_PON_DELAY);
+ pm_runtime_get_noresume(&client->dev);
+ pm_runtime_set_active(&client->dev);
+ pm_runtime_enable(&client->dev);
+
+ ret = bh1780_read(bh1780, BH1780_REG_PARTID);
+ if (ret < 0)
+ goto out_disable_pm;
+ dev_info(&client->dev,
+ "Ambient Light Sensor, Rev : %lu\n",
+ (ret & BH1780_REVMASK));
+
+ /*
+ * As the device takes 250 ms to even come up with a fresh
+ * measurement after power-on, do not shut it down unnecessarily.
+ * Set autosuspend to a five seconds.
+ */
+ pm_runtime_set_autosuspend_delay(&client->dev, 5000);
+ pm_runtime_use_autosuspend(&client->dev);
+ pm_runtime_put(&client->dev);
+
+ indio_dev->dev.parent = &client->dev;
+ indio_dev->info = &bh1780_info;
+ indio_dev->name = id->name;
+ indio_dev->channels = bh1780_channels;
+ indio_dev->num_channels = ARRAY_SIZE(bh1780_channels);
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ ret = iio_device_register(indio_dev);
+ if (ret)
+ goto out_disable_pm;
+ return 0;
+
+out_disable_pm:
+ pm_runtime_put_noidle(&client->dev);
+ pm_runtime_disable(&client->dev);
+ return ret;
+}
+
+static int bh1780_remove(struct i2c_client *client)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(client);
+
+ iio_device_unregister(indio_dev);
+ pm_runtime_get_sync(&client->dev);
+ pm_runtime_put_noidle(&client->dev);
+ pm_runtime_disable(&client->dev);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+static int bh1780_runtime_suspend(struct device *dev)
+{
+ struct bh1780_data *bh1780;
+ int ret;
+ struct i2c_client *client = to_i2c_client(dev);
+
+ bh1780 = i2c_get_clientdata(client);
+ ret = bh1780_write(bh1780, BH1780_REG_CONTROL, BH1780_POFF);
+ if (ret < 0) {
+ dev_err(dev, "failed to runtime suspend\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static int bh1780_runtime_resume(struct device *dev)
+{
+ struct bh1780_data *bh1780;
+ int ret;
+ struct i2c_client *client = to_i2c_client(dev);
+
+ bh1780 = i2c_get_clientdata(client);
+ ret = bh1780_write(bh1780, BH1780_REG_CONTROL, BH1780_PON);
+ if (ret < 0) {
+ dev_err(dev, "failed to runtime resume\n");
+ return ret;
+ }
+
+ /* Wait for power on, then for a value to be available */
+ msleep(BH1780_PON_DELAY + BH1780_INTERVAL);
+
+ return 0;
+}
+#endif /* CONFIG_PM */
+
+static const struct dev_pm_ops bh1780_dev_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
+ SET_RUNTIME_PM_OPS(bh1780_runtime_suspend,
+ bh1780_runtime_resume, NULL)
+};
+
+static const struct i2c_device_id bh1780_id[] = {
+ { "bh1780", 0 },
+ { },
+};
+
+MODULE_DEVICE_TABLE(i2c, bh1780_id);
+
+#ifdef CONFIG_OF
+static const struct of_device_id of_bh1780_match[] = {
+ { .compatible = "rohm,bh1780gli", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, of_bh1780_match);
+#endif
+
+static struct i2c_driver bh1780_driver = {
+ .probe = bh1780_probe,
+ .remove = bh1780_remove,
+ .id_table = bh1780_id,
+ .driver = {
+ .name = "bh1780",
+ .pm = &bh1780_dev_pm_ops,
+ .of_match_table = of_match_ptr(of_bh1780_match),
+ },
+};
+
+module_i2c_driver(bh1780_driver);
+
+MODULE_DESCRIPTION("ROHM BH1780GLI Ambient Light Sensor Driver");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
--
2.4.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4] iio: light: new driver for the ROHM BH1780
2016-04-06 13:03 [PATCH v4] iio: light: new driver for the ROHM BH1780 Linus Walleij
@ 2016-04-06 13:32 ` Ulf Hansson
2016-04-07 10:18 ` Linus Walleij
2016-04-10 13:42 ` Jonathan Cameron
1 sibling, 1 reply; 6+ messages in thread
From: Ulf Hansson @ 2016-04-06 13:32 UTC (permalink / raw)
To: Linus Walleij
Cc: Jonathan Cameron, linux-iio, Arnd Bergmann, Daniel Mack,
Peter Meerwald-Stadler
[...]
> +
> +static int bh1780_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + int ret;
> + struct bh1780_data *bh1780;
> + struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> + struct iio_dev *indio_dev;
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
> + return -EIO;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*bh1780));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + bh1780 = iio_priv(indio_dev);
> + bh1780->client = client;
> + i2c_set_clientdata(client, bh1780);
> +
> + /* Power up the device */
> + ret = bh1780_write(bh1780, BH1780_REG_CONTROL, BH1780_PON);
> + if (ret < 0)
> + return ret;
> + msleep(BH1780_PON_DELAY);
> + pm_runtime_get_noresume(&client->dev);
> + pm_runtime_set_active(&client->dev);
> + pm_runtime_enable(&client->dev);
> +
> + ret = bh1780_read(bh1780, BH1780_REG_PARTID);
> + if (ret < 0)
> + goto out_disable_pm;
> + dev_info(&client->dev,
> + "Ambient Light Sensor, Rev : %lu\n",
> + (ret & BH1780_REVMASK));
> +
> + /*
> + * As the device takes 250 ms to even come up with a fresh
> + * measurement after power-on, do not shut it down unnecessarily.
> + * Set autosuspend to a five seconds.
> + */
> + pm_runtime_set_autosuspend_delay(&client->dev, 5000);
I assume the parent device is the i2c controller for the sensor device!?
In the ux500 case, this i2c controller device resides in the VAPE PM
domain, thus when you keep the sensor device runtime resumed, it means
the VAPE PM domain will stay active.
Therefore, I am a bit concerned about the 5 s autosuspend delay here.
Do you have an idea of how a regular use case would be for the sensor?
In other words, at what frequency would you expect a new sensor value
to be read?
Moreover, what are the resume latency requirement of reading a new
sensor value? Is it a problem to have a 250 ms latency for each new
value?
> + pm_runtime_use_autosuspend(&client->dev);
> + pm_runtime_put(&client->dev);
> +
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->info = &bh1780_info;
> + indio_dev->name = id->name;
> + indio_dev->channels = bh1780_channels;
> + indio_dev->num_channels = ARRAY_SIZE(bh1780_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + ret = iio_device_register(indio_dev);
> + if (ret)
> + goto out_disable_pm;
> + return 0;
> +
> +out_disable_pm:
> + pm_runtime_put_noidle(&client->dev);
> + pm_runtime_disable(&client->dev);
> + return ret;
> +}
> +
> +static int bh1780_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> + iio_device_unregister(indio_dev);
> + pm_runtime_get_sync(&client->dev);
I believe you want to power off as well:
bh1780_write(bh1780, BH1780_REG_CONTROL, BH1780_POFF);
> + pm_runtime_put_noidle(&client->dev);
> + pm_runtime_disable(&client->dev);
> +
> + return 0;
> +}
> +
Otherwise this looks great!
Kind regards
Uffe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] iio: light: new driver for the ROHM BH1780
2016-04-06 13:32 ` Ulf Hansson
@ 2016-04-07 10:18 ` Linus Walleij
2016-05-11 8:44 ` Daniel Baluta
0 siblings, 1 reply; 6+ messages in thread
From: Linus Walleij @ 2016-04-07 10:18 UTC (permalink / raw)
To: Ulf Hansson
Cc: Jonathan Cameron, linux-iio@vger.kernel.org, Arnd Bergmann,
Daniel Mack, Peter Meerwald-Stadler, Daniel Baluta
(Looping in Daniel Baluta so he can tell me if I get the Android bits
wrong.)
On Wed, Apr 6, 2016 at 3:32 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> + /*
>> + * As the device takes 250 ms to even come up with a fresh
>> + * measurement after power-on, do not shut it down unnecessarily.
>> + * Set autosuspend to a five seconds.
>> + */
>> + pm_runtime_set_autosuspend_delay(&client->dev, 5000);
>
> I assume the parent device is the i2c controller for the sensor device!?
Yup.
> In the ux500 case, this i2c controller device resides in the VAPE PM
> domain, thus when you keep the sensor device runtime resumed, it means
> the VAPE PM domain will stay active.
After investigating: nope!
The I2C bus is not an issue in this context: the I2C host
will aggressively pm_runtime_suspend()/resume() even
between I2C transactions. With drivers/i2c/busses/i2c-nomadik.c
I get these debug prints:
cat in_illuminance_raw
[ 83.656036] nmk-i2c 80128000.i2c: 80128000.i2c nmk_i2c_runtime_resume()
[ 83.663543] nmk-i2c 80128000.i2c: 80128000.i2c nmk_i2c_runtime_suspend()
[ 83.937103] nmk-i2c 80128000.i2c: 80128000.i2c nmk_i2c_runtime_resume()
[ 83.945465] nmk-i2c 80128000.i2c: 80128000.i2c nmk_i2c_runtime_suspend()
[ 83.952178] nmk-i2c 80128000.i2c: 80128000.i2c nmk_i2c_runtime_resume()
[ 83.960540] nmk-i2c 80128000.i2c: 80128000.i2c nmk_i2c_runtime_suspend()
324
Then some time passes and the light sensor autosuspens leading to some
more i2c traffic:
[ 89.137084] nmk-i2c 80128000.i2c: 80128000.i2c nmk_i2c_runtim)
[ 89.143951] nmk-i2c 80128000.i2c: 80128000.i2c nmk_i2c_runtime_suspend()
Nice, the bus is actually clocked off, hardware can even power down
and the power domain is released. So your concern is addressed.
The reason it works so nicely I think is because the I2C driver
does this in probe():
pm_suspend_ignore_children(&adev->dev, true);
Which is what every i2c and spi driver should be doing, as there is
no need for the I2c/spi etc hardware to be clocked between
transactions. (I think some of them could be better off using
autosuspend though, aggressively hammering down the hardware
between every transaction seems a bit over the top.)
Let's look at the big picture though, as this driver can be
used on any I2C bus.
What do you think about auditing all i2c, spi and other external
bus drivers and send RFT patches to add a
pm_suspend_ignore_children() call to all of those missing it?
It seems to be a common mistake, these I2C drivers are
using runtime PM but not setting pm_suspend_ignore_children():
i2c-at91.c
i2c-cadence.c
i2c-designware-core.c
i2c-imx.c
i2c-omap.c
i2c-qup.c
i2c-rcar.c
i2c-s3c2410.c
i2c-xiic.c
Only Nomadik, hisilicon and SH seems to do it right :(
> Therefore, I am a bit concerned about the 5 s autosuspend delay here.
> Do you have an idea of how a regular use case would be for the sensor?
> In other words, at what frequency would you expect a new sensor value
> to be read?
Even if the I2C bus is not the issue (as described above) there
is still the question about a reasonable timeout.
For a suitable timeout value, let's see what Android does.
The framework lets you read values from the sensor at any
time of course.
Then the framework has a way for applications to register to
SENSOR_LIGHT events like this:
sensorManager.registerListener(sensorEventListener,
sensorManager.getDefaultSensor(Sensor.TYPE_LIGHT),
SensorManager.SENSOR_DELAY_FASTEST);
Possible delays are SENSOR_DELAY_NORMAL,
SENSOR_DELAY_GAME, SENSOR_DELAY_UI and
SENSOR_DELAY_FASTEST.
This is then translated deep in the Android framework
android/platform/frameworks/base in the file
core/java/android/hardware/SensorManager.java
to some chosen microsecond constants (how these were
chosen I have no clue about) here is the essence, the
delays are in microseconds:
SENSOR_DELAY_FASTEST: delay = 0;
SENSOR_DELAY_GAME: delay = 20000;
SENSOR_DELAY_UI: delay = 66667;
SENSOR_DELAY_NORMAL: delay = 200000;
So the delay between readings unless we're constantly
polling the sensor (this is wrong by the way: when using
Intels IIO android HAL, the fastest delay should be capped
to what the sensor reports in its sysfs file *_integration_time
but yeah, sigh) the delay will vary between 20 and 200 ms.
So in theory, Android would ask the sensor for a new value
every 200 ms for the normal usecase. Which is faster than
what the sensor can deliver, so it would report the same
value twice at times.
However I think there is execution overhead in Android,
so it's not really going to happen at that speed. I would have
to run a test rig to get a more reasonable value out.
I don't know if I can choose a better timeout value after
investigating (2)... I feel I know a lot more but to no avail.
Android will just hammer the device with requests when using
the above API, so we could set the timeout to 1 second for
example. Does that sound good?
I guess the algorithm that goes out to read the ambient light
to dim the backlight of the screen is just doing an odd
measurement every now and then. I haven't found this code,
but I strongly suspect it does not subscribe to events like
this, it just calls the raw API to get a sensor value at its
own interval.
> Moreover, what are the resume latency requirement of reading a new
> sensor value? Is it a problem to have a 250 ms latency for each new
> value?
We cannot do very much about that as it is a hardware property.
What you can do if you want lower latency is choose a different
component :)
In Android the sensor has methods like .getMinDelay() and
.getMaxDelay() and the application can adjust its behaviour.
It seems that its event manager doesn't care though, it just
goes and reads the sensor anyway. Maybe it's up to the
Android hardware layer to sleep and wait for a new value.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] iio: light: new driver for the ROHM BH1780
2016-04-06 13:03 [PATCH v4] iio: light: new driver for the ROHM BH1780 Linus Walleij
2016-04-06 13:32 ` Ulf Hansson
@ 2016-04-10 13:42 ` Jonathan Cameron
1 sibling, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2016-04-10 13:42 UTC (permalink / raw)
To: Linus Walleij, linux-iio
Cc: Arnd Bergmann, Ulf Hansson, Daniel Mack, Peter Meerwald-Stadler
On 06/04/16 14:03, Linus Walleij wrote:
> This is a reimplementation of the old misc device driver for the
> ROHM BH1780 ambient light sensor (drivers/misc/bh1780gli.c).
>
> Differences from the old driver:
> - Uses the IIO framework
> - Uses runtime PM to idle the hardware after 5 seconds
> - No weird custom power management from userspace
> - No homebrewn values in sysfs
>
> This uses the same (undocumented) device tree compatible-string
> as the old driver ("rohm,bh1780gli").
>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Daniel Mack <daniel@caiaq.de>
> Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
I was just giving this one last look and noticed an oddity in the sensor
read.... Why is it being done as two byte reads? Datasheet shows it as
a single word read which 'probably' avoids a horrible instability problem...
See inline.
> ---
> ChangeLog v3->v4:
> - Set the system suspend PM ops to pm_force_[suspend|resume]
> so we are in low power also in system suspend.
> - Error prints for failed I2C transactions in runtime
> suspend/resume
> ChangeLog v2->v3:
> - Dropped the _scale information just returning "1" from
> sysfs, userspace should assume scale by 1 if not present.
> - Dropped CC to maintainers who seemingly fell off the planet.
> ChangeLog v1->v2:
> - Use <linux/bitops.h>, BIT(), GENMASK()
> - Do not read state first when suspending.
> - Do not cast debug reg access return value.
> - Whitespace, comments.
>
> I want to phase out the old misc driver, but don't know how that
> should properly be handled. The driver has an old (totally
> undocumented) sysfs ABI for raw measurements, should I add
> this as backward-compatibility and symlink the device to the old
> location?
> ---
> drivers/iio/light/Kconfig | 11 ++
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/bh1780.c | 283 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 295 insertions(+)
> create mode 100644 drivers/iio/light/bh1780.c
>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index cfd3df8416bb..5ee1d453b3d8 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -73,6 +73,17 @@ config BH1750
> To compile this driver as a module, choose M here: the module will
> be called bh1750.
>
> +config BH1780
> + tristate "ROHM BH1780 ambient light sensor"
> + depends on I2C
> + depends on !SENSORS_BH1780
> + help
> + Say Y here to build support for the ROHM BH1780GLI ambient
> + light sensor.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called bh1780.
> +
> config CM32181
> depends on I2C
> tristate "CM32181 driver"
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index b2c31053db0c..4aeee2bd8f49 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_AL3320A) += al3320a.o
> obj-$(CONFIG_APDS9300) += apds9300.o
> obj-$(CONFIG_APDS9960) += apds9960.o
> obj-$(CONFIG_BH1750) += bh1750.o
> +obj-$(CONFIG_BH1780) += bh1780.o
> obj-$(CONFIG_CM32181) += cm32181.o
> obj-$(CONFIG_CM3232) += cm3232.o
> obj-$(CONFIG_CM3323) += cm3323.o
> diff --git a/drivers/iio/light/bh1780.c b/drivers/iio/light/bh1780.c
> new file mode 100644
> index 000000000000..3947aeecd058
> --- /dev/null
> +++ b/drivers/iio/light/bh1780.c
> @@ -0,0 +1,283 @@
> +/*
> + * ROHM 1780GLI Ambient Light Sensor Driver
> + *
> + * Copyright (C) 2016 Linaro Ltd.
> + * Author: Linus Walleij <linus.walleij@linaro.org>
> + * Loosely based on the previous BH1780 ALS misc driver
> + * Copyright (C) 2010 Texas Instruments
> + * Author: Hemanth V <hemanthv@ti.com>
> + */
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/bitops.h>
> +
> +#define BH1780_CMD_BIT BIT(7)
> +#define BH1780_REG_CONTROL 0x00
> +#define BH1780_REG_PARTID 0x0A
> +#define BH1780_REG_MANFID 0x0B
> +#define BH1780_REG_DLOW 0x0C
> +#define BH1780_REG_DHIGH 0x0D
> +
> +#define BH1780_REVMASK GENMASK(3,0)
> +#define BH1780_POWMASK GENMASK(1,0)
> +#define BH1780_POFF (0x0)
> +#define BH1780_PON (0x3)
> +
> +/* power on settling time in ms */
> +#define BH1780_PON_DELAY 2
> +/* max time before value available in ms */
> +#define BH1780_INTERVAL 250
> +
> +struct bh1780_data {
> + struct i2c_client *client;
> +};
> +
> +static int bh1780_write(struct bh1780_data *bh1780, u8 reg, u8 val)
> +{
> + int ret = i2c_smbus_write_byte_data(bh1780->client,
> + BH1780_CMD_BIT | reg,
> + val);
> + if (ret < 0)
> + dev_err(&bh1780->client->dev,
> + "i2c_smbus_write_byte_data failed error "
> + "%d, register %01x\n",
> + ret, reg);
> + return ret;
> +}
> +
> +static int bh1780_read(struct bh1780_data *bh1780, u8 reg)
> +{
> + int ret = i2c_smbus_read_byte_data(bh1780->client,
> + BH1780_CMD_BIT | reg);
> + if (ret < 0)
> + dev_err(&bh1780->client->dev,
> + "i2c_smbus_read_byte_data failed error "
> + "%d, register %01x\n",
> + ret, reg);
> + return ret;
> +}
> +
> +int bh1780_debugfs_reg_access(struct iio_dev *indio_dev,
> + unsigned int reg, unsigned int writeval,
> + unsigned int *readval)
> +{
> + struct bh1780_data *bh1780 = iio_priv(indio_dev);
> + int ret;
> +
> + if (!readval)
> + bh1780_write(bh1780, (u8)reg, (u8)writeval);
> +
> + ret = bh1780_read(bh1780, (u8)reg);
> + if (ret < 0)
> + return ret;
> +
> + *readval = ret;
> +
> + return 0;
> +}
> +
> +static int bh1780_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct bh1780_data *bh1780 = iio_priv(indio_dev);
> + int lsb, msb;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + switch (chan->type) {
> + case IIO_LIGHT:
> + pm_runtime_get_sync(&bh1780->client->dev);
Really silly question, but does the hardware actually ensure you get
a valid pair here? Can see some crazy jumping if the value is around
0xff if not and the read brackets an underlying ADC read
You could play games with double reading to ensure that you get a valid
answer if the hardware doesn't ensure it...
The datasheet I'm looking at does both registers in a single word read...
http://rohmfs.rohm.com/en/products/databook/datasheet/ic/sensor/light/bh1780gli-e.pdf page 7.
I would hope they are gating on that read finishing before updating the
ADC registers. Here however, you are doing 2 single byte reads which looks
very risky...
> + lsb = bh1780_read(bh1780, BH1780_REG_DLOW);
> + if (lsb < 0)
> + return lsb;
> + msb = bh1780_read(bh1780, BH1780_REG_DHIGH);
> + if (msb < 0)
> + return msb;
> + pm_runtime_mark_last_busy(&bh1780->client->dev);
> + pm_runtime_put_autosuspend(&bh1780->client->dev);
> + *val = (msb << 8 | lsb);
> +
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_INT_TIME:
> + *val = 0;
> + *val2 = BH1780_INTERVAL * 1000;
> + return IIO_VAL_INT_PLUS_MICRO;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_info bh1780_info = {
> + .driver_module = THIS_MODULE,
> + .read_raw = bh1780_read_raw,
> + .debugfs_reg_access = bh1780_debugfs_reg_access,
> +};
> +
> +static const struct iio_chan_spec bh1780_channels[] = {
> + {
> + .type = IIO_LIGHT,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_INT_TIME)
> + }
> +};
> +
> +static int bh1780_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + int ret;
> + struct bh1780_data *bh1780;
> + struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> + struct iio_dev *indio_dev;
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
> + return -EIO;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*bh1780));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + bh1780 = iio_priv(indio_dev);
> + bh1780->client = client;
> + i2c_set_clientdata(client, bh1780);
> +
> + /* Power up the device */
> + ret = bh1780_write(bh1780, BH1780_REG_CONTROL, BH1780_PON);
> + if (ret < 0)
> + return ret;
> + msleep(BH1780_PON_DELAY);
> + pm_runtime_get_noresume(&client->dev);
> + pm_runtime_set_active(&client->dev);
> + pm_runtime_enable(&client->dev);
> +
> + ret = bh1780_read(bh1780, BH1780_REG_PARTID);
> + if (ret < 0)
> + goto out_disable_pm;
> + dev_info(&client->dev,
> + "Ambient Light Sensor, Rev : %lu\n",
> + (ret & BH1780_REVMASK));
> +
> + /*
> + * As the device takes 250 ms to even come up with a fresh
> + * measurement after power-on, do not shut it down unnecessarily.
> + * Set autosuspend to a five seconds.
> + */
> + pm_runtime_set_autosuspend_delay(&client->dev, 5000);
> + pm_runtime_use_autosuspend(&client->dev);
> + pm_runtime_put(&client->dev);
> +
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->info = &bh1780_info;
> + indio_dev->name = id->name;
> + indio_dev->channels = bh1780_channels;
> + indio_dev->num_channels = ARRAY_SIZE(bh1780_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + ret = iio_device_register(indio_dev);
> + if (ret)
> + goto out_disable_pm;
> + return 0;
> +
> +out_disable_pm:
> + pm_runtime_put_noidle(&client->dev);
> + pm_runtime_disable(&client->dev);
> + return ret;
> +}
> +
> +static int bh1780_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> + iio_device_unregister(indio_dev);
> + pm_runtime_get_sync(&client->dev);
> + pm_runtime_put_noidle(&client->dev);
> + pm_runtime_disable(&client->dev);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int bh1780_runtime_suspend(struct device *dev)
> +{
> + struct bh1780_data *bh1780;
> + int ret;
> + struct i2c_client *client = to_i2c_client(dev);
> +
> + bh1780 = i2c_get_clientdata(client);
> + ret = bh1780_write(bh1780, BH1780_REG_CONTROL, BH1780_POFF);
> + if (ret < 0) {
> + dev_err(dev, "failed to runtime suspend\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int bh1780_runtime_resume(struct device *dev)
> +{
> + struct bh1780_data *bh1780;
> + int ret;
> + struct i2c_client *client = to_i2c_client(dev);
> +
> + bh1780 = i2c_get_clientdata(client);
> + ret = bh1780_write(bh1780, BH1780_REG_CONTROL, BH1780_PON);
> + if (ret < 0) {
> + dev_err(dev, "failed to runtime resume\n");
> + return ret;
> + }
> +
> + /* Wait for power on, then for a value to be available */
> + msleep(BH1780_PON_DELAY + BH1780_INTERVAL);
> +
> + return 0;
> +}
> +#endif /* CONFIG_PM */
> +
> +static const struct dev_pm_ops bh1780_dev_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> + pm_runtime_force_resume)
> + SET_RUNTIME_PM_OPS(bh1780_runtime_suspend,
> + bh1780_runtime_resume, NULL)
> +};
> +
> +static const struct i2c_device_id bh1780_id[] = {
> + { "bh1780", 0 },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, bh1780_id);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id of_bh1780_match[] = {
> + { .compatible = "rohm,bh1780gli", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, of_bh1780_match);
> +#endif
> +
> +static struct i2c_driver bh1780_driver = {
> + .probe = bh1780_probe,
> + .remove = bh1780_remove,
> + .id_table = bh1780_id,
> + .driver = {
> + .name = "bh1780",
> + .pm = &bh1780_dev_pm_ops,
> + .of_match_table = of_match_ptr(of_bh1780_match),
> + },
> +};
> +
> +module_i2c_driver(bh1780_driver);
> +
> +MODULE_DESCRIPTION("ROHM BH1780GLI Ambient Light Sensor Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] iio: light: new driver for the ROHM BH1780
2016-04-07 10:18 ` Linus Walleij
@ 2016-05-11 8:44 ` Daniel Baluta
2016-05-11 9:08 ` Linus Walleij
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Baluta @ 2016-05-11 8:44 UTC (permalink / raw)
To: Linus Walleij
Cc: Ulf Hansson, Jonathan Cameron, linux-iio@vger.kernel.org,
Arnd Bergmann, Daniel Mack, Peter Meerwald-Stadler, Daniel Baluta,
viorel.suman, Adriana Reus
On Thu, Apr 7, 2016 at 1:18 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> (Looping in Daniel Baluta so he can tell me if I get the Android bits
> wrong.)
>
> On Wed, Apr 6, 2016 at 3:32 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
>>> + /*
>>> + * As the device takes 250 ms to even come up with a fresh
>>> + * measurement after power-on, do not shut it down unnecessarily.
>>> + * Set autosuspend to a five seconds.
>>> + */
>>> + pm_runtime_set_autosuspend_delay(&client->dev, 5000);
Is the device powered off on suspend? If no, this comment is
misleading regarding
the 5 seconds timeout. Because this will only happen at probe, in the
rest of the time
the device will take a measurement much faster.
>>
>> I assume the parent device is the i2c controller for the sensor device!?
>
> Yup.
>
>> In the ux500 case, this i2c controller device resides in the VAPE PM
>> domain, thus when you keep the sensor device runtime resumed, it means
>> the VAPE PM domain will stay active.
>
> After investigating: nope!
>
> The I2C bus is not an issue in this context: the I2C host
> will aggressively pm_runtime_suspend()/resume() even
> between I2C transactions. With drivers/i2c/busses/i2c-nomadik.c
> I get these debug prints:
>
> cat in_illuminance_raw
> [ 83.656036] nmk-i2c 80128000.i2c: 80128000.i2c nmk_i2c_runtime_resume()
> [ 83.663543] nmk-i2c 80128000.i2c: 80128000.i2c nmk_i2c_runtime_suspend()
> [ 83.937103] nmk-i2c 80128000.i2c: 80128000.i2c nmk_i2c_runtime_resume()
> [ 83.945465] nmk-i2c 80128000.i2c: 80128000.i2c nmk_i2c_runtime_suspend()
> [ 83.952178] nmk-i2c 80128000.i2c: 80128000.i2c nmk_i2c_runtime_resume()
> [ 83.960540] nmk-i2c 80128000.i2c: 80128000.i2c nmk_i2c_runtime_suspend()
> 324
>
> Then some time passes and the light sensor autosuspens leading to some
> more i2c traffic:
>
> [ 89.137084] nmk-i2c 80128000.i2c: 80128000.i2c nmk_i2c_runtim)
> [ 89.143951] nmk-i2c 80128000.i2c: 80128000.i2c nmk_i2c_runtime_suspend()
>
> Nice, the bus is actually clocked off, hardware can even power down
> and the power domain is released. So your concern is addressed.
>
> The reason it works so nicely I think is because the I2C driver
> does this in probe():
> pm_suspend_ignore_children(&adev->dev, true);
>
> Which is what every i2c and spi driver should be doing, as there is
> no need for the I2c/spi etc hardware to be clocked between
> transactions. (I think some of them could be better off using
> autosuspend though, aggressively hammering down the hardware
> between every transaction seems a bit over the top.)
>
> Let's look at the big picture though, as this driver can be
> used on any I2C bus.
>
> What do you think about auditing all i2c, spi and other external
> bus drivers and send RFT patches to add a
> pm_suspend_ignore_children() call to all of those missing it?
> It seems to be a common mistake, these I2C drivers are
> using runtime PM but not setting pm_suspend_ignore_children():
> i2c-at91.c
> i2c-cadence.c
> i2c-designware-core.c
> i2c-imx.c
> i2c-omap.c
> i2c-qup.c
> i2c-rcar.c
> i2c-s3c2410.c
> i2c-xiic.c
>
> Only Nomadik, hisilicon and SH seems to do it right :(
>
>> Therefore, I am a bit concerned about the 5 s autosuspend delay here.
>> Do you have an idea of how a regular use case would be for the sensor?
>> In other words, at what frequency would you expect a new sensor value
>> to be read?
>
Android CDD doesn't say much about light sensor sampling frequency requirements
[1] (chapter 7.3.7).
Anyhow, reporting mode for Sensor.TYPE_LIGHT is OnChange [2] and for this
Intel IIO sensors HAL defines:
MIN_ON_CHANGE_SAMPLING_PERIOD_US 200000
MAX_ON_CHANGE_SAMPLING_PERIOD_US 10000000
that is supported sampling frequency is between 0.1 and 5 Hz (pretty relaxed!)
Adding Viorel, perhaps he can add more details on this.
> Even if the I2C bus is not the issue (as described above) there
> is still the question about a reasonable timeout.
>
> For a suitable timeout value, let's see what Android does.
> The framework lets you read values from the sensor at any
> time of course.
>
> Then the framework has a way for applications to register to
> SENSOR_LIGHT events like this:
>
> sensorManager.registerListener(sensorEventListener,
> sensorManager.getDefaultSensor(Sensor.TYPE_LIGHT),
> SensorManager.SENSOR_DELAY_FASTEST);
>
> Possible delays are SENSOR_DELAY_NORMAL,
> SENSOR_DELAY_GAME, SENSOR_DELAY_UI and
> SENSOR_DELAY_FASTEST.
Sure, with the comment that on the newest versions of Android spec
last parameter can by any delay in microseconds. [3]
>
> This is then translated deep in the Android framework
> android/platform/frameworks/base in the file
> core/java/android/hardware/SensorManager.java
> to some chosen microsecond constants (how these were
> chosen I have no clue about) here is the essence, the
> delays are in microseconds:
>
> SENSOR_DELAY_FASTEST: delay = 0;
> SENSOR_DELAY_GAME: delay = 20000;
> SENSOR_DELAY_UI: delay = 66667;
> SENSOR_DELAY_NORMAL: delay = 200000;
>
> So the delay between readings unless we're constantly
> polling the sensor (this is wrong by the way: when using
> Intels IIO android HAL, the fastest delay should be capped
> to what the sensor reports in its sysfs file *_integration_time
> but yeah, sigh) the delay will vary between 20 and 200 ms.
Intel IIO sensors HAL does this, see call trace going to [4]
but it looks at *_sampling_frequency.
>
> So in theory, Android would ask the sensor for a new value
> every 200 ms for the normal usecase. Which is faster than
> what the sensor can deliver, so it would report the same
> value twice at times.
>
> However I think there is execution overhead in Android,
> so it's not really going to happen at that speed. I would have
> to run a test rig to get a more reasonable value out.
>
> I don't know if I can choose a better timeout value after
> investigating (2)... I feel I know a lot more but to no avail.
> Android will just hammer the device with requests when using
> the above API, so we could set the timeout to 1 second for
> example. Does that sound good?
Looking at timeouts chosen by IIO drivers there are all between
2 and 5 seconds.
If Android would ask a new value every 200ms then it wouldn't
make any difference if the timeout is 1s or 5s, we would never
get into suspend.
>
> I guess the algorithm that goes out to read the ambient light
> to dim the backlight of the screen is just doing an odd
> measurement every now and then. I haven't found this code,
> but I strongly suspect it does not subscribe to events like
> this, it just calls the raw API to get a sensor value at its
> own interval.
>
>> Moreover, what are the resume latency requirement of reading a new
>> sensor value? Is it a problem to have a 250 ms latency for each new
>> value?
>
> We cannot do very much about that as it is a hardware property.
> What you can do if you want lower latency is choose a different
> component :)
>
> In Android the sensor has methods like .getMinDelay() and
> .getMaxDelay() and the application can adjust its behaviour.
> It seems that its event manager doesn't care though, it just
> goes and reads the sensor anyway. Maybe it's up to the
> Android hardware layer to sleep and wait for a new value.
Sorry for the late answer.
thanks,
Daniel.
[1] http://static.googleusercontent.com/media/source.android.com/en//compatibility/android-cdd.pdf
[2] https://source.android.com/devices/sensors/sensor-types.html#light
[3] http://developer.android.com/reference/android/hardware/SensorManager.html#registerListener(android.hardware.SensorEventListener,
android.hardware.Sensor, int, int)
[4] https://github.com/01org/android-iio-sensors-hal/blob/master/control.c#L753
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] iio: light: new driver for the ROHM BH1780
2016-05-11 8:44 ` Daniel Baluta
@ 2016-05-11 9:08 ` Linus Walleij
0 siblings, 0 replies; 6+ messages in thread
From: Linus Walleij @ 2016-05-11 9:08 UTC (permalink / raw)
To: Daniel Baluta
Cc: Ulf Hansson, Jonathan Cameron, linux-iio@vger.kernel.org,
Arnd Bergmann, Daniel Mack, Peter Meerwald-Stadler, viorel.suman,
Adriana Reus
On Wed, May 11, 2016 at 10:44 AM, Daniel Baluta <daniel.baluta@intel.com> wrote:
> On Thu, Apr 7, 2016 at 1:18 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> (Looping in Daniel Baluta so he can tell me if I get the Android bits
>> wrong.)
>>
>> On Wed, Apr 6, 2016 at 3:32 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>
>>>> + /*
>>>> + * As the device takes 250 ms to even come up with a fresh
>>>> + * measurement after power-on, do not shut it down unnecessarily.
>>>> + * Set autosuspend to a five seconds.
>>>> + */
>>>> + pm_runtime_set_autosuspend_delay(&client->dev, 5000);
>
>
> Is the device powered off on suspend?
Yes, because:
static const struct dev_pm_ops bh1780_dev_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
pm_runtime_force_resume)
SET_RUNTIME_PM_OPS(bh1780_runtime_suspend,
bh1780_runtime_resume, NULL)
};
> misleading regarding
> the 5 seconds timeout. Because this will only happen at probe, in the
> rest of the time the device will take a measurement much faster.
It will take up to 250ms before a new measurement is taken also
after runtime resume.
>>> Therefore, I am a bit concerned about the 5 s autosuspend delay here.
>>> Do you have an idea of how a regular use case would be for the sensor?
>>> In other words, at what frequency would you expect a new sensor value
>>> to be read?
>
> Android CDD doesn't say much about light sensor sampling frequency requirements
> [1] (chapter 7.3.7).
>
> Anyhow, reporting mode for Sensor.TYPE_LIGHT is OnChange [2] and for this
> Intel IIO sensors HAL defines:
>
> MIN_ON_CHANGE_SAMPLING_PERIOD_US 200000
> MAX_ON_CHANGE_SAMPLING_PERIOD_US 10000000
>
> that is supported sampling frequency is between 0.1 and 5 Hz (pretty relaxed!)
OK thanks it seems we are in compliance :)
>> Then the framework has a way for applications to register to
>> SENSOR_LIGHT events like this:
>>
>> sensorManager.registerListener(sensorEventListener,
>> sensorManager.getDefaultSensor(Sensor.TYPE_LIGHT),
>> SensorManager.SENSOR_DELAY_FASTEST);
>>
>> Possible delays are SENSOR_DELAY_NORMAL,
>> SENSOR_DELAY_GAME, SENSOR_DELAY_UI and
>> SENSOR_DELAY_FASTEST.
>
> Sure, with the comment that on the newest versions of Android spec
> last parameter can by any delay in microseconds. [3]
Yeah I saw some references to that in the code too.
It looked quite hackish ... but nice that they fixed it.
OK people can ask for arbitrary delays.
>> SENSOR_DELAY_FASTEST: delay = 0;
>> SENSOR_DELAY_GAME: delay = 20000;
>> SENSOR_DELAY_UI: delay = 66667;
>> SENSOR_DELAY_NORMAL: delay = 200000;
>>
>> So the delay between readings unless we're constantly
>> polling the sensor (this is wrong by the way: when using
>> Intels IIO android HAL, the fastest delay should be capped
>> to what the sensor reports in its sysfs file *_integration_time
>> but yeah, sigh) the delay will vary between 20 and 200 ms.
>
> Intel IIO sensors HAL does this, see call trace going to [4]
> but it looks at *_sampling_frequency.
OK good, thanks!
>> I don't know if I can choose a better timeout value after
>> investigating (2)... I feel I know a lot more but to no avail.
>> Android will just hammer the device with requests when using
>> the above API, so we could set the timeout to 1 second for
>> example. Does that sound good?
>
> Looking at timeouts chosen by IIO drivers there are all between
> 2 and 5 seconds.
>
> If Android would ask a new value every 200ms then it wouldn't
> make any difference if the timeout is 1s or 5s, we would never
> get into suspend.
True. I'm thinking about implementing autosuspend for
the ST Micro sensors too, I guess I will just use some
blanket value like 5s.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-05-11 9:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-04-06 13:03 [PATCH v4] iio: light: new driver for the ROHM BH1780 Linus Walleij
2016-04-06 13:32 ` Ulf Hansson
2016-04-07 10:18 ` Linus Walleij
2016-05-11 8:44 ` Daniel Baluta
2016-05-11 9:08 ` Linus Walleij
2016-04-10 13:42 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).