Message-ID: <BYAPR03MB4647634E63550B530FD063DB97E69@BYAPR03MB4647.namprd03.prod.outlook.com>
Date:   Thu, 7 Apr 2022 12:56:47 +0000
From:   "Hegbeli, Ciprian" <Ciprian.Hegbeli@...log.com>
To:     Jonathan Cameron <Jonathan.Cameron@...wei.com>
CC:     "jic23@...nel.org" <jic23@...nel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 3/3] iio: meter: add ADE9078 driver

> Hi Ciprian
> 
> I took a very quick end of day look so probably missed some stuff I'll
> pick up on in v2.  Biggest issue is new ABI without docs. Various
> other comments inline. Plus as already observed, please use full name
> in from and SoB as they form part of the Developer Certificate of Origin.
> 
> Thanks,
> 
> Jonathan

Hi Jonathan,

Thank you for the review. I wanted to answer the regmap related
questions and maybe ask for some advice regarding them. There
are some details in the messages below; the short versions is that
it's doable but I will have to give up on some of the regmap 
functionality and mask some of the basic regmap functions in driver 
specific functions. These changes might make the code less
readable.  

> > +static const struct reg_sequence ade9078_reg_sequence[] = {
> > +	{ ADE9078_REG_PGA_GAIN, ADE9078_PGA_GAIN },
> > +	{ ADE9078_REG_CONFIG0, ADE9078_CONFIG0 },
> > +	{ ADE9078_REG_CONFIG1, ADE9078_CONFIG1 },
> > +	{ ADE9078_REG_CONFIG2, ADE9078_CONFIG2 },
> > +	{ ADE9078_REG_CONFIG3, ADE9078_CONFIG3 },
> > +	{ ADE9078_REG_ACCMODE, ADE9078_ACCMODE },
> > +	{ ADE9078_REG_ZX_LP_SEL, ADE9078_ZX_LP_SEL },
> > +	{ ADE9078_REG_MASK0, ADE9078_MASK0 },
> > +	{ ADE9078_REG_MASK1, ADE9078_MASK1 },
> > +	{ ADE9078_REG_EVENT_MASK, ADE9078_EVENT_MASK },
> > +	{ ADE9078_REG_WFB_CFG, ADE9078_WFB_CFG },
> > +	{ ADE9078_REG_VLEVEL, ADE9078_VLEVEL },
> > +	{ ADE9078_REG_DICOEFF, ADE9078_DICOEFF },
> > +	{ ADE9078_REG_EGY_TIME, ADE9078_EGY_TIME },
> > +	{ ADE9078_REG_EP_CFG, ADE9078_EP_CFG },
> > +	{ ADE9078_REG_RUN, ADE9078_RUN_ON }
> > +};

CONFIG0, MASK0, MASK1, EVENT_MASK, VLEVEL, DICOEFF are 
32bit registers. 

