On 23/02/2025 18:28, Jonathan Cameron wrote:
On Wed, 19 Feb 2025 14:30:43 +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>
Hi Matti,
Some fairly superficial review follows. I'm travelling for next few weeks
so not sure when I'll get time to take a more thorough look.
Yeah, unfortunately people are allowed to have other life beyond the
ROHM drivers :D
Enjoy your journey(s) ;)
...
+
+static int bd79124_event_ratelimit_hi(struct bd79124_data *data,
+ unsigned int channel)
+{
+ int reg, limit;
+
+ guard(mutex)(&data->mutex);
+ data->alarm_suppressed[channel] |= BIT(IIO_EV_DIR_RISING);
+
+ reg = BD79124_GET_HIGH_LIMIT_REG(channel);
+ limit = BD79124_HIGH_LIMIT_MAX;
+
+ return __bd79124_event_ratelimit(data, reg, limit);
As below.
+}
+
+static int bd79124_event_ratelimit_lo(struct bd79124_data *data,
+ unsigned int channel)
+{
+ int reg, limit;
+
+ guard(mutex)(&data->mutex);
+ data->alarm_suppressed[channel] |= BIT(IIO_EV_DIR_FALLING);
+
+ reg = BD79124_GET_LOW_LIMIT_REG(channel);
+ limit = BD79124_LOW_LIMIT_MIN;
+
+ return __bd79124_event_ratelimit(data, reg, limit);
I'd put reg and limit inline. Local variables don't add much as
their meaning is obvious anyway from what you put in them.
I can do this. The main purpose of those variables was to keep the
function calls easier to read (on my limited monitor).
+}
+
...
+
+static int bd79124_chan_init(struct bd79124_data *data, int channel)
+{
+ struct bd79124_reg_init inits[] = {
+ { .reg = BD79124_GET_HIGH_LIMIT_REG(channel), .val = 4095 },
+ { .reg = BD79124_GET_LOW_LIMIT_REG(channel), .val = 0 },
+ };
+ int i, ret;
+
+ for (i = 0; i < ARRAY_SIZE(inits); i++) {
+ ret = regmap_write(data->map, inits[i].reg, inits[i].val);
+ if (ret)
+ return ret;
+ }
This is shorter as straight line code rather than a loop. I'd unwind
it. Fine to bring in a loop 'setter' like this once the benefit is
significant.
I suppose you're right. I think loops like this born out of a habit :)
+
+ return 0;
+}
+
+static bool bd79124_is_in_array(int *arr, int num_items, int val)
+{
+ int i;
+
+ for (i = 0; i < num_items; i++)
+ if (arr[i] == val)
+ return true;
+
+ return false;
+}
+
+static int bd79124_mux_init(struct bd79124_data *data)
+{
+ int adc_chans[BD79124_MAX_NUM_CHANNELS];
+ int num_adc, chan, regval = 0;
+
+ num_adc = iio_adc_device_channels_by_property(data->dev, &adc_chans[0],
+ BD79124_MAX_NUM_CHANNELS,
+ &expected_props);
+ if (num_adc < 0)
+ return num_adc;
+
+ /*
+ * Set a mux register bit for each pin which is free to be used as
+ * a GPO.
For this I would search the simpler iio_chan_spec array rather than passing
properties again.
I kind of agree. I did it like this because I thought that the
'iio_adc_device_channels_by_property()' might be useful for other
callers as well. And, if we had 'iio_adc_device_channels_by_property()'
- then the code in this driver file becomes simple (as seen here). After
I looked at the couple of other drivers I didn't easily spot any other
driver needing the 'iio_adc_device_channels_by_property()' - so I
suppose it is simpler to drop it and loop through the 'iio_chan_spec' as
you suggest.
Just look for gaps. Or do it in the top level probe()
function and build a bitmap of which channels are ADC ones from the iio_chan_spec
array and pass that down here.
+ */
+ for (chan = 0; chan < BD79124_MAX_NUM_CHANNELS; chan++)
+ if (!bd79124_is_in_array(&adc_chans[0], num_adc, chan))
+ regval |= BIT(chan);
+
+ return regmap_write(data->map, BD79124_REG_PINCFG, regval);
+}
+
+static int bd79124_hw_init(struct bd79124_data *data)
+{
+ int ret, regval, i;
+
+ ret = bd79124_mux_init(data);
+ if (ret)
+ return ret;
+
+ 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;
+ }
+ /* Stop auto sequencer */
+ ret = regmap_clear_bits(data->map, BD79124_REG_SEQUENCE_CFG,
+ BD79124_MASK_SEQ_START);
+ if (ret)
+ return ret;
+
+ /* Enable writing the measured values to the regsters */
+ ret = regmap_set_bits(data->map, BD79124_REG_GEN_CFG,
+ BD79124_MASK_STATS_EN);
+ if (ret)
+ return ret;
+
+ /* Set no channels to be auto-measured */
+ ret = regmap_write(data->map, BD79124_REG_AUTO_CHANNELS, 0x0);
+ if (ret)
+ return ret;
+
+ /* Set no channels to be manually measured */
+ ret = regmap_write(data->map, BD79124_REG_MANUAL_CHANNELS, 0x0);
+ if (ret)
+ return ret;
+
+ /* Set the measurement interval to 0.75 mS */
+ 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);
Where it doesn't make any other difference, align after (
If you are going shorter, single tab only.
Single tab only? You mean like:
ret = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
BD79124_MASK_AUTO_INTERVAL, regval);
Do you prefer that even if the variable holding the return value was
longer than 8 chars? To me it looks odd if arguments on the next line
begin earlier than the function on previous line:
longvariable = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
BD79124_MASK_AUTO_INTERVAL, regval);
(Just ensuring I understood your preference).
+ if (ret)
+ return ret;
+
+ /* Sequencer mode to auto */
+ ret = regmap_set_bits(data->map, BD79124_REG_SEQUENCE_CFG,
+ BD79124_MASK_SEQ_SEQ);
+ if (ret)
+ return ret;
+
+ /* Don't start the measurement */
+ regval = FIELD_PREP(BD79124_MASK_CONV_MODE, BD79124_CONV_MODE_MANSEQ);
What is this for?
Thank's for pointing it out! It is supposed to be used in call below.
The code works as it is just because the BD79124_CONV_MODE_MANSEQ
happens to be '0', but it's still better to use FIELD_PREP() for the
consistency. Below should change from:
+ return regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
+ BD79124_MASK_CONV_MODE, BD79124_CONV_MODE_MANSEQ);
+
to:
return regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
BD79124_MASK_CONV_MODE, regval);
Good catch, thanks! :)
+}
+
+static int bd79124_probe(struct i2c_client *i2c)
+{
+ struct bd79124_data *data;
+ struct iio_dev *iio_dev;
+ const struct iio_chan_spec *template;
+ struct iio_chan_spec *cs;
+ struct device *dev = &i2c->dev;
+ int ret;
+
+ iio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+ if (!iio_dev)
+ return -ENOMEM;
+
+ data = iio_priv(iio_dev);
+ data->dev = dev;
+ data->map = devm_regmap_init_i2c(i2c, &bd79124_regmap);
+ if (IS_ERR(data->map))
+ return dev_err_probe(dev, PTR_ERR(data->map),
+ "Failed to initialize Regmap\n");
+
+ ret = devm_regulator_get_enable_read_voltage(dev, "vdd");
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "Failed to get the Vdd\n");
+
+ data->vmax = ret;
+
+ ret = devm_regulator_get_enable(dev, "iovdd");
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "Failed to enable I/O voltage\n");
+
+ ret = devm_delayed_work_autocancel(dev, &data->alm_enable_work,
+ bd79124_alm_enable_worker);
+ if (ret)
+ return ret;
+
+ if (i2c->irq) {
+ template = &bd79124_chan_template;
+ } else {
+ template = &bd79124_chan_template_noirq;
+ dev_dbg(dev, "No IRQ found, events disabled\n");
+ }
+ ret = devm_iio_adc_device_alloc_chaninfo(dev, template, &cs,
+ &expected_props);
+ if (ret < 0)
+ return ret;
+
+ iio_dev->channels = cs;
+ iio_dev->num_channels = ret;
+ iio_dev->info = &bd79124_info;
+ iio_dev->name = "bd79124";
+ iio_dev->modes = INDIO_DIRECT_MODE;
+
+ data->gc = bd79124gpo_chip;
+ data->gc.parent = dev;
+
+ mutex_init(&data->mutex);
Whilst it doesn't bring huge advantage, now we have devm_mutex_init()
it seems reasonable to use it and maybe catch a use after free for the lock.
Ah, indeed. It's a good to learn to 'habitually' use devm_mutex_init().
Yours,
-- Matti
