Message-ID: <AM6PR10MB22631681F86F61E323055922809B0@AM6PR10MB2263.EURPRD10.PROD.OUTLOOK.COM>
Date:   Thu, 18 Jun 2020 12:48:38 +0000
From:   Adam Thomson <Adam.Thomson.Opensource@...semi.com>
To:     Lee Jones <lee.jones@...aro.org>,
        Adam Thomson <Adam.Thomson.Opensource@...semi.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Support Opensource <Support.Opensource@...semi.com>
Subject: RE: [PATCH v3 1/2] mfd: da9063: Fix revision handling to correctly
 select reg tables

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@...semi.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

Powered by blists - more mailing lists