Message-ID: <98c87420-e88a-43ca-a8af-2fa751b85d4f@gmail.com>
Date: Mon, 1 Jul 2024 14:32:52 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Mudit Sharma <muditsharma.info@...il.com>, jic23@...nel.org,
 lars@...afoo.de, krzk+dt@...nel.org, conor+dt@...nel.org, robh@...nel.org
Cc: linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, Ivan Orlov <ivan.orlov0322@...il.com>,
 Javier Carrasco <javier.carrasco.cruz@...il.com>
Subject: Re: [PATCH v6 2/2] iio: light: ROHM BH1745 colour sensor

On 6/26/24 01:03, Mudit Sharma wrote:
> Add support for BH1745, which is an I2C colour sensor with red, green,
> blue and clear channels. It has a programmable active low interrupt
> pin. Interrupt occurs when the signal from the selected interrupt
> source channel crosses set interrupt threshold high or low level.
> 
> Interrupt source for the device can be configured by enabling the
> corresponding event. Interrupt latch is always enabled when setting
> up interrupt.
> 
> Add myself as the maintainer for this driver in MAINTAINERS.
> 
> Signed-off-by: Mudit Sharma <muditsharma.info@...il.com>
> Reviewed-by: Ivan Orlov <ivan.orlov0322@...il.com>
> Reviewed-by: Javier Carrasco <javier.carrasco.cruz@...il.com>
> ---
> v5->v6:
> - Fix typo
> - Fix Indentation
> - Drop read in bh1745_set_trigger_state() as configuring all the value
> v4->v5:
> - Provide scale instead of HW gain
>    - Use GTS helpers
> - Code style fixes
> - Add check for part ID during probe
> - Always enable latch when enabling interrupt
> - Use devm_add_action_or_reset() and drop bh1745_remove() function
> - Drop custom DEVICE_ATTR and provide related read_avail and setup
>    interrupt source with event_config
> - Make buffer support independent of IRQ
> - Add power regulator handing with devm_regulator_get_enable()
> - Drop read and write wrappers and use regmap functions directly
> - Add MODULE_DEVICE_TABLE for of_device_id
> v3->v4:
> - Fix formatting:
>    - Remove redundant new line
>    - Use '0x' rather than '0X'
> v2->v3:
> - Squash commit for addition to MAINTAINERS
> - Fix code style for consistency:
>    - New line before last 'return'
>    - Use variable name 'value' instead of 'val' in
>      'bh1745_set_trigger_state()'
>    - Align function parameters to match parenthesis
>    - Avoid use of magic number
> - Use named enum instead of anonymous enum
> - Use 'guard(mutex)(&data->lock)' instead of 'mutex_lock()'
>    'mutex_unlock()'
> - Only initialize 'ret' and 'value' when necessary
> - Fix and optimize logic for `in_interrupt_latch_store()`
> - Fix error handling in irq , trigger handlers and dev attribute for
>    latch
> v1->v2:
> - No changes
> 
>   MAINTAINERS                |   6 +
>   drivers/iio/light/Kconfig  |  13 +
>   drivers/iio/light/Makefile |   1 +
>   drivers/iio/light/bh1745.c | 931 +++++++++++++++++++++++++++++++++++++
>   4 files changed, 951 insertions(+)
>   create mode 100644 drivers/iio/light/bh1745.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2ca8f35dfe03..e9ff6f465e7f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19404,6 +19404,12 @@ S:	Supported
>   F:	drivers/power/supply/bd99954-charger.c
>   F:	drivers/power/supply/bd99954-charger.h
>   
> +ROHM BH1745 COLOUR SENSOR
> +M:	Mudit Sharma <muditsharma.info@...il.com>
> +L:	linux-iio@...r.kernel.org
> +S:	Maintained
> +F:	drivers/iio/light/bh1745.c
> +
>   ROHM BH1750 AMBIENT LIGHT SENSOR DRIVER
>   M:	Tomasz Duszynski <tduszyns@...il.com>
>   S:	Maintained
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 9a587d403118..6cde702fa78d 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -114,6 +114,19 @@ config AS73211
>   	 This driver can also be built as a module.  If so, the module
>   	 will be called as73211.
>   
> +config BH1745
> +	tristate "ROHM BH1745 colour sensor"
> +	depends on I2C
> +	select REGMAP_I2C
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	select IIO_GTS_HELPER
> +	help
> +	  Say Y here to build support for the ROHM bh1745 colour sensor.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called bh1745.
> +
>   config BH1750
>   	tristate "ROHM BH1750 ambient light sensor"
>   	depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index a30f906e91ba..939a701a06ac 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_APDS9300)		+= apds9300.o
>   obj-$(CONFIG_APDS9306)		+= apds9306.o
>   obj-$(CONFIG_APDS9960)		+= apds9960.o
>   obj-$(CONFIG_AS73211)		+= as73211.o
> +obj-$(CONFIG_BH1745)		+= bh1745.o
>   obj-$(CONFIG_BH1750)		+= bh1750.o
>   obj-$(CONFIG_BH1780)		+= bh1780.o
>   obj-$(CONFIG_CM32181)		+= cm32181.o
> diff --git a/drivers/iio/light/bh1745.c b/drivers/iio/light/bh1745.c
> new file mode 100644
> index 000000000000..8412d5da3019
> --- /dev/null
> +++ b/drivers/iio/light/bh1745.c
> @@ -0,0 +1,931 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ROHM BH1745 digital colour sensor driver
> + *
> + * Copyright (C) Mudit Sharma <muditsharma.info@...il.com>
> + *
> + * 7-bit I2C slave addresses:
> + *  0x38 (ADDR pin low)
> + *  0x39 (ADDR pin high)
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/mutex.h>
> +#include <linux/util_macros.h>
> +#include <linux/iio/events.h>
> +#include <linux/regmap.h>
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/iio-gts-helper.h>
> +
> +/* BH1745 config regs */
> +#define BH1745_SYS_CTRL 0x40
> +
> +#define BH1745_MODE_CTRL_1 0x41
> +#define BH1745_MODE_CTRL_2 0x42
> +#define BH1745_MODE_CTRL_3 0x44
> +
> +#define BH1745_INTR 0x60
> +#define BH1745_INTR_STATUS BIT(7)
> +
> +#define BH1745_PERSISTENCE 0x61
> +
> +#define BH1745_TH_LSB 0x62
> +#define BH1745_TH_MSB 0x63
> +
> +#define BH1745_TL_LSB 0x64
> +#define BH1745_TL_MSB 0x65
> +
> +#define BH1745_MANU_ID 0x92
> +
> +/* BH1745 output regs */
> +#define BH1745_RED_LSB 0x50
> +#define BH1745_RED_MSB 0x51
> +#define BH1745_GREEN_LSB 0x52
> +#define BH1745_GREEN_MSB 0x53
> +#define BH1745_BLUE_LSB 0x54
> +#define BH1745_BLUE_MSB 0x55
> +#define BH1745_CLEAR_LSB 0x56
> +#define BH1745_CLEAR_MSB 0x57
> +
> +#define BH1745_SW_RESET BIT(7)
> +#define BH1745_INT_RESET BIT(6)
> +
> +#define BH1745_MEASUREMENT_TIME_MASK GENMASK(2, 0)
> +
> +#define BH1745_RGBC_EN BIT(4)
> +
> +#define BH1745_ADC_GAIN_MASK GENMASK(1, 0)
> +
> +#define BH1745_INT_ENABLE BIT(0)
> +#define BH1745_INT_SIGNAL_ACTIVE BIT(7)
> +
> +#define BH1745_INT_SIGNAL_LATCHED BIT(4)
> +
> +#define BH1745_INT_SOURCE_MASK GENMASK(3, 2)
> +
> +#define BH1745_PART_ID 0x0B
> +#define BH1745_PART_ID_MASK GENMASK(5, 0)
> +
> +// From 16x max HW gain and 32x max integration time
> +#define BH1745_MAX_GAIN 512
> +
> +static const int bh1745_int_time[][2] = {
> +	{ 0, 160000 }, /* 160 ms */
> +	{ 0, 320000 }, /* 320 ms */
> +	{ 0, 640000 }, /* 640 ms */
> +	{ 1, 280000 }, /* 1280 ms */
> +	{ 2, 560000 }, /* 2560 ms */
> +	{ 5, 120000 }, /* 5120 ms */
> +};
> +
> +static const u8 bh1745_gain_factor[] = { 1, 2, 16 };
> +
> +static const int bh1745_int_time_us[] = { 160000,  320000,  640000,
> +					  1280000, 2560000, 5120000 };

