Message-ID: <3e618f7c-891f-42fa-86b7-1383e55c3fb2@gmail.com>
Date: Thu, 20 Mar 2025 15:38:18 +0200
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
 Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>,
 Nuno Sa <nuno.sa@...log.com>, David Lechner <dlechner@...libre.com>,
 Javier Carrasco <javier.carrasco.cruz@...il.com>,
 Olivier Moysan <olivier.moysan@...s.st.com>,
 Guillaume Stols <gstols@...libre.com>,
 Dumitru Ceclan <mitrutzceclan@...il.com>,
 Trevor Gamblin <tgamblin@...libre.com>,
 Matteo Martelli <matteomartelli3@...il.com>,
 Alisa-Dariana Roman <alisadariana@...il.com>,
 João Paulo Gonçalves <joao.goncalves@...adex.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org
Subject: Re: [PATCH v9 6/8] iio: adc: Support ROHM BD79124 ADC

On 20/03/2025 15:16, Andy Shevchenko wrote:
> On Thu, Mar 20, 2025 at 10:22:00AM +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 do not support the BD79124's manual measurement mode but implement
>> the measurements using automatic measurement mode, relying on the
>> BD79124's ability of storing latest measurements into register.
>>
>> Support also 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, prevent the user-space from choking
>> on the events and mask the received event for a fixed duration (1 second)
>> when an event is handled.
>>
>> The ADC input pins can be also configured as general purpose outputs.
>> Make those pins which don't have corresponding ADC channel node in the
>> device-tree controllable as GPO.
> 
> ...
> 
>> +static void bd79124gpo_set(struct gpio_chip *gc, unsigned int offset, int value)
>> +{
>> +	struct bd79124_data *data = gpiochip_get_data(gc);
>> +
>> +	if (value)
>> +		regmap_set_bits(data->map, BD79124_REG_GPO_VAL, BIT(offset));
>> +	else
>> +		regmap_clear_bits(data->map, BD79124_REG_GPO_VAL, BIT(offset));
> 
> Hmm... Is there still any obstacle to use regmap_assign_bits()?

No. I missed this by accident. Thanks for the patience :)

