[PATCH 0/2] ROHM BU27034NUC to ROHM BU27034ANUC

[PATCH 0/2] ROHM BU27034NUC to ROHM BU27034ANUC

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]

As discussed here:
https://lore.kernel.org/all/ff8d6d14-6b48-4347-8525-e05eeb9721ff@gmail.com/

The ROHM BU27034NUC was cancelled before it entered mass-production. A
replacement was developed and named to BU27034ANUC. (Note the added
'A' in the model name). The BU27034ANUC has several changes that make
the old BU27034NUC driver unusable with it. I was told the old BU27034NUC
should not be encountered anywhere.

Hence, this series converts the rohm-bu27034.c to support the new
BU27034ANUC instead of the obsoleted BU27034NUC. Additionally, the
series adds a read-only entry for the HARDWAREGAIN to help understanding
what part of the scale is contributed by the gain, and what by the
integration time. This is useful when figuring out why some transitions
from one 'scale' to other are failing.

---

Matti Vaittinen (2):
  bu27034: ROHM BU27034NUC to BU27034ANUC
  iio: bu27034: Add a read only HWARDWAREGAIN

 drivers/iio/light/rohm-bu27034.c | 335 ++++++++-----------------------
 1 file changed, 81 insertions(+), 254 deletions(-)


base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
-- 
2.45.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 --]

[PATCH 1/2] bu27034: ROHM BU27034NUC to BU27034ANUC

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 20668 bytes --]

The ROHM BU27034NUC was cancelled and BU27034ANUC is replacing this
sensor. Use the BU27034NUC driver to support the new BU27034ANUC.

According to ROHM, the BU27034NUC was never mass-produced. Hence dropping
the BU27034NUC support and using this driver to support BU27034ANUC
should not be a problem to users.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Fixes: e52afbd61039 ("iio: light: ROHM BU27034 Ambient Light Sensor")
---
 drivers/iio/light/rohm-bu27034.c | 321 +++++++------------------------
 1 file changed, 68 insertions(+), 253 deletions(-)

diff --git a/drivers/iio/light/rohm-bu27034.c b/drivers/iio/light/rohm-bu27034.c
index bf3de853a811..51acad2cafbd 100644
--- a/drivers/iio/light/rohm-bu27034.c
+++ b/drivers/iio/light/rohm-bu27034.c
@@ -1,9 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * BU27034 ROHM Ambient Light Sensor
+ * BU27034ANUC ROHM Ambient Light Sensor
  *
  * Copyright (c) 2023, ROHM Semiconductor.
- * https://fscdn.rohm.com/en/products/databook/datasheet/ic/sensor/light/bu27034nuc-e.pdf
  */
 
 #include <linux/bitfield.h>
@@ -30,17 +29,15 @@
 
 #define BU27034_REG_MODE_CONTROL2	0x42
 #define BU27034_MASK_D01_GAIN		GENMASK(7, 3)
-#define BU27034_MASK_D2_GAIN_HI		GENMASK(7, 6)
-#define BU27034_MASK_D2_GAIN_LO		GENMASK(2, 0)
 
 #define BU27034_REG_MODE_CONTROL3	0x43
 #define BU27034_REG_MODE_CONTROL4	0x44
 #define BU27034_MASK_MEAS_EN		BIT(0)
 #define BU27034_MASK_VALID		BIT(7)
+#define BU27034_NUM_HW_DATA_CHANS	2
 #define BU27034_REG_DATA0_LO		0x50
 #define BU27034_REG_DATA1_LO		0x52
-#define BU27034_REG_DATA2_LO		0x54
-#define BU27034_REG_DATA2_HI		0x55
+#define BU27034_REG_DATA1_HI		0x53
 #define BU27034_REG_MANUFACTURER_ID	0x92
 #define BU27034_REG_MAX BU27034_REG_MANUFACTURER_ID
 
@@ -88,58 +85,48 @@ enum {
 	BU27034_CHAN_ALS,
 	BU27034_CHAN_DATA0,
 	BU27034_CHAN_DATA1,
-	BU27034_CHAN_DATA2,
 	BU27034_NUM_CHANS
 };
 
 static const unsigned long bu27034_scan_masks[] = {
-	GENMASK(BU27034_CHAN_DATA2, BU27034_CHAN_ALS), 0
+	GENMASK(BU27034_CHAN_DATA1, BU27034_CHAN_DATA0),
+	GENMASK(BU27034_CHAN_DATA1, BU27034_CHAN_ALS), 0
 };
 
 /*
- * Available scales with gain 1x - 4096x, timings 55, 100, 200, 400 mS
+ * Available scales with gain 1x - 1024x, 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) = 32768
+ * => Max total gain is HWGAIN * gain by integration time (8 * 1024) = 8192
+ * if 1x gain is scale 1, scale for 2x gain is 0.5, 4x => 0.25,
+ * ... 8192x => 0.0001220703125 => 122070.3125 nanos
  *
- * Using NANO precision for scale we must use scale 64x corresponding gain 1x
- * to avoid precision loss. (32x would result scale 976 562.5(nanos).
+ * Using NANO precision for scale, we must use scale 16x corresponding gain 1x
+ * to avoid precision loss. (8x would result scale 976 562.5(nanos).
  */
-#define BU27034_SCALE_1X	64
+#define BU27034_SCALE_1X	16
 
 /* See the data sheet for the "Gain Setting" table */
 #define BU27034_GSEL_1X		0x00 /* 00000 */
 #define BU27034_GSEL_4X		0x08 /* 01000 */
-#define BU27034_GSEL_16X	0x0a /* 01010 */
 #define BU27034_GSEL_32X	0x0b /* 01011 */
-#define BU27034_GSEL_64X	0x0c /* 01100 */
 #define BU27034_GSEL_256X	0x18 /* 11000 */
 #define BU27034_GSEL_512X	0x19 /* 11001 */
 #define BU27034_GSEL_1024X	0x1a /* 11010 */
-#define BU27034_GSEL_2048X	0x1b /* 11011 */
-#define BU27034_GSEL_4096X	0x1c /* 11100 */
 
 /* Available gain settings */
 static const struct iio_gain_sel_pair bu27034_gains[] = {
 	GAIN_SCALE_GAIN(1, BU27034_GSEL_1X),
 	GAIN_SCALE_GAIN(4, BU27034_GSEL_4X),
-	GAIN_SCALE_GAIN(16, BU27034_GSEL_16X),
 	GAIN_SCALE_GAIN(32, BU27034_GSEL_32X),
-	GAIN_SCALE_GAIN(64, BU27034_GSEL_64X),
 	GAIN_SCALE_GAIN(256, BU27034_GSEL_256X),
 	GAIN_SCALE_GAIN(512, BU27034_GSEL_512X),
 	GAIN_SCALE_GAIN(1024, BU27034_GSEL_1024X),
-	GAIN_SCALE_GAIN(2048, BU27034_GSEL_2048X),
-	GAIN_SCALE_GAIN(4096, BU27034_GSEL_4096X),
 };
 
 /*
- * The IC has 5 modes for sampling time. 5 mS mode is exceptional as it limits
- * the data collection to data0-channel only and cuts the supported range to
- * 10 bit. It is not supported by the driver.
- *
- * "normal" modes are 55, 100, 200 and 400 mS modes - which do have direct
- * multiplying impact to the register values (similar to gain).
+ * Measurement modes are 55, 100, 200 and 400 mS modes - which do have direct
+ * multiplying impact to the data register values (similar to gain).
  *
  * This means that if meas-mode is changed for example from 400 => 200,
  * the scale is doubled. Eg, time impact to total gain is x1, x2, x4, x8.
@@ -156,11 +143,11 @@ static const struct iio_itime_sel_mul bu27034_itimes[] = {
 	GAIN_SCALE_ITIME_US(55000, BU27034_MEAS_MODE_55MS, 1),
 };
 
-#define BU27034_CHAN_DATA(_name, _ch2)					\
+#define BU27034_CHAN_DATA(_name)					\
 {									\
 	.type = IIO_INTENSITY,						\
 	.channel = BU27034_CHAN_##_name,				\
-	.channel2 = (_ch2),						\
+	.channel2 = IIO_MOD_LIGHT_CLEAR,				\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
 			      BIT(IIO_CHAN_INFO_SCALE),			\
 	.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),	\
@@ -195,13 +182,12 @@ static const struct iio_chan_spec bu27034_channels[] = {
 	/*
 	 * The BU27034 DATA0 and DATA1 channels are both on the visible light
 	 * area (mostly). The data0 sensitivity peaks at 500nm, DATA1 at 600nm.
-	 * These wave lengths are pretty much on the border of colours making
-	 * these a poor candidates for R/G/B standardization. Hence they're both
-	 * marked as clear channels
+	 * These wave lengths are cyan(ish) and orange(ish), making these
+	 * sub-optiomal candidates for R/G/B standardization. Hence they're
+	 * both marked as clear channels.
 	 */
