On Thu, 7 Apr 2022 12:56:47 +0000
"Hegbeli, Ciprian" <Ciprian.Hegbeli@xxxxxxxxxx> wrote:
> > 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.
Ah. I'd missed that. Indeed changes the whole discussion.
>
> > > +/*
> >
> > 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).
Agreed - regmap doesn't fit well in my opinion when there are a mixture
of register sizes. This may just be a case where you indeed should
just not use regmap at all. It's challenging to make it work
and sometimes it's not sensible to bother.
>
> > > + * 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.
My assumption (wrong :) was that this was the normal 'large register' case
where we were actually looking at 2 registers that had to always be
read as a pair to make up the large register. That isn't true here.
>
> > > +
> > > + 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.
I'd just drop regmap and do things the old fashions (and more flexible)
way. Whilst the device is register based - mixed sized registers are
a horrible hardware quirk so it doesn't map (afaik) to regmap.
Jonathan
>
> - Ciprian
