* [PATCH 2/2] iio: light: bh1730: Add bh1730 light sensor driver
@ 2026-05-10 18:09 ` Alexandre Hamamdjian via B4 Relay
0 siblings, 0 replies; 21+ messages in thread
From: Alexandre Hamamdjian via B4 Relay @ 2026-05-10 18:09 UTC (permalink / raw)
To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, CTCaer
Cc: linux-iio, devicetree, linux-kernel, Alexandre Hamamdjian
From: CTCaer <ctcaer@gmail.com>
Add a driver for the ROHM BH1730FVC ambient light sensor. The device
is a 16-bit I2C digital sensor with separate visible and infrared
photodiodes, four selectable gains (1x/2x/64x/128x) and a programmable
integration time.
The driver exposes illuminance via IIO, performs runtime gain and
integration-time tracking to keep the ADC in range, and supports
optional als-vdd / als-vid regulators. Per-board lux calibration data
(integration cycles, lux multiplier, optical-window coefficients, and
gain sensitivity coefficients) can be supplied via device tree;
sensible defaults are used otherwise.
Signed-off-by: CTCaer <ctcaer@gmail.com>
Signed-off-by: Alexandre Hamamdjian <azkali.limited@gmail.com>
---
drivers/iio/light/Kconfig | 9 +
drivers/iio/light/Makefile | 1 +
drivers/iio/light/bh1730.c | 686 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 696 insertions(+)
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index eff33e456c70..3869060567dd 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -135,6 +135,15 @@ config AS73211
This driver can also be built as a module. If so, the module
will be called as73211.
+config BH1730
+ tristate "ROHM BH1730 ambient light sensor"
+ depends on I2C
+ help
+ Say Y here to build support for the ROHM BH1730 ambient light sensor.
+
+ To compile this driver as a module, choose M here: the module will
+ be called bh1730.
+
config BH1745
tristate "ROHM BH1745 colour sensor"
depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index c0048e0d5ca8..27ef7906a02a 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_APDS9300) += apds9300.o
obj-$(CONFIG_APDS9306) += apds9306.o
obj-$(CONFIG_APDS9960) += apds9960.o
obj-$(CONFIG_AS73211) += as73211.o
+obj-$(CONFIG_BH1730) += bh1730.o
obj-$(CONFIG_BH1745) += bh1745.o
obj-$(CONFIG_BH1750) += bh1750.o
obj-$(CONFIG_BH1780) += bh1780.o
diff --git a/drivers/iio/light/bh1730.c b/drivers/iio/light/bh1730.c
new file mode 100644
index 000000000000..c93290ff5661
--- /dev/null
+++ b/drivers/iio/light/bh1730.c
@@ -0,0 +1,686 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ROHM BH1730 ambient light sensor driver
+ *
+ * Copyright (c) 2021 CTCaer <ctcaer@gmail.com>
+ *
+ * Based on previous iio BH1730FVC drivers from:
+ * Copyright (c) 2018 Google, Inc.
+ * Author: Pierre Bourdon <delroth@google.com>
+ *
+ * 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>
+#include <linux/regulator/consumer.h>
+
+#define BH1730_NAME "bh1730fvc"
+
+#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 BIT(0)
+#define BH1730_CONTROL_ADC_EN BIT(1)
+#define BH1730_CONTROL_DATA0_ONLY BIT(2)
+#define BH1730_CONTROL_ONE_TIME BIT(3)
+#define BH1730_CONTROL_ADC_VALID BIT(4)
+#define BH1730_CONTROL_INTR BIT(5)
+
+#define BH1730_INTERNAL_CLOCK_NS 2800
+/* BH1730_INTERNAL_CLOCK_MS * 714 */
+#define BH1730_ADC_CALC_DELAY_US 2000
+/* BH1730_INTERNAL_CLOCK_MS * 964 */
+#define BH1730_ITIME_TO_US 2700
+
+#define BH1730_DEFAULT_INTEG_CYCLE 38
+#define BH1730_DEFAULT_ITIME_MS 100
+
+#define BH1730_POWER_ON_DELAY_US 10000
+
+#define BH1730_MAX_MEASURED_LUX 100000
+
+enum bh1730_gain {
+ BH1730_GAIN_1X = 0,
+ BH1730_GAIN_2X,
+ BH1730_GAIN_64X,
+ BH1730_GAIN_128X,
+};
+#define BH1730_MAX_GAIN_MULTIPLIER 128
+
+struct gain_coeff_t {
+ u32 cl;
+ u32 fl;
+};
+
+struct opt_win_coeff_t {
+ u32 rc;
+ u32 cv;
+ u32 ci;
+};
+
+struct lux_cal_data_t {
+ struct gain_coeff_t *gain_coeff;
+ struct opt_win_coeff_t *opt_win_coeff;
+ u32 opt_win_coeff_count;
+ u32 itime_cycle;
+ u32 mul;
+};
+
+/*
+ * No optical window or optical window that has flat transmittance
+ * from visible light to infrared light.
+ */
+static struct opt_win_coeff_t def_lux_coeff[] = {
+ { 260, 1290, 2733 },
+ { 550, 795, 859 },
+ { 1090, 510, 345 },
+ { 2130, 276, 130 }
+};
+
+static struct gain_coeff_t def_gain_coeff[] = {
+ { 3000, -1 }, /* 1x */
+ { 2000, 9800 }, /* 2x */
+ { 15, 60000 }, /* 64x */
+ { 0, 1300 } /* 128x */
+};
+
+static struct lux_cal_data_t def_lux_data = {
+ .gain_coeff = def_gain_coeff,
+ .opt_win_coeff = def_lux_coeff,
+ .opt_win_coeff_count = ARRAY_SIZE(def_lux_coeff),
+ .itime_cycle = BH1730_DEFAULT_INTEG_CYCLE,
+ .mul = 1000
+};
+
+struct bh1730_data {
+ struct i2c_client *client;
+ struct lux_cal_data_t cal;
+ struct regulator *reg_vdd;
+ struct regulator *reg_vid;
+ enum bh1730_gain gain;
+ u32 itime_us;
+ u32 tmt_us;
+};
+
+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 bh1730_gain_multiplier(struct bh1730_data *bh1730)
+{
+ int multiplier;
+
+ switch (bh1730->gain) {
+ case BH1730_GAIN_1X:
+ multiplier = 1;
+ break;
+ case BH1730_GAIN_2X:
+ multiplier = 2;
+ break;
+ case BH1730_GAIN_64X:
+ multiplier = 64;
+ break;
+ case BH1730_GAIN_128X:
+ multiplier = 128;
+ break;
+ default:
+ multiplier = -EINVAL;
+ break;
+ }
+
+ if (multiplier < 0) {
+ dev_warn(&bh1730->client->dev,
+ "invalid gain multiplier settings: %d\n",
+ bh1730->gain);
+ bh1730->gain = BH1730_GAIN_1X;
+ multiplier = 1;
+ }
+
+ return multiplier;
+}
+
+static int bh1730_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_itime_us(struct bh1730_data *bh1730, int cycle)
+{
+ return (BH1730_ITIME_TO_US * cycle);
+}
+
+
+static int bh1730_set_integration_time_cycle(struct bh1730_data *bh1730,
+ u32 cycle)
+{
+ int ret, itime;
+
+ itime = 256 - cycle;
+
+ /* ITIME == 0 is reserved for manual integration mode. */
+ if (itime <= 0 || itime > 255) {
+ dev_warn(&bh1730->client->dev,
+ "integration time out of range: %d cycles\n",
+ cycle);
+
+ return -ERANGE;
+ }
+
+ ret = bh1730_write(bh1730, BH1730_REG_TIMING, itime);
+ if (ret < 0)
+ return ret;
+
+ bh1730->itime_us = bh1730_itime_us(bh1730, cycle);
+ bh1730->tmt_us = bh1730->itime_us + BH1730_ADC_CALC_DELAY_US;
+
+ return 0;
+}
+
+static int bh1730_adjust_gain(struct bh1730_data *bh1730)
+{
+ int visible, ir, highest, gain, ret;
+
+ visible = bh1730_read_word(bh1730, BH1730_REG_DATA0LOW);
+ if (visible < 0)
+ return visible;
+
+ ir = bh1730_read_word(bh1730, BH1730_REG_DATA1LOW);
+ if (ir < 0)
+ return ir;
+
+ highest = max(visible, ir);
+
+ /* Adjust gain based on sensitivity calibration */
+ gain = bh1730->gain;
+ if (highest > bh1730->cal.gain_coeff[gain].fl &&
+ gain != BH1730_GAIN_1X) {
+ gain--; /* Decrease sensitivity */
+ } else if (highest < bh1730->cal.gain_coeff[gain].cl &&
+ gain != BH1730_GAIN_128X) {
+ gain++; /* Increase sensitivity */
+ }
+
+ /* Clamp to proper gain values */
+ if (gain < 0)
+ gain = BH1730_GAIN_1X;
+ else if (gain > BH1730_GAIN_128X)
+ gain = BH1730_GAIN_128X;
+
+ if (gain != bh1730->gain) {
+ ret = bh1730_set_gain(bh1730, gain);
+ if (ret < 0)
+ return ret;
+
+ usleep_range(bh1730->tmt_us, bh1730->tmt_us + 1000);
+ }
+
+ return 0;
+}
+
+static int bh1730_get_lux(struct bh1730_data *bh1730)
+{
+ int i, visible, ir;
+ struct opt_win_coeff_t *opt_win_coeff;
+ u64 lux = 0;
+
+ visible = bh1730_read_word(bh1730, BH1730_REG_DATA0LOW);
+
+ /* If visible is 0, skip calculations */
+ if (visible <= 0)
+ return visible;
+
+ ir = bh1730_read_word(bh1730, BH1730_REG_DATA1LOW);
+ if (ir < 0)
+ return ir;
+
+ /* Calibrate based on optical window */
+ for (i = 0; i < bh1730->cal.opt_win_coeff_count; i++) {
+ opt_win_coeff = &bh1730->cal.opt_win_coeff[i];
+ if (1000 * ir / visible < opt_win_coeff->rc) {
+ lux = ((u64)opt_win_coeff->cv * visible) -
+ (opt_win_coeff->ci * ir);
+ break;
+ }
+ }
+
+ /* Calculate lux */
+ lux *= BH1730_DEFAULT_ITIME_MS;
+ lux /= bh1730_gain_multiplier(bh1730) * bh1730->itime_us;
+ lux = (lux * bh1730->cal.mul) / 1000;
+
+ if (lux > BH1730_MAX_MEASURED_LUX)
+ lux = BH1730_MAX_MEASURED_LUX;
+
+ return (int)lux;
+}
+
+static int bh1730_power_ctrl(struct bh1730_data *bh1730, bool enable)
+{
+ struct device *dev = &bh1730->client->dev;
+ static bool enabled;
+ int ret = 0;
+
+ if (enabled == enable)
+ return ret;
+
+ if (enable) {
+ if (!IS_ERR_OR_NULL(bh1730->reg_vdd)) {
+ ret = regulator_enable(bh1730->reg_vdd);
+ if (ret) {
+ dev_err(dev, "%s: Failed to enable vdd: %d\n",
+ __func__, ret);
+ return ret;
+ }
+ }
+
+ if (!IS_ERR_OR_NULL(bh1730->reg_vid)) {
+ ret = regulator_enable(bh1730->reg_vid);
+ if (ret) {
+ dev_err(dev, "%s: Failed to enable vid: %d\n",
+ __func__, ret);
+ return ret;
+ }
+ }
+
+ usleep_range(BH1730_POWER_ON_DELAY_US, BH1730_POWER_ON_DELAY_US + 1000);
+ } else {
+ if (!IS_ERR_OR_NULL(bh1730->reg_vdd)) {
+ ret = regulator_disable(bh1730->reg_vdd);
+ if (ret) {
+ dev_err(dev, "%s: Failed to disable vdd: %d\n",
+ __func__, ret);
+ return ret;
+ }
+ }
+
+ if (!IS_ERR_OR_NULL(bh1730->reg_vid)) {
+ ret = regulator_disable(bh1730->reg_vid);
+ if (ret) {
+ dev_err(dev, "%s: Failed to disable vid: %d\n",
+ __func__, ret);
+ return ret;
+ }
+ }
+ }
+
+ enabled = enable;
+
+ return ret;
+}
+
+static int bh1730_power_on(struct bh1730_data *bh1730)
+{
+ int ret = bh1730_power_ctrl(bh1730, true);
+
+ if (ret < 0)
+ return ret;
+
+ return bh1730_write(bh1730, BH1730_REG_CONTROL,
+ BH1730_CONTROL_POWER | BH1730_CONTROL_ADC_EN);
+}
+
+static int bh1730_init_config(struct bh1730_data *bh1730)
+{
+ int ret;
+
+ ret = bh1730_set_gain(bh1730, BH1730_GAIN_64X);
+ if (ret < 0)
+ return ret;
+
+ return bh1730_set_integration_time_cycle(bh1730,
+ bh1730->cal.itime_cycle);
+}
+
+static int bh1730_power_off(struct bh1730_data *bh1730)
+{
+ int ret = bh1730_write(bh1730, BH1730_REG_CONTROL, 0);
+
+ if (ret < 0)
+ return ret;
+
+ return bh1730_power_ctrl(bh1730, false);
+}
+
+static int bh1730_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct bh1730_data *bh1730 = iio_priv(indio_dev);
+ int data_reg, ret;
+
+ ret = bh1730_adjust_gain(bh1730);
+ if (ret < 0)
+ return ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_PROCESSED:
+ ret = bh1730_get_lux(bh1730);
+ if (ret < 0)
+ return ret;
+ *val = ret;
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_RAW:
+ switch (chan->channel2) {
+ case IIO_MOD_LIGHT_CLEAR:
+ data_reg = BH1730_REG_DATA0LOW;
+ break;
+ case IIO_MOD_LIGHT_IR:
+ data_reg = BH1730_REG_DATA1LOW;
+ break;
+ default:
+ return -EINVAL;
+ }
+ ret = bh1730_read_word(bh1730, data_reg);
+ if (ret < 0)
+ return ret;
+ ret = ret * 1000 / bh1730_gain_multiplier(bh1730);
+ *val = ret / 1000;
+ *val2 = (ret % 1000) * 1000;
+ return IIO_VAL_INT_PLUS_MICRO;
+ case IIO_CHAN_INFO_INT_TIME:
+ *val = 0;
+ *val2 = bh1730->itime_us;
+ return IIO_VAL_INT_PLUS_MICRO;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int bh1730_parse_dt(struct bh1730_data *bh1730, struct device_node *dn)
+{
+ struct device *dev = &bh1730->client->dev;
+ int ret;
+ u32 *opt_win_coeff = NULL;
+ u32 *gain_coeff = NULL;
+ int opt_win_coeff_count = 0, gain_coeff_count = 0, cycle = 0, mul = 0;
+
+ memcpy(&bh1730->cal, &def_lux_data, sizeof(struct lux_cal_data_t));
+
+ if (dn) {
+ /* Get regulators */
+ bh1730->reg_vdd = regulator_get(dev, "als-vdd");
+ if (IS_ERR_OR_NULL(bh1730->reg_vdd)) {
+ bh1730->reg_vdd = NULL;
+ dev_warn(dev, "failed to get als-vdd");
+ }
+
+ bh1730->reg_vid = regulator_get(dev, "als-vid");
+ if (IS_ERR_OR_NULL(bh1730->reg_vid)) {
+ bh1730->reg_vid = NULL;
+ dev_warn(dev, "failed to get als-vid");
+ }
+
+ /* Get calibration */
+ ret = of_property_read_u32(dn,
+ "rohm,integration-cycle", &cycle);
+ if (ret < 0)
+ goto out;
+
+ ret = of_property_read_u32(dn,
+ "rohm,lux-multiplier", &mul);
+ if (ret < 0)
+ goto out;
+
+ if (cycle == 0 || mul == 0)
+ goto out;
+
+ /* Get optical window coefficients */
+ opt_win_coeff_count = of_property_count_elems_of_size(dn,
+ "rohm,opt-win-coeff", sizeof(u32));
+ if (opt_win_coeff_count > 0) {
+ opt_win_coeff = devm_kzalloc(dev,
+ sizeof(u32) * opt_win_coeff_count,
+ GFP_KERNEL);
+ if (!opt_win_coeff) {
+ dev_err(dev, "failed to allocate mem for opt_win_coeff");
+ return -ENOMEM;
+ }
+
+ ret = of_property_read_u32_array(dn, "rohm,opt-win-coeff",
+ opt_win_coeff, opt_win_coeff_count);
+
+ if (ret) {
+ devm_kfree(dev, opt_win_coeff);
+ goto out;
+ }
+ }
+
+ /* Get gain sensitivity coefficients */
+ gain_coeff_count = of_property_count_elems_of_size(dn,
+ "rohm,gain-coeff", sizeof(u32));
+
+ if (gain_coeff_count == 8) { /* 2 for each gain supported */
+ gain_coeff = devm_kzalloc(dev,
+ sizeof(u32) * gain_coeff_count,
+ GFP_KERNEL);
+ if (!gain_coeff) {
+ dev_err(dev, "failed to allocate mem for gain_coeff");
+ return -ENOMEM;
+ }
+
+ ret = of_property_read_u32_array(dn, "rohm,gain-coeff",
+ gain_coeff, gain_coeff_count);
+ if (ret) {
+ if (opt_win_coeff)
+ devm_kfree(dev, opt_win_coeff);
+ devm_kfree(dev, gain_coeff);
+ goto out;
+ }
+ }
+
+ if (opt_win_coeff) {
+ bh1730->cal.opt_win_coeff =
+ (struct opt_win_coeff_t *)opt_win_coeff;
+ bh1730->cal.opt_win_coeff_count =
+ opt_win_coeff_count /
+ (sizeof(struct opt_win_coeff_t) /
+ sizeof(u32));
+ }
+
+ if (gain_coeff)
+ bh1730->cal.gain_coeff = (struct gain_coeff_t *)gain_coeff;
+
+ bh1730->cal.itime_cycle = cycle;
+ bh1730->cal.mul = mul;
+
+ return 0;
+ }
+
+out:
+ dev_info(&bh1730->client->dev, "using default calibration");
+
+ return 0;
+}
+
+static const struct iio_info bh1730_info = {
+ .read_raw = bh1730_read_raw,
+};
+
+static const struct iio_chan_spec bh1730_channels[] = {
+ {
+ .type = IIO_LIGHT,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+ BIT(IIO_CHAN_INFO_INT_TIME),
+ },
+ {
+ .type = IIO_INTENSITY,
+ .modified = 1,
+ .channel2 = IIO_MOD_LIGHT_CLEAR,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ },
+ {
+ .type = IIO_INTENSITY,
+ .modified = 1,
+ .channel2 = IIO_MOD_LIGHT_IR,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ },
+};
+
+static int bh1730_probe(struct i2c_client *client)
+{
+ struct bh1730_data *bh1730;
+ struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
+ struct iio_dev *indio_dev;
+ int ret;
+
+ if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
+ return -EIO;
+
+ indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*bh1730));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ bh1730 = iio_priv(indio_dev);
+ bh1730->client = client;
+ i2c_set_clientdata(client, indio_dev);
+
+ ret = bh1730_parse_dt(bh1730, client->dev.of_node);
+ if (ret < 0)
+ return ret;
+
+ ret = bh1730_power_on(bh1730);
+ if (ret < 0)
+ return ret;
+
+ ret = bh1730_init_config(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:
+ dev_info(&client->dev, "%s: failed\n", __func__);
+ bh1730_power_off(bh1730);
+ return ret;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int bh1730_suspend(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct iio_dev *indio_dev = i2c_get_clientdata(client);
+ struct bh1730_data *bh1730 = iio_priv(indio_dev);
+
+ bh1730_power_off(bh1730);
+
+ return 0;
+}
+
+static int bh1730_resume(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct iio_dev *indio_dev = i2c_get_clientdata(client);
+ struct bh1730_data *bh1730 = iio_priv(indio_dev);
+ int ret;
+
+ ret = bh1730_power_on(bh1730);
+ if (ret < 0)
+ return ret;
+
+ return bh1730_init_config(bh1730);
+}
+
+static SIMPLE_DEV_PM_OPS(bh1730_pm_ops, bh1730_suspend, bh1730_resume);
+#endif
+
+static void 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);
+ bh1730_power_off(bh1730);
+}
+
+static const struct i2c_device_id bh1730_i2c_device_id[] = {
+ { BH1730_NAME, 0 },
+ {}
+};
+
+MODULE_DEVICE_TABLE(i2c, bh1730_i2c_device_id);
+
+static const struct of_device_id of_bh1730_match[] = {
+ { .compatible = "rohm,bh1730fvc" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, of_bh1730_match);
+
+static struct i2c_driver bh1730_driver = {
+ .probe = bh1730_probe,
+ .remove = bh1730_remove,
+ .driver = {
+ .name = BH1730_NAME,
+ .owner = THIS_MODULE,
+ .of_match_table = of_bh1730_match,
+#ifdef CONFIG_PM_SLEEP
+ .pm = &bh1730_pm_ops,
+#endif
+ },
+ .id_table = bh1730_i2c_device_id,
+};
+module_i2c_driver(bh1730_driver);
+
+MODULE_AUTHOR("CTCaer <ctcaer@gmail.com>");
+MODULE_DESCRIPTION("ROHM BH1730FVC driver");
+MODULE_LICENSE("GPL v2");
--
2.54.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] iio: light: bh1730: Add bh1730 light sensor driver
2026-05-10 18:09 ` Alexandre Hamamdjian via B4 Relay
(?)
@ 2026-05-10 18:18 ` Andy Shevchenko
2026-05-10 18:20 ` Andy Shevchenko
-1 siblings, 1 reply; 21+ messages in thread
From: Andy Shevchenko @ 2026-05-10 18:18 UTC (permalink / raw)
To: azkali.limited
Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, CTCaer, linux-iio,
devicetree, linux-kernel
On Mon, May 11, 2026 at 01:09:50AM +0700, Alexandre Hamamdjian via B4 Relay wrote:
> Add a driver for the ROHM BH1730FVC ambient light sensor. The device
> is a 16-bit I2C digital sensor with separate visible and infrared
> photodiodes, four selectable gains (1x/2x/64x/128x) and a programmable
> integration time.
>
> The driver exposes illuminance via IIO, performs runtime gain and
> integration-time tracking to keep the ADC in range, and supports
> optional als-vdd / als-vid regulators. Per-board lux calibration data
> (integration cycles, lux multiplier, optical-window coefficients, and
> gain sensitivity coefficients) can be supplied via device tree;
> sensible defaults are used otherwise.
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
Regular drivers do not to be OF-centric. This won't allow them to be used
outside of OF-only platforms.
> +#include <linux/time.h>
> +#include <linux/regulator/consumer.h>
Missing a lot of headers, follow IWYU.
...
Here I stop my review and recommend you first to review others' patches and
learn from other reviews. This will help you a lot with avoiding typical
mistakes.
Also Matti would be the best reviewer for this as he worked (still works?)
for ROHM and knows the HW a bit more than average kernel developer.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] iio: light: bh1730: Add bh1730 light sensor driver
2026-05-10 18:18 ` Andy Shevchenko
@ 2026-05-10 18:20 ` Andy Shevchenko
[not found] ` <CAL5cOWuXAD7+rJEKB9FjnwdCjoUJK+WNKXZXt8tfnq1WLmv5eg@mail.gmail.com>
2026-05-11 8:26 ` Matti Vaittinen
0 siblings, 2 replies; 21+ messages in thread
From: Andy Shevchenko @ 2026-05-10 18:20 UTC (permalink / raw)
To: azkali.limited, Matti Vaittinen
Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, CTCaer, linux-iio,
devicetree, linux-kernel
On Sun, May 10, 2026 at 09:18:48PM +0300, Andy Shevchenko wrote:
> On Mon, May 11, 2026 at 01:09:50AM +0700, Alexandre Hamamdjian via B4 Relay wrote:
>
> > Add a driver for the ROHM BH1730FVC ambient light sensor. The device
> > is a 16-bit I2C digital sensor with separate visible and infrared
> > photodiodes, four selectable gains (1x/2x/64x/128x) and a programmable
> > integration time.
> >
> > The driver exposes illuminance via IIO, performs runtime gain and
> > integration-time tracking to keep the ADC in range, and supports
> > optional als-vdd / als-vid regulators. Per-board lux calibration data
> > (integration cycles, lux multiplier, optical-window coefficients, and
> > gain sensitivity coefficients) can be supplied via device tree;
> > sensible defaults are used otherwise.
>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
>
> > +#include <linux/of.h>
>
> Regular drivers do not to be OF-centric. This won't allow them to be used
> outside of OF-only platforms.
>
> > +#include <linux/time.h>
> > +#include <linux/regulator/consumer.h>
>
> Missing a lot of headers, follow IWYU.
...
> Here I stop my review and recommend you first to review others' patches and
> learn from other reviews. This will help you a lot with avoiding typical
> mistakes.
>
> Also Matti would be the best reviewer for this as he worked (still works?)
> for ROHM and knows the HW a bit more than average kernel developer.
Forgot to Cc Matti since I mentioned him. Now done.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 21+ messages in thread[parent not found: <CAL5cOWuXAD7+rJEKB9FjnwdCjoUJK+WNKXZXt8tfnq1WLmv5eg@mail.gmail.com>]
* Re: [PATCH 2/2] iio: light: bh1730: Add bh1730 light sensor driver
[not found] ` <CAL5cOWuXAD7+rJEKB9FjnwdCjoUJK+WNKXZXt8tfnq1WLmv5eg@mail.gmail.com>
@ 2026-05-11 7:31 ` Andy Shevchenko
0 siblings, 0 replies; 21+ messages in thread
From: Andy Shevchenko @ 2026-05-11 7:31 UTC (permalink / raw)
To: Alexandre Hamamdjian
Cc: Matti Vaittinen, Jonathan Cameron, David Lechner, Nuno Sá,
Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
CTCaer, linux-iio, devicetree, linux-kernel
On Mon, May 11, 2026 at 02:11:10AM +0700, Alexandre Hamamdjian wrote:
Please, do not top-post!
> Thank you Andy I will talk with Mr. CTCaer if he agrees to use his real
> name for this driver, as well as the others he wrote that I already
> submitted.
If not, you can use Originally-by tag.
> I am also taking note of your review and will soon test if using the common
> BH17 light sensor driver is better for our devices, I will follow up
> whenever possible.
>
> Also awaiting for Mr. Vaittinen's review.
Yes, would be really nice to see Matti's review for this.
> On Mon, May 11, 2026, 1:20 AM Andy Shevchenko <andriy.shevchenko@intel.com>
> wrote:
> > On Sun, May 10, 2026 at 09:18:48PM +0300, Andy Shevchenko wrote:
> > > On Mon, May 11, 2026 at 01:09:50AM +0700, Alexandre Hamamdjian via B4
> > Relay wrote:
...
> > > Here I stop my review and recommend you first to review others' patches
> > and
> > > learn from other reviews. This will help you a lot with avoiding typical
> > > mistakes.
> > >
> > > Also Matti would be the best reviewer for this as he worked (still
> > works?)
> > > for ROHM and knows the HW a bit more than average kernel developer.
> >
> > Forgot to Cc Matti since I mentioned him. Now done.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] iio: light: bh1730: Add bh1730 light sensor driver
2026-05-10 18:20 ` Andy Shevchenko
[not found] ` <CAL5cOWuXAD7+rJEKB9FjnwdCjoUJK+WNKXZXt8tfnq1WLmv5eg@mail.gmail.com>
@ 2026-05-11 8:26 ` Matti Vaittinen
1 sibling, 0 replies; 21+ messages in thread
From: Matti Vaittinen @ 2026-05-11 8:26 UTC (permalink / raw)
To: Andy Shevchenko, azkali.limited
Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, CTCaer, linux-iio,
devicetree, linux-kernel, heikki.haikola
On 10/05/2026 21:20, Andy Shevchenko wrote:
> On Sun, May 10, 2026 at 09:18:48PM +0300, Andy Shevchenko wrote:
>> On Mon, May 11, 2026 at 01:09:50AM +0700, Alexandre Hamamdjian via B4 Relay wrote:
>>
>>> Add a driver for the ROHM BH1730FVC ambient light sensor. The device
>>> is a 16-bit I2C digital sensor with separate visible and infrared
>>> photodiodes, four selectable gains (1x/2x/64x/128x) and a programmable
>>> integration time.
>>>
>>> The driver exposes illuminance via IIO, performs runtime gain and
>>> integration-time tracking to keep the ADC in range, and supports
>>> optional als-vdd / als-vid regulators. Per-board lux calibration data
>>> (integration cycles, lux multiplier, optical-window coefficients, and
>>> gain sensitivity coefficients) can be supplied via device tree;
>>> sensible defaults are used otherwise.
>>
>>> +#include <linux/delay.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/module.h>
>>
>>> +#include <linux/of.h>
>>
>> Regular drivers do not to be OF-centric. This won't allow them to be used
>> outside of OF-only platforms.
>>
>>> +#include <linux/time.h>
>>> +#include <linux/regulator/consumer.h>
>>
>> Missing a lot of headers, follow IWYU.
>
> ...
>
>> Here I stop my review and recommend you first to review others' patches and
>> learn from other reviews. This will help you a lot with avoiding typical
>> mistakes.
>>
>> Also Matti would be the best reviewer for this as he worked (still works?)
>> for ROHM and knows the HW a bit more than average kernel developer.
>
> Forgot to Cc Matti since I mentioned him. Now done.
Thanks for pinging me :) I am not super familiar with this particular
sensor - original ROHM driver was written by my colleague Heikki, and
not by me. I will anyways take a look and provide what-ever input I can.
(I will also CC Heikki just in case, but he may not have the time to
look this further. Besides, the email client which can be used with
company email isn't really upstream compatible ;) ).
--
---
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] iio: light: bh1730: Add bh1730 light sensor driver
2026-05-10 18:09 ` Alexandre Hamamdjian via B4 Relay
(?)
(?)
@ 2026-05-11 10:17 ` Matti Vaittinen
-1 siblings, 0 replies; 21+ messages in thread
From: Matti Vaittinen @ 2026-05-11 10:17 UTC (permalink / raw)
To: Alexandre Hamamdjian, Jonathan Cameron, David Lechner,
Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, CTCaer
Cc: linux-iio, devicetree, linux-kernel
On 10/05/2026 21:09, Alexandre Hamamdjian wrote:
> From: CTCaer <ctcaer@gmail.com>
>
> Add a driver for the ROHM BH1730FVC ambient light sensor. The device
> is a 16-bit I2C digital sensor with separate visible and infrared
> photodiodes, four selectable gains (1x/2x/64x/128x) and a programmable
> integration time.
>
> The driver exposes illuminance via IIO, performs runtime gain and
> integration-time tracking to keep the ADC in range, and supports
> optional als-vdd / als-vid regulators. Per-board lux calibration data
> (integration cycles, lux multiplier, optical-window coefficients, and
> gain sensitivity coefficients) can be supplied via device tree;
> sensible defaults are used otherwise.
Thanks for the patch Alexandre :) It looks mostly good to me! It is
great to see support for this being upstreamed! Overall good work! A few
questions / comments below.
>
> Signed-off-by: CTCaer <ctcaer@gmail.com>
> Signed-off-by: Alexandre Hamamdjian <azkali.limited@gmail.com>
> ---
> drivers/iio/light/Kconfig | 9 +
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/bh1730.c | 686 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 696 insertions(+)
>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index eff33e456c70..3869060567dd 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -135,6 +135,15 @@ config AS73211
> This driver can also be built as a module. If so, the module
> will be called as73211.
>
> +config BH1730
> + tristate "ROHM BH1730 ambient light sensor"
> + depends on I2C
> + help
> + Say Y here to build support for the ROHM BH1730 ambient light sensor.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called bh1730.
> +
> config BH1745
> tristate "ROHM BH1745 colour sensor"
> depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index c0048e0d5ca8..27ef7906a02a 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_APDS9300) += apds9300.o
> obj-$(CONFIG_APDS9306) += apds9306.o
> obj-$(CONFIG_APDS9960) += apds9960.o
> obj-$(CONFIG_AS73211) += as73211.o
> +obj-$(CONFIG_BH1730) += bh1730.o
> obj-$(CONFIG_BH1745) += bh1745.o
> obj-$(CONFIG_BH1750) += bh1750.o
> obj-$(CONFIG_BH1780) += bh1780.o
> diff --git a/drivers/iio/light/bh1730.c b/drivers/iio/light/bh1730.c
> new file mode 100644
> index 000000000000..c93290ff5661
> --- /dev/null
> +++ b/drivers/iio/light/bh1730.c
> @@ -0,0 +1,686 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ROHM BH1730 ambient light sensor driver
> + *
> + * Copyright (c) 2021 CTCaer <ctcaer@gmail.com>
> + *
> + * Based on previous iio BH1730FVC drivers from:
> + * Copyright (c) 2018 Google, Inc.
> + * Author: Pierre Bourdon <delroth@google.com>
> + *
> + * 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>
> +#include <linux/regulator/consumer.h>
> +
> +#define BH1730_NAME "bh1730fvc"
> +
> +#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
I don't see all these registers used. Do you plan to add events for the
thresholds? If not, then maybe ditch the unused defines? (Unless you
think they should be kept for some documentation purposes).
> +#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 BIT(0)
> +#define BH1730_CONTROL_ADC_EN BIT(1)
> +#define BH1730_CONTROL_DATA0_ONLY BIT(2)
> +#define BH1730_CONTROL_ONE_TIME BIT(3)
> +#define BH1730_CONTROL_ADC_VALID BIT(4)
> +#define BH1730_CONTROL_INTR BIT(5)
> +
> +#define BH1730_INTERNAL_CLOCK_NS 2800
> +/* BH1730_INTERNAL_CLOCK_MS * 714 */
> +#define BH1730_ADC_CALC_DELAY_US 2000
I don't quite follow this. If BH1730_INTERNAL_CLOCK_NS is 2800, then:
BH1730_INTERNAL_CLOCK_US should be 2,800 and
BH1730_INTERNAL_CLOCK_MS should be 0,002800, right?
0,002800 * 714 => 1.9992, not 2000.
Furthermore, if the BH1730_ADC_CALC_DELAY_US results from the
BH1730_INTERNAL_CLOCK_NS - why not compute it in the macro instead of
using a comment?
> +/* BH1730_INTERNAL_CLOCK_MS * 964 */
> +#define BH1730_ITIME_TO_US 2700
> +
> +#define BH1730_DEFAULT_INTEG_CYCLE 38
> +#define BH1730_DEFAULT_ITIME_MS 100
> +
> +#define BH1730_POWER_ON_DELAY_US 10000
> +
> +#define BH1730_MAX_MEASURED_LUX 100000
> +
> +enum bh1730_gain {
> + BH1730_GAIN_1X = 0,
> + BH1730_GAIN_2X,
> + BH1730_GAIN_64X,
> + BH1730_GAIN_128X,
> +};
> +#define BH1730_MAX_GAIN_MULTIPLIER 128
> +
> +struct gain_coeff_t {
> + u32 cl;
> + u32 fl;
> +};
> +
> +struct opt_win_coeff_t {
> + u32 rc;
> + u32 cv;
> + u32 ci;
> +};
> +
> +struct lux_cal_data_t {
> + struct gain_coeff_t *gain_coeff;
> + struct opt_win_coeff_t *opt_win_coeff;
> + u32 opt_win_coeff_count;
> + u32 itime_cycle;
> + u32 mul;
> +};
> +
> +/*
> + * No optical window or optical window that has flat transmittance
> + * from visible light to infrared light.
> + */
> +static struct opt_win_coeff_t def_lux_coeff[] = {
> + { 260, 1290, 2733 },
> + { 550, 795, 859 },
> + { 1090, 510, 345 },
> + { 2130, 276, 130 }
> +};
> +
> +static struct gain_coeff_t def_gain_coeff[] = {
> + { 3000, -1 }, /* 1x */
> + { 2000, 9800 }, /* 2x */
> + { 15, 60000 }, /* 64x */
> + { 0, 1300 } /* 128x */
> +};
> +
> +static struct lux_cal_data_t def_lux_data = {
> + .gain_coeff = def_gain_coeff,
> + .opt_win_coeff = def_lux_coeff,
> + .opt_win_coeff_count = ARRAY_SIZE(def_lux_coeff),
> + .itime_cycle = BH1730_DEFAULT_INTEG_CYCLE,
> + .mul = 1000
> +};
> +
> +struct bh1730_data {
> + struct i2c_client *client;
> + struct lux_cal_data_t cal;
> + struct regulator *reg_vdd;
> + struct regulator *reg_vid;
> + enum bh1730_gain gain;
> + u32 itime_us;
> + u32 tmt_us;
> +};
> +
> +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;
> +}
Have you considered using the regmap? I personally prefer it as it gives
a few nice interfaces like the regmap_update bits(). It also implements
register caching - which typically allows us to drop cached stuff from
the driver private data. I think most of the upstream ROHM IC-drivers
use it so it might also make things look more familiar to some contributors.
Well, seeing you don't use too many control registers, I will leave this
to you and other reviewers to decide.
> +static int bh1730_gain_multiplier(struct bh1730_data *bh1730)
> +{
> + int multiplier;
> +
> + switch (bh1730->gain) {
> + case BH1730_GAIN_1X:
> + multiplier = 1;
> + break;
> + case BH1730_GAIN_2X:
> + multiplier = 2;
> + break;
> + case BH1730_GAIN_64X:
> + multiplier = 64;
> + break;
> + case BH1730_GAIN_128X:
> + multiplier = 128;
> + break;
> + default:
> + multiplier = -EINVAL;
> + break;
> + }
> +
> + if (multiplier < 0) {
> + dev_warn(&bh1730->client->dev,
> + "invalid gain multiplier settings: %d\n",
> + bh1730->gain);
> + bh1730->gain = BH1730_GAIN_1X;
> + multiplier = 1;
> + }
> +
> + return multiplier;
> +}
> +
> +static int bh1730_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_itime_us(struct bh1730_data *bh1730, int cycle)
> +{
> + return (BH1730_ITIME_TO_US * cycle);
> +}
> +
> +
> +static int bh1730_set_integration_time_cycle(struct bh1730_data *bh1730,
> + u32 cycle)
> +{
> + int ret, itime;
> +
> + itime = 256 - cycle;
> +
> + /* ITIME == 0 is reserved for manual integration mode. */
> + if (itime <= 0 || itime > 255) {
> + dev_warn(&bh1730->client->dev,
> + "integration time out of range: %d cycles\n",
> + cycle);
> +
> + return -ERANGE;
> + }
> +
> + ret = bh1730_write(bh1730, BH1730_REG_TIMING, itime);
> + if (ret < 0)
> + return ret;
> +
> + bh1730->itime_us = bh1730_itime_us(bh1730, cycle);
> + bh1730->tmt_us = bh1730->itime_us + BH1730_ADC_CALC_DELAY_US;
> +
> + return 0;
> +}
> +
> +static int bh1730_adjust_gain(struct bh1730_data *bh1730)
> +{
> + int visible, ir, highest, gain, ret;
> +
> + visible = bh1730_read_word(bh1730, BH1730_REG_DATA0LOW);
> + if (visible < 0)
> + return visible;
> +
> + ir = bh1730_read_word(bh1730, BH1730_REG_DATA1LOW);
> + if (ir < 0)
> + return ir;
> +
> + highest = max(visible, ir);
> +
> + /* Adjust gain based on sensitivity calibration */
> + gain = bh1730->gain;
> + if (highest > bh1730->cal.gain_coeff[gain].fl &&
> + gain != BH1730_GAIN_1X) {
> + gain--; /* Decrease sensitivity */
> + } else if (highest < bh1730->cal.gain_coeff[gain].cl &&
> + gain != BH1730_GAIN_128X) {
> + gain++; /* Increase sensitivity */
> + }
> +
> + /* Clamp to proper gain values */
> + if (gain < 0)
> + gain = BH1730_GAIN_1X;
> + else if (gain > BH1730_GAIN_128X)
> + gain = BH1730_GAIN_128X;
> +
> + if (gain != bh1730->gain) {
> + ret = bh1730_set_gain(bh1730, gain);
> + if (ret < 0)
> + return ret;
> +
> + usleep_range(bh1730->tmt_us, bh1730->tmt_us + 1000);
> + }
> +
> + return 0;
> +}
Hm. Is it usual thing to do this type of "automatic gain control"
in-kernel? I kind of expected seeing scale-setting exposed to the
user-space via the standard attributes, and assumed user-space would the
do the required tricks to tune the scale, (integration-time or the gain
as decided by the driver), if channel is saturated or accuracy drops too
much.
Anyways, the only "improvement" I can think of, would be adjusting also
the integrating time if data gets saturated with 1X gain - or if more
accuracy is needed. That might, or might not, make sense on some systems
using this component. If you decide to pursue this approach, then you
may want to check the gts-helpers code which should provide you some
tools for handling the gains and integration times.
Well, if you only use the Nintendo Switch, and if it does not need
anything fancier, then I am not complaining!
Finally, I am unsure if the default gain_coefficients need to be ever
overridden? I am not sure I understand why they would? Wouldn't simple
hard-coded limits work just fine? Anyways, as I wrote, I am not
complaining. ;) This looks clean-enough to me - let's see what wiser
people say!
> +static int bh1730_get_lux(struct bh1730_data *bh1730)
> +{
> + int i, visible, ir;
> + struct opt_win_coeff_t *opt_win_coeff;
> + u64 lux = 0;
> +
> + visible = bh1730_read_word(bh1730, BH1730_REG_DATA0LOW);
> +
> + /* If visible is 0, skip calculations */
> + if (visible <= 0)
> + return visible;
> +
> + ir = bh1730_read_word(bh1730, BH1730_REG_DATA1LOW);
> + if (ir < 0)
> + return ir;
> +
> + /* Calibrate based on optical window */
> + for (i = 0; i < bh1730->cal.opt_win_coeff_count; i++) {
> + opt_win_coeff = &bh1730->cal.opt_win_coeff[i];
> + if (1000 * ir / visible < opt_win_coeff->rc) {
> + lux = ((u64)opt_win_coeff->cv * visible) -
> + (opt_win_coeff->ci * ir);
> + break;
> + }
> + }
> +
> + /* Calculate lux */
> + lux *= BH1730_DEFAULT_ITIME_MS;
> + lux /= bh1730_gain_multiplier(bh1730) * bh1730->itime_us;
> + lux = (lux * bh1730->cal.mul) / 1000;
> +
> + if (lux > BH1730_MAX_MEASURED_LUX)
> + lux = BH1730_MAX_MEASURED_LUX;
> +
> + return (int)lux;
> +}
> +
> +static int bh1730_power_ctrl(struct bh1730_data *bh1730, bool enable)
> +{
> + struct device *dev = &bh1730->client->dev;
> + static bool enabled;
> + int ret = 0;
> +
> + if (enabled == enable)
> + return ret;
This flag smells racy. Does this require protection?
> +
> + if (enable) {
> + if (!IS_ERR_OR_NULL(bh1730->reg_vdd)) {
> + ret = regulator_enable(bh1730->reg_vdd);
> + if (ret) {
> + dev_err(dev, "%s: Failed to enable vdd: %d\n",
> + __func__, ret);
I think you should be able to drop the __func__ from all the prints.
> + return ret;
> + }
> + }
> +
> + if (!IS_ERR_OR_NULL(bh1730->reg_vid)) {
> + ret = regulator_enable(bh1730->reg_vid);
> + if (ret) {
> + dev_err(dev, "%s: Failed to enable vid: %d\n",
> + __func__, ret);
> + return ret;
> + }
> + }
> +
> + usleep_range(BH1730_POWER_ON_DELAY_US, BH1730_POWER_ON_DELAY_US + 1000);
> + } else {
> + if (!IS_ERR_OR_NULL(bh1730->reg_vdd)) {
> + ret = regulator_disable(bh1730->reg_vdd);
> + if (ret) {
> + dev_err(dev, "%s: Failed to disable vdd: %d\n",
> + __func__, ret);
> + return ret;
> + }
> + }
> +
> + if (!IS_ERR_OR_NULL(bh1730->reg_vid)) {
> + ret = regulator_disable(bh1730->reg_vid);
> + if (ret) {
> + dev_err(dev, "%s: Failed to disable vid: %d\n",
> + __func__, ret);
> + return ret;
> + }
> + }
> + }
> +
> + enabled = enable;
> +
> + return ret;
> +}
> +
> +static int bh1730_power_on(struct bh1730_data *bh1730)
> +{
> + int ret = bh1730_power_ctrl(bh1730, true);
> +
> + if (ret < 0)
> + return ret;
> +
> + return bh1730_write(bh1730, BH1730_REG_CONTROL,
> + BH1730_CONTROL_POWER | BH1730_CONTROL_ADC_EN);
> +}
> +
> +static int bh1730_init_config(struct bh1730_data *bh1730)
> +{
> + int ret;
> +
> + ret = bh1730_set_gain(bh1730, BH1730_GAIN_64X);
> + if (ret < 0)
> + return ret;
> +
> + return bh1730_set_integration_time_cycle(bh1730,
> + bh1730->cal.itime_cycle);
> +}
> +
> +static int bh1730_power_off(struct bh1730_data *bh1730)
> +{
> + int ret = bh1730_write(bh1730, BH1730_REG_CONTROL, 0);
> +
> + if (ret < 0)
> + return ret;
> +
> + return bh1730_power_ctrl(bh1730, false);
> +}
> +
> +static int bh1730_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct bh1730_data *bh1730 = iio_priv(indio_dev);
> + int data_reg, ret;
> +
> + ret = bh1730_adjust_gain(bh1730);
> + if (ret < 0)
> + return ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + ret = bh1730_get_lux(bh1730);
> + if (ret < 0)
> + return ret;
> + *val = ret;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_RAW:
> + switch (chan->channel2) {
> + case IIO_MOD_LIGHT_CLEAR:
> + data_reg = BH1730_REG_DATA0LOW;
> + break;
> + case IIO_MOD_LIGHT_IR:
> + data_reg = BH1730_REG_DATA1LOW;
> + break;
> + default:
> + return -EINVAL;
> + }
> + ret = bh1730_read_word(bh1730, data_reg);
> + if (ret < 0)
> + return ret;
> + ret = ret * 1000 / bh1730_gain_multiplier(bh1730);
Again, I am not sure if this is the usual way. For me it would look
"more raw", if the raw data was provided as it is in the channel, and
the used gain/integration time was exposed via scale. Well, I suppose it
wouldn't work with the 'automatic, id-driver gain correction' as reading
gain and data would be racy. Oh well... I'll leave this to others as well :)
> + *val = ret / 1000;
> + *val2 = (ret % 1000) * 1000;
> + return IIO_VAL_INT_PLUS_MICRO;
> + case IIO_CHAN_INFO_INT_TIME:
> + *val = 0;
> + *val2 = bh1730->itime_us;
> + return IIO_VAL_INT_PLUS_MICRO;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int bh1730_parse_dt(struct bh1730_data *bh1730, struct device_node *dn)
> +{
> + struct device *dev = &bh1730->client->dev;
> + int ret;
> + u32 *opt_win_coeff = NULL;
> + u32 *gain_coeff = NULL;
> + int opt_win_coeff_count = 0, gain_coeff_count = 0, cycle = 0, mul = 0;
> +
> + memcpy(&bh1730->cal, &def_lux_data, sizeof(struct lux_cal_data_t));
> +
> + if (dn) {
> + /* Get regulators */
> + bh1730->reg_vdd = regulator_get(dev, "als-vdd");
> + if (IS_ERR_OR_NULL(bh1730->reg_vdd)) {
> + bh1730->reg_vdd = NULL;
> + dev_warn(dev, "failed to get als-vdd");
> + }
> +
> + bh1730->reg_vid = regulator_get(dev, "als-vid");
> + if (IS_ERR_OR_NULL(bh1730->reg_vid)) {
> + bh1730->reg_vid = NULL;
> + dev_warn(dev, "failed to get als-vid");
> + }
> +
> + /* Get calibration */
> + ret = of_property_read_u32(dn,
> + "rohm,integration-cycle", &cycle);
> + if (ret < 0)
> + goto out;
> +
> + ret = of_property_read_u32(dn,
> + "rohm,lux-multiplier", &mul);
> + if (ret < 0)
> + goto out;
> +
> + if (cycle == 0 || mul == 0)
> + goto out;
> +
> + /* Get optical window coefficients */
> + opt_win_coeff_count = of_property_count_elems_of_size(dn,
> + "rohm,opt-win-coeff", sizeof(u32));
> + if (opt_win_coeff_count > 0) {
> + opt_win_coeff = devm_kzalloc(dev,
> + sizeof(u32) * opt_win_coeff_count,
> + GFP_KERNEL);
> + if (!opt_win_coeff) {
> + dev_err(dev, "failed to allocate mem for opt_win_coeff");
> + return -ENOMEM;
> + }
> +
> + ret = of_property_read_u32_array(dn, "rohm,opt-win-coeff",
> + opt_win_coeff, opt_win_coeff_count);
> +
> + if (ret) {
> + devm_kfree(dev, opt_win_coeff);
> + goto out;
> + }
> + }
> +
> + /* Get gain sensitivity coefficients */
> + gain_coeff_count = of_property_count_elems_of_size(dn,
> + "rohm,gain-coeff", sizeof(u32));
> +
> + if (gain_coeff_count == 8) { /* 2 for each gain supported */
> + gain_coeff = devm_kzalloc(dev,
> + sizeof(u32) * gain_coeff_count,
> + GFP_KERNEL);
> + if (!gain_coeff) {
> + dev_err(dev, "failed to allocate mem for gain_coeff");
> + return -ENOMEM;
> + }
> +
> + ret = of_property_read_u32_array(dn, "rohm,gain-coeff",
> + gain_coeff, gain_coeff_count);
> + if (ret) {
> + if (opt_win_coeff)
> + devm_kfree(dev, opt_win_coeff);
> + devm_kfree(dev, gain_coeff);
> + goto out;
> + }
> + }
> +
> + if (opt_win_coeff) {
> + bh1730->cal.opt_win_coeff =
> + (struct opt_win_coeff_t *)opt_win_coeff;
> + bh1730->cal.opt_win_coeff_count =
> + opt_win_coeff_count /
> + (sizeof(struct opt_win_coeff_t) /
> + sizeof(u32));
> + }
> +
> + if (gain_coeff)
> + bh1730->cal.gain_coeff = (struct gain_coeff_t *)gain_coeff;
> +
> + bh1730->cal.itime_cycle = cycle;
> + bh1730->cal.mul = mul;
> +
> + return 0;
> + }
> +
> +out:
> + dev_info(&bh1730->client->dev, "using default calibration");
Is this really required info? I might consider making this debug (if
defaults really work well), or a warn (if defaults aren't working well).
Not sure having this as 'info' at every boot makes sense.
> +
> + return 0;
> +}
> +
> +static const struct iio_info bh1730_info = {
> + .read_raw = bh1730_read_raw,
> +};
> +
> +static const struct iio_chan_spec bh1730_channels[] = {
> + {
> + .type = IIO_LIGHT,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> + BIT(IIO_CHAN_INFO_INT_TIME),
> + },
> + {
> + .type = IIO_INTENSITY,
> + .modified = 1,
> + .channel2 = IIO_MOD_LIGHT_CLEAR,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + },
> + {
> + .type = IIO_INTENSITY,
> + .modified = 1,
> + .channel2 = IIO_MOD_LIGHT_IR,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + },
> +};
> +
> +static int bh1730_probe(struct i2c_client *client)
> +{
> + struct bh1730_data *bh1730;
> + struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
> + return -EIO;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*bh1730));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + bh1730 = iio_priv(indio_dev);
> + bh1730->client = client;
> + i2c_set_clientdata(client, indio_dev);
> +
> + ret = bh1730_parse_dt(bh1730, client->dev.of_node);
> + if (ret < 0)
> + return ret;
> +
> + ret = bh1730_power_on(bh1730);
> + if (ret < 0)
> + return ret;
> +
> + ret = bh1730_init_config(bh1730);
> + if (ret < 0)
> + return ret;
This does not (try) power-off BH1730? but - see "devm" comment below...
> +
> + 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;
Why not the devm -variant? I think that if you used devm here, and added
an action for the power-off, then you might be able to ditch the remove()?
> +
> + return 0;
> +
> +out_power_off:
> + dev_info(&client->dev, "%s: failed\n", __func__);
> + bh1730_power_off(bh1730);
> + return ret;
> +}
> +
Yours,
-- Matti
--
---
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] iio: light: bh1730: Add bh1730 light sensor driver
2026-05-10 18:09 ` Alexandre Hamamdjian via B4 Relay
` (2 preceding siblings ...)
(?)
@ 2026-05-11 15:50 ` Jonathan Cameron
-1 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2026-05-11 15:50 UTC (permalink / raw)
To: Alexandre Hamamdjian via B4 Relay
Cc: azkali.limited, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, CTCaer, linux-iio,
devicetree, linux-kernel
On Mon, 11 May 2026 01:09:50 +0700
Alexandre Hamamdjian via B4 Relay <devnull+azkali.limited.gmail.com@kernel.org> wrote:
> From: CTCaer <ctcaer@gmail.com>
>
> Add a driver for the ROHM BH1730FVC ambient light sensor. The device
> is a 16-bit I2C digital sensor with separate visible and infrared
> photodiodes, four selectable gains (1x/2x/64x/128x) and a programmable
> integration time.
>
> The driver exposes illuminance via IIO, performs runtime gain and
> integration-time tracking to keep the ADC in range, and supports
> optional als-vdd / als-vid regulators. Per-board lux calibration data
> (integration cycles, lux multiplier, optical-window coefficients, and
> gain sensitivity coefficients) can be supplied via device tree;
> sensible defaults are used otherwise.
>
> Signed-off-by: CTCaer <ctcaer@gmail.com>
> Signed-off-by: Alexandre Hamamdjian <azkali.limited@gmail.com>
Hi Alexandre
Nice driver in general - some comments inline to add to those from Matti and
Andy.
thanks,
Jonathan
> ---
> drivers/iio/light/Kconfig | 9 +
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/bh1730.c | 686 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 696 insertions(+)
>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index eff33e456c70..3869060567dd 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -135,6 +135,15 @@ config AS73211
> This driver can also be built as a module. If so, the module
> will be called as73211.
>
> +config BH1730
> + tristate "ROHM BH1730 ambient light sensor"
> + depends on I2C
> + help
> + Say Y here to build support for the ROHM BH1730 ambient light sensor.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called bh1730.
> +
> config BH1745
> tristate "ROHM BH1745 colour sensor"
> depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index c0048e0d5ca8..27ef7906a02a 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_APDS9300) += apds9300.o
> obj-$(CONFIG_APDS9306) += apds9306.o
> obj-$(CONFIG_APDS9960) += apds9960.o
> obj-$(CONFIG_AS73211) += as73211.o
> +obj-$(CONFIG_BH1730) += bh1730.o
> obj-$(CONFIG_BH1745) += bh1745.o
> obj-$(CONFIG_BH1750) += bh1750.o
> obj-$(CONFIG_BH1780) += bh1780.o
> diff --git a/drivers/iio/light/bh1730.c b/drivers/iio/light/bh1730.c
> new file mode 100644
> index 000000000000..c93290ff5661
> --- /dev/null
> +++ b/drivers/iio/light/bh1730.c
> @@ -0,0 +1,686 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ROHM BH1730 ambient light sensor driver
> + *
> + * Copyright (c) 2021 CTCaer <ctcaer@gmail.com>
> + *
> + * Based on previous iio BH1730FVC drivers from:
> + * Copyright (c) 2018 Google, Inc.
> + * Author: Pierre Bourdon <delroth@google.com>
> + *
> + * 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>
> +#include <linux/regulator/consumer.h>
> +
> +#define BH1730_NAME "bh1730fvc"
Don't have a define like this - it is rarely the case that there
are places where it needs to be the same and it means the value isn't
readily available.
> +
> +#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 BIT(0)
> +#define BH1730_CONTROL_ADC_EN BIT(1)
> +#define BH1730_CONTROL_DATA0_ONLY BIT(2)
> +#define BH1730_CONTROL_ONE_TIME BIT(3)
> +#define BH1730_CONTROL_ADC_VALID BIT(4)
> +#define BH1730_CONTROL_INTR BIT(5)
> +
> +#define BH1730_INTERNAL_CLOCK_NS 2800
> +/* BH1730_INTERNAL_CLOCK_MS * 714 */
> +#define BH1730_ADC_CALC_DELAY_US 2000
> +/* BH1730_INTERNAL_CLOCK_MS * 964 */
> +#define BH1730_ITIME_TO_US 2700
> +
> +#define BH1730_DEFAULT_INTEG_CYCLE 38
> +#define BH1730_DEFAULT_ITIME_MS 100
> +
> +#define BH1730_POWER_ON_DELAY_US 10000
> +
> +#define BH1730_MAX_MEASURED_LUX 100000
> +
> +enum bh1730_gain {
> + BH1730_GAIN_1X = 0,
Which each takes a specific value because they are a hardware thing
I'd always set them all - even if it seems a bit silly!
> + BH1730_GAIN_2X,
> + BH1730_GAIN_64X,
> + BH1730_GAIN_128X,
> +};
> +#define BH1730_MAX_GAIN_MULTIPLIER 128
> +static int bh1730_read_word(struct bh1730_data *bh1730, u8 reg)
> +{
> + int ret = i2c_smbus_read_word_data(bh1730->client,
> + BH1730_CMD_BIT | reg);
> + if (ret < 0)
> + dev_err(&bh1730->client->dev,
> + "i2c read failed error %d, register %01x\n",
> + ret, reg);
As Matti suggested - might be worth looking at regmap as seems to be
fairly standard register interface and that bring a bunch of nice
helpers.
> +
> + 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 bh1730_itime_us(struct bh1730_data *bh1730, int cycle)
> +{
> + return (BH1730_ITIME_TO_US * cycle);
> +}
Not sure this function is worth having. The code seems simpler than the
function call!
> +
> +
> +static int bh1730_set_integration_time_cycle(struct bh1730_data *bh1730,
> + u32 cycle)
> +{
> + int ret, itime;
> +
> + itime = 256 - cycle;
> +
> + /* ITIME == 0 is reserved for manual integration mode. */
> + if (itime <= 0 || itime > 255) {
> + dev_warn(&bh1730->client->dev,
> + "integration time out of range: %d cycles\n",
> + cycle);
Align as:
dev_warn(&bh1730->client->dev,
"integration time out of range: %d cycles\n",
cycle);
> +
> + return -ERANGE;
> + }
> +
> + ret = bh1730_write(bh1730, BH1730_REG_TIMING, itime);
> + if (ret < 0)
> + return ret;
> +
> + bh1730->itime_us = bh1730_itime_us(bh1730, cycle);
> + bh1730->tmt_us = bh1730->itime_us + BH1730_ADC_CALC_DELAY_US;
> +
> + return 0;
> +}
> +
> +static int bh1730_adjust_gain(struct bh1730_data *bh1730)
Auto ranging is done by some sensor drivers and does explain why
you are hiding the scale. Ok. I don't mind doing it this way
though ti can be a bit tricky to get working right.
> +{
> + int visible, ir, highest, gain, ret;
> +
> + visible = bh1730_read_word(bh1730, BH1730_REG_DATA0LOW);
> + if (visible < 0)
> + return visible;
> +
> + ir = bh1730_read_word(bh1730, BH1730_REG_DATA1LOW);
> + if (ir < 0)
> + return ir;
> +
> + highest = max(visible, ir);
> +
> + /* Adjust gain based on sensitivity calibration */
> + gain = bh1730->gain;
> + if (highest > bh1730->cal.gain_coeff[gain].fl &&
> + gain != BH1730_GAIN_1X) {
> + gain--; /* Decrease sensitivity */
> + } else if (highest < bh1730->cal.gain_coeff[gain].cl &&
> + gain != BH1730_GAIN_128X) {
> + gain++; /* Increase sensitivity */
> + }
No {} as only single line statements.
> +
> + /* Clamp to proper gain values */
> + if (gain < 0)
> + gain = BH1730_GAIN_1X;
> + else if (gain > BH1730_GAIN_128X)
> + gain = BH1730_GAIN_128X;
clamp()
> +
> + if (gain != bh1730->gain) {
> + ret = bh1730_set_gain(bh1730, gain);
> + if (ret < 0)
> + return ret;
> +
> + usleep_range(bh1730->tmt_us, bh1730->tmt_us + 1000);
fsleep() see below.
> + }
> +
> + return 0;
> +}
> +
> +static int bh1730_get_lux(struct bh1730_data *bh1730)
> +{
> + int i, visible, ir;
> + struct opt_win_coeff_t *opt_win_coeff;
> + u64 lux = 0;
> +
> + visible = bh1730_read_word(bh1730, BH1730_REG_DATA0LOW);
> +
> + /* If visible is 0, skip calculations */
> + if (visible <= 0)
> + return visible;
> +
> + ir = bh1730_read_word(bh1730, BH1730_REG_DATA1LOW);
> + if (ir < 0)
> + return ir;
> +
> + /* Calibrate based on optical window */
> + for (i = 0; i < bh1730->cal.opt_win_coeff_count; i++) {
> + opt_win_coeff = &bh1730->cal.opt_win_coeff[i];
> + if (1000 * ir / visible < opt_win_coeff->rc) {
> + lux = ((u64)opt_win_coeff->cv * visible) -
> + (opt_win_coeff->ci * ir);
> + break;
> + }
> + }
> +
> + /* Calculate lux */
> + lux *= BH1730_DEFAULT_ITIME_MS;
> + lux /= bh1730_gain_multiplier(bh1730) * bh1730->itime_us;
> + lux = (lux * bh1730->cal.mul) / 1000;
> +
> + if (lux > BH1730_MAX_MEASURED_LUX)
> + lux = BH1730_MAX_MEASURED_LUX;
lux = min(lux, HB1730_MAX_MEASURED_LUX);
> +
> + return (int)lux;
Maybe mask it? That will make it more explicit if you expect overflow.
> +}
> +
> +static int bh1730_power_ctrl(struct bh1730_data *bh1730, bool enable)
There is very little shared in here. Would it be cleaner as a power_on and
a power_off as separate functions?
> +{
> + struct device *dev = &bh1730->client->dev;
> + static bool enabled;
> + int ret = 0;
> +
> + if (enabled == enable)
> + return ret;
> +
> + if (enable) {
> + if (!IS_ERR_OR_NULL(bh1730->reg_vdd)) {
Turn them on anyway - the stub regulators mentioned below should deal
with them not existing. We might end up with a pointless sleep but it's
not very long anyway ;)
> + ret = regulator_enable(bh1730->reg_vdd);
> + if (ret) {
> + dev_err(dev, "%s: Failed to enable vdd: %d\n",
> + __func__, ret);
> + return ret;
> + }
> + }
> +
> + if (!IS_ERR_OR_NULL(bh1730->reg_vid)) {
> + ret = regulator_enable(bh1730->reg_vid);
> + if (ret) {
> + dev_err(dev, "%s: Failed to enable vid: %d\n",
> + __func__, ret);
> + return ret;
> + }
> + }
> +
> + usleep_range(BH1730_POWER_ON_DELAY_US, BH1730_POWER_ON_DELAY_US + 1000);
fsleep() for all 'fuzzy' length sleeps like this - it provides
a standardized amount of slack and cleanly deals with a wider range
of sleep values than the more specific calls.
> + } else {
> + if (!IS_ERR_OR_NULL(bh1730->reg_vdd)) {
> + ret = regulator_disable(bh1730->reg_vdd);
> + if (ret) {
> + dev_err(dev, "%s: Failed to disable vdd: %d\n",
> + __func__, ret);
> + return ret;
> + }
> + }
> +
> + if (!IS_ERR_OR_NULL(bh1730->reg_vid)) {
> + ret = regulator_disable(bh1730->reg_vid);
> + if (ret) {
> + dev_err(dev, "%s: Failed to disable vid: %d\n",
> + __func__, ret);
> + return ret;
> + }
> + }
> + }
> +
> + enabled = enable;
> +
> + return ret;
> +}
> +
> +static int bh1730_power_off(struct bh1730_data *bh1730)
> +{
> + int ret = bh1730_write(bh1730, BH1730_REG_CONTROL, 0);
Really minor thing:
If you are checking a value like this it is generally nicer to
split declaration and initialization.
int ret;
ret = bh...
if (ret < 0)
return ret;
> +
> + if (ret < 0)
> + return ret;
> +
> + return bh1730_power_ctrl(bh1730, false);
> +}
> +
> +static int bh1730_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct bh1730_data *bh1730 = iio_priv(indio_dev);
> + int data_reg, ret;
> +
> + ret = bh1730_adjust_gain(bh1730);
> + if (ret < 0)
> + return ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + ret = bh1730_get_lux(bh1730);
> + if (ret < 0)
> + return ret;
> + *val = ret;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_RAW:
> + switch (chan->channel2) {
> + case IIO_MOD_LIGHT_CLEAR:
> + data_reg = BH1730_REG_DATA0LOW;
> + break;
> + case IIO_MOD_LIGHT_IR:
> + data_reg = BH1730_REG_DATA1LOW;
> + break;
> + default:
> + return -EINVAL;
> + }
> + ret = bh1730_read_word(bh1730, data_reg);
> + if (ret < 0)
> + return ret;
> + ret = ret * 1000 / bh1730_gain_multiplier(bh1730);
Intensity channels are always a little odd.
They have no defined units because there isn't a good general definition
but we can still have _SCALE attributes and as such I'd expect that
gain multiplier to be read via that. Those do have to take in account
changes due to integration time and amplifiers though so can be a bit
fiddly to get right!
> + *val = ret / 1000;
> + *val2 = (ret % 1000) * 1000;
> + return IIO_VAL_INT_PLUS_MICRO;
> + case IIO_CHAN_INFO_INT_TIME:
> + *val = 0;
> + *val2 = bh1730->itime_us;
> + return IIO_VAL_INT_PLUS_MICRO;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int bh1730_parse_dt(struct bh1730_data *bh1730, struct device_node *dn)
> +{
> + struct device *dev = &bh1730->client->dev;
> + int ret;
> + u32 *opt_win_coeff = NULL;
> + u32 *gain_coeff = NULL;
> + int opt_win_coeff_count = 0, gain_coeff_count = 0, cycle = 0, mul = 0;
> +
> + memcpy(&bh1730->cal, &def_lux_data, sizeof(struct lux_cal_data_t));
> +
> + if (dn) {
> + /* Get regulators */
> + bh1730->reg_vdd = regulator_get(dev, "als-vdd");
> + if (IS_ERR_OR_NULL(bh1730->reg_vdd)) {
> + bh1730->reg_vdd = NULL;
> + dev_warn(dev, "failed to get als-vdd");
Needs to be more nuanced. Might be a deferred probe. I would always
do
return dev_err_probe() in this case.
Note that regulators that aren't present will be provided with a
dummy regulator and that will do nothing at all when told to turn on
and off.
if you actually need them to be real then oddly enough you need to
do a regulator_get_optional() as then you'll get an error return.
However - if you do that then add a comment.
> + }
> +
> + bh1730->reg_vid = regulator_get(dev, "als-vid");
devm_regulator_get()
As it stands you are leaking a reference so the drivers providing these
can't be fully removed even if all users are.
> + if (IS_ERR_OR_NULL(bh1730->reg_vid)) {
> + bh1730->reg_vid = NULL;
> + dev_warn(dev, "failed to get als-vid");
Its either needed or not. (Not the stub regulator thing I mention above).
So error out on an error - maybe it's a deferred probe result and we need
to wait for the regulator driver to finish binding.
> + }
> +
> + /* Get calibration */
> + ret = of_property_read_u32(dn,
> + "rohm,integration-cycle", &cycle);
I'd put that on one line. We are a bit flexible when wrapping is
hurting readability and can go a little past 80 chars.
All these need replacing with equivalents for generic firmware accesses.
ret = device_property_read_u32() etc
> + if (ret < 0)
> + goto out;
> +
> + ret = of_property_read_u32(dn,
> + "rohm,lux-multiplier", &mul);
> + if (ret < 0)
> + goto out;
> +
> + if (cycle == 0 || mul == 0)
> + goto out;
> +
> + /* Get optical window coefficients */
> + opt_win_coeff_count = of_property_count_elems_of_size(dn,
> + "rohm,opt-win-coeff", sizeof(u32));
> + if (opt_win_coeff_count > 0) {
> + opt_win_coeff = devm_kzalloc(dev,
> + sizeof(u32) * opt_win_coeff_count,
> + GFP_KERNEL);
> + if (!opt_win_coeff) {
> + dev_err(dev, "failed to allocate mem for opt_win_coeff");
> + return -ENOMEM;
> + }
> +
> + ret = of_property_read_u32_array(dn, "rohm,opt-win-coeff",
> + opt_win_coeff, opt_win_coeff_count);
> +
> + if (ret) {
> + devm_kfree(dev, opt_win_coeff);
> + goto out;
> + }
> + }
> +
> + /* Get gain sensitivity coefficients */
> + gain_coeff_count = of_property_count_elems_of_size(dn,
> + "rohm,gain-coeff", sizeof(u32));
> +
> + if (gain_coeff_count == 8) { /* 2 for each gain supported */
> + gain_coeff = devm_kzalloc(dev,
> + sizeof(u32) * gain_coeff_count,
> + GFP_KERNEL);
> + if (!gain_coeff) {
> + dev_err(dev, "failed to allocate mem for gain_coeff");
> + return -ENOMEM;
> + }
> +
> + ret = of_property_read_u32_array(dn, "rohm,gain-coeff",
> + gain_coeff, gain_coeff_count);
> + if (ret) {
> + if (opt_win_coeff)
> + devm_kfree(dev, opt_win_coeff);
> + devm_kfree(dev, gain_coeff);
> + goto out;
> + }
> + }
> +
> + if (opt_win_coeff) {
> + bh1730->cal.opt_win_coeff =
> + (struct opt_win_coeff_t *)opt_win_coeff;
> + bh1730->cal.opt_win_coeff_count =
> + opt_win_coeff_count /
> + (sizeof(struct opt_win_coeff_t) /
> + sizeof(u32));
> + }
> +
> + if (gain_coeff)
> + bh1730->cal.gain_coeff = (struct gain_coeff_t *)gain_coeff;
> +
> + bh1730->cal.itime_cycle = cycle;
> + bh1730->cal.mul = mul;
> +
> + return 0;
> + }
> +
> +out:
> + dev_info(&bh1730->client->dev, "using default calibration");
Probably a dev_dbg() as seems like a fairly normal thing to do?
However, if a partial set of calibration properties are supplied then
we don't want to do this and you should just fail the probe.
> +
> + return 0;
> +}
> +
> +static int bh1730_probe(struct i2c_client *client)
> +{
> + struct bh1730_data *bh1730;
> + struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
I'd use as
struct device *dev = &client->dev; as lots of instances of it in here.
> + 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_parse_dt(bh1730, client->dev.of_node);
Andy mentioned this. Make it bh1730_parse_fw() and use
linux/property.h accessors. Those work for dt and other firwmare types.
Those operate against the struct device rather.
> + if (ret < 0)
> + return ret;
> +
> + ret = bh1730_power_on(bh1730);
> + if (ret < 0)
> + return ret;
Power is turned on here, but you don't turn it off if init_config fails?
The devm_add_action_or_reset() mentioned below probably belongs here.
> +
> + ret = bh1730_init_config(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:
> + dev_info(&client->dev, "%s: failed\n", __func__);
Print more relevant error messages where they occur. Also
dev_err and generally no need to add __func__ when messages
are more informative than this one.
> + bh1730_power_off(bh1730);
If you use the devm_add_action_or_reset() suggested below
this will get cleaned up anyway on error so you can do
direct returns in the error paths.
> + return ret;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
No need for these using modern intefaces.
> +static int bh1730_suspend(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> + struct bh1730_data *bh1730 = iio_priv(indio_dev);
> +
> + bh1730_power_off(bh1730);
> +
> + return 0;
> +}
> +
> +static int bh1730_resume(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> + struct bh1730_data *bh1730 = iio_priv(indio_dev);
> + int ret;
> +
> + ret = bh1730_power_on(bh1730);
> + if (ret < 0)
> + return ret;
> +
> + return bh1730_init_config(bh1730);
> +}
> +
> +static SIMPLE_DEV_PM_OPS(bh1730_pm_ops, bh1730_suspend, bh1730_resume);
DEFINE_SIMPLE_DEV_PM_OPS()
https://elixir.bootlin.com/linux/v7.0.5/source/include/linux/pm.h#L416
(and the deprecation note for the macro you are using just below that)
which does magic to ensure this code is referenced but that the compiler
can figure out it can drop it. That means we get build coverage but
no bloated code if power management isn't built in.
> +#endif
> +
> +static void 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);
> + bh1730_power_off(bh1730);
I'd suggest using a devm_add_action_or_reset() call to register that
power off in an appropriate place in probe. Then you can use
devm_iio_device_register() and no need for a remove() at all.
> +}
> +
> +static const struct i2c_device_id bh1730_i2c_device_id[] = {
> + { BH1730_NAME, 0 },
I have no idea why this ", 0" is common. No point to it as it
always gets changed if we add new devices anyway.
{ "bh1730fvc" }
> + {}
{ }
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, bh1730_i2c_device_id);
> +
> +static const struct of_device_id of_bh1730_match[] = {
> + { .compatible = "rohm,bh1730fvc" },
> + {},
{ }
So add a space + no comma on terminating entrees like this as
we never want to put anything after them.
> +};
> +MODULE_DEVICE_TABLE(of, of_bh1730_match);
> +
> +static struct i2c_driver bh1730_driver = {
> + .probe = bh1730_probe,
> + .remove = bh1730_remove,
> + .driver = {
> + .name = BH1730_NAME,
> + .owner = THIS_MODULE,
> + .of_match_table = of_bh1730_match,
> +#ifdef CONFIG_PM_SLEEP
We have had better handling for this for a long time now!
.pm = pm_sleep_ptr(&bh1730_pm_ops),
> + .pm = &bh1730_pm_ops,
> +#endif
> + },
> + .id_table = bh1730_i2c_device_id,
> +};
> +module_i2c_driver(bh1730_driver);
> +
> +MODULE_AUTHOR("CTCaer <ctcaer@gmail.com>");
> +MODULE_DESCRIPTION("ROHM BH1730FVC driver");
> +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] iio: light: bh1730: Add bh1730 light sensor driver
2026-05-10 18:09 ` Alexandre Hamamdjian via B4 Relay
` (3 preceding siblings ...)
(?)
@ 2026-05-11 22:09 ` sashiko-bot
-1 siblings, 0 replies; 21+ messages in thread
From: sashiko-bot @ 2026-05-11 22:09 UTC (permalink / raw)
To: Alexandre Hamamdjian via B4 Relay; +Cc: krzk+dt, devicetree, conor+dt, robh
Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Global shared state breaks power management for multiple devices.
- [High] Regulator resource leak and probe deferral mishandling.
- [High] Unsigned integer underflow in lux calculation causes erroneous maximum brightness readings.
- [High] Missing mutex protecting concurrent IIO sysfs reads causes race conditions.
- [High] Probe error path leaves the device powered on.
- [High] Unbalanced regulator enable counts in error handling paths.
- [Medium] Incorrect I2C functionality check in probe.
- [Medium] RAW IIO channel violates ABI by returning scaled fractional values.
- [Medium] Missing integration-time tracking logic as claimed in the commit message.
--
commit 92646ec6b1c5614b002c3fd3629112d1e7719033
Author: CTCaer <ctcaer@gmail.com>
iio: light: bh1730: Add bh1730 light sensor driver
Add a driver for the ROHM BH1730FVC ambient light sensor with runtime gain
tracking and configurable integration time.
> The driver exposes illuminance via IIO, performs runtime gain and
> integration-time tracking to keep the ADC in range, and supports
This isn't a bug, but the commit message mentions runtime integration-time
tracking. While runtime gain tracking is implemented in bh1730_adjust_gain(),
is there equivalent runtime logic to track or adjust the integration time,
or is it only configured once during bh1730_init_config()?
> diff --git a/drivers/iio/light/bh1730.c b/drivers/iio/light/bh1730.c
> new file mode 100644
> index 0000000000000..c93290ff5661f
> --- /dev/null
> +++ b/drivers/iio/light/bh1730.c
> @@ -0,0 +1,686 @@
[ ... ]
> +static int bh1730_get_lux(struct bh1730_data *bh1730)
> +{
[ ... ]
> + /* Calibrate based on optical window */
> + for (i = 0; i < bh1730->cal.opt_win_coeff_count; i++) {
> + opt_win_coeff = &bh1730->cal.opt_win_coeff[i];
> + if (1000 * ir / visible < opt_win_coeff->rc) {
> + lux = ((u64)opt_win_coeff->cv * visible) -
> + (opt_win_coeff->ci * ir);
> + break;
> + }
> + }
> +
> + /* Calculate lux */
> + lux *= BH1730_DEFAULT_ITIME_MS;
> + lux /= bh1730_gain_multiplier(bh1730) * bh1730->itime_us;
> + lux = (lux * bh1730->cal.mul) / 1000;
> +
> + if (lux > BH1730_MAX_MEASURED_LUX)
> + lux = BH1730_MAX_MEASURED_LUX;
> +
> + return (int)lux;
> +}
Can the subtraction underflow if the infrared component (ci * ir) is greater
than the visible component (cv * visible)? Since lux is an unsigned 64-bit
integer, this could result in a massive positive number.
The subsequent clamping logic would then set lux to
BH1730_MAX_MEASURED_LUX (100,000). Would this cause the sensor to report
maximum brightness instead of 0 lux in environments where IR light dominates?
[ ... ]
> +static int bh1730_power_ctrl(struct bh1730_data *bh1730, bool enable)
> +{
> + struct device *dev = &bh1730->client->dev;
> + static bool enabled;
> + int ret = 0;
> +
> + if (enabled == enable)
> + return ret;
Will this global static variable break power management for multiple devices?
If a system contains multiple BH1730 sensors, powering on the first sensor sets
enabled = true. When the second sensor attempts to power on during probe,
would the enabled == enable check evaluate to true and return immediately
without enabling the regulators for the second device?
> + if (enable) {
> + if (!IS_ERR_OR_NULL(bh1730->reg_vdd)) {
> + ret = regulator_enable(bh1730->reg_vdd);
> + if (ret) {
> + dev_err(dev, "%s: Failed to enable vdd: %d\n",
> + __func__, ret);
> + return ret;
> + }
> + }
> +
> + if (!IS_ERR_OR_NULL(bh1730->reg_vid)) {
> + ret = regulator_enable(bh1730->reg_vid);
> + if (ret) {
> + dev_err(dev, "%s: Failed to enable vid: %d\n",
> + __func__, ret);
> + return ret;
> + }
> + }
If regulator_enable(bh1730->reg_vdd) succeeds but the subsequent
regulator_enable(bh1730->reg_vid) fails, will this return without
disabling the vdd regulator and leave an unbalanced enable count?
[ ... ]
> +static int bh1730_power_off(struct bh1730_data *bh1730)
> +{
> + int ret = bh1730_write(bh1730, BH1730_REG_CONTROL, 0);
> +
> + if (ret < 0)
> + return ret;
> +
> + return bh1730_power_ctrl(bh1730, false);
> +}
If the initial I2C write fails here, the function returns early without calling
bh1730_power_ctrl(..., false). Will this permanently leak the regulator
enable references upon an I2C error during device removal or suspend?
[ ... ]
> +static int bh1730_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct bh1730_data *bh1730 = iio_priv(indio_dev);
> + int data_reg, ret;
> +
> + ret = bh1730_adjust_gain(bh1730);
> + if (ret < 0)
> + return ret;
Is a mutex needed here to protect concurrent IIO sysfs reads?
Without a driver-level lock, if multiple userspace threads read from the IIO
sysfs attributes concurrently, their executions will interleave.
Could concurrent access allow threads to trample the hardware gain state
in bh1730_adjust_gain(), corrupt I2C read sequences, and read invalid
mid-integration data?
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + ret = bh1730_get_lux(bh1730);
> + if (ret < 0)
> + return ret;
> + *val = ret;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_RAW:
> + switch (chan->channel2) {
> + case IIO_MOD_LIGHT_CLEAR:
> + data_reg = BH1730_REG_DATA0LOW;
> + break;
> + case IIO_MOD_LIGHT_IR:
> + data_reg = BH1730_REG_DATA1LOW;
> + break;
> + default:
> + return -EINVAL;
> + }
> + ret = bh1730_read_word(bh1730, data_reg);
> + if (ret < 0)
> + return ret;
> + ret = ret * 1000 / bh1730_gain_multiplier(bh1730);
> + *val = ret / 1000;
> + *val2 = (ret % 1000) * 1000;
> + return IIO_VAL_INT_PLUS_MICRO;
Is it permissible to return a scaled fractional value for IIO_CHAN_INFO_RAW?
The IIO ABI specifies that IIO_CHAN_INFO_RAW must return the exact, unscaled
integer value from the hardware. Should the driver expose the raw ADC count
using IIO_VAL_INT and provide a separate IIO_CHAN_INFO_SCALE attribute so that
userspace can properly interpret and scale the value?
[ ... ]
> +static int bh1730_parse_dt(struct bh1730_data *bh1730, struct device_node *dn)
> +{
> + struct device *dev = &bh1730->client->dev;
> + int ret;
> + u32 *opt_win_coeff = NULL;
> + u32 *gain_coeff = NULL;
> + int opt_win_coeff_count = 0, gain_coeff_count = 0, cycle = 0, mul = 0;
> +
> + memcpy(&bh1730->cal, &def_lux_data, sizeof(struct lux_cal_data_t));
> +
> + if (dn) {
> + /* Get regulators */
> + bh1730->reg_vdd = regulator_get(dev, "als-vdd");
> + if (IS_ERR_OR_NULL(bh1730->reg_vdd)) {
> + bh1730->reg_vdd = NULL;
> + dev_warn(dev, "failed to get als-vdd");
> + }
Are the regulator resources leaked here? regulator_get() is used to acquire
them, but there are no corresponding regulator_put() calls in bh1730_remove()
or any of the probe error paths.
Also, does checking the return value with IS_ERR_OR_NULL incorrectly swallow
the -EPROBE_DEFER error code?
If a regulator is not yet available, the pointer is set to NULL and the driver
proceeds instead of properly deferring the probe, which could break power
management. Could devm_regulator_get_optional() be used here instead?
[ ... ]
> +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;
Is I2C_FUNC_SMBUS_BYTE the correct functionality to check? The driver actively
uses i2c_smbus_read_word_data() and i2c_smbus_write_byte_data().
Should this verify I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA
instead? Relying on I2C_FUNC_SMBUS_BYTE might cause the driver to bind to an
I2C adapter that does not support the required operations.
> +
> + 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_parse_dt(bh1730, client->dev.of_node);
> + if (ret < 0)
> + return ret;
> +
> + ret = bh1730_power_on(bh1730);
> + if (ret < 0)
> + return ret;
> +
> + ret = bh1730_init_config(bh1730);
> + if (ret < 0)
> + return ret;
If bh1730_init_config() fails, does this return early and leave the
device powered on?
Returning directly here would leak the regulator enable count instead of
jumping to the out_power_off label to properly clean up the state.
> +
> + indio_dev->dev.parent = &client->dev;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-bh1730-v1-0-e0df1f499135@gmail.com?part=2
^ permalink raw reply [flat|nested] 21+ messages in thread