Re: [PATCH v5 07/10] iio: adc: Support ROHM BD79124 ADC

[Jonathan Cameron]

On Mon, 3 Mar 2025 13:33:39 +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

Just a few really trivial comments.  If all else in the
set was resolved I'd probably have applied with a tweak or two

Thanks,

Jonathan

>  obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
> diff --git a/drivers/iio/adc/rohm-bd79124.c b/drivers/iio/adc/rohm-bd79124.c
> new file mode 100644
> index 000000000000..466c7decf8fc
> --- /dev/null
> +++ b/drivers/iio/adc/rohm-bd79124.c
> @@ -0,0 +1,1108 @@
...

> +
> +/* Read-only regs */

Given naming this is pretty obvious. I would drop the comment
and any others that don't add much meaning.

> +static const struct regmap_range bd79124_ro_ranges[] = {



> +static int bd79124_read_event_value(struct iio_dev *iio_dev,
> +				    const struct iio_chan_spec *chan,
> +				    enum iio_event_type type,
> +				    enum iio_event_direction dir,
> +				    enum iio_event_info info, int *val,
> +				    int *val2)
> +{
> +	struct bd79124_data *data = iio_priv(iio_dev);
> +	int ret, reg;
> +
> +	if (chan->channel >= BD79124_MAX_NUM_CHANNELS)
> +		return -EINVAL;
> +
> +	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;
> +
> +	case IIO_EV_INFO_HYSTERESIS:
> +		reg = BD79124_GET_HYSTERESIS_REG(chan->channel);
> +		ret = regmap_read(data->map, reg, val);
> +		if (ret)
> +			return ret;
> +		/* Mask the non hysteresis bits */
> +		*val &= BD79124_MASK_HYSTERESIS;
> +		/*
> +		 * The data-sheet says the hysteresis register value needs to be
> +		 * sifted left by 3 (or multiplied by 8, depending on the
> +		 * page :] )

We don't really need this comment on the oddity of the way the datasheet
describes things in the final driver as effect is same in either case.
It did make me smile though ;)


> +		 */
> +		*val <<= 3;
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +
> +static int bd79124_disable_event(struct bd79124_data *data,
> +			enum iio_event_direction dir, int channel)
> +{
> +	int dir_bit = BIT(dir), reg;

I'd rather this one was split as I find it hard to read lines that
mix setting of some variables and not others.

> +	unsigned int limit;
> +
>
> +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;

Trivial but might as well but ret and reg on same line.


> +
> +}


>