Support ROHM BD79124 ADC

The new devm_iio_adc_device_alloc_chaninfo_se() -helper is intended to help drivers avoid open-coding the for_each_node -loop for getting the channel IDs. The helper provides standard way to detect the ADC channel nodes (by the node name), and a standard way to convert the "reg" -properties to channel identification numbers, used in the struct iio_chan_spec. Furthermore, the helper can optionally check the found channel IDs are smaller than given maximum. This is useful for callers which later use the IDs for example for indexing a channel data array.

The original driver treated all found child nodes as channel nodes. The new helper requires channel nodes to be named channel[@N]. This should help avoid problems with devices which may contain also other but ADC child nodes. Quick grep from arch/* with the rzg2l_adc's compatible string didn't reveal any in-tree .dts with channel nodes named otherwise. Also, same grep shows all the .dts seem to have channel IDs between 0..num of channels.

Use the new helper.

Signed-off-by Matti Vaittinen <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

Tested-by Claudiu Beznea<[email protected]>

Reviewed-by Claudiu Beznea<[email protected]>

The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports an automatic measurement mode, with an alarm interrupt for out-of-window measurements. The window is configurable for each channel.

The I2C protocol for manual start of the measurement and data reading is somewhat peculiar. It requires the master to do clock stretching after sending the I2C slave-address until the slave has captured the data. Needless to say this is not well suopported by the I2C controllers.

Thus do not support the BD79124's manual measurement mode but implement the measurements using automatic measurement mode, relying on the BD79124's ability of storing latest measurements into register.

Support also configuring the threshold events for detecting the out-of-window events.

The BD79124 keeps asserting IRQ for as long as the measured voltage is out of the configured window. Thus, prevent the user-space from choking on the events and mask the received event for a fixed duration (1 second) when an event is handled.

The ADC input pins can be also configured as general purpose outputs. Make those pins which don't have corresponding ADC channel node in the device-tree controllable as GPO.

+}

drivers/iio/adc/rohm-bd79124.c new file +4 -0

+	}
+
+	regmap_update_bits(data->map, BD79124_REG_GPO_VAL, *mask, *bits);
+}

drivers/iio/adc/rohm-bd79124.c new file +29 -0

+#define BD79124_RAW_TO_INT(r) ((r.val_bit4_11 << 4) | (r.val_bit0_3 >> 4))
+#define BD79124_INT_TO_RAW(val) {					\
+	.val_bit4_11 = (val) >> 4,					\
+	.val_bit0_3 = (val) << 4,					\
+}
+
+/*
+ * The high and low limits as well as the recent result values are stored in
+ * the same way in 2 consequent registers. The first register contains 4 bits
+ * of the value. These bits are stored in the high bits [7:4] of register, but
+ * they represent the low bits [3:0] of the value.
+ * The value bits [11:4] are stored in the next register.
+ *
+ * Read data from register and convert to integer.
+ */
+static int bd79124_read_reg_to_int(struct bd79124_data *data, int reg,
+				   unsigned int *val)
+{
+	int ret;
+	struct bd79124_raw raw;
+
+	ret = regmap_bulk_read(data->map, reg, &raw, sizeof(raw));
+	if (ret) {
+		dev_dbg(data->dev, "bulk_read failed %d\n", ret);
+
+		return ret;
+	}
+
+	*val = BD79124_RAW_TO_INT(raw);

+ * Convert the integer to register format and write it using rmw cycle.
+ */

drivers/iio/adc/rohm-bd79124.c new file +2 -0

+static int bd79124_write_int_to_reg(struct bd79124_data *data, int reg,
+				    unsigned int val)

drivers/iio/adc/rohm-bd79124.c new file +2 -0

+	return regmap_bulk_write(data->map, reg, &raw, sizeof(raw));
+}

+};

drivers/iio/adc/rohm-bd79124.c new file +4 -0

