In case mfd_add_devices() fails, da9063_irq_exit() is not called to
undo the IRQchip setup done by da9063_irq_init(). Fix this by adding
the missing fail path.
Signed-off-by: Marek Vasut <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Lee Jones <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Steve Twiss <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: [email protected]
---
V3: New patch
---
drivers/mfd/da9063-core.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
index 6c2870d4e754..8226ebd8b96d 100644
--- a/drivers/mfd/da9063-core.c
+++ b/drivers/mfd/da9063-core.c
@@ -229,10 +229,16 @@ int da9063_device_init(struct da9063 *da9063, unsigned int irq)
ret = mfd_add_devices(da9063->dev, -1, da9063_devs,
ARRAY_SIZE(da9063_devs), NULL, da9063->irq_base,
NULL);
- if (ret)
+ if (ret) {
dev_err(da9063->dev, "Cannot add MFD cells\n");
+ goto err_irq_exit;
+ }
return ret;
+
+err_irq_exit:
+ da9063_irq_exit(da9063);
+ return ret;
}
void da9063_device_exit(struct da9063 *da9063)
--
2.16.2
Add support for DA9063L, which is a reduced variant of the DA9063
with less regulators and without RTC.
Signed-off-by: Marek Vasut <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Lee Jones <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Steve Twiss <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: [email protected]
Acked-by: Mark Brown <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
---
V2: No change
V3: No change
---
drivers/mfd/da9063-i2c.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c
index f0d92a37df6b..74a3b10a7208 100644
--- a/drivers/mfd/da9063-i2c.c
+++ b/drivers/mfd/da9063-i2c.c
@@ -319,6 +319,7 @@ static struct regmap_config da9063_regmap_config = {
static const struct of_device_id da9063_dt_ids[] = {
{ .compatible = "dlg,da9063", },
+ { .compatible = "dlg,da9063l", },
{ }
};
MODULE_DEVICE_TABLE(of, da9063_dt_ids);
@@ -373,6 +374,7 @@ static int da9063_i2c_remove(struct i2c_client *i2c)
static const struct i2c_device_id da9063_i2c_id[] = {
{ "da9063", PMIC_TYPE_DA9063 },
+ { "da9063l", PMIC_TYPE_DA9063L },
{},
};
MODULE_DEVICE_TABLE(i2c, da9063_i2c_id);
--
2.16.2
Convert the regmap_irq table to use REGMAP_IRQ_REG().
Signed-off-by: Marek Vasut <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Lee Jones <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Steve Twiss <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: [email protected]
---
V3: New patch
Note: A sed oneliner was used:
sed -i "/\[DA9063_IRQ_/ {N;N;N;s/\n//g;s/.*\[\(DA9063_IRQ_[^]]\+\)].*reg_offset = \([^,]\+\),.* = \([^,]\+\),.*/\tREGMAP_IRQ_REG(\1, \2, \3),/}" drivers/mfd/da9063-irq.c
---
drivers/mfd/da9063-irq.c | 145 ++++++++++-------------------------------------
1 file changed, 29 insertions(+), 116 deletions(-)
diff --git a/drivers/mfd/da9063-irq.c b/drivers/mfd/da9063-irq.c
index 207bbfe55449..5b406ecfc14a 100644
--- a/drivers/mfd/da9063-irq.c
+++ b/drivers/mfd/da9063-irq.c
@@ -28,125 +28,38 @@
static const struct regmap_irq da9063_irqs[] = {
/* DA9063 event A register */
- [DA9063_IRQ_ONKEY] = {
- .reg_offset = DA9063_REG_EVENT_A_OFFSET,
- .mask = DA9063_M_ONKEY,
- },
- [DA9063_IRQ_ALARM] = {
- .reg_offset = DA9063_REG_EVENT_A_OFFSET,
- .mask = DA9063_M_ALARM,
- },
- [DA9063_IRQ_TICK] = {
- .reg_offset = DA9063_REG_EVENT_A_OFFSET,
- .mask = DA9063_M_TICK,
- },
- [DA9063_IRQ_ADC_RDY] = {
- .reg_offset = DA9063_REG_EVENT_A_OFFSET,
- .mask = DA9063_M_ADC_RDY,
- },
- [DA9063_IRQ_SEQ_RDY] = {
- .reg_offset = DA9063_REG_EVENT_A_OFFSET,
- .mask = DA9063_M_SEQ_RDY,
- },
+ REGMAP_IRQ_REG(DA9063_IRQ_ONKEY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ONKEY),
+ REGMAP_IRQ_REG(DA9063_IRQ_ALARM, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ALARM),
+ REGMAP_IRQ_REG(DA9063_IRQ_TICK, DA9063_REG_EVENT_A_OFFSET, DA9063_M_TICK),
+ REGMAP_IRQ_REG(DA9063_IRQ_ADC_RDY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ADC_RDY),
+ REGMAP_IRQ_REG(DA9063_IRQ_SEQ_RDY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_SEQ_RDY),
/* DA9063 event B register */
- [DA9063_IRQ_WAKE] = {
- .reg_offset = DA9063_REG_EVENT_B_OFFSET,
- .mask = DA9063_M_WAKE,
- },
- [DA9063_IRQ_TEMP] = {
- .reg_offset = DA9063_REG_EVENT_B_OFFSET,
- .mask = DA9063_M_TEMP,
- },
- [DA9063_IRQ_COMP_1V2] = {
- .reg_offset = DA9063_REG_EVENT_B_OFFSET,
- .mask = DA9063_M_COMP_1V2,
- },
- [DA9063_IRQ_LDO_LIM] = {
- .reg_offset = DA9063_REG_EVENT_B_OFFSET,
- .mask = DA9063_M_LDO_LIM,
- },
- [DA9063_IRQ_REG_UVOV] = {
- .reg_offset = DA9063_REG_EVENT_B_OFFSET,
- .mask = DA9063_M_UVOV,
- },
- [DA9063_IRQ_DVC_RDY] = {
- .reg_offset = DA9063_REG_EVENT_B_OFFSET,
- .mask = DA9063_M_DVC_RDY,
- },
- [DA9063_IRQ_VDD_MON] = {
- .reg_offset = DA9063_REG_EVENT_B_OFFSET,
- .mask = DA9063_M_VDD_MON,
- },
- [DA9063_IRQ_WARN] = {
- .reg_offset = DA9063_REG_EVENT_B_OFFSET,
- .mask = DA9063_M_VDD_WARN,
- },
+ REGMAP_IRQ_REG(DA9063_IRQ_WAKE, DA9063_REG_EVENT_B_OFFSET, DA9063_M_WAKE),
+ REGMAP_IRQ_REG(DA9063_IRQ_TEMP, DA9063_REG_EVENT_B_OFFSET, DA9063_M_TEMP),
+ REGMAP_IRQ_REG(DA9063_IRQ_COMP_1V2, DA9063_REG_EVENT_B_OFFSET, DA9063_M_COMP_1V2),
+ REGMAP_IRQ_REG(DA9063_IRQ_LDO_LIM, DA9063_REG_EVENT_B_OFFSET, DA9063_M_LDO_LIM),
+ REGMAP_IRQ_REG(DA9063_IRQ_REG_UVOV, DA9063_REG_EVENT_B_OFFSET, DA9063_M_UVOV),
+ REGMAP_IRQ_REG(DA9063_IRQ_DVC_RDY, DA9063_REG_EVENT_B_OFFSET, DA9063_M_DVC_RDY),
+ REGMAP_IRQ_REG(DA9063_IRQ_VDD_MON, DA9063_REG_EVENT_B_OFFSET, DA9063_M_VDD_MON),
+ REGMAP_IRQ_REG(DA9063_IRQ_WARN, DA9063_REG_EVENT_B_OFFSET, DA9063_M_VDD_WARN),
/* DA9063 event C register */
- [DA9063_IRQ_GPI0] = {
- .reg_offset = DA9063_REG_EVENT_C_OFFSET,
- .mask = DA9063_M_GPI0,
- },
- [DA9063_IRQ_GPI1] = {
- .reg_offset = DA9063_REG_EVENT_C_OFFSET,
- .mask = DA9063_M_GPI1,
- },
- [DA9063_IRQ_GPI2] = {
- .reg_offset = DA9063_REG_EVENT_C_OFFSET,
- .mask = DA9063_M_GPI2,
- },
- [DA9063_IRQ_GPI3] = {
- .reg_offset = DA9063_REG_EVENT_C_OFFSET,
- .mask = DA9063_M_GPI3,
- },
- [DA9063_IRQ_GPI4] = {
- .reg_offset = DA9063_REG_EVENT_C_OFFSET,
- .mask = DA9063_M_GPI4,
- },
- [DA9063_IRQ_GPI5] = {
- .reg_offset = DA9063_REG_EVENT_C_OFFSET,
- .mask = DA9063_M_GPI5,
- },
- [DA9063_IRQ_GPI6] = {
- .reg_offset = DA9063_REG_EVENT_C_OFFSET,
- .mask = DA9063_M_GPI6,
- },
- [DA9063_IRQ_GPI7] = {
- .reg_offset = DA9063_REG_EVENT_C_OFFSET,
- .mask = DA9063_M_GPI7,
- },
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI0, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI0),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI1, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI1),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI2, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI2),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI3, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI3),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI4, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI4),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI5, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI5),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI6, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI6),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI7, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI7),
/* DA9063 event D register */
- [DA9063_IRQ_GPI8] = {
- .reg_offset = DA9063_REG_EVENT_D_OFFSET,
- .mask = DA9063_M_GPI8,
- },
- [DA9063_IRQ_GPI9] = {
- .reg_offset = DA9063_REG_EVENT_D_OFFSET,
- .mask = DA9063_M_GPI9,
- },
- [DA9063_IRQ_GPI10] = {
- .reg_offset = DA9063_REG_EVENT_D_OFFSET,
- .mask = DA9063_M_GPI10,
- },
- [DA9063_IRQ_GPI11] = {
- .reg_offset = DA9063_REG_EVENT_D_OFFSET,
- .mask = DA9063_M_GPI11,
- },
- [DA9063_IRQ_GPI12] = {
- .reg_offset = DA9063_REG_EVENT_D_OFFSET,
- .mask = DA9063_M_GPI12,
- },
- [DA9063_IRQ_GPI13] = {
- .reg_offset = DA9063_REG_EVENT_D_OFFSET,
- .mask = DA9063_M_GPI13,
- },
- [DA9063_IRQ_GPI14] = {
- .reg_offset = DA9063_REG_EVENT_D_OFFSET,
- .mask = DA9063_M_GPI14,
- },
- [DA9063_IRQ_GPI15] = {
- .reg_offset = DA9063_REG_EVENT_D_OFFSET,
- .mask = DA9063_M_GPI15,
- },
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI8, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI8),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI9, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI9),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI10, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI10),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI11, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI11),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI12, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI12),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI13, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI13),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI14, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI14),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI15, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI15),
};
static const struct regmap_irq_chip da9063_irq_chip = {
--
2.16.2
Move the LDOs present only on DA9063 at the end of the list, so that
the DA9063L can simply indicate less LDOs and still share the list of
regulators with DA9063.
Signed-off-by: Marek Vasut <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Lee Jones <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Steve Twiss <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: [email protected]
Acked-by: Mark Brown <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
---
V2: No change
V3: No change
---
drivers/regulator/da9063-regulator.c | 76 +++++++++++++++++++++---------------
1 file changed, 45 insertions(+), 31 deletions(-)
diff --git a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-regulator.c
index eeb0d431dda1..0c09e73e0b23 100644
--- a/drivers/regulator/da9063-regulator.c
+++ b/drivers/regulator/da9063-regulator.c
@@ -529,6 +529,32 @@ static const struct da9063_regulator_info da9063_regulator_info[] = {
.ilimit = BFIELD(DA9063_REG_BUCK_ILIM_A,
DA9063_BMEM_ILIM_MASK),
},
+ {
+ DA9063_LDO(DA9063, LDO3, 900, 20, 3440),
+ .suspend = BFIELD(DA9063_REG_DVC_1, DA9063_VLDO3_SEL),
+ .oc_event = BFIELD(DA9063_REG_STATUS_D, DA9063_LDO3_LIM),
+ },
+ {
+ DA9063_LDO(DA9063, LDO7, 900, 50, 3600),
+ .suspend = BFIELD(DA9063_REG_LDO7_CONT, DA9063_VLDO7_SEL),
+ .oc_event = BFIELD(DA9063_REG_STATUS_D, DA9063_LDO7_LIM),
+ },
+ {
+ DA9063_LDO(DA9063, LDO8, 900, 50, 3600),
+ .suspend = BFIELD(DA9063_REG_LDO8_CONT, DA9063_VLDO8_SEL),
+ .oc_event = BFIELD(DA9063_REG_STATUS_D, DA9063_LDO8_LIM),
+ },
+ {
+ DA9063_LDO(DA9063, LDO9, 950, 50, 3600),
+ .suspend = BFIELD(DA9063_REG_LDO9_CONT, DA9063_VLDO9_SEL),
+ },
+ {
+ DA9063_LDO(DA9063, LDO11, 900, 50, 3600),
+ .suspend = BFIELD(DA9063_REG_LDO11_CONT, DA9063_VLDO11_SEL),
+ .oc_event = BFIELD(DA9063_REG_STATUS_D, DA9063_LDO11_LIM),
+ },
+
+ /* The following LDOs are present only on DA9063, not on DA9063L */
{
DA9063_LDO(DA9063, LDO1, 600, 20, 1860),
.suspend = BFIELD(DA9063_REG_DVC_1, DA9063_VLDO1_SEL),
@@ -537,11 +563,6 @@ static const struct da9063_regulator_info da9063_regulator_info[] = {
DA9063_LDO(DA9063, LDO2, 600, 20, 1860),
.suspend = BFIELD(DA9063_REG_DVC_1, DA9063_VLDO2_SEL),
},
- {
- DA9063_LDO(DA9063, LDO3, 900, 20, 3440),
- .suspend = BFIELD(DA9063_REG_DVC_1, DA9063_VLDO3_SEL),
- .oc_event = BFIELD(DA9063_REG_STATUS_D, DA9063_LDO3_LIM),
- },
{
DA9063_LDO(DA9063, LDO4, 900, 20, 3440),
.suspend = BFIELD(DA9063_REG_DVC_2, DA9063_VLDO4_SEL),
@@ -555,29 +576,11 @@ static const struct da9063_regulator_info da9063_regulator_info[] = {
DA9063_LDO(DA9063, LDO6, 900, 50, 3600),
.suspend = BFIELD(DA9063_REG_LDO6_CONT, DA9063_VLDO6_SEL),
},
- {
- DA9063_LDO(DA9063, LDO7, 900, 50, 3600),
- .suspend = BFIELD(DA9063_REG_LDO7_CONT, DA9063_VLDO7_SEL),
- .oc_event = BFIELD(DA9063_REG_STATUS_D, DA9063_LDO7_LIM),
- },
- {
- DA9063_LDO(DA9063, LDO8, 900, 50, 3600),
- .suspend = BFIELD(DA9063_REG_LDO8_CONT, DA9063_VLDO8_SEL),
- .oc_event = BFIELD(DA9063_REG_STATUS_D, DA9063_LDO8_LIM),
- },
- {
- DA9063_LDO(DA9063, LDO9, 950, 50, 3600),
- .suspend = BFIELD(DA9063_REG_LDO9_CONT, DA9063_VLDO9_SEL),
- },
+
{
DA9063_LDO(DA9063, LDO10, 900, 50, 3600),
.suspend = BFIELD(DA9063_REG_LDO10_CONT, DA9063_VLDO10_SEL),
},
- {
- DA9063_LDO(DA9063, LDO11, 900, 50, 3600),
- .suspend = BFIELD(DA9063_REG_LDO11_CONT, DA9063_VLDO11_SEL),
- .oc_event = BFIELD(DA9063_REG_STATUS_D, DA9063_LDO11_LIM),
- },
};
/* Link chip model with regulators info table */
@@ -587,6 +590,11 @@ static struct da9063_dev_model regulators_models[] = {
.n_regulators = ARRAY_SIZE(da9063_regulator_info),
.type = PMIC_TYPE_DA9063,
},
+ {
+ .regulator_info = da9063_regulator_info,
+ .n_regulators = ARRAY_SIZE(da9063_regulator_info) - 6,
+ .type = PMIC_TYPE_DA9063L,
+ },
{ }
};
@@ -641,28 +649,34 @@ static struct of_regulator_match da9063_matches[] = {
[DA9063_ID_BPERI] = { .name = "bperi", },
[DA9063_ID_BCORES_MERGED] = { .name = "bcores-merged" },
[DA9063_ID_BMEM_BIO_MERGED] = { .name = "bmem-bio-merged", },
+ [DA9063_ID_LDO3] = { .name = "ldo3", },
+ [DA9063_ID_LDO7] = { .name = "ldo7", },
+ [DA9063_ID_LDO8] = { .name = "ldo8", },
+ [DA9063_ID_LDO9] = { .name = "ldo9", },
+ [DA9063_ID_LDO11] = { .name = "ldo11", },
+ /* The following LDOs are present only on DA9063, not on DA9063L */
[DA9063_ID_LDO1] = { .name = "ldo1", },
[DA9063_ID_LDO2] = { .name = "ldo2", },
- [DA9063_ID_LDO3] = { .name = "ldo3", },
[DA9063_ID_LDO4] = { .name = "ldo4", },
[DA9063_ID_LDO5] = { .name = "ldo5", },
[DA9063_ID_LDO6] = { .name = "ldo6", },
- [DA9063_ID_LDO7] = { .name = "ldo7", },
- [DA9063_ID_LDO8] = { .name = "ldo8", },
- [DA9063_ID_LDO9] = { .name = "ldo9", },
[DA9063_ID_LDO10] = { .name = "ldo10", },
- [DA9063_ID_LDO11] = { .name = "ldo11", },
};
static struct da9063_regulators_pdata *da9063_parse_regulators_dt(
struct platform_device *pdev,
struct of_regulator_match **da9063_reg_matches)
{
+ struct da9063 *da9063 = dev_get_drvdata(pdev->dev.parent);
struct da9063_regulators_pdata *pdata;
struct da9063_regulator_data *rdata;
struct device_node *node;
+ int da9063_matches_len = ARRAY_SIZE(da9063_matches);
int i, n, num;
+ if (da9063->type == PMIC_TYPE_DA9063L)
+ da9063_matches_len -= 6;
+
node = of_get_child_by_name(pdev->dev.parent->of_node, "regulators");
if (!node) {
dev_err(&pdev->dev, "Regulators device node not found\n");
@@ -670,7 +684,7 @@ static struct da9063_regulators_pdata *da9063_parse_regulators_dt(
}
num = of_regulator_match(&pdev->dev, node, da9063_matches,
- ARRAY_SIZE(da9063_matches));
+ da9063_matches_len);
of_node_put(node);
if (num < 0) {
dev_err(&pdev->dev, "Failed to match regulators\n");
@@ -689,7 +703,7 @@ static struct da9063_regulators_pdata *da9063_parse_regulators_dt(
pdata->n_regulators = num;
n = 0;
- for (i = 0; i < ARRAY_SIZE(da9063_matches); i++) {
+ for (i = 0; i < da9063_matches_len; i++) {
if (!da9063_matches[i].init_data)
continue;
--
2.16.2
The PMIC_DA9063 is a complete misnomer, it denotes the value of the
DA9063 chip ID register, so rename it as such. It is also the value
of chip ID register of DA9063L though, so drop the enum as all the
DA9063 "models" share the same chip ID and thus the distinction will
have to be made using DT or otherwise.
Signed-off-by: Marek Vasut <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Lee Jones <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Steve Twiss <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: [email protected]
Acked-by: Mark Brown <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
---
V2: No change
V3: No change
---
drivers/mfd/da9063-core.c | 2 +-
drivers/mfd/da9063-i2c.c | 2 +-
drivers/regulator/da9063-regulator.c | 2 +-
include/linux/mfd/da9063/core.h | 4 +---
4 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
index 8226ebd8b96d..fb122316c421 100644
--- a/drivers/mfd/da9063-core.c
+++ b/drivers/mfd/da9063-core.c
@@ -192,7 +192,7 @@ int da9063_device_init(struct da9063 *da9063, unsigned int irq)
dev_err(da9063->dev, "Cannot read chip model id.\n");
return -EIO;
}
- if (model != PMIC_DA9063) {
+ if (model != PMIC_CHIP_ID_DA9063) {
dev_err(da9063->dev, "Invalid chip model id: 0x%02x\n", model);
return -ENODEV;
}
diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c
index 981805a2c521..7f84030c8d53 100644
--- a/drivers/mfd/da9063-i2c.c
+++ b/drivers/mfd/da9063-i2c.c
@@ -280,7 +280,7 @@ static int da9063_i2c_remove(struct i2c_client *i2c)
}
static const struct i2c_device_id da9063_i2c_id[] = {
- {"da9063", PMIC_DA9063},
+ { "da9063", PMIC_CHIP_ID_DA9063 },
{},
};
MODULE_DEVICE_TABLE(i2c, da9063_i2c_id);
diff --git a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-regulator.c
index 6a8f9cd69f52..87c884ae0064 100644
--- a/drivers/regulator/da9063-regulator.c
+++ b/drivers/regulator/da9063-regulator.c
@@ -585,7 +585,7 @@ static struct da9063_dev_model regulators_models[] = {
{
.regulator_info = da9063_regulator_info,
.n_regulators = ARRAY_SIZE(da9063_regulator_info),
- .dev_model = PMIC_DA9063,
+ .dev_model = PMIC_CHIP_ID_DA9063,
},
{ }
};
diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
index f3ae65db4c86..664f650d0086 100644
--- a/include/linux/mfd/da9063/core.h
+++ b/include/linux/mfd/da9063/core.h
@@ -29,9 +29,7 @@
#define DA9063_DRVNAME_RTC "da9063-rtc"
#define DA9063_DRVNAME_VIBRATION "da9063-vibration"
-enum da9063_models {
- PMIC_DA9063 = 0x61,
-};
+#define PMIC_CHIP_ID_DA9063 0x61
enum da9063_variant_codes {
PMIC_DA9063_AD = 0x3,
--
2.16.2
The DA9063L does not contain RTC block, unlike the full DA9063.
Split the RTC block into separate mfd cell and register it only
on DA9063.
Signed-off-by: Marek Vasut <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Lee Jones <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Steve Twiss <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: [email protected]
---
V2: No change
V3: Rework of mfd: da9063: Disallow RTC on DA9063L
---
drivers/mfd/da9063-core.c | 30 +++++++++++++++++++++++-------
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
index eebca3442cf3..b05910c797af 100644
--- a/drivers/mfd/da9063-core.c
+++ b/drivers/mfd/da9063-core.c
@@ -76,7 +76,7 @@ static struct resource da9063_hwmon_resources[] = {
};
-static const struct mfd_cell da9063_devs[] = {
+static const struct mfd_cell da9063_common_devs[] = {
{
.name = DA9063_DRVNAME_REGULATORS,
.num_resources = ARRAY_SIZE(da9063_regulators_resources),
@@ -100,15 +100,19 @@ static const struct mfd_cell da9063_devs[] = {
.resources = da9063_onkey_resources,
.of_compatible = "dlg,da9063-onkey",
},
+ {
+ .name = DA9063_DRVNAME_VIBRATION,
+ },
+};
+
+/* Only present on DA9063 , not on DA9063L */
+static const struct mfd_cell da9063_devs[] = {
{
.name = DA9063_DRVNAME_RTC,
.num_resources = ARRAY_SIZE(da9063_rtc_resources),
.resources = da9063_rtc_resources,
.of_compatible = "dlg,da9063-rtc",
},
- {
- .name = DA9063_DRVNAME_VIBRATION,
- },
};
static int da9063_clear_fault_log(struct da9063 *da9063)
@@ -225,16 +229,28 @@ int da9063_device_init(struct da9063 *da9063, unsigned int irq)
da9063->irq_base = regmap_irq_chip_get_base(da9063->regmap_irq);
- ret = mfd_add_devices(da9063->dev, -1, da9063_devs,
- ARRAY_SIZE(da9063_devs), NULL, da9063->irq_base,
- NULL);
+ ret = mfd_add_devices(da9063->dev, -1, da9063_common_devs,
+ ARRAY_SIZE(da9063_common_devs),
+ NULL, da9063->irq_base, NULL);
if (ret) {
dev_err(da9063->dev, "Cannot add MFD cells\n");
goto err_irq_exit;
}
+ if (da9063->type == PMIC_TYPE_DA9063) {
+ ret = mfd_add_devices(da9063->dev, -1, da9063_devs,
+ ARRAY_SIZE(da9063_devs),
+ NULL, da9063->irq_base, NULL);
+ if (ret) {
+ dev_err(da9063->dev, "Cannot add MFD cells\n");
+ goto err_mfd_cleanup;
+ }
+ }
+
return ret;
+err_mfd_cleanup:
+ mfd_remove_devices(da9063->dev);
err_irq_exit:
da9063_irq_exit(da9063);
return ret;
--
2.16.2
Add type for DA9063L, which is a reduced variant of the DA9063
without RTC block and with less regulators.
Signed-off-by: Marek Vasut <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Lee Jones <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Steve Twiss <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: [email protected]
Reviewed-by: Geert Uytterhoeven <[email protected]>
---
V2: No change
V3: No change
---
include/linux/mfd/da9063/core.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
index eb234582dcb2..c3e917dd1860 100644
--- a/include/linux/mfd/da9063/core.h
+++ b/include/linux/mfd/da9063/core.h
@@ -33,6 +33,7 @@
enum da9063_type {
PMIC_TYPE_DA9063 = 0,
+ PMIC_TYPE_DA9063L,
};
enum da9063_variant_codes {
--
2.16.2
The model number stored in the struct da9063 is the same for all
variants of the da9063 since it is the chip ID, which is always
the same. Replace that with a separate identifier instead, which
allows us to discern the DA9063 variants by setting the type
based on either DT match or otherwise.
Signed-off-by: Marek Vasut <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Lee Jones <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Steve Twiss <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: [email protected]
Acked-by: Mark Brown <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
---
V2: Drop useless cast of id->driver_data
V3: Fix kernel 0day error , s/dev_model/type/
---
drivers/mfd/da9063-core.c | 1 -
drivers/mfd/da9063-i2c.c | 5 +++--
drivers/regulator/da9063-regulator.c | 6 +++---
include/linux/mfd/da9063/core.h | 6 +++++-
4 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
index fb122316c421..eebca3442cf3 100644
--- a/drivers/mfd/da9063-core.c
+++ b/drivers/mfd/da9063-core.c
@@ -215,7 +215,6 @@ int da9063_device_init(struct da9063 *da9063, unsigned int irq)
return -ENODEV;
}
- da9063->model = model;
da9063->variant_code = variant_code;
ret = da9063_irq_init(da9063);
diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c
index 7f84030c8d53..048ce55ebc5b 100644
--- a/drivers/mfd/da9063-i2c.c
+++ b/drivers/mfd/da9063-i2c.c
@@ -236,7 +236,7 @@ static const struct of_device_id da9063_dt_ids[] = {
};
MODULE_DEVICE_TABLE(of, da9063_dt_ids);
static int da9063_i2c_probe(struct i2c_client *i2c,
- const struct i2c_device_id *id)
+ const struct i2c_device_id *id)
{
struct da9063 *da9063;
int ret;
@@ -248,6 +248,7 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
i2c_set_clientdata(i2c, da9063);
da9063->dev = &i2c->dev;
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;
@@ -280,7 +281,7 @@ static int da9063_i2c_remove(struct i2c_client *i2c)
}
static const struct i2c_device_id da9063_i2c_id[] = {
- { "da9063", PMIC_CHIP_ID_DA9063 },
+ { "da9063", PMIC_TYPE_DA9063 },
{},
};
MODULE_DEVICE_TABLE(i2c, da9063_i2c_id);
diff --git a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-regulator.c
index 87c884ae0064..eeb0d431dda1 100644
--- a/drivers/regulator/da9063-regulator.c
+++ b/drivers/regulator/da9063-regulator.c
@@ -98,7 +98,7 @@ struct da9063_regulator_info {
struct da9063_dev_model {
const struct da9063_regulator_info *regulator_info;
unsigned n_regulators;
- unsigned dev_model;
+ enum da9063_type type;
};
/* Single regulator settings */
@@ -585,7 +585,7 @@ static struct da9063_dev_model regulators_models[] = {
{
.regulator_info = da9063_regulator_info,
.n_regulators = ARRAY_SIZE(da9063_regulator_info),
- .dev_model = PMIC_CHIP_ID_DA9063,
+ .type = PMIC_TYPE_DA9063,
},
{ }
};
@@ -741,7 +741,7 @@ static int da9063_regulator_probe(struct platform_device *pdev)
/* Find regulators set for particular device model */
for (model = regulators_models; model->regulator_info; model++) {
- if (model->dev_model == da9063->model)
+ if (model->type == da9063->type)
break;
}
if (!model->regulator_info) {
diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
index 664f650d0086..eb234582dcb2 100644
--- a/include/linux/mfd/da9063/core.h
+++ b/include/linux/mfd/da9063/core.h
@@ -31,6 +31,10 @@
#define PMIC_CHIP_ID_DA9063 0x61
+enum da9063_type {
+ PMIC_TYPE_DA9063 = 0,
+};
+
enum da9063_variant_codes {
PMIC_DA9063_AD = 0x3,
PMIC_DA9063_BB = 0x5,
@@ -76,7 +80,7 @@ enum da9063_irqs {
struct da9063 {
/* Device */
struct device *dev;
- unsigned short model;
+ enum da9063_type type;
unsigned char variant_code;
unsigned int flags;
--
2.16.2
While the datasheet for DA9063L (2v1, 23-Mar-2017) lists the RTC register
block, the DA9063L does not have an RTC. Add custom regmap for DA9063L to
prevent access into that register block.
Signed-off-by: Marek Vasut <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Lee Jones <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Steve Twiss <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: [email protected]
---
V3: New patch
---
drivers/mfd/da9063-i2c.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 91 insertions(+)
diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c
index 048ce55ebc5b..f0d92a37df6b 100644
--- a/drivers/mfd/da9063-i2c.c
+++ b/drivers/mfd/da9063-i2c.c
@@ -208,6 +208,93 @@ static const struct regmap_access_table da9063_bb_volatile_table = {
.n_yes_ranges = ARRAY_SIZE(da9063_bb_volatile_ranges),
};
+static const struct regmap_range da9063l_bb_readable_ranges[] = {
+ {
+ .range_min = DA9063_REG_PAGE_CON,
+ .range_max = DA9063_REG_MON_A10_RES,
+ }, {
+ .range_min = DA9063_REG_SEQ,
+ .range_max = DA9063_REG_ID_32_31,
+ }, {
+ .range_min = DA9063_REG_SEQ_A,
+ .range_max = DA9063_REG_AUTO3_LOW,
+ }, {
+ .range_min = DA9063_REG_T_OFFSET,
+ .range_max = DA9063_BB_REG_GP_ID_19,
+ }, {
+ .range_min = DA9063_REG_CHIP_ID,
+ .range_max = DA9063_REG_CHIP_VARIANT,
+ },
+};
+
+static const struct regmap_range da9063l_bb_writeable_ranges[] = {
+ {
+ .range_min = DA9063_REG_PAGE_CON,
+ .range_max = DA9063_REG_PAGE_CON,
+ }, {
+ .range_min = DA9063_REG_FAULT_LOG,
+ .range_max = DA9063_REG_VSYS_MON,
+ }, {
+ .range_min = DA9063_REG_SEQ,
+ .range_max = DA9063_REG_ID_32_31,
+ }, {
+ .range_min = DA9063_REG_SEQ_A,
+ .range_max = DA9063_REG_AUTO3_LOW,
+ }, {
+ .range_min = DA9063_REG_CONFIG_I,
+ .range_max = DA9063_BB_REG_MON_REG_4,
+ }, {
+ .range_min = DA9063_BB_REG_GP_ID_0,
+ .range_max = DA9063_BB_REG_GP_ID_19,
+ },
+};
+
+static const struct regmap_range da9063l_bb_volatile_ranges[] = {
+ {
+ .range_min = DA9063_REG_PAGE_CON,
+ .range_max = DA9063_REG_EVENT_D,
+ }, {
+ .range_min = DA9063_REG_CONTROL_A,
+ .range_max = DA9063_REG_CONTROL_B,
+ }, {
+ .range_min = DA9063_REG_CONTROL_E,
+ .range_max = DA9063_REG_CONTROL_F,
+ }, {
+ .range_min = DA9063_REG_BCORE2_CONT,
+ .range_max = DA9063_REG_LDO11_CONT,
+ }, {
+ .range_min = DA9063_REG_DVC_1,
+ .range_max = DA9063_REG_ADC_MAN,
+ }, {
+ .range_min = DA9063_REG_ADC_RES_L,
+ .range_max = DA9063_REG_MON_A10_RES,
+ }, {
+ .range_min = DA9063_REG_SEQ,
+ .range_max = DA9063_REG_SEQ,
+ }, {
+ .range_min = DA9063_REG_EN_32K,
+ .range_max = DA9063_REG_EN_32K,
+ }, {
+ .range_min = DA9063_BB_REG_MON_REG_5,
+ .range_max = DA9063_BB_REG_MON_REG_6,
+ },
+};
+
+static const struct regmap_access_table da9063l_bb_readable_table = {
+ .yes_ranges = da9063l_bb_readable_ranges,
+ .n_yes_ranges = ARRAY_SIZE(da9063l_bb_readable_ranges),
+};
+
+static const struct regmap_access_table da9063l_bb_writeable_table = {
+ .yes_ranges = da9063l_bb_writeable_ranges,
+ .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_range_cfg da9063_range_cfg[] = {
{
.range_min = DA9063_REG_PAGE_CON,
@@ -254,6 +341,10 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
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;
--
2.16.2
While the datasheet for DA9063L (2v1, 23-Mar-2017) lists the RTC register
block, the DA9063L does not have an RTC. Add custom IRQ map for DA9063L to
ignore the Alarm and Tick IRQs from the PMIC.
Signed-off-by: Marek Vasut <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Lee Jones <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Steve Twiss <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: [email protected]
---
V3: New patch
---
drivers/mfd/da9063-irq.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 53 insertions(+), 2 deletions(-)
diff --git a/drivers/mfd/da9063-irq.c b/drivers/mfd/da9063-irq.c
index 5b406ecfc14a..b6a88861cc2e 100644
--- a/drivers/mfd/da9063-irq.c
+++ b/drivers/mfd/da9063-irq.c
@@ -74,8 +74,55 @@ static const struct regmap_irq_chip da9063_irq_chip = {
.init_ack_masked = true,
};
+static const struct regmap_irq da9063l_irqs[] = {
+ /* DA9063 event A register */
+ REGMAP_IRQ_REG(DA9063_IRQ_ONKEY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ONKEY),
+ REGMAP_IRQ_REG(DA9063_IRQ_ADC_RDY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ADC_RDY),
+ REGMAP_IRQ_REG(DA9063_IRQ_SEQ_RDY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_SEQ_RDY),
+ /* DA9063 event B register */
+ REGMAP_IRQ_REG(DA9063_IRQ_WAKE, DA9063_REG_EVENT_B_OFFSET, DA9063_M_WAKE),
+ REGMAP_IRQ_REG(DA9063_IRQ_TEMP, DA9063_REG_EVENT_B_OFFSET, DA9063_M_TEMP),
+ REGMAP_IRQ_REG(DA9063_IRQ_COMP_1V2, DA9063_REG_EVENT_B_OFFSET, DA9063_M_COMP_1V2),
+ REGMAP_IRQ_REG(DA9063_IRQ_LDO_LIM, DA9063_REG_EVENT_B_OFFSET, DA9063_M_LDO_LIM),
+ REGMAP_IRQ_REG(DA9063_IRQ_REG_UVOV, DA9063_REG_EVENT_B_OFFSET, DA9063_M_UVOV),
+ REGMAP_IRQ_REG(DA9063_IRQ_DVC_RDY, DA9063_REG_EVENT_B_OFFSET, DA9063_M_DVC_RDY),
+ REGMAP_IRQ_REG(DA9063_IRQ_VDD_MON, DA9063_REG_EVENT_B_OFFSET, DA9063_M_VDD_MON),
+ REGMAP_IRQ_REG(DA9063_IRQ_WARN, DA9063_REG_EVENT_B_OFFSET, DA9063_M_VDD_WARN),
+ /* DA9063 event C register */
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI0, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI0),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI1, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI1),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI2, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI2),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI3, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI3),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI4, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI4),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI5, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI5),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI6, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI6),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI7, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI7),
+ /* DA9063 event D register */
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI8, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI8),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI9, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI9),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI10, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI10),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI11, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI11),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI12, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI12),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI13, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI13),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI14, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI14),
+ REGMAP_IRQ_REG(DA9063_IRQ_GPI15, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI15),
+};
+
+static const struct regmap_irq_chip da9063l_irq_chip = {
+ .name = "da9063l-irq",
+ .irqs = da9063l_irqs,
+ .num_irqs = DA9063_NUM_IRQ,
+
+ .num_regs = 4,
+ .status_base = DA9063_REG_EVENT_A,
+ .mask_base = DA9063_REG_IRQ_MASK_A,
+ .ack_base = DA9063_REG_EVENT_A,
+ .init_ack_masked = true,
+};
+
int da9063_irq_init(struct da9063 *da9063)
{
+ struct regmap_irq_chip *irq_chip;
int ret;
if (!da9063->chip_irq) {
@@ -83,10 +130,14 @@ int da9063_irq_init(struct da9063 *da9063)
return -EINVAL;
}
+ if (da9063->type == PMIC_TYPE_DA9063)
+ irq_chip = &da9063_irq_chip;
+ else
+ irq_chip = &da9063l_irq_chip;
+
ret = regmap_add_irq_chip(da9063->regmap, da9063->chip_irq,
IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED,
- da9063->irq_base, &da9063_irq_chip,
- &da9063->regmap_irq);
+ da9063->irq_base, irq_chip, &da9063->regmap_irq);
if (ret) {
dev_err(da9063->dev, "Failed to reguest IRQ %d: %d\n",
da9063->chip_irq, ret);
--
2.16.2
On Sat, Jun 2, 2018 at 12:11 PM, Marek Vasut <[email protected]> wrote:
> In case mfd_add_devices() fails, da9063_irq_exit() is not called to
> undo the IRQchip setup done by da9063_irq_init(). Fix this by adding
> the missing fail path.
>
> Signed-off-by: Marek Vasut <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Sat, Jun 2, 2018 at 12:11 PM, Marek Vasut <[email protected]> wrote:
> Convert the regmap_irq table to use REGMAP_IRQ_REG().
>
> Signed-off-by: Marek Vasut <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Marek, Steve,
On Sat, Jun 2, 2018 at 12:11 PM, Marek Vasut <[email protected]> wrote:
> While the datasheet for DA9063L (2v1, 23-Mar-2017) lists the RTC register
> block, the DA9063L does not have an RTC. Add custom regmap for DA9063L to
> prevent access into that register block.
>
> Signed-off-by: Marek Vasut <[email protected]>
Thanks for your patch!
> --- a/drivers/mfd/da9063-i2c.c
> +++ b/drivers/mfd/da9063-i2c.c
> @@ -254,6 +341,10 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
Note that the line above doesn't check da9063->type, but da9063->variant_code...
> 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) {
... so this may be slightly confusing.
> + 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;
However, da9063->variant_code doesn't seem to have been filled in at this
point yet (the call to da9063_device_init() doing so is below, at the end
of the probe function!), so commit 9cb42e2a8ed06e91 ("mfd: da9063: Add
support for AD silicon variant") never actually handled the AD silicon variant
correctly? Or am I missing something?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Sat, Jun 2, 2018 at 3:41 PM, Marek Vasut <[email protected]> wrote:
> In case mfd_add_devices() fails, da9063_irq_exit() is not called to
> undo the IRQchip setup done by da9063_irq_init(). Fix this by adding
> the missing fail path.
>
> Signed-off-by: Marek Vasut <[email protected]>
Reviewed by: Vaishali Thakkar <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Lee Jones <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Steve Twiss <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: [email protected]
> ---
> V3: New patch
> ---
> drivers/mfd/da9063-core.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
> index 6c2870d4e754..8226ebd8b96d 100644
> --- a/drivers/mfd/da9063-core.c
> +++ b/drivers/mfd/da9063-core.c
> @@ -229,10 +229,16 @@ int da9063_device_init(struct da9063 *da9063, unsigned int irq)
> ret = mfd_add_devices(da9063->dev, -1, da9063_devs,
> ARRAY_SIZE(da9063_devs), NULL, da9063->irq_base,
> NULL);
> - if (ret)
> + if (ret) {
> dev_err(da9063->dev, "Cannot add MFD cells\n");
> + goto err_irq_exit;
> + }
>
> return ret;
> +
> +err_irq_exit:
> + da9063_irq_exit(da9063);
> + return ret;
> }
>
> void da9063_device_exit(struct da9063 *da9063)
> --
> 2.16.2
>
On Sat, 02 Jun 2018, Marek Vasut wrote:
> In case mfd_add_devices() fails, da9063_irq_exit() is not called to
> undo the IRQchip setup done by da9063_irq_init(). Fix this by adding
> the missing fail path.
>
> Signed-off-by: Marek Vasut <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Lee Jones <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Steve Twiss <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: [email protected]
> ---
> V3: New patch
> ---
> drivers/mfd/da9063-core.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
> index 6c2870d4e754..8226ebd8b96d 100644
> --- a/drivers/mfd/da9063-core.c
> +++ b/drivers/mfd/da9063-core.c
> @@ -229,10 +229,16 @@ int da9063_device_init(struct da9063 *da9063, unsigned int irq)
> ret = mfd_add_devices(da9063->dev, -1, da9063_devs,
> ARRAY_SIZE(da9063_devs), NULL, da9063->irq_base,
> NULL);
> - if (ret)
> + if (ret) {
> dev_err(da9063->dev, "Cannot add MFD cells\n");
> + goto err_irq_exit;
> + }
>
> return ret;
> +
> +err_irq_exit:
> + da9063_irq_exit(da9063);
> + return ret;
> }
>
> void da9063_device_exit(struct da9063 *da9063)
I haven't seen the later patches yet, so maybe the goto label expanded
on, but if it's not, then this would be better:
- if (ret)
+ if (ret) {
dev_err(da9063->dev, "Cannot add MFD cells\n");
+ da9063_irq_exit(da9063);
+ }
return ret;
}
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Sat, 02 Jun 2018, Marek Vasut wrote:
> Convert the regmap_irq table to use REGMAP_IRQ_REG().
>
> Signed-off-by: Marek Vasut <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Lee Jones <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Steve Twiss <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: [email protected]
> ---
> V3: New patch
> Note: A sed oneliner was used:
> sed -i "/\[DA9063_IRQ_/ {N;N;N;s/\n//g;s/.*\[\(DA9063_IRQ_[^]]\+\)].*reg_offset = \([^,]\+\),.* = \([^,]\+\),.*/\tREGMAP_IRQ_REG(\1, \2, \3),/}" drivers/mfd/da9063-irq.c
> ---
> drivers/mfd/da9063-irq.c | 145 ++++++++++-------------------------------------
> 1 file changed, 29 insertions(+), 116 deletions(-)
>
> diff --git a/drivers/mfd/da9063-irq.c b/drivers/mfd/da9063-irq.c
> index 207bbfe55449..5b406ecfc14a 100644
> --- a/drivers/mfd/da9063-irq.c
> +++ b/drivers/mfd/da9063-irq.c
> @@ -28,125 +28,38 @@
>
> static const struct regmap_irq da9063_irqs[] = {
> /* DA9063 event A register */
> - [DA9063_IRQ_ONKEY] = {
> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> - .mask = DA9063_M_ONKEY,
> - },
> - [DA9063_IRQ_ALARM] = {
> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> - .mask = DA9063_M_ALARM,
> - },
> - [DA9063_IRQ_TICK] = {
> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> - .mask = DA9063_M_TICK,
> - },
> - [DA9063_IRQ_ADC_RDY] = {
> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> - .mask = DA9063_M_ADC_RDY,
> - },
> - [DA9063_IRQ_SEQ_RDY] = {
> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> - .mask = DA9063_M_SEQ_RDY,
> - },
> + REGMAP_IRQ_REG(DA9063_IRQ_ONKEY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ONKEY),
> + REGMAP_IRQ_REG(DA9063_IRQ_ALARM, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ALARM),
> + REGMAP_IRQ_REG(DA9063_IRQ_TICK, DA9063_REG_EVENT_A_OFFSET, DA9063_M_TICK),
> + REGMAP_IRQ_REG(DA9063_IRQ_ADC_RDY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ADC_RDY),
> + REGMAP_IRQ_REG(DA9063_IRQ_SEQ_RDY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_SEQ_RDY),
Nice, but I think checkpatch.pl would complain.
Better to break after the first argument I think.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Sat, 02 Jun 2018, Marek Vasut wrote:
> The PMIC_DA9063 is a complete misnomer, it denotes the value of the
> DA9063 chip ID register, so rename it as such. It is also the value
> of chip ID register of DA9063L though, so drop the enum as all the
> DA9063 "models" share the same chip ID and thus the distinction will
> have to be made using DT or otherwise.
>
> Signed-off-by: Marek Vasut <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Lee Jones <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Steve Twiss <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: [email protected]
> Acked-by: Mark Brown <[email protected]>
> Reviewed-by: Geert Uytterhoeven <[email protected]>
> ---
> V2: No change
> V3: No change
> ---
> drivers/mfd/da9063-core.c | 2 +-
> drivers/mfd/da9063-i2c.c | 2 +-
> drivers/regulator/da9063-regulator.c | 2 +-
> include/linux/mfd/da9063/core.h | 4 +---
> 4 files changed, 4 insertions(+), 6 deletions(-)
For my own reference:
Acked-for-MFD-by: Lee Jones <[email protected]>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 06/04/2018 02:24 PM, Lee Jones wrote:
> On Sat, 02 Jun 2018, Marek Vasut wrote:
>
>> In case mfd_add_devices() fails, da9063_irq_exit() is not called to
>> undo the IRQchip setup done by da9063_irq_init(). Fix this by adding
>> the missing fail path.
>>
>> Signed-off-by: Marek Vasut <[email protected]>
>> Cc: Geert Uytterhoeven <[email protected]>
>> Cc: Lee Jones <[email protected]>
>> Cc: Mark Brown <[email protected]>
>> Cc: Steve Twiss <[email protected]>
>> Cc: Wolfram Sang <[email protected]>
>> Cc: [email protected]
>> ---
>> V3: New patch
>> ---
>> drivers/mfd/da9063-core.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
>> index 6c2870d4e754..8226ebd8b96d 100644
>> --- a/drivers/mfd/da9063-core.c
>> +++ b/drivers/mfd/da9063-core.c
>> @@ -229,10 +229,16 @@ int da9063_device_init(struct da9063 *da9063, unsigned int irq)
>> ret = mfd_add_devices(da9063->dev, -1, da9063_devs,
>> ARRAY_SIZE(da9063_devs), NULL, da9063->irq_base,
>> NULL);
>> - if (ret)
>> + if (ret) {
>> dev_err(da9063->dev, "Cannot add MFD cells\n");
>> + goto err_irq_exit;
>> + }
>>
>> return ret;
>> +
>> +err_irq_exit:
>> + da9063_irq_exit(da9063);
>> + return ret;
>> }
>>
>> void da9063_device_exit(struct da9063 *da9063)
>
> I haven't seen the later patches yet, so maybe the goto label expanded
> on, but if it's not, then this would be better:
>
> - if (ret)
> + if (ret) {
> dev_err(da9063->dev, "Cannot add MFD cells\n");
> + da9063_irq_exit(da9063);
> + }
>
> return ret;
> }
It did expand in the later patches, yes.
--
Best regards,
Marek Vasut
On 06/04/2018 09:39 AM, Geert Uytterhoeven wrote:
> Hi Marek, Steve,
>
> On Sat, Jun 2, 2018 at 12:11 PM, Marek Vasut <[email protected]> wrote:
>> While the datasheet for DA9063L (2v1, 23-Mar-2017) lists the RTC register
>> block, the DA9063L does not have an RTC. Add custom regmap for DA9063L to
>> prevent access into that register block.
>>
>> Signed-off-by: Marek Vasut <[email protected]>
>
> Thanks for your patch!
>
>> --- a/drivers/mfd/da9063-i2c.c
>> +++ b/drivers/mfd/da9063-i2c.c
>> @@ -254,6 +341,10 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
>
> Note that the line above doesn't check da9063->type, but da9063->variant_code...
>
>> 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) {
>
> ... so this may be slightly confusing.
I know.
>> + 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;
>
> However, da9063->variant_code doesn't seem to have been filled in at this
> point yet (the call to da9063_device_init() doing so is below, at the end
> of the probe function!), so commit 9cb42e2a8ed06e91 ("mfd: da9063: Add
> support for AD silicon variant") never actually handled the AD silicon variant
> correctly? Or am I missing something?
Ha, that is a good point.
--
Best regards,
Marek Vasut
On 06/04/2018 02:26 PM, Lee Jones wrote:
> On Sat, 02 Jun 2018, Marek Vasut wrote:
>
>> Convert the regmap_irq table to use REGMAP_IRQ_REG().
>>
>> Signed-off-by: Marek Vasut <[email protected]>
>> Cc: Geert Uytterhoeven <[email protected]>
>> Cc: Lee Jones <[email protected]>
>> Cc: Mark Brown <[email protected]>
>> Cc: Steve Twiss <[email protected]>
>> Cc: Wolfram Sang <[email protected]>
>> Cc: [email protected]
>> ---
>> V3: New patch
>> Note: A sed oneliner was used:
>> sed -i "/\[DA9063_IRQ_/ {N;N;N;s/\n//g;s/.*\[\(DA9063_IRQ_[^]]\+\)].*reg_offset = \([^,]\+\),.* = \([^,]\+\),.*/\tREGMAP_IRQ_REG(\1, \2, \3),/}" drivers/mfd/da9063-irq.c
>> ---
>> drivers/mfd/da9063-irq.c | 145 ++++++++++-------------------------------------
>> 1 file changed, 29 insertions(+), 116 deletions(-)
>>
>> diff --git a/drivers/mfd/da9063-irq.c b/drivers/mfd/da9063-irq.c
>> index 207bbfe55449..5b406ecfc14a 100644
>> --- a/drivers/mfd/da9063-irq.c
>> +++ b/drivers/mfd/da9063-irq.c
>> @@ -28,125 +28,38 @@
>>
>> static const struct regmap_irq da9063_irqs[] = {
>> /* DA9063 event A register */
>> - [DA9063_IRQ_ONKEY] = {
>> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
>> - .mask = DA9063_M_ONKEY,
>> - },
>> - [DA9063_IRQ_ALARM] = {
>> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
>> - .mask = DA9063_M_ALARM,
>> - },
>> - [DA9063_IRQ_TICK] = {
>> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
>> - .mask = DA9063_M_TICK,
>> - },
>> - [DA9063_IRQ_ADC_RDY] = {
>> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
>> - .mask = DA9063_M_ADC_RDY,
>> - },
>> - [DA9063_IRQ_SEQ_RDY] = {
>> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
>> - .mask = DA9063_M_SEQ_RDY,
>> - },
>> + REGMAP_IRQ_REG(DA9063_IRQ_ONKEY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ONKEY),
>> + REGMAP_IRQ_REG(DA9063_IRQ_ALARM, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ALARM),
>> + REGMAP_IRQ_REG(DA9063_IRQ_TICK, DA9063_REG_EVENT_A_OFFSET, DA9063_M_TICK),
>> + REGMAP_IRQ_REG(DA9063_IRQ_ADC_RDY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ADC_RDY),
>> + REGMAP_IRQ_REG(DA9063_IRQ_SEQ_RDY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_SEQ_RDY),
>
> Nice, but I think checkpatch.pl would complain.
>
> Better to break after the first argument I think.
Doesn't really help the readability, but done.
--
Best regards,
Marek Vasut
On 06/04/2018 02:26 PM, Lee Jones wrote:
> On Sat, 02 Jun 2018, Marek Vasut wrote:
>
>> The PMIC_DA9063 is a complete misnomer, it denotes the value of the
>> DA9063 chip ID register, so rename it as such. It is also the value
>> of chip ID register of DA9063L though, so drop the enum as all the
>> DA9063 "models" share the same chip ID and thus the distinction will
>> have to be made using DT or otherwise.
>>
>> Signed-off-by: Marek Vasut <[email protected]>
>> Cc: Geert Uytterhoeven <[email protected]>
>> Cc: Lee Jones <[email protected]>
>> Cc: Mark Brown <[email protected]>
>> Cc: Steve Twiss <[email protected]>
>> Cc: Wolfram Sang <[email protected]>
>> Cc: [email protected]
>> Acked-by: Mark Brown <[email protected]>
>> Reviewed-by: Geert Uytterhoeven <[email protected]>
>> ---
>> V2: No change
>> V3: No change
>> ---
>> drivers/mfd/da9063-core.c | 2 +-
>> drivers/mfd/da9063-i2c.c | 2 +-
>> drivers/regulator/da9063-regulator.c | 2 +-
>> include/linux/mfd/da9063/core.h | 4 +---
>> 4 files changed, 4 insertions(+), 6 deletions(-)
>
> For my own reference:
> Acked-for-MFD-by: Lee Jones <[email protected]>
Thanks.
Any other comments before I submit the next round in a few days?
--
Best regards,
Marek Vasut
On Mon, 04 Jun 2018, Marek Vasut wrote:
> On 06/04/2018 02:26 PM, Lee Jones wrote:
> > On Sat, 02 Jun 2018, Marek Vasut wrote:
> >
> >> The PMIC_DA9063 is a complete misnomer, it denotes the value of the
> >> DA9063 chip ID register, so rename it as such. It is also the value
> >> of chip ID register of DA9063L though, so drop the enum as all the
> >> DA9063 "models" share the same chip ID and thus the distinction will
> >> have to be made using DT or otherwise.
> >>
> >> Signed-off-by: Marek Vasut <[email protected]>
> >> Cc: Geert Uytterhoeven <[email protected]>
> >> Cc: Lee Jones <[email protected]>
> >> Cc: Mark Brown <[email protected]>
> >> Cc: Steve Twiss <[email protected]>
> >> Cc: Wolfram Sang <[email protected]>
> >> Cc: [email protected]
> >> Acked-by: Mark Brown <[email protected]>
> >> Reviewed-by: Geert Uytterhoeven <[email protected]>
> >> ---
> >> V2: No change
> >> V3: No change
> >> ---
> >> drivers/mfd/da9063-core.c | 2 +-
> >> drivers/mfd/da9063-i2c.c | 2 +-
> >> drivers/regulator/da9063-regulator.c | 2 +-
> >> include/linux/mfd/da9063/core.h | 4 +---
> >> 4 files changed, 4 insertions(+), 6 deletions(-)
> >
> > For my own reference:
> > Acked-for-MFD-by: Lee Jones <[email protected]>
>
> Thanks.
>
> Any other comments before I submit the next round in a few days?
Yes, please hold off until I can complete the set.
Should be done by EOD today.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 04 Jun 2018, Marek Vasut wrote:
> On 06/04/2018 02:26 PM, Lee Jones wrote:
> > On Sat, 02 Jun 2018, Marek Vasut wrote:
> >
> >> Convert the regmap_irq table to use REGMAP_IRQ_REG().
> >>
> >> Signed-off-by: Marek Vasut <[email protected]>
> >> Cc: Geert Uytterhoeven <[email protected]>
> >> Cc: Lee Jones <[email protected]>
> >> Cc: Mark Brown <[email protected]>
> >> Cc: Steve Twiss <[email protected]>
> >> Cc: Wolfram Sang <[email protected]>
> >> Cc: [email protected]
> >> ---
> >> V3: New patch
> >> Note: A sed oneliner was used:
> >> sed -i "/\[DA9063_IRQ_/ {N;N;N;s/\n//g;s/.*\[\(DA9063_IRQ_[^]]\+\)].*reg_offset = \([^,]\+\),.* = \([^,]\+\),.*/\tREGMAP_IRQ_REG(\1, \2, \3),/}" drivers/mfd/da9063-irq.c
> >> ---
> >> drivers/mfd/da9063-irq.c | 145 ++++++++++-------------------------------------
> >> 1 file changed, 29 insertions(+), 116 deletions(-)
> >>
> >> diff --git a/drivers/mfd/da9063-irq.c b/drivers/mfd/da9063-irq.c
> >> index 207bbfe55449..5b406ecfc14a 100644
> >> --- a/drivers/mfd/da9063-irq.c
> >> +++ b/drivers/mfd/da9063-irq.c
> >> @@ -28,125 +28,38 @@
> >>
> >> static const struct regmap_irq da9063_irqs[] = {
> >> /* DA9063 event A register */
> >> - [DA9063_IRQ_ONKEY] = {
> >> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> >> - .mask = DA9063_M_ONKEY,
> >> - },
> >> - [DA9063_IRQ_ALARM] = {
> >> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> >> - .mask = DA9063_M_ALARM,
> >> - },
> >> - [DA9063_IRQ_TICK] = {
> >> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> >> - .mask = DA9063_M_TICK,
> >> - },
> >> - [DA9063_IRQ_ADC_RDY] = {
> >> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> >> - .mask = DA9063_M_ADC_RDY,
> >> - },
> >> - [DA9063_IRQ_SEQ_RDY] = {
> >> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> >> - .mask = DA9063_M_SEQ_RDY,
> >> - },
> >> + REGMAP_IRQ_REG(DA9063_IRQ_ONKEY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ONKEY),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_ALARM, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ALARM),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_TICK, DA9063_REG_EVENT_A_OFFSET, DA9063_M_TICK),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_ADC_RDY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ADC_RDY),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_SEQ_RDY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_SEQ_RDY),
> >
> > Nice, but I think checkpatch.pl would complain.
> >
> > Better to break after the first argument I think.
>
> Doesn't really help the readability, but done.
I don't make the rules. :)
Personally /me is hoping that the 80 char limit is lifted to ~120 soon.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Sat, 02 Jun 2018, Marek Vasut wrote:
> The model number stored in the struct da9063 is the same for all
> variants of the da9063 since it is the chip ID, which is always
> the same. Replace that with a separate identifier instead, which
> allows us to discern the DA9063 variants by setting the type
> based on either DT match or otherwise.
>
> Signed-off-by: Marek Vasut <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Lee Jones <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Steve Twiss <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: [email protected]
> Acked-by: Mark Brown <[email protected]>
> Reviewed-by: Geert Uytterhoeven <[email protected]>
> ---
> V2: Drop useless cast of id->driver_data
> V3: Fix kernel 0day error , s/dev_model/type/
> ---
> drivers/mfd/da9063-core.c | 1 -
> drivers/mfd/da9063-i2c.c | 5 +++--
> drivers/regulator/da9063-regulator.c | 6 +++---
> include/linux/mfd/da9063/core.h | 6 +++++-
> 4 files changed, 11 insertions(+), 7 deletions(-)
For my own reference:
Acked-for-MFD-by: Lee Jones <[email protected]>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Sat, 02 Jun 2018, Marek Vasut wrote:
> Add type for DA9063L, which is a reduced variant of the DA9063
> without RTC block and with less regulators.
>
> Signed-off-by: Marek Vasut <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Lee Jones <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Steve Twiss <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: [email protected]
> Reviewed-by: Geert Uytterhoeven <[email protected]>
> ---
> V2: No change
> V3: No change
> ---
> include/linux/mfd/da9063/core.h | 1 +
> 1 file changed, 1 insertion(+)
For my own reference:
Acked-for-MFD-by: Lee Jones <[email protected]>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Lee,
On Tue, Jun 5, 2018 at 9:09 AM, Lee Jones <[email protected]> wrote:
> On Mon, 04 Jun 2018, Marek Vasut wrote:
>> On 06/04/2018 02:26 PM, Lee Jones wrote:
>> > On Sat, 02 Jun 2018, Marek Vasut wrote:
>> >> Convert the regmap_irq table to use REGMAP_IRQ_REG().
>> >>
>> >> Signed-off-by: Marek Vasut <[email protected]>
>> >> Cc: Geert Uytterhoeven <[email protected]>
>> >> Cc: Lee Jones <[email protected]>
>> >> Cc: Mark Brown <[email protected]>
>> >> Cc: Steve Twiss <[email protected]>
>> >> Cc: Wolfram Sang <[email protected]>
>> >> Cc: [email protected]
>> >> ---
>> >> V3: New patch
>> >> Note: A sed oneliner was used:
>> >> sed -i "/\[DA9063_IRQ_/ {N;N;N;s/\n//g;s/.*\[\(DA9063_IRQ_[^]]\+\)].*reg_offset = \([^,]\+\),.* = \([^,]\+\),.*/\tREGMAP_IRQ_REG(\1, \2, \3),/}" drivers/mfd/da9063-irq.c
>> >> ---
>> >> drivers/mfd/da9063-irq.c | 145 ++++++++++-------------------------------------
>> >> 1 file changed, 29 insertions(+), 116 deletions(-)
>> >>
>> >> diff --git a/drivers/mfd/da9063-irq.c b/drivers/mfd/da9063-irq.c
>> >> index 207bbfe55449..5b406ecfc14a 100644
>> >> --- a/drivers/mfd/da9063-irq.c
>> >> +++ b/drivers/mfd/da9063-irq.c
>> >> @@ -28,125 +28,38 @@
>> >>
>> >> static const struct regmap_irq da9063_irqs[] = {
>> >> /* DA9063 event A register */
>> >> - [DA9063_IRQ_ONKEY] = {
>> >> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
>> >> - .mask = DA9063_M_ONKEY,
>> >> - },
>> >> - [DA9063_IRQ_ALARM] = {
>> >> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
>> >> - .mask = DA9063_M_ALARM,
>> >> - },
>> >> - [DA9063_IRQ_TICK] = {
>> >> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
>> >> - .mask = DA9063_M_TICK,
>> >> - },
>> >> - [DA9063_IRQ_ADC_RDY] = {
>> >> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
>> >> - .mask = DA9063_M_ADC_RDY,
>> >> - },
>> >> - [DA9063_IRQ_SEQ_RDY] = {
>> >> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
>> >> - .mask = DA9063_M_SEQ_RDY,
>> >> - },
>> >> + REGMAP_IRQ_REG(DA9063_IRQ_ONKEY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ONKEY),
>> >> + REGMAP_IRQ_REG(DA9063_IRQ_ALARM, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ALARM),
>> >> + REGMAP_IRQ_REG(DA9063_IRQ_TICK, DA9063_REG_EVENT_A_OFFSET, DA9063_M_TICK),
>> >> + REGMAP_IRQ_REG(DA9063_IRQ_ADC_RDY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ADC_RDY),
>> >> + REGMAP_IRQ_REG(DA9063_IRQ_SEQ_RDY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_SEQ_RDY),
>> >
>> > Nice, but I think checkpatch.pl would complain.
>> >
>> > Better to break after the first argument I think.
>>
>> Doesn't really help the readability, but done.
>
> I don't make the rules. :)
Documentation/process/coding-style.rst:
"Statements longer than 80 columns will be broken into sensible chunks, unless
exceeding 80 columns significantly increases readability and does not hide
information."
> Personally /me is hoping that the 80 char limit is lifted to ~120 soon.
Please no ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Sat, 02 Jun 2018, Marek Vasut wrote:
> While the datasheet for DA9063L (2v1, 23-Mar-2017) lists the RTC register
> block, the DA9063L does not have an RTC. Add custom IRQ map for DA9063L to
> ignore the Alarm and Tick IRQs from the PMIC.
>
> Signed-off-by: Marek Vasut <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Lee Jones <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Steve Twiss <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: [email protected]
> ---
> V3: New patch
> ---
> drivers/mfd/da9063-irq.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 53 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mfd/da9063-irq.c b/drivers/mfd/da9063-irq.c
> index 5b406ecfc14a..b6a88861cc2e 100644
> --- a/drivers/mfd/da9063-irq.c
> +++ b/drivers/mfd/da9063-irq.c
> @@ -74,8 +74,55 @@ static const struct regmap_irq_chip da9063_irq_chip = {
> .init_ack_masked = true,
> };
>
> +static const struct regmap_irq da9063l_irqs[] = {
> + /* DA9063 event A register */
> + REGMAP_IRQ_REG(DA9063_IRQ_ONKEY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ONKEY),
> + REGMAP_IRQ_REG(DA9063_IRQ_ADC_RDY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ADC_RDY),
> + REGMAP_IRQ_REG(DA9063_IRQ_SEQ_RDY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_SEQ_RDY),
> + /* DA9063 event B register */
> + REGMAP_IRQ_REG(DA9063_IRQ_WAKE, DA9063_REG_EVENT_B_OFFSET, DA9063_M_WAKE),
> + REGMAP_IRQ_REG(DA9063_IRQ_TEMP, DA9063_REG_EVENT_B_OFFSET, DA9063_M_TEMP),
> + REGMAP_IRQ_REG(DA9063_IRQ_COMP_1V2, DA9063_REG_EVENT_B_OFFSET, DA9063_M_COMP_1V2),
> + REGMAP_IRQ_REG(DA9063_IRQ_LDO_LIM, DA9063_REG_EVENT_B_OFFSET, DA9063_M_LDO_LIM),
> + REGMAP_IRQ_REG(DA9063_IRQ_REG_UVOV, DA9063_REG_EVENT_B_OFFSET, DA9063_M_UVOV),
> + REGMAP_IRQ_REG(DA9063_IRQ_DVC_RDY, DA9063_REG_EVENT_B_OFFSET, DA9063_M_DVC_RDY),
> + REGMAP_IRQ_REG(DA9063_IRQ_VDD_MON, DA9063_REG_EVENT_B_OFFSET, DA9063_M_VDD_MON),
> + REGMAP_IRQ_REG(DA9063_IRQ_WARN, DA9063_REG_EVENT_B_OFFSET, DA9063_M_VDD_WARN),
> + /* DA9063 event C register */
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI0, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI0),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI1, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI1),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI2, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI2),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI3, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI3),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI4, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI4),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI5, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI5),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI6, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI6),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI7, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI7),
> + /* DA9063 event D register */
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI8, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI8),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI9, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI9),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI10, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI10),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI11, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI11),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI12, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI12),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI13, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI13),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI14, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI14),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI15, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI15),
> +};
Same here. Please make checkpatch.pl happen, even if it makes the
code slightly less readable.
> +static const struct regmap_irq_chip da9063l_irq_chip = {
> + .name = "da9063l-irq",
> + .irqs = da9063l_irqs,
> + .num_irqs = DA9063_NUM_IRQ,
> +
Nit: This '\n' is superfluous.
> + .num_regs = 4,
> + .status_base = DA9063_REG_EVENT_A,
> + .mask_base = DA9063_REG_IRQ_MASK_A,
> + .ack_base = DA9063_REG_EVENT_A,
> + .init_ack_masked = true,
> +};
> +
> int da9063_irq_init(struct da9063 *da9063)
> {
> + struct regmap_irq_chip *irq_chip;
> int ret;
>
> if (!da9063->chip_irq) {
> @@ -83,10 +130,14 @@ int da9063_irq_init(struct da9063 *da9063)
> return -EINVAL;
> }
>
> + if (da9063->type == PMIC_TYPE_DA9063)
> + irq_chip = &da9063_irq_chip;
> + else
> + irq_chip = &da9063l_irq_chip;
> +
> ret = regmap_add_irq_chip(da9063->regmap, da9063->chip_irq,
> IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED,
> - da9063->irq_base, &da9063_irq_chip,
> - &da9063->regmap_irq);
> + da9063->irq_base, irq_chip, &da9063->regmap_irq);
> if (ret) {
> dev_err(da9063->dev, "Failed to reguest IRQ %d: %d\n",
> da9063->chip_irq, ret);
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Sat, 02 Jun 2018, Marek Vasut wrote:
> The DA9063L does not contain RTC block, unlike the full DA9063.
> Split the RTC block into separate mfd cell and register it only
> on DA9063.
>
> Signed-off-by: Marek Vasut <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Lee Jones <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Steve Twiss <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: [email protected]
> ---
> V2: No change
> V3: Rework of mfd: da9063: Disallow RTC on DA9063L
> ---
> drivers/mfd/da9063-core.c | 30 +++++++++++++++++++++++-------
> 1 file changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
> index eebca3442cf3..b05910c797af 100644
> --- a/drivers/mfd/da9063-core.c
> +++ b/drivers/mfd/da9063-core.c
> @@ -76,7 +76,7 @@ static struct resource da9063_hwmon_resources[] = {
> };
>
>
> -static const struct mfd_cell da9063_devs[] = {
> +static const struct mfd_cell da9063_common_devs[] = {
> {
> .name = DA9063_DRVNAME_REGULATORS,
Appreciate that these are historical, but these device name defines
make me shudder. They only serve to act as an obfuscation layer when
debugging at platform level. Please consider getting rid of them.
> .num_resources = ARRAY_SIZE(da9063_regulators_resources),
> @@ -100,15 +100,19 @@ static const struct mfd_cell da9063_devs[] = {
> .resources = da9063_onkey_resources,
> .of_compatible = "dlg,da9063-onkey",
> },
> + {
> + .name = DA9063_DRVNAME_VIBRATION,
> + },
Place this on a single line please.
{ .name = DA9063_DRVNAME_VIBRATION },
> +};
> +
> +/* Only present on DA9063 , not on DA9063L */
> +static const struct mfd_cell da9063_devs[] = {
> {
> .name = DA9063_DRVNAME_RTC,
> .num_resources = ARRAY_SIZE(da9063_rtc_resources),
> .resources = da9063_rtc_resources,
> .of_compatible = "dlg,da9063-rtc",
> },
> - {
> - .name = DA9063_DRVNAME_VIBRATION,
> - },
> };
>
> static int da9063_clear_fault_log(struct da9063 *da9063)
> @@ -225,16 +229,28 @@ int da9063_device_init(struct da9063 *da9063, unsigned int irq)
>
> da9063->irq_base = regmap_irq_chip_get_base(da9063->regmap_irq);
>
> - ret = mfd_add_devices(da9063->dev, -1, da9063_devs,
> - ARRAY_SIZE(da9063_devs), NULL, da9063->irq_base,
> - NULL);
> + ret = mfd_add_devices(da9063->dev, -1, da9063_common_devs,
Please consider updating the -1's in this file with the appropriate
define in a separate patch.
> + ARRAY_SIZE(da9063_common_devs),
> + NULL, da9063->irq_base, NULL);
> if (ret) {
> dev_err(da9063->dev, "Cannot add MFD cells\n");
> goto err_irq_exit;
> }
>
> + if (da9063->type == PMIC_TYPE_DA9063) {
> + ret = mfd_add_devices(da9063->dev, -1, da9063_devs,
> + ARRAY_SIZE(da9063_devs),
> + NULL, da9063->irq_base, NULL);
> + if (ret) {
> + dev_err(da9063->dev, "Cannot add MFD cells\n");
Better to be more general here.
"Failed to add child devices" or such.
Users don't tend to care about MFD cells.
> + goto err_mfd_cleanup;
> + }
> + }
> +
> return ret;
>
> +err_mfd_cleanup:
> + mfd_remove_devices(da9063->dev);
Any reason why you can't use devm_*?
> err_irq_exit:
> da9063_irq_exit(da9063);
> return ret;
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Lee,
On Tue, Jun 5, 2018 at 9:47 AM, Lee Jones <[email protected]> wrote:
> On Sat, 02 Jun 2018, Marek Vasut wrote:
>> While the datasheet for DA9063L (2v1, 23-Mar-2017) lists the RTC register
>> block, the DA9063L does not have an RTC. Add custom IRQ map for DA9063L to
>> ignore the Alarm and Tick IRQs from the PMIC.
>>
>> Signed-off-by: Marek Vasut <[email protected]>
>> --- a/drivers/mfd/da9063-irq.c
>> +++ b/drivers/mfd/da9063-irq.c
>> @@ -74,8 +74,55 @@ static const struct regmap_irq_chip da9063_irq_chip = {
>> .init_ack_masked = true,
>> };
>>
>> +static const struct regmap_irq da9063l_irqs[] = {
>> + /* DA9063 event A register */
>> + REGMAP_IRQ_REG(DA9063_IRQ_ONKEY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ONKEY),
>> + REGMAP_IRQ_REG(DA9063_IRQ_ADC_RDY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ADC_RDY),
>> + REGMAP_IRQ_REG(DA9063_IRQ_SEQ_RDY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_SEQ_RDY),
>> + /* DA9063 event B register */
>> + REGMAP_IRQ_REG(DA9063_IRQ_WAKE, DA9063_REG_EVENT_B_OFFSET, DA9063_M_WAKE),
>> + REGMAP_IRQ_REG(DA9063_IRQ_TEMP, DA9063_REG_EVENT_B_OFFSET, DA9063_M_TEMP),
>> + REGMAP_IRQ_REG(DA9063_IRQ_COMP_1V2, DA9063_REG_EVENT_B_OFFSET, DA9063_M_COMP_1V2),
>> + REGMAP_IRQ_REG(DA9063_IRQ_LDO_LIM, DA9063_REG_EVENT_B_OFFSET, DA9063_M_LDO_LIM),
>> + REGMAP_IRQ_REG(DA9063_IRQ_REG_UVOV, DA9063_REG_EVENT_B_OFFSET, DA9063_M_UVOV),
>> + REGMAP_IRQ_REG(DA9063_IRQ_DVC_RDY, DA9063_REG_EVENT_B_OFFSET, DA9063_M_DVC_RDY),
>> + REGMAP_IRQ_REG(DA9063_IRQ_VDD_MON, DA9063_REG_EVENT_B_OFFSET, DA9063_M_VDD_MON),
>> + REGMAP_IRQ_REG(DA9063_IRQ_WARN, DA9063_REG_EVENT_B_OFFSET, DA9063_M_VDD_WARN),
>> + /* DA9063 event C register */
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI0, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI0),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI1, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI1),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI2, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI2),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI3, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI3),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI4, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI4),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI5, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI5),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI6, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI6),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI7, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI7),
>> + /* DA9063 event D register */
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI8, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI8),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI9, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI9),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI10, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI10),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI11, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI11),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI12, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI12),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI13, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI13),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI14, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI14),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI15, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI15),
>> +};
>
> Same here. Please make checkpatch.pl happen, even if it makes the
> code slightly less readable.
I beg to disagree: source code should be optimized for reading.
Checkpatch is a hinting tool, not an absolute check.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue, 05 Jun 2018, Geert Uytterhoeven wrote:
> Hi Lee,
>
> On Tue, Jun 5, 2018 at 9:09 AM, Lee Jones <[email protected]> wrote:
> > On Mon, 04 Jun 2018, Marek Vasut wrote:
> >> On 06/04/2018 02:26 PM, Lee Jones wrote:
> >> > On Sat, 02 Jun 2018, Marek Vasut wrote:
> >> >> Convert the regmap_irq table to use REGMAP_IRQ_REG().
> >> >>
> >> >> Signed-off-by: Marek Vasut <[email protected]>
> >> >> Cc: Geert Uytterhoeven <[email protected]>
> >> >> Cc: Lee Jones <[email protected]>
> >> >> Cc: Mark Brown <[email protected]>
> >> >> Cc: Steve Twiss <[email protected]>
> >> >> Cc: Wolfram Sang <[email protected]>
> >> >> Cc: [email protected]
> >> >> ---
> >> >> V3: New patch
> >> >> Note: A sed oneliner was used:
> >> >> sed -i "/\[DA9063_IRQ_/ {N;N;N;s/\n//g;s/.*\[\(DA9063_IRQ_[^]]\+\)].*reg_offset = \([^,]\+\),.* = \([^,]\+\),.*/\tREGMAP_IRQ_REG(\1, \2, \3),/}" drivers/mfd/da9063-irq.c
> >> >> ---
> >> >> drivers/mfd/da9063-irq.c | 145 ++++++++++-------------------------------------
> >> >> 1 file changed, 29 insertions(+), 116 deletions(-)
> >> >>
> >> >> diff --git a/drivers/mfd/da9063-irq.c b/drivers/mfd/da9063-irq.c
> >> >> index 207bbfe55449..5b406ecfc14a 100644
> >> >> --- a/drivers/mfd/da9063-irq.c
> >> >> +++ b/drivers/mfd/da9063-irq.c
> >> >> @@ -28,125 +28,38 @@
> >> >>
> >> >> static const struct regmap_irq da9063_irqs[] = {
> >> >> /* DA9063 event A register */
> >> >> - [DA9063_IRQ_ONKEY] = {
> >> >> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> >> >> - .mask = DA9063_M_ONKEY,
> >> >> - },
> >> >> - [DA9063_IRQ_ALARM] = {
> >> >> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> >> >> - .mask = DA9063_M_ALARM,
> >> >> - },
> >> >> - [DA9063_IRQ_TICK] = {
> >> >> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> >> >> - .mask = DA9063_M_TICK,
> >> >> - },
> >> >> - [DA9063_IRQ_ADC_RDY] = {
> >> >> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> >> >> - .mask = DA9063_M_ADC_RDY,
> >> >> - },
> >> >> - [DA9063_IRQ_SEQ_RDY] = {
> >> >> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> >> >> - .mask = DA9063_M_SEQ_RDY,
> >> >> - },
> >> >> + REGMAP_IRQ_REG(DA9063_IRQ_ONKEY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ONKEY),
> >> >> + REGMAP_IRQ_REG(DA9063_IRQ_ALARM, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ALARM),
> >> >> + REGMAP_IRQ_REG(DA9063_IRQ_TICK, DA9063_REG_EVENT_A_OFFSET, DA9063_M_TICK),
> >> >> + REGMAP_IRQ_REG(DA9063_IRQ_ADC_RDY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ADC_RDY),
> >> >> + REGMAP_IRQ_REG(DA9063_IRQ_SEQ_RDY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_SEQ_RDY),
> >> >
> >> > Nice, but I think checkpatch.pl would complain.
> >> >
> >> > Better to break after the first argument I think.
> >>
> >> Doesn't really help the readability, but done.
> >
> > I don't make the rules. :)
>
> Documentation/process/coding-style.rst:
>
> "Statements longer than 80 columns will be broken into sensible chunks, unless
> exceeding 80 columns significantly increases readability and does not hide
> information."
Operative word here is "significantly".
> > Personally /me is hoping that the 80 char limit is lifted to ~120 soon.
>
> Please no ;-)
Maybe 120 is pushing it a little, but 80 is so 19xx? ... well, 80!
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Sat, 02 Jun 2018, Marek Vasut wrote:
> Add support for DA9063L, which is a reduced variant of the DA9063
> with less regulators and without RTC.
>
> Signed-off-by: Marek Vasut <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Lee Jones <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Steve Twiss <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: [email protected]
> Acked-by: Mark Brown <[email protected]>
> Reviewed-by: Geert Uytterhoeven <[email protected]>
> ---
> V2: No change
> V3: No change
> ---
> drivers/mfd/da9063-i2c.c | 2 ++
> 1 file changed, 2 insertions(+)
For my own reference:
Acked-for-MFD-by: Lee Jones <[email protected]>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Sat, 02 Jun 2018, Marek Vasut wrote:
> Move the LDOs present only on DA9063 at the end of the list, so that
> the DA9063L can simply indicate less LDOs and still share the list of
> regulators with DA9063.
>
> Signed-off-by: Marek Vasut <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Lee Jones <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Steve Twiss <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: [email protected]
> Acked-by: Mark Brown <[email protected]>
> Reviewed-by: Geert Uytterhoeven <[email protected]>
> ---
> V2: No change
> V3: No change
> ---
> drivers/regulator/da9063-regulator.c | 76 +++++++++++++++++++++---------------
> 1 file changed, 45 insertions(+), 31 deletions(-)
Nit: Please change the $SUBJECT line.
s/mfd/regulator/
For my own reference:
Acked-for-MFD-by: Lee Jones <[email protected]>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, 05 Jun 2018, Geert Uytterhoeven wrote:
> Hi Lee,
>
> On Tue, Jun 5, 2018 at 9:47 AM, Lee Jones <[email protected]> wrote:
> > On Sat, 02 Jun 2018, Marek Vasut wrote:
> >> While the datasheet for DA9063L (2v1, 23-Mar-2017) lists the RTC register
> >> block, the DA9063L does not have an RTC. Add custom IRQ map for DA9063L to
> >> ignore the Alarm and Tick IRQs from the PMIC.
> >>
> >> Signed-off-by: Marek Vasut <[email protected]>
>
> >> --- a/drivers/mfd/da9063-irq.c
> >> +++ b/drivers/mfd/da9063-irq.c
> >> @@ -74,8 +74,55 @@ static const struct regmap_irq_chip da9063_irq_chip = {
> >> .init_ack_masked = true,
> >> };
> >>
> >> +static const struct regmap_irq da9063l_irqs[] = {
> >> + /* DA9063 event A register */
> >> + REGMAP_IRQ_REG(DA9063_IRQ_ONKEY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ONKEY),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_ADC_RDY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_ADC_RDY),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_SEQ_RDY, DA9063_REG_EVENT_A_OFFSET, DA9063_M_SEQ_RDY),
> >> + /* DA9063 event B register */
> >> + REGMAP_IRQ_REG(DA9063_IRQ_WAKE, DA9063_REG_EVENT_B_OFFSET, DA9063_M_WAKE),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_TEMP, DA9063_REG_EVENT_B_OFFSET, DA9063_M_TEMP),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_COMP_1V2, DA9063_REG_EVENT_B_OFFSET, DA9063_M_COMP_1V2),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_LDO_LIM, DA9063_REG_EVENT_B_OFFSET, DA9063_M_LDO_LIM),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_REG_UVOV, DA9063_REG_EVENT_B_OFFSET, DA9063_M_UVOV),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_DVC_RDY, DA9063_REG_EVENT_B_OFFSET, DA9063_M_DVC_RDY),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_VDD_MON, DA9063_REG_EVENT_B_OFFSET, DA9063_M_VDD_MON),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_WARN, DA9063_REG_EVENT_B_OFFSET, DA9063_M_VDD_WARN),
> >> + /* DA9063 event C register */
> >> + REGMAP_IRQ_REG(DA9063_IRQ_GPI0, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI0),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_GPI1, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI1),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_GPI2, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI2),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_GPI3, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI3),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_GPI4, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI4),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_GPI5, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI5),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_GPI6, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI6),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_GPI7, DA9063_REG_EVENT_C_OFFSET, DA9063_M_GPI7),
> >> + /* DA9063 event D register */
> >> + REGMAP_IRQ_REG(DA9063_IRQ_GPI8, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI8),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_GPI9, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI9),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_GPI10, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI10),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_GPI11, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI11),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_GPI12, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI12),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_GPI13, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI13),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_GPI14, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI14),
> >> + REGMAP_IRQ_REG(DA9063_IRQ_GPI15, DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI15),
> >> +};
> >
> > Same here. Please make checkpatch.pl happen, even if it makes the
> > code slightly less readable.
>
> I beg to disagree: source code should be optimized for reading.
> Checkpatch is a hinting tool, not an absolute check.
I agree with you to some degree, but as per the Coding Standards we
are to use max 80-chars unless it *significantly* reduces
readability.
Breaking lines into two on a natural break only *slightly* reduces
readability, if at all to be frank.
We have MACROS which encompass many arguments and we have to draw the
line somewhere. I choose 80-chars.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 06/05/2018 09:53 AM, Lee Jones wrote:
> On Sat, 02 Jun 2018, Marek Vasut wrote:
>
>> The DA9063L does not contain RTC block, unlike the full DA9063.
>> Split the RTC block into separate mfd cell and register it only
>> on DA9063.
>>
>> Signed-off-by: Marek Vasut <[email protected]>
>> Cc: Geert Uytterhoeven <[email protected]>
>> Cc: Lee Jones <[email protected]>
>> Cc: Mark Brown <[email protected]>
>> Cc: Steve Twiss <[email protected]>
>> Cc: Wolfram Sang <[email protected]>
>> Cc: [email protected]
>> ---
>> V2: No change
>> V3: Rework of mfd: da9063: Disallow RTC on DA9063L
>> ---
>> drivers/mfd/da9063-core.c | 30 +++++++++++++++++++++++-------
>> 1 file changed, 23 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
>> index eebca3442cf3..b05910c797af 100644
>> --- a/drivers/mfd/da9063-core.c
>> +++ b/drivers/mfd/da9063-core.c
>> @@ -76,7 +76,7 @@ static struct resource da9063_hwmon_resources[] = {
>> };
>>
>>
>> -static const struct mfd_cell da9063_devs[] = {
>> +static const struct mfd_cell da9063_common_devs[] = {
>> {
>> .name = DA9063_DRVNAME_REGULATORS,
>
> Appreciate that these are historical, but these device name defines
> make me shudder. They only serve to act as an obfuscation layer when
> debugging at platform level. Please consider getting rid of them.
The macro can be shared between the core and the drivers, so the names
never run out of sync.
>> .num_resources = ARRAY_SIZE(da9063_regulators_resources),
>> @@ -100,15 +100,19 @@ static const struct mfd_cell da9063_devs[] = {
>> .resources = da9063_onkey_resources,
>> .of_compatible = "dlg,da9063-onkey",
>> },
>> + {
>> + .name = DA9063_DRVNAME_VIBRATION,
>> + },
>
> Place this on a single line please.
This would only make the style inconsistent with the ie. LEDs entry.
> { .name = DA9063_DRVNAME_VIBRATION },
>
>> +};
>> +
>> +/* Only present on DA9063 , not on DA9063L */
>> +static const struct mfd_cell da9063_devs[] = {
>> {
>> .name = DA9063_DRVNAME_RTC,
>> .num_resources = ARRAY_SIZE(da9063_rtc_resources),
>> .resources = da9063_rtc_resources,
>> .of_compatible = "dlg,da9063-rtc",
>> },
>> - {
>> - .name = DA9063_DRVNAME_VIBRATION,
>> - },
>> };
>>
>> static int da9063_clear_fault_log(struct da9063 *da9063)
>> @@ -225,16 +229,28 @@ int da9063_device_init(struct da9063 *da9063, unsigned int irq)
>>
>> da9063->irq_base = regmap_irq_chip_get_base(da9063->regmap_irq);
>>
>> - ret = mfd_add_devices(da9063->dev, -1, da9063_devs,
>> - ARRAY_SIZE(da9063_devs), NULL, da9063->irq_base,
>> - NULL);
>> + ret = mfd_add_devices(da9063->dev, -1, da9063_common_devs,
>
> Please consider updating the -1's in this file with the appropriate
> define in a separate patch.
Done
>> + ARRAY_SIZE(da9063_common_devs),
>> + NULL, da9063->irq_base, NULL);
>> if (ret) {
>> dev_err(da9063->dev, "Cannot add MFD cells\n");
>> goto err_irq_exit;
>> }
>>
>> + if (da9063->type == PMIC_TYPE_DA9063) {
>> + ret = mfd_add_devices(da9063->dev, -1, da9063_devs,
>> + ARRAY_SIZE(da9063_devs),
>> + NULL, da9063->irq_base, NULL);
>> + if (ret) {
>> + dev_err(da9063->dev, "Cannot add MFD cells\n");
>
> Better to be more general here.
>
> "Failed to add child devices" or such.
>
> Users don't tend to care about MFD cells.
Hum, done.
>> + goto err_mfd_cleanup;
>> + }
>> + }
>> +
>> return ret;
>>
>> +err_mfd_cleanup:
>> + mfd_remove_devices(da9063->dev);
>
> Any reason why you can't use devm_*?
Because we need to undo the MFD setup before the IRQ setup.
>> err_irq_exit:
>> da9063_irq_exit(da9063);
>> return ret;
>
--
Best regards,
Marek Vasut
On 04 June 2018 19:32, Marek Vasut wrote,
> Subject: Re: [PATCH v3 03/10] mfd: da9063: Rename PMIC_DA9063 to
> PMIC_CHIP_ID_DA9063
>
> On 06/04/2018 02:26 PM, Lee Jones wrote:
> > On Sat, 02 Jun 2018, Marek Vasut wrote:
> >
> >> The PMIC_DA9063 is a complete misnomer, it denotes the value of the
> >> DA9063 chip ID register, so rename it as such. It is also the value
> >> of chip ID register of DA9063L though, so drop the enum as all the
> >> DA9063 "models" share the same chip ID and thus the distinction will
> >> have to be made using DT or otherwise.
> >>
> >> Signed-off-by: Marek Vasut <[email protected]>
> >> Cc: Geert Uytterhoeven <[email protected]>
> >> Cc: Lee Jones <[email protected]>
> >> Cc: Mark Brown <[email protected]>
> >> Cc: Steve Twiss <[email protected]>
> >> Cc: Wolfram Sang <[email protected]>
> >> Cc: [email protected]
> >> Acked-by: Mark Brown <[email protected]>
> >> Reviewed-by: Geert Uytterhoeven <[email protected]>
> >> ---
> >> V2: No change
> >> V3: No change
> >> ---
> >> drivers/mfd/da9063-core.c | 2 +-
> >> drivers/mfd/da9063-i2c.c | 2 +-
> >> drivers/regulator/da9063-regulator.c | 2 +-
> >> include/linux/mfd/da9063/core.h | 4 +---
> >> 4 files changed, 4 insertions(+), 6 deletions(-)
> >
> > For my own reference:
> > Acked-for-MFD-by: Lee Jones <[email protected]>
>
> Thanks.
>
> Any other comments before I submit the next round in a few days?
>
Hi Marek,
Thanks.
I'm reviewing it now.
Could you hold off until tomorrow please?
Regards,
Steve
On 06/05/2018 07:16 PM, Steve Twiss wrote:
>
>
> On 04 June 2018 19:32, Marek Vasut wrote,
>
>> Subject: Re: [PATCH v3 03/10] mfd: da9063: Rename PMIC_DA9063 to
>> PMIC_CHIP_ID_DA9063
>>
>> On 06/04/2018 02:26 PM, Lee Jones wrote:
>>> On Sat, 02 Jun 2018, Marek Vasut wrote:
>>>
>>>> The PMIC_DA9063 is a complete misnomer, it denotes the value of the
>>>> DA9063 chip ID register, so rename it as such. It is also the value
>>>> of chip ID register of DA9063L though, so drop the enum as all the
>>>> DA9063 "models" share the same chip ID and thus the distinction will
>>>> have to be made using DT or otherwise.
>>>>
>>>> Signed-off-by: Marek Vasut <[email protected]>
>>>> Cc: Geert Uytterhoeven <[email protected]>
>>>> Cc: Lee Jones <[email protected]>
>>>> Cc: Mark Brown <[email protected]>
>>>> Cc: Steve Twiss <[email protected]>
>>>> Cc: Wolfram Sang <[email protected]>
>>>> Cc: [email protected]
>>>> Acked-by: Mark Brown <[email protected]>
>>>> Reviewed-by: Geert Uytterhoeven <[email protected]>
>>>> ---
>>>> V2: No change
>>>> V3: No change
>>>> ---
>>>> drivers/mfd/da9063-core.c | 2 +-
>>>> drivers/mfd/da9063-i2c.c | 2 +-
>>>> drivers/regulator/da9063-regulator.c | 2 +-
>>>> include/linux/mfd/da9063/core.h | 4 +---
>>>> 4 files changed, 4 insertions(+), 6 deletions(-)
>>>
>>> For my own reference:
>>> Acked-for-MFD-by: Lee Jones <[email protected]>
>>
>> Thanks.
>>
>> Any other comments before I submit the next round in a few days?
>>
>
> Hi Marek,
>
> Thanks.
> I'm reviewing it now.
> Could you hold off until tomorrow please?
Yeah, why ?
--
Best regards,
Marek Vasut
On 02 June 2018 11:12, Marek Vasut wrote,
> Subject: [PATCH v3 10/10] mfd: da9063: Add DA9063L support
>
> Add support for DA9063L, which is a reduced variant of the DA9063
> with less regulators and without RTC.
>
> Signed-off-by: Marek Vasut <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Lee Jones <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Steve Twiss <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: [email protected]
> Acked-by: Mark Brown <[email protected]>
> Reviewed-by: Geert Uytterhoeven <[email protected]>
> ---
> V2: No change
> V3: No change
> ---
> drivers/mfd/da9063-i2c.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c
> index f0d92a37df6b..74a3b10a7208 100644
> --- a/drivers/mfd/da9063-i2c.c
> +++ b/drivers/mfd/da9063-i2c.c
> @@ -319,6 +319,7 @@ static struct regmap_config da9063_regmap_config = {
>
> static const struct of_device_id da9063_dt_ids[] = {
> { .compatible = "dlg,da9063", },
> + { .compatible = "dlg,da9063l", },
> { }
> };
> MODULE_DEVICE_TABLE(of, da9063_dt_ids);
> @@ -373,6 +374,7 @@ static int da9063_i2c_remove(struct i2c_client *i2c)
>
> static const struct i2c_device_id da9063_i2c_id[] = {
> { "da9063", PMIC_TYPE_DA9063 },
> + { "da9063l", PMIC_TYPE_DA9063L },
> {},
> };
> MODULE_DEVICE_TABLE(i2c, da9063_i2c_id);
> --
> 2.16.2
Hi Marek,
Acked-by: Steve Twiss <[email protected]>
Regards,
Steve
On 02 June 2018 11:12, Marek Vasut wrote,
> Subject: [PATCH v3 02/10] mfd: da9063: Use REGMAP_IRQ_REG
>
> Convert the regmap_irq table to use REGMAP_IRQ_REG().
>
> Signed-off-by: Marek Vasut <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Lee Jones <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Steve Twiss <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: [email protected]
> ---
> V3: New patch
> Note: A sed oneliner was used:
> sed -i "/\[DA9063_IRQ_/
> {N;N;N;s/\n//g;s/.*\[\(DA9063_IRQ_[^]]\+\)].*reg_offset = \([^,]\+\),.* =
> \([^,]\+\),.*/\tREGMAP_IRQ_REG(\1, \2, \3),/}" drivers/mfd/da9063-irq.c
> ---
> drivers/mfd/da9063-irq.c | 145 ++++++++++-------------------------------------
> 1 file changed, 29 insertions(+), 116 deletions(-)
>
> diff --git a/drivers/mfd/da9063-irq.c b/drivers/mfd/da9063-irq.c
> index 207bbfe55449..5b406ecfc14a 100644
> --- a/drivers/mfd/da9063-irq.c
> +++ b/drivers/mfd/da9063-irq.c
> @@ -28,125 +28,38 @@
>
> static const struct regmap_irq da9063_irqs[] = {
> /* DA9063 event A register */
> - [DA9063_IRQ_ONKEY] = {
> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> - .mask = DA9063_M_ONKEY,
> - },
> - [DA9063_IRQ_ALARM] = {
> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> - .mask = DA9063_M_ALARM,
> - },
> - [DA9063_IRQ_TICK] = {
> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> - .mask = DA9063_M_TICK,
> - },
> - [DA9063_IRQ_ADC_RDY] = {
> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> - .mask = DA9063_M_ADC_RDY,
> - },
> - [DA9063_IRQ_SEQ_RDY] = {
> - .reg_offset = DA9063_REG_EVENT_A_OFFSET,
> - .mask = DA9063_M_SEQ_RDY,
> - },
> + REGMAP_IRQ_REG(DA9063_IRQ_ONKEY,
> DA9063_REG_EVENT_A_OFFSET, DA9063_M_ONKEY),
> + REGMAP_IRQ_REG(DA9063_IRQ_ALARM,
> DA9063_REG_EVENT_A_OFFSET, DA9063_M_ALARM),
> + REGMAP_IRQ_REG(DA9063_IRQ_TICK, DA9063_REG_EVENT_A_OFFSET,
> DA9063_M_TICK),
> + REGMAP_IRQ_REG(DA9063_IRQ_ADC_RDY,
> DA9063_REG_EVENT_A_OFFSET, DA9063_M_ADC_RDY),
> + REGMAP_IRQ_REG(DA9063_IRQ_SEQ_RDY,
> DA9063_REG_EVENT_A_OFFSET, DA9063_M_SEQ_RDY),
> /* DA9063 event B register */
> - [DA9063_IRQ_WAKE] = {
> - .reg_offset = DA9063_REG_EVENT_B_OFFSET,
> - .mask = DA9063_M_WAKE,
> - },
> - [DA9063_IRQ_TEMP] = {
> - .reg_offset = DA9063_REG_EVENT_B_OFFSET,
> - .mask = DA9063_M_TEMP,
> - },
> - [DA9063_IRQ_COMP_1V2] = {
> - .reg_offset = DA9063_REG_EVENT_B_OFFSET,
> - .mask = DA9063_M_COMP_1V2,
> - },
> - [DA9063_IRQ_LDO_LIM] = {
> - .reg_offset = DA9063_REG_EVENT_B_OFFSET,
> - .mask = DA9063_M_LDO_LIM,
> - },
> - [DA9063_IRQ_REG_UVOV] = {
> - .reg_offset = DA9063_REG_EVENT_B_OFFSET,
> - .mask = DA9063_M_UVOV,
> - },
> - [DA9063_IRQ_DVC_RDY] = {
> - .reg_offset = DA9063_REG_EVENT_B_OFFSET,
> - .mask = DA9063_M_DVC_RDY,
> - },
> - [DA9063_IRQ_VDD_MON] = {
> - .reg_offset = DA9063_REG_EVENT_B_OFFSET,
> - .mask = DA9063_M_VDD_MON,
> - },
> - [DA9063_IRQ_WARN] = {
> - .reg_offset = DA9063_REG_EVENT_B_OFFSET,
> - .mask = DA9063_M_VDD_WARN,
> - },
> + REGMAP_IRQ_REG(DA9063_IRQ_WAKE,
> DA9063_REG_EVENT_B_OFFSET, DA9063_M_WAKE),
> + REGMAP_IRQ_REG(DA9063_IRQ_TEMP,
> DA9063_REG_EVENT_B_OFFSET, DA9063_M_TEMP),
> + REGMAP_IRQ_REG(DA9063_IRQ_COMP_1V2,
> DA9063_REG_EVENT_B_OFFSET, DA9063_M_COMP_1V2),
> + REGMAP_IRQ_REG(DA9063_IRQ_LDO_LIM,
> DA9063_REG_EVENT_B_OFFSET, DA9063_M_LDO_LIM),
> + REGMAP_IRQ_REG(DA9063_IRQ_REG_UVOV,
> DA9063_REG_EVENT_B_OFFSET, DA9063_M_UVOV),
> + REGMAP_IRQ_REG(DA9063_IRQ_DVC_RDY,
> DA9063_REG_EVENT_B_OFFSET, DA9063_M_DVC_RDY),
> + REGMAP_IRQ_REG(DA9063_IRQ_VDD_MON,
> DA9063_REG_EVENT_B_OFFSET, DA9063_M_VDD_MON),
> + REGMAP_IRQ_REG(DA9063_IRQ_WARN,
> DA9063_REG_EVENT_B_OFFSET, DA9063_M_VDD_WARN),
> /* DA9063 event C register */
> - [DA9063_IRQ_GPI0] = {
> - .reg_offset = DA9063_REG_EVENT_C_OFFSET,
> - .mask = DA9063_M_GPI0,
> - },
> - [DA9063_IRQ_GPI1] = {
> - .reg_offset = DA9063_REG_EVENT_C_OFFSET,
> - .mask = DA9063_M_GPI1,
> - },
> - [DA9063_IRQ_GPI2] = {
> - .reg_offset = DA9063_REG_EVENT_C_OFFSET,
> - .mask = DA9063_M_GPI2,
> - },
> - [DA9063_IRQ_GPI3] = {
> - .reg_offset = DA9063_REG_EVENT_C_OFFSET,
> - .mask = DA9063_M_GPI3,
> - },
> - [DA9063_IRQ_GPI4] = {
> - .reg_offset = DA9063_REG_EVENT_C_OFFSET,
> - .mask = DA9063_M_GPI4,
> - },
> - [DA9063_IRQ_GPI5] = {
> - .reg_offset = DA9063_REG_EVENT_C_OFFSET,
> - .mask = DA9063_M_GPI5,
> - },
> - [DA9063_IRQ_GPI6] = {
> - .reg_offset = DA9063_REG_EVENT_C_OFFSET,
> - .mask = DA9063_M_GPI6,
> - },
> - [DA9063_IRQ_GPI7] = {
> - .reg_offset = DA9063_REG_EVENT_C_OFFSET,
> - .mask = DA9063_M_GPI7,
> - },
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI0, DA9063_REG_EVENT_C_OFFSET,
> DA9063_M_GPI0),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI1, DA9063_REG_EVENT_C_OFFSET,
> DA9063_M_GPI1),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI2, DA9063_REG_EVENT_C_OFFSET,
> DA9063_M_GPI2),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI3, DA9063_REG_EVENT_C_OFFSET,
> DA9063_M_GPI3),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI4, DA9063_REG_EVENT_C_OFFSET,
> DA9063_M_GPI4),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI5, DA9063_REG_EVENT_C_OFFSET,
> DA9063_M_GPI5),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI6, DA9063_REG_EVENT_C_OFFSET,
> DA9063_M_GPI6),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI7, DA9063_REG_EVENT_C_OFFSET,
> DA9063_M_GPI7),
> /* DA9063 event D register */
> - [DA9063_IRQ_GPI8] = {
> - .reg_offset = DA9063_REG_EVENT_D_OFFSET,
> - .mask = DA9063_M_GPI8,
> - },
> - [DA9063_IRQ_GPI9] = {
> - .reg_offset = DA9063_REG_EVENT_D_OFFSET,
> - .mask = DA9063_M_GPI9,
> - },
> - [DA9063_IRQ_GPI10] = {
> - .reg_offset = DA9063_REG_EVENT_D_OFFSET,
> - .mask = DA9063_M_GPI10,
> - },
> - [DA9063_IRQ_GPI11] = {
> - .reg_offset = DA9063_REG_EVENT_D_OFFSET,
> - .mask = DA9063_M_GPI11,
> - },
> - [DA9063_IRQ_GPI12] = {
> - .reg_offset = DA9063_REG_EVENT_D_OFFSET,
> - .mask = DA9063_M_GPI12,
> - },
> - [DA9063_IRQ_GPI13] = {
> - .reg_offset = DA9063_REG_EVENT_D_OFFSET,
> - .mask = DA9063_M_GPI13,
> - },
> - [DA9063_IRQ_GPI14] = {
> - .reg_offset = DA9063_REG_EVENT_D_OFFSET,
> - .mask = DA9063_M_GPI14,
> - },
> - [DA9063_IRQ_GPI15] = {
> - .reg_offset = DA9063_REG_EVENT_D_OFFSET,
> - .mask = DA9063_M_GPI15,
> - },
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI8, DA9063_REG_EVENT_D_OFFSET,
> DA9063_M_GPI8),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI9, DA9063_REG_EVENT_D_OFFSET,
> DA9063_M_GPI9),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI10,
> DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI10),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI11,
> DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI11),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI12,
> DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI12),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI13,
> DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI13),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI14,
> DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI14),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI15,
> DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI15),
> };
>
> static const struct regmap_irq_chip da9063_irq_chip = {
> --
> 2.16.2
Hi Marek,
Acked-by: Steve Twiss <[email protected]>
Regards,
Steve
On 02 June 2018 11:12, Marek Vasut wrote,
> Subject: [PATCH v3 03/10] mfd: da9063: Rename PMIC_DA9063 to
> PMIC_CHIP_ID_DA9063
>
> The PMIC_DA9063 is a complete misnomer, it denotes the value of the
> DA9063 chip ID register, so rename it as such. It is also the value
> of chip ID register of DA9063L though, so drop the enum as all the
> DA9063 "models" share the same chip ID and thus the distinction will
> have to be made using DT or otherwise.
>
> Signed-off-by: Marek Vasut <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Lee Jones <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Steve Twiss <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: [email protected]
> Acked-by: Mark Brown <[email protected]>
> Reviewed-by: Geert Uytterhoeven <[email protected]>
> ---
> V2: No change
> V3: No change
> ---
> drivers/mfd/da9063-core.c | 2 +-
> drivers/mfd/da9063-i2c.c | 2 +-
> drivers/regulator/da9063-regulator.c | 2 +-
> include/linux/mfd/da9063/core.h | 4 +---
> 4 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
> index 8226ebd8b96d..fb122316c421 100644
> --- a/drivers/mfd/da9063-core.c
> +++ b/drivers/mfd/da9063-core.c
> @@ -192,7 +192,7 @@ int da9063_device_init(struct da9063 *da9063, unsigned
> int irq)
> dev_err(da9063->dev, "Cannot read chip model id.\n");
> return -EIO;
> }
> - if (model != PMIC_DA9063) {
> + if (model != PMIC_CHIP_ID_DA9063) {
> dev_err(da9063->dev, "Invalid chip model id: 0x%02x\n", model);
> return -ENODEV;
> }
> diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c
> index 981805a2c521..7f84030c8d53 100644
> --- a/drivers/mfd/da9063-i2c.c
> +++ b/drivers/mfd/da9063-i2c.c
> @@ -280,7 +280,7 @@ static int da9063_i2c_remove(struct i2c_client *i2c)
> }
>
> static const struct i2c_device_id da9063_i2c_id[] = {
> - {"da9063", PMIC_DA9063},
> + { "da9063", PMIC_CHIP_ID_DA9063 },
> {},
> };
> MODULE_DEVICE_TABLE(i2c, da9063_i2c_id);
> diff --git a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-
> regulator.c
> index 6a8f9cd69f52..87c884ae0064 100644
> --- a/drivers/regulator/da9063-regulator.c
> +++ b/drivers/regulator/da9063-regulator.c
> @@ -585,7 +585,7 @@ static struct da9063_dev_model regulators_models[] = {
> {
> .regulator_info = da9063_regulator_info,
> .n_regulators = ARRAY_SIZE(da9063_regulator_info),
> - .dev_model = PMIC_DA9063,
> + .dev_model = PMIC_CHIP_ID_DA9063,
> },
> { }
> };
> diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
> index f3ae65db4c86..664f650d0086 100644
> --- a/include/linux/mfd/da9063/core.h
> +++ b/include/linux/mfd/da9063/core.h
> @@ -29,9 +29,7 @@
> #define DA9063_DRVNAME_RTC "da9063-rtc"
> #define DA9063_DRVNAME_VIBRATION "da9063-vibration"
>
> -enum da9063_models {
> - PMIC_DA9063 = 0x61,
> -};
> +#define PMIC_CHIP_ID_DA9063 0x61
>
> enum da9063_variant_codes {
> PMIC_DA9063_AD = 0x3,
> --
> 2.16.2
Hi Marek,
Acked-by: Steve Twiss <[email protected]>
Regards,
Stephen
On 02 June 2018 11:12, Marek Vasut wrote,
> Subject: [PATCH v3 05/10] mfd: da9063: Add DA9063L type
>
> Add type for DA9063L, which is a reduced variant of the DA9063
> without RTC block and with less regulators.
>
> Signed-off-by: Marek Vasut <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Lee Jones <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Steve Twiss <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: [email protected]
> Reviewed-by: Geert Uytterhoeven <[email protected]>
> ---
> V2: No change
> V3: No change
> ---
> include/linux/mfd/da9063/core.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
> index eb234582dcb2..c3e917dd1860 100644
> --- a/include/linux/mfd/da9063/core.h
> +++ b/include/linux/mfd/da9063/core.h
> @@ -33,6 +33,7 @@
>
> enum da9063_type {
> PMIC_TYPE_DA9063 = 0,
> + PMIC_TYPE_DA9063L,
> };
>
> enum da9063_variant_codes {
> --
> 2.16.2
Acked-by: Steve Twiss <[email protected]>
Hi Marek,
Thanks.
On 02 June 2018 11:12, Marek Vasut wrote,
> Subject: [PATCH v3 07/10] mfd: da9063: Add custom IRQ map for DA9063L
>
> While the datasheet for DA9063L (2v1, 23-Mar-2017) lists the RTC register
> block, the DA9063L does not have an RTC. Add custom IRQ map for DA9063L to
> ignore the Alarm and Tick IRQs from the PMIC.
I've added a request to remove the RTC references from the DA9063L datasheet.
Adding that first part to the sentence in the commit log: "While the datasheet for DA9063L
(2v1, 23-Mar-2017) lists the RTC register block" is just pointing out an ambiguity from the
datasheet since the it also identifies those registers in Table 102 on page 126 as
"Reserved".
I would like to suggest, the commit log entry: "Add custom IRQ map for DA9063L to ignore
the Alarm and Tick IRQs from the PMIC".
> Signed-off-by: Marek Vasut <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Lee Jones <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Steve Twiss <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: [email protected]
> ---
> V3: New patch
> ---
> drivers/mfd/da9063-irq.c | 55
> ++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 53 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mfd/da9063-irq.c b/drivers/mfd/da9063-irq.c
> index 5b406ecfc14a..b6a88861cc2e 100644
> --- a/drivers/mfd/da9063-irq.c
> +++ b/drivers/mfd/da9063-irq.c
> @@ -74,8 +74,55 @@ static const struct regmap_irq_chip da9063_irq_chip = {
> .init_ack_masked = true,
> };
>
> +static const struct regmap_irq da9063l_irqs[] = {
> + /* DA9063 event A register */
> + REGMAP_IRQ_REG(DA9063_IRQ_ONKEY,
> DA9063_REG_EVENT_A_OFFSET, DA9063_M_ONKEY),
> + REGMAP_IRQ_REG(DA9063_IRQ_ADC_RDY,
> DA9063_REG_EVENT_A_OFFSET, DA9063_M_ADC_RDY),
> + REGMAP_IRQ_REG(DA9063_IRQ_SEQ_RDY,
> DA9063_REG_EVENT_A_OFFSET, DA9063_M_SEQ_RDY),
> + /* DA9063 event B register */
> + REGMAP_IRQ_REG(DA9063_IRQ_WAKE,
> DA9063_REG_EVENT_B_OFFSET, DA9063_M_WAKE),
> + REGMAP_IRQ_REG(DA9063_IRQ_TEMP,
> DA9063_REG_EVENT_B_OFFSET, DA9063_M_TEMP),
> + REGMAP_IRQ_REG(DA9063_IRQ_COMP_1V2,
> DA9063_REG_EVENT_B_OFFSET, DA9063_M_COMP_1V2),
> + REGMAP_IRQ_REG(DA9063_IRQ_LDO_LIM,
> DA9063_REG_EVENT_B_OFFSET, DA9063_M_LDO_LIM),
> + REGMAP_IRQ_REG(DA9063_IRQ_REG_UVOV,
> DA9063_REG_EVENT_B_OFFSET, DA9063_M_UVOV),
> + REGMAP_IRQ_REG(DA9063_IRQ_DVC_RDY,
> DA9063_REG_EVENT_B_OFFSET, DA9063_M_DVC_RDY),
> + REGMAP_IRQ_REG(DA9063_IRQ_VDD_MON,
> DA9063_REG_EVENT_B_OFFSET, DA9063_M_VDD_MON),
> + REGMAP_IRQ_REG(DA9063_IRQ_WARN,
> DA9063_REG_EVENT_B_OFFSET, DA9063_M_VDD_WARN),
> + /* DA9063 event C register */
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI0, DA9063_REG_EVENT_C_OFFSET,
> DA9063_M_GPI0),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI1, DA9063_REG_EVENT_C_OFFSET,
> DA9063_M_GPI1),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI2, DA9063_REG_EVENT_C_OFFSET,
> DA9063_M_GPI2),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI3, DA9063_REG_EVENT_C_OFFSET,
> DA9063_M_GPI3),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI4, DA9063_REG_EVENT_C_OFFSET,
> DA9063_M_GPI4),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI5, DA9063_REG_EVENT_C_OFFSET,
> DA9063_M_GPI5),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI6, DA9063_REG_EVENT_C_OFFSET,
> DA9063_M_GPI6),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI7, DA9063_REG_EVENT_C_OFFSET,
> DA9063_M_GPI7),
> + /* DA9063 event D register */
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI8, DA9063_REG_EVENT_D_OFFSET,
> DA9063_M_GPI8),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI9, DA9063_REG_EVENT_D_OFFSET,
> DA9063_M_GPI9),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI10,
> DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI10),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI11,
> DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI11),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI12,
> DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI12),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI13,
> DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI13),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI14,
> DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI14),
> + REGMAP_IRQ_REG(DA9063_IRQ_GPI15,
> DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI15),
> +};
> +
> +static const struct regmap_irq_chip da9063l_irq_chip = {
> + .name = "da9063l-irq",
> + .irqs = da9063l_irqs,
> + .num_irqs = DA9063_NUM_IRQ,
Isn't the number of IRQs in the DA9063L different to the standard DA9063
because of the missing RTC?
Regards,
Steve
> +
> + .num_regs = 4,
> + .status_base = DA9063_REG_EVENT_A,
> + .mask_base = DA9063_REG_IRQ_MASK_A,
> + .ack_base = DA9063_REG_EVENT_A,
> + .init_ack_masked = true,
> +};
> +
> int da9063_irq_init(struct da9063 *da9063)
> {
> + struct regmap_irq_chip *irq_chip;
> int ret;
>
> if (!da9063->chip_irq) {
> @@ -83,10 +130,14 @@ int da9063_irq_init(struct da9063 *da9063)
> return -EINVAL;
> }
>
> + if (da9063->type == PMIC_TYPE_DA9063)
> + irq_chip = &da9063_irq_chip;
> + else
> + irq_chip = &da9063l_irq_chip;
> +
> ret = regmap_add_irq_chip(da9063->regmap, da9063->chip_irq,
> IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED,
> - da9063->irq_base, &da9063_irq_chip,
> - &da9063->regmap_irq);
> + da9063->irq_base, irq_chip, &da9063->regmap_irq);
> if (ret) {
> dev_err(da9063->dev, "Failed to reguest IRQ %d: %d\n",
> da9063->chip_irq, ret);
> --
> 2.16.2
Hi Marek and Geert,
On 04 June 2018 17:25 Marek Vasut wrote,
> Subject: Re: [PATCH v3 06/10] mfd: da9063: Add custom regmap for DA9063L
>
> On 06/04/2018 09:39 AM, Geert Uytterhoeven wrote:
> > Hi Marek, Steve,
> >
> > On Sat, Jun 2, 2018 at 12:11 PM, Marek Vasut <[email protected]> wrote:
> >> While the datasheet for DA9063L (2v1, 23-Mar-2017) lists the RTC register
> >> block, the DA9063L does not have an RTC. Add custom regmap for DA9063L to
> >> prevent access into that register block.
Ok. I've said previously in [v3 07/10], but I'll copy again:
There is now an internal Dialog request to remove the RTC references from the DA9063L datasheet.
Adding that first part to the sentence in the commit log: "While the datasheet for DA9063L
(2v1, 23-Mar-2017) lists the RTC register block" -- it exists in error for the register map table
on page 91, but the datasheet also identifies those registers in Table 102 on page 126 as
"Reserved".
Pointing out the ambiguity in this version of the datasheet seems redundant in the commit log.
Also Dialog do not store a history of Datasheets on their website so once this is updated (although
this update is not in my hands) the datasheet will be replaced. So, it seems this comment could
make the commit message just as misleading as the current datasheet.
How about something simpler?
"The DA9063L does not have an RTC. Add custom regmap for DA9063L to prevent access
into that register block."
> >>
> >> Signed-off-by: Marek Vasut <[email protected]>
> >
> > Thanks for your patch!
> >
> >> --- a/drivers/mfd/da9063-i2c.c
> >> +++ b/drivers/mfd/da9063-i2c.c
> >> @@ -254,6 +341,10 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
> >
> > Note that the line above doesn't check da9063->type, but da9063-
> >variant_code...
> >
> >> 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) {
> >
> > ... so this may be slightly confusing.
>
> I know.
>
> >> + 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;
> >
> > However, da9063->variant_code doesn't seem to have been filled in at this
> > point yet (the call to da9063_device_init() doing so is below, at the end
> > of the probe function!), so commit 9cb42e2a8ed06e91 ("mfd: da9063: Add
> > support for AD silicon variant") never actually handled the AD silicon variant
> > correctly? Or am I missing something?
Okay ... No. You're not missing anything. I had noticed that.
The AD chip model is not referenced and by default only the BB chip model is used.
> Ha, that is a good point.
Yeah, it's a good point, but it's not an amusing point.
The device tree only distinguishes a "dlg,da9063", there is no AD type in the DT schema.
There is no datasheet listing AD registers supported by Dialog, only BB.
But, AD registers were added back into the header file in commit 9cb42e2a8ed06e91
and the RTC driver was updated to distinguish between the AD and BB according to
the type of variant detected at run-time during the da9063_device_init() call.
The real problem is that this leads to two competing chip detection methods for the
DA9063. The function da9063_device_init() autodetects the chip variant, but
autodetection cannot define the chip model. It's circular: the chip model cannot be
autodetected because a chip model is needed to access the register used during
autodetection.
Which leads me back to what I said two paragraphs up:
> The device tree only distinguishes a "dlg,da9063", there is no AD type in the DT schema.
> There is no datasheet listing AD registers supported by Dialog, only BB.
This is not how it is done in the DA9062 and DA9061 driver: the variant code is only
used to print the information to the console during start-up and it is the DT that defines
the chip model based upon "dlg,da9062" or "dlg,da9061".
Steve
On 06/05/2018 09:52 PM, Steve Twiss wrote:
> Hi Marek,
Hi,
> Thanks.
>
> On 02 June 2018 11:12, Marek Vasut wrote,
>
>> Subject: [PATCH v3 07/10] mfd: da9063: Add custom IRQ map for DA9063L
>>
>> While the datasheet for DA9063L (2v1, 23-Mar-2017) lists the RTC register
>> block, the DA9063L does not have an RTC. Add custom IRQ map for DA9063L to
>> ignore the Alarm and Tick IRQs from the PMIC.
>
> I've added a request to remove the RTC references from the DA9063L datasheet.
>
> Adding that first part to the sentence in the commit log: "While the datasheet for DA9063L
> (2v1, 23-Mar-2017) lists the RTC register block" is just pointing out an ambiguity from the
> datasheet since the it also identifies those registers in Table 102 on page 126 as
> "Reserved".
>
> I would like to suggest, the commit log entry: "Add custom IRQ map for DA9063L to ignore
> the Alarm and Tick IRQs from the PMIC".
OK, I'll hide it.
>> Signed-off-by: Marek Vasut <[email protected]>
>> Cc: Geert Uytterhoeven <[email protected]>
>> Cc: Lee Jones <[email protected]>
>> Cc: Mark Brown <[email protected]>
>> Cc: Steve Twiss <[email protected]>
>> Cc: Wolfram Sang <[email protected]>
>> Cc: [email protected]
>> ---
>> V3: New patch
>> ---
>> drivers/mfd/da9063-irq.c | 55
>> ++++++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 53 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mfd/da9063-irq.c b/drivers/mfd/da9063-irq.c
>> index 5b406ecfc14a..b6a88861cc2e 100644
>> --- a/drivers/mfd/da9063-irq.c
>> +++ b/drivers/mfd/da9063-irq.c
>> @@ -74,8 +74,55 @@ static const struct regmap_irq_chip da9063_irq_chip = {
>> .init_ack_masked = true,
>> };
>>
>> +static const struct regmap_irq da9063l_irqs[] = {
>> + /* DA9063 event A register */
>> + REGMAP_IRQ_REG(DA9063_IRQ_ONKEY,
>> DA9063_REG_EVENT_A_OFFSET, DA9063_M_ONKEY),
>> + REGMAP_IRQ_REG(DA9063_IRQ_ADC_RDY,
>> DA9063_REG_EVENT_A_OFFSET, DA9063_M_ADC_RDY),
>> + REGMAP_IRQ_REG(DA9063_IRQ_SEQ_RDY,
>> DA9063_REG_EVENT_A_OFFSET, DA9063_M_SEQ_RDY),
>> + /* DA9063 event B register */
>> + REGMAP_IRQ_REG(DA9063_IRQ_WAKE,
>> DA9063_REG_EVENT_B_OFFSET, DA9063_M_WAKE),
>> + REGMAP_IRQ_REG(DA9063_IRQ_TEMP,
>> DA9063_REG_EVENT_B_OFFSET, DA9063_M_TEMP),
>> + REGMAP_IRQ_REG(DA9063_IRQ_COMP_1V2,
>> DA9063_REG_EVENT_B_OFFSET, DA9063_M_COMP_1V2),
>> + REGMAP_IRQ_REG(DA9063_IRQ_LDO_LIM,
>> DA9063_REG_EVENT_B_OFFSET, DA9063_M_LDO_LIM),
>> + REGMAP_IRQ_REG(DA9063_IRQ_REG_UVOV,
>> DA9063_REG_EVENT_B_OFFSET, DA9063_M_UVOV),
>> + REGMAP_IRQ_REG(DA9063_IRQ_DVC_RDY,
>> DA9063_REG_EVENT_B_OFFSET, DA9063_M_DVC_RDY),
>> + REGMAP_IRQ_REG(DA9063_IRQ_VDD_MON,
>> DA9063_REG_EVENT_B_OFFSET, DA9063_M_VDD_MON),
>> + REGMAP_IRQ_REG(DA9063_IRQ_WARN,
>> DA9063_REG_EVENT_B_OFFSET, DA9063_M_VDD_WARN),
>> + /* DA9063 event C register */
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI0, DA9063_REG_EVENT_C_OFFSET,
>> DA9063_M_GPI0),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI1, DA9063_REG_EVENT_C_OFFSET,
>> DA9063_M_GPI1),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI2, DA9063_REG_EVENT_C_OFFSET,
>> DA9063_M_GPI2),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI3, DA9063_REG_EVENT_C_OFFSET,
>> DA9063_M_GPI3),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI4, DA9063_REG_EVENT_C_OFFSET,
>> DA9063_M_GPI4),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI5, DA9063_REG_EVENT_C_OFFSET,
>> DA9063_M_GPI5),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI6, DA9063_REG_EVENT_C_OFFSET,
>> DA9063_M_GPI6),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI7, DA9063_REG_EVENT_C_OFFSET,
>> DA9063_M_GPI7),
>> + /* DA9063 event D register */
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI8, DA9063_REG_EVENT_D_OFFSET,
>> DA9063_M_GPI8),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI9, DA9063_REG_EVENT_D_OFFSET,
>> DA9063_M_GPI9),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI10,
>> DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI10),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI11,
>> DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI11),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI12,
>> DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI12),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI13,
>> DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI13),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI14,
>> DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI14),
>> + REGMAP_IRQ_REG(DA9063_IRQ_GPI15,
>> DA9063_REG_EVENT_D_OFFSET, DA9063_M_GPI15),
>> +};
>> +
>> +static const struct regmap_irq_chip da9063l_irq_chip = {
>> + .name = "da9063l-irq",
>> + .irqs = da9063l_irqs,
>> + .num_irqs = DA9063_NUM_IRQ,
>
> Isn't the number of IRQs in the DA9063L different to the standard DA9063
> because of the missing RTC?
I think this can even be replaced by ARRAY_SIZE() ?
--
Best regards,
Marek Vasut
On 06/05/2018 10:17 PM, Steve Twiss wrote:
> Hi Marek and Geert,
>
> On 04 June 2018 17:25 Marek Vasut wrote,
>
>> Subject: Re: [PATCH v3 06/10] mfd: da9063: Add custom regmap for DA9063L
>>
>> On 06/04/2018 09:39 AM, Geert Uytterhoeven wrote:
>>> Hi Marek, Steve,
>>>
>>> On Sat, Jun 2, 2018 at 12:11 PM, Marek Vasut <[email protected]> wrote:
>>>> While the datasheet for DA9063L (2v1, 23-Mar-2017) lists the RTC register
>>>> block, the DA9063L does not have an RTC. Add custom regmap for DA9063L to
>>>> prevent access into that register block.
>
> Ok. I've said previously in [v3 07/10], but I'll copy again:
> There is now an internal Dialog request to remove the RTC references from the DA9063L datasheet.
> Adding that first part to the sentence in the commit log: "While the datasheet for DA9063L
> (2v1, 23-Mar-2017) lists the RTC register block" -- it exists in error for the register map table
> on page 91, but the datasheet also identifies those registers in Table 102 on page 126 as
> "Reserved".
>
> Pointing out the ambiguity in this version of the datasheet seems redundant in the commit log.
> Also Dialog do not store a history of Datasheets on their website so once this is updated (although
> this update is not in my hands) the datasheet will be replaced. So, it seems this comment could
> make the commit message just as misleading as the current datasheet.
>
> How about something simpler?
> "The DA9063L does not have an RTC. Add custom regmap for DA9063L to prevent access
> into that register block."
>
>>>>
>>>> Signed-off-by: Marek Vasut <[email protected]>
>>>
>>> Thanks for your patch!
>>>
>>>> --- a/drivers/mfd/da9063-i2c.c
>>>> +++ b/drivers/mfd/da9063-i2c.c
>>>> @@ -254,6 +341,10 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
>>>
>>> Note that the line above doesn't check da9063->type, but da9063-
>>> variant_code...
>>>
>>>> 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) {
>>>
>>> ... so this may be slightly confusing.
>>
>> I know.
>>
>>>> + 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;
>>>
>>> However, da9063->variant_code doesn't seem to have been filled in at this
>>> point yet (the call to da9063_device_init() doing so is below, at the end
>>> of the probe function!), so commit 9cb42e2a8ed06e91 ("mfd: da9063: Add
>>> support for AD silicon variant") never actually handled the AD silicon variant
>>> correctly? Or am I missing something?
>
> Okay ... No. You're not missing anything. I had noticed that.
> The AD chip model is not referenced and by default only the BB chip model is used.
>
>> Ha, that is a good point.
>
> Yeah, it's a good point, but it's not an amusing point.
> The device tree only distinguishes a "dlg,da9063", there is no AD type in the DT schema.
> There is no datasheet listing AD registers supported by Dialog, only BB.
>
> But, AD registers were added back into the header file in commit 9cb42e2a8ed06e91
> and the RTC driver was updated to distinguish between the AD and BB according to
> the type of variant detected at run-time during the da9063_device_init() call.
>
> The real problem is that this leads to two competing chip detection methods for the
> DA9063. The function da9063_device_init() autodetects the chip variant, but
> autodetection cannot define the chip model. It's circular: the chip model cannot be
> autodetected because a chip model is needed to access the register used during
> autodetection.
>
> Which leads me back to what I said two paragraphs up:
>> The device tree only distinguishes a "dlg,da9063", there is no AD type in the DT schema.
>> There is no datasheet listing AD registers supported by Dialog, only BB.
>
> This is not how it is done in the DA9062 and DA9061 driver: the variant code is only
> used to print the information to the console during start-up and it is the DT that defines
> the chip model based upon "dlg,da9062" or "dlg,da9061".
So the AD was broken since forever and noone noticed ? :)
Do you have an AD hardware and can you fix it ?
--
Best regards,
Marek Vasut
On Tue, 05 Jun 2018, Marek Vasut wrote:
> On 06/05/2018 09:53 AM, Lee Jones wrote:
> > On Sat, 02 Jun 2018, Marek Vasut wrote:
> >
> >> The DA9063L does not contain RTC block, unlike the full DA9063.
> >> Split the RTC block into separate mfd cell and register it only
> >> on DA9063.
> >>
> >> Signed-off-by: Marek Vasut <[email protected]>
> >> Cc: Geert Uytterhoeven <[email protected]>
> >> Cc: Lee Jones <[email protected]>
> >> Cc: Mark Brown <[email protected]>
> >> Cc: Steve Twiss <[email protected]>
> >> Cc: Wolfram Sang <[email protected]>
> >> Cc: [email protected]
> >> ---
> >> V2: No change
> >> V3: Rework of mfd: da9063: Disallow RTC on DA9063L
> >> ---
> >> drivers/mfd/da9063-core.c | 30 +++++++++++++++++++++++-------
> >> 1 file changed, 23 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
> >> index eebca3442cf3..b05910c797af 100644
> >> --- a/drivers/mfd/da9063-core.c
> >> +++ b/drivers/mfd/da9063-core.c
> >> @@ -76,7 +76,7 @@ static struct resource da9063_hwmon_resources[] = {
> >> };
> >>
> >>
> >> -static const struct mfd_cell da9063_devs[] = {
> >> +static const struct mfd_cell da9063_common_devs[] = {
> >> {
> >> .name = DA9063_DRVNAME_REGULATORS,
> >
> > Appreciate that these are historical, but these device name defines
> > make me shudder. They only serve to act as an obfuscation layer when
> > debugging at platform level. Please consider getting rid of them.
>
> The macro can be shared between the core and the drivers, so the names
> never run out of sync.
Platform driver name changes are vary rare. Even if they are changed,
even light testing would uncover the fact that child drivers do not
.probe(). Due to the current obfuscation, I currently have no idea
what this device's name is. This technique is not allowed for new
drivers - unfortunately I didn't not review this driver in the first
instance.
It doesn't bother me enough to go and change it myself and I'm not
going to have a baby over patches not being submitted to fix it.
> >> .num_resources = ARRAY_SIZE(da9063_regulators_resources),
> >> @@ -100,15 +100,19 @@ static const struct mfd_cell da9063_devs[] = {
> >> .resources = da9063_onkey_resources,
> >> .of_compatible = "dlg,da9063-onkey",
> >> },
> >> + {
> >> + .name = DA9063_DRVNAME_VIBRATION,
> >> + },
> >
> > Place this on a single line please.
>
> This would only make the style inconsistent with the ie. LEDs entry.
>
> > { .name = DA9063_DRVNAME_VIBRATION },
If that is a one line entry spaced over multiple lines, then that
should also be changed.
Maybe I will go through and stylise this driver a bit after all (but
as time is short at the moment, maybe not!) :)
[...]
> >> +err_mfd_cleanup:
> >> + mfd_remove_devices(da9063->dev);
> >
> > Any reason why you can't use devm_*?
>
> Because we need to undo the MFD setup before the IRQ setup.
Sounds like a good enough reason.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Marek and Geert,
On 06 June 2018 00:02 Marek Vasut wrote,
> Subject: Re: [PATCH v3 06/10] mfd: da9063: Add custom regmap for DA9063L
>
> On 06/05/2018 10:17 PM, Steve Twiss wrote:
> > Hi Marek and Geert,
> >
> > On 04 June 2018 17:25 Marek Vasut wrote,
> >
> >> Subject: Re: [PATCH v3 06/10] mfd: da9063: Add custom regmap for DA9063L
> >>
> >> On 06/04/2018 09:39 AM, Geert Uytterhoeven wrote:
> >>> Hi Marek, Steve,
> >>>
> >>> On Sat, Jun 2, 2018 at 12:11 PM, Marek Vasut <[email protected]> wrote:
> >>>> While the datasheet for DA9063L (2v1, 23-Mar-2017) lists the RTC register
> >>>> block, the DA9063L does not have an RTC. Add custom regmap for DA9063L to
> >>>> prevent access into that register block.
> >
> > Ok. I've said previously in [v3 07/10], but I'll copy again:
> > There is now an internal Dialog request to remove the RTC references from the DA9063L datasheet.
> > Adding that first part to the sentence in the commit log: "While the datasheet for DA9063L
> > (2v1, 23-Mar-2017) lists the RTC register block" -- it exists in error for the register map table
> > on page 91, but the datasheet also identifies those registers in Table 102 on page 126 as
> > "Reserved".
> >
> > Pointing out the ambiguity in this version of the datasheet seems redundant in the commit log.
> > Also Dialog do not store a history of Datasheets on their website so once this is updated (although
> > this update is not in my hands) the datasheet will be replaced. So, it seems this comment could
> > make the commit message just as misleading as the current datasheet.
> >
> > How about something simpler?
> > "The DA9063L does not have an RTC. Add custom regmap for DA9063L to prevent access
> > into that register block."
> >
> >>>>
> >>>> Signed-off-by: Marek Vasut <[email protected]>
> >>>
> >>> Thanks for your patch!
> >>>
> >>>> --- a/drivers/mfd/da9063-i2c.c
> >>>> +++ b/drivers/mfd/da9063-i2c.c
> >>>> @@ -254,6 +341,10 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
> >>>
> >>> Note that the line above doesn't check da9063->type, but da9063-
> >>> variant_code...
> >>>
> >>>> 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) {
> >>>
> >>> ... so this may be slightly confusing.
> >>
> >> I know.
> >>
> >>>> + 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;
> >>>
> >>> However, da9063->variant_code doesn't seem to have been filled in at this
> >>> point yet (the call to da9063_device_init() doing so is below, at the end
> >>> of the probe function!), so commit 9cb42e2a8ed06e91 ("mfd: da9063: Add
> >>> support for AD silicon variant") never actually handled the AD silicon variant
> >>> correctly? Or am I missing something?
> >
> > Okay ... No. You're not missing anything. I had noticed that.
> > The AD chip model is not referenced and by default only the BB chip model is used.
> >
> >> Ha, that is a good point.
> >
> > Yeah, it's a good point, but it's not an amusing point.
> > The device tree only distinguishes a "dlg,da9063", there is no AD type in the DT schema.
> > There is no datasheet listing AD registers supported by Dialog, only BB.
> >
> > But, AD registers were added back into the header file in commit 9cb42e2a8ed06e91
> > and the RTC driver was updated to distinguish between the AD and BB according to
> > the type of variant detected at run-time during the da9063_device_init() call.
> >
> > The real problem is that this leads to two competing chip detection methods for the
> > DA9063. The function da9063_device_init() autodetects the chip variant, but
> > autodetection cannot define the chip model. It's circular: the chip model cannot be
> > autodetected because a chip model is needed to access the register used during
> > autodetection.
> >
> > Which leads me back to what I said two paragraphs up:
> >> The device tree only distinguishes a "dlg,da9063", there is no AD type in the DT schema.
> >> There is no datasheet listing AD registers supported by Dialog, only BB.
> >
> > This is not how it is done in the DA9062 and DA9061 driver: the variant code is only
> > used to print the information to the console during start-up and it is the DT that defines
> > the chip model based upon "dlg,da9062" or "dlg,da9061".
>
> So the AD was broken since forever and noone noticed ? :)
Not quite.
The AD support is working, but the driver doesn't work like everybody
expects because it uses the BB chip model. But it does work because the chip
model for BB is valid for AD; in this case BB represents a superset of AD
registers (and any mismatches are never accessed or mean anything in AD).
> Do you have an AD hardware and can you fix it ?
Part of my work is to support the community and I think this is fixable.
But all of this shouldn't affect your DA9063L submission should it?
Regards,
Steve
On 06/06/2018 08:16 AM, Lee Jones wrote:
> On Tue, 05 Jun 2018, Marek Vasut wrote:
[...]
>>>> -static const struct mfd_cell da9063_devs[] = {
>>>> +static const struct mfd_cell da9063_common_devs[] = {
>>>> {
>>>> .name = DA9063_DRVNAME_REGULATORS,
>>>
>>> Appreciate that these are historical, but these device name defines
>>> make me shudder. They only serve to act as an obfuscation layer when
>>> debugging at platform level. Please consider getting rid of them.
>>
>> The macro can be shared between the core and the drivers, so the names
>> never run out of sync.
>
> Platform driver name changes are vary rare. Even if they are changed,
> even light testing would uncover the fact that child drivers do not
> .probe().
Sure, while if the macro is used, this problem is avoided altogether.
> Due to the current obfuscation, I currently have no idea
> what this device's name is.
I'm sure ctags or git grep can easily help.
> This technique is not allowed for new
> drivers - unfortunately I didn't not review this driver in the first
> instance.
Why not ? This looks like a step back to me.
> It doesn't bother me enough to go and change it myself and I'm not
> going to have a baby over patches not being submitted to fix it.
>
>>>> .num_resources = ARRAY_SIZE(da9063_regulators_resources),
>>>> @@ -100,15 +100,19 @@ static const struct mfd_cell da9063_devs[] = {
>>>> .resources = da9063_onkey_resources,
>>>> .of_compatible = "dlg,da9063-onkey",
>>>> },
>>>> + {
>>>> + .name = DA9063_DRVNAME_VIBRATION,
>>>> + },
>>>
>>> Place this on a single line please.
>>
>> This would only make the style inconsistent with the ie. LEDs entry.
>>
>>> { .name = DA9063_DRVNAME_VIBRATION },
>
> If that is a one line entry spaced over multiple lines, then that
> should also be changed.
>
> Maybe I will go through and stylise this driver a bit after all (but
> as time is short at the moment, maybe not!) :)
You'd end up with two entries which look different then the rest, which
triggers my OCD.
> [...]
>
>>>> +err_mfd_cleanup:
>>>> + mfd_remove_devices(da9063->dev);
>>>
>>> Any reason why you can't use devm_*?
>>
>> Because we need to undo the MFD setup before the IRQ setup.
>
> Sounds like a good enough reason.
Or the da9063_irq_init() could use devm_regmap_add_irq_chip().
--
Best regards,
Marek Vasut
On 06/06/2018 11:47 AM, Steve Twiss wrote:
> Hi Marek and Geert,
>
> On 06 June 2018 00:02 Marek Vasut wrote,
>
>> Subject: Re: [PATCH v3 06/10] mfd: da9063: Add custom regmap for DA9063L
>>
>> On 06/05/2018 10:17 PM, Steve Twiss wrote:
>>> Hi Marek and Geert,
>>>
>>> On 04 June 2018 17:25 Marek Vasut wrote,
>>>
>>>> Subject: Re: [PATCH v3 06/10] mfd: da9063: Add custom regmap for DA9063L
>>>>
>>>> On 06/04/2018 09:39 AM, Geert Uytterhoeven wrote:
>>>>> Hi Marek, Steve,
>>>>>
>>>>> On Sat, Jun 2, 2018 at 12:11 PM, Marek Vasut <[email protected]> wrote:
>>>>>> While the datasheet for DA9063L (2v1, 23-Mar-2017) lists the RTC register
>>>>>> block, the DA9063L does not have an RTC. Add custom regmap for DA9063L to
>>>>>> prevent access into that register block.
>>>
>>> Ok. I've said previously in [v3 07/10], but I'll copy again:
>>> There is now an internal Dialog request to remove the RTC references from the DA9063L datasheet.
>>> Adding that first part to the sentence in the commit log: "While the datasheet for DA9063L
>>> (2v1, 23-Mar-2017) lists the RTC register block" -- it exists in error for the register map table
>>> on page 91, but the datasheet also identifies those registers in Table 102 on page 126 as
>>> "Reserved".
>>>
>>> Pointing out the ambiguity in this version of the datasheet seems redundant in the commit log.
>>> Also Dialog do not store a history of Datasheets on their website so once this is updated (although
>>> this update is not in my hands) the datasheet will be replaced. So, it seems this comment could
>>> make the commit message just as misleading as the current datasheet.
>>>
>>> How about something simpler?
>>> "The DA9063L does not have an RTC. Add custom regmap for DA9063L to prevent access
>>> into that register block."
>>>
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <[email protected]>
>>>>>
>>>>> Thanks for your patch!
>>>>>
>>>>>> --- a/drivers/mfd/da9063-i2c.c
>>>>>> +++ b/drivers/mfd/da9063-i2c.c
>>>>>> @@ -254,6 +341,10 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
>>>>>
>>>>> Note that the line above doesn't check da9063->type, but da9063-
>>>>> variant_code...
>>>>>
>>>>>> 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) {
>>>>>
>>>>> ... so this may be slightly confusing.
>>>>
>>>> I know.
>>>>
>>>>>> + 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;
>>>>>
>>>>> However, da9063->variant_code doesn't seem to have been filled in at this
>>>>> point yet (the call to da9063_device_init() doing so is below, at the end
>>>>> of the probe function!), so commit 9cb42e2a8ed06e91 ("mfd: da9063: Add
>>>>> support for AD silicon variant") never actually handled the AD silicon variant
>>>>> correctly? Or am I missing something?
>>>
>>> Okay ... No. You're not missing anything. I had noticed that.
>>> The AD chip model is not referenced and by default only the BB chip model is used.
>>>
>>>> Ha, that is a good point.
>>>
>>> Yeah, it's a good point, but it's not an amusing point.
>>> The device tree only distinguishes a "dlg,da9063", there is no AD type in the DT schema.
>>> There is no datasheet listing AD registers supported by Dialog, only BB.
>>>
>>> But, AD registers were added back into the header file in commit 9cb42e2a8ed06e91
>>> and the RTC driver was updated to distinguish between the AD and BB according to
>>> the type of variant detected at run-time during the da9063_device_init() call.
>>>
>>> The real problem is that this leads to two competing chip detection methods for the
>>> DA9063. The function da9063_device_init() autodetects the chip variant, but
>>> autodetection cannot define the chip model. It's circular: the chip model cannot be
>>> autodetected because a chip model is needed to access the register used during
>>> autodetection.
>>>
>>> Which leads me back to what I said two paragraphs up:
>>>> The device tree only distinguishes a "dlg,da9063", there is no AD type in the DT schema.
>>>> There is no datasheet listing AD registers supported by Dialog, only BB.
>>>
>>> This is not how it is done in the DA9062 and DA9061 driver: the variant code is only
>>> used to print the information to the console during start-up and it is the DT that defines
>>> the chip model based upon "dlg,da9062" or "dlg,da9061".
>>
>> So the AD was broken since forever and noone noticed ? :)
>
> Not quite.
> The AD support is working, but the driver doesn't work like everybody
> expects because it uses the BB chip model. But it does work because the chip
> model for BB is valid for AD; in this case BB represents a superset of AD
> registers (and any mismatches are never accessed or mean anything in AD).
>
>> Do you have an AD hardware and can you fix it ?
>
> Part of my work is to support the community and I think this is fixable.
>
> But all of this shouldn't affect your DA9063L submission should it?
I think there might be conflict between those patchsets, so let me send
out a V5 so you can play around with the AD and fix that too.
--
Best regards,
Marek Vasut
On Wed, 06 Jun 2018, Marek Vasut wrote:
> On 06/06/2018 08:16 AM, Lee Jones wrote:
> > On Tue, 05 Jun 2018, Marek Vasut wrote:
>
> [...]
>
> >>>> -static const struct mfd_cell da9063_devs[] = {
> >>>> +static const struct mfd_cell da9063_common_devs[] = {
> >>>> {
> >>>> .name = DA9063_DRVNAME_REGULATORS,
> >>>
> >>> Appreciate that these are historical, but these device name defines
> >>> make me shudder. They only serve to act as an obfuscation layer when
> >>> debugging at platform level. Please consider getting rid of them.
> >>
> >> The macro can be shared between the core and the drivers, so the names
> >> never run out of sync.
> >
> > Platform driver name changes are vary rare. Even if they are changed,
> > even light testing would uncover the fact that child drivers do not
> > .probe().
>
> Sure, while if the macro is used, this problem is avoided altogether.
>
> > Due to the current obfuscation, I currently have no idea
> > what this device's name is.
>
> I'm sure ctags or git grep can easily help.
I'm aware how to get around the 'issue', but it's an additional step
which is avoidable. For me personally it comes from doing *a lot* of
platform level work and being irritated by the extra grepping. Macros
for driver names does not sit right with me at all. There are even
worse examples of people defining the MACROs *inside* the driver,
which doesn't even benefit from the small redeeming feature you
mention above.
Anyway, I'm happy with you not wanting to change it. Just leave them
as they are for now.
> >>>> + {
> >>>> + .name = DA9063_DRVNAME_VIBRATION,
> >>>> + },
> >>>
> >>> Place this on a single line please.
> >>
> >> This would only make the style inconsistent with the ie. LEDs entry.
> >>
> >>> { .name = DA9063_DRVNAME_VIBRATION },
> >
> > If that is a one line entry spaced over multiple lines, then that
> > should also be changed.
> >
> > Maybe I will go through and stylise this driver a bit after all (but
> > as time is short at the moment, maybe not!) :)
>
> You'd end up with two entries which look different then the rest, which
> triggers my OCD.
OCD or not, it's never okay to waste lines. If ordering it not
important (which it should not be -- it's fragile to rely on device
ordering in MFD cells), the multi-line entries go at the top, followed
by the single line entries. If done right, it looks the opposite of
bad/out of place.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 06/07/2018 07:04 AM, Lee Jones wrote:
> On Wed, 06 Jun 2018, Marek Vasut wrote:
>> On 06/06/2018 08:16 AM, Lee Jones wrote:
>>> On Tue, 05 Jun 2018, Marek Vasut wrote:
>>
>> [...]
>>
>>>>>> -static const struct mfd_cell da9063_devs[] = {
>>>>>> +static const struct mfd_cell da9063_common_devs[] = {
>>>>>> {
>>>>>> .name = DA9063_DRVNAME_REGULATORS,
>>>>>
>>>>> Appreciate that these are historical, but these device name defines
>>>>> make me shudder. They only serve to act as an obfuscation layer when
>>>>> debugging at platform level. Please consider getting rid of them.
>>>>
>>>> The macro can be shared between the core and the drivers, so the names
>>>> never run out of sync.
>>>
>>> Platform driver name changes are vary rare. Even if they are changed,
>>> even light testing would uncover the fact that child drivers do not
>>> .probe().
>>
>> Sure, while if the macro is used, this problem is avoided altogether.
>>
>>> Due to the current obfuscation, I currently have no idea
>>> what this device's name is.
>>
>> I'm sure ctags or git grep can easily help.
>
> I'm aware how to get around the 'issue', but it's an additional step
> which is avoidable. For me personally it comes from doing *a lot* of
> platform level work and being irritated by the extra grepping. Macros
> for driver names does not sit right with me at all. There are even
> worse examples of people defining the MACROs *inside* the driver,
> which doesn't even benefit from the small redeeming feature you
> mention above.
If we follow this line of thinking, we could just run cpp and expand all
macros. Then there's no need for grepping at all. That probably won't be
the result anyone would like though.
> Anyway, I'm happy with you not wanting to change it. Just leave them
> as they are for now.
>
>>>>>> + {
>>>>>> + .name = DA9063_DRVNAME_VIBRATION,
>>>>>> + },
>>>>>
>>>>> Place this on a single line please.
>>>>
>>>> This would only make the style inconsistent with the ie. LEDs entry.
>>>>
>>>>> { .name = DA9063_DRVNAME_VIBRATION },
>>>
>>> If that is a one line entry spaced over multiple lines, then that
>>> should also be changed.
>>>
>>> Maybe I will go through and stylise this driver a bit after all (but
>>> as time is short at the moment, maybe not!) :)
>>
>> You'd end up with two entries which look different then the rest, which
>> triggers my OCD.
>
> OCD or not, it's never okay to waste lines. If ordering it not
> important (which it should not be -- it's fragile to rely on device
> ordering in MFD cells), the multi-line entries go at the top, followed
> by the single line entries. If done right, it looks the opposite of
> bad/out of place.
My point is, the style should at least be consistent. But anyway.
--
Best regards,
Marek Vasut
On Thu, 07 Jun 2018, Marek Vasut wrote:
> On 06/07/2018 07:04 AM, Lee Jones wrote:
> > On Wed, 06 Jun 2018, Marek Vasut wrote:
> >> On 06/06/2018 08:16 AM, Lee Jones wrote:
> >>> On Tue, 05 Jun 2018, Marek Vasut wrote:
> >>
> >> [...]
> >>
> >>>>>> -static const struct mfd_cell da9063_devs[] = {
> >>>>>> +static const struct mfd_cell da9063_common_devs[] = {
> >>>>>> {
> >>>>>> .name = DA9063_DRVNAME_REGULATORS,
> >>>>>
> >>>>> Appreciate that these are historical, but these device name defines
> >>>>> make me shudder. They only serve to act as an obfuscation layer when
> >>>>> debugging at platform level. Please consider getting rid of them.
> >>>>
> >>>> The macro can be shared between the core and the drivers, so the names
> >>>> never run out of sync.
> >>>
> >>> Platform driver name changes are vary rare. Even if they are changed,
> >>> even light testing would uncover the fact that child drivers do not
> >>> .probe().
> >>
> >> Sure, while if the macro is used, this problem is avoided altogether.
> >>
> >>> Due to the current obfuscation, I currently have no idea
> >>> what this device's name is.
> >>
> >> I'm sure ctags or git grep can easily help.
> >
> > I'm aware how to get around the 'issue', but it's an additional step
> > which is avoidable. For me personally it comes from doing *a lot* of
> > platform level work and being irritated by the extra grepping. Macros
> > for driver names does not sit right with me at all. There are even
> > worse examples of people defining the MACROs *inside* the driver,
> > which doesn't even benefit from the small redeeming feature you
> > mention above.
>
> If we follow this line of thinking, we could just run cpp and expand all
> macros. Then there's no need for grepping at all. That probably won't be
> the result anyone would like though.
Hmm ... yes, that's the same! :D
> > Anyway, I'm happy with you not wanting to change it. Just leave them
> > as they are for now.
> >>>>>> + {
> >>>>>> + .name = DA9063_DRVNAME_VIBRATION,
> >>>>>> + },
> >>>>>
> >>>>> Place this on a single line please.
> >>>>
> >>>> This would only make the style inconsistent with the ie. LEDs entry.
> >>>>
> >>>>> { .name = DA9063_DRVNAME_VIBRATION },
> >>>
> >>> If that is a one line entry spaced over multiple lines, then that
> >>> should also be changed.
> >>>
> >>> Maybe I will go through and stylise this driver a bit after all (but
> >>> as time is short at the moment, maybe not!) :)
> >>
> >> You'd end up with two entries which look different then the rest, which
> >> triggers my OCD.
> >
> > OCD or not, it's never okay to waste lines. If ordering it not
> > important (which it should not be -- it's fragile to rely on device
> > ordering in MFD cells), the multi-line entries go at the top, followed
> > by the single line entries. If done right, it looks the opposite of
> > bad/out of place.
>
> My point is, the style should at least be consistent. But anyway.
It is consistent.
- Multi-line entries go on multiple lines.
- Single line entries go on single lines.
See drivers/mfd/max77620.c for how it should look.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog