Re: [PATCH v7 06/10] iio: adc: Support ROHM BD79124 ADC

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Mar 13, 2025 at 09:19:03AM +0200, Matti Vaittinen wrote:
> 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 the driver does not support the BD79124's manual measurement mode
> but implements the measurements using automatic measurement mode relying
> on the BD79124's ability of storing latest measurements into register.
> 
> The driver does also support 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 the driver masks the received event
> for a fixed duration (1 second) when an event is handled. This prevents
> the user-space from choking on the events
> 
> The ADC input pins can be also configured as general purpose outputs.
> Those pins which don't have corresponding ADC channel node in the
> device-tree will be controllable as GPO.

...

> +#define BD79124_REG_SYSTEM_STATUS	0x0

I would expect to see 0x00

> +#define BD79124_REG_GEN_CFG		0x01
> +#define BD79124_REG_OPMODE_CFG		0x04
> +#define BD79124_REG_PINCFG		0x05
> +#define BD79124_REG_GPO_VAL		0x0B
> +#define BD79124_REG_SEQUENCE_CFG	0x10
> +#define BD79124_REG_MANUAL_CHANNELS	0x11
> +#define BD79124_REG_AUTO_CHANNELS	0x12
> +#define BD79124_REG_ALERT_CH_SEL	0x14
> +#define BD79124_REG_EVENT_FLAG		0x18
> +#define BD79124_REG_EVENT_FLAG_HI	0x1a
> +#define BD79124_REG_EVENT_FLAG_LO	0x1c
> +#define BD79124_REG_HYSTERESIS_CH0	0x20
> +#define BD79124_REG_EVENTCOUNT_CH0	0x22
> +#define BD79124_REG_RECENT_CH0_LSB	0xa0
> +#define BD79124_REG_RECENT_CH7_MSB	0xaf

...

Wouldn't be better...

> +#define BD79124_MASK_CONV_MODE GENMASK(6, 5)
> +#define BD79124_MASK_AUTO_INTERVAL GENMASK(1, 0)
> +#define BD79124_CONV_MODE_MANSEQ 0
> +#define BD79124_CONV_MODE_AUTO 1
> +#define BD79124_INTERVAL_750_US 0

To group masks and values of the same bitfields?

#define BD79124_MASK_CONV_MODE GENMASK(6, 5)
#define BD79124_CONV_MODE_MANSEQ 0
#define BD79124_CONV_MODE_AUTO 1
#define BD79124_MASK_AUTO_INTERVAL GENMASK(1, 0)
#define BD79124_INTERVAL_750_US 0

> +#define BD79124_MASK_DWC_EN BIT(4)
> +#define BD79124_MASK_STATS_EN BIT(5)
> +#define BD79124_MASK_SEQ_START BIT(4)
> +#define BD79124_MASK_SEQ_MODE GENMASK(1, 0)
> +#define BD79124_MASK_SEQ_MANUAL 0
> +#define BD79124_MASK_SEQ_SEQ 1
> +
> +#define BD79124_MASK_HYSTERESIS GENMASK(3, 0)
> +#define BD79124_LOW_LIMIT_MIN 0
> +#define BD79124_HIGH_LIMIT_MAX GENMASK(11, 0)

These masks are not named after registers (or I don't see it clearly), it's
hard to understand which one relates to which register. Also, why not using
bitfield.h?

...

> + * These macros return the address of the 1.st reg for the given channel

first

(and missing period at the end).

> +#define BD79124_GET_HIGH_LIMIT_REG(ch) (BD79124_REG_HYSTERESIS_CH0 + (ch) * 4)
> +#define BD79124_GET_LOW_LIMIT_REG(ch) (BD79124_REG_EVENTCOUNT_CH0 + (ch) * 4)
> +#define BD79124_GET_LIMIT_REG(ch, dir) ((dir) == IIO_EV_DIR_RISING ?		\
> +		BD79124_GET_HIGH_LIMIT_REG(ch) : BD79124_GET_LOW_LIMIT_REG(ch))
> +#define BD79124_GET_RECENT_RES_REG(ch) (BD79124_REG_RECENT_CH0_LSB + (ch) * 2)

Don't we want to have something in bitfield.h for arrays in the register, i.e.
when register(s) is(are) split to 2+ bits per an element in an array of the
same semantics. Would be nice to eliminate such a boilerplate here and in many
other drivers.

> +/*
> + * The hysteresis for a channel is stored in the same register where the
> + * 4 bits of high limit reside.
> + */
> +#define BD79124_GET_HYSTERESIS_REG(ch) BD79124_GET_HIGH_LIMIT_REG(ch)
> +
> +#define BD79124_MAX_NUM_CHANNELS 8
> +
> +struct bd79124_data {
> +	s64 timestamp;
> +	struct regmap *map;
> +	struct device *dev;
> +	int vmax;
> +	/*
> +	 * Keep measurement status so read_raw() knows if the measurement needs
> +	 * to be started.
> +	 */
> +	int alarm_monitored[BD79124_MAX_NUM_CHANNELS];
> +	/*
> +	 * The BD79124 does not allow disabling/enabling limit separately for
> +	 * one direction only. Hence, we do the disabling by changing the limit
> +	 * to maximum/minimum measurable value. This means we need to cache
> +	 * the limit in order to maintain it over the time limit is disabled.
> +	 */
> +	u16 alarm_r_limit[BD79124_MAX_NUM_CHANNELS];
> +	u16 alarm_f_limit[BD79124_MAX_NUM_CHANNELS];
> +	/* Bitmask of disabled events (for rate limiting) for each channel. */
> +	int alarm_suppressed[BD79124_MAX_NUM_CHANNELS];
> +	/*
> +	 * The BD79124 is configured to run the measurements in the background.
> +	 * This is done for the event monitoring as well as for the read_raw().
> +	 * Protect the measurement starting/stopping using a mutex.
> +	 */
> +	struct mutex mutex;
> +	struct delayed_work alm_enable_work;
> +	struct gpio_chip gc;
> +	u8 gpio_valid_mask;
> +};