+	case IIO_EV_INFO_HYSTERESIS:
+		reg = BD79124_GET_HYSTERESIS_REG(chan->channel);
+		ret = regmap_read(data->map, reg, val);
+		if (ret)

drivers/iio/adc/rohm-bd79124.c new file +3 -0

+	default:
+		return -EINVAL;
+	}

drivers/iio/adc/rohm-bd79124.c new file +5 -0

+	/*
+	 * Set thresholds either to 0 or to 2^12 - 1 as appropriate to prevent
+	 * alerts and thus disable event generation.
+	 */
+	if (dir == IIO_EV_DIR_RISING) {

drivers/iio/adc/rohm-bd79124.c new file +13 -0

+	/*
+	 * Stop measurement if there is no more events to monitor.
+	 * We don't bother checking the retval because the limit
+	 * setting should in any case effectively disable the alarm.
+	 */
+	if (!data->alarm_monitored[channel]) {
+		bd79124_stop_measurement(data, channel);
+		regmap_clear_bits(data->map, BD79124_REG_ALERT_CH_SEL,
+				  BIT(channel));
+	}
+
+	return bd79124_write_int_to_reg(data, reg, limit);
+}

drivers/iio/adc/rohm-bd79124.c new file +36 -0

+		guard(mutex)(&data->mutex);
+
+		/*
+		 * Start the automatic conversion. This is needed here if no
+		 * events have been enabled.
+		 */
+		regval = FIELD_PREP(BD79124_MSK_CONV_MODE,
+				    BD79124_CONV_MODE_AUTO);
+		ret = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
+					 BD79124_MSK_CONV_MODE, regval);
+		if (ret)
+			return ret;
+
+		ret = bd79124_single_chan_seq(data, chan->channel, &old_chan_cfg);
+		if (ret)
+			return ret;
+
+		/* The maximum conversion time is 6 uS. */
+		udelay(6);
+
+		ret = bd79124_read_reg_to_int(data,
+			BD79124_GET_RECENT_RES_REG(chan->channel), val);
+		/*
+		 * Return the old chan config even if data reading failed in
+		 * order to re-enable the event monitoring.
+		 */
+		tmp = bd79124_single_chan_seq_end(data, old_chan_cfg);
+		if (tmp)
+			dev_err(data->dev,
+				"Failed to return config. Alarms may be disabled\n");
+
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	}

drivers/iio/adc/rohm-bd79124.c new file +19 -0

+	for (i = 0; i < BD79124_MAX_NUM_CHANNELS; i++) {
+		u64 ecode;
+
+		if (BIT(i) & i_hi) {
+			ecode = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,
+						     IIO_EV_TYPE_THRESH,
+						     IIO_EV_DIR_RISING);
+
+			iio_push_event(iio_dev, ecode, data->timestamp);
+			/*
+			 * The BD79124 keeps the IRQ asserted for as long as
+			 * the voltage exceeds the threshold. It causes the IRQ
+			 * to keep firing.
+			 *
+			 * Disable the event for the channel and schedule the
+			 * re-enabling the event later to prevent storm of
+			 * events.
+			 */
+			ret = bd79124_event_ratelimit_hi(data, i);

drivers/iio/adc/rohm-bd79124.c new file +5 -0

+			ret = bd79124_event_ratelimit_lo(data, i);
+			if (ret)
+				return IRQ_NONE;
+		}
+	}

(quoted diff) +2 -0

+		return ret;
+	}

Quoted 24 lines · Matti Vaittinen

The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports an automatic measurement mode, with an alarm interrupt for out-of-window measurements. The window is configurable for each channel.

The I2C protocol for manual start of the measurement and data reading is somewhat peculiar. It requires the master to do clock stretching after sending the I2C slave-address until the slave has captured the data. Needless to say this is not well suopported by the I2C controllers.

Thus do not support the BD79124's manual measurement mode but implement the measurements using automatic measurement mode, relying on the BD79124's ability of storing latest measurements into register.

