| 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
| ||
|
Message-ID: <aJi-YCsgepj0GLip@debian-BULLSEYE-live-builder-AMD64>
Date: Sun, 10 Aug 2025 12:44:32 -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 v3 3/4] iio: adc: add ade9000 support
Hi Antoniu,
Similarly to Nuno, I'm also getting into review only in v3 and may be missing
remarks made in previous iterations. Also, this driver is extensive and for now,
I only reviewed a few things (mostly register definitions and clock setup).
I agree with somebody's suggestion I've seen about splitting the code into more
patches according to the features that are going to be supported.
More comments inline.
On 08/08, 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>
> ---
...
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 1c6ca5fd4b6d..98876d1ea8bf 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_AD7091R5) += ad7091r5.o
> obj-$(CONFIG_AD7091R8) += ad7091r8.o
> obj-$(CONFIG_AD7124) += ad7124.o
> obj-$(CONFIG_AD7173) += ad7173.o
> +obj-$(CONFIG_ADE9000) += ade9000.o
> obj-$(CONFIG_AD7191) += ad7191.o
> obj-$(CONFIG_AD7192) += ad7192.o
> obj-$(CONFIG_AD7266) += ad7266.o
> @@ -46,6 +47,7 @@ obj-$(CONFIG_AD7944) += ad7944.o
> obj-$(CONFIG_AD7949) += ad7949.o
> obj-$(CONFIG_AD799X) += ad799x.o
> obj-$(CONFIG_AD9467) += ad9467.o
> +obj-$(CONFIG_ADE9000) += ade9000.o
Duplicated addition to into Makefile?
> obj-$(CONFIG_ADI_AXI_ADC) += adi-axi-adc.o
> obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o
> obj-$(CONFIG_AT91_ADC) += at91_adc.o
> diff --git a/drivers/iio/adc/ade9000.c b/drivers/iio/adc/ade9000.c
> new file mode 100644
> index 000000000000..a05327119128
> --- /dev/null
> +++ b/drivers/iio/adc/ade9000.c
> @@ -0,0 +1,2033 @@
...
> +
> +/* Address of ADE9000 registers */
> +#define ADE9000_REG_AIGAIN 0x000
> +#define ADE9000_REG_AVGAIN 0x00B
> +#define ADE9000_REG_AIRMSOS 0x00C
> +#define ADE9000_REG_AVRMSOS 0x00D
> +#define ADE9000_REG_APGAIN 0x00E
> +#define ADE9000_REG_AWATTOS 0x00F
> +#define ADE9000_REG_AVAROS 0x010
> +#define ADE9000_REG_AFVAROS 0x012
> +#define ADE9000_REG_CONFIG0 0x060
> +#define ADE9000_REG_DICOEFF 0x072
> +#define ADE9000_REG_AI_PCF 0x20A
> +#define ADE9000_REG_AV_PCF 0x20B
> +#define ADE9000_REG_AIRMS 0x20C
> +#define ADE9000_REG_AVRMS 0x20D
> +#define ADE9000_REG_AWATT 0x210
> +#define ADE9000_REG_AVAR 0x211
> +#define ADE9000_REG_AVA 0x212
> +#define ADE9000_REG_AFVAR 0x214
> +#define ADE9000_REG_APF 0x216
> +#define ADE9000_REG_BI_PCF 0x22A
> +#define ADE9000_REG_BV_PCF 0x22B
> +#define ADE9000_REG_BIRMS 0x22C
> +#define ADE9000_REG_BVRMS 0x22D
> +#define ADE9000_REG_CI_PCF 0x24A
> +#define ADE9000_REG_CV_PCF 0x24B
> +#define ADE9000_REG_CIRMS 0x24C
> +#define ADE9000_REG_CVRMS 0x24D
> +#define ADE9000_REG_AWATT_ACC 0x2E5
> +#define ADE9000_REG_AWATTHR_LO 0x2E6
Some defines are using tabs before the define name, others are using spaces.
I would standardize to have one space after each '#define' but I guess it's
also okay if you use all tabs.
> +#define ADE9000_REG_AVAHR_LO 0x2FA
> +#define ADE9000_REG_AFVARHR_LO 0x30E
> +#define ADE9000_REG_BWATTHR_LO 0x322
> +#define ADE9000_REG_BVAHR_LO 0x336
> +#define ADE9000_REG_BFVARHR_LO 0x34A
...
> +
> +/* Disable all interrupts except EGYRDY */
> +#define ADE9000_MASK0 0x00000001
Can we have more suggestive names for the masks?
If this disables all interrupts except EGYRDY, would it be reasonable to call it
ADE9000_EGYRDY_INT_MASK or ADE9000_MASK0_EGYRDY_MASK (or something that hints
about that interrupt)?
By the way, to me, this almost looks more like a constant than a mask.
Does it become clearer that these are masks if we define them with BIT macro?
#define ADE9000_MASK0_EGYRDY_MASK BIT(0) ?
> +
> +/* Disable all interrupts */
> +#define ADE9000_MASK1 0x00000000
Same suggestion would apply here although this really looks like a constant.
ADE9000_MASK1_INT_DISABLE maybe?
> +
> +/* Events disabled */
> +#define ADE9000_EVENT_MASK 0x00000000
I would call these predefined values just constants and drop the mask part of
the name.
> +
> +/*
> + * Assuming Vnom=1/2 of full scale.
> + * Refer to Technical reference manual for detailed calculations.
> + */
> +#define ADE9000_VLEVEL 0x0022EA28
> +
...
> +
> +static unsigned long ade9000_clkout_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + /* CLKOUT provides the same frequency as the crystal/external clock */
> + return parent_rate ? parent_rate : 24576000; /* Default 24.576 MHz */
This value is a data sheet constant and is used twice in the driver. Does it
make sense to have a define for it?
#define ADE9000_DEFAULT_CLK_FREQ_HZ 24576000 ?
> +}
> +
> +static const struct clk_ops ade9000_clkout_ops = {
> + .prepare = ade9000_clkout_prepare,
> + .unprepare = ade9000_clkout_unprepare,
> + .recalc_rate = ade9000_clkout_recalc_rate,
> +};
...
> +
> +static int ade9000_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct iio_dev *indio_dev;
> + struct ade9000_state *st;
> + struct regmap *regmap;
> + int irq;
> + int ret;
...
> +
> + /* Optional external clock input */
> + st->clkin = devm_clk_get_optional_enabled(dev, "clkin");
> + if (IS_ERR(st->clkin))
> + return dev_err_probe(dev, PTR_ERR(st->clkin), "Failed to get and enable clkin: %ld", PTR_ERR(st->clkin));
> +
> + /* Register clock output provider */
If I'm correctly reading ADE9000 data sheet, CLKOUT will have one of the
connections to the external crystal when a crystal is used. So, maybe condition
the clock provider feature to the supply of an external CMOS clock (not crystal)?
Also, the description of CLKIN pin gives me the impression that the device
requires an external clock input (either crystal or CMOS clock) so clkin would
not be optional.
> + if (device_property_present(dev, "#clock-cells")) {
> + struct clk_init_data clk_init = {};
> + struct clk *clkout;
> + unsigned long parent_rate = 24576000; /* Default crystal frequency */
> +
> + if (st->clkin)
> + parent_rate = clk_get_rate(st->clkin);
> +
> + clk_init.name = "clkout";
> + clk_init.ops = &ade9000_clkout_ops;
> + clk_init.flags = CLK_GET_RATE_NOCACHE;
> + clk_init.num_parents = 0;
> +
> + st->clkout_hw.init = &clk_init;
> +
> + clkout = devm_clk_register(dev, &st->clkout_hw);
> + if (IS_ERR(clkout))
> + return dev_err_probe(dev, PTR_ERR(clkout), "Failed to register clkout: %ld", PTR_ERR(clkout));
> +
> + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &st->clkout_hw);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to add clock provider: %d", ret);
> + }
Best regards,
Marcelo
Powered by blists - more mailing lists