Re: [PATCH v7 06/10] iio: adc: Support ROHM BD79124 ADC
From: Andy Shevchenko
Date: Thu Mar 13 2025 - 09:19:28 EST
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