linux-kernel - Re: [PATCH v5 4/6] iio: adc: add ade9000 support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aKxWxfbUNMFbZXvN@debian-BULLSEYE-live-builder-AMD64>
Date: Mon, 25 Aug 2025 09:27:49 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: Antoniu Miclaus <antoniu.miclaus@...log.com>
Cc: jic23@...nel.org, robh@...nel.org, conor+dt@...nel.org,
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org
Subject: Re: [PATCH v5 4/6] iio: adc: add ade9000 support

Hi Antoniu,

This is still not a complete review, though, if going to re-spin, you may
consider changing a few minor things.

On 08/22, Antoniu Miclaus wrote:
> Add driver support for the ade9000. highly accurate,
> fully integrated, multiphase energy and power quality
> monitoring device.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
> ---
...
> +#define ADE9000_PHASE_B_POS_BIT		BIT(5)
> +#define ADE9000_PHASE_C_POS_BIT		BIT(6)
> +
> +#define ADE9000_MAX_PHASE_NR		3
> +#define AD9000_CHANNELS_PER_PHASE	10
> +
> +#define ADE9000_ADDR_ADJUST(addr, chan)					\
> +	(((chan) == 0 ? 0 : (chan) == 1 ? 2 : 4) << 4 | (addr))
Found it a bit hard to understand the reason why this macro is like that.
I wonder if a comment to help understand it but not sure. Also, couldn't
come up with any suggestion for an alternative. Guess this device's register
layout is just a bit unusual.

> +
...
> +static int ade9000_filter_type_get(struct iio_dev *indio_dev,
> +				   const struct iio_chan_spec *chan)
> +{
> +	struct ade9000_state *st = iio_priv(indio_dev);
> +	u32 val;
> +	int ret, i;
nitpicking:
unsigned int i;
?

> +
> +	ret = regmap_read(st->regmap, ADE9000_REG_WFB_CFG, &val);
> +	if (ret)
> +		return ret;
> +
> +	val = FIELD_GET(ADE9000_WF_SRC_MASK, val);
> +
> +	for (i = 0; i < ARRAY_SIZE(ade9000_filter_type_values); i++) {
> +		if (ade9000_filter_type_values[i] == val)
> +			return i;
> +	}
> +
> +	return -EINVAL;
> +}
> +
...
> +static const struct iio_enum ade9000_filter_type_enum = {
> +	.items = ade9000_filter_type_items,
> +	.num_items = ARRAY_SIZE(ade9000_filter_type_items),
> +	.get = ade9000_filter_type_get,
> +	.set = ade9000_filter_type_set,
> +};
> +
> +static const struct iio_chan_spec_ext_info ade9000_ext_info[] = {
> +	IIO_ENUM("filter_type", IIO_SHARED_BY_ALL, &ade9000_filter_type_enum),
> +	IIO_ENUM_AVAILABLE("filter_type", IIO_SHARED_BY_ALL, &ade9000_filter_type_enum),
> +	{}
nitpicking: these sentinels have been standardized to use a space between the brackets
	{ }
https://lore.kernel.org/linux-iio/20250411-iio-sentinel-normalization-v1-1-d293de3e3d93@baylibre.com/

> +};
> +
...
> +
> +#define ADE9000_VOLTAGE_CHANNEL(num) {					\
> +	.type = IIO_VOLTAGE,						\
> +	.channel = num,							\
> +	.address = ADE9000_ADDR_ADJUST(ADE9000_REG_AV_PCF, num),	\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
> +			      BIT(IIO_CHAN_INFO_SCALE) |		\
> +			      BIT(IIO_CHAN_INFO_CALIBSCALE) |		\
> +			      BIT(IIO_CHAN_INFO_CALIBSCALE) |		\
duplicated IIO_CHAN_INFO_CALIBSCALE ?

> +			      BIT(IIO_CHAN_INFO_FREQUENCY),		\
> +	.event_spec = ade9000_events,					\
> +	.num_event_specs = ARRAY_SIZE(ade9000_events),			\
> +	.scan_index = num + 1,	/* interleave with current channels */	\
> +	.indexed = 1,							\
> +	.scan_type = {							\
> +		.sign = 's',						\
> +		.realbits = 32,						\
> +		.storagebits = 32,					\
> +		.endianness = IIO_BE,					\
> +	},								\
> +	.ext_info = ade9000_ext_info,					\
> +}
> +
...
> +		case ADE9000_ST1_SEQERR_BIT:
> +			iio_push_event(indio_dev,
> +				       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
> +							    ADE9000_ST1_SEQERR_BIT >> 12,
> +							    IIO_EV_TYPE_CHANGE,
> +							    IIO_EV_DIR_NONE),
> +				       timestamp);
> +			handled_irq |= ADE9000_ST1_SEQERR_BIT;
> +			break;
> +		default:
> +			return IRQ_HANDLED;
> +		}
> +	}
> +
> +	ret = regmap_write(st->regmap, ADE9000_REG_STATUS1, handled_irq);
> +	if (ret)
> +		return ret;
maybe
	if (ret)
		dev_err(&st->spi->dev, "IRQ1 write status1 fail\n");

	return IRQ_HANDLED;
?
So the IRQ always gets handled.

> +
> +	return IRQ_HANDLED;
> +}
> +

Best regards,
Marcelo