Support also configuring the threshold events for detecting the out-of-window events.

The BD79124 keeps asserting IRQ for as long as the measured voltage is out of the configured window. Thus, prevent the user-space from choking on the events and mask the received event for a fixed duration (1 second) when an event is handled.

The ADC input pins can be also configured as general purpose outputs. Make those pins which don't have corresponding ADC channel node in the device-tree controllable as GPO.

...

drivers/iio/adc/rohm-bd79124.c line 205

Quoted 8 lines

drivers/iio/adc/rohm-bd79124.c new file +8 -0

+static void bd79124gpo_set(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	struct bd79124_data *data = gpiochip_get_data(gc);
+
+	if (value)
+		regmap_set_bits(data->map, BD79124_REG_GPO_VAL, BIT(offset));
+	else
+		regmap_clear_bits(data->map, BD79124_REG_GPO_VAL, BIT(offset));

Hmm... Is there still any obstacle to use regmap_assign_bits()?

Quoted 1 line

+}

...

Quoted 11 lines

drivers/iio/adc/rohm-bd79124.c new file +11 -0

+static void bd79124gpo_set_multiple(struct gpio_chip *gc, unsigned long *mask,
+				    unsigned long *bits)
+{
+	unsigned int val;
+	int ret;
+	struct bd79124_data *data = gpiochip_get_data(gc);
+
+	/* Ensure all GPIOs in 'mask' are set to be GPIOs */
+	ret = regmap_read(data->map, BD79124_REG_PINCFG, &val);
+	if (ret)
+		return;

drivers/iio/adc/rohm-bd79124.c line 220

Quoted 1 line

drivers/iio/adc/rohm-bd79124.c new file +1 -0

+	if ((val & *mask) != *mask) {

This is basically an equivalent to

	if (val ^ *mask) {

drivers/iio/adc/rohm-bd79124.c line 223

Quoted 4 lines

drivers/iio/adc/rohm-bd79124.c new file +3 -0

+		dev_dbg(data->dev, "Invalid mux config. Can't set value.\n");
 
+		/* Do not set value for pins configured as ADC inputs */
+		*mask &= val;

But this still puzzles me. If we have a valid_mask set, this should never happen. Maybe I missed something, though.

On top of that I have just realised that you modifying the input here

which may ne quite surprising (I dunno why we don't have const specifier in the callbacks from day 1.

Quoted 4 lines

drivers/iio/adc/rohm-bd79124.c new file +4 -0

+	}
+
+	regmap_update_bits(data->map, BD79124_REG_GPO_VAL, *mask, *bits);
+}

...

drivers/iio/adc/rohm-bd79124.c line 255

Quoted 4 lines

drivers/iio/adc/rohm-bd79124.c new file +4 -0

+struct bd79124_raw {
+	u8 val_bit0_3; /* Is set in high bits of the byte */
+	u8 val_bit4_11;
+};

Again, this is confusing.

Just put a bit order map in the comment as I suggested previously. When I see variable name containing bit range like above I think about the same bit order, i.e. with your comment it makes like this

bit number 7 6 5 4 3 2 1 0
data bit 0 1 2 3 x x x x

All the same for the second variable.

Quoted 35 lines

drivers/iio/adc/rohm-bd79124.c new file +34 -0

+#define BD79124_RAW_TO_INT(r) ((r.val_bit4_11 << 4) | (r.val_bit0_3 >> 4))
+#define BD79124_INT_TO_RAW(val) {					\
+	.val_bit4_11 = (val) >> 4,					\
+	.val_bit0_3 = (val) << 4,					\
+}
+
+/*
+ * The high and low limits as well as the recent result values are stored in
+ * the same way in 2 consequent registers. The first register contains 4 bits
+ * of the value. These bits are stored in the high bits [7:4] of register, but
+ * they represent the low bits [3:0] of the value.
+ * The value bits [11:4] are stored in the next register.
+ *
+ * Read data from register and convert to integer.
+ */
+static int bd79124_read_reg_to_int(struct bd79124_data *data, int reg,
+				   unsigned int *val)
+{
+	int ret;
+	struct bd79124_raw raw;
+
+	ret = regmap_bulk_read(data->map, reg, &raw, sizeof(raw));
+	if (ret) {
+		dev_dbg(data->dev, "bulk_read failed %d\n", ret);
+
+		return ret;
+	}
+
+	*val = BD79124_RAW_TO_INT(raw);
+
+	return 0;
+}
+
+/*

Quoted 5 lines

+ * The high and low limits as well as the recent result values are stored in
+ * the same way in 2 consequent registers. The first register contains 4 bits
+ * of the value. These bits are stored in the high bits [7:4] of register, but
+ * they represent the low bits [3:0] of the value.
+ * The value bits [11:4] are stored in the next register.

And you probably don't need a long comment copied twice when data type is well described.

Quoted 4 lines

+ * Convert the integer to register format and write it using rmw cycle.
+ */

drivers/iio/adc/rohm-bd79124.c new file +2 -0

+static int bd79124_write_int_to_reg(struct bd79124_data *data, int reg,
+				    unsigned int val)

drivers/iio/adc/rohm-bd79124.c line 309

Quoted 10 lines

drivers/iio/adc/rohm-bd79124.c new file +9 -0

+{
+	struct bd79124_raw raw = BD79124_INT_TO_RAW(val);
+	unsigned int tmp;
+	int ret;
+
+	ret = regmap_read(data->map, reg, &tmp);
+	if (ret)
+		return ret;
 
+	raw.val_bit0_3 |= (tmp & 0xf);

Parentheses are not needed. Bit we already discussed that and it's principal disagreement between us. :-(

Quoted 2 lines

drivers/iio/adc/rohm-bd79124.c new file +2 -0

+	return regmap_bulk_write(data->map, reg, &raw, sizeof(raw));
+}

...

drivers/iio/adc/rohm-bd79124.c line 347

Quoted 7 lines

drivers/iio/adc/rohm-bd79124.c new file +7 -0

+static const struct iio_chan_spec bd79124_chan_template = {
+	.type = IIO_VOLTAGE,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+	.indexed = 1,
+	.event_spec = bd79124_events,
+	.num_event_specs = ARRAY_SIZE(bd79124_events),

+ array_size.h which is missing.

Quoted 1 line

+};

...

drivers/iio/adc/rohm-bd79124.c line 373

Quoted 12 lines

drivers/iio/adc/rohm-bd79124.c new file +11 -0

+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		if (dir == IIO_EV_DIR_RISING)
+			*val = data->alarm_r_limit[chan->channel];
+		else if (dir == IIO_EV_DIR_FALLING)
+			*val = data->alarm_f_limit[chan->channel];
+		else
+			return -EINVAL;
+
+		return IIO_VAL_INT;
 
+

Blank line...

Quoted 4 lines

drivers/iio/adc/rohm-bd79124.c new file +4 -0

+	case IIO_EV_INFO_HYSTERESIS:
+		reg = BD79124_GET_HYSTERESIS_REG(chan->channel);
+		ret = regmap_read(data->map, reg, val);
+		if (ret)

drivers/iio/adc/rohm-bd79124.c line 387

Quoted 10 lines

drivers/iio/adc/rohm-bd79124.c new file +10 -0

+			return ret;
+
+		*val &= BD79124_MSK_HYSTERESIS;
+		/*
+		 * The data-sheet says the hysteresis register value needs to be
+		 * sifted left by 3.
+		 */
+		*val <<= 3;
+
+		return IIO_VAL_INT;

...no blank line. Can you choose one style and use it everywhere?

Quoted 3 lines

drivers/iio/adc/rohm-bd79124.c new file +3 -0

+	default:
+		return -EINVAL;
+	}