>> +}
> 
> ...
> 
>> +static void bd79124gpo_set_multiple(struct gpio_chip *gc, unsigned long *mask,
>> +				    unsigned long *bits)
>> +{
>> +	unsigned int val;
>> +	int ret;
>> +	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) {
> 
> This is basically an equivalent to
> 
> 	if (val ^ *mask) {

Right.

>> +		dev_dbg(data->dev, "Invalid mux config. Can't set value.\n");
> 
>> +		/* Do not set value for pins configured as ADC inputs */
>> +		*mask &= val;
> 
> But this still puzzles me. If we have a valid_mask set, this should never
> happen. Maybe I missed something, though.

I think I already replied this. Wonder if my message was lost. Well, 
here's a recap:

"It can until this gets merged:
https://lore.kernel.org/all/cd5e067b80e1bb590027bc3bfa817e7f794f21c3.1741180097.git.mazziesaccount@gmail.com/

Furthermore, the cost of the check is fairly low. Knowing that the
drivers tend to get backported (because very few if any device makers
use latest kernels) I will keep it here for couple of cycles."

> On top of that I have just realised that you modifying the input here

Valid point. I didn't think of that.

> which may ne quite surprising (I dunno why we don't have const specifier
> in the callbacks from day 1.
> 
>> +	}
>> +
>> +	regmap_update_bits(data->map, BD79124_REG_GPO_VAL, *mask, *bits);
>> +}
> 
> ...
> 
>> +struct bd79124_raw {
>> +	u8 val_bit0_3; /* Is set in high bits of the byte */
>> +	u8 val_bit4_11;
>> +};
> 
> Again, this is confusing.
> 
> Just put a bit order map in the comment as I suggested previously.
> When I see variable name containing bit range like above I think
> about the same bit order, i.e. with your comment it makes like this
> 
> bit number	7 6 5 4 3 2 1 0
> data bit	0 1 2 3 x x x x
> 
> All the same for the second variable.

I find this very confusing. I wonder if you missed my previous reply? 
Just in case you did:
https://lore.kernel.org/all/dc241efd-fcb1-4d2d-9009-b5e4d9fc9b68@gmail.com/

> 
>> +#define BD79124_RAW_TO_INT(r) ((r.val_bit4_11 << 4) | (r.val_bit0_3 >> 4))
>> +#define BD79124_INT_TO_RAW(val) {					\
>> +	.val_bit4_11 = (val) >> 4,					\
>> +	.val_bit0_3 = (val) << 4,					\
>> +}
>> +
>> +/*
>> + * The high and low limits as well as the recent result values are stored in
>> + * the same way in 2 consequent registers. The first register contains 4 bits
>> + * of the value. These bits are stored in the high bits [7:4] of register, but
>> + * they represent the low bits [3:0] of the value.
>> + * The value bits [11:4] are stored in the next register.
>> + *
>> + * Read data from register and convert to integer.
>> + */
>> +static int bd79124_read_reg_to_int(struct bd79124_data *data, int reg,
>> +				   unsigned int *val)
>> +{
>> +	int ret;
>> +	struct bd79124_raw raw;
>> +
>> +	ret = regmap_bulk_read(data->map, reg, &raw, sizeof(raw));
>> +	if (ret) {
>> +		dev_dbg(data->dev, "bulk_read failed %d\n", ret);
>> +
>> +		return ret;
>> +	}
>> +
>> +	*val = BD79124_RAW_TO_INT(raw);
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * The high and low limits as well as the recent result values are stored in
>> + * the same way in 2 consequent registers. The first register contains 4 bits
>> + * of the value. These bits are stored in the high bits [7:4] of register, but
>> + * they represent the low bits [3:0] of the value.
>> + * The value bits [11:4] are stored in the next register.
> 
> And you probably don't need a long comment copied twice when data type is well
> described.

I did also explain in the previous reply why I think the correct place 
for this doc is the function documentation.

>> + * Convert the integer to register format and write it using rmw cycle.
>> + */
>> +static int bd79124_write_int_to_reg(struct bd79124_data *data, int reg,
>> +				    unsigned int val)
>> +{
>> +	struct bd79124_raw raw = BD79124_INT_TO_RAW(val);
>> +	unsigned int tmp;
>> +	int ret;
>> +
>> +	ret = regmap_read(data->map, reg, &tmp);
>> +	if (ret)
>> +		return ret;
> 
>> +	raw.val_bit0_3 |= (tmp & 0xf);
> 
> Parentheses are not needed. Bit we already discussed that and it's principal
> disagreement between us. :-(
> 
>> +	return regmap_bulk_write(data->map, reg, &raw, sizeof(raw));
>> +}
> 
> ...
> 
>> +static const struct iio_chan_spec bd79124_chan_template = {
>> +	.type = IIO_VOLTAGE,
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>> +	.indexed = 1,
>> +	.event_spec = bd79124_events,
>> +	.num_event_specs = ARRAY_SIZE(bd79124_events),
> 
> + array_size.h which is missing.

Thanks!

>> +};
> 
> ...
> 
>> +	switch (info) {
>> +	case IIO_EV_INFO_VALUE:
>> +		if (dir == IIO_EV_DIR_RISING)
>> +			*val = data->alarm_r_limit[chan->channel];
>> +		else if (dir == IIO_EV_DIR_FALLING)
>> +			*val = data->alarm_f_limit[chan->channel];
>> +		else
>> +			return -EINVAL;
>> +
>> +		return IIO_VAL_INT;
> 
>> +
> 
> Blank line...
> 
>> +	case IIO_EV_INFO_HYSTERESIS:
>> +		reg = BD79124_GET_HYSTERESIS_REG(chan->channel);
>> +		ret = regmap_read(data->map, reg, val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		*val &= BD79124_MSK_HYSTERESIS;
>> +		/*
>> +		 * The data-sheet says the hysteresis register value needs to be
>> +		 * sifted left by 3.
>> +		 */
>> +		*val <<= 3;
>> +
>> +		return IIO_VAL_INT;
> 
> ...no blank line. Can you choose one style and use it everywhere?

Ok.

>> +	default:
>> +		return -EINVAL;
>> +	}
> 
> ...
> 
>> +	case IIO_CHAN_INFO_RAW:
>> +	{
>> +		unsigned int old_chan_cfg, regval;
>> +		int tmp;
> 
> This tmp sounds to me like
> 
> 		int ret2; // or rc, retval, etc.

I prefer tmp as it indicates this is really a value which is used to 
store a temporary data. For me ret, rc and retval sound like something 
that is returned to the caller.

> 
>> +		guard(mutex)(&data->mutex);
>> +
>> +		/*
>> +		 * Start the automatic conversion. This is needed here if no
>> +		 * events have been enabled.
>> +		 */
>> +		regval = FIELD_PREP(BD79124_MSK_CONV_MODE,
>> +				    BD79124_CONV_MODE_AUTO);
>> +		ret = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
>> +					 BD79124_MSK_CONV_MODE, regval);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = bd79124_single_chan_seq(data, chan->channel, &old_chan_cfg);
>> +		if (ret)
>> +			return ret;
>> +
>> +		/* The maximum conversion time is 6 uS. */
>> +		udelay(6);
>> +
>> +		ret = bd79124_read_reg_to_int(data,
>> +			BD79124_GET_RECENT_RES_REG(chan->channel), val);
>> +		/*
>> +		 * Return the old chan config even if data reading failed in
>> +		 * order to re-enable the event monitoring.
>> +		 */
>> +		tmp = bd79124_single_chan_seq_end(data, old_chan_cfg);
>> +		if (tmp)
>> +			dev_err(data->dev,
>> +				"Failed to return config. Alarms may be disabled\n");
>> +
>> +		if (ret)
>> +			return ret;
>> +
>> +		return IIO_VAL_INT;
>> +	}
> 
> ...
> 
>> +	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);
> 
> Sounds like possibility to deduplicate as the above only differs by ecode.

I am unsure what 'deduplicate' means?

> 
>> +			ret = bd79124_event_ratelimit_lo(data, i);
>> +			if (ret)
>> +				return IRQ_NONE;
>> +		}
>> +	}
> 
> ...
> 
>> +	ret = devm_iio_adc_device_alloc_chaninfo_se(dev, template,
>> +		BD79124_MAX_NUM_CHANNELS - 1, &cs);
>> +	if (ret < 0) {
>> +		/* Register all pins as GPOs if there are no ADC channels */
>> +		if (ret == -ENOENT)
>> +			goto register_gpios;
> 
> It's possible to move outside of the above conditional, but I think I got your
> idea, however it is still not a hot path.

As I replied earlier, I liked the comment you suggested, but dislike 
having two checks instead of one in the usual operation. It's not like 
this caused too deep nesting or anything.

>> +		return ret;
>> +	}

Yours,
	-- Matti

Powered by blists - more mailing lists