* [PATCH v3 0/2] Resolve revision handling and add support for DA silicon
@ 2020-04-17 10:55 Adam Thomson
2020-04-17 10:55 ` [PATCH v3 1/2] mfd: da9063: Fix revision handling to correctly select reg tables Adam Thomson
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Adam Thomson @ 2020-04-17 10:55 UTC (permalink / raw)
To: Lee Jones; +Cc: linux-kernel, Support Opensource
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.
v3:
- Replaced magic numbers around I2C paged access code with enums and defines
- Small style tidy ups as requested by Lee Jones
v2:
- Use raw I2C read access instead of a temporary regmap to interrogate chip and
variant id registers
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 | 271 +++++++++++++++++++++++++++++++----
include/linux/mfd/da9063/core.h | 1 +
include/linux/mfd/da9063/registers.h | 15 +-
4 files changed, 257 insertions(+), 61 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 1/2] mfd: da9063: Fix revision handling to correctly select reg tables
2020-04-17 10:55 [PATCH v3 0/2] Resolve revision handling and add support for DA silicon Adam Thomson
@ 2020-04-17 10:55 ` Adam Thomson
2020-06-18 10:15 ` Lee Jones
2020-04-17 10:55 ` [PATCH v3 2/2] mfd: da9063: Add support for latest DA silicon revision Adam Thomson
2020-06-15 12:33 ` [PATCH v3 0/2] Resolve revision handling and add support for DA silicon Adam Thomson
2 siblings, 1 reply; 9+ messages in thread
From: Adam Thomson @ 2020-04-17 10:55 UTC (permalink / raw)
To: Lee Jones; +Cc: linux-kernel, Support Opensource
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(-)
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..4815489 100644
--- a/drivers/mfd/da9063-i2c.c
+++ b/drivers/mfd/da9063-i2c.c
@@ -22,12 +22,124 @@
#include <linux/of.h>
#include <linux/regulator/of_regulator.h>
+/*
+ * 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;
+}
+
+enum {
+ DA9063_DEV_ID_REG = 0,
+ DA9063_VAR_ID_REG,
+ DA9063_CHIP_ID_REGS,
+};
+
+static int da9063_get_device_type(struct i2c_client *i2c, struct da9063 *da9063)
+{
+ u8 buf[DA9063_CHIP_ID_REGS];
+ int ret;
+
+ ret = da9063_i2c_blockreg_read(i2c, DA9063_REG_DEVICE_ID, buf,
+ DA9063_CHIP_ID_REGS);
+ if (ret)
+ return ret;
+
+ if (buf[DA9063_DEV_ID_REG] != PMIC_CHIP_ID_DA9063) {
+ dev_err(da9063->dev,
+ "Invalid chip device ID: 0x%02x\n",
+ buf[DA9063_DEV_ID_REG]);
+ return -ENODEV;
+ }
+
+ dev_info(da9063->dev,
+ "Device detected (chip-ID: 0x%02X, var-ID: 0x%02X)\n",
+ buf[DA9063_DEV_ID_REG], buf[DA9063_VAR_ID_REG]);
+
+ da9063->variant_code =
+ (buf[DA9063_VAR_ID_REG] & DA9063_VARIANT_ID_MRC_MASK)
+ >> DA9063_VARIANT_ID_MRC_SHIFT;
+
+ 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 +184,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 +229,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 +271,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 +285,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 +311,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)
+ 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 2/2] mfd: da9063: Add support for latest DA silicon revision
2020-04-17 10:55 [PATCH v3 0/2] Resolve revision handling and add support for DA silicon Adam Thomson
2020-04-17 10:55 ` [PATCH v3 1/2] mfd: da9063: Fix revision handling to correctly select reg tables Adam Thomson
@ 2020-04-17 10:55 ` Adam Thomson
2020-06-15 12:33 ` [PATCH v3 0/2] Resolve revision handling and add support for DA silicon Adam Thomson
2 siblings, 0 replies; 9+ messages in thread
From: Adam Thomson @ 2020-04-17 10:55 UTC (permalink / raw)
To: Lee Jones; +Cc: linux-kernel, Support Opensource
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>
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
---
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 4815489..b8217ad 100644
--- a/drivers/mfd/da9063-i2c.c
+++ b/drivers/mfd/da9063-i2c.c
@@ -197,7 +197,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),
@@ -219,9 +219,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[] = {
@@ -241,7 +241,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),
@@ -263,9 +263,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[] = {
@@ -333,7 +388,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,
@@ -350,7 +413,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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [PATCH v3 0/2] Resolve revision handling and add support for DA silicon
2020-04-17 10:55 [PATCH v3 0/2] Resolve revision handling and add support for DA silicon Adam Thomson
2020-04-17 10:55 ` [PATCH v3 1/2] mfd: da9063: Fix revision handling to correctly select reg tables Adam Thomson
2020-04-17 10:55 ` [PATCH v3 2/2] mfd: da9063: Add support for latest DA silicon revision Adam Thomson
@ 2020-06-15 12:33 ` Adam Thomson
2 siblings, 0 replies; 9+ messages in thread
From: Adam Thomson @ 2020-06-15 12:33 UTC (permalink / raw)
To: Adam Thomson, Lee Jones; +Cc: linux-kernel, Support Opensource
On 17 April 2020 11:56, Adam Thomson wrote:
> 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.
Do I need to resend this patch, or will you still pick up the original mail
submission from April, Lee?
>
> v3:
> - Replaced magic numbers around I2C paged access code with enums and
> defines
> - Small style tidy ups as requested by Lee Jones
>
> v2:
> - Use raw I2C read access instead of a temporary regmap to interrogate chip and
> variant id registers
>
> 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 | 271 +++++++++++++++++++++++++++++++---
> -
> include/linux/mfd/da9063/core.h | 1 +
> include/linux/mfd/da9063/registers.h | 15 +-
> 4 files changed, 257 insertions(+), 61 deletions(-)
>
> --
> 1.9.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] mfd: da9063: Fix revision handling to correctly select reg tables
2020-04-17 10:55 ` [PATCH v3 1/2] mfd: da9063: Fix revision handling to correctly select reg tables Adam Thomson
@ 2020-06-18 10:15 ` Lee Jones
2020-06-18 10:57 ` Adam Thomson
0 siblings, 1 reply; 9+ messages in thread
From: Lee Jones @ 2020-06-18 10:15 UTC (permalink / raw)
To: Adam Thomson; +Cc: linux-kernel, Support Opensource
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v3 1/2] mfd: da9063: Fix revision handling to correctly select reg tables
2020-06-18 10:15 ` Lee Jones
@ 2020-06-18 10:57 ` Adam Thomson
2020-06-18 11:15 ` Lee Jones
0 siblings, 1 reply; 9+ messages in thread
From: Adam Thomson @ 2020-06-18 10:57 UTC (permalink / raw)
To: Lee Jones, Adam Thomson; +Cc: linux-kernel, Support Opensource
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@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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] mfd: da9063: Fix revision handling to correctly select reg tables
2020-06-18 10:57 ` Adam Thomson
@ 2020-06-18 11:15 ` Lee Jones
2020-06-18 12:48 ` Adam Thomson
0 siblings, 1 reply; 9+ messages in thread
From: Lee Jones @ 2020-06-18 11:15 UTC (permalink / raw)
To: Adam Thomson; +Cc: linux-kernel, Support Opensource
On Thu, 18 Jun 2020, Adam Thomson wrote:
> 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@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.
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.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v3 1/2] mfd: da9063: Fix revision handling to correctly select reg tables
2020-06-18 11:15 ` Lee Jones
@ 2020-06-18 12:48 ` Adam Thomson
2020-06-18 16:38 ` Lee Jones
0 siblings, 1 reply; 9+ messages in thread
From: Adam Thomson @ 2020-06-18 12:48 UTC (permalink / raw)
To: Lee Jones, Adam Thomson; +Cc: linux-kernel, Support Opensource
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] mfd: da9063: Fix revision handling to correctly select reg tables
2020-06-18 12:48 ` Adam Thomson
@ 2020-06-18 16:38 ` Lee Jones
0 siblings, 0 replies; 9+ messages in thread
From: Lee Jones @ 2020-06-18 16:38 UTC (permalink / raw)
To: Adam Thomson; +Cc: linux-kernel, Support Opensource
On Thu, 18 Jun 2020, Adam Thomson wrote:
> 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(-)
[...]
> > > > 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.
It would be very difficult to remember complete revision history for
every patch received. Especially with the lack of a provided
patch-level changelog. So I have to take each submission on its own
merits. This is particularly true for patch-sets ranging over 6
months or more.
The Regmap vs raw accesses decision could easily have gone either way,
so it's not surprising the other was considered during the review of
each submission. Both times were phrased as an "I wonder if" enquiry,
rather than a demand.
> > 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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-06-18 16:38 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-17 10:55 [PATCH v3 0/2] Resolve revision handling and add support for DA silicon Adam Thomson
2020-04-17 10:55 ` [PATCH v3 1/2] mfd: da9063: Fix revision handling to correctly select reg tables Adam Thomson
2020-06-18 10:15 ` Lee Jones
2020-06-18 10:57 ` Adam Thomson
2020-06-18 11:15 ` Lee Jones
2020-06-18 12:48 ` Adam Thomson
2020-06-18 16:38 ` Lee Jones
2020-04-17 10:55 ` [PATCH v3 2/2] mfd: da9063: Add support for latest DA silicon revision Adam Thomson
2020-06-15 12:33 ` [PATCH v3 0/2] Resolve revision handling and add support for DA silicon Adam Thomson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).