...

drivers/iio/adc/rohm-bd79124.c line 693

Quoted 4 lines

drivers/iio/adc/rohm-bd79124.c new file +4 -0

+	case IIO_CHAN_INFO_RAW:
+	{
+		unsigned int old_chan_cfg, regval;
+		int tmp;

This tmp sounds to me like

int ret2; // or rc, retval, etc.

Quoted 37 lines

drivers/iio/adc/rohm-bd79124.c new file +36 -0

+		guard(mutex)(&data->mutex);
+
+		/*
+		 * Start the automatic conversion. This is needed here if no
+		 * events have been enabled.
+		 */
+		regval = FIELD_PREP(BD79124_MSK_CONV_MODE,
+				    BD79124_CONV_MODE_AUTO);
+		ret = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
+					 BD79124_MSK_CONV_MODE, regval);
+		if (ret)
+			return ret;
+
+		ret = bd79124_single_chan_seq(data, chan->channel, &old_chan_cfg);
+		if (ret)
+			return ret;
+
+		/* The maximum conversion time is 6 uS. */
+		udelay(6);
+
+		ret = bd79124_read_reg_to_int(data,
+			BD79124_GET_RECENT_RES_REG(chan->channel), val);
+		/*
+		 * Return the old chan config even if data reading failed in
+		 * order to re-enable the event monitoring.
+		 */
+		tmp = bd79124_single_chan_seq_end(data, old_chan_cfg);
+		if (tmp)
+			dev_err(data->dev,
+				"Failed to return config. Alarms may be disabled\n");
+
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	}