I am not sure why you need these tables above? Can't the iio_gts do all 
the conversions from register-value to int time/gain and int-time/gain 
to register value, as well as the checks for supported values? Ideally, 
you would not need anything else but the bh1745_itimes and the 
bh1745_gain tables below - they should contain all the same information.

> +
> +enum bh1745_int_source {
> +	BH1745_INT_SOURCE_RED,
> +	BH1745_INT_SOURCE_GREEN,
> +	BH1745_INT_SOURCE_BLUE,
> +	BH1745_INT_SOURCE_CLEAR,
> +};
> +
> +enum bh1745_gain {
> +	BH1745_ADC_GAIN_1X,
> +	BH1745_ADC_GAIN_2X,
> +	BH1745_ADC_GAIN_16X,
> +};
> +
> +enum bh1745_measurement_time {
> +	BH1745_MEASUREMENT_TIME_160MS,
> +	BH1745_MEASUREMENT_TIME_320MS,
> +	BH1745_MEASUREMENT_TIME_640MS,
> +	BH1745_MEASUREMENT_TIME_1280MS,
> +	BH1745_MEASUREMENT_TIME_2560MS,
> +	BH1745_MEASUREMENT_TIME_5120MS,
> +};
> +
> +enum bh1745_presistence_value {
> +	BH1745_PRESISTENCE_UPDATE_TOGGLE,
> +	BH1745_PRESISTENCE_UPDATE_EACH_MEASUREMENT,
> +	BH1745_PRESISTENCE_UPDATE_FOUR_MEASUREMENT,
> +	BH1745_PRESISTENCE_UPDATE_EIGHT_MEASUREMENT,
> +};
> +
> +static const struct iio_gain_sel_pair bh1745_gain[] = {
> +	GAIN_SCALE_GAIN(1, BH1745_ADC_GAIN_1X),
> +	GAIN_SCALE_GAIN(2, BH1745_ADC_GAIN_2X),
> +	GAIN_SCALE_GAIN(16, BH1745_ADC_GAIN_16X),
> +};
> +
> +static const struct iio_itime_sel_mul bh1745_itimes[] = {
> +	GAIN_SCALE_ITIME_US(5120000, BH1745_MEASUREMENT_TIME_5120MS, 32),
> +	GAIN_SCALE_ITIME_US(2560000, BH1745_MEASUREMENT_TIME_2560MS, 16),
> +	GAIN_SCALE_ITIME_US(1280000, BH1745_MEASUREMENT_TIME_1280MS, 8),
> +	GAIN_SCALE_ITIME_US(640000, BH1745_MEASUREMENT_TIME_640MS, 4),
> +	GAIN_SCALE_ITIME_US(320000, BH1745_MEASUREMENT_TIME_320MS, 2),
> +	GAIN_SCALE_ITIME_US(160000, BH1745_MEASUREMENT_TIME_160MS, 1),
> +};
> +
> +struct bh1745_data {
> +	/*
> +	 * Lock to prevent device setting update or read before related
> +	 * calculations or event push are completed
> +	 */
> +	struct mutex lock;
> +	struct regmap *regmap;
> +	struct i2c_client *client;
> +	struct iio_trigger *trig;
> +	struct iio_gts gts;
> +	u8 int_src;
> +};
> +
> +static const struct regmap_range bh1745_volatile_ranges[] = {
> +	regmap_reg_range(BH1745_MODE_CTRL_2, BH1745_MODE_CTRL_2), /* VALID */
> +	regmap_reg_range(BH1745_RED_LSB, BH1745_CLEAR_MSB), /* Data */
> +	regmap_reg_range(BH1745_INTR, BH1745_INTR), /* Interrupt */
> +};
> +
> +static const struct regmap_access_table bh1745_volatile_regs = {
> +	.yes_ranges = bh1745_volatile_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(bh1745_volatile_ranges),
> +};
> +
> +static const struct regmap_range bh1745_read_ranges[] = {
> +	regmap_reg_range(BH1745_SYS_CTRL, BH1745_MODE_CTRL_2),
> +	regmap_reg_range(BH1745_RED_LSB, BH1745_CLEAR_MSB),
> +	regmap_reg_range(BH1745_INTR, BH1745_INTR),
> +	regmap_reg_range(BH1745_PERSISTENCE, BH1745_TL_MSB),
> +	regmap_reg_range(BH1745_MANU_ID, BH1745_MANU_ID),
> +};
> +
> +static const struct regmap_access_table bh1745_ro_regs = {
> +	.yes_ranges = bh1745_read_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(bh1745_read_ranges),
> +};
> +
> +static const struct regmap_range bh1745_writable_ranges[] = {
> +	regmap_reg_range(BH1745_SYS_CTRL, BH1745_MODE_CTRL_2),
> +	regmap_reg_range(BH1745_INTR, BH1745_INTR),
> +	regmap_reg_range(BH1745_PERSISTENCE, BH1745_TL_MSB),
> +};
> +
> +static const struct regmap_access_table bh1745_wr_regs = {
> +	.yes_ranges = bh1745_writable_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(bh1745_writable_ranges),
> +};
> +
> +static const struct regmap_config bh1745_regmap = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = BH1745_MANU_ID,
> +	.cache_type = REGCACHE_RBTREE,
> +	.volatile_table = &bh1745_volatile_regs,
> +	.wr_table = &bh1745_wr_regs,
> +	.rd_table = &bh1745_ro_regs,