...

> +static void bd79124gpo_set(struct gpio_chip *gc, unsigned int offset, int value)

You should use .set_rv()

...

> +static void bd79124gpo_set_multiple(struct gpio_chip *gc, unsigned long *mask,
> +				    unsigned long *bits)

Ditto, .set_multiple_rv().

> +{
> +	int ret, val;

Here and everywhere else, the returned value by regmap is unsigned int.

> +	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;
> +
> +	if ((val & *mask) != *mask) {
> +		dev_dbg(data->dev, "Invalid mux config. Can't set value.\n");
> +		/* Do not set value for pins configured as ADC inputs */
> +		*mask &= val;
> +	}
> +
> +	regmap_update_bits(data->map, BD79124_REG_GPO_VAL, *mask, *bits);
> +}

...

> +struct bd79124_raw {
> +	u8 bit0_3; /* Is set in high bits of the byte */
> +	u8 bit4_11;
> +};
> +#define BD79124_RAW_TO_INT(r) ((r.bit4_11 << 4) | (r.bit0_3 >> 4))

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

...

> +static int bd79124_write_int_to_reg(struct bd79124_data *data, int reg,
> +				    unsigned int val)
> +{
> +	struct bd79124_raw raw;
> +	int ret, tmp;

> +	raw.bit4_11 = (u8)(val >> 4);
> +	raw.bit0_3 = (u8)(val << 4);

Why casting?


Make sense to have a symmetrical macro instead of hiding it in the code.

> +	ret = regmap_read(data->map, reg, &tmp);
> +	if (ret)
> +		return ret;
> +
> +	raw.bit0_3 |= (0xf & tmp);

GENMASK() ?

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

...

> +		/*
> +		 * The data-sheet says the hysteresis register value needs to be
> +		 * sifted left by 3

Missing period.

> +		 */

...

> +	return (data->alarm_monitored[chan->channel] & BIT(dir));

Unneeded parentheses.

...

> +	case IIO_EV_INFO_HYSTERESIS:
> +		reg = BD79124_GET_HYSTERESIS_REG(chan->channel);
> +		val >>= 3;

Second time I see this. Perhaps you need a macro to explain this right shift?

> +		return regmap_update_bits(data->map, reg, BD79124_MASK_HYSTERESIS,
> +					  val);

Can be one line.

...

> +static irqreturn_t bd79124_event_handler(int irq, void *priv)
> +{
> +	int ret, i_hi, i_lo, i;
> +	struct iio_dev *iio_dev = priv;
> +	struct bd79124_data *data = iio_priv(iio_dev);
> +
> +	/*
> +	 * Return IRQ_NONE if bailing-out without acking. This allows the IRQ
> +	 * subsystem to disable the offending IRQ line if we get a hardware
> +	 * problem. This behaviour has saved my poor bottom a few times in the
> +	 * past as, instead of getting unusably unresponsive, the system has
> +	 * spilled out the magic words "...nobody cared".
> +	 */
> +	ret = regmap_read(data->map, BD79124_REG_EVENT_FLAG_HI, &i_hi);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	ret = regmap_read(data->map, BD79124_REG_EVENT_FLAG_LO, &i_lo);
> +	if (ret)
> +		return IRQ_NONE;

Only I don't get why you can't use bulk read here.
The registers seem to be sequential.

> +	if (!i_lo && !i_hi)
> +		return IRQ_NONE;
> +
> +	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) {
> +			ecode = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,
> +						     IIO_EV_TYPE_THRESH,
> +						     IIO_EV_DIR_FALLING);
> +
> +			iio_push_event(iio_dev, ecode, data->timestamp);
> +			ret = bd79124_event_ratelimit_lo(data, i);
> +			if (ret)
> +				return IRQ_NONE;
> +		}
> +	}
> +
> +	ret = regmap_write(data->map, BD79124_REG_EVENT_FLAG_HI, i_hi);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	ret = regmap_write(data->map, BD79124_REG_EVENT_FLAG_LO, i_lo);
> +	if (ret)
> +		return IRQ_NONE;

In the similar way bulk write.

> +	return IRQ_HANDLED;
> +}

...

> +static int bd79124_chan_init(struct bd79124_data *data, int channel)
> +{
> +	int ret;
> +
> +	ret = regmap_write(data->map, BD79124_GET_HIGH_LIMIT_REG(channel), 4095);

BD79124_HIGH_LIMIT_MAX ?

> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(data->map, BD79124_GET_LOW_LIMIT_REG(channel), 0);

BD79124_HIGH_LIMIT_MIN ?

> +}

...

> +	for (i = 0; i < BD79124_MAX_NUM_CHANNELS; i++) {
> +		ret = bd79124_chan_init(data, i);
> +		if (ret)
> +			return ret;
> +		data->alarm_r_limit[i] = 4095;

As per above?

> +	}

...

> +	gpio_pins = bd79124_get_gpio_pins(iio_dev->channels,
> +					  iio_dev->num_channels);

It may be a single line (84 characters).

...

> +static const struct i2c_device_id bd79124_id[] = {
> +	{ "bd79124", },

Redundant inner comma.

> +	{ }
> +};


-- 
With Best Regards,
Andy Shevchenko