...

Quoted 23 lines

drivers/iio/adc/rohm-bd79124.c new file +23 -0

+	for (i = 0; i < BD79124_MAX_NUM_CHANNELS; i++) {
+		u64 ecode;
+
+		if (BIT(i) & i_hi) {
+			ecode = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,
+						     IIO_EV_TYPE_THRESH,
+						     IIO_EV_DIR_RISING);
+
+			iio_push_event(iio_dev, ecode, data->timestamp);
+			/*
+			 * The BD79124 keeps the IRQ asserted for as long as
+			 * the voltage exceeds the threshold. It causes the IRQ
+			 * to keep firing.
+			 *
+			 * Disable the event for the channel and schedule the
+			 * re-enabling the event later to prevent storm of
+			 * events.
+			 */
+			ret = bd79124_event_ratelimit_hi(data, i);
+			if (ret)
+				return IRQ_NONE;
+		}
+		if (BIT(i) & i_lo) {

drivers/iio/adc/rohm-bd79124.c line 904

Quoted 5 lines

drivers/iio/adc/rohm-bd79124.c new file +5 -0

+			ecode = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,
+						     IIO_EV_TYPE_THRESH,
+						     IIO_EV_DIR_FALLING);
+
+			iio_push_event(iio_dev, ecode, data->timestamp);

Sounds like possibility to deduplicate as the above only differs by ecode.

Quoted 6 lines

drivers/iio/adc/rohm-bd79124.c new file +5 -0

+			ret = bd79124_event_ratelimit_lo(data, i);
+			if (ret)
+				return IRQ_NONE;
+		}
+	}

...

drivers/iio/adc/rohm-bd79124.c line 1064

Quoted 6 lines

drivers/iio/adc/rohm-bd79124.c new file +6 -0

+	ret = devm_iio_adc_device_alloc_chaninfo_se(dev, template,
+		BD79124_MAX_NUM_CHANNELS - 1, &cs);
+	if (ret < 0) {
+		/* Register all pins as GPOs if there are no ADC channels */
+		if (ret == -ENOENT)
+			goto register_gpios;

It's possible to move outside of the above conditional, but I think I got your idea, however it is still not a hot path.

Review

Quoted 2 lines

(quoted diff) +2 -0

+		return ret;
+	}

