RE: [PATCH v3 1/2] mfd: da9063: Fix revision handling to correctly select reg tables

From: Adam Thomson
Date: Thu Jun 18 2020 - 08:48:48 EST


On 18 June 2020 12:15, Lee Jones wrote:

> > > > The current implementation performs checking in the i2c_probe()
> > > > function of the variant_code but does this immediately after the
> > > > containing struct has been initialised as all zero. This means the
> > > > check for variant code will always default to using the BB tables
> > > > and will never select AD. The variant code is subsequently set
> > > > by device_init() and later used by the RTC so really it's a little
> > > > fortunate this mismatch works.
> > > >
> > > > This update adds raw I2C read access functionality to read the chip
> > > > and variant/revision information (common to all revisions) so that
> > > > it can subsequently correctly choose the proper regmap tables for
> > > > real initialisation.
> > > >
> > > > Signed-off-by: Adam Thomson
> <Adam.Thomson.Opensource@xxxxxxxxxxx>
> > > > ---
> > > > drivers/mfd/da9063-core.c | 31 ------
> > > > drivers/mfd/da9063-i2c.c | 184
> +++++++++++++++++++++++++++++++-
> > > ---
> > > > include/linux/mfd/da9063/registers.h | 15 ++-
> > > > 3 files changed, 177 insertions(+), 53 deletions(-)
> > >
> > > [...]
> > >
> > > > + * Raw I2C access required for just accessing chip and variant info before
> we
> > > > + * know which device is present. The info read from the device using this
> > > > + * approach is then used to select the correct regmap tables.
> > > > + */
> > > > +
> > > > +#define DA9063_REG_PAGE_SIZE 0x100
> > > > +#define DA9063_REG_PAGED_ADDR_MASK 0xFF
> > > > +
> > > > +enum da9063_page_sel_buf_fmt {
> > > > + DA9063_PAGE_SEL_BUF_PAGE_REG = 0,
> > > > + DA9063_PAGE_SEL_BUF_PAGE_VAL,
> > > > + DA9063_PAGE_SEL_BUF_SIZE,
> > > > +};
> > > > +
> > > > +enum da9063_paged_read_msgs {
> > > > + DA9063_PAGED_READ_MSG_PAGE_SEL = 0,
> > > > + DA9063_PAGED_READ_MSG_REG_SEL,
> > > > + DA9063_PAGED_READ_MSG_DATA,
> > > > + DA9063_PAGED_READ_MSG_CNT,
> > > > +};
> > > > +
> > > > +static int da9063_i2c_blockreg_read(struct i2c_client *client, u16 addr,
> > > > + u8 *buf, int count)
> > > > +{
> > > > + struct i2c_msg xfer[DA9063_PAGED_READ_MSG_CNT];
> > > > + u8 page_sel_buf[DA9063_PAGE_SEL_BUF_SIZE];
> > > > + u8 page_num, paged_addr;
> > > > + int ret;
> > > > +
> > > > + /* Determine page info based on register address */
> > > > + page_num = (addr / DA9063_REG_PAGE_SIZE);
> > > > + if (page_num > 1) {
> > > > + dev_err(&client->dev, "Invalid register address provided\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + paged_addr = (addr % DA9063_REG_PAGE_SIZE) &
> > > DA9063_REG_PAGED_ADDR_MASK;
> > > > + page_sel_buf[DA9063_PAGE_SEL_BUF_PAGE_REG] =
> > > DA9063_REG_PAGE_CON;
> > > > + page_sel_buf[DA9063_PAGE_SEL_BUF_PAGE_VAL] =
> > > > + (page_num << DA9063_I2C_PAGE_SEL_SHIFT) &
> > > DA9063_REG_PAGE_MASK;
> > > > +
> > > > + /* Write reg address, page selection */
> > > > + xfer[DA9063_PAGED_READ_MSG_PAGE_SEL].addr = client->addr;
> > > > + xfer[DA9063_PAGED_READ_MSG_PAGE_SEL].flags = 0;
> > > > + xfer[DA9063_PAGED_READ_MSG_PAGE_SEL].len =
> > > DA9063_PAGE_SEL_BUF_SIZE;
> > > > + xfer[DA9063_PAGED_READ_MSG_PAGE_SEL].buf = page_sel_buf;
> > > > +
> > > > + /* Select register address */
> > > > + xfer[DA9063_PAGED_READ_MSG_REG_SEL].addr = client->addr;
> > > > + xfer[DA9063_PAGED_READ_MSG_REG_SEL].flags = 0;
> > > > + xfer[DA9063_PAGED_READ_MSG_REG_SEL].len = sizeof(paged_addr);
> > > > + xfer[DA9063_PAGED_READ_MSG_REG_SEL].buf = &paged_addr;
> > > > +
> > > > + /* Read data */
> > > > + xfer[DA9063_PAGED_READ_MSG_DATA].addr = client->addr;
> > > > + xfer[DA9063_PAGED_READ_MSG_DATA].flags = I2C_M_RD;
> > > > + xfer[DA9063_PAGED_READ_MSG_DATA].len = count;
> > > > + xfer[DA9063_PAGED_READ_MSG_DATA].buf = buf;
> > > > +
> > > > + ret = i2c_transfer(client->adapter, xfer,
> > > DA9063_PAGED_READ_MSG_CNT);
> > > > + if (ret < 0) {
> > > > + dev_err(&client->dev, "Paged block read failed: %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + if (ret != DA9063_PAGED_READ_MSG_CNT) {
> > > > + dev_err(&client->dev, "Paged block read failed to complete\n");
> > > > + return -EIO;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > >
> > > Rather than open coding this, does it make sense to register a small
> > > (temporary?) Device ID Regmap to read from?
> >
> > The original patch submission did exactly that but you indicated you weren't
> > keen due to overheads, hence the implementation above. Actually what we
> have
> > here is a bit smaller than the regmap approach and I really I'd rather not
> > have to respin again just to revert to something that was dismissed in the first
> > place over 6 months ago.
>
> Actually the conversation went like:
>
> Lee:
> IIUC, you have a dependency issue whereby the device type is required
> before you can select the correct Regmap configuration. Is that
> correct?
>
> If so, using Regmap for the initial register reads sounds like
> over-kill. What's stopping you simply using raw reads before the
> Regmap is instantiated?
>
> Adam:
> Actually nothing and I did consider this at the start. Nice thing with regmap
> is it's all tidily contained and provides the page swapping mechanism to access
> higher page registers like the variant information. Given this is only once at
> probe time it felt like this was a reasonable solution. However if you're not
> keen I can update to use raw access instead.
>
> Lee:
> It would be nice to compare the 2 solutions side by side. I can't see
> the raw reads of a few device-ID registers being anywhere near 170
> lines though.
>
> Ah, they are I2C transactions? Not the nice readl()s I had in mind.
>
> Adam:
> I can knock something together though just to see what it looks like.
>
> Lee:
> Well, I'd appreciated that, thanks.
>
> So now we can see them side-by-side we can take them on their own
> merits. When I initially requested raw reads, I had readl()s in mind,
> rather than the extensive code required to read the required registers
> via I2C.

To be fair those changes were in V2 of the patch set, which is why I was a quite
surprised by your suggestion today as you hadn't made this comment against that
version, given the previous discussion.

>
> However, it looks like there is very little difference between them,
> thus I do not see a benefit to reverting it back. The current version
> seems fine.
>
> I'll conduct a full review shortly, when I have a little more spare
> time (looking at my current TODO list, this will probably be Monday
> now). Although everything does look fine at first glance.
>

Thanks. That would be appreciated.

> --
> Lee Jones [æçæ]
> Senior Technical Lead - Developer Services
> Linaro.org â Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog