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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AM6PR10MB226325A1203A99CE49C364EC809B0@AM6PR10MB2263.EURPRD10.PROD.OUTLOOK.COM>
Date:   Thu, 18 Jun 2020 10:57:47 +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 11: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.