Re: [PATCH v3 3/9] iio: adc: Support ROHM BD79124 ADC

[Matti Vaittinen]

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