On Mon, 24 Feb 2025 20:34:30 +0200
Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
Some minor stuff inline.
Thanks,
Jonathan
> +
> +#define BD79124_REG_SYSTEM_STATUS 0x0
> +#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
> +
> +#define BD79124_ADC_BITS 12
> +#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_075 0
Can we make these units in these explicit?
#define BD79124_INTERVAL_MS_0_75
maybe? Nice to avoid need for comments on what the units are where
you use these.
> +#define BD79124_INTERVAL_150 1
> +#define BD79124_INTERVAL_300 2
> +#define BD79124_INTERVAL_600 3
> +
> +static int bd79124_enable_event(struct bd79124_data *data,
> + enum iio_event_direction dir, unsigned int channel)
> +{
> + int dir_bit = BIT(dir);
> + int reg;
> + u16 *limit;
> + int ret;
> +
> + guard(mutex)(&data->mutex);
> + /* Set channel to be measured */
> + ret = bd79124_start_measurement(data, channel);
> + if (ret)
> + return ret;
> +
> + data->alarm_monitored[channel] |= dir_bit;
> +
> + /* Add the channel to the list of monitored channels */
> + ret = regmap_set_bits(data->map, BD79124_REG_ALERT_CH_SEL,
> + BIT(channel));
> + if (ret)
> + return ret;
> +
> + if (dir == IIO_EV_DIR_RISING) {
> + limit = &data->alarm_f_limit[channel];
> + reg = BD79124_GET_HIGH_LIMIT_REG(channel);
> + } else {
> + limit = &data->alarm_f_limit[channel];
> + reg = BD79124_GET_LOW_LIMIT_REG(channel);
> + }
> + /* Don't write the new limit to the hardware if we are in the
> + * rate-limit period. The timer which re-enables the event will set
> + * the limit.
> + */
/*
* Don't
Check for other cases of this...
> +static void bd79124_re_enable_hi(struct bd79124_data *data, unsigned int channel)
> +{
> + int ret, evbit = BIT(IIO_EV_DIR_RISING);
> +
> + if (!(data->alarm_suppressed[channel] & evbit))
> + return;
> +
> + data->alarm_suppressed[channel] &= (~evbit);
No brackets around the ~evbit.
Check for other cases of this.
Otherwise we'll get some script written 'cleanup'.
> +
> + if (!(data->alarm_monitored[channel] & evbit))
> + return;
> +
> + ret = bd79124_write_int_to_reg(data, BD79124_GET_HIGH_LIMIT_REG(channel),
> + data->alarm_r_limit[channel]);
> + if (ret)
> + dev_warn(data->dev, "High limit enabling failed for channel%d\n",
> + channel);
> +}
> +
> +static void bd79124_alm_enable_worker(struct work_struct *work)
> +{
> + int i;
> + struct bd79124_data *data = container_of(work, struct bd79124_data,
> + alm_enable_work.work);
> +
> + guard(mutex)(&data->mutex);
> + /*
> + * We should not re-enable the event if user has disabled it while
> + * rate-limiting was enabled.
> + */
Is this comment suggesting something that isn't done or referring to specific
code? I think it wants to be in the function above where the decision is made.
> + for (i = 0; i < BD79124_MAX_NUM_CHANNELS; i++) {
> + bd79124_re_enable_hi(data, i);
> + bd79124_re_enable_lo(data, i);
> + }
> +}
> +
> +
> +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;
> +
> + 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);
Align this to less tabs as per discussion on previous version.
> +
> +
> +struct bd79124_reg_init {
> + int reg;
> + int val;
> +};
Not used any more.
> +
> +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);
> + if (ret)
> + return ret;
> +
> + return regmap_write(data->map, BD79124_GET_LOW_LIMIT_REG(channel), 0);
> +}
> +
> +static int bd79124_init_mux(struct bd79124_data *data, int gpio_pins)
> +{
> + return regmap_write(data->map, BD79124_REG_PINCFG, gpio_pins);
Maybe squash this inline. Doesn't seem to add a lot to have the helper.
> +}
> +
> +static int bd79124_hw_init(struct bd79124_data *data, int gpio_pins)
> +{
...
> +
> + /* Set the measurement interval to 0.75 mS */
This lead me to comment on defines. I'd rather code was fully self
documenting and remove the comment if we can. Makes for less chance of it
becoming out of sync over time.
> + regval = FIELD_PREP(BD79124_MASK_AUTO_INTERVAL, BD79124_INTERVAL_075);
> + ret = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
> + BD79124_MASK_AUTO_INTERVAL, regval);
> + if (ret)
> + return ret;
