* [PATCH] iio: light: add driver for bh1730fvc chips @ 2018-02-21 12:55 Pierre Bourdon 2018-02-21 15:31 ` Daniel Baluta ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Pierre Bourdon @ 2018-02-21 12:55 UTC (permalink / raw) Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Pierre Bourdon, linux-iio, linux-kernel Ambient light sensor that supports visible light and IR measurements and configurable gain/integration time. Signed-off-by: Pierre Bourdon <delroth@google.com> --- .../devicetree/bindings/iio/light/bh1730.txt | 15 + drivers/iio/light/Kconfig | 10 + drivers/iio/light/Makefile | 1 + drivers/iio/light/bh1730.c | 429 ++++++++++++++++++ 4 files changed, 455 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/light/bh1730.txt create mode 100644 drivers/iio/light/bh1730.c 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>; +}; 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..6912aaa3295b --- /dev/null +++ b/drivers/iio/light/bh1730.c @@ -0,0 +1,429 @@ +// 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 2800ULL + +#define BH1730_DEFAULT_INTEG_MS 150 + +enum bh1730_gain { + BH1730_GAIN_1X = 0, + BH1730_GAIN_2X, + BH1730_GAIN_64X, + BH1730_GAIN_128X, +}; + +struct bh1730_data { + struct i2c_client *client; + enum bh1730_gain gain; + int itime; +}; + +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 -1; + } +} + +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 u64 bh1730_itime_ns(struct bh1730_data *bh1730) +{ + return BH1730_INTERNAL_CLOCK_NS * 964 * (256 - bh1730->itime); +} + +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; + u64 time_ns = time_ms * (u64)NSEC_PER_MSEC; + u64 itime_step_ns = BH1730_INTERNAL_CLOCK_NS * 964; + int itime = 256 - (int)DIV_ROUND_CLOSEST_ULL(time_ns, itime_step_ns); + + /* ITIME == 0 is reserved for manual integration mode. */ + if (itime <= 0 || itime > 255) { + dev_warn(&bh1730->client->dev, + "integration time out of range: %dms\n", time_ms); + return -ERANGE; + } + + ret = bh1730_write(bh1730, BH1730_REG_TIMING, itime); + if (ret < 0) + return ret; + + bh1730->itime = itime; + return 0; +} + +static void bh1730_wait_for_next_measurement(struct bh1730_data *bh1730) +{ + ndelay(bh1730_itime_ns(bh1730) + BH1730_INTERNAL_CLOCK_NS * 714); +} + +static int bh1730_adjust_gain(struct bh1730_data *bh1730) +{ + int visible, ir, highest, 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) + highest *= 128; + else + highest = (highest * 128) / bh1730_gain_multiplier(bh1730); + + /* + * 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) / 128; + + 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 s64 bh1730_get_millilux(struct bh1730_data *bh1730) +{ + int visible, ir, visible_coef, ir_coef; + u64 itime_us = bh1730_itime_ns(bh1730) / NSEC_PER_USEC; + 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 = (u64)USEC_PER_MSEC * (visible_coef * visible - ir_coef * ir); + millilux /= bh1730_gain_multiplier(bh1730); + millilux *= 103; + millilux /= itime_us; + return millilux; +} + +static int bh1730_power_on(struct bh1730_data *bh1730) +{ + int ret; + int control = + BH1730_CONTROL_POWER_ON | + BH1730_CONTROL_MEASURE; + + ret = bh1730_write(bh1730, BH1730_REG_CONTROL, control); + if (ret < 0) + return ret; + return 0; +} + +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 ret; + s64 millilux; + + ret = bh1730_adjust_gain(bh1730); + if (ret < 0) + return ret; + + switch (mask) { + case IIO_CHAN_INFO_PROCESSED: + millilux = bh1730_get_millilux(bh1730); + if (millilux < 0) + return millilux; + *val = millilux / 1000; + *val2 = (millilux % 1000) * 1000; + return IIO_VAL_INT_PLUS_MICRO; + case IIO_CHAN_INFO_RAW: + switch (chan->channel2) { + case IIO_MOD_LIGHT_CLEAR: + ret = bh1730_read_word(bh1730, BH1730_REG_DATA0LOW); + if (ret < 0) + return ret; + *val = ret; + return IIO_VAL_INT; + case IIO_MOD_LIGHT_IR: + ret = bh1730_read_word(bh1730, BH1730_REG_DATA1LOW); + if (ret < 0) + return ret; + *val = ret; + return IIO_VAL_INT; + default: + return -EINVAL; + } + case IIO_CHAN_INFO_SCALE: + *val = bh1730_gain_multiplier(bh1730); + return IIO_VAL_INT; + 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, + .modified = 1, + .channel2 = IIO_MOD_LIGHT_BOTH, + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), + }, + { + .type = IIO_LIGHT, + .modified = 1, + .channel2 = IIO_MOD_LIGHT_CLEAR, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + }, + { + .type = IIO_LIGHT, + .modified = 1, + .channel2 = IIO_MOD_LIGHT_IR, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + }, +}; + +static int bh1730_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + 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 i2c_device_id bh1730_id[] = { + { "bh1730", 0 }, + {}, +}; +MODULE_DEVICE_TABLE(i2c, bh1730_id); + +#ifdef CONFIG_OF +static const struct of_device_id of_bh1730_match[] = { + { .compatible = "rohm,bh1730fvc" }, + {}, +}; +MODULE_DEVICE_TABLE(of, of_bh1730_match); +#endif + +static struct i2c_driver bh1730_driver = { + .probe = bh1730_probe, + .remove = bh1730_remove, + .id_table = bh1730_id, + .driver = { + .name = "bh1730", + .of_match_table = of_match_ptr(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.1.291.g4437f3f132-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] iio: light: add driver for bh1730fvc chips 2018-02-21 12:55 [PATCH] iio: light: add driver for bh1730fvc chips Pierre Bourdon @ 2018-02-21 15:31 ` Daniel Baluta 2018-02-21 16:15 ` Pierre Bourdon (delroth) 2018-02-21 19:45 ` [PATCH v2 1/2] " Pierre Bourdon 2018-02-23 7:56 ` [PATCH] " kbuild test robot 2 siblings, 1 reply; 11+ messages in thread From: Daniel Baluta @ 2018-02-21 15:31 UTC (permalink / raw) To: Pierre Bourdon Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, Linux Kernel Mailing List Hi Pierre, Few comments inline: On Wed, Feb 21, 2018 at 2:55 PM, Pierre Bourdon <delroth@google.com> wrote: > Ambient light sensor that supports visible light and IR measurements and > configurable gain/integration time. > Can you have a quick look to existing ROHM light sensor support. Perhaps your sensor is similar enough with existing code. I'm talking about: drivers/iio/light/{bh1750.c bh1780.c } > Signed-off-by: Pierre Bourdon <delroth@google.com> > --- > .../devicetree/bindings/iio/light/bh1730.txt | 15 + > drivers/iio/light/Kconfig | 10 + > drivers/iio/light/Makefile | 1 + > drivers/iio/light/bh1730.c | 429 ++++++++++++++++++ > 4 files changed, 455 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/light/bh1730.txt > create mode 100644 drivers/iio/light/bh1730.c > > 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>; > +}; Usually the devicetree binding part should be sent as a separate patch with device-tree mailing list and maintainer at CC. Use ./scripts/get_maintainer.pl to find the exact emails. Mind that you should sent a patchseries: 1/2 - driver code 2/2 - documentation > 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..6912aaa3295b > --- /dev/null > +++ b/drivers/iio/light/bh1730.c > @@ -0,0 +1,429 @@ > +// 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 2800ULL > + > +#define BH1730_DEFAULT_INTEG_MS 150 > + > +enum bh1730_gain { > + BH1730_GAIN_1X = 0, > + BH1730_GAIN_2X, > + BH1730_GAIN_64X, > + BH1730_GAIN_128X, > +}; > + > +struct bh1730_data { > + struct i2c_client *client; > + enum bh1730_gain gain; > + int itime; > +}; > + > +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 -1; > + } > +} > + > +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 u64 bh1730_itime_ns(struct bh1730_data *bh1730) > +{ > + return BH1730_INTERNAL_CLOCK_NS * 964 * (256 - bh1730->itime); > +} > + > +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; > + u64 time_ns = time_ms * (u64)NSEC_PER_MSEC; > + u64 itime_step_ns = BH1730_INTERNAL_CLOCK_NS * 964; > + int itime = 256 - (int)DIV_ROUND_CLOSEST_ULL(time_ns, itime_step_ns); > + > + /* ITIME == 0 is reserved for manual integration mode. */ > + if (itime <= 0 || itime > 255) { > + dev_warn(&bh1730->client->dev, > + "integration time out of range: %dms\n", time_ms); > + return -ERANGE; > + } > + > + ret = bh1730_write(bh1730, BH1730_REG_TIMING, itime); > + if (ret < 0) > + return ret; > + > + bh1730->itime = itime; > + return 0; > +} > + > +static void bh1730_wait_for_next_measurement(struct bh1730_data *bh1730) > +{ > + ndelay(bh1730_itime_ns(bh1730) + BH1730_INTERNAL_CLOCK_NS * 714); > +} > + > +static int bh1730_adjust_gain(struct bh1730_data *bh1730) > +{ > + int visible, ir, highest, 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) > + highest *= 128; > + else > + highest = (highest * 128) / bh1730_gain_multiplier(bh1730); > + > + /* > + * 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) / 128; > + > + 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 s64 bh1730_get_millilux(struct bh1730_data *bh1730) > +{ > + int visible, ir, visible_coef, ir_coef; > + u64 itime_us = bh1730_itime_ns(bh1730) / NSEC_PER_USEC; > + 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 = (u64)USEC_PER_MSEC * (visible_coef * visible - ir_coef * ir); > + millilux /= bh1730_gain_multiplier(bh1730); > + millilux *= 103; > + millilux /= itime_us; > + return millilux; > +} > + > +static int bh1730_power_on(struct bh1730_data *bh1730) > +{ > + int ret; > + int control = > + BH1730_CONTROL_POWER_ON | > + BH1730_CONTROL_MEASURE; > + > + ret = bh1730_write(bh1730, BH1730_REG_CONTROL, control); > + if (ret < 0) > + return ret; Directly return bh1730_write , remove ret and control. Similar with power_off. > + return 0; > +} > + > +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 ret; > + s64 millilux; > + > + ret = bh1730_adjust_gain(bh1730); > + if (ret < 0) > + return ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_PROCESSED: > + millilux = bh1730_get_millilux(bh1730); > + if (millilux < 0) > + return millilux; > + *val = millilux / 1000; > + *val2 = (millilux % 1000) * 1000; > + return IIO_VAL_INT_PLUS_MICRO; > + case IIO_CHAN_INFO_RAW: > + switch (chan->channel2) { > + case IIO_MOD_LIGHT_CLEAR: > + ret = bh1730_read_word(bh1730, BH1730_REG_DATA0LOW); > + if (ret < 0) > + return ret; > + *val = ret; > + return IIO_VAL_INT; > + case IIO_MOD_LIGHT_IR: > + ret = bh1730_read_word(bh1730, BH1730_REG_DATA1LOW); > + if (ret < 0) > + return ret; > + *val = ret; > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > + case IIO_CHAN_INFO_SCALE: > + *val = bh1730_gain_multiplier(bh1730); > + return IIO_VAL_INT; > + 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, > + .modified = 1, > + .channel2 = IIO_MOD_LIGHT_BOTH, > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), > + }, > + { > + .type = IIO_LIGHT, > + .modified = 1, > + .channel2 = IIO_MOD_LIGHT_CLEAR, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE), > + }, > + { > + .type = IIO_LIGHT, > + .modified = 1, > + .channel2 = IIO_MOD_LIGHT_IR, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE), > + }, > +}; > + > +static int bh1730_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + 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 i2c_device_id bh1730_id[] = { > + { "bh1730", 0 }, > + {}, > +}; > +MODULE_DEVICE_TABLE(i2c, bh1730_id); > + > +#ifdef CONFIG_OF > +static const struct of_device_id of_bh1730_match[] = { > + { .compatible = "rohm,bh1730fvc" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, of_bh1730_match); > +#endif > + > +static struct i2c_driver bh1730_driver = { > + .probe = bh1730_probe, I wonder if you can use the probe_new API here. e.g https://patchwork.kernel.org/patch/9395073/ > + .remove = bh1730_remove, > + .id_table = bh1730_id, > + .driver = { > + .name = "bh1730", > + .of_match_table = of_match_ptr(of_bh1730_match), > + }, > +}; > +module_i2c_driver(bh1730_driver); > + > +MODULE_AUTHOR("Pierre Bourdon <delroth@google.com>"); > +MODULE_DESCRIPTION("ROHM BH1730FVC driver"); > +MODULE_LICENSE("GPL v2"); > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] iio: light: add driver for bh1730fvc chips 2018-02-21 15:31 ` Daniel Baluta @ 2018-02-21 16:15 ` Pierre Bourdon (delroth) 2018-02-21 18:32 ` Daniel Baluta 0 siblings, 1 reply; 11+ messages in thread From: Pierre Bourdon (delroth) @ 2018-02-21 16:15 UTC (permalink / raw) To: Daniel Baluta Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, Linux Kernel Mailing List Hi Daniel, On Wed, Feb 21, 2018 at 4:31 PM, Daniel Baluta <daniel.baluta@gmail.com> wrote: > On Wed, Feb 21, 2018 at 2:55 PM, Pierre Bourdon <delroth@google.com> wrote: >> Ambient light sensor that supports visible light and IR measurements and >> configurable gain/integration time. >> > > Can you have a quick look to existing ROHM light sensor support. > Perhaps your sensor > is similar enough with existing code. > > I'm talking about: > > drivers/iio/light/{bh1750.c bh1780.c } bh1750 does things sufficiently different in its protocol that the amount of common code would be fairly small. Unlike bh1730/bh1780 it's not fully SMBus compatible for example. bh1780 could probably be implemented as a subset of bh1730. It's basically a simplified bh1730 with less knobs. But there is still a significant amount of differences that make common code difficult to extract. For example, bh1780 reads lux values directly from the chip, whereas bh1730 has two raw channels that need to be combined in software to get a processed lux value. It could be done, but my gut feeling is that writing a "HAL" for this is going to make things more complicated, not simpler. My experience in this domain is close to zero, so I'm completely open to giving it a try if you think merging these two makes sense. >> Signed-off-by: Pierre Bourdon <delroth@google.com> >> --- >> .../devicetree/bindings/iio/light/bh1730.txt | 15 + >> drivers/iio/light/Kconfig | 10 + >> drivers/iio/light/Makefile | 1 + >> drivers/iio/light/bh1730.c | 429 ++++++++++++++++++ >> 4 files changed, 455 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/iio/light/bh1730.txt >> create mode 100644 drivers/iio/light/bh1730.c >> >> 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>; >> +}; > > Usually the devicetree binding part should be sent as a separate patch > with device-tree mailing list and maintainer at CC. > > Use ./scripts/get_maintainer.pl to find the exact emails. > > Mind that you should sent a patchseries: > > 1/2 - driver code > 2/2 - documentation Will do. Should the of_match_table in the driver code be part of 1/2 (driver) or 2/2 (dt-bindings)? I'll send a v2 with this + the other changes you've suggested. Thanks! >> 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..6912aaa3295b >> --- /dev/null >> +++ b/drivers/iio/light/bh1730.c >> @@ -0,0 +1,429 @@ >> +// 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 2800ULL >> + >> +#define BH1730_DEFAULT_INTEG_MS 150 >> + >> +enum bh1730_gain { >> + BH1730_GAIN_1X = 0, >> + BH1730_GAIN_2X, >> + BH1730_GAIN_64X, >> + BH1730_GAIN_128X, >> +}; >> + >> +struct bh1730_data { >> + struct i2c_client *client; >> + enum bh1730_gain gain; >> + int itime; >> +}; >> + >> +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 -1; >> + } >> +} >> + >> +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 u64 bh1730_itime_ns(struct bh1730_data *bh1730) >> +{ >> + return BH1730_INTERNAL_CLOCK_NS * 964 * (256 - bh1730->itime); >> +} >> + >> +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; >> + u64 time_ns = time_ms * (u64)NSEC_PER_MSEC; >> + u64 itime_step_ns = BH1730_INTERNAL_CLOCK_NS * 964; >> + int itime = 256 - (int)DIV_ROUND_CLOSEST_ULL(time_ns, itime_step_ns); >> + >> + /* ITIME == 0 is reserved for manual integration mode. */ >> + if (itime <= 0 || itime > 255) { >> + dev_warn(&bh1730->client->dev, >> + "integration time out of range: %dms\n", time_ms); >> + return -ERANGE; >> + } >> + >> + ret = bh1730_write(bh1730, BH1730_REG_TIMING, itime); >> + if (ret < 0) >> + return ret; >> + >> + bh1730->itime = itime; >> + return 0; >> +} >> + >> +static void bh1730_wait_for_next_measurement(struct bh1730_data *bh1730) >> +{ >> + ndelay(bh1730_itime_ns(bh1730) + BH1730_INTERNAL_CLOCK_NS * 714); >> +} >> + >> +static int bh1730_adjust_gain(struct bh1730_data *bh1730) >> +{ >> + int visible, ir, highest, 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) >> + highest *= 128; >> + else >> + highest = (highest * 128) / bh1730_gain_multiplier(bh1730); >> + >> + /* >> + * 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) / 128; >> + >> + 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 s64 bh1730_get_millilux(struct bh1730_data *bh1730) >> +{ >> + int visible, ir, visible_coef, ir_coef; >> + u64 itime_us = bh1730_itime_ns(bh1730) / NSEC_PER_USEC; >> + 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 = (u64)USEC_PER_MSEC * (visible_coef * visible - ir_coef * ir); >> + millilux /= bh1730_gain_multiplier(bh1730); >> + millilux *= 103; >> + millilux /= itime_us; >> + return millilux; >> +} >> + >> +static int bh1730_power_on(struct bh1730_data *bh1730) >> +{ >> + int ret; >> + int control = >> + BH1730_CONTROL_POWER_ON | >> + BH1730_CONTROL_MEASURE; >> + >> + ret = bh1730_write(bh1730, BH1730_REG_CONTROL, control); >> + if (ret < 0) >> + return ret; > > Directly return bh1730_write , remove ret and control. Similar with power_off. > >> + return 0; >> +} >> + >> +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 ret; >> + s64 millilux; >> + >> + ret = bh1730_adjust_gain(bh1730); >> + if (ret < 0) >> + return ret; >> + >> + switch (mask) { >> + case IIO_CHAN_INFO_PROCESSED: >> + millilux = bh1730_get_millilux(bh1730); >> + if (millilux < 0) >> + return millilux; >> + *val = millilux / 1000; >> + *val2 = (millilux % 1000) * 1000; >> + return IIO_VAL_INT_PLUS_MICRO; >> + case IIO_CHAN_INFO_RAW: >> + switch (chan->channel2) { >> + case IIO_MOD_LIGHT_CLEAR: >> + ret = bh1730_read_word(bh1730, BH1730_REG_DATA0LOW); >> + if (ret < 0) >> + return ret; >> + *val = ret; >> + return IIO_VAL_INT; >> + case IIO_MOD_LIGHT_IR: >> + ret = bh1730_read_word(bh1730, BH1730_REG_DATA1LOW); >> + if (ret < 0) >> + return ret; >> + *val = ret; >> + return IIO_VAL_INT; >> + default: >> + return -EINVAL; >> + } >> + case IIO_CHAN_INFO_SCALE: >> + *val = bh1730_gain_multiplier(bh1730); >> + return IIO_VAL_INT; >> + 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, >> + .modified = 1, >> + .channel2 = IIO_MOD_LIGHT_BOTH, >> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), >> + }, >> + { >> + .type = IIO_LIGHT, >> + .modified = 1, >> + .channel2 = IIO_MOD_LIGHT_CLEAR, >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | >> + BIT(IIO_CHAN_INFO_SCALE), >> + }, >> + { >> + .type = IIO_LIGHT, >> + .modified = 1, >> + .channel2 = IIO_MOD_LIGHT_IR, >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | >> + BIT(IIO_CHAN_INFO_SCALE), >> + }, >> +}; >> + >> +static int bh1730_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + 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 i2c_device_id bh1730_id[] = { >> + { "bh1730", 0 }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(i2c, bh1730_id); >> + >> +#ifdef CONFIG_OF >> +static const struct of_device_id of_bh1730_match[] = { >> + { .compatible = "rohm,bh1730fvc" }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, of_bh1730_match); >> +#endif >> + >> +static struct i2c_driver bh1730_driver = { >> + .probe = bh1730_probe, > > I wonder if you can use the probe_new API here. > > e.g > > https://patchwork.kernel.org/patch/9395073/ >> + .remove = bh1730_remove, >> + .id_table = bh1730_id, >> + .driver = { >> + .name = "bh1730", >> + .of_match_table = of_match_ptr(of_bh1730_match), >> + }, >> +}; >> +module_i2c_driver(bh1730_driver); >> + >> +MODULE_AUTHOR("Pierre Bourdon <delroth@google.com>"); >> +MODULE_DESCRIPTION("ROHM BH1730FVC driver"); >> +MODULE_LICENSE("GPL v2"); >> -- Pierre Bourdon (delroth@) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] iio: light: add driver for bh1730fvc chips 2018-02-21 16:15 ` Pierre Bourdon (delroth) @ 2018-02-21 18:32 ` Daniel Baluta 0 siblings, 0 replies; 11+ messages in thread From: Daniel Baluta @ 2018-02-21 18:32 UTC (permalink / raw) To: Pierre Bourdon (delroth) Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, Linux Kernel Mailing List On Wed, Feb 21, 2018 at 6:15 PM, Pierre Bourdon (delroth) <delroth@google.com> wrote: > Hi Daniel, > > On Wed, Feb 21, 2018 at 4:31 PM, Daniel Baluta <daniel.baluta@gmail.com> wrote: >> On Wed, Feb 21, 2018 at 2:55 PM, Pierre Bourdon <delroth@google.com> wrote: >>> Ambient light sensor that supports visible light and IR measurements and >>> configurable gain/integration time. >>> >> >> Can you have a quick look to existing ROHM light sensor support. >> Perhaps your sensor >> is similar enough with existing code. >> >> I'm talking about: >> >> drivers/iio/light/{bh1750.c bh1780.c } > > bh1750 does things sufficiently different in its protocol that the > amount of common code would be fairly small. Unlike bh1730/bh1780 it's > not fully SMBus compatible for example. > > bh1780 could probably be implemented as a subset of bh1730. It's > basically a simplified bh1730 with less knobs. But there is still a > significant amount of differences that make common code difficult to > extract. For example, bh1780 reads lux values directly from the chip, > whereas bh1730 has two raw channels that need to be combined in > software to get a processed lux value. It could be done, but my gut > feeling is that writing a "HAL" for this is going to make things more > complicated, not simpler. > I only had a very quick look at existing ROHM sensors drivers. So, sure if it doesn't makes sense then we can have bh1730 as a standalone driver. <snip> >> >> Usually the devicetree binding part should be sent as a separate patch >> with device-tree mailing list and maintainer at CC. >> >> Use ./scripts/get_maintainer.pl to find the exact emails. >> >> Mind that you should sent a patchseries: >> >> 1/2 - driver code >> 2/2 - documentation > > Will do. Should the of_match_table in the driver code be part of 1/2 > (driver) or 2/2 (dt-bindings)? > Should be part of 1/2. 2/2 should only containt the patch for Documentation. > I'll send a v2 with this + the other changes you've suggested. Great thanks! Any idea how to make my gmail not to wrap the lines in plain text mode? :D It drives me crazy when I reread my emails. thanks, Daniel. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/2] iio: light: add driver for bh1730fvc chips 2018-02-21 12:55 [PATCH] iio: light: add driver for bh1730fvc chips Pierre Bourdon 2018-02-21 15:31 ` Daniel Baluta @ 2018-02-21 19:45 ` Pierre Bourdon 2018-02-21 19:45 ` [PATCH v2 2/2] dt-bindings: iio: light: Add bh1730fvc bindings Pierre Bourdon ` (2 more replies) 2018-02-23 7:56 ` [PATCH] " kbuild test robot 2 siblings, 3 replies; 11+ messages in thread From: Pierre Bourdon @ 2018-02-21 19:45 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 Ambient light sensor that supports visible light and IR measurements and configurable gain/integration time. Changed in v2: * Split off DT documentation change into a separate commit. * Use i2c's probe_new. Signed-off-by: Pierre Bourdon <delroth@google.com> --- drivers/iio/light/Kconfig | 10 + drivers/iio/light/Makefile | 1 + drivers/iio/light/bh1730.c | 414 +++++++++++++++++++++++++++++++++++++ 3 files changed, 425 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..7f1c50b3c55b --- /dev/null +++ b/drivers/iio/light/bh1730.c @@ -0,0 +1,414 @@ +// 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 2800ULL + +#define BH1730_DEFAULT_INTEG_MS 150 + +enum bh1730_gain { + BH1730_GAIN_1X = 0, + BH1730_GAIN_2X, + BH1730_GAIN_64X, + BH1730_GAIN_128X, +}; + +struct bh1730_data { + struct i2c_client *client; + enum bh1730_gain gain; + int itime; +}; + +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 -1; + } +} + +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 u64 bh1730_itime_ns(struct bh1730_data *bh1730) +{ + return BH1730_INTERNAL_CLOCK_NS * 964 * (256 - bh1730->itime); +} + +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; + u64 time_ns = time_ms * (u64)NSEC_PER_MSEC; + u64 itime_step_ns = BH1730_INTERNAL_CLOCK_NS * 964; + int itime = 256 - (int)DIV_ROUND_CLOSEST_ULL(time_ns, itime_step_ns); + + /* ITIME == 0 is reserved for manual integration mode. */ + if (itime <= 0 || itime > 255) { + dev_warn(&bh1730->client->dev, + "integration time out of range: %dms\n", time_ms); + return -ERANGE; + } + + ret = bh1730_write(bh1730, BH1730_REG_TIMING, itime); + if (ret < 0) + return ret; + + bh1730->itime = itime; + return 0; +} + +static void bh1730_wait_for_next_measurement(struct bh1730_data *bh1730) +{ + ndelay(bh1730_itime_ns(bh1730) + BH1730_INTERNAL_CLOCK_NS * 714); +} + +static int bh1730_adjust_gain(struct bh1730_data *bh1730) +{ + int visible, ir, highest, 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) + highest *= 128; + else + highest = (highest * 128) / bh1730_gain_multiplier(bh1730); + + /* + * 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) / 128; + + 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 s64 bh1730_get_millilux(struct bh1730_data *bh1730) +{ + int visible, ir, visible_coef, ir_coef; + u64 itime_us = bh1730_itime_ns(bh1730) / NSEC_PER_USEC; + 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 = (u64)USEC_PER_MSEC * (visible_coef * visible - ir_coef * ir); + millilux /= bh1730_gain_multiplier(bh1730); + millilux *= 103; + millilux /= itime_us; + return 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 ret; + s64 millilux; + + ret = bh1730_adjust_gain(bh1730); + if (ret < 0) + return ret; + + switch (mask) { + case IIO_CHAN_INFO_PROCESSED: + millilux = bh1730_get_millilux(bh1730); + if (millilux < 0) + return millilux; + *val = millilux / 1000; + *val2 = (millilux % 1000) * 1000; + return IIO_VAL_INT_PLUS_MICRO; + case IIO_CHAN_INFO_RAW: + switch (chan->channel2) { + case IIO_MOD_LIGHT_CLEAR: + ret = bh1730_read_word(bh1730, BH1730_REG_DATA0LOW); + if (ret < 0) + return ret; + *val = ret; + return IIO_VAL_INT; + case IIO_MOD_LIGHT_IR: + ret = bh1730_read_word(bh1730, BH1730_REG_DATA1LOW); + if (ret < 0) + return ret; + *val = ret; + return IIO_VAL_INT; + default: + return -EINVAL; + } + case IIO_CHAN_INFO_SCALE: + *val = bh1730_gain_multiplier(bh1730); + return IIO_VAL_INT; + 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, + .modified = 1, + .channel2 = IIO_MOD_LIGHT_BOTH, + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), + }, + { + .type = IIO_LIGHT, + .modified = 1, + .channel2 = IIO_MOD_LIGHT_CLEAR, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + }, + { + .type = IIO_LIGHT, + .modified = 1, + .channel2 = IIO_MOD_LIGHT_IR, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + }, +}; + +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); +} + +#ifdef CONFIG_OF +static const struct of_device_id of_bh1730_match[] = { + { .compatible = "rohm,bh1730fvc" }, + {}, +}; +MODULE_DEVICE_TABLE(of, of_bh1730_match); +#endif + +static struct i2c_driver bh1730_driver = { + .probe_new = bh1730_probe, + .remove = bh1730_remove, + .driver = { + .name = "bh1730", + .of_match_table = of_match_ptr(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.1.291.g4437f3f132-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] dt-bindings: iio: light: Add bh1730fvc bindings 2018-02-21 19:45 ` [PATCH v2 1/2] " Pierre Bourdon @ 2018-02-21 19:45 ` Pierre Bourdon 2018-02-21 21:57 ` [PATCH v2 1/2] iio: light: add driver for bh1730fvc chips Andy Shevchenko 2018-02-24 12:53 ` Jonathan Cameron 2 siblings, 0 replies; 11+ messages in thread From: Pierre Bourdon @ 2018-02-21 19:45 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 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.1.291.g4437f3f132-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] iio: light: add driver for bh1730fvc chips 2018-02-21 19:45 ` [PATCH v2 1/2] " Pierre Bourdon 2018-02-21 19:45 ` [PATCH v2 2/2] dt-bindings: iio: light: Add bh1730fvc bindings Pierre Bourdon @ 2018-02-21 21:57 ` Andy Shevchenko 2018-02-22 14:58 ` Pierre Bourdon (delroth) 2018-02-24 12:53 ` Jonathan Cameron 2 siblings, 1 reply; 11+ messages in thread From: Andy Shevchenko @ 2018-02-21 21:57 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 21, 2018 at 9:45 PM, Pierre Bourdon <delroth@google.com> wrote: > Ambient light sensor that supports visible light and IR measurements and > configurable gain/integration time. > > Changed in v2: > * Split off DT documentation change into a separate commit. > * Use i2c's probe_new. Btw, how big the difference with existing drivers? > + default: > + return -1; Better to return -EINVAL; > + 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) > + highest *= 128; > + else > + highest = (highest * 128) / bh1730_gain_multiplier(bh1730); In both cases you multiply. Why not just highest = max(visible, ir) * 128; if (highest < USHRT_MAX) ... ? > + millilux = (u64)USEC_PER_MSEC * (visible_coef * visible - ir_coef * ir); I'm not sure I understand how time units is related to lux one. > + millilux /= bh1730_gain_multiplier(bh1730); > + millilux *= 103; > + millilux /= itime_us; > + return millilux; > +} > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*bh1730)); > + if (!indio_dev) > + return -ENOMEM; > + > + indio_dev->dev.parent = &client->dev; Strange, it's not done in IIO core... Jonathan, is it true that in case of devm_iio_device_alloc() all drivers use supplied struct device as a parent one? If so, doesn't make sense to modify IIO core to do this? > +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); Hmm... Do you still need this even with devm IIO in ->probe()? > +#ifdef CONFIG_OF > +#endif This... > + .of_match_table = of_match_ptr(of_bh1730_match), ...and of_match_ptr() now pointless. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] iio: light: add driver for bh1730fvc chips 2018-02-21 21:57 ` [PATCH v2 1/2] iio: light: add driver for bh1730fvc chips Andy Shevchenko @ 2018-02-22 14:58 ` Pierre Bourdon (delroth) 2018-02-24 12:41 ` Jonathan Cameron 0 siblings, 1 reply; 11+ messages in thread From: Pierre Bourdon (delroth) @ 2018-02-22 14:58 UTC (permalink / raw) To: Andy Shevchenko Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, Rob Herring, Mark Rutland, devicetree, Linux Kernel Mailing List, Daniel Baluta Hi Andy, Thanks for the review! Answers inline. I'll send a v3 when the two open questions are resolved. On Wed, Feb 21, 2018 at 10:57 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Feb 21, 2018 at 9:45 PM, Pierre Bourdon <delroth@google.com> wrote: >> Ambient light sensor that supports visible light and IR measurements and >> configurable gain/integration time. >> >> Changed in v2: >> * Split off DT documentation change into a separate commit. >> * Use i2c's probe_new. > > Btw, how big the difference with existing drivers? See https://lkml.org/lkml/2018/2/21/982 . In retrospect, I should have added this to the commit message. It will be there in v3. >> + 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) >> + highest *= 128; >> + else >> + highest = (highest * 128) / bh1730_gain_multiplier(bh1730); > > In both cases you multiply. > Why not just > > highest = max(visible, ir) * 128; > > if (highest < USHRT_MAX) > ... > ? You'd need < USHRT_MAX * 128 then. I refactored this a bit but differently. WDYT? if (highest == USHRT_MAX) gain = 1; else gain = bh1730_gain_multiplier(bh1730); highest = (highest * BH1730_MAX_GAIN_MULTIPLIER) / gain; >> + millilux = (u64)USEC_PER_MSEC * (visible_coef * visible - ir_coef * ir); > > I'm not sure I understand how time units is related to lux one. Now looks like this, which should match the units together better while keeping most multiplications before divisions (for accuracy). Not much better I can do here I think, for example the magic "103" is straight from the datasheet with no explanation. millilux = visible_coef * visible - ir_coef * ir; millilux = (millilux * USEC_PER_MSEC) / itime_us; millilux *= 103; millilux /= bh1730_gain_multiplier(bh1730); >> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*bh1730)); >> + if (!indio_dev) >> + return -ENOMEM; >> + > >> + indio_dev->dev.parent = &client->dev; > > Strange, it's not done in IIO core... Jonathan, is it true that in > case of devm_iio_device_alloc() all drivers use supplied struct device > as a parent one? > If so, doesn't make sense to modify IIO core to do this? It doesn't seem like IIO core is doing it at this point. Should we just leave that as-is for now and get everything fixed in a future patch? >> +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); > > Hmm... Do you still need this even with devm IIO in ->probe()? I *think* so, but that's mostly because all the other IIO light drivers that I've looked at are doing it. Can someone who knows the IIO subsystem well weigh in? Thanks! Best, -- Pierre Bourdon (delroth@) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] iio: light: add driver for bh1730fvc chips 2018-02-22 14:58 ` Pierre Bourdon (delroth) @ 2018-02-24 12:41 ` Jonathan Cameron 0 siblings, 0 replies; 11+ messages in thread From: Jonathan Cameron @ 2018-02-24 12:41 UTC (permalink / raw) To: Pierre Bourdon (delroth) 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 Thu, 22 Feb 2018 15:58:10 +0100 "Pierre Bourdon (delroth)" <delroth@google.com> wrote: > Hi Andy, > > Thanks for the review! Answers inline. I'll send a v3 when the two > open questions are resolved. > > On Wed, Feb 21, 2018 at 10:57 PM, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Wed, Feb 21, 2018 at 9:45 PM, Pierre Bourdon <delroth@google.com> wrote: > >> Ambient light sensor that supports visible light and IR measurements and > >> configurable gain/integration time. > >> > >> Changed in v2: > >> * Split off DT documentation change into a separate commit. > >> * Use i2c's probe_new. > > > > Btw, how big the difference with existing drivers? > > See https://lkml.org/lkml/2018/2/21/982 . In retrospect, I should have > added this to the commit message. It will be there in v3. > > >> + 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) > >> + highest *= 128; > >> + else > >> + highest = (highest * 128) / bh1730_gain_multiplier(bh1730); > > > > In both cases you multiply. > > Why not just > > > > highest = max(visible, ir) * 128; > > > > if (highest < USHRT_MAX) > > ... > > ? > > You'd need < USHRT_MAX * 128 then. I refactored this a bit but > differently. WDYT? > > if (highest == USHRT_MAX) > gain = 1; > else > gain = bh1730_gain_multiplier(bh1730); > > highest = (highest * BH1730_MAX_GAIN_MULTIPLIER) / gain; > > >> + millilux = (u64)USEC_PER_MSEC * (visible_coef * visible - ir_coef * ir); > > > > I'm not sure I understand how time units is related to lux one. > > Now looks like this, which should match the units together better > while keeping most multiplications before divisions (for accuracy). > Not much better I can do here I think, for example the magic "103" is > straight from the datasheet with no explanation. > > millilux = visible_coef * visible - ir_coef * ir; > millilux = (millilux * USEC_PER_MSEC) / itime_us; > millilux *= 103; > millilux /= bh1730_gain_multiplier(bh1730); > > >> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*bh1730)); > >> + if (!indio_dev) > >> + return -ENOMEM; > >> + > > > >> + indio_dev->dev.parent = &client->dev; > > > > Strange, it's not done in IIO core... Jonathan, is it true that in > > case of devm_iio_device_alloc() all drivers use supplied struct device > > as a parent one? > > If so, doesn't make sense to modify IIO core to do this? > > It doesn't seem like IIO core is doing it at this point. Should we > just leave that as-is for now and get everything fixed in a future > patch? It's an interesting point - the bit that bothers me about rolling that into the devm_iio_device_alloc is that it then means that function is doing something a little non-obvious given we don't have the relevant struct device available in the iio_device_alloc call case. However, we only have two iio_device_alloc calls left. The dummy driver one doesn't have a parent (hardly surprising as there is no hardware). The other is harder to argue as it's inside the toshiba acpi driver where reworking to make devm possible would be a major job. Hmm. Will think about it... > > >> +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); > > > > Hmm... Do you still need this even with devm IIO in ->probe()? > > I *think* so, but that's mostly because all the other IIO light > drivers that I've looked at are doing it. Can someone who knows the > IIO subsystem well weigh in? Yes, it is absolutely needed. This removes the userspace and core kernel interfaces 'before' we turn the power off. Without it there are some obvious race conditions. It's hard to query a sensor which is powered down after all. You could wrap up the power off call in devm magic, but that will probably be more complex and error prone than doing it as we have here. In cases where there is no clean up to do in order we can move to fully managed - this isn't one of those though. Jonathan > > Thanks! > Best, > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] iio: light: add driver for bh1730fvc chips 2018-02-21 19:45 ` [PATCH v2 1/2] " Pierre Bourdon 2018-02-21 19:45 ` [PATCH v2 2/2] dt-bindings: iio: light: Add bh1730fvc bindings Pierre Bourdon 2018-02-21 21:57 ` [PATCH v2 1/2] iio: light: add driver for bh1730fvc chips Andy Shevchenko @ 2018-02-24 12:53 ` Jonathan Cameron 2 siblings, 0 replies; 11+ messages in thread From: Jonathan Cameron @ 2018-02-24 12:53 UTC (permalink / raw) To: Pierre Bourdon Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio, Rob Herring, Mark Rutland, devicetree, linux-kernel, Daniel Baluta On Wed, 21 Feb 2018 20:45:25 +0100 Pierre Bourdon <delroth@google.com> wrote: > Ambient light sensor that supports visible light and IR measurements and > configurable gain/integration time. > > Changed in v2: > * Split off DT documentation change into a separate commit. > * Use i2c's probe_new. > > Signed-off-by: Pierre Bourdon <delroth@google.com> Please send new versions as a new thread - all that happens with chaining them to the previous thread is things get very very difficult to follow sometimes. It's not obvious which version particular comments apply to or which patches are the latest for review. A few questions inline but looking pretty good on the whole. Jonathan > --- > drivers/iio/light/Kconfig | 10 + > drivers/iio/light/Makefile | 1 + > drivers/iio/light/bh1730.c | 414 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 425 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..7f1c50b3c55b > --- /dev/null > +++ b/drivers/iio/light/bh1730.c > @@ -0,0 +1,414 @@ > +// 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 2800ULL > + > +#define BH1730_DEFAULT_INTEG_MS 150 > + > +enum bh1730_gain { > + BH1730_GAIN_1X = 0, > + BH1730_GAIN_2X, > + BH1730_GAIN_64X, > + BH1730_GAIN_128X, > +}; > + > +struct bh1730_data { > + struct i2c_client *client; > + enum bh1730_gain gain; > + int itime; > +}; > + > +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 -1; > + } > +} > + > +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 u64 bh1730_itime_ns(struct bh1730_data *bh1730) > +{ > + return BH1730_INTERNAL_CLOCK_NS * 964 * (256 - bh1730->itime); > +} > + > +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; > + u64 time_ns = time_ms * (u64)NSEC_PER_MSEC; > + u64 itime_step_ns = BH1730_INTERNAL_CLOCK_NS * 964; > + int itime = 256 - (int)DIV_ROUND_CLOSEST_ULL(time_ns, itime_step_ns); > + > + /* ITIME == 0 is reserved for manual integration mode. */ > + if (itime <= 0 || itime > 255) { > + dev_warn(&bh1730->client->dev, > + "integration time out of range: %dms\n", time_ms); > + return -ERANGE; > + } > + > + ret = bh1730_write(bh1730, BH1730_REG_TIMING, itime); > + if (ret < 0) > + return ret; > + > + bh1730->itime = itime; > + return 0; > +} > + > +static void bh1730_wait_for_next_measurement(struct bh1730_data *bh1730) > +{ > + ndelay(bh1730_itime_ns(bh1730) + BH1730_INTERNAL_CLOCK_NS * 714); > +} > + > +static int bh1730_adjust_gain(struct bh1730_data *bh1730) > +{ > + int visible, ir, highest, 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) > + highest *= 128; > + else > + highest = (highest * 128) / bh1730_gain_multiplier(bh1730); > + > + /* > + * 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) / 128; > + > + 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); > + } blank line here preferred and before all simple return statements like this. > + return 0; > +} > + > +static s64 bh1730_get_millilux(struct bh1730_data *bh1730) > +{ > + int visible, ir, visible_coef, ir_coef; > + u64 itime_us = bh1730_itime_ns(bh1730) / NSEC_PER_USEC; > + 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 = (u64)USEC_PER_MSEC * (visible_coef * visible - ir_coef * ir); > + millilux /= bh1730_gain_multiplier(bh1730); > + millilux *= 103; > + millilux /= itime_us; Looks like the autobuilders got this but not all platforms support simple 64 bit divides so you'll need to use do_div and friends. > + return 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 ret; > + s64 millilux; > + > + ret = bh1730_adjust_gain(bh1730); > + if (ret < 0) > + return ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_PROCESSED: > + millilux = bh1730_get_millilux(bh1730); > + if (millilux < 0) > + return millilux; > + *val = millilux / 1000; > + *val2 = (millilux % 1000) * 1000; > + return IIO_VAL_INT_PLUS_MICRO; > + case IIO_CHAN_INFO_RAW: > + switch (chan->channel2) { > + case IIO_MOD_LIGHT_CLEAR: > + ret = bh1730_read_word(bh1730, BH1730_REG_DATA0LOW); > + if (ret < 0) > + return ret; > + *val = ret; > + return IIO_VAL_INT; > + case IIO_MOD_LIGHT_IR: > + ret = bh1730_read_word(bh1730, BH1730_REG_DATA1LOW); > + if (ret < 0) > + return ret; > + *val = ret; > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > + case IIO_CHAN_INFO_SCALE: > + *val = bh1730_gain_multiplier(bh1730); As I read this, we get an instantaneous value of an auto adjusting gain. Given that won't necessarily correlate with any of the values, this doesn't work from the point of view of a user interface. If you are going to do auto gain adjustment (which is fine) then it needs to be totally transparent to userspace to avoid issues like this. Here I think that means we need to not expose scale but instead apply it a relevant to the raw values. > + return IIO_VAL_INT; > + 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, > + .modified = 1, > + .channel2 = IIO_MOD_LIGHT_BOTH, > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), > + }, > + { > + .type = IIO_LIGHT, Hmm. When we have a split sensor like this it is usually impossible to relate the particular light sensor to the units of 'light' - i.e. lux in IIO. Hence we use the intensity type for the separate fields. I'm guessing you copied this from another driver though which probably means something go through when we weren't paying attention... > + .modified = 1, > + .channel2 = IIO_MOD_LIGHT_CLEAR, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE), What are units of this and the IR sensor below? > + }, > + { > + .type = IIO_LIGHT, > + .modified = 1, > + .channel2 = IIO_MOD_LIGHT_IR, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE), > + }, > +}; > + > +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); > +} > + > +#ifdef CONFIG_OF > +static const struct of_device_id of_bh1730_match[] = { > + { .compatible = "rohm,bh1730fvc" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, of_bh1730_match); > +#endif > + > +static struct i2c_driver bh1730_driver = { > + .probe_new = bh1730_probe, > + .remove = bh1730_remove, > + .driver = { > + .name = "bh1730", > + .of_match_table = of_match_ptr(of_bh1730_match), > + }, > +}; > +module_i2c_driver(bh1730_driver); > + > +MODULE_AUTHOR("Pierre Bourdon <delroth@google.com>"); > +MODULE_DESCRIPTION("ROHM BH1730FVC driver"); > +MODULE_LICENSE("GPL v2"); ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] iio: light: add driver for bh1730fvc chips 2018-02-21 12:55 [PATCH] iio: light: add driver for bh1730fvc chips Pierre Bourdon 2018-02-21 15:31 ` Daniel Baluta 2018-02-21 19:45 ` [PATCH v2 1/2] " Pierre Bourdon @ 2018-02-23 7:56 ` kbuild test robot 2 siblings, 0 replies; 11+ messages in thread From: kbuild test robot @ 2018-02-23 7:56 UTC (permalink / raw) To: Pierre Bourdon Cc: kbuild-all, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Pierre Bourdon, linux-iio, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1200 bytes --] Hi Pierre, Thank you for the patch! Yet something to improve: [auto build test ERROR on iio/togreg] [also build test ERROR on v4.16-rc2 next-20180222] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Pierre-Bourdon/iio-light-add-driver-for-bh1730fvc-chips/20180222-064336 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg config: i386-allyesconfig (attached as .config) compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): drivers/iio/light/bh1730.o: In function `bh1730_read_raw': >> bh1730.c:(.text+0x31d): undefined reference to `__udivdi3' bh1730.c:(.text+0x37d): undefined reference to `__udivdi3' bh1730.c:(.text+0x38a): undefined reference to `__udivdi3' >> bh1730.c:(.text+0x3a4): undefined reference to `__divdi3' >> bh1730.c:(.text+0x3b8): undefined reference to `__moddi3' --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 61829 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-02-24 12:53 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-21 12:55 [PATCH] iio: light: add driver for bh1730fvc chips Pierre Bourdon 2018-02-21 15:31 ` Daniel Baluta 2018-02-21 16:15 ` Pierre Bourdon (delroth) 2018-02-21 18:32 ` Daniel Baluta 2018-02-21 19:45 ` [PATCH v2 1/2] " Pierre Bourdon 2018-02-21 19:45 ` [PATCH v2 2/2] dt-bindings: iio: light: Add bh1730fvc bindings Pierre Bourdon 2018-02-21 21:57 ` [PATCH v2 1/2] iio: light: add driver for bh1730fvc chips Andy Shevchenko 2018-02-22 14:58 ` Pierre Bourdon (delroth) 2018-02-24 12:41 ` Jonathan Cameron 2018-02-24 12:53 ` Jonathan Cameron 2018-02-23 7:56 ` [PATCH] " kbuild test robot
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).