LinuxLists.cc - [PATCH 0/5] regulator: act8865: add support for act8846

[permalink] [raw]

Subject: [PATCH 4/5] regulator: act8865: add support for act8846

Add device id and definition of registers and regulators to support
the act8846 PMU.

Signed-off-by: Beniamino Galvani <[email protected]>
---
drivers/regulator/act8865-regulator.c | 71 +++++++++++++++++++++++++++++++++
include/linux/regulator/act8865.h | 17 ++++++++
2 files changed, 88 insertions(+)

diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index c604b34..1a693a5 100644
--- a/drivers/regulator/act8865-regulator.c
+++ b/drivers/regulator/act8865-regulator.c
@@ -29,6 +29,40 @@
#include <linux/regmap.h>

/*
+ * ACT8846 Global Register Map.
+ */
+#define ACT8846_SYS0 0x00
+#define ACT8846_SYS1 0x01
+#define ACT8846_REG1_VSET 0x10
+#define ACT8846_REG1_CTRL 0x12
+#define ACT8846_REG2_VSET0 0x20
+#define ACT8846_REG2_VSET1 0x21
+#define ACT8846_REG2_CTRL 0x22
+#define ACT8846_REG3_VSET0 0x30
+#define ACT8846_REG3_VSET1 0x31
+#define ACT8846_REG3_CTRL 0x32
+#define ACT8846_REG4_VSET0 0x40
+#define ACT8846_REG4_VSET1 0x41
+#define ACT8846_REG4_CTRL 0x42
+#define ACT8846_REG5_VSET 0x50
+#define ACT8846_REG5_CTRL 0x51
+#define ACT8846_REG6_VSET 0x58
+#define ACT8846_REG6_CTRL 0x59
+#define ACT8846_REG7_VSET 0x60
+#define ACT8846_REG7_CTRL 0x61
+#define ACT8846_REG8_VSET 0x68
+#define ACT8846_REG8_CTRL 0x69
+#define ACT8846_REG9_VSET 0x70
+#define ACT8846_REG9_CTRL 0x71
+#define ACT8846_REG10_VSET 0x80
+#define ACT8846_REG10_CTRL 0x81
+#define ACT8846_REG11_VSET 0x90
+#define ACT8846_REG11_CTRL 0x91
+#define ACT8846_REG12_VSET 0xa0
+#define ACT8846_REG12_CTRL 0xa1
+#define ACT8846_REG13_CTRL 0xb1
+
+/*
* ACT8865 Global Register Map.
*/
#define ACT8865_SYS_MODE 0x00
@@ -103,6 +137,21 @@ static struct regulator_ops act8865_ops = {
.owner = THIS_MODULE, \
}