Quoted 24 lines · Matti Vaittinen

The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports an automatic measurement mode, with an alarm interrupt for out-of-window measurements. The window is configurable for each channel.

The I2C protocol for manual start of the measurement and data reading is somewhat peculiar. It requires the master to do clock stretching after sending the I2C slave-address until the slave has captured the data. Needless to say this is not well suopported by the I2C controllers.

Thus do not support the BD79124's manual measurement mode but implement the measurements using automatic measurement mode, relying on the BD79124's ability of storing latest measurements into register.

Support also configuring the threshold events for detecting the out-of-window events.

The BD79124 keeps asserting IRQ for as long as the measured voltage is out of the configured window. Thus, prevent the user-space from choking on the events and mask the received event for a fixed duration (1 second) when an event is handled.

The ADC input pins can be also configured as general purpose outputs. Make those pins which don't have corresponding ADC channel node in the device-tree controllable as GPO.

...

drivers/iio/adc/rohm-bd79124.c line 255

Quoted 4 lines

drivers/iio/adc/rohm-bd79124.c new file +4 -0

+struct bd79124_raw {
+	u8 val_bit0_3; /* Is set in high bits of the byte */
+	u8 val_bit4_11;
+};

Again, this is confusing.

Just put a bit order map in the comment as I suggested previously. When I see variable name containing bit range like above I think about the same bit order, i.e. with your comment it makes like this

bit number 7 6 5 4 3 2 1 0
data bit 0 1 2 3 x x x x

bit number 7 6 5 4 3 2 1 0
data bit 3 2 1 0 x x x x

bit number 7 6 5 4 3 2 1 0
data bit b a 9 8 7 6 5 4

Quoted 1 line · Andy Shevchenko

On Thu, Mar 20, 2025 at 10:22:00AM +0200, Matti Vaittinen wrote:

Quoted 7 lines

drivers/iio/adc/rohm-bd79124.c line 255

Quoted 4 lines

drivers/iio/adc/rohm-bd79124.c new file +4 -0

+struct bd79124_raw {
+	u8 val_bit0_3; /* Is set in high bits of the byte */
+	u8 val_bit4_11;
+};

Again, this is confusing.

Quoted 6 lines

Just put a bit order map in the comment as I suggested previously. When I see variable name containing bit range like above I think about the same bit order, i.e. with your comment it makes like this

bit number 7 6 5 4 3 2 1 0
data bit 0 1 2 3 x x x x

Gah. I think I now understand what you're after. And, I agree, I haven't been as clear as I could've been.

The pit numbers in the struct members:

	u8 val_bit0_3; and u8 val_bit4_11;

are _not_ intended to represent the bit ordering - only the bit positions. Like, bits from bit 0 to bit 3 are stored in high bits of this u8 - where the "0 to 3" was just picked as order based on it being from the smaller to greater (which I believe is grammatically typical) - not based on how the bits are ordered in the register. If the order of the bits was indeed reverted, then we should see much more complex conversions than what is presented in these macros.

I will update the variable names to:

val_bit3_0; and val_bit11_4; I think it should sort out the confusion. I won't go to bit level representation of the full registers:

Quoted 2 lines

bit number 7 6 5 4 3 2 1 0
data bit 3 2 1 0 x x x x

and

Quoted 2 lines

bit number 7 6 5 4 3 2 1 0
data bit b a 9 8 7 6 5 4

because it suggests there is something very strange in the registers (which is not the case) - and it is hard to spot if some bits have indeed changed the place.

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Thu, Mar 20, 2025 at 10:22:00AM +0200, Matti Vaittinen wrote:

You can get rid of all of these by simply using __le16. I do not understand why it's not used so far. I thought that bits are mirrored, that may explain the case, but now I do not see any problem to use __le16 directly.

And how this is different from treating it as __le16? Needs a good comment about bit layout.

drivers/iio/adc/rohm-bd79124.c line 255

