Re: [PATCH v2 1/2] iio: light: add driver for bh1730fvc chips

[Jonathan Cameron]

On Wed, 21 Feb 2018 20:45:25 +0100
Pierre Bourdon <delroth@xxxxxxxxxx> 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@xxxxxxxxxx>
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@xxxxxxxxxx>
> + *
> + * Based on a previous non-iio BH1730FVC driver:
> + * Copyright (C) 2012 Samsung Electronics. All rights reserved.
> + * Author: Won Huh <won.huh@xxxxxxxxxxx>
> + *
> + * 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@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("ROHM BH1730FVC driver");
> +MODULE_LICENSE("GPL v2");