+static const struct regulator_desc act8846_regulators[] = {
+ ACT88xx_REG("REG1", ACT8846, REG1, VSET),
+ ACT88xx_REG("REG2", ACT8846, REG2, VSET0),
+ ACT88xx_REG("REG3", ACT8846, REG3, VSET0),
+ ACT88xx_REG("REG4", ACT8846, REG4, VSET0),
+ ACT88xx_REG("REG5", ACT8846, REG5, VSET),
+ ACT88xx_REG("REG6", ACT8846, REG6, VSET),
+ ACT88xx_REG("REG7", ACT8846, REG7, VSET),
+ ACT88xx_REG("REG8", ACT8846, REG8, VSET),
+ ACT88xx_REG("REG9", ACT8846, REG9, VSET),
+ ACT88xx_REG("REG10", ACT8846, REG10, VSET),
+ ACT88xx_REG("REG11", ACT8846, REG11, VSET),
+ ACT88xx_REG("REG12", ACT8846, REG12, VSET),
+};
+
static const struct regulator_desc act8865_regulators[] = {
ACT88xx_REG("DCDC_REG1", ACT8865, DCDC1, VSET1),
ACT88xx_REG("DCDC_REG2", ACT8865, DCDC2, VSET1),
@@ -115,11 +164,27 @@ static const struct regulator_desc act8865_regulators[] = {

#ifdef CONFIG_OF
static const struct of_device_id act8865_dt_ids[] = {
+ { .compatible = "active-semi,act8846", .data = (void *)ACT8846 },
{ .compatible = "active-semi,act8865", .data = (void *)ACT8865 },
{ }
};
MODULE_DEVICE_TABLE(of, act8865_dt_ids);

+static struct of_regulator_match act8846_matches[] = {
+ [ACT8846_ID_REG1] = { .name = "REG1" },
+ [ACT8846_ID_REG2] = { .name = "REG2" },
+ [ACT8846_ID_REG3] = { .name = "REG3" },
+ [ACT8846_ID_REG4] = { .name = "REG4" },
+ [ACT8846_ID_REG5] = { .name = "REG5" },
+ [ACT8846_ID_REG6] = { .name = "REG6" },
+ [ACT8846_ID_REG7] = { .name = "REG7" },
+ [ACT8846_ID_REG8] = { .name = "REG8" },
+ [ACT8846_ID_REG9] = { .name = "REG9" },
+ [ACT8846_ID_REG10] = { .name = "REG10" },
+ [ACT8846_ID_REG11] = { .name = "REG11" },
+ [ACT8846_ID_REG12] = { .name = "REG12" },
+};
+
static struct of_regulator_match act8865_matches[] = {
[ACT8865_ID_DCDC1] = { .name = "DCDC_REG1"},
[ACT8865_ID_DCDC2] = { .name = "DCDC_REG2"},
@@ -222,6 +287,11 @@ static int act8865_pmic_probe(struct i2c_client *client,
}

switch (type) {
+ case ACT8846:
+ matches = act8846_matches;
+ regulators = act8846_regulators;
+ num_regulators = ARRAY_SIZE(act8846_regulators);
+ break;
case ACT8865:
matches = act8865_matches;
regulators = act8865_regulators;
@@ -290,6 +360,7 @@ static int act8865_pmic_probe(struct i2c_client *client,
}

static const struct i2c_device_id act8865_ids[] = {
+ { .name = "act8846", .driver_data = ACT8846 },
{ .name = "act8865", .driver_data = ACT8865 },
{ },
};
diff --git a/include/linux/regulator/act8865.h b/include/linux/regulator/act8865.h
index b49be81..b6c4909 100644
--- a/include/linux/regulator/act8865.h
+++ b/include/linux/regulator/act8865.h
@@ -30,7 +30,24 @@ enum {
};

enum {
+ ACT8846_ID_REG1,
+ ACT8846_ID_REG2,
+ ACT8846_ID_REG3,
+ ACT8846_ID_REG4,
+ ACT8846_ID_REG5,
+ ACT8846_ID_REG6,
+ ACT8846_ID_REG7,
+ ACT8846_ID_REG8,
+ ACT8846_ID_REG9,
+ ACT8846_ID_REG10,
+ ACT8846_ID_REG11,
+ ACT8846_ID_REG12,
+ ACT8846_REG_NUM,
+};
+
+enum {
ACT8865,
+ ACT8846,
};

/**
--
1.7.10.4

2014-06-22 15:33:45

[permalink] [raw]

Subject: [PATCH 5/5] regulator: act8865: add act8846 to DT binding documentation

This patch adds a new "active-semi,act8846" compatible string and a
list of supported regulator names to the devicetree binding
documentation for Active-Semi PMUs.

Signed-off-by: Beniamino Galvani <[email protected]>
---
.../devicetree/bindings/regulator/act8865-regulator.txt | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/act8865-regulator.txt b/Documentation/devicetree/bindings/regulator/act8865-regulator.txt
index bef1fbb..865614b 100644
--- a/Documentation/devicetree/bindings/regulator/act8865-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/act8865-regulator.txt
@@ -1,13 +1,16 @@
-ACT8865 regulator
+ACT88xx regulators
-------------------

Required properties:
-- compatible: "active-semi,act8865"
+- compatible: "active-semi,act8846" or "active-semi,act8865"
- reg: I2C slave address

Any standard regulator properties can be used to configure the single regulator.

The valid names for regulators are:
+ - for act8846:
+ REG1, REG2, REG3, REG4, REG5, REG6, REG7, REG8, REG9, REG10, REG11, REG12
+ - for act8865:
DCDC_REG1, DCDC_REG2, DCDC_REG3, LDO_REG1, LDO_REG2, LDO_REG3, LDO_REG4.

Example:
--
1.7.10.4

2014-06-22 15:33:42

[permalink] [raw]

Subject: [PATCH 3/5] regulator: act8865: prepare support for other act88xx devices

This patch prepares support for other devices in the act88xx family of
PMUs manufactured by Active-Semi.

http://www.active-semi.com/products/power-management-units/act88xx/

Signed-off-by: Beniamino Galvani <[email protected]>
---
drivers/regulator/act8865-regulator.c | 212 +++++++++++++--------------------
include/linux/regulator/act8865.h | 6 +-
2 files changed, 88 insertions(+), 130 deletions(-)

diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index b03a0e8..c604b34 100644
--- a/drivers/regulator/act8865-regulator.c
+++ b/drivers/regulator/act8865-regulator.c
@@ -1,6 +1,7 @@
/*
- * act8865-regulator.c - Voltage regulation for the active-semi ACT8865
- * http://www.active-semi.com/sheets/ACT8865_Datasheet.pdf
+ * act8865-regulator.c - Voltage regulation for active-semi ACT88xx PMUs
+ *
+ * http://www.active-semi.com/products/power-management-units/act88xx/
*
* Copyright (C) 2013 Atmel Corporation
*
@@ -70,7 +71,7 @@ static const struct regmap_config act8865_regmap_config = {
.val_bits = 8,
};

-static const struct regulator_linear_range act8865_volatge_ranges[] = {
+static const struct regulator_linear_range act8865_voltage_ranges[] = {
REGULATOR_LINEAR_RANGE(600000, 0, 23, 25000),
REGULATOR_LINEAR_RANGE(1200000, 24, 47, 50000),
REGULATOR_LINEAR_RANGE(2400000, 48, 63, 100000),
@@ -86,110 +87,35 @@ static struct regulator_ops act8865_ops = {
.is_enabled = regulator_is_enabled_regmap,
};

-static const struct regulator_desc act8865_reg[] = {
- {
- .name = "DCDC_REG1",
- .id = ACT8865_ID_DCDC1,
- .ops = &act8865_ops,
- .type = REGULATOR_VOLTAGE,
- .n_voltages = ACT8865_VOLTAGE_NUM,
- .linear_ranges = act8865_volatge_ranges,
- .n_linear_ranges = ARRAY_SIZE(act8865_volatge_ranges),
- .vsel_reg = ACT8865_DCDC1_VSET1,
- .vsel_mask = ACT8865_VSEL_MASK,
- .enable_reg = ACT8865_DCDC1_CTRL,
- .enable_mask = ACT8865_ENA,
- .owner = THIS_MODULE,
- },
- {
- .name = "DCDC_REG2",
- .id = ACT8865_ID_DCDC2,
- .ops = &act8865_ops,
- .type = REGULATOR_VOLTAGE,
- .n_voltages = ACT8865_VOLTAGE_NUM,
- .linear_ranges = act8865_volatge_ranges,
- .n_linear_ranges = ARRAY_SIZE(act8865_volatge_ranges),
- .vsel_reg = ACT8865_DCDC2_VSET1,
- .vsel_mask = ACT8865_VSEL_MASK,
- .enable_reg = ACT8865_DCDC2_CTRL,
- .enable_mask = ACT8865_ENA,
- .owner = THIS_MODULE,
- },
- {
- .name = "DCDC_REG3",
- .id = ACT8865_ID_DCDC3,
- .ops = &act8865_ops,
- .type = REGULATOR_VOLTAGE,
- .n_voltages = ACT8865_VOLTAGE_NUM,
- .linear_ranges = act8865_volatge_ranges,
- .n_linear_ranges = ARRAY_SIZE(act8865_volatge_ranges),
- .vsel_reg = ACT8865_DCDC3_VSET1,
- .vsel_mask = ACT8865_VSEL_MASK,
- .enable_reg = ACT8865_DCDC3_CTRL,
- .enable_mask = ACT8865_ENA,
- .owner = THIS_MODULE,
- },
- {
- .name = "LDO_REG1",
- .id = ACT8865_ID_LDO1,
- .ops = &act8865_ops,
- .type = REGULATOR_VOLTAGE,
- .n_voltages = ACT8865_VOLTAGE_NUM,
- .linear_ranges = act8865_volatge_ranges,
- .n_linear_ranges = ARRAY_SIZE(act8865_volatge_ranges),
- .vsel_reg = ACT8865_LDO1_VSET,
- .vsel_mask = ACT8865_VSEL_MASK,
- .enable_reg = ACT8865_LDO1_CTRL,
- .enable_mask = ACT8865_ENA,
- .owner = THIS_MODULE,
- },
- {
- .name = "LDO_REG2",
- .id = ACT8865_ID_LDO2,
- .ops = &act8865_ops,
- .type = REGULATOR_VOLTAGE,
- .n_voltages = ACT8865_VOLTAGE_NUM,
- .linear_ranges = act8865_volatge_ranges,
- .n_linear_ranges = ARRAY_SIZE(act8865_volatge_ranges),
- .vsel_reg = ACT8865_LDO2_VSET,
- .vsel_mask = ACT8865_VSEL_MASK,
- .enable_reg = ACT8865_LDO2_CTRL,
- .enable_mask = ACT8865_ENA,
- .owner = THIS_MODULE,
- },
- {
- .name = "LDO_REG3",
- .id = ACT8865_ID_LDO3,
- .ops = &act8865_ops,
- .type = REGULATOR_VOLTAGE,
- .n_voltages = ACT8865_VOLTAGE_NUM,
- .linear_ranges = act8865_volatge_ranges,
- .n_linear_ranges = ARRAY_SIZE(act8865_volatge_ranges),
- .vsel_reg = ACT8865_LDO3_VSET,
- .vsel_mask = ACT8865_VSEL_MASK,
- .enable_reg = ACT8865_LDO3_CTRL,
- .enable_mask = ACT8865_ENA,
- .owner = THIS_MODULE,
- },
- {
- .name = "LDO_REG4",
- .id = ACT8865_ID_LDO4,
- .ops = &act8865_ops,
- .type = REGULATOR_VOLTAGE,
- .n_voltages = ACT8865_VOLTAGE_NUM,
- .linear_ranges = act8865_volatge_ranges,
- .n_linear_ranges = ARRAY_SIZE(act8865_volatge_ranges),
- .vsel_reg = ACT8865_LDO4_VSET,
- .vsel_mask = ACT8865_VSEL_MASK,
- .enable_reg = ACT8865_LDO4_CTRL,
- .enable_mask = ACT8865_ENA,
- .owner = THIS_MODULE,
- },
+#define ACT88xx_REG(_name, _family, _id, _vsel_reg) \
+ [_family##_ID_##_id] = { \
+ .name = _name, \
+ .id = _family##_ID_##_id, \
+ .type = REGULATOR_VOLTAGE, \
+ .ops = &act8865_ops, \
+ .n_voltages = ACT8865_VOLTAGE_NUM, \
+ .linear_ranges = act8865_voltage_ranges, \
+ .n_linear_ranges = ARRAY_SIZE(act8865_voltage_ranges), \
+ .vsel_reg = _family##_##_id##_##_vsel_reg, \
+ .vsel_mask = ACT8865_VSEL_MASK, \
+ .enable_reg = _family##_##_id##_CTRL, \
+ .enable_mask = ACT8865_ENA, \
+ .owner = THIS_MODULE, \
+ }
+
+static const struct regulator_desc act8865_regulators[] = {
+ ACT88xx_REG("DCDC_REG1", ACT8865, DCDC1, VSET1),
+ ACT88xx_REG("DCDC_REG2", ACT8865, DCDC2, VSET1),
+ ACT88xx_REG("DCDC_REG3", ACT8865, DCDC3, VSET1),
+ ACT88xx_REG("LDO_REG1", ACT8865, LDO1, VSET),
+ ACT88xx_REG("LDO_REG2", ACT8865, LDO2, VSET),
+ ACT88xx_REG("LDO_REG3", ACT8865, LDO3, VSET),
+ ACT88xx_REG("LDO_REG4", ACT8865, LDO4, VSET),
};

#ifdef CONFIG_OF
static const struct of_device_id act8865_dt_ids[] = {
- { .compatible = "active-semi,act8865" },
+ { .compatible = "active-semi,act8865", .data = (void *)ACT8865 },
{ }
};
MODULE_DEVICE_TABLE(of, act8865_dt_ids);
@@ -206,7 +132,9 @@ static struct of_regulator_match act8865_matches[] = {

static int act8865_pdata_from_dt(struct device *dev,
struct device_node **of_node,
- struct act8865_platform_data *pdata)
+ struct act8865_platform_data *pdata,
+ struct of_regulator_match *matches,
+ int num_matches)
{
int matched, i;
struct device_node *np;
@@ -218,26 +146,25 @@ static int act8865_pdata_from_dt(struct device *dev,
return -EINVAL;
}

- matched = of_regulator_match(dev, np,
- act8865_matches, ARRAY_SIZE(act8865_matches));
+ matched = of_regulator_match(dev, np, matches, num_matches);
of_node_put(np);
if (matched <= 0)
return matched;

pdata->regulators = devm_kzalloc(dev,
- sizeof(struct act8865_regulator_data) *
- ARRAY_SIZE(act8865_matches), GFP_KERNEL);
+ sizeof(struct act8865_regulator_data) *
+ num_matches, GFP_KERNEL);
if (!pdata->regulators)
return -ENOMEM;

- pdata->num_regulators = ARRAY_SIZE(act8865_matches);
+ pdata->num_regulators = num_matches;
regulator = pdata->regulators;

- for (i = 0; i < ARRAY_SIZE(act8865_matches); i++) {
+ for (i = 0; i < num_matches; i++) {
regulator->id = i;
- regulator->name = act8865_matches[i].name;
- regulator->platform_data = act8865_matches[i].init_data;
- of_node[i] = act8865_matches[i].of_node;
+ regulator->name = matches[i].name;
+ regulator->platform_data = matches[i].init_data;
+ of_node[i] = matches[i].of_node;
regulator++;
}

@@ -269,35 +196,59 @@ static struct regulator_init_data
}

static int act8865_pmic_probe(struct i2c_client *client,
- const struct i2c_device_id *i2c_id)
+ const struct i2c_device_id *i2c_id)
{
- struct regulator_dev *rdev;
+ static const struct regulator_desc *regulators;
+ struct act8865_platform_data pdata_of, *pdata;
+ struct of_regulator_match *matches;
struct device *dev = &client->dev;
- struct act8865_platform_data *pdata = dev_get_platdata(dev);
- struct regulator_config config = { };
+ int i, ret, num_regulators, error;
+ struct device_node **of_node;
struct act8865 *act8865;
- struct device_node *of_node[ACT8865_REG_NUM];
- int i;
- int ret = -EINVAL;
- int error;
+ unsigned long type;
+
+ pdata = dev_get_platdata(dev);

if (dev->of_node && !pdata) {
const struct of_device_id *id;
- struct act8865_platform_data pdata_of;

id = of_match_device(of_match_ptr(act8865_dt_ids), dev);
if (!id)
return -ENODEV;

- ret = act8865_pdata_from_dt(dev, of_node, &pdata_of);
+ type = (unsigned long) id->data;
+ } else {
+ type = i2c_id->driver_data;
+ }
+
+ switch (type) {
+ case ACT8865:
+ matches = act8865_matches;
+ regulators = act8865_regulators;
+ num_regulators = ARRAY_SIZE(act8865_regulators);
+ break;
+ default:
+ dev_err(dev, "invalid device id %lu\n", type);
+ return -EINVAL;
+ }
+
+ of_node = devm_kzalloc(dev, sizeof(struct device_node *) *
+ num_regulators, GFP_KERNEL);
+ if (!of_node)
+ return -ENOMEM;
+
+ if (dev->of_node && !pdata) {
+ ret = act8865_pdata_from_dt(dev, of_node, &pdata_of, matches,
+ num_regulators);
if (ret < 0)
return ret;

pdata = &pdata_of;
}

- if (pdata->num_regulators > ACT8865_REG_NUM) {
- dev_err(dev, "Too many regulators found!\n");
+ if (pdata->num_regulators > num_regulators) {
+ dev_err(dev, "too many regulators: %d\n",
+ pdata->num_regulators);
return -EINVAL;
}

@@ -314,8 +265,10 @@ static int act8865_pmic_probe(struct i2c_client *client,
}

/* Finally register devices */
- for (i = 0; i < ACT8865_REG_NUM; i++) {
- const struct regulator_desc *desc = &act8865_reg[i];
+ for (i = 0; i < num_regulators; i++) {
+ const struct regulator_desc *desc = &regulators[i];
+ struct regulator_config config = { };
+ struct regulator_dev *rdev;

config.dev = dev;
config.init_data = act8865_get_init_data(desc->id, pdata);
@@ -331,12 +284,13 @@ static int act8865_pmic_probe(struct i2c_client *client,
}

i2c_set_clientdata(client, act8865);
+ devm_kfree(dev, of_node);

return 0;
}

static const struct i2c_device_id act8865_ids[] = {
- { "act8865", 0 },
+ { .name = "act8865", .driver_data = ACT8865 },
{ },
};
MODULE_DEVICE_TABLE(i2c, act8865_ids);
@@ -352,6 +306,6 @@ static struct i2c_driver act8865_pmic_driver = {

module_i2c_driver(act8865_pmic_driver);

-MODULE_DESCRIPTION("active-semi act8865 voltage regulator driver");
+MODULE_DESCRIPTION("active-semi act88xx voltage regulator driver");
MODULE_AUTHOR("Wenyou Yang <[email protected]>");
MODULE_LICENSE("GPL v2");
diff --git a/include/linux/regulator/act8865.h b/include/linux/regulator/act8865.h
index 49206c1..b49be81 100644
--- a/include/linux/regulator/act8865.h
+++ b/include/linux/regulator/act8865.h
@@ -1,5 +1,5 @@
/*
- * act8865.h -- Voltage regulation for the active-semi act8865
+ * act8865.h -- Voltage regulation for active-semi act88xx PMUs
*
* Copyright (C) 2013 Atmel Corporation.
*
@@ -29,6 +29,10 @@ enum {
ACT8865_REG_NUM,
};

+enum {
+ ACT8865,
+};
+
/**
* act8865_regulator_data - regulator data
* @id: regulator id
--
1.7.10.4

2014-06-22 15:33:40

[permalink] [raw]

Subject: [PATCH 2/5] regulator: act8865: set correct number of regulators in pdata

When platform data is populated from DT all the regulators are
instantiated and the value of num_regulators should be the number of
all available regulators rather than the number of matched ones.

Signed-off-by: Beniamino Galvani <[email protected]>
---
drivers/regulator/act8865-regulator.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index fe2c038..b03a0e8 100644
--- a/drivers/regulator/act8865-regulator.c
+++ b/drivers/regulator/act8865-regulator.c
@@ -230,7 +230,7 @@ static int act8865_pdata_from_dt(struct device *dev,
if (!pdata->regulators)
return -ENOMEM;

- pdata->num_regulators = matched;
+ pdata->num_regulators = ARRAY_SIZE(act8865_matches);
regulator = pdata->regulators;

for (i = 0; i < ARRAY_SIZE(act8865_matches); i++) {
--
1.7.10.4

2014-06-22 15:33:37

[permalink] [raw]

Subject: [PATCH 1/5] regulator: act8865: fix parsing of platform data

The driver loops through all available regulators (ACT8865_REG_NUM)
and accesses pdata->regulators[i].platform_data without checking the
actual value of num_regulators in platform data, potentially causing a
invalid memory access.

Fix this and look up the regulator init_data by id in platform data.

Signed-off-by: Beniamino Galvani <[email protected]>
---
drivers/regulator/act8865-regulator.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index b92d7dd..fe2c038 100644
--- a/drivers/regulator/act8865-regulator.c
+++ b/drivers/regulator/act8865-regulator.c
@@ -252,6 +252,22 @@ static inline int act8865_pdata_from_dt(struct device *dev,
}
#endif

+static struct regulator_init_data
+*act8865_get_init_data(int id, struct act8865_platform_data *pdata)
+{
+ int i;
+
+ if (!pdata)
+ return NULL;
+
+ for (i = 0; i < pdata->num_regulators; i++) {
+ if (pdata->regulators[i].id == id)
+ return pdata->regulators[i].platform_data;
+ }
+
+ return NULL;
+}
+
static int act8865_pmic_probe(struct i2c_client *client,
const struct i2c_device_id *i2c_id)
{
@@ -261,7 +277,7 @@ static int act8865_pmic_probe(struct i2c_client *client,
struct regulator_config config = { };
struct act8865 *act8865;
struct device_node *of_node[ACT8865_REG_NUM];
- int i, id;
+ int i;
int ret = -EINVAL;
int error;

@@ -299,20 +315,17 @@ static int act8865_pmic_probe(struct i2c_client *client,

/* Finally register devices */
for (i = 0; i < ACT8865_REG_NUM; i++) {
-
- id = pdata->regulators[i].id;
+ const struct regulator_desc *desc = &act8865_reg[i];

config.dev = dev;
- config.init_data = pdata->regulators[i].platform_data;
+ config.init_data = act8865_get_init_data(desc->id, pdata);
config.of_node = of_node[i];
config.driver_data = act8865;
config.regmap = act8865->regmap;

- rdev = devm_regulator_register(&client->dev, &act8865_reg[i],
- &config);
+ rdev = devm_regulator_register(&client->dev, desc, &config);
if (IS_ERR(rdev)) {
- dev_err(dev, "failed to register %s\n",
- act8865_reg[id].name);
+ dev_err(dev, "failed to register %s\n", desc->name);
return PTR_ERR(rdev);
}
}
--
1.7.10.4

2014-06-23 09:46:01

[permalink] [raw]

Subject: RE: [PATCH 1/5] regulator: act8865: fix parsing of platform data

2014-06-23 09:46:16

[permalink] [raw]

Subject: RE: [PATCH 2/5] regulator: act8865: set correct number of regulators in pdata

2014-06-23 09:47:09

[permalink] [raw]

Subject: RE: [PATCH 4/5] regulator: act8865: add support for act8846

2014-06-23 09:52:14

[permalink] [raw]

Subject: RE: [PATCH 3/5] regulator: act8865: prepare support for other act88xx devices

> -----Original Message-----
> From: Beniamino Galvani [mailto:[email protected]]
> Sent: Sunday, June 22, 2014 11:32 PM
> To: Liam Girdwood; Mark Brown
> Cc: Yang, Wenyou; Rob Herring; Pawel Moll; Mark Rutland; Ian Campbell;
> Kumar Gala; Heiko Stuebner; [email protected]; linux-
> [email protected]; [email protected]; linux-arm-
> [email protected]; Beniamino Galvani
> Subject: [PATCH 3/5] regulator: act8865: prepare support for other
> act88xx devices
>
> This patch prepares support for other devices in the act88xx family of
> PMUs manufactured by Active-Semi.
>
> http://www.active-semi.com/products/power-management-units/act88xx/
>
> Signed-off-by: Beniamino Galvani <[email protected]>
> ---
> drivers/regulator/act8865-regulator.c | 212 +++++++++++++------------
> --------
> include/linux/regulator/act8865.h | 6 +-
> 2 files changed, 88 insertions(+), 130 deletions(-)
>
> diff --git a/drivers/regulator/act8865-regulator.c
> b/drivers/regulator/act8865-regulator.c
> index b03a0e8..c604b34 100644
> --- a/drivers/regulator/act8865-regulator.c
> +++ b/drivers/regulator/act8865-regulator.c
> @@ -1,6 +1,7 @@
> /*
> - * act8865-regulator.c - Voltage regulation for the active-semi
> ACT8865
> - * http://www.active-semi.com/sheets/ACT8865_Datasheet.pdf
> + * act8865-regulator.c - Voltage regulation for active-semi ACT88xx
> + PMUs
> + *
> + * http://www.active-semi.com/products/power-management-units/act88xx/
> *
> * Copyright (C) 2013 Atmel Corporation
> *
> @@ -70,7 +71,7 @@ static const struct regmap_config
> act8865_regmap_config = {
> .val_bits = 8,
> };
>
> -static const struct regulator_linear_range act8865_volatge_ranges[] =
> {
> +static const struct regulator_linear_range act8865_voltage_ranges[] =
> {
> REGULATOR_LINEAR_RANGE(600000, 0, 23, 25000),
> REGULATOR_LINEAR_RANGE(1200000, 24, 47, 50000),
> REGULATOR_LINEAR_RANGE(2400000, 48, 63, 100000), @@ -86,110
> +87,35 @@ static struct regulator_ops act8865_ops = {
> .is_enabled = regulator_is_enabled_regmap,
> };
>
> -static const struct regulator_desc act8865_reg[] = {
> - {
> - .name = "DCDC_REG1",
> - .id = ACT8865_ID_DCDC1,
> - .ops = &act8865_ops,
> - .type = REGULATOR_VOLTAGE,
> - .n_voltages = ACT8865_VOLTAGE_NUM,
> - .linear_ranges = act8865_volatge_ranges,
> - .n_linear_ranges = ARRAY_SIZE(act8865_volatge_ranges),
> - .vsel_reg = ACT8865_DCDC1_VSET1,
> - .vsel_mask = ACT8865_VSEL_MASK,
> - .enable_reg = ACT8865_DCDC1_CTRL,
> - .enable_mask = ACT8865_ENA,
> - .owner = THIS_MODULE,
> - },
> - {
> - .name = "DCDC_REG2",
> - .id = ACT8865_ID_DCDC2,
> - .ops = &act8865_ops,
> - .type = REGULATOR_VOLTAGE,
> - .n_voltages = ACT8865_VOLTAGE_NUM,
> - .linear_ranges = act8865_volatge_ranges,
> - .n_linear_ranges = ARRAY_SIZE(act8865_volatge_ranges),
> - .vsel_reg = ACT8865_DCDC2_VSET1,
> - .vsel_mask = ACT8865_VSEL_MASK,
> - .enable_reg = ACT8865_DCDC2_CTRL,
> - .enable_mask = ACT8865_ENA,
> - .owner = THIS_MODULE,
> - },
> - {
> - .name = "DCDC_REG3",
> - .id = ACT8865_ID_DCDC3,
> - .ops = &act8865_ops,
> - .type = REGULATOR_VOLTAGE,
> - .n_voltages = ACT8865_VOLTAGE_NUM,
> - .linear_ranges = act8865_volatge_ranges,
> - .n_linear_ranges = ARRAY_SIZE(act8865_volatge_ranges),
> - .vsel_reg = ACT8865_DCDC3_VSET1,
> - .vsel_mask = ACT8865_VSEL_MASK,
> - .enable_reg = ACT8865_DCDC3_CTRL,
> - .enable_mask = ACT8865_ENA,
> - .owner = THIS_MODULE,
> - },
> - {
> - .name = "LDO_REG1",
> - .id = ACT8865_ID_LDO1,
> - .ops = &act8865_ops,
> - .type = REGULATOR_VOLTAGE,
> - .n_voltages = ACT8865_VOLTAGE_NUM,
> - .linear_ranges = act8865_volatge_ranges,
> - .n_linear_ranges = ARRAY_SIZE(act8865_volatge_ranges),
> - .vsel_reg = ACT8865_LDO1_VSET,
> - .vsel_mask = ACT8865_VSEL_MASK,
> - .enable_reg = ACT8865_LDO1_CTRL,
> - .enable_mask = ACT8865_ENA,
> - .owner = THIS_MODULE,
> - },
> - {
> - .name = "LDO_REG2",
> - .id = ACT8865_ID_LDO2,
> - .ops = &act8865_ops,
> - .type = REGULATOR_VOLTAGE,
> - .n_voltages = ACT8865_VOLTAGE_NUM,
> - .linear_ranges = act8865_volatge_ranges,
> - .n_linear_ranges = ARRAY_SIZE(act8865_volatge_ranges),
> - .vsel_reg = ACT8865_LDO2_VSET,
> - .vsel_mask = ACT8865_VSEL_MASK,
> - .enable_reg = ACT8865_LDO2_CTRL,
> - .enable_mask = ACT8865_ENA,
> - .owner = THIS_MODULE,
> - },
> - {
> - .name = "LDO_REG3",
> - .id = ACT8865_ID_LDO3,
> - .ops = &act8865_ops,
> - .type = REGULATOR_VOLTAGE,
> - .n_voltages = ACT8865_VOLTAGE_NUM,
> - .linear_ranges = act8865_volatge_ranges,
> - .n_linear_ranges = ARRAY_SIZE(act8865_volatge_ranges),
> - .vsel_reg = ACT8865_LDO3_VSET,
> - .vsel_mask = ACT8865_VSEL_MASK,
> - .enable_reg = ACT8865_LDO3_CTRL,
> - .enable_mask = ACT8865_ENA,
> - .owner = THIS_MODULE,
> - },
> - {
> - .name = "LDO_REG4",
> - .id = ACT8865_ID_LDO4,
> - .ops = &act8865_ops,
> - .type = REGULATOR_VOLTAGE,
> - .n_voltages = ACT8865_VOLTAGE_NUM,
> - .linear_ranges = act8865_volatge_ranges,
> - .n_linear_ranges = ARRAY_SIZE(act8865_volatge_ranges),
> - .vsel_reg = ACT8865_LDO4_VSET,
> - .vsel_mask = ACT8865_VSEL_MASK,
> - .enable_reg = ACT8865_LDO4_CTRL,
> - .enable_mask = ACT8865_ENA,
> - .owner = THIS_MODULE,
> - },
> +#define ACT88xx_REG(_name, _family, _id, _vsel_reg) \
> + [_family##_ID_##_id] = { \
> + .name = _name, \
> + .id = _family##_ID_##_id, \
> + .type = REGULATOR_VOLTAGE, \
> + .ops = &act8865_ops, \
> + .n_voltages = ACT8865_VOLTAGE_NUM, \
> + .linear_ranges = act8865_voltage_ranges, \
> + .n_linear_ranges = ARRAY_SIZE(act8865_voltage_ranges), \
> + .vsel_reg = _family##_##_id##_##_vsel_reg, \
> + .vsel_mask = ACT8865_VSEL_MASK, \
> + .enable_reg = _family##_##_id##_CTRL, \
> + .enable_mask = ACT8865_ENA, \
> + .owner = THIS_MODULE, \
> + }
> +
> +static const struct regulator_desc act8865_regulators[] = {
> + ACT88xx_REG("DCDC_REG1", ACT8865, DCDC1, VSET1),
> + ACT88xx_REG("DCDC_REG2", ACT8865, DCDC2, VSET1),
> + ACT88xx_REG("DCDC_REG3", ACT8865, DCDC3, VSET1),
> + ACT88xx_REG("LDO_REG1", ACT8865, LDO1, VSET),
> + ACT88xx_REG("LDO_REG2", ACT8865, LDO2, VSET),
> + ACT88xx_REG("LDO_REG3", ACT8865, LDO3, VSET),
> + ACT88xx_REG("LDO_REG4", ACT8865, LDO4, VSET),
> };
>
> #ifdef CONFIG_OF
> static const struct of_device_id act8865_dt_ids[] = {
> - { .compatible = "active-semi,act8865" },
> + { .compatible = "active-semi,act8865", .data = (void *)ACT8865 },
> { }
> };
> MODULE_DEVICE_TABLE(of, act8865_dt_ids); @@ -206,7 +132,9 @@ static
> struct of_regulator_match act8865_matches[] = {
>
> static int act8865_pdata_from_dt(struct device *dev,
> struct device_node **of_node,
> - struct act8865_platform_data *pdata)
> + struct act8865_platform_data *pdata,
> + struct of_regulator_match *matches,
> + int num_matches)
> {
> int matched, i;
> struct device_node *np;
> @@ -218,26 +146,25 @@ static int act8865_pdata_from_dt(struct device
> *dev,
> return -EINVAL;
> }
>
> - matched = of_regulator_match(dev, np,
> - act8865_matches, ARRAY_SIZE(act8865_matches));
> + matched = of_regulator_match(dev, np, matches, num_matches);
> of_node_put(np);
> if (matched <= 0)
> return matched;
>
> pdata->regulators = devm_kzalloc(dev,
> - sizeof(struct act8865_regulator_data) *
> - ARRAY_SIZE(act8865_matches), GFP_KERNEL);
> + sizeof(struct act8865_regulator_data) *
> + num_matches, GFP_KERNEL);
> if (!pdata->regulators)
> return -ENOMEM;
>
> - pdata->num_regulators = ARRAY_SIZE(act8865_matches);
> + pdata->num_regulators = num_matches;
> regulator = pdata->regulators;
>
> - for (i = 0; i < ARRAY_SIZE(act8865_matches); i++) {
> + for (i = 0; i < num_matches; i++) {
> regulator->id = i;
> - regulator->name = act8865_matches[i].name;
> - regulator->platform_data = act8865_matches[i].init_data;
> - of_node[i] = act8865_matches[i].of_node;
> + regulator->name = matches[i].name;
> + regulator->platform_data = matches[i].init_data;
> + of_node[i] = matches[i].of_node;
> regulator++;
> }
>
> @@ -269,35 +196,59 @@ static struct regulator_init_data }
>
> static int act8865_pmic_probe(struct i2c_client *client,
> - const struct i2c_device_id *i2c_id)
> + const struct i2c_device_id *i2c_id)
> {
> - struct regulator_dev *rdev;
> + static const struct regulator_desc *regulators;
> + struct act8865_platform_data pdata_of, *pdata;
> + struct of_regulator_match *matches;
> struct device *dev = &client->dev;
> - struct act8865_platform_data *pdata = dev_get_platdata(dev);
> - struct regulator_config config = { };
> + int i, ret, num_regulators, error;
> + struct device_node **of_node;
> struct act8865 *act8865;
> - struct device_node *of_node[ACT8865_REG_NUM];
> - int i;
> - int ret = -EINVAL;
> - int error;
> + unsigned long type;
> +
> + pdata = dev_get_platdata(dev);
>
> if (dev->of_node && !pdata) {
> const struct of_device_id *id;
> - struct act8865_platform_data pdata_of;
>
> id = of_match_device(of_match_ptr(act8865_dt_ids), dev);
> if (!id)
> return -ENODEV;
>
> - ret = act8865_pdata_from_dt(dev, of_node, &pdata_of);
> + type = (unsigned long) id->data;
> + } else {
> + type = i2c_id->driver_data;
> + }
> +
> + switch (type) {
> + case ACT8865:
> + matches = act8865_matches;
> + regulators = act8865_regulators;
> + num_regulators = ARRAY_SIZE(act8865_regulators);
> + break;
> + default:
> + dev_err(dev, "invalid device id %lu\n", type);
> + return -EINVAL;
> + }
> +
> + of_node = devm_kzalloc(dev, sizeof(struct device_node *) *
> + num_regulators, GFP_KERNEL);
> + if (!of_node)
> + return -ENOMEM;
> +
> + if (dev->of_node && !pdata) {
> + ret = act8865_pdata_from_dt(dev, of_node, &pdata_of,
> matches,
> + num_regulators);
> if (ret < 0)
> return ret;
>
> pdata = &pdata_of;
> }
>
> - if (pdata->num_regulators > ACT8865_REG_NUM) {
> - dev_err(dev, "Too many regulators found!\n");
> + if (pdata->num_regulators > num_regulators) {
> + dev_err(dev, "too many regulators: %d\n",
> + pdata->num_regulators);
> return -EINVAL;
> }
>
> @@ -314,8 +265,10 @@ static int act8865_pmic_probe(struct i2c_client
> *client,
> }
>
> /* Finally register devices */
> - for (i = 0; i < ACT8865_REG_NUM; i++) {
> - const struct regulator_desc *desc = &act8865_reg[i];
> + for (i = 0; i < num_regulators; i++) {
> + const struct regulator_desc *desc = &regulators[i];
> + struct regulator_config config = { };
> + struct regulator_dev *rdev;
>
> config.dev = dev;
> config.init_data = act8865_get_init_data(desc->id, pdata);
> @@ -331,12 +284,13 @@ static int act8865_pmic_probe(struct i2c_client
> *client,
> }
>
> i2c_set_clientdata(client, act8865);
> + devm_kfree(dev, of_node);
>
> return 0;
> }
>
> static const struct i2c_device_id act8865_ids[] = {
> - { "act8865", 0 },
> + { .name = "act8865", .driver_data = ACT8865 },
> { },
> };
> MODULE_DEVICE_TABLE(i2c, act8865_ids);
> @@ -352,6 +306,6 @@ static struct i2c_driver act8865_pmic_driver = {
>
> module_i2c_driver(act8865_pmic_driver);
>
> -MODULE_DESCRIPTION("active-semi act8865 voltage regulator driver");
> +MODULE_DESCRIPTION("active-semi act88xx voltage regulator driver");
> MODULE_AUTHOR("Wenyou Yang <[email protected]>");
> MODULE_LICENSE("GPL v2"); diff --git
> a/include/linux/regulator/act8865.h b/include/linux/regulator/act8865.h
> index 49206c1..b49be81 100644
> --- a/include/linux/regulator/act8865.h
> +++ b/include/linux/regulator/act8865.h
> @@ -1,5 +1,5 @@
> /*
> - * act8865.h -- Voltage regulation for the active-semi act8865
> + * act8865.h -- Voltage regulation for active-semi act88xx PMUs
> *
> * Copyright (C) 2013 Atmel Corporation.
> *
> @@ -29,6 +29,10 @@ enum {
> ACT8865_REG_NUM,
> };
>
> +enum {
> + ACT8865,
> +};
> +
> /**
> * act8865_regulator_data - regulator data
> * @id: regulator id
> --
> 1.7.10.4

Tested on at91-sama5d3_xplained with ACT8865

Tested-by Wenyou.Yang <[email protected]>

Best Regards,
Wenyou Yang

2014-06-27 16:08:47

[permalink] [raw]

Subject: Re: [PATCH 2/5] regulator: act8865: set correct number of regulators in pdata

On Sun, Jun 22, 2014 at 05:31:42PM +0200, Beniamino Galvani wrote:
> When platform data is populated from DT all the regulators are
> instantiated and the value of num_regulators should be the number of
> all available regulators rather than the number of matched ones.

Could you go into more detail on your logic on this one please? The
platform data (and DT configuration) for regulators should be completely
optional.

Attachments:

(No filename) (423.00 B)
signature.asc (819.00 B)
Digital signature Download all attachments

2014-06-27 16:09:36

[permalink] [raw]

Subject: Re: [PATCH 1/5] regulator: act8865: fix parsing of platform data

On Sun, Jun 22, 2014 at 05:31:41PM +0200, Beniamino Galvani wrote:
> The driver loops through all available regulators (ACT8865_REG_NUM)
> and accesses pdata->regulators[i].platform_data without checking the
> actual value of num_regulators in platform data, potentially causing a
> invalid memory access.

Applied, thanks.

Attachments:

(No filename) (324.00 B)
signature.asc (819.00 B)
Digital signature Download all attachments

2014-06-29 09:48:11

[permalink] [raw]

Subject: Re: [PATCH 2/5] regulator: act8865: set correct number of regulators in pdata

On Fri, Jun 27, 2014 at 05:07:25PM +0100, Mark Brown wrote:
> On Sun, Jun 22, 2014 at 05:31:42PM +0200, Beniamino Galvani wrote:
> > When platform data is populated from DT all the regulators are
> > instantiated and the value of num_regulators should be the number of
> > all available regulators rather than the number of matched ones.
>
> Could you go into more detail on your logic on this one please? The
> platform data (and DT configuration) for regulators should be completely
> optional.

pdata contains an array of struct act8865_regulator_data and a
num_regulators field which should hold the number of elements in the
array, if I understand correctly.

So in the current code, when populating platform data from DT, there
is a mismatch between the number of elements that are put in the array
(ARRAY_SIZE(act8865_matches)) and the value of num_regulators, which
is set to the number of regulators found in the DT.

In case of missing regulator nodes in DT, pdata->regulators will be
populated with all the regulators but their init_data will be NULL.

Beniamino

2014-07-03 15:56:28

[permalink] [raw]

Subject: Re: [PATCH 2/5] regulator: act8865: set correct number of regulators in pdata

On Sun, Jun 29, 2014 at 11:46:43AM +0200, Beniamino Galvani wrote:
> On Fri, Jun 27, 2014 at 05:07:25PM +0100, Mark Brown wrote:
> > On Sun, Jun 22, 2014 at 05:31:42PM +0200, Beniamino Galvani wrote:
> > > When platform data is populated from DT all the regulators are
> > > instantiated and the value of num_regulators should be the number of
> > > all available regulators rather than the number of matched ones.

> > Could you go into more detail on your logic on this one please? The
> > platform data (and DT configuration) for regulators should be completely
> > optional.

> pdata contains an array of struct act8865_regulator_data and a
> num_regulators field which should hold the number of elements in the
> array, if I understand correctly.

> So in the current code, when populating platform data from DT, there
> is a mismatch between the number of elements that are put in the array
> (ARRAY_SIZE(act8865_matches)) and the value of num_regulators, which
> is set to the number of regulators found in the DT.

> In case of missing regulator nodes in DT, pdata->regulators will be
> populated with all the regulators but their init_data will be NULL.

OK, so that was slightly unclear but I see what you're saying now - the
issue isn't that there will be fewer valid elements in the array, it's
that the array is sparsely populated.

Attachments:

(No filename) (1.31 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2014-07-03 15:59:32

[permalink] [raw]

Subject: Re: [PATCH 2/5] regulator: act8865: set correct number of regulators in pdata

Attachments:

(No filename) (288.00 B)
signature.asc (819.00 B)
Digital signature Download all attachments

2014-07-03 16:00:54