[RESEND PATCH 0/2] Resolve revision handling and add support for DA silicon

[RESEND PATCH 0/2] Resolve revision handling and add support for DA silicon

[Adam Thomson]

This patch set fixes the currently broken revision handling in the driver's
i2c_probe() function and then adds DA support to existing permitted revisions.

Adam Thomson (2):
  mfd: da9063: Fix revision handling to correctly select reg tables
  mfd: da9063: Add support for latest DA silicon revision

 drivers/mfd/da9063-core.c            |  31 -----
 drivers/mfd/da9063-i2c.c             | 254 +++++++++++++++++++++++++++++++----
 include/linux/mfd/da9063/core.h      |   1 +
 include/linux/mfd/da9063/registers.h |  15 ++-
 4 files changed, 240 insertions(+), 61 deletions(-)

-- 
1.9.1

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

[Adam Thomson]

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 creates an initial temporary regmap instantiation to
simply 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             | 167 +++++++++++++++++++++++++++++++----
 include/linux/mfd/da9063/registers.h |  15 ++--
 3 files changed, 160 insertions(+), 53 deletions(-)

diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
index b125f90d..a353d52 100644
--- a/drivers/mfd/da9063-core.c
+++ b/drivers/mfd/da9063-core.c
@@ -160,7 +160,6 @@ static int da9063_clear_fault_log(struct da9063 *da9063)
 
 int da9063_device_init(struct da9063 *da9063, unsigned int irq)
 {
-	int model, variant_id, variant_code;
 	int ret;
 
 	ret = da9063_clear_fault_log(da9063);
@@ -171,36 +170,6 @@ int da9063_device_init(struct da9063 *da9063, unsigned int irq)
 	da9063->irq_base = -1;
 	da9063->chip_irq = irq;
 
-	ret = regmap_read(da9063->regmap, DA9063_REG_CHIP_ID, &model);
-	if (ret < 0) {
-		dev_err(da9063->dev, "Cannot read chip model id.\n");
-		return -EIO;
-	}
-	if (model != PMIC_CHIP_ID_DA9063) {
-		dev_err(da9063->dev, "Invalid chip model id: 0x%02x\n", model);
-		return -ENODEV;
-	}
-
-	ret = regmap_read(da9063->regmap, DA9063_REG_CHIP_VARIANT, &variant_id);
-	if (ret < 0) {
-		dev_err(da9063->dev, "Cannot read chip variant id.\n");
-		return -EIO;
-	}
-
-	variant_code = variant_id >> DA9063_CHIP_VARIANT_SHIFT;
-
-	dev_info(da9063->dev,
-		 "Device detected (chip-ID: 0x%02X, var-ID: 0x%02X)\n",
-		 model, variant_id);
-
-	if (variant_code < PMIC_DA9063_BB && variant_code != PMIC_DA9063_AD) {
-		dev_err(da9063->dev,
-			"Cannot support variant code: 0x%02X\n", variant_code);
-		return -ENODEV;
-	}
-
-	da9063->variant_code = variant_code;
-
 	ret = da9063_irq_init(da9063);
 	if (ret) {
 		dev_err(da9063->dev, "Cannot initialize interrupts.\n");
diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c
index 455de74..c7f3057 100644
--- a/drivers/mfd/da9063-i2c.c
+++ b/drivers/mfd/da9063-i2c.c
@@ -22,12 +22,107 @@
 #include <linux/of.h>
 #include <linux/regulator/of_regulator.h>
 
+/*
+ * Temporary regmap config for just accessing chip and variant info before we
+ * know which device is present. The info read from the device using this config
+ * is then used to select the correct regmap tables.
+ */
+
+static const struct regmap_range da9063_tmp_readable_ranges[] = {
+	regmap_reg_range(DA9063_REG_PAGE_CON, DA9063_REG_PAGE_CON),
+	regmap_reg_range(DA9063_REG_DEVICE_ID, DA9063_REG_VARIANT_ID),
+};
+
+static const struct regmap_range da9063_tmp_writeable_ranges[] = {
+	regmap_reg_range(DA9063_REG_PAGE_CON, DA9063_REG_PAGE_CON),
+};
+
+static const struct regmap_access_table da9063_tmp_readable_table = {
+	.yes_ranges = da9063_tmp_readable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(da9063_tmp_readable_ranges),
+};
+
+static const struct regmap_access_table da9063_tmp_writeable_table = {
+	.yes_ranges = da9063_tmp_writeable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(da9063_tmp_writeable_ranges),
+};
+
+static const struct regmap_range_cfg da9063_tmp_range_cfg[] = {
+	{
+		.range_min = DA9063_REG_PAGE_CON,
+		.range_max = DA9063_REG_VARIANT_ID,
+		.selector_reg = DA9063_REG_PAGE_CON,
+		.selector_mask = 1 << DA9063_I2C_PAGE_SEL_SHIFT,
+		.selector_shift = DA9063_I2C_PAGE_SEL_SHIFT,
+		.window_start = 0,
+		.window_len = 256,
+	}
+};
+
+static struct regmap_config da9063_tmp_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.ranges = da9063_tmp_range_cfg,
+	.num_ranges = ARRAY_SIZE(da9063_tmp_range_cfg),
+	.max_register = DA9063_REG_VARIANT_ID,
+	.cache_type = REGCACHE_NONE,
+	.rd_table = &da9063_tmp_readable_table,
+	.wr_table = &da9063_tmp_writeable_table,
+};
+
+static int da9063_get_device_type(struct i2c_client *i2c, struct da9063 *da9063)
+{
+	int ret;
+	int device_id, variant_id;
+	struct regmap *tmp_regmap;
+
+	tmp_regmap = regmap_init_i2c(i2c, &da9063_tmp_regmap_config);
+	if (IS_ERR(tmp_regmap)) {
+		dev_err(da9063->dev,
+			"Failed to allocate temporary register map\n");
+		return PTR_ERR(tmp_regmap);
+	}
+
+	ret = regmap_read(tmp_regmap, DA9063_REG_DEVICE_ID, &device_id);
+	if (ret < 0) {
+		dev_err(da9063->dev, "Cannot read chip device id.\n");
+		return -EIO;
+	}
+
+	if (device_id != PMIC_CHIP_ID_DA9063) {
+		dev_err(da9063->dev,
+			"Invalid chip device id: 0x%02x\n", device_id);
+		return -ENODEV;
+	}
+
+	ret = regmap_read(tmp_regmap, DA9063_REG_VARIANT_ID, &variant_id);
+	if (ret < 0) {
+		dev_err(da9063->dev, "Cannot read chip variant id.\n");
+		return -EIO;
+	}
+
+	dev_info(da9063->dev,
+		 "Device detected (chip-ID: 0x%02X, var-ID: 0x%02X)\n",
+		 device_id, variant_id);
+
+	da9063->variant_code = (variant_id & DA9063_VARIANT_ID_MRC_MASK)
+			       >> DA9063_VARIANT_ID_MRC_SHIFT;
+
+	regmap_exit(tmp_regmap);
+
+	return 0;
+}
+
+/*
+ * Variant specific regmap configs
+ */
+
 static const struct regmap_range da9063_ad_readable_ranges[] = {
 	regmap_reg_range(DA9063_REG_PAGE_CON, DA9063_AD_REG_SECOND_D),
 	regmap_reg_range(DA9063_REG_SEQ, DA9063_REG_ID_32_31),
 	regmap_reg_range(DA9063_REG_SEQ_A, DA9063_REG_AUTO3_LOW),
 	regmap_reg_range(DA9063_REG_T_OFFSET, DA9063_AD_REG_GP_ID_19),
-	regmap_reg_range(DA9063_REG_CHIP_ID, DA9063_REG_CHIP_VARIANT),
+	regmap_reg_range(DA9063_REG_DEVICE_ID, DA9063_REG_VARIANT_ID),
 };
 
 static const struct regmap_range da9063_ad_writeable_ranges[] = {
@@ -72,7 +167,7 @@
 	regmap_reg_range(DA9063_REG_SEQ, DA9063_REG_ID_32_31),
 	regmap_reg_range(DA9063_REG_SEQ_A, DA9063_REG_AUTO3_LOW),
 	regmap_reg_range(DA9063_REG_T_OFFSET, DA9063_BB_REG_GP_ID_19),
-	regmap_reg_range(DA9063_REG_CHIP_ID, DA9063_REG_CHIP_VARIANT),
+	regmap_reg_range(DA9063_REG_DEVICE_ID, DA9063_REG_VARIANT_ID),
 };
 
 static const struct regmap_range da9063_bb_writeable_ranges[] = {
@@ -117,7 +212,7 @@
 	regmap_reg_range(DA9063_REG_SEQ, DA9063_REG_ID_32_31),
 	regmap_reg_range(DA9063_REG_SEQ_A, DA9063_REG_AUTO3_LOW),
 	regmap_reg_range(DA9063_REG_T_OFFSET, DA9063_BB_REG_GP_ID_19),
-	regmap_reg_range(DA9063_REG_CHIP_ID, DA9063_REG_CHIP_VARIANT),
+	regmap_reg_range(DA9063_REG_DEVICE_ID, DA9063_REG_VARIANT_ID),
 };
 
 static const struct regmap_range da9063l_bb_writeable_ranges[] = {
@@ -159,7 +254,7 @@
 static const struct regmap_range_cfg da9063_range_cfg[] = {
 	{
 		.range_min = DA9063_REG_PAGE_CON,
-		.range_max = DA9063_REG_CHIP_VARIANT,
+		.range_max = DA9063_REG_CONFIG_ID,
 		.selector_reg = DA9063_REG_PAGE_CON,
 		.selector_mask = 1 << DA9063_I2C_PAGE_SEL_SHIFT,
 		.selector_shift = DA9063_I2C_PAGE_SEL_SHIFT,
@@ -173,7 +268,7 @@
 	.val_bits = 8,
 	.ranges = da9063_range_cfg,
 	.num_ranges = ARRAY_SIZE(da9063_range_cfg),
-	.max_register = DA9063_REG_CHIP_VARIANT,
+	.max_register = DA9063_REG_CONFIG_ID,
 
 	.cache_type = REGCACHE_RBTREE,
 };
@@ -199,18 +294,56 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
 	da9063->chip_irq = i2c->irq;
 	da9063->type = id->driver_data;
 
-	if (da9063->variant_code == PMIC_DA9063_AD) {
-		da9063_regmap_config.rd_table = &da9063_ad_readable_table;
-		da9063_regmap_config.wr_table = &da9063_ad_writeable_table;
-		da9063_regmap_config.volatile_table = &da9063_ad_volatile_table;
-	} else if (da9063->type == PMIC_TYPE_DA9063L) {
-		da9063_regmap_config.rd_table = &da9063l_bb_readable_table;
-		da9063_regmap_config.wr_table = &da9063l_bb_writeable_table;
-		da9063_regmap_config.volatile_table = &da9063l_bb_volatile_table;
-	} else {
-		da9063_regmap_config.rd_table = &da9063_bb_readable_table;
-		da9063_regmap_config.wr_table = &da9063_bb_writeable_table;
-		da9063_regmap_config.volatile_table = &da9063_bb_volatile_table;
+	ret = da9063_get_device_type(i2c, da9063);
+	if (ret < 0)
+		return ret;
+
+	switch (da9063->type) {
+	case PMIC_TYPE_DA9063:
+		switch (da9063->variant_code) {
+		case PMIC_DA9063_AD:
+			da9063_regmap_config.rd_table =
+				&da9063_ad_readable_table;
+			da9063_regmap_config.wr_table =
+				&da9063_ad_writeable_table;
+			da9063_regmap_config.volatile_table =
+				&da9063_ad_volatile_table;
+			break;
+		case PMIC_DA9063_BB:
+		case PMIC_DA9063_CA:
+			da9063_regmap_config.rd_table =
+				&da9063_bb_readable_table;
+			da9063_regmap_config.wr_table =
+				&da9063_bb_writeable_table;
+			da9063_regmap_config.volatile_table =
+				&da9063_bb_volatile_table;
+			break;
+		default:
+			dev_err(da9063->dev,
+				"Chip variant not supported for DA9063\n");
+			return -ENODEV;
+		}
+		break;
+	case PMIC_TYPE_DA9063L:
+		switch (da9063->variant_code) {
+		case PMIC_DA9063_BB:
+		case PMIC_DA9063_CA:
+			da9063_regmap_config.rd_table =
+				&da9063l_bb_readable_table;
+			da9063_regmap_config.wr_table =
+				&da9063l_bb_writeable_table;
+			da9063_regmap_config.volatile_table =
+				&da9063l_bb_volatile_table;
+			break;
+		default:
+			dev_err(da9063->dev,
+				"Chip variant not supported for DA9063L\n");
+			return -ENODEV;
+		}
+		break;
+	default:
+		dev_err(da9063->dev, "Chip type not supported\n");
+		return -ENODEV;
 	}
 
 	da9063->regmap = devm_regmap_init_i2c(i2c, &da9063_regmap_config);
diff --git a/include/linux/mfd/da9063/registers.h b/include/linux/mfd/da9063/registers.h
index ba706b0..1dbabf1 100644
--- a/include/linux/mfd/da9063/registers.h
+++ b/include/linux/mfd/da9063/registers.h
@@ -292,8 +292,10 @@
 #define	DA9063_BB_REG_GP_ID_19		0x134
 
 /* Chip ID and variant */
-#define	DA9063_REG_CHIP_ID		0x181
-#define	DA9063_REG_CHIP_VARIANT		0x182
+#define	DA9063_REG_DEVICE_ID		0x181
+#define	DA9063_REG_VARIANT_ID		0x182
+#define	DA9063_REG_CUSTOMER_ID		0x183
+#define	DA9063_REG_CONFIG_ID		0x184
 
 /*
  * PMIC registers bits
@@ -929,9 +931,6 @@
 #define	DA9063_RTC_CLOCK			0x40
 #define	DA9063_OUT_32K_EN			0x80
 
-/* DA9063_REG_CHIP_VARIANT */
-#define	DA9063_CHIP_VARIANT_SHIFT		4
-
 /* DA9063_REG_BUCK_ILIM_A (addr=0x9A) */
 #define DA9063_BIO_ILIM_MASK			0x0F
 #define DA9063_BMEM_ILIM_MASK			0xF0
@@ -1065,4 +1064,10 @@
 #define		DA9063_MON_A10_IDX_LDO9		0x04
 #define		DA9063_MON_A10_IDX_LDO10	0x05
 
+/* DA9063_REG_VARIANT_ID (addr=0x182) */
+#define	DA9063_VARIANT_ID_VRC_SHIFT		0
+#define DA9063_VARIANT_ID_VRC_MASK		0x0F
+#define	DA9063_VARIANT_ID_MRC_SHIFT		4
+#define DA9063_VARIANT_ID_MRC_MASK		0xF0
+
 #endif /* _DA9063_REG_H */
-- 
1.9.1

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

[Lee Jones]

On Fri, 24 Jan 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 creates an initial temporary regmap instantiation to
> simply 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.

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?

> Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> ---
>  drivers/mfd/da9063-core.c            |  31 -------
>  drivers/mfd/da9063-i2c.c             | 167 +++++++++++++++++++++++++++++++----
>  include/linux/mfd/da9063/registers.h |  15 ++--
>  3 files changed, 160 insertions(+), 53 deletions(-)

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

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

[Adam Thomson]

On 24 February 2020 09:57, Lee Jones wrote:

> On Fri, 24 Jan 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 creates an initial temporary regmap instantiation to
> > simply 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.
> 
> IIUC, you have a dependency issue whereby the device type is required
> before you can select the correct Regmap configuration.  Is that
> correct?

Yep, spot on.

> 
> 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?

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.

> 
> > Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> > ---
> >  drivers/mfd/da9063-core.c            |  31 -------
> >  drivers/mfd/da9063-i2c.c             | 167 +++++++++++++++++++++++++++++++-
> ---
> >  include/linux/mfd/da9063/registers.h |  15 ++--
> >  3 files changed, 160 insertions(+), 53 deletions(-)
> 
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

On Mon, 24 Feb 2020, Adam Thomson wrote:

> On 24 February 2020 09:57, Lee Jones wrote:
> 
> > On Fri, 24 Jan 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 creates an initial temporary regmap instantiation to
> > > simply 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.
> > 
> > IIUC, you have a dependency issue whereby the device type is required
> > before you can select the correct Regmap configuration.  Is that
> > correct?
> 
> Yep, spot on.
> 
> > 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?
> 
> 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.

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.

> > > Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> > > ---
> > >  drivers/mfd/da9063-core.c            |  31 -------
> > >  drivers/mfd/da9063-i2c.c             | 167 +++++++++++++++++++++++++++++++-
> > ---
> > >  include/linux/mfd/da9063/registers.h |  15 ++--
> > >  3 files changed, 160 insertions(+), 53 deletions(-)
> > 

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

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

[Adam Thomson]

On 24 February 2020 11:27, Lee Jones wrote:

> On Mon, 24 Feb 2020, Adam Thomson wrote:
> 
> > On 24 February 2020 09:57, Lee Jones wrote:
> >
> > > On Fri, 24 Jan 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 creates an initial temporary regmap instantiation to
> > > > simply 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.
> > >
> > > IIUC, you have a dependency issue whereby the device type is required
> > > before you can select the correct Regmap configuration.  Is that
> > > correct?
> >
> > Yep, spot on.
> >
> > > 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?
> >
> > 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.
> 
> 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.

To be fair the regmap specific additions for the temporary register access, are
maybe 50 - 60 lines. The rest is to do with handling the result which you'll
need anyway to select the correct register map. I reckon to provide raw read and
write access we're going to probably be similar or more as we'll need to write
the page register then read from the relevant ID registers. Using this an
example for the lines count:

https://elixir.bootlin.com/linux/latest/source/drivers/mfd/tps6507x.c#L37

I can knock something together though just to see what it looks like.

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

[Lee Jones]

On Mon, 24 Feb 2020, Adam Thomson wrote:

> On 24 February 2020 11:27, Lee Jones wrote:
> 
> > On Mon, 24 Feb 2020, Adam Thomson wrote:
> > 
> > > On 24 February 2020 09:57, Lee Jones wrote:
> > >
> > > > On Fri, 24 Jan 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 creates an initial temporary regmap instantiation to
> > > > > simply 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.
> > > >
> > > > IIUC, you have a dependency issue whereby the device type is required
> > > > before you can select the correct Regmap configuration.  Is that
> > > > correct?
> > >
> > > Yep, spot on.
> > >
> > > > 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?
> > >
> > > 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.
> > 
> > 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.
> 
> To be fair the regmap specific additions for the temporary register access, are
> maybe 50 - 60 lines. The rest is to do with handling the result which you'll
> need anyway to select the correct register map. I reckon to provide raw read and
> write access we're going to probably be similar or more as we'll need to write
> the page register then read from the relevant ID registers. Using this an
> example for the lines count:
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/mfd/tps6507x.c#L37

Ah, they are I2C transactions?  Not the nice readl()s I had in mind.

> I can knock something together though just to see what it looks like.

Well, I'd appreciated that, thanks.

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

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

[Adam Thomson]

On 25 February 2020 09:00, Lee Jones wrote:

> On Mon, 24 Feb 2020, Adam Thomson wrote:
> 
> > On 24 February 2020 11:27, Lee Jones wrote:
> >
> > > On Mon, 24 Feb 2020, Adam Thomson wrote:
> > >
> > > > On 24 February 2020 09:57, Lee Jones wrote:
> > > >
> > > > > On Fri, 24 Jan 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 creates an initial temporary regmap instantiation to
> > > > > > simply 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.
> > > > >
> > > > > IIUC, you have a dependency issue whereby the device type is required
> > > > > before you can select the correct Regmap configuration.  Is that
> > > > > correct?
> > > >
> > > > Yep, spot on.
> > > >
> > > > > 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?
> > > >
> > > > 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.
> > >
> > > 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.
> >
> > To be fair the regmap specific additions for the temporary register access, are
> > maybe 50 - 60 lines. The rest is to do with handling the result which you'll
> > need anyway to select the correct register map. I reckon to provide raw read
> and
> > write access we're going to probably be similar or more as we'll need to write
> > the page register then read from the relevant ID registers. Using this an
> > example for the lines count:
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/mfd/tps6507x.c#L37
> 
> Ah, they are I2C transactions?  Not the nice readl()s I had in mind.

Yes, I2C register access so needs more work.

> 
> > I can knock something together though just to see what it looks like.
> 
> Well, I'd appreciated that, thanks.

No problem, I'll try and get something back soon.

[RESEND PATCH 2/2] mfd: da9063: Add support for latest DA silicon revision

[Adam Thomson]

This update adds new regmap tables to support the latest DA silicon
which will automatically be selected based on the chip and variant
information read from the device.

Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
---
 drivers/mfd/da9063-i2c.c        | 91 ++++++++++++++++++++++++++++++++++++-----
 include/linux/mfd/da9063/core.h |  1 +
 2 files changed, 82 insertions(+), 10 deletions(-)

diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c
index c7f3057..ee48203 100644
--- a/drivers/mfd/da9063-i2c.c
+++ b/drivers/mfd/da9063-i2c.c
@@ -180,7 +180,7 @@ static int da9063_get_device_type(struct i2c_client *i2c, struct da9063 *da9063)
 	regmap_reg_range(DA9063_BB_REG_GP_ID_0, DA9063_BB_REG_GP_ID_19),
 };
 
-static const struct regmap_range da9063_bb_volatile_ranges[] = {
+static const struct regmap_range da9063_bb_da_volatile_ranges[] = {
 	regmap_reg_range(DA9063_REG_PAGE_CON, DA9063_REG_EVENT_D),
 	regmap_reg_range(DA9063_REG_CONTROL_A, DA9063_REG_CONTROL_B),
 	regmap_reg_range(DA9063_REG_CONTROL_E, DA9063_REG_CONTROL_F),
@@ -202,9 +202,9 @@ static int da9063_get_device_type(struct i2c_client *i2c, struct da9063 *da9063)
 	.n_yes_ranges = ARRAY_SIZE(da9063_bb_writeable_ranges),
 };
 
-static const struct regmap_access_table da9063_bb_volatile_table = {
-	.yes_ranges = da9063_bb_volatile_ranges,
-	.n_yes_ranges = ARRAY_SIZE(da9063_bb_volatile_ranges),
+static const struct regmap_access_table da9063_bb_da_volatile_table = {
+	.yes_ranges = da9063_bb_da_volatile_ranges,
+	.n_yes_ranges = ARRAY_SIZE(da9063_bb_da_volatile_ranges),
 };
 
 static const struct regmap_range da9063l_bb_readable_ranges[] = {
@@ -224,7 +224,7 @@ static int da9063_get_device_type(struct i2c_client *i2c, struct da9063 *da9063)
 	regmap_reg_range(DA9063_BB_REG_GP_ID_0, DA9063_BB_REG_GP_ID_19),
 };
 
-static const struct regmap_range da9063l_bb_volatile_ranges[] = {
+static const struct regmap_range da9063l_bb_da_volatile_ranges[] = {
 	regmap_reg_range(DA9063_REG_PAGE_CON, DA9063_REG_EVENT_D),
 	regmap_reg_range(DA9063_REG_CONTROL_A, DA9063_REG_CONTROL_B),
 	regmap_reg_range(DA9063_REG_CONTROL_E, DA9063_REG_CONTROL_F),
@@ -246,9 +246,64 @@ static int da9063_get_device_type(struct i2c_client *i2c, struct da9063 *da9063)
 	.n_yes_ranges = ARRAY_SIZE(da9063l_bb_writeable_ranges),
 };
 
-static const struct regmap_access_table da9063l_bb_volatile_table = {
-	.yes_ranges = da9063l_bb_volatile_ranges,
-	.n_yes_ranges = ARRAY_SIZE(da9063l_bb_volatile_ranges),
+static const struct regmap_access_table da9063l_bb_da_volatile_table = {
+	.yes_ranges = da9063l_bb_da_volatile_ranges,
+	.n_yes_ranges = ARRAY_SIZE(da9063l_bb_da_volatile_ranges),
+};
+
+static const struct regmap_range da9063_da_readable_ranges[] = {
+	regmap_reg_range(DA9063_REG_PAGE_CON, DA9063_BB_REG_SECOND_D),
+	regmap_reg_range(DA9063_REG_SEQ, DA9063_REG_ID_32_31),
+	regmap_reg_range(DA9063_REG_SEQ_A, DA9063_REG_AUTO3_LOW),
+	regmap_reg_range(DA9063_REG_T_OFFSET, DA9063_BB_REG_GP_ID_11),
+	regmap_reg_range(DA9063_REG_DEVICE_ID, DA9063_REG_VARIANT_ID),
+};
+
+static const struct regmap_range da9063_da_writeable_ranges[] = {
+	regmap_reg_range(DA9063_REG_PAGE_CON, DA9063_REG_PAGE_CON),
+	regmap_reg_range(DA9063_REG_FAULT_LOG, DA9063_REG_VSYS_MON),
+	regmap_reg_range(DA9063_REG_COUNT_S, DA9063_BB_REG_ALARM_Y),
+	regmap_reg_range(DA9063_REG_SEQ, DA9063_REG_ID_32_31),
+	regmap_reg_range(DA9063_REG_SEQ_A, DA9063_REG_AUTO3_LOW),
+	regmap_reg_range(DA9063_REG_CONFIG_I, DA9063_BB_REG_MON_REG_4),
+	regmap_reg_range(DA9063_BB_REG_GP_ID_0, DA9063_BB_REG_GP_ID_11),
+};
+
+static const struct regmap_access_table da9063_da_readable_table = {
+	.yes_ranges = da9063_da_readable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(da9063_da_readable_ranges),
+};
+
+static const struct regmap_access_table da9063_da_writeable_table = {
+	.yes_ranges = da9063_da_writeable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(da9063_da_writeable_ranges),
+};
+
+static const struct regmap_range da9063l_da_readable_ranges[] = {
+	regmap_reg_range(DA9063_REG_PAGE_CON, DA9063_REG_MON_A10_RES),
+	regmap_reg_range(DA9063_REG_SEQ, DA9063_REG_ID_32_31),
+	regmap_reg_range(DA9063_REG_SEQ_A, DA9063_REG_AUTO3_LOW),
+	regmap_reg_range(DA9063_REG_T_OFFSET, DA9063_BB_REG_GP_ID_11),
+	regmap_reg_range(DA9063_REG_DEVICE_ID, DA9063_REG_VARIANT_ID),
+};
+
+static const struct regmap_range da9063l_da_writeable_ranges[] = {
+	regmap_reg_range(DA9063_REG_PAGE_CON, DA9063_REG_PAGE_CON),
+	regmap_reg_range(DA9063_REG_FAULT_LOG, DA9063_REG_VSYS_MON),
+	regmap_reg_range(DA9063_REG_SEQ, DA9063_REG_ID_32_31),
+	regmap_reg_range(DA9063_REG_SEQ_A, DA9063_REG_AUTO3_LOW),
+	regmap_reg_range(DA9063_REG_CONFIG_I, DA9063_BB_REG_MON_REG_4),
+	regmap_reg_range(DA9063_BB_REG_GP_ID_0, DA9063_BB_REG_GP_ID_11),
+};
+
+static const struct regmap_access_table da9063l_da_readable_table = {
+	.yes_ranges = da9063l_da_readable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(da9063l_da_readable_ranges),
+};
+
+static const struct regmap_access_table da9063l_da_writeable_table = {
+	.yes_ranges = da9063l_da_writeable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(da9063l_da_writeable_ranges),
 };
 
 static const struct regmap_range_cfg da9063_range_cfg[] = {
@@ -316,7 +371,15 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
 			da9063_regmap_config.wr_table =
 				&da9063_bb_writeable_table;
 			da9063_regmap_config.volatile_table =
-				&da9063_bb_volatile_table;
+				&da9063_bb_da_volatile_table;
+			break;
+		case PMIC_DA9063_DA:
+			da9063_regmap_config.rd_table =
+				&da9063_da_readable_table;
+			da9063_regmap_config.wr_table =
+				&da9063_da_writeable_table;
+			da9063_regmap_config.volatile_table =
+				&da9063_bb_da_volatile_table;
 			break;
 		default:
 			dev_err(da9063->dev,
@@ -333,7 +396,15 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
 			da9063_regmap_config.wr_table =
 				&da9063l_bb_writeable_table;
 			da9063_regmap_config.volatile_table =
-				&da9063l_bb_volatile_table;
+				&da9063l_bb_da_volatile_table;
+			break;
+		case PMIC_DA9063_DA:
+			da9063_regmap_config.rd_table =
+				&da9063l_da_readable_table;
+			da9063_regmap_config.wr_table =
+				&da9063l_da_writeable_table;
+			da9063_regmap_config.volatile_table =
+				&da9063l_bb_da_volatile_table;
 			break;
 		default:
 			dev_err(da9063->dev,
diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
index 5cd06ab..fa7a43f 100644
--- a/include/linux/mfd/da9063/core.h
+++ b/include/linux/mfd/da9063/core.h
@@ -35,6 +35,7 @@ enum da9063_variant_codes {
 	PMIC_DA9063_AD = 0x3,
 	PMIC_DA9063_BB = 0x5,
 	PMIC_DA9063_CA = 0x6,
+	PMIC_DA9063_DA = 0x7,
 };
 
 /* Interrupts */
-- 
1.9.1

Re: [RESEND PATCH 2/2] mfd: da9063: Add support for latest DA silicon revision

[Lee Jones]

On Fri, 24 Jan 2020, Adam Thomson wrote:

> This update adds new regmap tables to support the latest DA silicon
> which will automatically be selected based on the chip and variant
> information read from the device.
> 
> Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> ---
>  drivers/mfd/da9063-i2c.c        | 91 ++++++++++++++++++++++++++++++++++++-----
>  include/linux/mfd/da9063/core.h |  1 +
>  2 files changed, 82 insertions(+), 10 deletions(-)

Looks fine at first blush:

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

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