* [PATCH v2 1/3] dt-bindings: iio: ROHM BU27010 RGBC + flickering sensor
2023-07-07 11:22 [PATCH v2 0/3] Support ROHM BU27010 RGBC sensor Matti Vaittinen
@ 2023-07-07 11:22 ` Matti Vaittinen
2023-07-10 9:05 ` Krzysztof Kozlowski
2023-07-07 11:23 ` [PATCH v2 2/3] iio: light: bu27008: add chip info Matti Vaittinen
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Matti Vaittinen @ 2023-07-07 11:22 UTC (permalink / raw)
To: Matti Vaittinen, Matti Vaittinen
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Matti Vaittinen, linux-iio,
devicetree, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2626 bytes --]
The ROHM BU27010 is a sensor with 6 photodiodes (red, green, blue, clear,
IR and flickering detection) with five configurable channels. Red, green
and flickering detection being always available and two out of the rest
three (blue, clear, IR) can be selected to be simultaneously measured.
Typical application is adjusting LCD/OLED backlight of TVs, mobile phones
and tablet PCs.
Add binding document for ROHM BU27010.
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
Revision history:
v1 => v2:
- Drop trailing blank line
- Require VDD-supply
- Fix subject prefix
---
.../bindings/iio/light/rohm,bu27010.yaml | 49 +++++++++++++++++++
1 file changed, 49 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bu27010.yaml
diff --git a/Documentation/devicetree/bindings/iio/light/rohm,bu27010.yaml b/Documentation/devicetree/bindings/iio/light/rohm,bu27010.yaml
new file mode 100644
index 000000000000..8376d64a641a
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/rohm,bu27010.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/rohm,bu27010.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ROHM BU27010 color sensor
+
+maintainers:
+ - Matti Vaittinen <mazziesaccount@gmail.com>
+
+description: |
+ The ROHM BU27010 is a sensor with 6 photodiodes (red, green, blue, clear,
+ IR and flickering detection) with five configurable channels. Red, green
+ and flickering detection being always available and two out of the rest
+ three (blue, clear, IR) can be selected to be simultaneously measured.
+ Typical application is adjusting LCD/OLED backlight of TVs, mobile phones
+ and tablet PCs.
+
+properties:
+ compatible:
+ const: rohm,bu27010
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ vdd-supply: true
+
+required:
+ - compatible
+ - reg
+ - vdd-supply
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ light-sensor@38 {
+ compatible = "rohm,bu27010";
+ reg = <0x38>;
+ };
+ };
--
2.40.1
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 1/3] dt-bindings: iio: ROHM BU27010 RGBC + flickering sensor
2023-07-07 11:22 ` [PATCH v2 1/3] dt-bindings: iio: ROHM BU27010 RGBC + flickering sensor Matti Vaittinen
@ 2023-07-10 9:05 ` Krzysztof Kozlowski
0 siblings, 0 replies; 8+ messages in thread
From: Krzysztof Kozlowski @ 2023-07-10 9:05 UTC (permalink / raw)
To: Matti Vaittinen, Matti Vaittinen
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-iio, devicetree,
linux-kernel
On 07/07/2023 13:22, Matti Vaittinen wrote:
> The ROHM BU27010 is a sensor with 6 photodiodes (red, green, blue, clear,
> IR and flickering detection) with five configurable channels. Red, green
> and flickering detection being always available and two out of the rest
> three (blue, clear, IR) can be selected to be simultaneously measured.
> Typical application is adjusting LCD/OLED backlight of TVs, mobile phones
> and tablet PCs.
>
> Add binding document for ROHM BU27010.
>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/3] iio: light: bu27008: add chip info
2023-07-07 11:22 [PATCH v2 0/3] Support ROHM BU27010 RGBC sensor Matti Vaittinen
2023-07-07 11:22 ` [PATCH v2 1/3] dt-bindings: iio: ROHM BU27010 RGBC + flickering sensor Matti Vaittinen
@ 2023-07-07 11:23 ` Matti Vaittinen
2023-07-08 16:16 ` Jonathan Cameron
2023-07-07 11:23 ` [PATCH v2 3/3] iio: light: bd27008: Support BD27010 RGB Matti Vaittinen
2023-07-08 16:06 ` [PATCH v2 0/3] Support ROHM BU27010 RGBC sensor Jonathan Cameron
3 siblings, 1 reply; 8+ messages in thread
From: Matti Vaittinen @ 2023-07-07 11:23 UTC (permalink / raw)
To: Matti Vaittinen, Matti Vaittinen
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Matti Vaittinen, linux-iio,
devicetree, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 16425 bytes --]
The ROHM BU27010 RGB + flickering sensor is in many regards similar to
the BU27008. Prepare for adding support for BU27010 by allowing
chip-specific properties to be brought from the of_device_id data.
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
Revision history:
v1 => v2:
- Move all generic refactoring from next patch to this one so that the
next one will only contain the BU27010 specific additions.
- reorder bu27008 chip-data assignments to match the struct member
placement
- Cleanup
---
drivers/iio/light/rohm-bu27008.c | 323 ++++++++++++++++++++-----------
1 file changed, 208 insertions(+), 115 deletions(-)
diff --git a/drivers/iio/light/rohm-bu27008.c b/drivers/iio/light/rohm-bu27008.c
index b50bf8973d9a..08e2b1194bf4 100644
--- a/drivers/iio/light/rohm-bu27008.c
+++ b/drivers/iio/light/rohm-bu27008.c
@@ -211,7 +211,35 @@ static const struct iio_chan_spec bu27008_channels[] = {
IIO_CHAN_SOFT_TIMESTAMP(BU27008_NUM_CHANS),
};
+struct bu27008_data;
+
+struct bu27_chip_data {
+ const char *name;
+ int (*chip_init)(struct bu27008_data *data);
+ int (*get_gain_sel)(struct bu27008_data *data, int *sel);
+ int (*write_gain_sel)(struct bu27008_data *data, int sel);
+ const struct regmap_config *regmap_cfg;
+ const struct iio_gain_sel_pair *gains;
+ const struct iio_gain_sel_pair *gains_ir;
+ const struct iio_itime_sel_mul *itimes;
+ int num_gains;
+ int num_gains_ir;
+ int num_itimes;
+ int scale1x;
+
+ int drdy_en_reg;
+ int drdy_en_mask;
+ int meas_en_reg;
+ int meas_en_mask;
+ int valid_reg;
+ int chan_sel_reg;
+ int chan_sel_mask;
+ int int_time_mask;
+ u8 part_id;
+};
+
struct bu27008_data {
+ const struct bu27_chip_data *cd;
struct regmap *regmap;
struct iio_trigger *trig;
struct device *dev;
@@ -282,51 +310,6 @@ static const struct regmap_config bu27008_regmap = {
.disable_locking = true,
};
-#define BU27008_MAX_VALID_RESULT_WAIT_US 50000
-#define BU27008_VALID_RESULT_WAIT_QUANTA_US 1000
-
-static int bu27008_chan_read_data(struct bu27008_data *data, int reg, int *val)
-{
- int ret, valid;
- __le16 tmp;
-
- ret = regmap_read_poll_timeout(data->regmap, BU27008_REG_MODE_CONTROL3,
- valid, (valid & BU27008_MASK_VALID),
- BU27008_VALID_RESULT_WAIT_QUANTA_US,
- BU27008_MAX_VALID_RESULT_WAIT_US);
- if (ret)
- return ret;
-
- ret = regmap_bulk_read(data->regmap, reg, &tmp, sizeof(tmp));
- if (ret)
- dev_err(data->dev, "Reading channel data failed\n");
-
- *val = le16_to_cpu(tmp);
-
- return ret;
-}
-
-static int bu27008_get_gain(struct bu27008_data *data, struct iio_gts *gts, int *gain)
-{
- int ret, sel;
-
- ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL2, &sel);
- if (ret)
- return ret;
-
- sel = FIELD_GET(BU27008_MASK_RGBC_GAIN, sel);
-
- ret = iio_gts_find_gain_by_sel(gts, sel);
- if (ret < 0) {
- dev_err(data->dev, "unknown gain value 0x%x\n", sel);
- return ret;
- }
-
- *gain = ret;
-
- return 0;
-}
-
static int bu27008_write_gain_sel(struct bu27008_data *data, int sel)
{
int regval;
@@ -368,6 +351,123 @@ static int bu27008_write_gain_sel(struct bu27008_data *data, int sel)
BU27008_MASK_RGBC_GAIN, regval);
}
+static int bu27008_get_gain_sel(struct bu27008_data *data, int *sel)
+{
+ int ret;
+
+ /*
+ * If we always "lock" the gain selectors for all channels to prevent
+ * unsupported configs, then it does not matter which channel is used
+ * we can just return selector from any of them.
+ *
+ * This, however is not true if we decide to support only 4X and 16X
+ * and then individual gains for channels. Currently this is not the
+ * case.
+ *
+ * If we some day decide to support individual gains, then we need to
+ * have channel information here.
+ */
+
+ ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL2, sel);
+ if (ret)
+ return ret;
+
+ *sel = FIELD_GET(BU27008_MASK_RGBC_GAIN, *sel);
+
+ return 0;
+}
+
+static int bu27008_chip_init(struct bu27008_data *data)
+{
+ int ret;
+
+ ret = regmap_write_bits(data->regmap, BU27008_REG_SYSTEM_CONTROL,
+ BU27008_MASK_SW_RESET, BU27008_MASK_SW_RESET);
+ if (ret)
+ return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
+
+ /*
+ * The data-sheet does not tell how long performing the IC reset takes.
+ * However, the data-sheet says the minimum time it takes the IC to be
+ * able to take inputs after power is applied, is 100 uS. I'd assume
+ * > 1 mS is enough.
+ */
+ msleep(1);
+
+ ret = regmap_reinit_cache(data->regmap, data->cd->regmap_cfg);
+ if (ret)
+ dev_err(data->dev, "Failed to reinit reg cache\n");
+
+ return ret;
+}
+
+static const struct bu27_chip_data bu27008_chip = {
+ .name = "bu27008",
+ .chip_init = bu27008_chip_init,
+ .get_gain_sel = bu27008_get_gain_sel,
+ .write_gain_sel = bu27008_write_gain_sel,
+ .regmap_cfg = &bu27008_regmap,
+ .gains = &bu27008_gains[0],
+ .gains_ir = &bu27008_gains_ir[0],
+ .itimes = &bu27008_itimes[0],
+ .num_gains = ARRAY_SIZE(bu27008_gains),
+ .num_gains_ir = ARRAY_SIZE(bu27008_gains_ir),
+ .num_itimes = ARRAY_SIZE(bu27008_itimes),
+ .scale1x = BU27008_SCALE_1X,
+ .drdy_en_reg = BU27008_REG_MODE_CONTROL3,
+ .drdy_en_mask = BU27008_MASK_INT_EN,
+ .valid_reg = BU27008_REG_MODE_CONTROL3,
+ .meas_en_reg = BU27008_REG_MODE_CONTROL3,
+ .meas_en_mask = BU27008_MASK_MEAS_EN,
+ .chan_sel_reg = BU27008_REG_MODE_CONTROL3,
+ .chan_sel_mask = BU27008_MASK_CHAN_SEL,
+ .int_time_mask = BU27008_MASK_MEAS_MODE,
+ .part_id = BU27008_ID,
+};
+
+#define BU27008_MAX_VALID_RESULT_WAIT_US 50000
+#define BU27008_VALID_RESULT_WAIT_QUANTA_US 1000
+
+static int bu27008_chan_read_data(struct bu27008_data *data, int reg, int *val)
+{
+ int ret, valid;
+ __le16 tmp;
+
+ ret = regmap_read_poll_timeout(data->regmap, data->cd->valid_reg,
+ valid, (valid & BU27008_MASK_VALID),
+ BU27008_VALID_RESULT_WAIT_QUANTA_US,
+ BU27008_MAX_VALID_RESULT_WAIT_US);
+ if (ret)
+ return ret;
+
+ ret = regmap_bulk_read(data->regmap, reg, &tmp, sizeof(tmp));
+ if (ret)
+ dev_err(data->dev, "Reading channel data failed\n");
+
+ *val = le16_to_cpu(tmp);
+
+ return ret;
+}
+
+static int bu27008_get_gain(struct bu27008_data *data, struct iio_gts *gts, int *gain)
+{
+ int ret, sel;
+
+ ret = data->cd->get_gain_sel(data, &sel);
+ if (ret)
+ return ret;
+
+ ret = iio_gts_find_gain_by_sel(gts, sel);
+ if (ret < 0) {
+ dev_err(data->dev, "unknown gain value 0x%x\n", sel);
+ return ret;
+ }
+
+ *gain = ret;
+
+ return 0;
+}
+
static int bu27008_set_gain(struct bu27008_data *data, int gain)
{
int ret;
@@ -376,7 +476,7 @@ static int bu27008_set_gain(struct bu27008_data *data, int gain)
if (ret < 0)
return ret;
- return bu27008_write_gain_sel(data, ret);
+ return data->cd->write_gain_sel(data, ret);
}
static int bu27008_get_int_time_sel(struct bu27008_data *data, int *sel)
@@ -384,15 +484,23 @@ static int bu27008_get_int_time_sel(struct bu27008_data *data, int *sel)
int ret, val;
ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL1, &val);
- *sel = FIELD_GET(BU27008_MASK_MEAS_MODE, val);
+ if (ret)
+ return ret;
- return ret;
+ val &= data->cd->int_time_mask;
+ val >>= ffs(data->cd->int_time_mask) - 1;
+
+ *sel = val;
+
+ return 0;
}
static int bu27008_set_int_time_sel(struct bu27008_data *data, int sel)
{
+ sel <<= ffs(data->cd->int_time_mask) - 1;
+
return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL1,
- BU27008_MASK_MEAS_MODE, sel);
+ data->cd->int_time_mask, sel);
}
static int bu27008_get_int_time_us(struct bu27008_data *data)
@@ -448,8 +556,7 @@ static int bu27008_set_int_time(struct bu27008_data *data, int time)
if (ret < 0)
return ret;
- return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL1,
- BU27008_MASK_MEAS_MODE, ret);
+ return bu27008_set_int_time_sel(data, ret);
}
/* Try to change the time so that the scale is maintained */
@@ -527,10 +634,13 @@ static int bu27008_try_set_int_time(struct bu27008_data *data, int int_time_new)
return ret;
}
-static int bu27008_meas_set(struct bu27008_data *data, int state)
+static int bu27008_meas_set(struct bu27008_data *data, bool enable)
{
- return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
- BU27008_MASK_MEAS_EN, state);
+ if (enable)
+ return regmap_set_bits(data->regmap, data->cd->meas_en_reg,
+ data->cd->meas_en_mask);
+ return regmap_clear_bits(data->regmap, data->cd->meas_en_reg,
+ data->cd->meas_en_mask);
}
static int bu27008_chan_cfg(struct bu27008_data *data,
@@ -543,9 +653,15 @@ static int bu27008_chan_cfg(struct bu27008_data *data,
else
chan_sel = BU27008_CLEAR2_IR3;
- chan_sel = FIELD_PREP(BU27008_MASK_CHAN_SEL, chan_sel);
+ /*
+ * prepare bitfield for channel sel. The FIELD_PREP works only when
+ * mask is constant. In our case the mask is assigned based on the
+ * chip type. Hence the open-coded FIELD_PREP here. We don't bother
+ * zeroing the irrelevant bits though - update_bits takes care of that.
+ */
+ chan_sel <<= ffs(data->cd->chan_sel_mask) - 1;
- return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
+ return regmap_update_bits(data->regmap, data->cd->chan_sel_reg,
BU27008_MASK_CHAN_SEL, chan_sel);
}
@@ -558,7 +674,7 @@ static int bu27008_read_one(struct bu27008_data *data, struct iio_dev *idev,
if (ret)
return ret;
- ret = bu27008_meas_set(data, BU27008_MEAS_EN);
+ ret = bu27008_meas_set(data, true);
if (ret)
return ret;
@@ -574,7 +690,7 @@ static int bu27008_read_one(struct bu27008_data *data, struct iio_dev *idev,
if (!ret)
ret = IIO_VAL_INT;
- if (bu27008_meas_set(data, BU27008_MEAS_DIS))
+ if (bu27008_meas_set(data, false))
dev_warn(data->dev, "measurement disabling failed\n");
return ret;
@@ -669,7 +785,7 @@ static int bu27008_set_scale(struct bu27008_data *data,
goto unlock_out;
}
- ret = bu27008_write_gain_sel(data, gain_sel);
+ ret = data->cd->write_gain_sel(data, gain_sel);
unlock_out:
mutex_unlock(&data->mutex);
@@ -762,10 +878,10 @@ static int bu27008_update_scan_mode(struct iio_dev *idev,
chan_sel = BU27008_CLEAR2_IR3;
}
- chan_sel = FIELD_PREP(BU27008_MASK_CHAN_SEL, chan_sel);
+ chan_sel <<= ffs(data->cd->chan_sel_mask) - 1;
- return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
- BU27008_MASK_CHAN_SEL, chan_sel);
+ return regmap_update_bits(data->regmap, data->cd->chan_sel_reg,
+ data->cd->chan_sel_mask, chan_sel);
}
static const struct iio_info bu27008_info = {
@@ -777,46 +893,18 @@ static const struct iio_info bu27008_info = {
.validate_trigger = iio_validate_own_trigger,
};
-static int bu27008_chip_init(struct bu27008_data *data)
-{
- int ret;
-
- ret = regmap_write_bits(data->regmap, BU27008_REG_SYSTEM_CONTROL,
- BU27008_MASK_SW_RESET, BU27008_MASK_SW_RESET);
- if (ret)
- return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
-
- /*
- * The data-sheet does not tell how long performing the IC reset takes.
- * However, the data-sheet says the minimum time it takes the IC to be
- * able to take inputs after power is applied, is 100 uS. I'd assume
- * > 1 mS is enough.
- */
- msleep(1);
-
- ret = regmap_reinit_cache(data->regmap, &bu27008_regmap);
- if (ret)
- dev_err(data->dev, "Failed to reinit reg cache\n");
-
- return ret;
-}
-
-static int bu27008_set_drdy_irq(struct bu27008_data *data, int state)
-{
- return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
- BU27008_MASK_INT_EN, state);
-}
-
-static int bu27008_trigger_set_state(struct iio_trigger *trig,
- bool state)
+static int bu27008_trigger_set_state(struct iio_trigger *trig, bool state)
{
struct bu27008_data *data = iio_trigger_get_drvdata(trig);
int ret;
+
if (state)
- ret = bu27008_set_drdy_irq(data, BU27008_INT_EN);
+ ret = regmap_set_bits(data->regmap, data->cd->drdy_en_reg,
+ data->cd->drdy_en_mask);
else
- ret = bu27008_set_drdy_irq(data, BU27008_INT_DIS);
+ ret = regmap_clear_bits(data->regmap, data->cd->drdy_en_reg,
+ data->cd->drdy_en_mask);
if (ret)
dev_err(data->dev, "Failed to set trigger state\n");
@@ -852,7 +940,7 @@ static irqreturn_t bu27008_trigger_handler(int irq, void *p)
* After some measurements, it seems reading the
* BU27008_REG_MODE_CONTROL3 debounces the IRQ line
*/
- ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL3, &dummy);
+ ret = regmap_read(data->regmap, data->cd->valid_reg, &dummy);
if (ret < 0)
goto err_read;
@@ -872,14 +960,14 @@ static int bu27008_buffer_preenable(struct iio_dev *idev)
{
struct bu27008_data *data = iio_priv(idev);
- return bu27008_meas_set(data, BU27008_MEAS_EN);
+ return bu27008_meas_set(data, true);
}
static int bu27008_buffer_postdisable(struct iio_dev *idev)
{
struct bu27008_data *data = iio_priv(idev);
- return bu27008_meas_set(data, BU27008_MEAS_DIS);
+ return bu27008_meas_set(data, false);
}
static const struct iio_buffer_setup_ops bu27008_buffer_ops = {
@@ -952,11 +1040,6 @@ static int bu27008_probe(struct i2c_client *i2c)
struct iio_dev *idev;
int ret;
- regmap = devm_regmap_init_i2c(i2c, &bu27008_regmap);
- if (IS_ERR(regmap))
- return dev_err_probe(dev, PTR_ERR(regmap),
- "Failed to initialize Regmap\n");
-
idev = devm_iio_device_alloc(dev, sizeof(*data));
if (!idev)
return -ENOMEM;
@@ -967,24 +1050,34 @@ static int bu27008_probe(struct i2c_client *i2c)
data = iio_priv(idev);
+ data->cd = device_get_match_data(&i2c->dev);
+ if (!data->cd)
+ return -ENODEV;
+
+ regmap = devm_regmap_init_i2c(i2c, data->cd->regmap_cfg);
+ if (IS_ERR(regmap))
+ return dev_err_probe(dev, PTR_ERR(regmap),
+ "Failed to initialize Regmap\n");
+
+
ret = regmap_read(regmap, BU27008_REG_SYSTEM_CONTROL, ®);
if (ret)
return dev_err_probe(dev, ret, "Failed to access sensor\n");
part_id = FIELD_GET(BU27008_MASK_PART_ID, reg);
- if (part_id != BU27008_ID)
+ if (part_id != data->cd->part_id)
dev_warn(dev, "unknown device 0x%x\n", part_id);
- ret = devm_iio_init_iio_gts(dev, BU27008_SCALE_1X, 0, bu27008_gains,
- ARRAY_SIZE(bu27008_gains), bu27008_itimes,
- ARRAY_SIZE(bu27008_itimes), &data->gts);
+ ret = devm_iio_init_iio_gts(dev, data->cd->scale1x, 0, data->cd->gains,
+ data->cd->num_gains, data->cd->itimes,
+ data->cd->num_itimes, &data->gts);
if (ret)
return ret;
- ret = devm_iio_init_iio_gts(dev, BU27008_SCALE_1X, 0, bu27008_gains_ir,
- ARRAY_SIZE(bu27008_gains_ir), bu27008_itimes,
- ARRAY_SIZE(bu27008_itimes), &data->gts_ir);
+ ret = devm_iio_init_iio_gts(dev, data->cd->scale1x, 0, data->cd->gains_ir,
+ data->cd->num_gains_ir, data->cd->itimes,
+ data->cd->num_itimes, &data->gts_ir);
if (ret)
return ret;
@@ -993,14 +1086,14 @@ static int bu27008_probe(struct i2c_client *i2c)
data->dev = dev;
data->irq = i2c->irq;
- idev->channels = bu27008_channels;
+ idev->channels = &bu27008_channels[0];
idev->num_channels = ARRAY_SIZE(bu27008_channels);
- idev->name = "bu27008";
+ idev->name = data->cd->name;
idev->info = &bu27008_info;
idev->modes = INDIO_DIRECT_MODE;
idev->available_scan_masks = bu27008_scan_masks;
- ret = bu27008_chip_init(data);
+ ret = data->cd->chip_init(data);
if (ret)
return ret;
@@ -1021,7 +1114,7 @@ static int bu27008_probe(struct i2c_client *i2c)
}
static const struct of_device_id bu27008_of_match[] = {
- { .compatible = "rohm,bu27008" },
+ { .compatible = "rohm,bu27008", .data = &bu27008_chip },
{ }
};
MODULE_DEVICE_TABLE(of, bu27008_of_match);
--
2.40.1
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 2/3] iio: light: bu27008: add chip info
2023-07-07 11:23 ` [PATCH v2 2/3] iio: light: bu27008: add chip info Matti Vaittinen
@ 2023-07-08 16:16 ` Jonathan Cameron
0 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2023-07-08 16:16 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Matti Vaittinen, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-iio, devicetree,
linux-kernel
On Fri, 7 Jul 2023 14:23:03 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> The ROHM BU27010 RGB + flickering sensor is in many regards similar to
> the BU27008. Prepare for adding support for BU27010 by allowing
> chip-specific properties to be brought from the of_device_id data.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>
One trivial comment.
The ffs stuff is a pain. There have been a few goes at defining
standard functions for doing that but I don' think anything ever landed
upstream.
> ---
> Revision history:
> v1 => v2:
> - Move all generic refactoring from next patch to this one so that the
> next one will only contain the BU27010 specific additions.
> - reorder bu27008 chip-data assignments to match the struct member
> placement
> - Cleanup
> ---
> drivers/iio/light/rohm-bu27008.c | 323 ++++++++++++++++++++-----------
> 1 file changed, 208 insertions(+), 115 deletions(-)
>
> diff --git a/drivers/iio/light/rohm-bu27008.c b/drivers/iio/light/rohm-bu27008.c
> index b50bf8973d9a..08e2b1194bf4 100644
> --- a/drivers/iio/light/rohm-bu27008.c
> +++ b/drivers/iio/light/rohm-bu27008.c
> @@ -211,7 +211,35 @@ static const struct iio_chan_spec bu27008_channels[] = {
> @@ -993,14 +1086,14 @@ static int bu27008_probe(struct i2c_client *i2c)
> data->dev = dev;
> data->irq = i2c->irq;
>
> - idev->channels = bu27008_channels;
> + idev->channels = &bu27008_channels[0];
Why? When setting a pointer to the 'whole' array I tend to prefer
the original. Can't see a reason why this should be changed to the
second one.
> idev->num_channels = ARRAY_SIZE(bu27008_channels);
> - idev->name = "bu27008";
> + idev->name = data->cd->name;
> idev->info = &bu27008_info;
> idev->modes = INDIO_DIRECT_MODE;
> idev->available_scan_masks = bu27008_scan_masks;
>
> - ret = bu27008_chip_init(data);
> + ret = data->cd->chip_init(data);
> if (ret)
> return ret;
>
> @@ -1021,7 +1114,7 @@ static int bu27008_probe(struct i2c_client *i2c)
> }
>
> static const struct of_device_id bu27008_of_match[] = {
> - { .compatible = "rohm,bu27008" },
> + { .compatible = "rohm,bu27008", .data = &bu27008_chip },
> { }
> };
> MODULE_DEVICE_TABLE(of, bu27008_of_match);
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 3/3] iio: light: bd27008: Support BD27010 RGB
2023-07-07 11:22 [PATCH v2 0/3] Support ROHM BU27010 RGBC sensor Matti Vaittinen
2023-07-07 11:22 ` [PATCH v2 1/3] dt-bindings: iio: ROHM BU27010 RGBC + flickering sensor Matti Vaittinen
2023-07-07 11:23 ` [PATCH v2 2/3] iio: light: bu27008: add chip info Matti Vaittinen
@ 2023-07-07 11:23 ` Matti Vaittinen
2023-07-08 16:21 ` Jonathan Cameron
2023-07-08 16:06 ` [PATCH v2 0/3] Support ROHM BU27010 RGBC sensor Jonathan Cameron
3 siblings, 1 reply; 8+ messages in thread
From: Matti Vaittinen @ 2023-07-07 11:23 UTC (permalink / raw)
To: Matti Vaittinen, Matti Vaittinen
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Matti Vaittinen, linux-iio,
devicetree, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 17806 bytes --]
The ROHM BU27010 is an RGBC sensor with a flickering detection FIFO. The
RGBC+IR sensor functionality is largely similar to what the BU27008 has.
There are some notable things though:
- gain setting is once again new and exotic. Now, there is 6bit gain
setting where 4 of the bits are common to all channels and 2 bits
can be configured separately for each channel. The BU27010 has
similar "1X on other channels vs 2X on IR when selector is 0x0"
gain design as BU27008 had. So, we use same gain setting policy for
BU27010 as we did for BU27008 - driver sets same gain selector for all
channels but shows the gains separately for all channels so users
can (at least in theory) detect this 1X vs 2X madness...
- BU27010 has suffled all the control register bitfields to new
addresses and bit positions while still keeping the register naming
same.
- Some more power/reset control is added.
- FIFO for "flickering detection" is added.
The control register suffling made this slightly nasty. Still, it is
easier for maintenance perspective to add the BU27010 support in BU27008
driver because - even though the bit positions/addresses were changed -
most of the driver structure can be re-used. Writing own driver for
BU27010 would mean plenty of duplicate code albeit a tad more clarity.
The flickering FIFO is not supported by the driver.
Add BU27010 RGBC+IR support to rohm-bu27008 driver.
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
Revision history:
v1 => v2:
- refactor changes so that this patch only adds BU27010 functionality
- re-order bu27101 chip-data initialization
This is my 4.th re-write. My first idea was to write separate drivers
for the BU27008 and BU27010. I abandoned this as I ended up copying most
of the functions and just changing some of the bit offsets / register
addresses. This felt like a plenty of copy-paste.
So, 2.nd attempt was to combine BU27008 and BU27010. I hit the wall
as I tried to support setting RGBC / IR gains independently for BU27008
and setting the gain independently for each channel on BU27010 when
gains were such that the HW could support them independently. This went
to complete madness.
This is when I asked for a second opinion - which was to clear the mess
by separating the BU27008 and BU27010 in own drivers. After a while it
become obvious that supporting independet gains for the channels when HW
supported it required decisions which could not predictably be done for
all users. There is a comment explaining some of this in
bu27008_write_gain_sel().
After the complexity of shared gain bits was killed, the two separate
drivers started to look like a copy-paste again - and all the bugs had
to be fixed in two places... As a result I again pulled it all in one
driver. As a result of this experimenting, I firmly believe supporting
both ICs in the same driver is a correct design.
---
drivers/iio/light/rohm-bu27008.c | 303 ++++++++++++++++++++++++++++++-
1 file changed, 301 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/light/rohm-bu27008.c b/drivers/iio/light/rohm-bu27008.c
index 08e2b1194bf4..a88edc373d9b 100644
--- a/drivers/iio/light/rohm-bu27008.c
+++ b/drivers/iio/light/rohm-bu27008.c
@@ -1,6 +1,8 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
- * BU27008 ROHM Colour Sensor
+ * ROHM Colour Sensor driver for
+ * - BU27008 RGBC sensor
+ * - BU27010 RGBC + Flickering sensor
*
* Copyright (c) 2023, ROHM Semiconductor.
*/
@@ -22,6 +24,25 @@
#include <linux/iio/trigger_consumer.h>
#include <linux/iio/triggered_buffer.h>
+/*
+ * A word about register address and mask definitions.
+ *
+ * At a quick glance to the data-sheet register tables, the BU27010 has all the
+ * registers that the BU27008 has. On top of that the BU27010 adds couple of new
+ * ones.
+ *
+ * So, all definitions BU27008_REG_* are there also for BU27010 but none of the
+ * BU27010_REG_* are present on BU27008. This makes sense as BU27010 just adds
+ * some features (Flicker FIFO, more power control) on top of the BU27008.
+ *
+ * Unfortunately, some of the wheel has been re-invented. Even though the names
+ * of the registers have stayed the same, pretty much all of the functionality
+ * provided by the registers has changed place. Contents of all MODE_CONTROL
+ * registers on BU27008 and BU27010 are different.
+ *
+ * Chip-specific mapping from register addresses/bits to functionality is done
+ * in bu27_chip_data structures.
+ */
#define BU27008_REG_SYSTEM_CONTROL 0x40
#define BU27008_MASK_SW_RESET BIT(7)
#define BU27008_MASK_PART_ID GENMASK(5, 0)
@@ -52,6 +73,56 @@
#define BU27008_REG_MANUFACTURER_ID 0x92
#define BU27008_REG_MAX BU27008_REG_MANUFACTURER_ID
+/* BU27010 specific definitions */
+
+#define BU27010_MASK_SW_RESET BIT(7)
+#define BU27010_ID 0x1b
+#define BU27010_REG_POWER 0x3e
+#define BU27010_MASK_POWER BIT(0)
+
+#define BU27010_REG_RESET 0x3f
+#define BU27010_MASK_RESET BIT(0)
+#define BU27010_RESET_RELEASE BU27010_MASK_RESET
+
+#define BU27010_MASK_MEAS_EN BIT(1)
+
+#define BU27010_MASK_CHAN_SEL GENMASK(7, 6)
+#define BU27010_MASK_MEAS_MODE GENMASK(5, 4)
+#define BU27010_MASK_RGBC_GAIN GENMASK(3, 0)
+
+#define BU27010_MASK_DATA3_GAIN GENMASK(7, 6)
+#define BU27010_MASK_DATA2_GAIN GENMASK(5, 4)
+#define BU27010_MASK_DATA1_GAIN GENMASK(3, 2)
+#define BU27010_MASK_DATA0_GAIN GENMASK(1, 0)
+
+#define BU27010_MASK_FLC_MODE BIT(7)
+#define BU27010_MASK_FLC_GAIN GENMASK(4, 0)
+
+#define BU27010_REG_MODE_CONTROL4 0x44
+/* If flicker is ever to be supported the IRQ must be handled as a field */
+#define BU27010_IRQ_DIS_ALL GENMASK(1, 0)
+#define BU27010_DRDY_EN BIT(0)
+#define BU27010_MASK_INT_SEL GENMASK(1, 0)
+
+#define BU27010_REG_MODE_CONTROL5 0x45
+#define BU27010_MASK_RGB_VALID BIT(7)
+#define BU27010_MASK_FLC_VALID BIT(6)
+#define BU27010_MASK_WAIT_EN BIT(3)
+#define BU27010_MASK_FIFO_EN BIT(2)
+#define BU27010_MASK_RGB_EN BIT(1)
+#define BU27010_MASK_FLC_EN BIT(0)
+
+#define BU27010_REG_DATA_FLICKER_LO 0x56
+#define BU27010_MASK_DATA_FLICKER_HI GENMASK(2, 0)
+#define BU27010_REG_FLICKER_COUNT 0x5a
+#define BU27010_REG_FIFO_LEVEL_LO 0x5b
+#define BU27010_MASK_FIFO_LEVEL_HI BIT(0)
+#define BU27010_REG_FIFO_DATA_LO 0x5d
+#define BU27010_REG_FIFO_DATA_HI 0x5e
+#define BU27010_MASK_FIFO_DATA_HI GENMASK(2, 0)
+#define BU27010_REG_MANUFACTURER_ID 0x92
+#define BU27010_REG_MAX BU27010_REG_MANUFACTURER_ID
+
/**
* enum bu27008_chan_type - BU27008 channel types
* @BU27008_RED: Red channel. Always via data0.
@@ -117,6 +188,17 @@ static const unsigned long bu27008_scan_masks[] = {
*/
#define BU27008_SCALE_1X 16
+/*
+ * On BU27010 available scales with gain 1x - 4096x,
+ * timings 55, 100, 200, 400 mS. Time impacts to gain: 1x, 2x, 4x, 8x.
+ *
+ * => Max total gain is HWGAIN * gain by integration time (8 * 4096)
+ *
+ * Using NANO precision for scale we must use scale 64x corresponding gain 1x
+ * to avoid precision loss.
+ */
+#define BU27010_SCALE_1X 64
+
/* See the data sheet for the "Gain Setting" table */
#define BU27008_GSEL_1X 0x00
#define BU27008_GSEL_4X 0x08
@@ -152,10 +234,44 @@ static const struct iio_gain_sel_pair bu27008_gains_ir[] = {
GAIN_SCALE_GAIN(1024, BU27008_GSEL_1024X),
};
+#define BU27010_GSEL_1X 0x00 /* 000000 */
+#define BU27010_GSEL_4X 0x08 /* 001000 */
+#define BU27010_GSEL_16X 0x09 /* 001001 */
+#define BU27010_GSEL_64X 0x0e /* 001110 */
+#define BU27010_GSEL_256X 0x1e /* 011110 */
+#define BU27010_GSEL_1024X 0x2e /* 101110 */
+#define BU27010_GSEL_4096X 0x3f /* 111111 */
+
+static const struct iio_gain_sel_pair bu27010_gains[] = {
+ GAIN_SCALE_GAIN(1, BU27010_GSEL_1X),
+ GAIN_SCALE_GAIN(4, BU27010_GSEL_4X),
+ GAIN_SCALE_GAIN(16, BU27010_GSEL_16X),
+ GAIN_SCALE_GAIN(64, BU27010_GSEL_64X),
+ GAIN_SCALE_GAIN(256, BU27010_GSEL_256X),
+ GAIN_SCALE_GAIN(1024, BU27010_GSEL_1024X),
+ GAIN_SCALE_GAIN(4096, BU27010_GSEL_4096X),
+};
+
+static const struct iio_gain_sel_pair bu27010_gains_ir[] = {
+ GAIN_SCALE_GAIN(2, BU27010_GSEL_1X),
+ GAIN_SCALE_GAIN(4, BU27010_GSEL_4X),
+ GAIN_SCALE_GAIN(16, BU27010_GSEL_16X),
+ GAIN_SCALE_GAIN(64, BU27010_GSEL_64X),
+ GAIN_SCALE_GAIN(256, BU27010_GSEL_256X),
+ GAIN_SCALE_GAIN(1024, BU27010_GSEL_1024X),
+ GAIN_SCALE_GAIN(4096, BU27010_GSEL_4096X),
+};
+
#define BU27008_MEAS_MODE_100MS 0x00
#define BU27008_MEAS_MODE_55MS 0x01
#define BU27008_MEAS_MODE_200MS 0x02
#define BU27008_MEAS_MODE_400MS 0x04
+
+#define BU27010_MEAS_MODE_100MS 0x00
+#define BU27010_MEAS_MODE_55MS 0x03
+#define BU27010_MEAS_MODE_200MS 0x01
+#define BU27010_MEAS_MODE_400MS 0x02
+
#define BU27008_MEAS_TIME_MAX_MS 400
static const struct iio_itime_sel_mul bu27008_itimes[] = {
@@ -165,6 +281,13 @@ static const struct iio_itime_sel_mul bu27008_itimes[] = {
GAIN_SCALE_ITIME_US(55000, BU27008_MEAS_MODE_55MS, 1),
};
+static const struct iio_itime_sel_mul bu27010_itimes[] = {
+ GAIN_SCALE_ITIME_US(400000, BU27010_MEAS_MODE_400MS, 8),
+ GAIN_SCALE_ITIME_US(200000, BU27010_MEAS_MODE_200MS, 4),
+ GAIN_SCALE_ITIME_US(100000, BU27010_MEAS_MODE_100MS, 2),
+ GAIN_SCALE_ITIME_US(55000, BU27010_MEAS_MODE_55MS, 1),
+};
+
/*
* All the RGBC channels share the same gain.
* IR gain can be fine-tuned from the gain set for the RGBC by 2 bit, but this
@@ -268,11 +391,29 @@ static const struct regmap_range bu27008_volatile_ranges[] = {
},
};
+static const struct regmap_range bu27010_volatile_ranges[] = {
+ {
+ .range_min = BU27010_REG_RESET, /* RSTB */
+ .range_max = BU27008_REG_SYSTEM_CONTROL, /* RESET */
+ }, {
+ .range_min = BU27010_REG_MODE_CONTROL5, /* VALID bits */
+ .range_max = BU27010_REG_MODE_CONTROL5,
+ }, {
+ .range_min = BU27008_REG_DATA0_LO,
+ .range_max = BU27010_REG_FIFO_DATA_HI,
+ },
+};
+
static const struct regmap_access_table bu27008_volatile_regs = {
.yes_ranges = &bu27008_volatile_ranges[0],
.n_yes_ranges = ARRAY_SIZE(bu27008_volatile_ranges),
};
+static const struct regmap_access_table bu27010_volatile_regs = {
+ .yes_ranges = &bu27010_volatile_ranges[0],
+ .n_yes_ranges = ARRAY_SIZE(bu27010_volatile_ranges),
+};
+
static const struct regmap_range bu27008_read_only_ranges[] = {
{
.range_min = BU27008_REG_DATA0_LO,
@@ -283,11 +424,26 @@ static const struct regmap_range bu27008_read_only_ranges[] = {
},
};
+static const struct regmap_range bu27010_read_only_ranges[] = {
+ {
+ .range_min = BU27008_REG_DATA0_LO,
+ .range_max = BU27010_REG_FIFO_DATA_HI,
+ }, {
+ .range_min = BU27010_REG_MANUFACTURER_ID,
+ .range_max = BU27010_REG_MANUFACTURER_ID,
+ }
+};
+
static const struct regmap_access_table bu27008_ro_regs = {
.no_ranges = &bu27008_read_only_ranges[0],
.n_no_ranges = ARRAY_SIZE(bu27008_read_only_ranges),
};
+static const struct regmap_access_table bu27010_ro_regs = {
+ .no_ranges = &bu27010_read_only_ranges[0],
+ .n_no_ranges = ARRAY_SIZE(bu27010_read_only_ranges),
+};
+
static const struct regmap_config bu27008_regmap = {
.reg_bits = 8,
.val_bits = 8,
@@ -310,6 +466,17 @@ static const struct regmap_config bu27008_regmap = {
.disable_locking = true,
};
+static const struct regmap_config bu27010_regmap = {
+ .reg_bits = 8,
+ .val_bits = 8,
+
+ .max_register = BU27010_REG_MAX,
+ .cache_type = REGCACHE_RBTREE,
+ .volatile_table = &bu27010_volatile_regs,
+ .wr_table = &bu27010_ro_regs,
+ .disable_locking = true,
+};
+
static int bu27008_write_gain_sel(struct bu27008_data *data, int sel)
{
int regval;
@@ -351,6 +518,41 @@ static int bu27008_write_gain_sel(struct bu27008_data *data, int sel)
BU27008_MASK_RGBC_GAIN, regval);
}
+static int bu27010_write_gain_sel(struct bu27008_data *data, int sel)
+{
+ unsigned int regval;
+ int ret, chan_selector;
+
+ /*
+ * Gain 'selector' is composed of two registers. Selector is 6bit value,
+ * 4 high bits being the RGBC gain fieild in MODE_CONTROL1 register and
+ * two low bits being the channel specific gain in MODE_CONTROL2.
+ *
+ * Let's take the 4 high bits of whole 6 bit selector, and prepare
+ * the MODE_CONTROL1 value (RGBC gain part).
+ */
+ regval = FIELD_PREP(BU27010_MASK_RGBC_GAIN, (sel >> 2));
+
+ ret = regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL1,
+ BU27010_MASK_RGBC_GAIN, regval);
+ if (ret)
+ return ret;
+
+ /*
+ * Two low two bits of the selector must be written for all 4
+ * channels in the MODE_CONTROL2 register. Copy these two bits for
+ * all channels.
+ */
+ chan_selector = sel & GENMASK(1, 0);
+
+ regval = FIELD_PREP(BU27010_MASK_DATA0_GAIN, chan_selector);
+ regval |= FIELD_PREP(BU27010_MASK_DATA1_GAIN, chan_selector);
+ regval |= FIELD_PREP(BU27010_MASK_DATA2_GAIN, chan_selector);
+ regval |= FIELD_PREP(BU27010_MASK_DATA3_GAIN, chan_selector);
+
+ return regmap_write(data->regmap, BU27008_REG_MODE_CONTROL2, regval);
+}
+
static int bu27008_get_gain_sel(struct bu27008_data *data, int *sel)
{
int ret;
@@ -377,6 +579,30 @@ static int bu27008_get_gain_sel(struct bu27008_data *data, int *sel)
return 0;
}
+static int bu27010_get_gain_sel(struct bu27008_data *data, int *sel)
+{
+ int ret, tmp;
+
+ /*
+ * We always "lock" the gain selectors for all channels to prevent
+ * unsupported configs. It does not matter which channel is used
+ * we can just return selector from any of them.
+ */
+ ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL2, sel);
+ if (ret)
+ return ret;
+
+ *sel = FIELD_GET(BU27010_MASK_DATA0_GAIN, *sel);
+
+ ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL1, &tmp);
+ if (ret)
+ return ret;
+
+ *sel |= FIELD_GET(BU27010_MASK_RGBC_GAIN, tmp) << fls(BU27010_MASK_DATA0_GAIN);
+
+ return ret;
+}
+
static int bu27008_chip_init(struct bu27008_data *data)
{
int ret;
@@ -401,6 +627,78 @@ static int bu27008_chip_init(struct bu27008_data *data)
return ret;
}
+static int bu27010_chip_init(struct bu27008_data *data)
+{
+ int ret;
+
+ ret = regmap_write_bits(data->regmap, BU27008_REG_SYSTEM_CONTROL,
+ BU27010_MASK_SW_RESET, BU27010_MASK_SW_RESET);
+ if (ret)
+ return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
+
+ msleep(1);
+
+ /* Power ON*/
+ ret = regmap_write_bits(data->regmap, BU27010_REG_POWER,
+ BU27010_MASK_POWER, BU27010_MASK_POWER);
+ if (ret)
+ return dev_err_probe(data->dev, ret, "Sensor power-on failed\n");
+
+ msleep(1);
+
+ /* Release blocks from reset */
+ ret = regmap_write_bits(data->regmap, BU27010_REG_RESET,
+ BU27010_MASK_RESET, BU27010_RESET_RELEASE);
+ if (ret)
+ return dev_err_probe(data->dev, ret, "Sensor powering failed\n");
+
+ msleep(1);
+
+ /*
+ * The IRQ enabling on BU27010 is done in a peculiar way. The IRQ
+ * enabling is not a bit mask where individual IRQs could be enabled but
+ * a field which values are:
+ * 00 => IRQs disabled
+ * 01 => Data-ready (RGBC/IR)
+ * 10 => Data-ready (flicker)
+ * 11 => Flicker FIFO
+ *
+ * So, only one IRQ can be enabled at a time and enabling for example
+ * flicker FIFO would automagically disable data-ready IRQ.
+ *
+ * Currently the driver does not support the flicker. Hence, we can
+ * just treat the RGBC data-ready as single bit which can be enabled /
+ * disabled. This works for as long as the second bit in the field
+ * stays zero. Here we ensure it gets zeroed.
+ */
+ return regmap_clear_bits(data->regmap, BU27010_REG_MODE_CONTROL4,
+ BU27010_IRQ_DIS_ALL);
+}
+
+static const struct bu27_chip_data bu27010_chip = {
+ .name = "bu27010",
+ .chip_init = bu27010_chip_init,
+ .get_gain_sel = bu27010_get_gain_sel,
+ .write_gain_sel = bu27010_write_gain_sel,
+ .regmap_cfg = &bu27010_regmap,
+ .gains = &bu27010_gains[0],
+ .gains_ir = &bu27010_gains_ir[0],
+ .itimes = &bu27010_itimes[0],
+ .num_gains = ARRAY_SIZE(bu27010_gains),
+ .num_gains_ir = ARRAY_SIZE(bu27010_gains_ir),
+ .num_itimes = ARRAY_SIZE(bu27010_itimes),
+ .scale1x = BU27010_SCALE_1X,
+ .drdy_en_reg = BU27010_REG_MODE_CONTROL4,
+ .drdy_en_mask = BU27010_DRDY_EN,
+ .meas_en_reg = BU27010_REG_MODE_CONTROL5,
+ .meas_en_mask = BU27010_MASK_MEAS_EN,
+ .valid_reg = BU27010_REG_MODE_CONTROL5,
+ .chan_sel_reg = BU27008_REG_MODE_CONTROL1,
+ .chan_sel_mask = BU27010_MASK_CHAN_SEL,
+ .int_time_mask = BU27010_MASK_MEAS_MODE,
+ .part_id = BU27010_ID,
+};
+
static const struct bu27_chip_data bu27008_chip = {
.name = "bu27008",
.chip_init = bu27008_chip_init,
@@ -1115,6 +1413,7 @@ static int bu27008_probe(struct i2c_client *i2c)
static const struct of_device_id bu27008_of_match[] = {
{ .compatible = "rohm,bu27008", .data = &bu27008_chip },
+ { .compatible = "rohm,bu27010", .data = &bu27010_chip },
{ }
};
MODULE_DEVICE_TABLE(of, bu27008_of_match);
@@ -1129,7 +1428,7 @@ static struct i2c_driver bu27008_i2c_driver = {
};
module_i2c_driver(bu27008_i2c_driver);
-MODULE_DESCRIPTION("ROHM BU27008 colour sensor driver");
+MODULE_DESCRIPTION("ROHM BU27008 and BU27010 colour sensor driver");
MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
MODULE_LICENSE("GPL");
MODULE_IMPORT_NS(IIO_GTS_HELPER);
--
2.40.1
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 3/3] iio: light: bd27008: Support BD27010 RGB
2023-07-07 11:23 ` [PATCH v2 3/3] iio: light: bd27008: Support BD27010 RGB Matti Vaittinen
@ 2023-07-08 16:21 ` Jonathan Cameron
0 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2023-07-08 16:21 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Matti Vaittinen, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-iio, devicetree,
linux-kernel
One trivial request for a comment.
Otherwise (subject to jetlag - which snuck up on me during this review)
this looks fine to me..
Will be a few weeks though until the precursor series does the
round trip via upstream to get into the branch I'd want to apply this
on.
Jonathan
>
> +static int bu27010_get_gain_sel(struct bu27008_data *data, int *sel)
> +{
> + int ret, tmp;
> +
> + /*
> + * We always "lock" the gain selectors for all channels to prevent
> + * unsupported configs. It does not matter which channel is used
> + * we can just return selector from any of them.
> + */
> + ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL2, sel);
> + if (ret)
> + return ret;
> +
> + *sel = FIELD_GET(BU27010_MASK_DATA0_GAIN, *sel);
> +
> + ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL1, &tmp);
> + if (ret)
> + return ret;
> +
> + *sel |= FIELD_GET(BU27010_MASK_RGBC_GAIN, tmp) << fls(BU27010_MASK_DATA0_GAIN);
Odd enough to perhaps warrant a comment on the maths.
> +
> + return ret;
> +}
> +
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/3] Support ROHM BU27010 RGBC sensor
2023-07-07 11:22 [PATCH v2 0/3] Support ROHM BU27010 RGBC sensor Matti Vaittinen
` (2 preceding siblings ...)
2023-07-07 11:23 ` [PATCH v2 3/3] iio: light: bd27008: Support BD27010 RGB Matti Vaittinen
@ 2023-07-08 16:06 ` Jonathan Cameron
3 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2023-07-08 16:06 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Matti Vaittinen, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-iio, devicetree,
linux-kernel
On Fri, 7 Jul 2023 14:22:14 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> Support ROHM BU27010 RGBC + flickering sensor.
>
> Following description copied from commit log:
>
> > The ROHM BU27010 is an RGBC sensor with a flickering detection FIFO. The
> > RGBC+IR sensor functionality is largely similar to what the BU27008 has.
> > There are some notable things though:
> > - gain setting is once again new and exotic. Now, there is 6bit gain
> > setting where 4 of the bits are common to all channels and 2 bits
> > can be configured separately for each channel. The BU27010 has
> > similar "1X on other channels vs 2X on IR when selector is 0x0"
> > gain design as BU27008 had. So, we use same gain setting policy for
> > BU27010 as we did for BU27008 - driver sets same gain selector for
> > all channels but shows the gains separately for all channels so users
> > can (at least in theory) detect this 1X vs 2X madness...
> > - BU27010 has suffled all the control register bitfields to new
> > addresses and bit positions while still keeping the register naming
> > same.
> > - Some more power/reset control is added.
> > - FIFO for "flickering detection" is added.
> >
> > The control register suffling made this slightly nasty. Still, it is
> > easier for maintenance perspective to add the BU27010 support in BU27008
> > driver because - even though the bit positions/addresses were changed -
> > most of the driver structure can be re-used. Writing own driver for
> > BU27010 would mean plenty of duplicate code albeit a tad more clarity.
>
> This series is done on top of the iio-for-6.5a + this fix-up series:
> https://lore.kernel.org/all/cover.1686648422.git.mazziesaccount@gmail.com/
>
> I didn't spot these fixups from iio git although I think I saw mail
> stating they were applied.
They are now on the fixes-togreg branch of iio. Things are sometimes
fiddly around the merge window. That tree may well be rebased again after
rc1 is available.
This series is going to have to wait to be applied until those fixes
are in my upstream for the togreg branch.
Thanks,
Jonathan
>
> Revision history:
>
> v1 => v2:
> - make vdd-supply required binding.
> - Some re-ordering for struct member init.
> - Some re-ordering for code to get rid of function declarations.
> - Tidy up the mess from patch 2/3.
> - Refactor parts from 3/3 to 2/3.
> - Clean-up commented out code and tidy things in general.
>
> Please note: I'll be mostly away from the computer during July. Please,
> take your time reviewing and forgive me if sending follow-up revisions
> is delayed.
>
> ---
>
> Matti Vaittinen (3):
> dt-bindings: iio: ROHM BU27010 RGBC + flickering sensor
> iio: light: bu27008: add chip info
> iio: light: bd27008: Support BD27010 RGB
>
> .../bindings/iio/light/rohm,bu27010.yaml | 49 ++
> drivers/iio/light/rohm-bu27008.c | 622 ++++++++++++++----
> 2 files changed, 556 insertions(+), 115 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bu27010.yaml
>
^ permalink raw reply [flat|nested] 8+ messages in thread