-	BU27034_CHAN_DATA(DATA0, IIO_MOD_LIGHT_CLEAR),
-	BU27034_CHAN_DATA(DATA1, IIO_MOD_LIGHT_CLEAR),
-	BU27034_CHAN_DATA(DATA2, IIO_MOD_LIGHT_IR),
+	BU27034_CHAN_DATA(DATA0),
+	BU27034_CHAN_DATA(DATA1),
 	IIO_CHAN_SOFT_TIMESTAMP(4),
 };
 
@@ -215,20 +201,14 @@ struct bu27034_data {
 	struct mutex mutex;
 	struct iio_gts gts;
 	struct task_struct *task;
-	__le16 raw[3];
+	__le16 raw[BU27034_NUM_HW_DATA_CHANS];
 	struct {
 		u32 mlux;
-		__le16 channels[3];
+		__le16 channels[BU27034_NUM_HW_DATA_CHANS];
 		s64 ts __aligned(8);
 	} scan;
 };
 
-struct bu27034_result {
-	u16 ch0;
-	u16 ch1;
-	u16 ch2;
-};
-
 static const struct regmap_range bu27034_volatile_ranges[] = {
 	{
 		.range_min = BU27034_REG_SYSTEM_CONTROL,
@@ -238,7 +218,7 @@ static const struct regmap_range bu27034_volatile_ranges[] = {
 		.range_max = BU27034_REG_MODE_CONTROL4,
 	}, {
 		.range_min = BU27034_REG_DATA0_LO,
-		.range_max = BU27034_REG_DATA2_HI,
+		.range_max = BU27034_REG_DATA1_HI,
 	},
 };
 
@@ -250,7 +230,7 @@ static const struct regmap_access_table bu27034_volatile_regs = {
 static const struct regmap_range bu27034_read_only_ranges[] = {
 	{
 		.range_min = BU27034_REG_DATA0_LO,
-		.range_max = BU27034_REG_DATA2_HI,
+		.range_max = BU27034_REG_DATA1_HI,
 	}, {
 		.range_min = BU27034_REG_MANUFACTURER_ID,
 		.range_max = BU27034_REG_MANUFACTURER_ID,
@@ -281,39 +261,15 @@ static int bu27034_get_gain_sel(struct bu27034_data *data, int chan)
 {
 	int ret, val;
 
-	switch (chan) {
-	case BU27034_CHAN_DATA0:
-	case BU27034_CHAN_DATA1:
-	{
-		int reg[] = {
-			[BU27034_CHAN_DATA0] = BU27034_REG_MODE_CONTROL2,
-			[BU27034_CHAN_DATA1] = BU27034_REG_MODE_CONTROL3,
-		};
-		ret = regmap_read(data->regmap, reg[chan], &val);
-		if (ret)
-			return ret;
-
-		return FIELD_GET(BU27034_MASK_D01_GAIN, val);
-	}
-	case BU27034_CHAN_DATA2:
-	{
-		int d2_lo_bits = fls(BU27034_MASK_D2_GAIN_LO);
-
-		ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL2, &val);
-		if (ret)
-			return ret;
+	int reg[] = {
+		[BU27034_CHAN_DATA0] = BU27034_REG_MODE_CONTROL2,
+		[BU27034_CHAN_DATA1] = BU27034_REG_MODE_CONTROL3,
+	};
+	ret = regmap_read(data->regmap, reg[chan], &val);
+	if (ret)
+		return ret;
 
-		/*
-		 * The data2 channel gain is composed by 5 non continuous bits
-		 * [7:6], [2:0]. Thus when we combine the 5-bit 'selector'
-		 * from register value we must right shift the high bits by 3.
-		 */
-		return FIELD_GET(BU27034_MASK_D2_GAIN_HI, val) << d2_lo_bits |
-		       FIELD_GET(BU27034_MASK_D2_GAIN_LO, val);
-	}
-	default:
-		return -EINVAL;
-	}
+	return FIELD_GET(BU27034_MASK_D01_GAIN, val);
 }
 
 static int bu27034_get_gain(struct bu27034_data *data, int chan, int *gain)
@@ -396,44 +352,9 @@ static int bu27034_write_gain_sel(struct bu27034_data *data, int chan, int sel)
 	};
 	int mask, val;
 
-	if (chan != BU27034_CHAN_DATA0 && chan != BU27034_CHAN_DATA1)
-		return -EINVAL;
-
 	val = FIELD_PREP(BU27034_MASK_D01_GAIN, sel);
-
 	mask = BU27034_MASK_D01_GAIN;
 
-	if (chan == BU27034_CHAN_DATA0) {
-		/*
-		 * We keep the same gain for channel 2 as we set for channel 0
-		 * We can't allow them to be individually controlled because
-		 * setting one will impact also the other. Also, if we don't
-		 * always update both gains we may result unsupported bit
-		 * combinations.
-		 *
-		 * This is not nice but this is yet another place where the
-		 * user space must be prepared to surprizes. Namely, see chan 2
-		 * gain changed when chan 0 gain is changed.
-		 *
-		 * This is not fatal for most users though. I don't expect the
-		 * channel 2 to be used in any generic cases - the intensity
-		 * values provided by the sensor for IR area are not openly
-		 * documented. Also, channel 2 is not used for visible light.
-		 *
-		 * So, if there is application which is written to utilize the
-		 * channel 2 - then it is probably specifically targeted to this
-		 * sensor and knows how to utilize those values. It is safe to
-		 * hope such user can also cope with the gain changes.
-		 */
-		mask |=  BU27034_MASK_D2_GAIN_LO;
-
-		/*
-		 * The D2 gain bits are directly the lowest bits of selector.
-		 * Just do add those bits to the value
-		 */
-		val |= sel & BU27034_MASK_D2_GAIN_LO;
-	}
-
 	return regmap_update_bits(data->regmap, reg[chan], mask, val);
 }
 
@@ -441,13 +362,6 @@ static int bu27034_set_gain(struct bu27034_data *data, int chan, int gain)
 {
 	int ret;
 
-	/*
-	 * We don't allow setting channel 2 gain as it messes up the
-	 * gain for channel 0 - which shares the high bits
-	 */
-	if (chan != BU27034_CHAN_DATA0 && chan != BU27034_CHAN_DATA1)
-		return -EINVAL;
-
 	ret = iio_gts_find_sel_by_gain(&data->gts, gain);
 	if (ret < 0)
 		return ret;
@@ -571,9 +485,6 @@ static int bu27034_set_scale(struct bu27034_data *data, int chan,
 	int ret, time_sel, gain_sel, i;
 	bool found = false;
 
-	if (chan == BU27034_CHAN_DATA2)
-		return -EINVAL;
-
 	if (chan == BU27034_CHAN_ALS) {
 		if (val == 0 && val2 == 1000000)
 			return 0;
@@ -598,9 +509,7 @@ static int bu27034_set_scale(struct bu27034_data *data, int chan,
 
 		/*
 		 * Populate information for the other channel which should also
-		 * maintain the scale. (Due to the HW limitations the chan2
-		 * gets the same gain as chan0, so we only need to explicitly
-		 * set the chan 0 and 1).
+		 * maintain the scale.
 		 */
 		if (chan == BU27034_CHAN_DATA0)
 			gain.chan = BU27034_CHAN_DATA1;
@@ -614,7 +523,7 @@ static int bu27034_set_scale(struct bu27034_data *data, int chan,
 		/*
 		 * Iterate through all the times to see if we find one which
 		 * can support requested scale for requested channel, while
-		 * maintaining the scale for other channels
+		 * maintaining the scale for the other channel
 		 */
 		for (i = 0; i < data->gts.num_itime; i++) {
 			new_time_sel = data->gts.itime_table[i].sel;
@@ -629,7 +538,7 @@ static int bu27034_set_scale(struct bu27034_data *data, int chan,
 			if (ret)
 				continue;
 
-			/* Can the other channel(s) maintain scale? */
+			/* Can the other channel maintain scale? */
 			ret = iio_gts_find_new_gain_sel_by_old_gain_time(
 				&data->gts, gain.old_gain, time_sel,
 				new_time_sel, &gain.new_gain);
@@ -641,7 +550,7 @@ static int bu27034_set_scale(struct bu27034_data *data, int chan,
 		}
 		if (!found) {
 			dev_dbg(data->dev,
-				"Can't set scale maintaining other channels\n");
+				"Can't set scale maintaining other channel\n");
 			ret = -EINVAL;
 
 			goto unlock_out;
@@ -665,102 +574,21 @@ static int bu27034_set_scale(struct bu27034_data *data, int chan,
 }
 
 /*
- * for (D1/D0 < 0.87):
- * lx = 0.004521097 * D1 - 0.002663996 * D0 +
- *	0.00012213 * D1 * D1 / D0
- *
- * =>	115.7400832 * ch1 / gain1 / mt -
- *	68.1982976 * ch0 / gain0 / mt +
- *	0.00012213 * 25600 * (ch1 / gain1 / mt) * 25600 *
- *	(ch1 /gain1 / mt) / (25600 * ch0 / gain0 / mt)
+ * for (D1/D0 < 1.5):
+ *    lx = (0.001193 * D0 + (-0.0000747) * D1) * ((D1/D0 – 1.5) * (0.25) + 1)
  *
- * A =	0.00012213 * 25600 * (ch1 /gain1 / mt) * 25600 *
- *	(ch1 /gain1 / mt) / (25600 * ch0 / gain0 / mt)
- * =>	0.00012213 * 25600 * (ch1 /gain1 / mt) *
- *	(ch1 /gain1 / mt) / (ch0 / gain0 / mt)
- * =>	0.00012213 * 25600 * (ch1 / gain1) * (ch1 /gain1 / mt) /
- *	(ch0 / gain0)
- * =>	0.00012213 * 25600 * (ch1 / gain1) * (ch1 /gain1 / mt) *
- *	gain0 / ch0
- * =>	3.126528 * ch1 * ch1 * gain0 / gain1 / gain1 / mt /ch0
+ *    => -0.000745625 * D0 + 0.0002515625 * D1 + -0.000018675 * D1 * D1 / D0
  *
- * lx = (115.7400832 * ch1 / gain1 - 68.1982976 * ch0 / gain0) /
- *	mt + A
- * =>	(115.7400832 * ch1 / gain1 - 68.1982976 * ch0 / gain0) /
- *	mt + 3.126528 * ch1 * ch1 * gain0 / gain1 / gain1 / mt /
- *	ch0
+ *    => (6.44 * ch1 / gain1 + 19.088 * ch0 / gain0 -
+ *       0.47808 * ch1 * ch1 * gain0 / gain1 / gain1 / ch0) /
+ *       mt
  *
- * =>	(115.7400832 * ch1 / gain1 - 68.1982976 * ch0 / gain0 +
- *	  3.126528 * ch1 * ch1 * gain0 / gain1 / gain1 / ch0) /
- *	  mt
+ * Else
+ *    lx = 0.001193 * D0 - 0.0000747 * D1
  *
- * For (0.87 <= D1/D0 < 1.00)
- * lx = (0.001331* D0 + 0.0000354 * D1) * ((D1/D0 – 0.87) * (0.385) + 1)
- * =>	(0.001331 * 256 * 100 * ch0 / gain0 / mt + 0.0000354 * 256 *
- *	100 * ch1 / gain1 / mt) * ((D1/D0 -  0.87) * (0.385) + 1)
- * =>	(34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
- *	((D1/D0 -  0.87) * (0.385) + 1)
- * =>	(34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
- *	(0.385 * D1/D0 - 0.66505)
- * =>	(34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
- *	(0.385 * 256 * 100 * ch1 / gain1 / mt / (256 * 100 * ch0 / gain0 / mt) - 0.66505)
- * =>	(34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
- *	(9856 * ch1 / gain1 / mt / (25600 * ch0 / gain0 / mt) + 0.66505)
- * =>	13.118336 * ch1 / (gain1 * mt)
- *	+ 22.66064768 * ch0 / (gain0 * mt)
- *	+ 8931.90144 * ch1 * ch1 * gain0 /
- *	  (25600 * ch0 * gain1 * gain1 * mt)
- *	+ 0.602694912 * ch1 / (gain1 * mt)
- *
- * =>	[0.3489024 * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1)
- *	 + 22.66064768 * ch0 / gain0
- *	 + 13.721030912 * ch1 / gain1
- *	] / mt
- *
- * For (D1/D0 >= 1.00)
- *
- * lx	= (0.001331* D0 + 0.0000354 * D1) * ((D1/D0 – 2.0) * (-0.05) + 1)
- *	=> (0.001331* D0 + 0.0000354 * D1) * (-0.05D1/D0 + 1.1)
- *	=> (0.001331 * 256 * 100 * ch0 / gain0 / mt + 0.0000354 * 256 *
- *	   100 * ch1 / gain1 / mt) * (-0.05D1/D0 + 1.1)
- *	=> (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
- *	   (-0.05 * 256 * 100 * ch1 / gain1 / mt / (256 * 100 * ch0 / gain0 / mt) + 1.1)
- *	=> (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) *
- *	   (-1280 * ch1 / (gain1 * mt * 25600 * ch0 / gain0 / mt) + 1.1)
- *	=> (34.0736 * ch0 * -1280 * ch1 * gain0 * mt /( gain0 * mt * gain1 * mt * 25600 * ch0)
- *	    + 34.0736 * 1.1 * ch0 / (gain0 * mt)
- *	    + 0.90624 * ch1 * -1280 * ch1 *gain0 * mt / (gain1 * mt *gain1 * mt * 25600 * ch0)
- *	    + 1.1 * 0.90624 * ch1 / (gain1 * mt)
- *	=> -43614.208 * ch1 / (gain1 * mt * 25600)
- *	    + 37.48096  ch0 / (gain0 * mt)
- *	    - 1159.9872 * ch1 * ch1 * gain0 / (gain1 * gain1 * mt * 25600 * ch0)
- *	    + 0.996864 ch1 / (gain1 * mt)
- *	=> [
- *		- 0.045312 * ch1 * ch1 * gain0 / (gain1 * gain1 * ch0)
- *		- 0.706816 * ch1 / gain1
- *		+ 37.48096  ch0 /gain0
- *	   ] * mt
- *
- *
- * So, the first case (D1/D0 < 0.87) can be computed to a form:
- *
- * lx = (3.126528 * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1) +
- *	 115.7400832 * ch1 / gain1 +
- *	-68.1982976 * ch0 / gain0
- *	 / mt
- *
- * Second case (0.87 <= D1/D0 < 1.00) goes to form:
- *
- *	=> [0.3489024 * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1) +
- *	    13.721030912 * ch1 / gain1 +
- *	    22.66064768 * ch0 / gain0
- *	   ] / mt
- *
- * Third case (D1/D0 >= 1.00) goes to form:
- *	=> [-0.045312 * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1) +
- *	    -0.706816 * ch1 / gain1 +
- *	    37.48096  ch0 /(gain0
- *	   ] / mt
+ *    => (1.91232 * ch1 / gain1 + 30.5408 * ch0 / gain0 +
+ *        [0 * ch1 * ch1 * gain0 / gain1 / gain1 / ch0] ) /
+ *        mt
  *
  * This can be unified to format:
  * lx = [
@@ -770,19 +598,14 @@ static int bu27034_set_scale(struct bu27034_data *data, int chan,
  *	] / mt
  *
  * For case 1:
- * A = 3.126528,
- * B = 115.7400832
- * C = -68.1982976
+ * A = -0.47808,
+ * B = 6.44,
+ * C = 19.088
  *
  * For case 2:
- * A = 0.3489024
- * B = 13.721030912
- * C = 22.66064768
- *
- * For case 3:
- * A = -0.045312
- * B = -0.706816
- * C = 37.48096
+ * A = 0
+ * B = 1.91232
+ * C = 30.5408
  */
 
 struct bu27034_lx_coeff {
@@ -887,21 +710,16 @@ static int bu27034_fixp_calc_lx(unsigned int ch0, unsigned int ch1,
 {
 	static const struct bu27034_lx_coeff coeff[] = {
 		{
-			.A = 31265280,		/* 3.126528 */
-			.B = 1157400832,	/*115.7400832 */
-			.C = 681982976,		/* -68.1982976 */
-			.is_neg = {false, false, true},
+			.A = 4780800,		/* -0.47808 */
+			.B = 64400000,		/*6.44 */
+			.C = 190880000,		/* 19.088 */
+			.is_neg = {true, false, false},
 		}, {
-			.A = 3489024,		/* 0.3489024 */
-			.B = 137210309,		/* 13.721030912 */
-			.C = 226606476,		/* 22.66064768 */
+			.A = 0,			/* 0 */
+			.B = 19123200,		/* 1.91232 */
+			.C = 305408000,		/* 30.5408 */
 			/* All terms positive */
-		}, {
-			.A = 453120,		/* -0.045312 */
-			.B = 7068160,		/* -0.706816 */
-			.C = 374809600,		/* 37.48096 */
-			.is_neg = {true, true, false},
-		}
+		},
 	};
 	const struct bu27034_lx_coeff *c = &coeff[coeff_idx];
 	u64 res = 0, terms[3];
@@ -973,7 +791,6 @@ static int bu27034_read_result(struct bu27034_data *data, int chan, int *res)
 	int reg[] = {
 		[BU27034_CHAN_DATA0] = BU27034_REG_DATA0_LO,
 		[BU27034_CHAN_DATA1] = BU27034_REG_DATA1_LO,
-		[BU27034_CHAN_DATA2] = BU27034_REG_DATA2_LO,
 	};
 	int valid, ret;
 	__le16 val;
@@ -1040,7 +857,7 @@ static int bu27034_get_single_result(struct bu27034_data *data, int chan,
 {
 	int ret;
 
-	if (chan < BU27034_CHAN_DATA0 || chan > BU27034_CHAN_DATA2)
+	if (chan < BU27034_CHAN_DATA0 || chan > BU27034_CHAN_DATA1)
 		return -EINVAL;
 
 	ret = bu27034_meas_set(data, true);
@@ -1065,12 +882,10 @@ static int bu27034_get_single_result(struct bu27034_data *data, int chan,
  * D1 = data1/ch1_gain/meas_time_ms * 25600
  *
  * Then:
- * if (D1/D0 < 0.87)
- *	lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 0.87) * 3.45 + 1)
- * else if (D1/D0 < 1)
- *	lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 0.87) * 0.385 + 1)
- * else
- *	lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 2) * -0.05 + 1)
+ * If (D1/D0 < 1.5)
+ *    lx = (0.001193 * D0 + (-0.0000747) * D1) * ((D1/D0 – 1.5) * (0.25) + 1)
+ * Else
+ *    lx = (0.001193* D0 + (-0.0000747) * D1)
  *
  * We use it here. Users who have for example some colored lens
  * need to modify the calculation but I hope this gives a starting point for
@@ -1139,7 +954,7 @@ static int bu27034_calc_mlux(struct bu27034_data *data, __le16 *res, int *val)
 
 static int bu27034_get_mlux(struct bu27034_data *data, int chan, int *val)
 {
-	__le16 res[3];
+	__le16 res[BU27034_NUM_HW_DATA_CHANS];
 	int ret;
 
 	ret = bu27034_meas_set(data, true);
-- 
2.45.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 --]

Re: [PATCH 1/2] bu27034: ROHM BU27034NUC to BU27034ANUC

[Jonathan Cameron]

On Mon, 10 Jun 2024 13:01:23 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> The ROHM BU27034NUC was cancelled and BU27034ANUC is replacing this
> sensor. Use the BU27034NUC driver to support the new BU27034ANUC.
> 
> According to ROHM, the BU27034NUC was never mass-produced. Hence dropping
> the BU27034NUC support and using this driver to support BU27034ANUC
> should not be a problem to users.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> Fixes: e52afbd61039 ("iio: light: ROHM BU27034 Ambient Light Sensor")

This is an odd case.  I don't think a fixes tag is appropriate and I
don't think we can use the original compatible.  I don't mind breaking
support for the non existent port going forwards and indeed dropping
all indication it ever existed, but the old kernel's are out there and
even getting this into stable is far from a guarantee there won't be
a kernel run on a board that has this compatible but has the old
driver.  It's also too big really to be stable material.

So I think the path forwards is a new compatible and drop the old
one from the dt bindings and driver.  Thus any new dts for a board
that actually has this device will use the new compatible and avoid
any risk of encountering the old driver.

Maybe we can be more relaxed - what actually happens if you use the
existing driver with the new part?

I'm trusting you copied the maths right for the computed
channels (that take too long to review!)  So everything inline is
formatting type stuff.

> ---
>  drivers/iio/light/rohm-bu27034.c | 321 +++++++------------------------
>  1 file changed, 68 insertions(+), 253 deletions(-)
> 
> diff --git a/drivers/iio/light/rohm-bu27034.c b/drivers/iio/light/rohm-bu27034.c
> index bf3de853a811..51acad2cafbd 100644
> --- a/drivers/iio/light/rohm-bu27034.c
> +++ b/drivers/iio/light/rohm-bu27034.c

...

>  /*
> - * Available scales with gain 1x - 4096x, timings 55, 100, 200, 400 mS
> + * Available scales with gain 1x - 1024x, 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) = 32768
> + * => Max total gain is HWGAIN * gain by integration time (8 * 1024) = 8192
> + * if 1x gain is scale 1, scale for 2x gain is 0.5, 4x => 0.25,
> + * ... 8192x => 0.0001220703125 => 122070.3125 nanos
>   *
> - * Using NANO precision for scale we must use scale 64x corresponding gain 1x
> - * to avoid precision loss. (32x would result scale 976 562.5(nanos).
> + * Using NANO precision for scale, we must use scale 16x corresponding gain 1x
> + * to avoid precision loss. (8x would result scale 976 562.5(nanos).
>   */
> -#define BU27034_SCALE_1X	64
> +#define BU27034_SCALE_1X	16

...

> -#define BU27034_CHAN_DATA(_name, _ch2)					\
> +#define BU27034_CHAN_DATA(_name)					\
>  {									\
>  	.type = IIO_INTENSITY,						\
>  	.channel = BU27034_CHAN_##_name,				\
> -	.channel2 = (_ch2),						\
> +	.channel2 = IIO_MOD_LIGHT_CLEAR,				\
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
>  			      BIT(IIO_CHAN_INFO_SCALE),			\
>  	.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),	\
> @@ -195,13 +182,12 @@ static const struct iio_chan_spec bu27034_channels[] = {
>  	/*
>  	 * The BU27034 DATA0 and DATA1 channels are both on the visible light
>  	 * area (mostly). The data0 sensitivity peaks at 500nm, DATA1 at 600nm.
> -	 * These wave lengths are pretty much on the border of colours making
> -	 * these a poor candidates for R/G/B standardization. Hence they're both
> -	 * marked as clear channels
> +	 * These wave lengths are cyan(ish) and orange(ish), making these
> +	 * sub-optiomal candidates for R/G/B standardization. Hence they're
> +	 * both marked as clear channels.

I think just indexing them and not giving a modifier is probably better than
claiming they are clear.  Leave it more vague basically.

>  	 */
> -	BU27034_CHAN_DATA(DATA0, IIO_MOD_LIGHT_CLEAR),
> -	BU27034_CHAN_DATA(DATA1, IIO_MOD_LIGHT_CLEAR),
> -	BU27034_CHAN_DATA(DATA2, IIO_MOD_LIGHT_IR),
> +	BU27034_CHAN_DATA(DATA0),
> +	BU27034_CHAN_DATA(DATA1),
>  	IIO_CHAN_SOFT_TIMESTAMP(4),
>  };

> @@ -281,39 +261,15 @@ static int bu27034_get_gain_sel(struct bu27034_data *data, int chan)
>  {
>  	int ret, val;
>  
Probably no blank line here.

> -	switch (chan) {
> -	case BU27034_CHAN_DATA0:
> -	case BU27034_CHAN_DATA1:
> -	{
> -		int reg[] = {
> -			[BU27034_CHAN_DATA0] = BU27034_REG_MODE_CONTROL2,
> -			[BU27034_CHAN_DATA1] = BU27034_REG_MODE_CONTROL3,
> -		};
> -		ret = regmap_read(data->regmap, reg[chan], &val);
> -		if (ret)
> -			return ret;
> -
> -		return FIELD_GET(BU27034_MASK_D01_GAIN, val);
> -	}
> -	case BU27034_CHAN_DATA2:
> -	{
> -		int d2_lo_bits = fls(BU27034_MASK_D2_GAIN_LO);
> -
> -		ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL2, &val);
> -		if (ret)
> -			return ret;
> +	int reg[] = {
> +		[BU27034_CHAN_DATA0] = BU27034_REG_MODE_CONTROL2,
> +		[BU27034_CHAN_DATA1] = BU27034_REG_MODE_CONTROL3,
> +	};
blank line here

> +	ret = regmap_read(data->regmap, reg[chan], &val);
> +	if (ret)
> +		return ret;

...

>  
>  struct bu27034_lx_coeff {
> @@ -887,21 +710,16 @@ static int bu27034_fixp_calc_lx(unsigned int ch0, unsigned int ch1,
>  {
>  	static const struct bu27034_lx_coeff coeff[] = {
>  		{
> -			.A = 31265280,		/* 3.126528 */
> -			.B = 1157400832,	/*115.7400832 */
> -			.C = 681982976,		/* -68.1982976 */
> -			.is_neg = {false, false, true},
> +			.A = 4780800,		/* -0.47808 */
> +			.B = 64400000,		/*6.44 */


/* 6.44 */
It was odd before but might as well tidy that up!

> +			.C = 190880000,		/* 19.088 */
> +			.is_neg = {true, false, false},
{ true, false, false }
Tidy that up as well.
>  		}, {
> -			.A = 3489024,		/* 0.3489024 */
> -			.B = 137210309,		/* 13.721030912 */
> -			.C = 226606476,		/* 22.66064768 */
> +			.A = 0,			/* 0 */
> +			.B = 19123200,		/* 1.91232 */
> +			.C = 305408000,		/* 30.5408 */
>  			/* All terms positive */
> -		}, {
> -			.A = 453120,		/* -0.045312 */
> -			.B = 7068160,		/* -0.706816 */
> -			.C = 374809600,		/* 37.48096 */
> -			.is_neg = {true, true, false},
> -		}
> +		},
>  	};

...

> @@ -1065,12 +882,10 @@ static int bu27034_get_single_result(struct bu27034_data *data, int chan,
>   * D1 = data1/ch1_gain/meas_time_ms * 25600
>   *
>   * Then:
> - * if (D1/D0 < 0.87)
> - *	lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 0.87) * 3.45 + 1)
> - * else if (D1/D0 < 1)
> - *	lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 0.87) * 0.385 + 1)
> - * else
> - *	lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 2) * -0.05 + 1)
> + * If (D1/D0 < 1.5)
> + *    lx = (0.001193 * D0 + (-0.0000747) * D1) * ((D1/D0 – 1.5) * (0.25) + 1)

Brackets around 0.25 odd.  The negative one might be justified if the code
is such that   you can't just do D0 - 0.0000747 instead.

> + * Else
> + *    lx = (0.001193* D0 + (-0.0000747) * D1)
Space between 3 and *

>   *
>   * We use it here. Users who have for example some colored lens
>   * need to modify the calculation but I hope this gives a starting point for
> @@ -1139,7 +954,7 @@ static int bu27034_calc_mlux(struct bu27034_data *data, __le16 *res, int *val)
>  
>  static int bu27034_get_mlux(struct bu27034_data *data, int chan, int *val)
>  {
> -	__le16 res[3];
> +	__le16 res[BU27034_NUM_HW_DATA_CHANS];
>  	int ret;
>  
>  	ret = bu27034_meas_set(data, true);

Re: [PATCH 1/2] bu27034: ROHM BU27034NUC to BU27034ANUC

[Matti Vaittinen]

On 6/15/24 20:47, Jonathan Cameron wrote:
> On Mon, 10 Jun 2024 13:01:23 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> The ROHM BU27034NUC was cancelled and BU27034ANUC is replacing this
>> sensor. Use the BU27034NUC driver to support the new BU27034ANUC.
>>
>> According to ROHM, the BU27034NUC was never mass-produced. Hence dropping
>> the BU27034NUC support and using this driver to support BU27034ANUC
>> should not be a problem to users.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>> Fixes: e52afbd61039 ("iio: light: ROHM BU27034 Ambient Light Sensor")
> 
> This is an odd case.  I don't think a fixes tag is appropriate

Last week I was thinking this purely from a new BU27034ANUC user's point 
of view. For user of the new sensor it appears the current driver is 
broken. Hence this felt like a fix. That was last week though...

  and I
> don't think we can use the original compatible.

I didn't think so either back in March. However, I forgot what so ever 
plans I had while I was waiting for some internal decision regarding 
upstreaming of this... I just went back to the mail I sent about this in 
March, and I see we discussed adding new compatible back then, and I 
also promised to send this patch as a series of smaller changes... Sorry!

>  I don't mind breaking
> support for the non existent port going forwards and indeed dropping
> all indication it ever existed, but the old kernel's are out there and
> even getting this into stable is far from a guarantee there won't be
> a kernel run on a board that has this compatible but has the old
> driver.

I agree it's not a guarantee, but it would be the best we can do. At 
least some stable users would upgrade - have upgraded - could pick 
stable with fixed driver.

>  It's also too big really to be stable material.

I am not really going to argue on that. Asking for the stable 
maintainers to look at this and port it is indeed a bit too much. I 
guess we just need to live with having the b0rked version out there.

> So I think the path forwards is a new compatible and drop the old
> one from the dt bindings and driver.  Thus any new dts for a board
> that actually has this device will use the new compatible and avoid
> any risk of encountering the old driver.

Yes. This should be the way forward.

> Maybe we can be more relaxed - what actually happens if you use the
> existing driver with the new part?

I am pretty sure the world does not explode. I've not tried this so I am 
not sure if reading the register area where the removed data channel 
used to be is succeeding. My assumption is it does, but just returns 
garbage. Furthermore, I am not sure what happens if those removed gains 
are tried to be set.

So, not tried but I would guess that the read data would just be insane.

> I'm trusting you copied the maths right for the computed
> channels (that take too long to review!)

I understand the reviewing problem. I feel it is time consuming and 
sometimes very much energy draining. :) And I _really_ appreciate the 
work you do with reviewing and maintaining! But trusting me? I suppose 
we all make mistakes XD

>  So everything inline is
> formatting type stuff.

Thanks! I will fix the compatible and formatting. I agree with all of 
the comments - but it may take a while until I send the next version. 
I'm having a vacation and trying to spend my time AFK for next couple of 
weeks. My old motorbike and tiny boat are calling for me - so amount of 
code I write is (probably) inversely proportional to the amount of 
sunshine in Finland ;)

>>   	/*
>>   	 * The BU27034 DATA0 and DATA1 channels are both on the visible light
>>   	 * area (mostly). The data0 sensitivity peaks at 500nm, DATA1 at 600nm.
>> -	 * These wave lengths are pretty much on the border of colours making
>> -	 * these a poor candidates for R/G/B standardization. Hence they're both
>> -	 * marked as clear channels
>> +	 * These wave lengths are cyan(ish) and orange(ish), making these
>> +	 * sub-optiomal candidates for R/G/B standardization. Hence they're
>> +	 * both marked as clear channels.
> 
> I think just indexing them and not giving a modifier is probably better than
> claiming they are clear.  Leave it more vague basically.

Agree. I just didn't see leaving out the modifier as an option.

>>   	 */
>> -	BU27034_CHAN_DATA(DATA0, IIO_MOD_LIGHT_CLEAR),
>> -	BU27034_CHAN_DATA(DATA1, IIO_MOD_LIGHT_CLEAR),
>> -	BU27034_CHAN_DATA(DATA2, IIO_MOD_LIGHT_IR),
>> +	BU27034_CHAN_DATA(DATA0),
>> +	BU27034_CHAN_DATA(DATA1),
>>   	IIO_CHAN_SOFT_TIMESTAMP(4),
>>   };
> 

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

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

[PATCH 2/2] iio: bu27034: Add a read only HWARDWAREGAIN

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 3020 bytes --]

The ROHM BU27034 light sensor has two data channels for measuring
different frequencies of light. The result from these channels is
combined into Lux value while the raw channel values are reported via
intensity channels.

Both of the intensity channels have adjustable gain setting which
impacts the scale of the raw channels. Eg, doubling the gain will double
the values read from the raw channels, which halves the scale value. The
integration time can also be set for the sensor. This does also have an
impact to the scale of the intensity channels because increasing the
integration time will also increase the values reported via the raw
channels.

Impact of integration time to the scale and the fact that the scale value
does not start from '1', can make it hard for a human reader to compute the
gain values based on the scale.

Add read-only HARDWAREGAIN to help debugging.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/iio/light/rohm-bu27034.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/light/rohm-bu27034.c b/drivers/iio/light/rohm-bu27034.c
index 51acad2cafbd..b299ff2aacce 100644
--- a/drivers/iio/light/rohm-bu27034.c
+++ b/drivers/iio/light/rohm-bu27034.c
@@ -149,7 +149,8 @@ static const struct iio_itime_sel_mul bu27034_itimes[] = {
 	.channel = BU27034_CHAN_##_name,				\
 	.channel2 = IIO_MOD_LIGHT_CLEAR,				\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
-			      BIT(IIO_CHAN_INFO_SCALE),			\
+			      BIT(IIO_CHAN_INFO_SCALE) |		\
+			      BIT(IIO_CHAN_INFO_HARDWAREGAIN),		\
 	.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),	\
 	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),		\
 	.info_mask_shared_by_all_available =				\
@@ -992,6 +993,13 @@ static int bu27034_read_raw(struct iio_dev *idev,
 
 		return IIO_VAL_INT_PLUS_MICRO;
 
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		ret = bu27034_get_gain(data, chan->channel, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+
 	case IIO_CHAN_INFO_SCALE:
 		return bu27034_get_scale(data, chan->channel, val, val2);
 
@@ -1036,12 +1044,16 @@ static int bu27034_write_raw_get_fmt(struct iio_dev *indio_dev,
 				     struct iio_chan_spec const *chan,
 				     long mask)
 {
+	struct bu27034_data *data = iio_priv(indio_dev);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
 		return IIO_VAL_INT_PLUS_NANO;
 	case IIO_CHAN_INFO_INT_TIME:
 		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		dev_dbg(data->dev,
+			"HARDWAREGAIN is read-only, use scale to set\n");
 	default:
 		return -EINVAL;
 	}
-- 
2.45.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 --]

Re: [PATCH 2/2] iio: bu27034: Add a read only HWARDWAREGAIN

[kernel test robot]

Hi Matti,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0]

url:    https://github.com/intel-lab-lkp/linux/commits/Matti-Vaittinen/bu27034-ROHM-BU27034NUC-to-BU27034ANUC/20240610-180426
base:   1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
patch link:    https://lore.kernel.org/r/5e88c7b7b0389c6c011f15e05e065791f7561cf5.1718013518.git.mazziesaccount%40gmail.com
patch subject: [PATCH 2/2] iio: bu27034: Add a read only HWARDWAREGAIN
config: i386-buildonly-randconfig-002-20240610 (https://download.01.org/0day-ci/archive/20240610/202406102012.s3Qrfbm7-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240610/202406102012.s3Qrfbm7-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406102012.s3Qrfbm7-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/linux/device.h:15,
                    from drivers/iio/light/rohm-bu27034.c:10:
   drivers/iio/light/rohm-bu27034.c: In function 'bu27034_write_raw_get_fmt':
>> include/linux/dev_printk.h:138:20: warning: this statement may fall through [-Wimplicit-fallthrough=]
     137 |         ({                                                              \
         |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     138 |                 if (0)                                                  \
         |                 ~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     139 |                         _dev_printk(level, dev, fmt, ##__VA_ARGS__);    \
         |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     140 |         })
         |         ~~          
   include/linux/dev_printk.h:171:9: note: in expansion of macro 'dev_no_printk'
     171 |         dev_no_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |         ^~~~~~~~~~~~~
   drivers/iio/light/rohm-bu27034.c:1055:17: note: in expansion of macro 'dev_dbg'
    1055 |                 dev_dbg(data->dev,
         |                 ^~~~~~~
   drivers/iio/light/rohm-bu27034.c:1057:9: note: here
    1057 |         default:
         |         ^~~~~~~


vim +138 include/linux/dev_printk.h

af628aae8640c2 Greg Kroah-Hartman 2019-12-09   99  
ad7d61f159db73 Chris Down         2021-06-15  100  /*
ad7d61f159db73 Chris Down         2021-06-15  101   * Need to take variadic arguments even though we don't use them, as dev_fmt()
ad7d61f159db73 Chris Down         2021-06-15  102   * may only just have been expanded and may result in multiple arguments.
ad7d61f159db73 Chris Down         2021-06-15  103   */
ad7d61f159db73 Chris Down         2021-06-15  104  #define dev_printk_index_emit(level, fmt, ...) \
ad7d61f159db73 Chris Down         2021-06-15  105  	printk_index_subsys_emit("%s %s: ", level, fmt)
ad7d61f159db73 Chris Down         2021-06-15  106  
ad7d61f159db73 Chris Down         2021-06-15  107  #define dev_printk_index_wrap(_p_func, level, dev, fmt, ...)		\
ad7d61f159db73 Chris Down         2021-06-15  108  	({								\
ad7d61f159db73 Chris Down         2021-06-15  109  		dev_printk_index_emit(level, fmt);			\
ad7d61f159db73 Chris Down         2021-06-15  110  		_p_func(dev, fmt, ##__VA_ARGS__);			\
ad7d61f159db73 Chris Down         2021-06-15  111  	})
ad7d61f159db73 Chris Down         2021-06-15  112  
ad7d61f159db73 Chris Down         2021-06-15  113  /*
ad7d61f159db73 Chris Down         2021-06-15  114   * Some callsites directly call dev_printk rather than going through the
ad7d61f159db73 Chris Down         2021-06-15  115   * dev_<level> infrastructure, so we need to emit here as well as inside those
ad7d61f159db73 Chris Down         2021-06-15  116   * level-specific macros. Only one index entry will be produced, either way,
ad7d61f159db73 Chris Down         2021-06-15  117   * since dev_printk's `fmt` isn't known at compile time if going through the
ad7d61f159db73 Chris Down         2021-06-15  118   * dev_<level> macros.
ad7d61f159db73 Chris Down         2021-06-15  119   *
ad7d61f159db73 Chris Down         2021-06-15  120   * dev_fmt() isn't called for dev_printk when used directly, as it's used by
ad7d61f159db73 Chris Down         2021-06-15  121   * the dev_<level> macros internally which already have dev_fmt() processed.
ad7d61f159db73 Chris Down         2021-06-15  122   *
ad7d61f159db73 Chris Down         2021-06-15  123   * We also can't use dev_printk_index_wrap directly, because we have a separate
ad7d61f159db73 Chris Down         2021-06-15  124   * level to process.
ad7d61f159db73 Chris Down         2021-06-15  125   */
ad7d61f159db73 Chris Down         2021-06-15  126  #define dev_printk(level, dev, fmt, ...)				\
ad7d61f159db73 Chris Down         2021-06-15  127  	({								\
ad7d61f159db73 Chris Down         2021-06-15  128  		dev_printk_index_emit(level, fmt);			\
ad7d61f159db73 Chris Down         2021-06-15  129  		_dev_printk(level, dev, fmt, ##__VA_ARGS__);		\
ad7d61f159db73 Chris Down         2021-06-15  130  	})
ad7d61f159db73 Chris Down         2021-06-15  131  
c26ec799042a38 Geert Uytterhoeven 2024-02-28  132  /*
c26ec799042a38 Geert Uytterhoeven 2024-02-28  133   * Dummy dev_printk for disabled debugging statements to use whilst maintaining
c26ec799042a38 Geert Uytterhoeven 2024-02-28  134   * gcc's format checking.
c26ec799042a38 Geert Uytterhoeven 2024-02-28  135   */
c26ec799042a38 Geert Uytterhoeven 2024-02-28  136  #define dev_no_printk(level, dev, fmt, ...)				\
c26ec799042a38 Geert Uytterhoeven 2024-02-28  137  	({								\
c26ec799042a38 Geert Uytterhoeven 2024-02-28 @138  		if (0)							\
c26ec799042a38 Geert Uytterhoeven 2024-02-28  139  			_dev_printk(level, dev, fmt, ##__VA_ARGS__);	\
c26ec799042a38 Geert Uytterhoeven 2024-02-28  140  	})
c26ec799042a38 Geert Uytterhoeven 2024-02-28  141  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/2] iio: bu27034: Add a read only HWARDWAREGAIN

[kernel test robot]

Hi Matti,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0]

url:    https://github.com/intel-lab-lkp/linux/commits/Matti-Vaittinen/bu27034-ROHM-BU27034NUC-to-BU27034ANUC/20240610-180426
base:   1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
patch link:    https://lore.kernel.org/r/5e88c7b7b0389c6c011f15e05e065791f7561cf5.1718013518.git.mazziesaccount%40gmail.com
patch subject: [PATCH 2/2] iio: bu27034: Add a read only HWARDWAREGAIN
config: i386-buildonly-randconfig-004-20240610 (https://download.01.org/0day-ci/archive/20240610/202406102113.7w2Td20S-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240610/202406102113.7w2Td20S-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406102113.7w2Td20S-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/light/rohm-bu27034.c:1057:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
    1057 |         default:
         |         ^
   drivers/iio/light/rohm-bu27034.c:1057:2: note: insert '__attribute__((fallthrough));' to silence this warning
    1057 |         default:
         |         ^
         |         __attribute__((fallthrough)); 
   drivers/iio/light/rohm-bu27034.c:1057:2: note: insert 'break;' to avoid fall-through
    1057 |         default:
         |         ^
         |         break; 
   1 warning generated.

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for REGMAP_SPI
   Depends on [n]: SPI [=n]
   Selected by [y]:
   - AD9739A [=y] && IIO [=y] && (SPI [=n] || COMPILE_TEST [=y])


vim +1057 drivers/iio/light/rohm-bu27034.c

e52afbd61039e2 Matti Vaittinen 2023-03-31  1042  
d47b9b84292706 Matti Vaittinen 2023-06-13  1043  static int bu27034_write_raw_get_fmt(struct iio_dev *indio_dev,
d47b9b84292706 Matti Vaittinen 2023-06-13  1044  				     struct iio_chan_spec const *chan,
d47b9b84292706 Matti Vaittinen 2023-06-13  1045  				     long mask)
d47b9b84292706 Matti Vaittinen 2023-06-13  1046  {
6196c88810e86d Matti Vaittinen 2024-06-10  1047  	struct bu27034_data *data = iio_priv(indio_dev);
d47b9b84292706 Matti Vaittinen 2023-06-13  1048  
d47b9b84292706 Matti Vaittinen 2023-06-13  1049  	switch (mask) {
d47b9b84292706 Matti Vaittinen 2023-06-13  1050  	case IIO_CHAN_INFO_SCALE:
d47b9b84292706 Matti Vaittinen 2023-06-13  1051  		return IIO_VAL_INT_PLUS_NANO;
d47b9b84292706 Matti Vaittinen 2023-06-13  1052  	case IIO_CHAN_INFO_INT_TIME:
d47b9b84292706 Matti Vaittinen 2023-06-13  1053  		return IIO_VAL_INT_PLUS_MICRO;
6196c88810e86d Matti Vaittinen 2024-06-10  1054  	case IIO_CHAN_INFO_HARDWAREGAIN:
6196c88810e86d Matti Vaittinen 2024-06-10  1055  		dev_dbg(data->dev,
6196c88810e86d Matti Vaittinen 2024-06-10  1056  			"HARDWAREGAIN is read-only, use scale to set\n");
d47b9b84292706 Matti Vaittinen 2023-06-13 @1057  	default:
d47b9b84292706 Matti Vaittinen 2023-06-13  1058  		return -EINVAL;
d47b9b84292706 Matti Vaittinen 2023-06-13  1059  	}
d47b9b84292706 Matti Vaittinen 2023-06-13  1060  }
d47b9b84292706 Matti Vaittinen 2023-06-13  1061  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/2] iio: bu27034: Add a read only HWARDWAREGAIN

[Jonathan Cameron]

On Mon, 10 Jun 2024 13:01:40 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> The ROHM BU27034 light sensor has two data channels for measuring
> different frequencies of light. The result from these channels is
> combined into Lux value while the raw channel values are reported via
> intensity channels.
> 
> Both of the intensity channels have adjustable gain setting which
> impacts the scale of the raw channels. Eg, doubling the gain will double
> the values read from the raw channels, which halves the scale value. The
> integration time can also be set for the sensor. This does also have an
> impact to the scale of the intensity channels because increasing the
> integration time will also increase the values reported via the raw
> channels.
> 
> Impact of integration time to the scale and the fact that the scale value
> does not start from '1', can make it hard for a human reader to compute the
> gain values based on the scale.
> 
> Add read-only HARDWAREGAIN to help debugging.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Other than the thing the bot found with fallthrough on the switch statement
not being marked LGTM.
> ---
>  drivers/iio/light/rohm-bu27034.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/rohm-bu27034.c b/drivers/iio/light/rohm-bu27034.c
> index 51acad2cafbd..b299ff2aacce 100644
> --- a/drivers/iio/light/rohm-bu27034.c
> +++ b/drivers/iio/light/rohm-bu27034.c
> @@ -149,7 +149,8 @@ static const struct iio_itime_sel_mul bu27034_itimes[] = {
>  	.channel = BU27034_CHAN_##_name,				\
>  	.channel2 = IIO_MOD_LIGHT_CLEAR,				\
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
> -			      BIT(IIO_CHAN_INFO_SCALE),			\
> +			      BIT(IIO_CHAN_INFO_SCALE) |		\
> +			      BIT(IIO_CHAN_INFO_HARDWAREGAIN),		\
>  	.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),	\
>  	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),		\
>  	.info_mask_shared_by_all_available =				\
> @@ -992,6 +993,13 @@ static int bu27034_read_raw(struct iio_dev *idev,
>  
>  		return IIO_VAL_INT_PLUS_MICRO;
>  
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		ret = bu27034_get_gain(data, chan->channel, val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +
>  	case IIO_CHAN_INFO_SCALE:
>  		return bu27034_get_scale(data, chan->channel, val, val2);
>  
> @@ -1036,12 +1044,16 @@ static int bu27034_write_raw_get_fmt(struct iio_dev *indio_dev,
>  				     struct iio_chan_spec const *chan,
>  				     long mask)
>  {
> +	struct bu27034_data *data = iio_priv(indio_dev);
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
>  		return IIO_VAL_INT_PLUS_NANO;
>  	case IIO_CHAN_INFO_INT_TIME:
>  		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		dev_dbg(data->dev,
> +			"HARDWAREGAIN is read-only, use scale to set\n");

return -EINVAL here.  You could use a fall through marking but it gains
little so I wouldn't bother.

>  	default:
>  		return -EINVAL;
>  	}

Re: [PATCH 2/2] iio: bu27034: Add a read only HWARDWAREGAIN

[Matti Vaittinen]

On 6/15/24 20:50, Jonathan Cameron wrote:
> On Mon, 10 Jun 2024 13:01:40 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> The ROHM BU27034 light sensor has two data channels for measuring
>> different frequencies of light. The result from these channels is
>> combined into Lux value while the raw channel values are reported via
>> intensity channels.
>>
>> Both of the intensity channels have adjustable gain setting which
>> impacts the scale of the raw channels. Eg, doubling the gain will double
>> the values read from the raw channels, which halves the scale value. The
>> integration time can also be set for the sensor. This does also have an
>> impact to the scale of the intensity channels because increasing the
>> integration time will also increase the values reported via the raw
>> channels.
>>
>> Impact of integration time to the scale and the fact that the scale value
>> does not start from '1', can make it hard for a human reader to compute the
>> gain values based on the scale.
>>
>> Add read-only HARDWAREGAIN to help debugging.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> Other than the thing the bot found with fallthrough on the switch statement
> not being marked LGTM.
>> ---
>>   drivers/iio/light/rohm-bu27034.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/light/rohm-bu27034.c b/drivers/iio/light/rohm-bu27034.c
>> index 51acad2cafbd..b299ff2aacce 100644
>> --- a/drivers/iio/light/rohm-bu27034.c
>> +++ b/drivers/iio/light/rohm-bu27034.c
>> @@ -149,7 +149,8 @@ static const struct iio_itime_sel_mul bu27034_itimes[] = {
>>   	.channel = BU27034_CHAN_##_name,				\
>>   	.channel2 = IIO_MOD_LIGHT_CLEAR,				\
>>   	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
>> -			      BIT(IIO_CHAN_INFO_SCALE),			\
>> +			      BIT(IIO_CHAN_INFO_SCALE) |		\
>> +			      BIT(IIO_CHAN_INFO_HARDWAREGAIN),		\
>>   	.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),	\
>>   	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),		\
>>   	.info_mask_shared_by_all_available =				\
>> @@ -992,6 +993,13 @@ static int bu27034_read_raw(struct iio_dev *idev,
>>   
>>   		return IIO_VAL_INT_PLUS_MICRO;
>>   
>> +	case IIO_CHAN_INFO_HARDWAREGAIN:
>> +		ret = bu27034_get_gain(data, chan->channel, val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		return IIO_VAL_INT;
>> +
>>   	case IIO_CHAN_INFO_SCALE:
>>   		return bu27034_get_scale(data, chan->channel, val, val2);
>>   
>> @@ -1036,12 +1044,16 @@ static int bu27034_write_raw_get_fmt(struct iio_dev *indio_dev,
>>   				     struct iio_chan_spec const *chan,
>>   				     long mask)
>>   {
>> +	struct bu27034_data *data = iio_priv(indio_dev);
>>   
>>   	switch (mask) {
>>   	case IIO_CHAN_INFO_SCALE:
>>   		return IIO_VAL_INT_PLUS_NANO;
>>   	case IIO_CHAN_INFO_INT_TIME:
>>   		return IIO_VAL_INT_PLUS_MICRO;
>> +	case IIO_CHAN_INFO_HARDWAREGAIN:
>> +		dev_dbg(data->dev,
>> +			"HARDWAREGAIN is read-only, use scale to set\n");
> 
> return -EINVAL here.  You could use a fall through marking but it gains
> little so I wouldn't bother.

Right, thanks.

> 
>>   	default:
>>   		return -EINVAL;
>>   	}
> 

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

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