Messages in this thread | | | From | Adam Thomson <> | Subject | RE: [PATCH v3 1/2] mfd: da9063: Fix revision handling to correctly select reg tables | Date | Thu, 18 Jun 2020 12:48:38 +0000 |
| |
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@diasemi.com> > > > > --- > > > > 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
| |