> > +/*
> 
> As mentioned below, I don't immediately understand why this can't
> be done with appropriate standard regmap.  Perhaps you could give
> more details of what is missing.  I'd like to see that added to
> regmap if possible.
> 

I started implementing the driver with a standard SPI and using 
bulk read/write for the 32 bit registers. This however renders some 
of the standard features of the regmap unusable (as far as I know).
For example I can't use regmap_update_bits for 32 bit registers,
which happens a few times, which I have marked in the code below.
I could implement a substitute function in my driver for this, but that
would complicate the code (in my opinion).   

> > + * ade9078_spi_write_reg() - ade9078 write register over SPI
> > + * the data format for communicating with the ade9078 over SPI
> > + * is very specific and can access both 32bit and 16bit registers
> > + * @context:	void pointer to the SPI device
> > + * @reg:	address of the of desired register
> > + * @val:	value to be written to the ade9078
> > + */
> > +static int ade9078_spi_write_reg(void *context,
> > +				 unsigned int reg,
> > +				 unsigned int val)
> > +{
> > +	struct device *dev = context;
> > +	struct spi_device *spi = to_spi_device(dev);
> > +	struct ade9078_state *st = spi_get_drvdata(spi);
> 
> > +
> > +	u16 addr;
> > +	int ret = 0;
> > +	struct spi_transfer xfer[] = {
> > +		{
> > +			.tx_buf = st->tx,
> > +		},
> > +	};
> > +
> > +	addr = FIELD_PREP(ADE9078_REG_ADDR_MASK, reg);
> > +
> > +	put_unaligned_be16(addr, st->tx);
> > +	put_unaligned_be32(val, &st->tx[2]);
> > +
> > +	if (reg > ADE9078_REG_RUN && reg < ADE9078_REG_VERSION) {
> > +		put_unaligned_be16(val, &st->tx[2]);
> > +		xfer[0].len = 4;
> > +	} else {
> > +		xfer[0].len = 6;
> > +	}
> > +
> > +	ret = spi_sync_transfer(st->spi, xfer, ARRAY_SIZE(xfer));
> > +	if (ret) {
> > +		dev_err(&st->spi->dev, "problem when writing register
> 0x%x",
> > +			reg);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +/*
> > + * ade9078_spi_write_reg() - ade9078 read register over SPI
> > + * the data format for communicating with the ade9078 over SPI
> > + * is very specific and can access both 32bit and 16bit registers
> > + * @context:	void pointer to the SPI device
> > + * @reg:	address of the of desired register
> > + * @val:	value to be read to the ade9078
> > + */
> > +static int ade9078_spi_read_reg(void *context,
> > +				unsigned int reg,
> > +				unsigned int *val)
> > +{
> > +	struct device *dev = context;
> > +	struct spi_device *spi = to_spi_device(dev);
> > +	struct ade9078_state *st = spi_get_drvdata(spi);
> > +
> > +	u16 addr;
> > +	int ret = 0;
> > +	struct spi_transfer xfer[] = {
> > +		{
> > +			.tx_buf = st->tx,
> > +			.len = 2,
> > +		},
> > +		{
> > +			.rx_buf = st->rx,
> > +		},
> > +	};
> > +
> > +	addr = FIELD_PREP(ADE9078_REG_ADDR_MASK, reg) |
> > +	       ADE9078_REG_READ_BIT_MASK;
> > +
> > +	put_unaligned_be16(addr, st->tx);
> > +
> > +	if (reg > ADE9078_REG_RUN && reg < ADE9078_REG_VERSION)
> > +		xfer[1].len = 4;
> > +	else
> > +		xfer[1].len = 6;
> 
> This doesn't look like a fixed length register which is expected
> for regmap...  Also that len should just be the rx register which
> you treat below as 16 bit and 32 bit (so 2 and 4, not 4 and 6).
> 
> Can the larger registers be treated as bulk reads of pairs of smaller ones?
> 

It could, but I would still need an if or a type of "decoder" to determine
if it is a 16 bit or 32 bit register, which means I still need a form of 
ade9078_read_reg which selects between a regmap_read function
or regmap_bulk_read. This goes for write as well. 

> > +
> > +	ret = spi_sync_transfer(st->spi, xfer, ARRAY_SIZE(xfer));
> > +	if (ret) {
> > +		dev_err(&st->spi->dev, "problem when reading register
> 0x%x",
> > +			reg);
> > +		goto err_ret;
> > +	}
> > +
> > +	//registers which are 16 bits
> > +	if (reg > 0x480 && reg < 0x4FE)
> > +		*val = get_unaligned_be16(st->rx);
> > +	else
> > +		*val = get_unaligned_be32(st->rx);
> > +
> > +err_ret:
> > +	return ret;
> > +}
> > +
> > +/*
> > + * ade9078_is_volatile_reg() - list of ade9078 registers which should use
> > + * caching
> > + * @dev:	device data
> > + * @reg:	address of the of desired register
> > + */
> > +static bool ade9078_is_volatile_reg(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case ADE9078_REG_STATUS0:
> > +	case ADE9078_REG_STATUS1:
> > +	case ADE9078_REG_MASK0:
> > +	case ADE9078_REG_MASK1:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +

I left this part in for context.

> > +/*
> > + * ade9078_write_event_config() - IIO event configure to enable zero-
> crossing
> 
> All these function description comments should be valid kernel-doc.
> 
> > + * and zero-crossing timeout on voltage and current for each phases.
> These
> > + * events will also influence the trigger conditions for the buffer capture.
> > + */
> > +static int ade9078_write_event_config(struct iio_dev *indio_dev,
> > +				      const struct iio_chan_spec *chan,
> > +				      enum iio_event_type type,
> > +				      enum iio_event_direction dir,
> > +				      int state)
> > +{
> > +	struct ade9078_state *st = iio_priv(indio_dev);
> > +	u32 interrupts;
> > +	u32 number;
> > +	int ret;
> > +
> > +	number = chan->channel;
> > +
> > +	switch (number) {
> > +	case ADE9078_PHASE_A_NR:
> 
> I would use a lookup on the phase into an array of structures.
> The structure would then have fields for which bit to set etc
> for a voltage channel and for a current channel.
> 
> That way this all becomes one bit of code and some const data
> rather that 3 sets of near identical code. If you can make
> this sort of thing data rather than code that is almost always
> the best choice.
> 
> > +		if (chan->type == IIO_VOLTAGE) {
> > +			if (state) {
> > +				interrupts |= ADE9078_ST1_ZXVA_BIT;
> > +				interrupts |= ADE9078_ST1_ZXTOVA_BIT;
> > +				st->wfb_trg |=
> ADE9078_WFB_TRG_ZXVA_BIT;
> > +			} else {
> > +				interrupts &= ~ADE9078_ST1_ZXVA_BIT;
> > +				interrupts &= ~ADE9078_ST1_ZXTOVA_BIT;
> > +				st->wfb_trg &=
> ~ADE9078_WFB_TRG_ZXVA_BIT;
> > +			}
> > +		} else if (chan->type == IIO_CURRENT) {
> > +			if (state) {
> > +				interrupts |= ADE9078_ST1_ZXIA_BIT;
> > +				st->wfb_trg |=
> ADE9078_WFB_TRG_ZXIA_BIT;
> > +			} else {
> > +				interrupts &= ~ADE9078_ST1_ZXIA_BIT;
> > +				st->wfb_trg &=
> ~ADE9078_WFB_TRG_ZXIA_BIT;
> > +			}
> > +		}
> > +		break;
> > +	case ADE9078_PHASE_B_NR:
> > +		if (chan->type == IIO_VOLTAGE) {
> > +			if (state) {
> > +				interrupts |= ADE9078_ST1_ZXVB_BIT;
> > +				interrupts |= ADE9078_ST1_ZXTOVB_BIT;
> > +				st->wfb_trg |=
> ADE9078_WFB_TRG_ZXVB_BIT;
> > +			} else {
> > +				interrupts &= ~ADE9078_ST1_ZXVB_BIT;
> > +				interrupts &= ~ADE9078_ST1_ZXTOVB_BIT;
> > +				st->wfb_trg &=
> ~ADE9078_WFB_TRG_ZXVB_BIT;
> > +			}
> > +		} else if (chan->type == IIO_CURRENT) {
> > +			if (state) {
> > +				interrupts |= ADE9078_ST1_ZXIB_BIT;
> > +				st->wfb_trg |=
> ADE9078_WFB_TRG_ZXIB_BIT;
> > +			} else {
> > +				interrupts &= ~ADE9078_ST1_ZXIB_BIT;
> > +				st->wfb_trg &=
> ~ADE9078_WFB_TRG_ZXIB_BIT;
> > +			}
> > +		}
> > +		break;
> > +	case ADE9078_PHASE_C_NR:
> > +		if (chan->type == IIO_VOLTAGE) {
> > +			if (state) {
> > +				interrupts |= ADE9078_ST1_ZXVC_BIT;
> > +				interrupts |= ADE9078_ST1_ZXTOVC_BIT;
> > +				st->wfb_trg |=
> ADE9078_WFB_TRG_ZXVC_BIT;
> > +			} else {
> > +				interrupts &= ~ADE9078_ST1_ZXVC_BIT;
> > +				interrupts &= ~ADE9078_ST1_ZXTOVC_BIT;
> > +				st->wfb_trg &=
> ~ADE9078_WFB_TRG_ZXVC_BIT;
> > +			}
> > +		} else if (chan->type == IIO_CURRENT) {
> > +			if (state) {
> > +				interrupts |= ADE9078_ST1_ZXIC_BIT;
> > +				st->wfb_trg |=
> ADE9078_WFB_TRG_ZXIC_BIT;
> > +			} else {
> > +				interrupts &= ~ADE9078_ST1_ZXIC_BIT;
> > +				st->wfb_trg &=
> ~ADE9078_WFB_TRG_ZXIC_BIT;
> > +			}
> > +		}
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = regmap_write(st->regmap, ADE9078_REG_STATUS1,
> GENMASK(31, 0));
> > +	if (ret)
> > +		return ret;
> > +
> > +	return regmap_update_bits(st->regmap, ADE9078_REG_MASK1,
> interrupts,
> > +				  interrupts);
> > +}

MASK1 is a 32 bit registers.

> > +/*
> > + * ade9078_wfb_interrupt_setup() - Configures the wave form buffer
> interrupt
> > + * according to modes
> > + * @st:		ade9078 device data
> > + * @mode:	modes according to datasheet; values [0-2]
> > + *
> > + * This sets the interrupt register and other registers related to the
> > + * interrupts according to mode [0-2] from the datasheet
> > + */
> > +static int ade9078_wfb_interrupt_setup(struct ade9078_state *st, u8
> mode)
> > +{
> > +	int ret;
> > +
> > +	ret = regmap_write(st->regmap, ADE9078_REG_WFB_TRG_CFG, 0x0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (mode == ADE9078_WFB_FULL_MODE || mode ==
> ADE9078_WFB_EN_TRIG_MODE) {
> 
> A switch statement would be cleaner here.
> 
> > +		ret = regmap_write(st->regmap,
> ADE9078_REG_WFB_PG_IRQEN,
> > +				   ADE9078_MODE_0_1_PAGE_BIT);
> > +		if (ret)
> > +			return ret;
> > +	} else if (mode == ADE9078_WFB_C_EN_TRIG_MODE) {
> > +		ret = regmap_write(st->regmap,
> ADE9078_REG_WFB_PG_IRQEN,
> > +				   ADE9078_MODE_2_PAGE_BIT);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	ret = regmap_write(st->regmap, ADE9078_REG_STATUS0,
> GENMASK(31, 0));
> > +	if (ret)
> > +		return ret;
> > +
> > +	return regmap_update_bits(st->regmap, ADE9078_REG_MASK0,
> > +				  ADE9078_ST0_PAGE_FULL_BIT,
> > +				  ADE9078_ST0_PAGE_FULL_BIT);
> > +	if (ret)
> 
> Unreachable code...
> 
> > +		return ret;
> > +
> > +	return 0;
> > +}

MASK0 is a 32 bit registers.

> > +/*
> > + * ade9078_buffer_postdisable() - after the iio is disable
> > + * this will disable the ade9078 internal buffer for acquisition
> > + * @indio_dev:	the IIO device
> > + */
> > +static int ade9078_buffer_postdisable(struct iio_dev *indio_dev)
> > +{
> > +	struct ade9078_state *st = iio_priv(indio_dev);
> > +	u32 interrupts = 0;
> > +	int ret;
> > +
> > +	ret = ade9078_en_wfb(st, false);
> > +	if (ret) {
> > +		dev_err(&st->spi->dev, "Post-disable wfb disable fail");
> > +		return ret;
> > +	}
> > +
> > +	ret = regmap_write(st->regmap, ADE9078_REG_WFB_TRG_CFG, 0x0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	interrupts |= ADE9078_ST0_WFB_TRIG_BIT;
> > +	interrupts |= ADE9078_ST0_PAGE_FULL_BIT;
> > +
> > +	return regmap_update_bits(st->regmap, ADE9078_REG_MASK0,
> interrupts, 0);
> > +	if (ret) {
> > +		dev_err(&st->spi->dev, "Post-disable update maks0 fail");
> > +		return ret;
> > +	}
> > +
> > +	ret = regmap_write(st->regmap, ADE9078_REG_STATUS0,
> GENMASK(31, 0));
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> 
> return regmap_write()...
> 
> > +}

MASK0 is updated again.

> > +/*
> > + * ade9078_setup() - initial register setup of the ade9078
> > + * @st:		ade9078 device data
> > + */
> > +static int ade9078_setup(struct ade9078_state *st)
> > +{
> > +	int ret = 0;
> The value here is never used.
> 
> Make sure you run a bunch of build tests including setting W=1 and ensuring
> it
> is clean + run at very least sparse.  Some of those tests would warn about
> this.
> 
> > +
> > +	ret = regmap_multi_reg_write(st->regmap, ade9078_reg_sequence,
> > +				     ARRAY_SIZE(ade9078_reg_sequence));
> > +	if (ret)
> > +		return ret;
> > +

regmap_multi_reg_write is another function I don't think I could use at its 
full extent if I switch to standard regmap with bulk transfers, because I 
configure both 16 bit and 32 bit registres. The 32 bit registers configured
in this set are listed above, near the begging of the e-mail.

> > +	msleep_interruptible(2);
> > +
> > +	ret = regmap_write(st->regmap, ADE9078_REG_STATUS0,
> GENMASK(31, 0));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(st->regmap, ADE9078_REG_STATUS1,
> GENMASK(31, 0));
> 
> return regmap_write()
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	return ret;
> > +}
> > +

I left this part in for context.

> > +/*
> > + * Regmap configuration
> > + * The register access of the ade9078 requires a 16 bit address
> > + * with the read flag on bit 3. This is not supported by default
> > + * regmap functionality, thus reg_read and reg_write have been
> > + * replaced with custom functions
> 
> How big would the changes needed to support this in the regmap
> core be?   Superficially I can't immediately see why it won't work.
> regmap appears to support setting flags in any of the address bytes
> as it uses regmap_set_work_buf_flag_mask() internally and
> that will set bits in any of the reg_bits/8 bytes.
> 
> > + */
> > +static const struct regmap_config ade9078_regmap_config = {
> > +	.reg_bits = 16,
> > +	.val_bits = 32,
> > +	.zero_flag_mask = true,
> > +	.cache_type = REGCACHE_RBTREE,
> > +	.reg_read = ade9078_spi_read_reg,
> > +	.reg_write = ade9078_spi_write_reg,
> > +	.volatile_reg = ade9078_is_volatile_reg,
> > +};

It dose work but as mentioned above it comes with an additional set
of driver specific functions which will ultimately mask the underlying 
regmap functions in the best case scenario.

All in all, I could implement the changes for V2 but given the fact that
this would mean replacing some regmap functions, I decide to ask for your 
opinion and some guidance first.

- Ciprian

Powered by blists - more mailing lists