Quoted 4 lines

drivers/iio/adc/rohm-bd79124.c new file +4 -0

+struct bd79124_raw {
+	u8 bit0_3; /* Is set in high bits of the byte */
+	u8 bit4_11;
+};

Quoted 7 lines · Andy Shevchenko

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Thu, Mar 20, 2025 at 10:22:00AM +0200, Matti Vaittinen wrote:

You can get rid of all of these by simply using __le16. I do not understand why it's not used so far. I thought that bits are mirrored, that may explain the case, but now I do not see any problem to use __le16 directly.

This discussion is going in circles now. That was discussed in the RFC review with Jonathan, which I did also tell to you during the v7 review:

Quoted 11 lines · Matti Vaittinen

Quoted 7 lines · Andy Shevchenko

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Thu, Mar 20, 2025 at 10:22:00AM +0200, Matti Vaittinen wrote:

You can get rid of all of these by simply using __le16. I do not understand why it's not used so far. I thought that bits are mirrored, that may explain the case, but now I do not see any problem to use __le16 directly.

This discussion is going in circles now. That was discussed in the RFC review with Jonathan, which I did also tell to you during the v7 review:

Yes, because I think we all were confused by the bits representations, but now I see it clearly and I do not understand why should we go the way you suggested as it makes things a bit tangled in my opinion.

Jonathan, do you still think the two separate bytes are better than __le16? If so, what are the pros of this solution?

Quoted 13 lines · Andy Shevchenko

Quoted 11 lines · Matti Vaittinen

Quoted 7 lines · Andy Shevchenko

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Thu, Mar 20, 2025 at 10:22:00AM +0200, Matti Vaittinen wrote:

You can get rid of all of these by simply using __le16. I do not understand why it's not used so far. I thought that bits are mirrored, that may explain the case, but now I do not see any problem to use __le16 directly.

This discussion is going in circles now. That was discussed in the RFC review with Jonathan, which I did also tell to you during the v7 review:

Quoted 6 lines

Yes, because I think we all were confused by the bits representations, but now I see it clearly and I do not understand why should we go the way you suggested as it makes things a bit tangled in my opinion.

Jonathan, do you still think the two separate bytes are better than __le16? If so, what are the pros of this solution?

I don't think Jonathan thought this is better. I'm not sure if you read the RFC conversation.

I told this is easier for me to understand. Jonathan merely told he can live with that. For this particular driver it matters because I'm expecting to be maintaining it. It's easier to maintain code which one can understand, and if subsystem maintainer can live with it, then I suppose it's the pro you are looking for.

Quoted 24 lines · Matti Vaittinen

Quoted 13 lines · Andy Shevchenko

Quoted 11 lines · Matti Vaittinen

Quoted 7 lines · Andy Shevchenko

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Thu, Mar 20, 2025 at 10:22:00AM +0200, Matti Vaittinen wrote:

You can get rid of all of these by simply using __le16. I do not understand why it's not used so far. I thought that bits are mirrored, that may explain the case, but now I do not see any problem to use __le16 directly.

This discussion is going in circles now. That was discussed in the RFC review with Jonathan, which I did also tell to you during the v7 review:

Quoted 6 lines

Yes, because I think we all were confused by the bits representations, but now I see it clearly and I do not understand why should we go the way you suggested as it makes things a bit tangled in my opinion.

Jonathan, do you still think the two separate bytes are better than __le16? If so, what are the pros of this solution?

I don't think Jonathan thought this is better. I'm not sure if you read the RFC conversation.

Quoted 5 lines

I told this is easier for me to understand. Jonathan merely told he can live with that. For this particular driver it matters because I'm expecting to be maintaining it. It's easier to maintain code which one can understand, and if subsystem maintainer can live with it, then I suppose it's the pro you are looking for.

What if the maintainer will be hit by a bus? The point is we should also think for unfamiliar possible maintainers and strange readers.