I am not 100% sure what this does. (Let's say it is just my ignorance 
:)). Does the 'ro' in 'bh1745_ro_regs' stand for read-only?

If so, shouldn't the read-inly registers be marked as "not writable", 
which would be adding them in .wr_table in 'no_ranges'? Also, what is 
the idea of the 'wr_regs'?

> +};
> +
> +static const struct iio_event_spec bh1745_event_spec[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_EITHER,
> +		.mask_shared_by_type = BIT(IIO_EV_INFO_PERIOD),
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +	},
> +};
> +
> +#define BH1745_CHANNEL(_colour, _si, _addr)                             \
> +	{                                                               \
> +		.type = IIO_INTENSITY, .modified = 1,                   \
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),           \
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE) |   \
> +					   BIT(IIO_CHAN_INFO_INT_TIME), \
> +		.info_mask_shared_by_all_available =                    \
> +			BIT(IIO_CHAN_INFO_SCALE) |                      \
> +			BIT(IIO_CHAN_INFO_INT_TIME),                    \
> +		.event_spec = bh1745_event_spec,                        \
> +		.num_event_specs = ARRAY_SIZE(bh1745_event_spec),       \
> +		.channel2 = IIO_MOD_LIGHT_##_colour, .address = _addr,  \
> +		.scan_index = _si,                                      \
> +		.scan_type = {                                          \
> +			.sign = 'u',                                    \
> +			.realbits = 16,                                 \
> +			.storagebits = 16,                              \
> +			.endianness = IIO_CPU,                          \
> +		},                                                      \
> +	}
> +
> +static const struct iio_chan_spec bh1745_channels[] = {
> +	BH1745_CHANNEL(RED, 0, BH1745_RED_LSB),
> +	BH1745_CHANNEL(GREEN, 1, BH1745_GREEN_LSB),
> +	BH1745_CHANNEL(BLUE, 2, BH1745_BLUE_LSB),
> +	BH1745_CHANNEL(CLEAR, 3, BH1745_CLEAR_LSB),
> +	IIO_CHAN_SOFT_TIMESTAMP(4),
> +};
> +
> +static int bh1745_reset(struct bh1745_data *data)
> +{
> +	int ret;
> +	int value;
> +
> +	ret = regmap_read(data->regmap, BH1745_SYS_CTRL, &value);
> +	if (ret)
> +		return ret;
> +
> +	value |= (BH1745_SW_RESET | BH1745_INT_RESET);
> +
> +	return regmap_write(data->regmap, BH1745_SYS_CTRL, value);

Would it work if you used regmap_write_bits() instead?

... Sorry, my reviewing time is out :/ I may continue later but no need 
to wait for my comments if I am not responding. I've too much stuff 
piling on :(


Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Powered by blists - more mailing lists