* [PATCH v3 1/2] iio: light: add driver for bh1730fvc chips
@ 2018-02-28 0:15 Pierre Bourdon
2018-02-28 0:15 ` [PATCH v3 2/2] dt-bindings: iio: light: Add bh1730fvc bindings Pierre Bourdon
2018-02-28 15:06 ` [PATCH v3 1/2] iio: light: add driver for bh1730fvc chips Andy Shevchenko
0 siblings, 2 replies; 8+ messages in thread
From: Pierre Bourdon @ 2018-02-28 0:15 UTC (permalink / raw)
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Pierre Bourdon, linux-iio, Rob Herring,
Mark Rutland, devicetree, linux-kernel, Daniel Baluta,
Andy Shevchenko
Ambient light sensor that supports visible light and IR measurements and
configurable gain/integration time.
This is written as an additional driver instead of being added into the
existing bh1750 / bh1780 drivers. The bh1730 chip is significantly
different from either of these two:
* bh1750 is not fully smbus compatible and only partially supports
continuous reading mode since not all of its supported chips have this
feature.
* bh1780 does gain adjustment in hardware and exposes lux values
directly, as opposed to bh1730 which provides two raw channels (IR +
visible light) and requires manual gain adjustment and lux computation
in the driver.
Changed in v2:
* Split off DT documentation change into a separate commit.
* Use i2c's probe_new.
Changed in v3:
* Moved from 64 bit to 32 bit arithmetic where possible.
* Changed IIO channel configuration for consistency with other drivers.
* Refactored mathematical computations for readability.
* Removed unnecessary CONFIG_OF checks.
Signed-off-by: Pierre Bourdon <delroth@google.com>
---
drivers/iio/light/Kconfig | 10 +
drivers/iio/light/Makefile | 1 +
drivers/iio/light/bh1730.c | 434 +++++++++++++++++++++++++++++++++++++
3 files changed, 445 insertions(+)
create mode 100644 drivers/iio/light/bh1730.c
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 93fd421b10d7..286a7f78762b 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -63,6 +63,16 @@ config APDS9960
To compile this driver as a module, choose M here: the
module will be called apds9960
+config BH1730
+ tristate "ROHM BH1730 ambient light sensor"
+ depends on I2C
+ help
+ Say Y here to build support for the ROHM BH1730FVC ambient
+ light sensor.
+
+ To compile this driver as a module, choose M here: the module will
+ be called bh1730.
+
config BH1750
tristate "ROHM BH1750 ambient light sensor"
depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index f714067a7816..eb9010a10536 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_ADJD_S311) += adjd_s311.o
obj-$(CONFIG_AL3320A) += al3320a.o
obj-$(CONFIG_APDS9300) += apds9300.o
obj-$(CONFIG_APDS9960) += apds9960.o
+obj-$(CONFIG_BH1730) += bh1730.o
obj-$(CONFIG_BH1750) += bh1750.o
obj-$(CONFIG_BH1780) += bh1780.o
obj-$(CONFIG_CM32181) += cm32181.o
diff --git a/drivers/iio/light/bh1730.c b/drivers/iio/light/bh1730.c
new file mode 100644
index 000000000000..4a23fbd6ff84
--- /dev/null
+++ b/drivers/iio/light/bh1730.c
@@ -0,0 +1,434 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ROHM BH1730 ambient light sensor driver
+ *
+ * Copyright (c) 2018 Google, Inc.
+ * Author: Pierre Bourdon <delroth@google.com>
+ *
+ * Based on a previous non-iio BH1730FVC driver:
+ * Copyright (C) 2012 Samsung Electronics. All rights reserved.
+ * Author: Won Huh <won.huh@samsung.com>
+ *
+ * Data sheets:
+ * http://www.rohm.com/web/global/datasheet/BH1730FVC/bh1730fvc-e
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/time.h>
+
+#define BH1730_CMD_BIT BIT(7)
+
+#define BH1730_REG_CONTROL 0x00
+#define BH1730_REG_TIMING 0x01
+#define BH1730_REG_INTERRUPT 0x02
+#define BH1730_REG_THLLOW 0x03
+#define BH1730_REG_THLHIGH 0x04
+#define BH1730_REG_THHLOW 0x05
+#define BH1730_REG_THHHIGH 0x06
+#define BH1730_REG_GAIN 0x07
+#define BH1730_REG_ID 0x12
+#define BH1730_REG_DATA0LOW 0x14
+#define BH1730_REG_DATA0HIGH 0x15
+#define BH1730_REG_DATA1LOW 0x16
+#define BH1730_REG_DATA1HIGH 0x17
+
+#define BH1730_CONTROL_POWER_ON BIT(0)
+#define BH1730_CONTROL_MEASURE BIT(1)
+
+#define BH1730_INTERNAL_CLOCK_NS 2800
+
+#define BH1730_DEFAULT_INTEG_MS 150
+
+enum bh1730_gain {
+ BH1730_GAIN_1X = 0,
+ BH1730_GAIN_2X,
+ BH1730_GAIN_64X,
+ BH1730_GAIN_128X,
+};
+#define BH1730_MAX_GAIN_MULTIPLIER 128
+
+struct bh1730_data {
+ struct i2c_client *client;
+ enum bh1730_gain gain;
+ int itime;
+};
+#define BH1730_MAX_GAIN_MULTIPLIER 128
+
+static int bh1730_read_word(struct bh1730_data *bh1730, u8 reg)
+{
+ int ret = i2c_smbus_read_word_data(bh1730->client,
+ BH1730_CMD_BIT | reg);
+ if (ret < 0)
+ dev_err(&bh1730->client->dev,
+ "i2c read failed error %d, register %01x\n",
+ ret, reg);
+
+ return ret;
+}
+
+static int bh1730_write(struct bh1730_data *bh1730, u8 reg, u8 val)
+{
+ int ret = i2c_smbus_write_byte_data(bh1730->client,
+ BH1730_CMD_BIT | reg,
+ val);
+ if (ret < 0)
+ dev_err(&bh1730->client->dev,
+ "i2c write failed error %d, register %01x\n",
+ ret, reg);
+
+ return ret;
+}
+
+static int gain_setting_to_multiplier(enum bh1730_gain gain)
+{
+ switch (gain) {
+ case BH1730_GAIN_1X:
+ return 1;
+ case BH1730_GAIN_2X:
+ return 2;
+ case BH1730_GAIN_64X:
+ return 64;
+ case BH1730_GAIN_128X:
+ return 128;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int bh1730_gain_multiplier(struct bh1730_data *bh1730)
+{
+ int multiplier = gain_setting_to_multiplier(bh1730->gain);
+
+ if (multiplier < 0) {
+ dev_warn(&bh1730->client->dev,
+ "invalid gain multiplier settings: %d\n",
+ bh1730->gain);
+ bh1730->gain = BH1730_GAIN_1X;
+ multiplier = 1;
+ }
+
+ return multiplier;
+}
+
+static int bh1730_itime_us(struct bh1730_data *bh1730)
+{
+ return (BH1730_INTERNAL_CLOCK_NS * 964 * (256 - bh1730->itime))
+ / NSEC_PER_USEC;
+}
+
+static int bh1730_set_gain(struct bh1730_data *bh1730, enum bh1730_gain gain)
+{
+ int ret = bh1730_write(bh1730, BH1730_REG_GAIN, gain);
+
+ if (ret < 0)
+ return ret;
+
+ bh1730->gain = gain;
+
+ return 0;
+}
+
+static int bh1730_set_integration_time_ms(struct bh1730_data *bh1730,
+ int time_ms)
+{
+ int ret, time_ns, itime;
+
+ /* Prefilter obviously invalid time_ms values that would overflow. */
+ if (time_ms <= 0 || time_ms > 1000)
+ goto out_of_range;
+
+ time_ns = time_ms * NSEC_PER_MSEC;
+ itime = 256 - DIV_ROUND_CLOSEST(time_ns,
+ BH1730_INTERNAL_CLOCK_NS * 964);
+
+ /* ITIME == 0 is reserved for manual integration mode. */
+ if (itime <= 0 || itime > 255)
+ goto out_of_range;
+
+ ret = bh1730_write(bh1730, BH1730_REG_TIMING, itime);
+ if (ret < 0)
+ return ret;
+
+ bh1730->itime = itime;
+
+ return 0;
+
+out_of_range:
+ dev_warn(&bh1730->client->dev, "integration time out of range: %dms\n",
+ time_ms);
+
+ return -ERANGE;
+}
+
+static void bh1730_wait_for_next_measurement(struct bh1730_data *bh1730)
+{
+ udelay(bh1730_itime_us(bh1730) +
+ DIV_ROUND_UP(BH1730_INTERNAL_CLOCK_NS * 714, NSEC_PER_USEC));
+}
+
+static int bh1730_adjust_gain(struct bh1730_data *bh1730)
+{
+ int visible, ir, highest, gain, ret, i;
+
+ visible = bh1730_read_word(bh1730, BH1730_REG_DATA0LOW);
+ if (visible < 0)
+ return visible;
+
+ ir = bh1730_read_word(bh1730, BH1730_REG_DATA1LOW);
+ if (ir < 0)
+ return ir;
+
+ highest = max(visible, ir);
+
+ /*
+ * If the read value is being clamped, assume the worst and go to the
+ * lowest possible gain. The alternative is doing multiple
+ * recalibrations, which would be slower and have the same effect.
+ */
+ if (highest == USHRT_MAX)
+ gain = 1;
+ else
+ gain = bh1730_gain_multiplier(bh1730);
+
+ highest = (highest * BH1730_MAX_GAIN_MULTIPLIER) / gain;
+
+ /*
+ * Find the lowest gain multiplier which puts the measured values
+ * above 1024. This threshold is chosen to match the gap between 2X
+ * multiplier and 64X (next available) while keeping some margin.
+ */
+ for (i = BH1730_GAIN_1X; i < BH1730_GAIN_128X; ++i) {
+ int adj = highest * gain_setting_to_multiplier(i) /
+ BH1730_MAX_GAIN_MULTIPLIER;
+
+ if (adj >= 1024)
+ break;
+ }
+
+ if (i != bh1730->gain) {
+ ret = bh1730_set_gain(bh1730, i);
+ if (ret < 0)
+ return ret;
+
+ bh1730_wait_for_next_measurement(bh1730);
+ }
+
+ return 0;
+}
+
+static int bh1730_get_millilux(struct bh1730_data *bh1730)
+{
+ int visible, ir, visible_coef, ir_coef;
+ u64 millilux;
+
+ visible = bh1730_read_word(bh1730, BH1730_REG_DATA0LOW);
+ if (visible < 0)
+ return visible;
+
+ ir = bh1730_read_word(bh1730, BH1730_REG_DATA1LOW);
+ if (ir < 0)
+ return ir;
+
+ if (ir * 1000 / visible < 500) {
+ visible_coef = 5002;
+ ir_coef = 7502;
+ } else if (ir * 1000 / visible < 754) {
+ visible_coef = 2250;
+ ir_coef = 2000;
+ } else if (ir * 1000 / visible < 1029) {
+ visible_coef = 1999;
+ ir_coef = 1667;
+ } else if (ir * 1000 / visible < 1373) {
+ visible_coef = 885;
+ ir_coef = 583;
+ } else if (ir * 1000 / visible < 1879) {
+ visible_coef = 309;
+ ir_coef = 165;
+ } else {
+ return 0;
+ }
+
+ millilux = 103ULL * (visible_coef * visible - ir_coef * ir);
+ millilux *= USEC_PER_MSEC;
+ do_div(millilux, bh1730_itime_us(bh1730));
+ do_div(millilux, bh1730_gain_multiplier(bh1730));
+
+ /*
+ * Overflow here can only happen in extreme conditions:
+ * - Completely saturated visible light sensor and no measured IR.
+ * - Integration time < 16ms (driver currently defaults to 150ms).
+ */
+ if (millilux > INT_MAX)
+ return -ERANGE;
+
+ return (int)millilux;
+}
+
+static int bh1730_power_on(struct bh1730_data *bh1730)
+{
+ return bh1730_write(bh1730, BH1730_REG_CONTROL,
+ BH1730_CONTROL_POWER_ON | BH1730_CONTROL_MEASURE);
+}
+
+static int bh1730_set_defaults(struct bh1730_data *bh1730)
+{
+ int ret;
+
+ ret = bh1730_set_gain(bh1730, BH1730_GAIN_1X);
+ if (ret < 0)
+ return ret;
+
+ ret = bh1730_set_integration_time_ms(bh1730, BH1730_DEFAULT_INTEG_MS);
+ if (ret < 0)
+ return ret;
+
+ bh1730_wait_for_next_measurement(bh1730);
+
+ return 0;
+}
+
+static int bh1730_power_off(struct bh1730_data *bh1730)
+{
+ return bh1730_write(bh1730, BH1730_REG_CONTROL, 0);
+}
+
+static int bh1730_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct bh1730_data *bh1730 = iio_priv(indio_dev);
+ int data_reg, ret;
+
+ ret = bh1730_adjust_gain(bh1730);
+ if (ret < 0)
+ return ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_PROCESSED:
+ ret = bh1730_get_millilux(bh1730);
+ if (ret < 0)
+ return ret;
+ *val = ret / 1000;
+ *val2 = (ret % 1000) * 1000;
+ return IIO_VAL_INT_PLUS_MICRO;
+ case IIO_CHAN_INFO_RAW:
+ switch (chan->channel2) {
+ case IIO_MOD_LIGHT_CLEAR:
+ data_reg = BH1730_REG_DATA0LOW;
+ break;
+ case IIO_MOD_LIGHT_IR:
+ data_reg = BH1730_REG_DATA1LOW;
+ break;
+ default:
+ return -EINVAL;
+ }
+ ret = bh1730_read_word(bh1730, data_reg);
+ if (ret < 0)
+ return ret;
+ ret = ret * 1000 / bh1730_gain_multiplier(bh1730);
+ *val = ret / 1000;
+ *val2 = (ret % 1000) * 1000;
+ return IIO_VAL_INT_PLUS_MICRO;
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_info bh1730_info = {
+ .read_raw = bh1730_read_raw,
+};
+
+static const struct iio_chan_spec bh1730_channels[] = {
+ {
+ .type = IIO_LIGHT,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+ },
+ {
+ .type = IIO_INTENSITY,
+ .modified = 1,
+ .channel2 = IIO_MOD_LIGHT_CLEAR,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ },
+ {
+ .type = IIO_INTENSITY,
+ .modified = 1,
+ .channel2 = IIO_MOD_LIGHT_IR,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ },
+};
+
+static int bh1730_probe(struct i2c_client *client)
+{
+ struct bh1730_data *bh1730;
+ struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
+ struct iio_dev *indio_dev;
+ int ret;
+
+ if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
+ return -EIO;
+
+ indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*bh1730));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ bh1730 = iio_priv(indio_dev);
+ bh1730->client = client;
+ i2c_set_clientdata(client, indio_dev);
+
+ ret = bh1730_power_on(bh1730);
+ if (ret < 0)
+ return ret;
+
+ ret = bh1730_set_defaults(bh1730);
+ if (ret < 0)
+ return ret;
+
+ indio_dev->dev.parent = &client->dev;
+ indio_dev->info = &bh1730_info;
+ indio_dev->name = "bh1730";
+ indio_dev->channels = bh1730_channels;
+ indio_dev->num_channels = ARRAY_SIZE(bh1730_channels);
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ ret = iio_device_register(indio_dev);
+ if (ret)
+ goto out_power_off;
+ return 0;
+
+out_power_off:
+ bh1730_power_off(bh1730);
+ return ret;
+}
+
+static int bh1730_remove(struct i2c_client *client)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(client);
+ struct bh1730_data *bh1730 = iio_priv(indio_dev);
+
+ iio_device_unregister(indio_dev);
+ return bh1730_power_off(bh1730);
+}
+
+static const struct of_device_id of_bh1730_match[] = {
+ { .compatible = "rohm,bh1730fvc" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, of_bh1730_match);
+
+static struct i2c_driver bh1730_driver = {
+ .probe_new = bh1730_probe,
+ .remove = bh1730_remove,
+ .driver = {
+ .name = "bh1730",
+ .of_match_table = of_bh1730_match,
+ },
+};
+module_i2c_driver(bh1730_driver);
+
+MODULE_AUTHOR("Pierre Bourdon <delroth@google.com>");
+MODULE_DESCRIPTION("ROHM BH1730FVC driver");
+MODULE_LICENSE("GPL v2");
--
2.16.2.395.g2e18187dfd-goog
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v3 2/2] dt-bindings: iio: light: Add bh1730fvc bindings
2018-02-28 0:15 [PATCH v3 1/2] iio: light: add driver for bh1730fvc chips Pierre Bourdon
@ 2018-02-28 0:15 ` Pierre Bourdon
2018-03-05 22:56 ` Rob Herring
2018-02-28 15:06 ` [PATCH v3 1/2] iio: light: add driver for bh1730fvc chips Andy Shevchenko
1 sibling, 1 reply; 8+ messages in thread
From: Pierre Bourdon @ 2018-02-28 0:15 UTC (permalink / raw)
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Pierre Bourdon, linux-iio, Rob Herring,
Mark Rutland, devicetree, linux-kernel, Daniel Baluta,
Andy Shevchenko
Signed-off-by: Pierre Bourdon <delroth@google.com>
---
.../devicetree/bindings/iio/light/bh1730.txt | 15 +++++++++++++++
1 file changed, 15 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/light/bh1730.txt
diff --git a/Documentation/devicetree/bindings/iio/light/bh1730.txt b/Documentation/devicetree/bindings/iio/light/bh1730.txt
new file mode 100644
index 000000000000..6b38c4010647
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/bh1730.txt
@@ -0,0 +1,15 @@
+* ROHM BH1730FVC ambient light sensor
+
+http://www.rohm.com/web/global/products/-/product/BH1730FVC
+
+Required properties:
+
+ - compatible : should be "rohm,bh1730fvc"
+ - reg : the I2C address of the sensor
+
+Example:
+
+bh1730fvc@29 {
+ compatible = "rohm,bh1730fvc";
+ reg = <0x29>;
+};
--
2.16.2.395.g2e18187dfd-goog
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] iio: light: add driver for bh1730fvc chips
2018-02-28 0:15 [PATCH v3 1/2] iio: light: add driver for bh1730fvc chips Pierre Bourdon
2018-02-28 0:15 ` [PATCH v3 2/2] dt-bindings: iio: light: Add bh1730fvc bindings Pierre Bourdon
@ 2018-02-28 15:06 ` Andy Shevchenko
2018-03-03 15:37 ` Jonathan Cameron
1 sibling, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2018-02-28 15:06 UTC (permalink / raw)
To: Pierre Bourdon
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio, Rob Herring, Mark Rutland,
devicetree, Linux Kernel Mailing List, Daniel Baluta
On Wed, Feb 28, 2018 at 2:15 AM, Pierre Bourdon <delroth@google.com> wrote:
> Ambient light sensor that supports visible light and IR measurements and
> configurable gain/integration time.
>
> This is written as an additional driver instead of being added into the
> existing bh1750 / bh1780 drivers. The bh1730 chip is significantly
> different from either of these two:
>
> * bh1750 is not fully smbus compatible and only partially supports
> continuous reading mode since not all of its supported chips have this
> feature.
>
> * bh1780 does gain adjustment in hardware and exposes lux values
> directly, as opposed to bh1730 which provides two raw channels (IR +
> visible light) and requires manual gain adjustment and lux computation
> in the driver.
>
FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Some minors below. You might address them.
> Changed in v2:
> * Split off DT documentation change into a separate commit.
> * Use i2c's probe_new.
>
> Changed in v3:
> * Moved from 64 bit to 32 bit arithmetic where possible.
> * Changed IIO channel configuration for consistency with other drivers.
> * Refactored mathematical computations for readability.
> * Removed unnecessary CONFIG_OF checks.
>
> Signed-off-by: Pierre Bourdon <delroth@google.com>
> ---
> drivers/iio/light/Kconfig | 10 +
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/bh1730.c | 434 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 445 insertions(+)
> create mode 100644 drivers/iio/light/bh1730.c
>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 93fd421b10d7..286a7f78762b 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -63,6 +63,16 @@ config APDS9960
> To compile this driver as a module, choose M here: the
> module will be called apds9960
>
> +config BH1730
> + tristate "ROHM BH1730 ambient light sensor"
> + depends on I2C
> + help
> + Say Y here to build support for the ROHM BH1730FVC ambient
> + light sensor.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called bh1730.
> +
> config BH1750
> tristate "ROHM BH1750 ambient light sensor"
> depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index f714067a7816..eb9010a10536 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_ADJD_S311) += adjd_s311.o
> obj-$(CONFIG_AL3320A) += al3320a.o
> obj-$(CONFIG_APDS9300) += apds9300.o
> obj-$(CONFIG_APDS9960) += apds9960.o
> +obj-$(CONFIG_BH1730) += bh1730.o
> obj-$(CONFIG_BH1750) += bh1750.o
> obj-$(CONFIG_BH1780) += bh1780.o
> obj-$(CONFIG_CM32181) += cm32181.o
> diff --git a/drivers/iio/light/bh1730.c b/drivers/iio/light/bh1730.c
> new file mode 100644
> index 000000000000..4a23fbd6ff84
> --- /dev/null
> +++ b/drivers/iio/light/bh1730.c
> @@ -0,0 +1,434 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ROHM BH1730 ambient light sensor driver
> + *
> + * Copyright (c) 2018 Google, Inc.
> + * Author: Pierre Bourdon <delroth@google.com>
> + *
> + * Based on a previous non-iio BH1730FVC driver:
> + * Copyright (C) 2012 Samsung Electronics. All rights reserved.
> + * Author: Won Huh <won.huh@samsung.com>
> + *
> + * Data sheets:
> + * http://www.rohm.com/web/global/datasheet/BH1730FVC/bh1730fvc-e
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/time.h>
> +
> +#define BH1730_CMD_BIT BIT(7)
> +
> +#define BH1730_REG_CONTROL 0x00
> +#define BH1730_REG_TIMING 0x01
> +#define BH1730_REG_INTERRUPT 0x02
> +#define BH1730_REG_THLLOW 0x03
> +#define BH1730_REG_THLHIGH 0x04
> +#define BH1730_REG_THHLOW 0x05
> +#define BH1730_REG_THHHIGH 0x06
> +#define BH1730_REG_GAIN 0x07
> +#define BH1730_REG_ID 0x12
> +#define BH1730_REG_DATA0LOW 0x14
> +#define BH1730_REG_DATA0HIGH 0x15
> +#define BH1730_REG_DATA1LOW 0x16
> +#define BH1730_REG_DATA1HIGH 0x17
> +
> +#define BH1730_CONTROL_POWER_ON BIT(0)
> +#define BH1730_CONTROL_MEASURE BIT(1)
> +
> +#define BH1730_INTERNAL_CLOCK_NS 2800
> +
> +#define BH1730_DEFAULT_INTEG_MS 150
> +
> +enum bh1730_gain {
> + BH1730_GAIN_1X = 0,
> + BH1730_GAIN_2X,
> + BH1730_GAIN_64X,
> + BH1730_GAIN_128X,
> +};
> +#define BH1730_MAX_GAIN_MULTIPLIER 128
> +
> +struct bh1730_data {
> + struct i2c_client *client;
> + enum bh1730_gain gain;
> + int itime;
> +};
> +#define BH1730_MAX_GAIN_MULTIPLIER 128
> +
> +static int bh1730_read_word(struct bh1730_data *bh1730, u8 reg)
> +{
> + int ret = i2c_smbus_read_word_data(bh1730->client,
> + BH1730_CMD_BIT | reg);
> + if (ret < 0)
> + dev_err(&bh1730->client->dev,
> + "i2c read failed error %d, register %01x\n",
> + ret, reg);
> +
> + return ret;
> +}
> +
> +static int bh1730_write(struct bh1730_data *bh1730, u8 reg, u8 val)
> +{
> + int ret = i2c_smbus_write_byte_data(bh1730->client,
> + BH1730_CMD_BIT | reg,
> + val);
> + if (ret < 0)
> + dev_err(&bh1730->client->dev,
> + "i2c write failed error %d, register %01x\n",
> + ret, reg);
> +
> + return ret;
> +}
> +
> +static int gain_setting_to_multiplier(enum bh1730_gain gain)
> +{
> + switch (gain) {
> + case BH1730_GAIN_1X:
> + return 1;
> + case BH1730_GAIN_2X:
> + return 2;
> + case BH1730_GAIN_64X:
> + return 64;
> + case BH1730_GAIN_128X:
> + return 128;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int bh1730_gain_multiplier(struct bh1730_data *bh1730)
> +{
> + int multiplier = gain_setting_to_multiplier(bh1730->gain);
> +
> + if (multiplier < 0) {
> + dev_warn(&bh1730->client->dev,
> + "invalid gain multiplier settings: %d\n",
> + bh1730->gain);
> + bh1730->gain = BH1730_GAIN_1X;
> + multiplier = 1;
> + }
> +
> + return multiplier;
> +}
> +
> +static int bh1730_itime_us(struct bh1730_data *bh1730)
> +{
> + return (BH1730_INTERNAL_CLOCK_NS * 964 * (256 - bh1730->itime))
> + / NSEC_PER_USEC;
Better to leave / on the above line
Or
/* At worst case mul will be 688296000, so, there is no 32-bit overflow */
...()
{
int mul = BH1730_INTERNAL_CLOCK_NS * 964 * (256 - bh1730->itime);
return mul / NSEC_PER_USEC;
}
> +}
> +
> +static int bh1730_set_gain(struct bh1730_data *bh1730, enum bh1730_gain gain)
> +{
> + int ret = bh1730_write(bh1730, BH1730_REG_GAIN, gain);
> +
> + if (ret < 0)
> + return ret;
> +
> + bh1730->gain = gain;
> +
> + return 0;
> +}
> +
> +static int bh1730_set_integration_time_ms(struct bh1730_data *bh1730,
> + int time_ms)
> +{
> + int ret, time_ns, itime;
> +
> + /* Prefilter obviously invalid time_ms values that would overflow. */
> + if (time_ms <= 0 || time_ms > 1000)
> + goto out_of_range;
> +
> + time_ns = time_ms * NSEC_PER_MSEC;
> + itime = 256 - DIV_ROUND_CLOSEST(time_ns,
> + BH1730_INTERNAL_CLOCK_NS * 964);
> +
> + /* ITIME == 0 is reserved for manual integration mode. */
> + if (itime <= 0 || itime > 255)
Just side note: Suprisingly how many in_range() implementations we
have in kernel...
> + goto out_of_range;
> +
> + ret = bh1730_write(bh1730, BH1730_REG_TIMING, itime);
> + if (ret < 0)
> + return ret;
> +
> + bh1730->itime = itime;
> +
> + return 0;
> +
> +out_of_range:
> + dev_warn(&bh1730->client->dev, "integration time out of range: %dms\n",
> + time_ms);
> +
> + return -ERANGE;
> +}
> +
> +static void bh1730_wait_for_next_measurement(struct bh1730_data *bh1730)
> +{
> + udelay(bh1730_itime_us(bh1730) +
> + DIV_ROUND_UP(BH1730_INTERNAL_CLOCK_NS * 714, NSEC_PER_USEC));
int step = DIV_ROUND_UP(BH1730_INTERNAL_CLOCK_NS * 714, NSEC_PER_USEC);
udelay(... + step);
?
> +}
> +
> +static int bh1730_adjust_gain(struct bh1730_data *bh1730)
> +{
> + int visible, ir, highest, gain, ret, i;
int visible, ir, highest, gain;
unsigned int i;
int ret;
> +
> + visible = bh1730_read_word(bh1730, BH1730_REG_DATA0LOW);
> + if (visible < 0)
> + return visible;
> +
> + ir = bh1730_read_word(bh1730, BH1730_REG_DATA1LOW);
> + if (ir < 0)
> + return ir;
> +
> + highest = max(visible, ir);
> +
> + /*
> + * If the read value is being clamped, assume the worst and go to the
> + * lowest possible gain. The alternative is doing multiple
> + * recalibrations, which would be slower and have the same effect.
> + */
> + if (highest == USHRT_MAX)
> + gain = 1;
> + else
> + gain = bh1730_gain_multiplier(bh1730);
> +
> + highest = (highest * BH1730_MAX_GAIN_MULTIPLIER) / gain;
> +
> + /*
> + * Find the lowest gain multiplier which puts the measured values
> + * above 1024. This threshold is chosen to match the gap between 2X
> + * multiplier and 64X (next available) while keeping some margin.
> + */
> + for (i = BH1730_GAIN_1X; i < BH1730_GAIN_128X; ++i) {
> + int adj = highest * gain_setting_to_multiplier(i) /
> + BH1730_MAX_GAIN_MULTIPLIER;
I would rather
int adj;
adj = ...;
if (adj ...)
break;
> +
> + if (adj >= 1024)
> + break;
> + }
> +
> + if (i != bh1730->gain) {
> + ret = bh1730_set_gain(bh1730, i);
> + if (ret < 0)
> + return ret;
> +
> + bh1730_wait_for_next_measurement(bh1730);
> + }
> +
> + return 0;
> +}
> +
> +static int bh1730_get_millilux(struct bh1730_data *bh1730)
> +{
> + int visible, ir, visible_coef, ir_coef;
> + u64 millilux;
> +
> + visible = bh1730_read_word(bh1730, BH1730_REG_DATA0LOW);
> + if (visible < 0)
> + return visible;
> +
> + ir = bh1730_read_word(bh1730, BH1730_REG_DATA1LOW);
> + if (ir < 0)
> + return ir;
> +
> + if (ir * 1000 / visible < 500) {
> + visible_coef = 5002;
> + ir_coef = 7502;
> + } else if (ir * 1000 / visible < 754) {
> + visible_coef = 2250;
> + ir_coef = 2000;
> + } else if (ir * 1000 / visible < 1029) {
> + visible_coef = 1999;
> + ir_coef = 1667;
> + } else if (ir * 1000 / visible < 1373) {
> + visible_coef = 885;
> + ir_coef = 583;
> + } else if (ir * 1000 / visible < 1879) {
> + visible_coef = 309;
> + ir_coef = 165;
> + } else {
> + return 0;
> + }
> +
> + millilux = 103ULL * (visible_coef * visible - ir_coef * ir);
> + millilux *= USEC_PER_MSEC;
Can it be one line?
> + do_div(millilux, bh1730_itime_us(bh1730));
> + do_div(millilux, bh1730_gain_multiplier(bh1730));
> +
> + /*
> + * Overflow here can only happen in extreme conditions:
> + * - Completely saturated visible light sensor and no measured IR.
> + * - Integration time < 16ms (driver currently defaults to 150ms).
> + */
> + if (millilux > INT_MAX)
> + return -ERANGE;
> +
> + return (int)millilux;
> +}
> +
> +static int bh1730_power_on(struct bh1730_data *bh1730)
> +{
> + return bh1730_write(bh1730, BH1730_REG_CONTROL,
> + BH1730_CONTROL_POWER_ON | BH1730_CONTROL_MEASURE);
> +}
> +
> +static int bh1730_set_defaults(struct bh1730_data *bh1730)
> +{
> + int ret;
> +
> + ret = bh1730_set_gain(bh1730, BH1730_GAIN_1X);
> + if (ret < 0)
> + return ret;
> +
> + ret = bh1730_set_integration_time_ms(bh1730, BH1730_DEFAULT_INTEG_MS);
> + if (ret < 0)
> + return ret;
> +
> + bh1730_wait_for_next_measurement(bh1730);
> +
> + return 0;
> +}
> +
> +static int bh1730_power_off(struct bh1730_data *bh1730)
> +{
> + return bh1730_write(bh1730, BH1730_REG_CONTROL, 0);
> +}
> +
> +static int bh1730_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct bh1730_data *bh1730 = iio_priv(indio_dev);
> + int data_reg, ret;
> +
> + ret = bh1730_adjust_gain(bh1730);
> + if (ret < 0)
> + return ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + ret = bh1730_get_millilux(bh1730);
> + if (ret < 0)
> + return ret;
> + *val = ret / 1000;
> + *val2 = (ret % 1000) * 1000;
> + return IIO_VAL_INT_PLUS_MICRO;
> + case IIO_CHAN_INFO_RAW:
> + switch (chan->channel2) {
> + case IIO_MOD_LIGHT_CLEAR:
> + data_reg = BH1730_REG_DATA0LOW;
> + break;
> + case IIO_MOD_LIGHT_IR:
> + data_reg = BH1730_REG_DATA1LOW;
> + break;
> + default:
> + return -EINVAL;
> + }
> + ret = bh1730_read_word(bh1730, data_reg);
> + if (ret < 0)
> + return ret;
> + ret = ret * 1000 / bh1730_gain_multiplier(bh1730);
> + *val = ret / 1000;
> + *val2 = (ret % 1000) * 1000;
> + return IIO_VAL_INT_PLUS_MICRO;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_info bh1730_info = {
> + .read_raw = bh1730_read_raw,
> +};
> +
> +static const struct iio_chan_spec bh1730_channels[] = {
> + {
> + .type = IIO_LIGHT,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + },
> + {
> + .type = IIO_INTENSITY,
> + .modified = 1,
> + .channel2 = IIO_MOD_LIGHT_CLEAR,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + },
> + {
> + .type = IIO_INTENSITY,
> + .modified = 1,
> + .channel2 = IIO_MOD_LIGHT_IR,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + },
> +};
> +
> +static int bh1730_probe(struct i2c_client *client)
> +{
> + struct bh1730_data *bh1730;
> + struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
> + return -EIO;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*bh1730));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + bh1730 = iio_priv(indio_dev);
> + bh1730->client = client;
> + i2c_set_clientdata(client, indio_dev);
> +
> + ret = bh1730_power_on(bh1730);
> + if (ret < 0)
> + return ret;
> +
> + ret = bh1730_set_defaults(bh1730);
> + if (ret < 0)
> + return ret;
> +
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->info = &bh1730_info;
> + indio_dev->name = "bh1730";
> + indio_dev->channels = bh1730_channels;
> + indio_dev->num_channels = ARRAY_SIZE(bh1730_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + ret = iio_device_register(indio_dev);
> + if (ret)
> + goto out_power_off;
> + return 0;
> +
> +out_power_off:
> + bh1730_power_off(bh1730);
> + return ret;
> +}
> +
> +static int bh1730_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> + struct bh1730_data *bh1730 = iio_priv(indio_dev);
> +
> + iio_device_unregister(indio_dev);
> + return bh1730_power_off(bh1730);
> +}
> +
> +static const struct of_device_id of_bh1730_match[] = {
> + { .compatible = "rohm,bh1730fvc" },
> + {},
No need to have a comma.
> +};
> +MODULE_DEVICE_TABLE(of, of_bh1730_match);
> +
> +static struct i2c_driver bh1730_driver = {
> + .probe_new = bh1730_probe,
> + .remove = bh1730_remove,
> + .driver = {
> + .name = "bh1730",
> + .of_match_table = of_bh1730_match,
> + },
> +};
> +module_i2c_driver(bh1730_driver);
> +
> +MODULE_AUTHOR("Pierre Bourdon <delroth@google.com>");
> +MODULE_DESCRIPTION("ROHM BH1730FVC driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.16.2.395.g2e18187dfd-goog
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] iio: light: add driver for bh1730fvc chips
2018-02-28 15:06 ` [PATCH v3 1/2] iio: light: add driver for bh1730fvc chips Andy Shevchenko
@ 2018-03-03 15:37 ` Jonathan Cameron
2018-03-03 15:44 ` Andy Shevchenko
0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2018-03-03 15:37 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Pierre Bourdon, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio, Rob Herring, Mark Rutland,
devicetree, Linux Kernel Mailing List, Daniel Baluta
On Wed, 28 Feb 2018 17:06:09 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Wed, Feb 28, 2018 at 2:15 AM, Pierre Bourdon <delroth@google.com> wrote:
> > Ambient light sensor that supports visible light and IR measurements and
> > configurable gain/integration time.
> >
> > This is written as an additional driver instead of being added into the
> > existing bh1750 / bh1780 drivers. The bh1730 chip is significantly
> > different from either of these two:
> >
> > * bh1750 is not fully smbus compatible and only partially supports
> > continuous reading mode since not all of its supported chips have this
> > feature.
> >
> > * bh1780 does gain adjustment in hardware and exposes lux values
> > directly, as opposed to bh1730 which provides two raw channels (IR +
> > visible light) and requires manual gain adjustment and lux computation
> > in the driver.
> >
>
> FWIW,
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Andy, one quick query inline for you.
(I'm just curious ;)
>
> Some minors below. You might address them.
Given we aren't at that close to the merge window opening and these
are all minor changes, it probably makes sense to address them.
Andy has left it up to you however and I don't feel strongly
enough about any of these if you want to leave things as they are.
So let me know either way.
Jonathan
>
> > Changed in v2:
> > * Split off DT documentation change into a separate commit.
> > * Use i2c's probe_new.
> >
> > Changed in v3:
> > * Moved from 64 bit to 32 bit arithmetic where possible.
> > * Changed IIO channel configuration for consistency with other drivers.
> > * Refactored mathematical computations for readability.
> > * Removed unnecessary CONFIG_OF checks.
> >
> > Signed-off-by: Pierre Bourdon <delroth@google.com>
> > ---
> > drivers/iio/light/Kconfig | 10 +
> > drivers/iio/light/Makefile | 1 +
> > drivers/iio/light/bh1730.c | 434 +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 445 insertions(+)
> > create mode 100644 drivers/iio/light/bh1730.c
> >
> > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > index 93fd421b10d7..286a7f78762b 100644
> > --- a/drivers/iio/light/Kconfig
> > +++ b/drivers/iio/light/Kconfig
> > @@ -63,6 +63,16 @@ config APDS9960
> > To compile this driver as a module, choose M here: the
> > module will be called apds9960
> >
> > +config BH1730
> > + tristate "ROHM BH1730 ambient light sensor"
> > + depends on I2C
> > + help
> > + Say Y here to build support for the ROHM BH1730FVC ambient
> > + light sensor.
> > +
> > + To compile this driver as a module, choose M here: the module will
> > + be called bh1730.
> > +
> > config BH1750
> > tristate "ROHM BH1750 ambient light sensor"
> > depends on I2C
> > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> > index f714067a7816..eb9010a10536 100644
> > --- a/drivers/iio/light/Makefile
> > +++ b/drivers/iio/light/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_ADJD_S311) += adjd_s311.o
> > obj-$(CONFIG_AL3320A) += al3320a.o
> > obj-$(CONFIG_APDS9300) += apds9300.o
> > obj-$(CONFIG_APDS9960) += apds9960.o
> > +obj-$(CONFIG_BH1730) += bh1730.o
> > obj-$(CONFIG_BH1750) += bh1750.o
> > obj-$(CONFIG_BH1780) += bh1780.o
> > obj-$(CONFIG_CM32181) += cm32181.o
> > diff --git a/drivers/iio/light/bh1730.c b/drivers/iio/light/bh1730.c
> > new file mode 100644
> > index 000000000000..4a23fbd6ff84
> > --- /dev/null
> > +++ b/drivers/iio/light/bh1730.c
> > @@ -0,0 +1,434 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ROHM BH1730 ambient light sensor driver
> > + *
> > + * Copyright (c) 2018 Google, Inc.
> > + * Author: Pierre Bourdon <delroth@google.com>
> > + *
> > + * Based on a previous non-iio BH1730FVC driver:
> > + * Copyright (C) 2012 Samsung Electronics. All rights reserved.
> > + * Author: Won Huh <won.huh@samsung.com>
> > + *
> > + * Data sheets:
> > + * http://www.rohm.com/web/global/datasheet/BH1730FVC/bh1730fvc-e
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/time.h>
> > +
> > +#define BH1730_CMD_BIT BIT(7)
> > +
> > +#define BH1730_REG_CONTROL 0x00
> > +#define BH1730_REG_TIMING 0x01
> > +#define BH1730_REG_INTERRUPT 0x02
> > +#define BH1730_REG_THLLOW 0x03
> > +#define BH1730_REG_THLHIGH 0x04
> > +#define BH1730_REG_THHLOW 0x05
> > +#define BH1730_REG_THHHIGH 0x06
> > +#define BH1730_REG_GAIN 0x07
> > +#define BH1730_REG_ID 0x12
> > +#define BH1730_REG_DATA0LOW 0x14
> > +#define BH1730_REG_DATA0HIGH 0x15
> > +#define BH1730_REG_DATA1LOW 0x16
> > +#define BH1730_REG_DATA1HIGH 0x17
> > +
> > +#define BH1730_CONTROL_POWER_ON BIT(0)
> > +#define BH1730_CONTROL_MEASURE BIT(1)
> > +
> > +#define BH1730_INTERNAL_CLOCK_NS 2800
> > +
> > +#define BH1730_DEFAULT_INTEG_MS 150
> > +
> > +enum bh1730_gain {
> > + BH1730_GAIN_1X = 0,
> > + BH1730_GAIN_2X,
> > + BH1730_GAIN_64X,
> > + BH1730_GAIN_128X,
> > +};
> > +#define BH1730_MAX_GAIN_MULTIPLIER 128
> > +
> > +struct bh1730_data {
> > + struct i2c_client *client;
> > + enum bh1730_gain gain;
> > + int itime;
> > +};
> > +#define BH1730_MAX_GAIN_MULTIPLIER 128
> > +
> > +static int bh1730_read_word(struct bh1730_data *bh1730, u8 reg)
> > +{
> > + int ret = i2c_smbus_read_word_data(bh1730->client,
> > + BH1730_CMD_BIT | reg);
> > + if (ret < 0)
> > + dev_err(&bh1730->client->dev,
> > + "i2c read failed error %d, register %01x\n",
> > + ret, reg);
> > +
> > + return ret;
> > +}
> > +
> > +static int bh1730_write(struct bh1730_data *bh1730, u8 reg, u8 val)
> > +{
> > + int ret = i2c_smbus_write_byte_data(bh1730->client,
> > + BH1730_CMD_BIT | reg,
> > + val);
> > + if (ret < 0)
> > + dev_err(&bh1730->client->dev,
> > + "i2c write failed error %d, register %01x\n",
> > + ret, reg);
> > +
> > + return ret;
> > +}
> > +
> > +static int gain_setting_to_multiplier(enum bh1730_gain gain)
> > +{
> > + switch (gain) {
> > + case BH1730_GAIN_1X:
> > + return 1;
> > + case BH1730_GAIN_2X:
> > + return 2;
> > + case BH1730_GAIN_64X:
> > + return 64;
> > + case BH1730_GAIN_128X:
> > + return 128;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int bh1730_gain_multiplier(struct bh1730_data *bh1730)
> > +{
> > + int multiplier = gain_setting_to_multiplier(bh1730->gain);
> > +
> > + if (multiplier < 0) {
> > + dev_warn(&bh1730->client->dev,
> > + "invalid gain multiplier settings: %d\n",
> > + bh1730->gain);
> > + bh1730->gain = BH1730_GAIN_1X;
> > + multiplier = 1;
> > + }
> > +
> > + return multiplier;
> > +}
> > +
> > +static int bh1730_itime_us(struct bh1730_data *bh1730)
> > +{
> > + return (BH1730_INTERNAL_CLOCK_NS * 964 * (256 - bh1730->itime))
> > + / NSEC_PER_USEC;
>
> Better to leave / on the above line
>
> Or
>
> /* At worst case mul will be 688296000, so, there is no 32-bit overflow */
> ...()
> {
> int mul = BH1730_INTERNAL_CLOCK_NS * 964 * (256 - bh1730->itime);
>
> return mul / NSEC_PER_USEC;
> }
>
> > +}
> > +
> > +static int bh1730_set_gain(struct bh1730_data *bh1730, enum bh1730_gain gain)
> > +{
> > + int ret = bh1730_write(bh1730, BH1730_REG_GAIN, gain);
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + bh1730->gain = gain;
> > +
> > + return 0;
> > +}
> > +
> > +static int bh1730_set_integration_time_ms(struct bh1730_data *bh1730,
> > + int time_ms)
> > +{
> > + int ret, time_ns, itime;
> > +
> > + /* Prefilter obviously invalid time_ms values that would overflow. */
> > + if (time_ms <= 0 || time_ms > 1000)
> > + goto out_of_range;
> > +
> > + time_ns = time_ms * NSEC_PER_MSEC;
> > + itime = 256 - DIV_ROUND_CLOSEST(time_ns,
> > + BH1730_INTERNAL_CLOCK_NS * 964);
> > +
> > + /* ITIME == 0 is reserved for manual integration mode. */
>
> > + if (itime <= 0 || itime > 255)
>
> Just side note: Suprisingly how many in_range() implementations we
> have in kernel...
I guess one of those things that is so simple it's not worth having
one true in_range to rule them all ;)
>
> > + goto out_of_range;
> > +
> > + ret = bh1730_write(bh1730, BH1730_REG_TIMING, itime);
> > + if (ret < 0)
> > + return ret;
> > +
> > + bh1730->itime = itime;
> > +
> > + return 0;
> > +
> > +out_of_range:
> > + dev_warn(&bh1730->client->dev, "integration time out of range: %dms\n",
> > + time_ms);
> > +
> > + return -ERANGE;
> > +}
> > +
> > +static void bh1730_wait_for_next_measurement(struct bh1730_data *bh1730)
> > +{
> > + udelay(bh1730_itime_us(bh1730) +
> > + DIV_ROUND_UP(BH1730_INTERNAL_CLOCK_NS * 714, NSEC_PER_USEC));
>
> int step = DIV_ROUND_UP(BH1730_INTERNAL_CLOCK_NS * 714, NSEC_PER_USEC);
>
> udelay(... + step);
>
> ?
>
> > +}
> > +
> > +static int bh1730_adjust_gain(struct bh1730_data *bh1730)
> > +{
> > + int visible, ir, highest, gain, ret, i;
>
> int visible, ir, highest, gain;
> unsigned int i;
Is there a strong reason for this one that I'm missing?
(beyond personal taste!)
> int ret;
>
> > +
> > + visible = bh1730_read_word(bh1730, BH1730_REG_DATA0LOW);
> > + if (visible < 0)
> > + return visible;
> > +
> > + ir = bh1730_read_word(bh1730, BH1730_REG_DATA1LOW);
> > + if (ir < 0)
> > + return ir;
> > +
> > + highest = max(visible, ir);
> > +
> > + /*
> > + * If the read value is being clamped, assume the worst and go to the
> > + * lowest possible gain. The alternative is doing multiple
> > + * recalibrations, which would be slower and have the same effect.
> > + */
> > + if (highest == USHRT_MAX)
> > + gain = 1;
> > + else
> > + gain = bh1730_gain_multiplier(bh1730);
> > +
> > + highest = (highest * BH1730_MAX_GAIN_MULTIPLIER) / gain;
> > +
> > + /*
> > + * Find the lowest gain multiplier which puts the measured values
> > + * above 1024. This threshold is chosen to match the gap between 2X
> > + * multiplier and 64X (next available) while keeping some margin.
> > + */
> > + for (i = BH1730_GAIN_1X; i < BH1730_GAIN_128X; ++i) {
>
> > + int adj = highest * gain_setting_to_multiplier(i) /
> > + BH1730_MAX_GAIN_MULTIPLIER;
>
> I would rather
>
> int adj;
>
> adj = ...;
> if (adj ...)
> break;
>
> > +
> > + if (adj >= 1024)
> > + break;
> > + }
> > +
> > + if (i != bh1730->gain) {
> > + ret = bh1730_set_gain(bh1730, i);
> > + if (ret < 0)
> > + return ret;
> > +
> > + bh1730_wait_for_next_measurement(bh1730);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int bh1730_get_millilux(struct bh1730_data *bh1730)
> > +{
> > + int visible, ir, visible_coef, ir_coef;
> > + u64 millilux;
> > +
> > + visible = bh1730_read_word(bh1730, BH1730_REG_DATA0LOW);
> > + if (visible < 0)
> > + return visible;
> > +
> > + ir = bh1730_read_word(bh1730, BH1730_REG_DATA1LOW);
> > + if (ir < 0)
> > + return ir;
> > +
>
> > + if (ir * 1000 / visible < 500) {
> > + visible_coef = 5002;
> > + ir_coef = 7502;
> > + } else if (ir * 1000 / visible < 754) {
> > + visible_coef = 2250;
> > + ir_coef = 2000;
> > + } else if (ir * 1000 / visible < 1029) {
> > + visible_coef = 1999;
> > + ir_coef = 1667;
> > + } else if (ir * 1000 / visible < 1373) {
> > + visible_coef = 885;
> > + ir_coef = 583;
> > + } else if (ir * 1000 / visible < 1879) {
> > + visible_coef = 309;
> > + ir_coef = 165;
> > + } else {
> > + return 0;
> > + }
>
> > +
> > + millilux = 103ULL * (visible_coef * visible - ir_coef * ir);
> > + millilux *= USEC_PER_MSEC;
>
> Can it be one line?
>
> > + do_div(millilux, bh1730_itime_us(bh1730));
> > + do_div(millilux, bh1730_gain_multiplier(bh1730));
> > +
> > + /*
> > + * Overflow here can only happen in extreme conditions:
> > + * - Completely saturated visible light sensor and no measured IR.
> > + * - Integration time < 16ms (driver currently defaults to 150ms).
> > + */
> > + if (millilux > INT_MAX)
> > + return -ERANGE;
> > +
> > + return (int)millilux;
> > +}
> > +
> > +static int bh1730_power_on(struct bh1730_data *bh1730)
> > +{
> > + return bh1730_write(bh1730, BH1730_REG_CONTROL,
> > + BH1730_CONTROL_POWER_ON | BH1730_CONTROL_MEASURE);
> > +}
> > +
> > +static int bh1730_set_defaults(struct bh1730_data *bh1730)
> > +{
> > + int ret;
> > +
> > + ret = bh1730_set_gain(bh1730, BH1730_GAIN_1X);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = bh1730_set_integration_time_ms(bh1730, BH1730_DEFAULT_INTEG_MS);
> > + if (ret < 0)
> > + return ret;
> > +
> > + bh1730_wait_for_next_measurement(bh1730);
> > +
> > + return 0;
> > +}
> > +
> > +static int bh1730_power_off(struct bh1730_data *bh1730)
> > +{
> > + return bh1730_write(bh1730, BH1730_REG_CONTROL, 0);
> > +}
> > +
> > +static int bh1730_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct bh1730_data *bh1730 = iio_priv(indio_dev);
> > + int data_reg, ret;
> > +
> > + ret = bh1730_adjust_gain(bh1730);
> > + if (ret < 0)
> > + return ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_PROCESSED:
> > + ret = bh1730_get_millilux(bh1730);
> > + if (ret < 0)
> > + return ret;
> > + *val = ret / 1000;
> > + *val2 = (ret % 1000) * 1000;
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + case IIO_CHAN_INFO_RAW:
> > + switch (chan->channel2) {
> > + case IIO_MOD_LIGHT_CLEAR:
> > + data_reg = BH1730_REG_DATA0LOW;
> > + break;
> > + case IIO_MOD_LIGHT_IR:
> > + data_reg = BH1730_REG_DATA1LOW;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + ret = bh1730_read_word(bh1730, data_reg);
> > + if (ret < 0)
> > + return ret;
> > + ret = ret * 1000 / bh1730_gain_multiplier(bh1730);
>
> > + *val = ret / 1000;
> > + *val2 = (ret % 1000) * 1000;
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static const struct iio_info bh1730_info = {
> > + .read_raw = bh1730_read_raw,
> > +};
> > +
> > +static const struct iio_chan_spec bh1730_channels[] = {
> > + {
> > + .type = IIO_LIGHT,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > + },
> > + {
> > + .type = IIO_INTENSITY,
> > + .modified = 1,
> > + .channel2 = IIO_MOD_LIGHT_CLEAR,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > + },
> > + {
> > + .type = IIO_INTENSITY,
> > + .modified = 1,
> > + .channel2 = IIO_MOD_LIGHT_IR,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > + },
> > +};
> > +
> > +static int bh1730_probe(struct i2c_client *client)
> > +{
> > + struct bh1730_data *bh1730;
> > + struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> > + struct iio_dev *indio_dev;
> > + int ret;
> > +
> > + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
> > + return -EIO;
> > +
> > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*bh1730));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + bh1730 = iio_priv(indio_dev);
> > + bh1730->client = client;
> > + i2c_set_clientdata(client, indio_dev);
> > +
> > + ret = bh1730_power_on(bh1730);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = bh1730_set_defaults(bh1730);
> > + if (ret < 0)
> > + return ret;
> > +
> > + indio_dev->dev.parent = &client->dev;
> > + indio_dev->info = &bh1730_info;
> > + indio_dev->name = "bh1730";
> > + indio_dev->channels = bh1730_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(bh1730_channels);
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > + ret = iio_device_register(indio_dev);
> > + if (ret)
> > + goto out_power_off;
> > + return 0;
> > +
> > +out_power_off:
> > + bh1730_power_off(bh1730);
> > + return ret;
> > +}
> > +
> > +static int bh1730_remove(struct i2c_client *client)
> > +{
> > + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > + struct bh1730_data *bh1730 = iio_priv(indio_dev);
> > +
> > + iio_device_unregister(indio_dev);
> > + return bh1730_power_off(bh1730);
> > +}
> > +
> > +static const struct of_device_id of_bh1730_match[] = {
> > + { .compatible = "rohm,bh1730fvc" },
>
> > + {},
>
> No need to have a comma.
>
> > +};
> > +MODULE_DEVICE_TABLE(of, of_bh1730_match);
> > +
> > +static struct i2c_driver bh1730_driver = {
> > + .probe_new = bh1730_probe,
> > + .remove = bh1730_remove,
> > + .driver = {
> > + .name = "bh1730",
> > + .of_match_table = of_bh1730_match,
> > + },
> > +};
> > +module_i2c_driver(bh1730_driver);
> > +
> > +MODULE_AUTHOR("Pierre Bourdon <delroth@google.com>");
> > +MODULE_DESCRIPTION("ROHM BH1730FVC driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.16.2.395.g2e18187dfd-goog
> >
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] iio: light: add driver for bh1730fvc chips
2018-03-03 15:37 ` Jonathan Cameron
@ 2018-03-03 15:44 ` Andy Shevchenko
2018-03-03 16:15 ` Jonathan Cameron
0 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2018-03-03 15:44 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Pierre Bourdon, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio, Rob Herring, Mark Rutland,
devicetree, Linux Kernel Mailing List, Daniel Baluta
On Sat, Mar 3, 2018 at 5:37 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On Wed, 28 Feb 2018 17:06:09 +0200
>> On Wed, Feb 28, 2018 at 2:15 AM, Pierre Bourdon <delroth@google.com> wrote:
Better to address even minors before submission.
>> > + if (itime <= 0 || itime > 255)
>>
>> Just side note: Suprisingly how many in_range() implementations we
>> have in kernel...
> I guess one of those things that is so simple it's not worth having
> one true in_range to rule them all ;)
We have already several implementations of the macro.
>> > +static int bh1730_adjust_gain(struct bh1730_data *bh1730)
>> > +{
>> > + int visible, ir, highest, gain, ret, i;
>>
>> int visible, ir, highest, gain;
>> unsigned int i;
>
> Is there a strong reason for this one that I'm missing?
> (beyond personal taste!)
First of all, I'm far from being fan of mixing int ret into other
variable definitions.
unsigned int i OTOH shows explicitly that we have counter which is not
supposed to be negative.
int i in most of the cases will work, so, it's a minor. I'm not
insisting, though having counter variable on separate line is also a
good thing.
In general, having different things in one line is a bad idea for my opinion.
>> int ret;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] iio: light: add driver for bh1730fvc chips
2018-03-03 15:44 ` Andy Shevchenko
@ 2018-03-03 16:15 ` Jonathan Cameron
2018-03-03 20:40 ` Pierre Bourdon (delroth)
0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2018-03-03 16:15 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Pierre Bourdon, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio, Rob Herring, Mark Rutland,
devicetree, Linux Kernel Mailing List, Daniel Baluta
On Sat, 3 Mar 2018 17:44:44 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Sat, Mar 3, 2018 at 5:37 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> > On Wed, 28 Feb 2018 17:06:09 +0200
> >> On Wed, Feb 28, 2018 at 2:15 AM, Pierre Bourdon <delroth@google.com> wrote:
>
> Better to address even minors before submission.
Absolutely.
>
> >> > + if (itime <= 0 || itime > 255)
> >>
> >> Just side note: Suprisingly how many in_range() implementations we
> >> have in kernel...
> > I guess one of those things that is so simple it's not worth having
> > one true in_range to rule them all ;)
>
> We have already several implementations of the macro.
>
> >> > +static int bh1730_adjust_gain(struct bh1730_data *bh1730)
> >> > +{
> >> > + int visible, ir, highest, gain, ret, i;
> >>
> >> int visible, ir, highest, gain;
> >> unsigned int i;
> >
> > Is there a strong reason for this one that I'm missing?
> > (beyond personal taste!)
>
> First of all, I'm far from being fan of mixing int ret into other
> variable definitions.
>
> unsigned int i OTOH shows explicitly that we have counter which is not
> supposed to be negative.
Given it's specifically indexing over an enum (which can have any definition
it likes) I wouldn't normally care, but fair enough.
>
> int i in most of the cases will work, so, it's a minor. I'm not
> insisting, though having counter variable on separate line is also a
> good thing.
>
> In general, having different things in one line is a bad idea for my opinion.
Agreed.
>
> >> int ret;
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] iio: light: add driver for bh1730fvc chips
2018-03-03 16:15 ` Jonathan Cameron
@ 2018-03-03 20:40 ` Pierre Bourdon (delroth)
0 siblings, 0 replies; 8+ messages in thread
From: Pierre Bourdon (delroth) @ 2018-03-03 20:40 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Andy Shevchenko, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio, Rob Herring, Mark Rutland,
devicetree, Linux Kernel Mailing List, Daniel Baluta
On Sat, Mar 3, 2018 at 8:15 AM Jonathan Cameron <jic23@kernel.org> wrote:
> On Sat, 3 Mar 2018 17:44:44 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Sat, Mar 3, 2018 at 5:37 PM, Jonathan Cameron <jic23@kernel.org>
wrote:
> > > On Wed, 28 Feb 2018 17:06:09 +0200
> > >> On Wed, Feb 28, 2018 at 2:15 AM, Pierre Bourdon <delroth@google.com>
wrote:
> >
> > Better to address even minors before submission.
> Absolutely.
Agreed. I'll be sending a hopefully final v4 addressing these next week.
I'm giving v3 just a bit more settle time in case anyone else has comments.
Best,
--
Pierre Bourdon (delroth@)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/2] dt-bindings: iio: light: Add bh1730fvc bindings
2018-02-28 0:15 ` [PATCH v3 2/2] dt-bindings: iio: light: Add bh1730fvc bindings Pierre Bourdon
@ 2018-03-05 22:56 ` Rob Herring
0 siblings, 0 replies; 8+ messages in thread
From: Rob Herring @ 2018-03-05 22:56 UTC (permalink / raw)
To: Pierre Bourdon
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio, Mark Rutland, devicetree,
linux-kernel, Daniel Baluta, Andy Shevchenko
On Wed, Feb 28, 2018 at 01:15:25AM +0100, Pierre Bourdon wrote:
> Signed-off-by: Pierre Bourdon <delroth@google.com>
> ---
> .../devicetree/bindings/iio/light/bh1730.txt | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/light/bh1730.txt
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-03-05 22:56 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-28 0:15 [PATCH v3 1/2] iio: light: add driver for bh1730fvc chips Pierre Bourdon
2018-02-28 0:15 ` [PATCH v3 2/2] dt-bindings: iio: light: Add bh1730fvc bindings Pierre Bourdon
2018-03-05 22:56 ` Rob Herring
2018-02-28 15:06 ` [PATCH v3 1/2] iio: light: add driver for bh1730fvc chips Andy Shevchenko
2018-03-03 15:37 ` Jonathan Cameron
2018-03-03 15:44 ` Andy Shevchenko
2018-03-03 16:15 ` Jonathan Cameron
2018-03-03 20:40 ` Pierre Bourdon (delroth)
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).