| 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: <20250825144815.6cfeee2d@jic23-huawei>
Date: Mon, 25 Aug 2025 14:48:15 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Antoniu Miclaus <antoniu.miclaus@...log.com>
Cc: <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
On Fri, 22 Aug 2025 16:01:53 +0000
Antoniu Miclaus <antoniu.miclaus@...log.com> 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>
Various comments inline.
Thanks,
Jonathan
> diff --git a/drivers/iio/adc/ade9000.c b/drivers/iio/adc/ade9000.c
> new file mode 100644
> index 000000000000..41f766e00de0
> --- /dev/null
> +++ b/drivers/iio/adc/ade9000.c
> @@ -0,0 +1,2011 @@
> +/* Peak and overcurrent detection disabled */
> +#define ADE9000_CONFIG3 0x0000
> +
> +/*
> + * 50Hz operation, 3P4W Wye configuration, signed accumulation
No idea what a 3P4W Wye configuration is...
Spell that out.
> + * Clear bit 8 i.e. ACCMODE=0x00xx for 50Hz operation
> + * ACCMODE=0x0x9x for 3Wire delta when phase B is used as reference
> + */
>
> +
> +static const struct iio_event_spec ade9000_events[] = {
> + {
> + /* Energy ready event - datasheet: EGYRDY interrupt */
I'll address these alongside the docs.
> + .type = IIO_EV_TYPE_MAG,
> + .dir = IIO_EV_DIR_NONE, /* Non-directional event */
> + .mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE),
> + },
> + {
> + /* Sequence error event - datasheet: SEQERR interrupt */
> + .type = IIO_EV_TYPE_CHANGE,
> + .dir = IIO_EV_DIR_NONE, /* Non-directional event */
> + .mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE),
> + },
> + {
> + /* Threshold events - datasheet: zero crossing timeout, sag/swell */
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_NONE, /* Timeout events (ZXTOUT register) */
> + .mask_separate = BIT(IIO_EV_INFO_ENABLE), /* Per-channel enable */
> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE), /* Shared threshold value */
> + },
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_EITHER,
> + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> + },
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_RISING, /* for swell */
> + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
> + },
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_FALLING, /* for dip */
> + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
> + },
> +};
> +#define ADE9000_POWER_ACTIVE_CHANNEL(num) { \
> + .type = IIO_POWER, \
> + .channel = num, \
> + .address = ADE9000_ADDR_ADJUST(ADE9000_REG_AWATT, num), \
> + .channel2 = IIO_MOD_ACTIVE, \
> + .modified = 1, \
> + .indexed = 1, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_CALIBBIAS) | \
> + BIT(IIO_CHAN_INFO_CALIBSCALE) | \
> + BIT(IIO_CHAN_INFO_POWERFACTOR), \
You have power factor documented as in_power0_powerfactor which is not what
this will generate. Check the ABI docs vs what the driver generates
closely.
> + .scan_index = -1 \
> +}
> +static int ade9000_spi_write_reg(void *context, unsigned int reg,
> + unsigned int val)
> +{
> + struct ade9000_state *st = context;
> + u16 addr;
> + int ret, len;
> +
> + guard(mutex)(&st->lock);
> +
> + addr = FIELD_PREP(ADE9000_REG_ADDR_MASK, reg);
> + put_unaligned_be16(addr, st->tx);
> +
> + if (reg > ADE9000_REG_RUN && reg < ADE9000_REG_VERSION) {
> + put_unaligned_be16(val, &st->tx[2]);
> + len = 4;
> + } else {
> + put_unaligned_be32(val, &st->tx[2]);
> + len = 6;
> + }
> +
> + ret = spi_write(st->spi, st->tx, len);
See below. I'd make this spi_write_then_read() and use local variables + a read
size of 0.
> + if (ret)
> + dev_err(&st->spi->dev, "problem when writing register 0x%x\n", reg);
> +
> + return ret;
> +}
> +
> +static int ade9000_spi_read_reg(void *context, unsigned int reg,
> + unsigned int *val)
> +{
> + struct ade9000_state *st = context;
> + u16 addr;
> + int ret, rx_len;
> +
> + guard(mutex)(&st->lock);
> +
> + addr = FIELD_PREP(ADE9000_REG_ADDR_MASK, reg) |
> + ADE9000_REG_READ_BIT_MASK;
> +
> + put_unaligned_be16(addr, st->tx);
> +
> + /* Skip CRC bytes - only read actual data */
> + if (reg > ADE9000_REG_RUN && reg < ADE9000_REG_VERSION)
> + rx_len = 2;
> + else
> + rx_len = 4;
> +
> + ret = spi_write_then_read(st->spi, st->tx, 2, st->rx, rx_len);
Using write then read doesn't need to use a DMA safe buffer (it is documented
as always bouncing the data) so you could use variables on the stack to
keep the data local for read_reg.
Could also use write_then_read with a zero size read to allow the same
in *spi_write_reg()
> + if (ret) {
> + dev_err(&st->spi->dev, "error reading register 0x%x\n", reg);
> + return ret;
> + }
> +
> + if (reg > ADE9000_REG_RUN && reg < ADE9000_REG_VERSION)
> + *val = get_unaligned_be16(st->rx);
> + else
> + *val = get_unaligned_be32(st->rx);
> +
> + return 0;
> +}
> +
> +static int ade9000_configure_scan(struct iio_dev *indio_dev, u32 wfb_addr)
> +{
> + struct ade9000_state *st = iio_priv(indio_dev);
> + u16 addr;
> +
> + addr = FIELD_PREP(ADE9000_REG_ADDR_MASK, wfb_addr) |
> + ADE9000_REG_READ_BIT_MASK;
> +
> + put_unaligned_be16(addr, st->tx_buff);
> +
> + st->xfer[0].tx_buf = &st->tx_buff[0];
> + st->xfer[0].len = 2;
> +
> + st->xfer[1].rx_buf = st->rx_buff.byte;
> +
> + /* Always use streaming mode */
> + st->xfer[1].len = (st->wfb_nr_samples / 2) * 4;
> +
> + spi_message_init_with_transfers(&st->spi_msg, st->xfer, ARRAY_SIZE(st->xfer));
> + return 0;
Never fails, return type of void to make that clear.
> +}
> +static irqreturn_t ade9000_irq1_thread(int irq, void *data)
> +{
> + struct iio_dev *indio_dev = data;
> + struct ade9000_state *st = iio_priv(indio_dev);
> + unsigned int bit = ADE9000_ST1_CROSSING_FIRST;
> + s64 timestamp = iio_get_time_ns(indio_dev);
> + u32 handled_irq = 0;
> + u32 interrupts;
u32 interrupts, result, status, tmp;
> + u32 result;
> + u32 status;
> + u32 tmp;
> + unsigned long interrupt_bits;
> + int ret;
> +
> + if (!completion_done(&st->reset_completion)) {
> + ret = regmap_read(st->regmap, ADE9000_REG_STATUS1, &result);
> + if (ret) {
> + dev_err(&st->spi->dev, "IRQ1 read status fail\n");
> + return ret;
Don't return errno in an irq thread handler. They aren't irqreturn_t
> + }
> +
> + if (result & ADE9000_ST1_RSTDONE_BIT)
> + complete(&st->reset_completion);
> + else
> + dev_err(&st->spi->dev, "Error testing reset done\n");
> +
> + return IRQ_HANDLED;
> + }
> +
> + ret = regmap_read(st->regmap, ADE9000_REG_STATUS1, &status);
> + if (ret) {
> + dev_err(&st->spi->dev, "IRQ1 read status fail\n");
> + return IRQ_HANDLED;
> + }
> +
> + ret = regmap_read(st->regmap, ADE9000_REG_MASK1, &interrupts);
> + if (ret) {
> + dev_err(&st->spi->dev, "IRQ1 read status fail\n");
> + return IRQ_HANDLED;
> + }
> +
> + interrupt_bits = interrupts;
> + for_each_set_bit_from(bit, &interrupt_bits,
> + ADE9000_ST1_CROSSING_DEPTH){
> + tmp = status & BIT(bit);
> +
> + switch (tmp) {
> + case ADE9000_ST1_ZXVA_BIT:
There are a lot of these, maybe a lookup table with event code and field
in handled_irq? Use a marker like no event code to indicate the bits
we don't handle.
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
> + ADE9000_ST1_ZXVA_BIT,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_EITHER),
> + timestamp);
> + handled_irq |= ADE9000_ST1_ZXVA_BIT;
These are all the same as handled_irq |= tmp; ?
maybe can take that out of the switch statement?
If you do a lookup table on bit, that will end up even nicer.
> + break;
> + case ADE9000_ST1_ZXTOVA_BIT:
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
> + ADE9000_ST1_ZXTOVA_BIT,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_EITHER),
> + timestamp);
> + handled_irq |= ADE9000_ST1_ZXTOVA_BIT;
> + break;
> + case ADE9000_ST1_ZXIA_BIT:
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_CURRENT,
> + ADE9000_ST1_ZXIA_BIT,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_EITHER),
> + timestamp);
> + handled_irq |= ADE9000_ST1_ZXIA_BIT;
> + break;
> + case ADE9000_ST1_ZXVB_BIT:
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
> + ADE9000_ST1_ZXVB_BIT,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_EITHER),
> + timestamp);
> + handled_irq |= ADE9000_ST1_ZXVB_BIT;
> + break;
> + case ADE9000_ST1_ZXTOVB_BIT:
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
> + ADE9000_ST1_ZXTOVB_BIT,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_EITHER),
> + timestamp);
> + handled_irq |= ADE9000_ST1_ZXTOVB_BIT;
> + break;
> + case ADE9000_ST1_ZXIB_BIT:
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_CURRENT,
> + ADE9000_ST1_ZXIB_BIT,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_EITHER),
> + timestamp);
> + handled_irq |= ADE9000_ST1_ZXIB_BIT;
> + break;
> + case ADE9000_ST1_ZXVC_BIT:
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
> + ADE9000_ST1_ZXVC_BIT,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_EITHER),
> + timestamp);
> + handled_irq |= ADE9000_ST1_ZXVC_BIT;
> + break;
> + case ADE9000_ST1_ZXTOVC_BIT:
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
> + ADE9000_ST1_ZXTOVC_BIT,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_EITHER),
> + timestamp);
> + handled_irq |= ADE9000_ST1_ZXTOVC_BIT;
> + break;
> + case ADE9000_ST1_ZXIC_BIT:
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_CURRENT,
> + ADE9000_ST1_ZXIC_BIT,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_EITHER),
> + timestamp);
> + handled_irq |= ADE9000_ST1_ZXIC_BIT;
> + break;
> + case ADE9000_ST1_SWELLA_BIT:
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
> + ADE9000_ST1_SWELLA_BIT >> 20,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_RISING),
> + timestamp);
> + handled_irq |= ADE9000_ST1_SWELLA_BIT;
> + break;
> + case ADE9000_ST1_SWELLB_BIT:
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
> + ADE9000_ST1_SWELLB_BIT >> 20,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_RISING),
> + timestamp);
> + handled_irq |= ADE9000_ST1_SWELLB_BIT;
> + break;
> + case ADE9000_ST1_SWELLC_BIT:
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
> + ADE9000_ST1_SWELLC_BIT >> 20,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_RISING),
> + timestamp);
> + handled_irq |= ADE9000_ST1_SWELLC_BIT;
> + break;
> + case ADE9000_ST1_DIPA_BIT:
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
> + ADE9000_ST1_DIPA_BIT >> 20,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_FALLING),
> + timestamp);
> + handled_irq |= ADE9000_ST1_DIPA_BIT;
> + break;
> + case ADE9000_ST1_DIPB_BIT:
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
> + ADE9000_ST1_DIPB_BIT >> 20,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_FALLING),
> + timestamp);
> + handled_irq |= ADE9000_ST1_DIPB_BIT;
> + break;
> + case ADE9000_ST1_DIPC_BIT:
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
> + ADE9000_ST1_DIPC_BIT >> 20,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_FALLING),
> + timestamp);
> + handled_irq |= ADE9000_ST1_DIPC_BIT;
> + break;
> + 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;
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int ade9000_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val,
> + int *val2,
> + long mask)
> +{
> + struct ade9000_state *st = iio_priv(indio_dev);
> + unsigned int reg, measured;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_FREQUENCY:
> + if (chan->type == IIO_VOLTAGE) {
> + int period_reg;
> + int period;
> +
> + switch (chan->channel) {
> + case ADE9000_PHASE_A_NR:
> + period_reg = ADE9000_REG_APERIOD;
> + break;
> + case ADE9000_PHASE_B_NR:
> + period_reg = ADE9000_REG_BPERIOD;
> + break;
> + case ADE9000_PHASE_C_NR:
> + period_reg = ADE9000_REG_CPERIOD;
> + break;
> + default:
> + return -EINVAL;
> + }
> + ret = regmap_read(st->regmap, period_reg, &period);
> + if (ret)
> + return ret;
> + *val = 4000 * 65536;
Why this particular multiplier? Maybe a spec reference.
> + *val2 = period + 1;
> + return IIO_VAL_FRACTIONAL;
> + }
> +
> + ret = regmap_read(st->regmap, ADE9000_REG_ACCMODE, ®);
> + if (ret)
> + return ret;
> + *val = (reg & ADE9000_ACCMODE_60HZ) ? 60 : 50;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_RAW:
> + if (chan->type == IIO_ENERGY) {
> + u16 lo_reg = chan->address;
> +
> + ret = regmap_bulk_read(st->regmap, lo_reg,
> + st->bulk_read_buf, 2);
> + if (ret)
> + return ret;
> +
> + *val = st->bulk_read_buf[0]; /* Lower 32 bits */
> + *val2 = st->bulk_read_buf[1]; /* Upper 32 bits */
> + return IIO_VAL_INT_64;
> + }
> +
> + ret = iio_device_claim_direct(indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, chan->address, &measured);
> + iio_device_release_direct(indio_dev);
> + if (ret)
> + return ret;
> +
> + *val = measured;
> +
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_POWERFACTOR:
> + ret = iio_device_claim_direct(indio_dev);
> + if (ret)
> + return ret;
> +
> + /* Map power channel to corresponding power factor register */
> + reg = ADE9000_ADDR_ADJUST(ADE9000_REG_APF, chan->channel);
> + ret = regmap_read(st->regmap, reg, &measured);
> + iio_device_release_direct(indio_dev);
> + if (ret)
> + return ret;
> +
> + *val = measured;
> +
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + if (chan->info_mask_shared_by_all) {
> + /* Shared PGA gain read - only for channel with shared frequency */
> + ret = regmap_read(st->regmap, ADE9000_REG_PGA_GAIN, ®);
> + if (ret)
> + return ret;
> + *val = min(1 << ((reg >> (8 + chan->channel)) & GENMASK(1, 0)), 4);
> + return IIO_VAL_INT;
return, so no need for else to follow.
same for later else ifs
> + } else if (chan->type == IIO_CURRENT || chan->type == IIO_VOLTAGE ||
> + chan->type == IIO_ALTVOLTAGE) {
> + switch (chan->address) {
> + case ADE9000_REG_AI_PCF:
> + case ADE9000_REG_AV_PCF:
> + case ADE9000_REG_BI_PCF:
> + case ADE9000_REG_BV_PCF:
> + case ADE9000_REG_CI_PCF:
> + case ADE9000_REG_CV_PCF:
> + *val = 1;
> + *val2 = ADE9000_PCF_FULL_SCALE_CODES;
> + return IIO_VAL_FRACTIONAL;
> + case ADE9000_REG_AIRMS:
> + case ADE9000_REG_AVRMS:
> + case ADE9000_REG_BIRMS:
> + case ADE9000_REG_BVRMS:
> + case ADE9000_REG_CIRMS:
> + case ADE9000_REG_CVRMS:
> + *val = 1;
> + *val2 = ADE9000_RMS_FULL_SCALE_CODES;
> + return IIO_VAL_FRACTIONAL;
> + default:
> + return -EINVAL;
> + }
> + } else if (chan->type == IIO_POWER) {
> + *val = 1;
> + *val2 = ADE9000_WATT_FULL_SCALE_CODES;
> + return IIO_VAL_FRACTIONAL;
> + } else {
> + return -EINVAL;
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int ade9000_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val,
> + int val2,
> + long mask)
> +{
> + struct ade9000_state *st = iio_priv(indio_dev);
> + u32 addr;
> + u32 tmp;
u32 addr, tmp;
saves us a line for no lost of readability.
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_FREQUENCY:
> +}
> +static int ade9000_read_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir)
> +{
> + struct ade9000_state *st = iio_priv(indio_dev);
> + u32 interrupts0, interrupts1, number;
> + int ret;
> +
> + ret = regmap_read(st->regmap, ADE9000_REG_MASK0, &interrupts0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, ADE9000_REG_MASK1, &interrupts1);
> + if (ret)
> + return ret;
> +
> + if (type == IIO_EV_TYPE_MAG)
> + return interrupts0 & ADE9000_ST0_EGYRDY;
Might as well do that before reading interrupts1. Save a bit of bus traffic
without complicating the code. Even better, only read interrupts0 at all
if type == IIO_EV_TYPE_MAG.
> +
> + if (type == IIO_EV_TYPE_CHANGE)
> + return interrupts1 & ADE9000_ST1_SEQERR_BIT;
> +
> + number = chan->channel;
> +
> + switch (number) {
> + case ADE9000_PHASE_A_NR:
> + if (chan->type == IIO_VOLTAGE) {
> + if (dir == IIO_EV_DIR_EITHER)
> + return interrupts1 & ADE9000_ST1_ZXVA_BIT;
> + if (dir == IIO_EV_DIR_NONE)
> + return interrupts1 & ADE9000_ST1_ZXTOVA_BIT;
> + if (dir == IIO_EV_DIR_RISING)
> + return interrupts1 & ADE9000_ST1_SWELLA_BIT;
> + if (dir == IIO_EV_DIR_FALLING)
> + return interrupts1 & ADE9000_ST1_DIPA_BIT;
> + } else if (chan->type == IIO_CURRENT) {
> + return interrupts1 & ADE9000_ST1_ZXIA_BIT;
> + }
> + break;
> + case ADE9000_PHASE_B_NR:
> + if (chan->type == IIO_VOLTAGE) {
> + if (dir == IIO_EV_DIR_EITHER)
> + return interrupts1 & ADE9000_ST1_ZXVB_BIT;
> + if (dir == IIO_EV_DIR_NONE)
> + return interrupts1 & ADE9000_ST1_ZXTOVB_BIT;
> + if (dir == IIO_EV_DIR_RISING)
> + return interrupts1 & ADE9000_ST1_SWELLB_BIT;
> + if (dir == IIO_EV_DIR_FALLING)
> + return interrupts1 & ADE9000_ST1_DIPB_BIT;
> + } else if (chan->type == IIO_CURRENT) {
> + return interrupts1 & ADE9000_ST1_ZXIB_BIT;
> + }
> + break;
> + case ADE9000_PHASE_C_NR:
> + if (chan->type == IIO_VOLTAGE) {
> + if (dir == IIO_EV_DIR_EITHER)
> + return interrupts1 & ADE9000_ST1_ZXVC_BIT;
> + if (dir == IIO_EV_DIR_NONE)
> + return interrupts1 & ADE9000_ST1_ZXTOVC_BIT;
> + if (dir == IIO_EV_DIR_RISING)
> + return interrupts1 & ADE9000_ST1_SWELLC_BIT;
> + if (dir == IIO_EV_DIR_FALLING)
> + return interrupts1 & ADE9000_ST1_DIPC_BIT;
> + } else if (chan->type == IIO_CURRENT) {
> + return interrupts1 & ADE9000_ST1_ZXIC_BIT;
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int ade9000_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + bool state)
> +{
> + struct ade9000_state *st = iio_priv(indio_dev);
> + u32 interrupts, tmp;
> + int ret;
> +
> + /* Clear all pending events in STATUS1 register (write 1 to clear) */
> + ret = regmap_write(st->regmap, ADE9000_REG_STATUS1, GENMASK(31, 0));
> + if (ret)
> + return ret;
> +
> + if (type == IIO_EV_TYPE_MAG) {
> + ret = regmap_update_bits(st->regmap, ADE9000_REG_STATUS0,
> + ADE9000_ST0_EGYRDY, ADE9000_ST0_EGYRDY);
regmap_set_bits() exists for this case.
> + if (ret)
> + return ret;
> + return regmap_update_bits(st->regmap, ADE9000_REG_MASK0,
> + ADE9000_ST0_EGYRDY,
MASK and field don't seem to match even wrt to which register they are in.
Having fixed that regmap_assign_bits() probably useufl here.
> + state ? ADE9000_ST1_SEQERR_BIT : 0);
> + }
> +
> + if (type == IIO_EV_TYPE_CHANGE)
> + return regmap_update_bits(st->regmap, ADE9000_REG_MASK1,
> + ADE9000_ST1_SEQERR_BIT,
> + state ? ADE9000_ST1_SEQERR_BIT : 0);
return regmap_assign_bits(st->regmap, ADE9000_REG_MASK1,
ADE9000_ST1_SEQERR_BIT, state);
is a little simpler and avoids chance of register / field mismatch like above.
> +
> + if (dir == IIO_EV_DIR_EITHER) {
> + static const struct {
> + u32 irq;
> + u32 wfb_trg;
> + } trig_arr[6] = {
> + {
> + .irq = ADE9000_ST1_ZXVA_BIT,
> + .wfb_trg = ADE9000_WFB_TRG_ZXVA_BIT
> + }, {
> + .irq = ADE9000_ST1_ZXIA_BIT,
> + .wfb_trg = ADE9000_WFB_TRG_ZXIA_BIT
> + }, {
> + .irq = ADE9000_ST1_ZXVB_BIT,
> + .wfb_trg = ADE9000_WFB_TRG_ZXVB_BIT
> + }, {
> + .irq = ADE9000_ST1_ZXIB_BIT,
> + .wfb_trg = ADE9000_WFB_TRG_ZXIB_BIT
> + }, {
> + .irq = ADE9000_ST1_ZXVC_BIT,
> + .wfb_trg = ADE9000_WFB_TRG_ZXVC_BIT
> + }, {
> + .irq = ADE9000_ST1_ZXIC_BIT,
> + .wfb_trg = ADE9000_WFB_TRG_ZXIC_BIT
> + },
> + };
> + if (state) {
> + interrupts |= trig_arr[chan->channel * 2 + chan->type].irq;
> + st->wfb_trg |= trig_arr[chan->channel * 2 + chan->type].wfb_trg;
> + } else {
> + interrupts &= ~trig_arr[chan->channel * 2 + chan->type].irq;
> + st->wfb_trg &= ~trig_arr[chan->channel * 2 + chan->type].wfb_trg;
> + }
> + }
> +
> + if (dir == IIO_EV_DIR_NONE) {
> + switch (chan->channel) {
> + case ADE9000_PHASE_A_NR:
> + tmp |= ADE9000_ST1_ZXTOVA_BIT;
> + break;
> + case ADE9000_PHASE_B_NR:
> + tmp |= ADE9000_ST1_ZXTOVB_BIT;
> + break;
> + case ADE9000_PHASE_C_NR:
> + tmp |= ADE9000_ST1_ZXTOVC_BIT;
> + break;
> + default:
> + break;
> + }
> +
> + if (state)
> + interrupts |= tmp;
> + else
> + interrupts &= ~tmp;
> + } else if (dir == IIO_EV_DIR_RISING) {
> + switch (chan->channel) {
> + case ADE9000_PHASE_A_NR:
> + tmp |= ADE9000_ST1_SWELLA_BIT;
> + break;
> + case ADE9000_PHASE_B_NR:
> + tmp |= ADE9000_ST1_SWELLB_BIT;
> + break;
> + case ADE9000_PHASE_C_NR:
> + tmp |= ADE9000_ST1_SWELLC_BIT;
> + break;
> + default:
> + break;
> + }
> +
> + if (state) {
> + interrupts |= tmp;
> + st->wfb_trg |= ADE9000_WFB_TRG_SWELL_BIT;
> + } else {
> + interrupts &= ~tmp;
> + st->wfb_trg &= ~ADE9000_WFB_TRG_SWELL_BIT;
> + }
> + } else if (dir == IIO_EV_DIR_FALLING) {
> + switch (chan->channel) {
> + case ADE9000_PHASE_A_NR:
> + tmp |= ADE9000_ST1_DIPA_BIT;
> + break;
> + case ADE9000_PHASE_B_NR:
> + tmp |= ADE9000_ST1_DIPB_BIT;
> + break;
> + case ADE9000_PHASE_C_NR:
> + tmp |= ADE9000_ST1_DIPC_BIT;
> + break;
> + default:
> + break;
> + }
> +
> + if (state) {
> + interrupts |= tmp;
> + st->wfb_trg |= ADE9000_WFB_TRG_DIP_BIT;
> + } else {
> + interrupts &= ~tmp;
> + st->wfb_trg &= ~ADE9000_WFB_TRG_DIP_BIT;
> + }
> + }
> +
> + return regmap_update_bits(st->regmap, ADE9000_REG_MASK1, interrupts,
> + interrupts);
return regmap_set_bits()
check the rest of the driver for places where these clear/set/assign
regmap functions can simplifiy things.
> +}
> +static int ade9000_reset(struct ade9000_state *st)
> +{
> + struct device *dev = &st->spi->dev;
> + struct gpio_desc *gpio_reset;
> + int ret;
> +
> + gpio_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(gpio_reset))
> + return PTR_ERR(gpio_reset);
> +
> + if (gpio_reset) {
> + fsleep(10);
> + gpiod_set_value_cansleep(gpio_reset, 0);
> + fsleep(50000);
> + } else {
> + ret = regmap_update_bits(st->regmap, ADE9000_REG_CONFIG1,
> + ADE9000_SWRST_BIT, ADE9000_SWRST_BIT);
> + if (ret)
> + return ret;
> + fsleep(90);
> + return 0;
This code structure is more complex than it needs to be. There is no sharing
of the code that follows between the if / else so either drag that
into the if (gpio_reset) branch and return early or flip the logic and
do the if (!gpio_reset) first allowing the fsleep(10) gpio_set_value_cansleep()
etc to be indented less. That is
if (gpio_reset) {
fsleep(10);
gpiod_set_value_cansleep(gpio_reset, 0);
fsleep(50000);
if (!wait_for_completion_timeout(&st->reset_completion,
msecs_to_jiffies(1000))) {
dev_err(dev, "Reset timeout after 1s\n");
return -ETIMEDOUT;
}
return 0;
}
ret = regmap_update_bits(st->regmap, ADE9000_REG_CONFIG1,
ADE9000_SWRST_BIT, ADE9000_SWRST_BIT);
if (ret)
return ret;
fsleep(90);
return 0;
Or the other way around.
> + }
> +
> + if (!wait_for_completion_timeout(&st->reset_completion,
> + msecs_to_jiffies(1000))) {
> + dev_err(dev, "Reset timeout after 1s\n");
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
> +}
Powered by blists - more mailing lists