Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 18 Jun 2020 11:15:11 +0100
From:   Lee Jones <lee.jones@linaro.org>
To:     Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
Cc:     linux-kernel@vger.kernel.org,
        Support Opensource <support.opensource@diasemi.com>
Subject: Re: [PATCH v3 1/2] mfd: da9063: Fix revision handling to correctly
 select reg tables
Message-ID: <20200618101511.GE954398@dell>
References: <cover.1587120185.git.Adam.Thomson.Opensource@diasemi.com>
 <a019b698f0c643455e07e7a94dcf0478b1b1f4d4.1587120185.git.Adam.Thomson.Opensource@diasemi.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <a019b698f0c643455e07e7a94dcf0478b1b1f4d4.1587120185.git.Adam.Thomson.Opensource@diasemi.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, 17 Apr 2020, Adam Thomson 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?

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog