* [RESEND PATCH v2 0/2] Resolve revision handling and add support for DA silicon
@ 2020-04-06 8:54 Adam Thomson
2020-04-06 8:54 ` [RESEND PATCH v2 1/2] mfd: da9063: Fix revision handling to correctly select reg tables Adam Thomson
2020-04-06 8:54 ` [RESEND v2 PATCH 2/2] mfd: da9063: Add support for latest DA silicon revision Adam Thomson
0 siblings, 2 replies; 11+ messages in thread
From: Adam Thomson @ 2020-04-06 8:54 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.
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 | 249 +++++++++++++++++++++++++++++++----
include/linux/mfd/da9063/core.h | 1 +
include/linux/mfd/da9063/registers.h | 15 ++-
4 files changed, 235 insertions(+), 61 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [RESEND PATCH v2 1/2] mfd: da9063: Fix revision handling to correctly select reg tables
2020-04-06 8:54 [RESEND PATCH v2 0/2] Resolve revision handling and add support for DA silicon Adam Thomson
@ 2020-04-06 8:54 ` Adam Thomson
2020-04-16 7:59 ` Lee Jones
2020-04-06 8:54 ` [RESEND v2 PATCH 2/2] mfd: da9063: Add support for latest DA silicon revision Adam Thomson
1 sibling, 1 reply; 11+ messages in thread
From: Adam Thomson @ 2020-04-06 8:54 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 | 162 +++++++++++++++++++++++++++++++----
include/linux/mfd/da9063/registers.h | 15 ++--
3 files changed, 155 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..2654b49 100644
--- a/drivers/mfd/da9063-i2c.c
+++ b/drivers/mfd/da9063-i2c.c
@@ -22,12 +22,102 @@
#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.
+ */
+static int da9063_i2c_blockreg_read(struct i2c_client *client, u16 addr,
+ u8 *buf, int count)
+{
+ struct i2c_msg xfer[3];
+ u8 page_num, paged_addr;
+ u8 page_buf[2];
+ int ret;
+
+ /* Determine page info based on register address */
+ page_num = (addr / 0x100);
+ if (page_num > 1)
+ return -EINVAL;
+
+ paged_addr = (addr % 0x100) & 0xFF;
+ page_buf[0] = DA9063_REG_PAGE_CON;
+ page_buf[1] = (page_num << DA9063_I2C_PAGE_SEL_SHIFT) &
+ DA9063_REG_PAGE_MASK;
+
+ /* Write reg address, page selection */
+ xfer[0].addr = client->addr;
+ xfer[0].flags = 0;
+ xfer[0].len = 2;
+ xfer[0].buf = page_buf;
+
+ /* Select register address */
+ xfer[1].addr = client->addr;
+ xfer[1].flags = 0;
+ xfer[1].len = 1;
+ xfer[1].buf = &paged_addr;
+
+ /* Read data */
+ xfer[2].addr = client->addr;
+ xfer[2].flags = I2C_M_RD;
+ xfer[2].len = count;
+ xfer[2].buf = buf;
+
+ ret = i2c_transfer(client->adapter, xfer, 3);
+ if (ret == 3)
+ return 0;
+ else if (ret < 0)
+ return ret;
+ else
+ return -EIO;
+}
+
+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)
+{
+ int ret;
+ u8 buf[DA9063_CHIP_ID_REGS];
+
+ ret = da9063_i2c_blockreg_read(i2c, DA9063_REG_DEVICE_ID, buf,
+ DA9063_CHIP_ID_REGS);
+ if (ret < 0) {
+ dev_err(da9063->dev, "Cannot read chip id info.\n");
+ 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 +162,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 +207,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 +249,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 +263,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 +289,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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [RESEND v2 PATCH 2/2] mfd: da9063: Add support for latest DA silicon revision
2020-04-06 8:54 [RESEND PATCH v2 0/2] Resolve revision handling and add support for DA silicon Adam Thomson
2020-04-06 8:54 ` [RESEND PATCH v2 1/2] mfd: da9063: Fix revision handling to correctly select reg tables Adam Thomson
@ 2020-04-06 8:54 ` Adam Thomson
2020-04-16 8:06 ` Lee Jones
1 sibling, 1 reply; 11+ messages in thread
From: Adam Thomson @ 2020-04-06 8:54 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 2654b49..38452ae 100644
--- a/drivers/mfd/da9063-i2c.c
+++ b/drivers/mfd/da9063-i2c.c
@@ -175,7 +175,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),
@@ -197,9 +197,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[] = {
@@ -219,7 +219,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),
@@ -241,9 +241,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[] = {
@@ -311,7 +366,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,
@@ -328,7 +391,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] 11+ messages in thread
* Re: [RESEND PATCH v2 1/2] mfd: da9063: Fix revision handling to correctly select reg tables
2020-04-06 8:54 ` [RESEND PATCH v2 1/2] mfd: da9063: Fix revision handling to correctly select reg tables Adam Thomson
@ 2020-04-16 7:59 ` Lee Jones
2020-04-16 9:04 ` Adam Thomson
0 siblings, 1 reply; 11+ messages in thread
From: Lee Jones @ 2020-04-16 7:59 UTC (permalink / raw)
To: Adam Thomson; +Cc: linux-kernel, Support Opensource
On Mon, 06 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 | 162 +++++++++++++++++++++++++++++++----
> include/linux/mfd/da9063/registers.h | 15 ++--
> 3 files changed, 155 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..2654b49 100644
> --- a/drivers/mfd/da9063-i2c.c
> +++ b/drivers/mfd/da9063-i2c.c
> @@ -22,12 +22,102 @@
> #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.
> + */
> +static int da9063_i2c_blockreg_read(struct i2c_client *client, u16 addr,
> + u8 *buf, int count)
> +{
> + struct i2c_msg xfer[3];
> + u8 page_num, paged_addr;
> + u8 page_buf[2];
> + int ret;
> +
> + /* Determine page info based on register address */
> + page_num = (addr / 0x100);
Please define magic numbers.
> + if (page_num > 1)
Please define magic numbers.
> + return -EINVAL;
Do you want to fail silently here?
> + paged_addr = (addr % 0x100) & 0xFF;
> + page_buf[0] = DA9063_REG_PAGE_CON;
> + page_buf[1] = (page_num << DA9063_I2C_PAGE_SEL_SHIFT) &
> + DA9063_REG_PAGE_MASK;
> +
> + /* Write reg address, page selection */
> + xfer[0].addr = client->addr;
> + xfer[0].flags = 0;
> + xfer[0].len = 2;
> + xfer[0].buf = page_buf;
> +
> + /* Select register address */
> + xfer[1].addr = client->addr;
> + xfer[1].flags = 0;
> + xfer[1].len = 1;
> + xfer[1].buf = &paged_addr;
> +
> + /* Read data */
> + xfer[2].addr = client->addr;
> + xfer[2].flags = I2C_M_RD;
> + xfer[2].len = count;
> + xfer[2].buf = buf;
> +
> + ret = i2c_transfer(client->adapter, xfer, 3);
Why is this 3? 'count' and a NULL char?
> + if (ret == 3)
> + return 0;
> + else if (ret < 0)
> + return ret;
> + else
> + return -EIO;
I think the following makes it slightly clearer.
if (ret < 0)
return ret;
if (ret == 3)
return 0;
else
return -EIO;
> +}
> +
> +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)
> +{
> + int ret;
> + u8 buf[DA9063_CHIP_ID_REGS];
Really small nit: Could you reverse these please.
> + ret = da9063_i2c_blockreg_read(i2c, DA9063_REG_DEVICE_ID, buf,
> + DA9063_CHIP_ID_REGS);
> + if (ret < 0) {
if (ret)
Or better yet, as this is a read function, you could just return
i2c_transfer() and do the appropriate error checking here *instead*.
> + dev_err(da9063->dev, "Cannot read chip id info.\n");
> + return ret;
> + }
> +
> + if (buf[DA9063_DEV_ID_REG] != PMIC_CHIP_ID_DA9063) {
> + dev_err(da9063->dev,
> + "Invalid chip device id: 0x%02x\n",
s/id/ID/
> + 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 +162,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 +207,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 +249,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 +263,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 +289,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)
Is a positive return value valid?
If not: 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 */
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RESEND v2 PATCH 2/2] mfd: da9063: Add support for latest DA silicon revision
2020-04-06 8:54 ` [RESEND v2 PATCH 2/2] mfd: da9063: Add support for latest DA silicon revision Adam Thomson
@ 2020-04-16 8:06 ` Lee Jones
2020-04-16 9:06 ` Adam Thomson
0 siblings, 1 reply; 11+ messages in thread
From: Lee Jones @ 2020-04-16 8:06 UTC (permalink / raw)
To: Adam Thomson; +Cc: linux-kernel, Support Opensource
On Mon, 06 Apr 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>
> 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(-)
Can this patch be applied on its own?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [RESEND PATCH v2 1/2] mfd: da9063: Fix revision handling to correctly select reg tables
2020-04-16 7:59 ` Lee Jones
@ 2020-04-16 9:04 ` Adam Thomson
2020-04-17 9:24 ` Lee Jones
0 siblings, 1 reply; 11+ messages in thread
From: Adam Thomson @ 2020-04-16 9:04 UTC (permalink / raw)
To: Lee Jones, Adam Thomson; +Cc: linux-kernel, Support Opensource
On 16 April 2020 09:00, Lee Jones wrote:
> > +/*
> > + * 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.
> > + */
> > +static int da9063_i2c_blockreg_read(struct i2c_client *client, u16 addr,
> > + u8 *buf, int count)
> > +{
> > + struct i2c_msg xfer[3];
> > + u8 page_num, paged_addr;
> > + u8 page_buf[2];
> > + int ret;
> > +
> > + /* Determine page info based on register address */
> > + page_num = (addr / 0x100);
>
> Please define magic numbers.
>
> > + if (page_num > 1)
>
> Please define magic numbers.
I was going to but decided against it given the minimal use. Easy enough to
change though.
>
> > + return -EINVAL;
>
> Do you want to fail silently here?
Well an error message is printed in the calling code, so didn't feel like it
was necessary to have additional debug here. Felt like bloat.
>
> > + paged_addr = (addr % 0x100) & 0xFF;
> > + page_buf[0] = DA9063_REG_PAGE_CON;
> > + page_buf[1] = (page_num << DA9063_I2C_PAGE_SEL_SHIFT) &
> > + DA9063_REG_PAGE_MASK;
> > +
> > + /* Write reg address, page selection */
> > + xfer[0].addr = client->addr;
> > + xfer[0].flags = 0;
> > + xfer[0].len = 2;
> > + xfer[0].buf = page_buf;
> > +
> > + /* Select register address */
> > + xfer[1].addr = client->addr;
> > + xfer[1].flags = 0;
> > + xfer[1].len = 1;
> > + xfer[1].buf = &paged_addr;
> > +
> > + /* Read data */
> > + xfer[2].addr = client->addr;
> > + xfer[2].flags = I2C_M_RD;
> > + xfer[2].len = count;
> > + xfer[2].buf = buf;
> > +
> > + ret = i2c_transfer(client->adapter, xfer, 3);
>
> Why is this 3? 'count' and a NULL char?
Well there are 3 messages defined above so I want to process all of them. One to
set the page register to the page we want to read from, one to select the
register we want to read from in that page and then finally the read back of
the chip id and revision/variant info.
>
> > + if (ret == 3)
> > + return 0;
> > + else if (ret < 0)
> > + return ret;
> > + else
> > + return -EIO;
>
> I think the following makes it slightly clearer.
>
> if (ret < 0)
> return ret;
>
> if (ret == 3)
> return 0;
> else
> return -EIO;
>
Ok. Don't think it makes much of a difference but don't mind really. I can add a
#define for the number of messages to be sent which will clarify this slightly
anyway.
> > +}
> > +
> > +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)
> > +{
> > + int ret;
> > + u8 buf[DA9063_CHIP_ID_REGS];
>
> Really small nit: Could you reverse these please.
Yep, agreed.
>
> > + ret = da9063_i2c_blockreg_read(i2c, DA9063_REG_DEVICE_ID, buf,
> > + DA9063_CHIP_ID_REGS);
> > + if (ret < 0) {
>
> if (ret)
>
> Or better yet, as this is a read function, you could just return
> i2c_transfer() and do the appropriate error checking here *instead*.
I think given that the function handles all of the I2C specific stuff I'd prefer
it be kept there. Logically that to me makes more sense. Can change this to
'if (ret)'
>
> > + dev_err(da9063->dev, "Cannot read chip id info.\n");
> > + return ret;
> > + }
> > +
> > + if (buf[DA9063_DEV_ID_REG] != PMIC_CHIP_ID_DA9063) {
> > + dev_err(da9063->dev,
> > + "Invalid chip device id: 0x%02x\n",
>
> s/id/ID/
yep, will update.
> > @@ -199,18 +289,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)
>
> Is a positive return value valid?
>
> If not: if (ret)
>
Yep, think this can be just 'if (ret)'
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [RESEND v2 PATCH 2/2] mfd: da9063: Add support for latest DA silicon revision
2020-04-16 8:06 ` Lee Jones
@ 2020-04-16 9:06 ` Adam Thomson
0 siblings, 0 replies; 11+ messages in thread
From: Adam Thomson @ 2020-04-16 9:06 UTC (permalink / raw)
To: Lee Jones, Adam Thomson; +Cc: linux-kernel, Support Opensource
On 16 April 2020 09:07, Lee Jones wrote:
> On Mon, 06 Apr 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>
> > 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(-)
>
> Can this patch be applied on its own?
Sadly not. Relies on a new switch statement added by the first patch.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RESEND PATCH v2 1/2] mfd: da9063: Fix revision handling to correctly select reg tables
2020-04-16 9:04 ` Adam Thomson
@ 2020-04-17 9:24 ` Lee Jones
2020-04-17 9:51 ` Adam Thomson
0 siblings, 1 reply; 11+ messages in thread
From: Lee Jones @ 2020-04-17 9:24 UTC (permalink / raw)
To: Adam Thomson; +Cc: linux-kernel, Support Opensource
On Thu, 16 Apr 2020, Adam Thomson wrote:
> On 16 April 2020 09:00, Lee Jones wrote:
>
> > > +/*
> > > + * 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.
> > > + */
> > > +static int da9063_i2c_blockreg_read(struct i2c_client *client, u16 addr,
> > > + u8 *buf, int count)
> > > +{
> > > + struct i2c_msg xfer[3];
> > > + u8 page_num, paged_addr;
> > > + u8 page_buf[2];
> > > + int ret;
> > > +
> > > + /* Determine page info based on register address */
> > > + page_num = (addr / 0x100);
> >
> > Please define magic numbers.
> >
> > > + if (page_num > 1)
> >
> > Please define magic numbers.
>
> I was going to but decided against it given the minimal use. Easy enough to
> change though.
It's purely for readability purposes.
> > > + return -EINVAL;
> >
> > Do you want to fail silently here?
>
> Well an error message is printed in the calling code, so didn't feel like it
> was necessary to have additional debug here. Felt like bloat.
As a user, I would prefer a more specific reason.
Thus, I would provide an error message here and omit the generic one.
> > > + paged_addr = (addr % 0x100) & 0xFF;
> > > + page_buf[0] = DA9063_REG_PAGE_CON;
> > > + page_buf[1] = (page_num << DA9063_I2C_PAGE_SEL_SHIFT) &
> > > + DA9063_REG_PAGE_MASK;
> > > +
> > > + /* Write reg address, page selection */
> > > + xfer[0].addr = client->addr;
> > > + xfer[0].flags = 0;
> > > + xfer[0].len = 2;
> > > + xfer[0].buf = page_buf;
> > > +
> > > + /* Select register address */
> > > + xfer[1].addr = client->addr;
> > > + xfer[1].flags = 0;
> > > + xfer[1].len = 1;
> > > + xfer[1].buf = &paged_addr;
> > > +
> > > + /* Read data */
> > > + xfer[2].addr = client->addr;
> > > + xfer[2].flags = I2C_M_RD;
> > > + xfer[2].len = count;
> > > + xfer[2].buf = buf;
> > > +
> > > + ret = i2c_transfer(client->adapter, xfer, 3);
> >
> > Why is this 3? 'count' and a NULL char?
>
> Well there are 3 messages defined above so I want to process all of them. One to
> set the page register to the page we want to read from, one to select the
> register we want to read from in that page and then finally the read back of
> the chip id and revision/variant info.
I see. Thank you for the explanation.
> > > + if (ret == 3)
> > > + return 0;
> > > + else if (ret < 0)
> > > + return ret;
> > > + else
> > > + return -EIO;
> >
> > I think the following makes it slightly clearer.
> >
> > if (ret < 0)
> > return ret;
> >
> > if (ret == 3)
> > return 0;
> > else
> > return -EIO;
> >
>
> Ok. Don't think it makes much of a difference but don't mind really. I can add a
> #define for the number of messages to be sent which will clarify this slightly
> anyway.
Yes, I think that would be good.
> > > +}
> > > +
> > > +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)
> > > +{
> > > + int ret;
> > > + u8 buf[DA9063_CHIP_ID_REGS];
> >
> > Really small nit: Could you reverse these please.
>
> Yep, agreed.
>
> >
> > > + ret = da9063_i2c_blockreg_read(i2c, DA9063_REG_DEVICE_ID, buf,
> > > + DA9063_CHIP_ID_REGS);
> > > + if (ret < 0) {
> >
> > if (ret)
> >
> > Or better yet, as this is a read function, you could just return
> > i2c_transfer() and do the appropriate error checking here *instead*.
>
> I think given that the function handles all of the I2C specific stuff I'd prefer
> it be kept there. Logically that to me makes more sense. Can change this to
> 'if (ret)'
Yes, not that I understand the message length (3) has more do to with
the I2C interactions (rather than a derisive of 'count'), it makes
sense to handle that inside the function.
However, it does seem odd to handle the return value of a *_read()
function in this way. They usually return the number of bytes read,
which in this case would be DA9063_CHIP_ID_REGS (count), right?
[...]
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [RESEND PATCH v2 1/2] mfd: da9063: Fix revision handling to correctly select reg tables
2020-04-17 9:24 ` Lee Jones
@ 2020-04-17 9:51 ` Adam Thomson
2020-04-20 7:31 ` Lee Jones
0 siblings, 1 reply; 11+ messages in thread
From: Adam Thomson @ 2020-04-17 9:51 UTC (permalink / raw)
To: Lee Jones, Adam Thomson; +Cc: linux-kernel, Support Opensource
On 17 April 2020 10:24, Lee Jones wrote:
> > > > + return -EINVAL;
> > >
> > > Do you want to fail silently here?
> >
> > Well an error message is printed in the calling code, so didn't feel like it
> > was necessary to have additional debug here. Felt like bloat.
>
> As a user, I would prefer a more specific reason.
>
> Thus, I would provide an error message here and omit the generic one.
I can update although I'll of course then need to do similar messages for the
other error legs of this function. FWIW, as this is only being called once in
the same file this error leg of code currently can never occur.
> > > > +}
> > > > +
> > > > +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)
> > > > +{
> > > > + int ret;
> > > > + u8 buf[DA9063_CHIP_ID_REGS];
> > >
> > > Really small nit: Could you reverse these please.
> >
> > Yep, agreed.
> >
> > >
> > > > + ret = da9063_i2c_blockreg_read(i2c, DA9063_REG_DEVICE_ID, buf,
> > > > + DA9063_CHIP_ID_REGS);
> > > > + if (ret < 0) {
> > >
> > > if (ret)
> > >
> > > Or better yet, as this is a read function, you could just return
> > > i2c_transfer() and do the appropriate error checking here *instead*.
> >
> > I think given that the function handles all of the I2C specific stuff I'd prefer
> > it be kept there. Logically that to me makes more sense. Can change this to
> > 'if (ret)'
>
> Yes, not that I understand the message length (3) has more do to with
> the I2C interactions (rather than a derisive of 'count'), it makes
> sense to handle that inside the function.
>
> However, it does seem odd to handle the return value of a *_read()
> function in this way. They usually return the number of bytes read,
> which in this case would be DA9063_CHIP_ID_REGS (count), right?
Well regmap_bulk_read and regmap_read return 0 for success and negative for
failure so I'd disagree on this part.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RESEND PATCH v2 1/2] mfd: da9063: Fix revision handling to correctly select reg tables
2020-04-17 9:51 ` Adam Thomson
@ 2020-04-20 7:31 ` Lee Jones
2020-04-20 8:14 ` Adam Thomson
0 siblings, 1 reply; 11+ messages in thread
From: Lee Jones @ 2020-04-20 7:31 UTC (permalink / raw)
To: Adam Thomson; +Cc: linux-kernel, Support Opensource
On Fri, 17 Apr 2020, Adam Thomson wrote:
> On 17 April 2020 10:24, Lee Jones wrote:
>
> > > > > + return -EINVAL;
> > > >
> > > > Do you want to fail silently here?
> > >
> > > Well an error message is printed in the calling code, so didn't feel like it
> > > was necessary to have additional debug here. Felt like bloat.
> >
> > As a user, I would prefer a more specific reason.
> >
> > Thus, I would provide an error message here and omit the generic one.
>
> I can update although I'll of course then need to do similar messages for the
> other error legs of this function. FWIW, as this is only being called once in
> the same file this error leg of code currently can never occur.
As a tiny improvement, it's not a deal breaker. If it's too much
work, you can either submit a subsequent patch or omit it completely.
> > > > > +}
> > > > > +
> > > > > +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)
> > > > > +{
> > > > > + int ret;
> > > > > + u8 buf[DA9063_CHIP_ID_REGS];
> > > >
> > > > Really small nit: Could you reverse these please.
> > >
> > > Yep, agreed.
> > >
> > > >
> > > > > + ret = da9063_i2c_blockreg_read(i2c, DA9063_REG_DEVICE_ID, buf,
> > > > > + DA9063_CHIP_ID_REGS);
> > > > > + if (ret < 0) {
> > > >
> > > > if (ret)
> > > >
> > > > Or better yet, as this is a read function, you could just return
> > > > i2c_transfer() and do the appropriate error checking here *instead*.
> > >
> > > I think given that the function handles all of the I2C specific stuff I'd prefer
> > > it be kept there. Logically that to me makes more sense. Can change this to
> > > 'if (ret)'
> >
> > Yes, not that I understand the message length (3) has more do to with
> > the I2C interactions (rather than a derisive of 'count'), it makes
> > sense to handle that inside the function.
> >
> > However, it does seem odd to handle the return value of a *_read()
> > function in this way. They usually return the number of bytes read,
> > which in this case would be DA9063_CHIP_ID_REGS (count), right?
>
> Well regmap_bulk_read and regmap_read return 0 for success and negative for
> failure so I'd disagree on this part.
Fair enough. :)
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [RESEND PATCH v2 1/2] mfd: da9063: Fix revision handling to correctly select reg tables
2020-04-20 7:31 ` Lee Jones
@ 2020-04-20 8:14 ` Adam Thomson
0 siblings, 0 replies; 11+ messages in thread
From: Adam Thomson @ 2020-04-20 8:14 UTC (permalink / raw)
To: Lee Jones, Adam Thomson; +Cc: linux-kernel, Support Opensource
On 20 April 2020 08:32, Lee Jones wrote:
> >
> > > > > > + return -EINVAL;
> > > > >
> > > > > Do you want to fail silently here?
> > > >
> > > > Well an error message is printed in the calling code, so didn't feel like it
> > > > was necessary to have additional debug here. Felt like bloat.
> > >
> > > As a user, I would prefer a more specific reason.
> > >
> > > Thus, I would provide an error message here and omit the generic one.
> >
> > I can update although I'll of course then need to do similar messages for the
> > other error legs of this function. FWIW, as this is only being called once in
> > the same file this error leg of code currently can never occur.
>
> As a tiny improvement, it's not a deal breaker. If it's too much
> work, you can either submit a subsequent patch or omit it completely.
Taken care of in the v3 patch set I submitted Friday. Hopefully that covers
everything. :)
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-04-20 8:14 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-06 8:54 [RESEND PATCH v2 0/2] Resolve revision handling and add support for DA silicon Adam Thomson
2020-04-06 8:54 ` [RESEND PATCH v2 1/2] mfd: da9063: Fix revision handling to correctly select reg tables Adam Thomson
2020-04-16 7:59 ` Lee Jones
2020-04-16 9:04 ` Adam Thomson
2020-04-17 9:24 ` Lee Jones
2020-04-17 9:51 ` Adam Thomson
2020-04-20 7:31 ` Lee Jones
2020-04-20 8:14 ` Adam Thomson
2020-04-06 8:54 ` [RESEND v2 PATCH 2/2] mfd: da9063: Add support for latest DA silicon revision Adam Thomson
2020-04-16 8:06 ` Lee Jones
2020-04-16 9:06 ` 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).