Message-ID: <125ab96e-1e92-4022-95fe-324cd47ce1d9@gmail.com>
Date: Mon, 24 Feb 2025 08:14:23 +0200
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
 Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>,
 Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
 Chen-Yu Tsai <wens@...e.org>, Jernej Skrabec <jernej.skrabec@...il.com>,
 Samuel Holland <samuel@...lland.org>,
 Hugo Villeneuve <hvilleneuve@...onoff.com>, Nuno Sa <nuno.sa@...log.com>,
 David Lechner <dlechner@...libre.com>,
 Javier Carrasco <javier.carrasco.cruz@...il.com>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
 Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH v3 3/9] iio: adc: Support ROHM BD79124 ADC

On 23/02/2025 18:28, Jonathan Cameron wrote:
> On Wed, 19 Feb 2025 14:30:43 +0200
> Matti Vaittinen <mazziesaccount@...il.com> 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@...il.com>
>>
> 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

Powered by blists - more mailing lists