Add the regulator driver for PMIC 88pm800 including device tree
support.
88pm800 is an I2C-based power-management IC containing voltage
regulators, a real-time clock, and some general purpose ADC devices,
Signed-off-by: Yi Zhang <[email protected]>
Signed-off-by: Chao Xie <[email protected]>
---
.../devicetree/bindings/regulator/88pm800.txt | 38 ++
drivers/regulator/88pm800.c | 455 ++++++++++++++++++++
drivers/regulator/Kconfig | 10 +
drivers/regulator/Makefile | 1 +
4 files changed, 504 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/regulator/88pm800.txt
create mode 100644 drivers/regulator/88pm800.c
diff --git a/Documentation/devicetree/bindings/regulator/88pm800.txt b/Documentation/devicetree/bindings/regulator/88pm800.txt
new file mode 100644
index 0000000..e8a54c2
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/88pm800.txt
@@ -0,0 +1,38 @@
+Marvell 88PM800 regulator
+
+Required properties:
+- compatible: "marvell,88pm800"
+- reg: I2C slave address
+- regulators: A node that houses a sub-node for each regulator within the
+ device. Each sub-node is identified using the node's name (or the deprecated
+ regulator-compatible property if present), with valid values listed below.
+ The content of each sub-node is defined by the standard binding for
+ regulators; see regulator.txt.
+
+The valid names for regulators are:
+
+ buck1, buck2, buck3, buck4, buck5, ldo1, ldo2, ldo3, ldo4, ldo5, ldo6, ldo7,
+ ldo8, ldo9, ldo10, ldo11, ldo12, ldo13, ldo14, ldo15, ldo16, ldo17, ldo18, ldo19
+
+Example:
+
+ pmic: 88pm800@31 {
+ compatible = "marvell,88pm800";
+ reg = <0x31>;
+
+ regulators {
+ buck1 {
+ regulator-min-microvolt = <600000>;
+ regulator-max-microvolt = <3950000>;
+ regulator-boot-on;
+ regulator-always-on;
+ };
+ ldo1 {
+ regulator-min-microvolt = <600000>;
+ regulator-max-microvolt = <15000000>;
+ regulator-boot-on;
+ regulator-always-on;
+ };
+...
+ };
+ };
diff --git a/drivers/regulator/88pm800.c b/drivers/regulator/88pm800.c
new file mode 100644
index 0000000..89f52b4
--- /dev/null
+++ b/drivers/regulator/88pm800.c
@@ -0,0 +1,455 @@
+/*
+ * Regulators driver for Marvell 88PM800
+ *
+ * Copyright (C) 2012 Marvell International Ltd.
+ * Joseph(Yossi) Hanin <[email protected]>
+ * Yi Zhang <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/mfd/88pm80x.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/regulator/of_regulator.h>
+
+/* LDO1 with DVC[0..3] */
+#define PM800_LDO1_VOUT (0x08) /* VOUT1 */
+#define PM800_LDO1_VOUT_2 (0x09)
+#define PM800_LDO1_VOUT_3 (0x0A)
+#define PM800_LDO2_VOUT (0x0B)
+#define PM800_LDO3_VOUT (0x0C)
+#define PM800_LDO4_VOUT (0x0D)
+#define PM800_LDO5_VOUT (0x0E)
+#define PM800_LDO6_VOUT (0x0F)
+#define PM800_LDO7_VOUT (0x10)
+#define PM800_LDO8_VOUT (0x11)
+#define PM800_LDO9_VOUT (0x12)
+#define PM800_LDO10_VOUT (0x13)
+#define PM800_LDO11_VOUT (0x14)
+#define PM800_LDO12_VOUT (0x15)
+#define PM800_LDO13_VOUT (0x16)
+#define PM800_LDO14_VOUT (0x17)
+#define PM800_LDO15_VOUT (0x18)
+#define PM800_LDO16_VOUT (0x19)
+#define PM800_LDO17_VOUT (0x1A)
+#define PM800_LDO18_VOUT (0x1B)
+#define PM800_LDO19_VOUT (0x1C)
+
+/* BUCK1 with DVC[0..3] */
+#define PM800_BUCK1 (0x3C)
+#define PM800_BUCK1_1 (0x3D)
+#define PM800_BUCK1_2 (0x3E)
+#define PM800_BUCK1_3 (0x3F)
+#define PM800_BUCK2 (0x40)
+#define PM800_BUCK3 (0x41)
+#define PM800_BUCK3 (0x41)
+#define PM800_BUCK4 (0x42)
+#define PM800_BUCK4_1 (0x43)
+#define PM800_BUCK4_2 (0x44)
+#define PM800_BUCK4_3 (0x45)
+#define PM800_BUCK5 (0x46)
+
+#define PM800_BUCK_ENA (0x50)
+#define PM800_LDO_ENA1_1 (0x51)
+#define PM800_LDO_ENA1_2 (0x52)
+#define PM800_LDO_ENA1_3 (0x53)
+
+#define PM800_LDO_ENA2_1 (0x56)
+#define PM800_LDO_ENA2_2 (0x57)
+#define PM800_LDO_ENA2_3 (0x58)
+
+#define PM800_BUCK1_MISC1 (0x78)
+#define PM800_BUCK3_MISC1 (0x7E)
+#define PM800_BUCK4_MISC1 (0x81)
+#define PM800_BUCK5_MISC1 (0x84)
+
+struct pm800_regulator_volt_range {
+ int min_uv;
+ int max_uv;
+ int step_uv;
+ /* the register value for min_uv */
+ int min_val;
+};
+
+struct pm800_regulator_info {
+ struct regulator_desc desc;
+ int max_ua;
+
+ struct pm800_regulator_volt_range *volt;
+ unsigned int ranges;
+};
+
+struct pm800_regulators {
+ struct regulator_dev *regulators[PM800_ID_RG_MAX];
+ struct pm80x_chip *chip;
+ struct regmap *map;
+};
+
+/*
+ * vreg - the buck regs string.
+ * ereg - the string for the enable register.
+ * ebit - the bit number in the enable register.
+ * amax - the current
+ * Buck has 2 kinds of voltage steps. It is easy to find voltage by ranges,
+ * not the constant voltage table.
+ */
+#define PM800_BUCK(vreg, ereg, ebit, amax, volt_ranges) \
+{ \
+ .desc = { \
+ .name = #vreg, \
+ .ops = &pm800_volt_range_ops, \
+ .type = REGULATOR_VOLTAGE, \
+ .id = PM800_ID_##vreg, \
+ .owner = THIS_MODULE, \
+ .vsel_reg = PM800_##vreg, \
+ .vsel_mask = 0x7f, \
+ .enable_reg = PM800_##ereg, \
+ .enable_mask = 1 << (ebit), \
+ }, \
+ .max_ua = (amax), \
+ .volt = (volt_ranges), \
+ .ranges = ARRAY_SIZE(volt_ranges), \
+}
+
+/*
+ * vreg - the LDO regs string
+ * ereg - the string for the enable register.
+ * ebit - the bit number in the enable register.
+ * amax - the current
+ * volt_table - the LDO voltage table
+ * For all the LDOes, there are too many ranges. Using volt_table will be
+ * simpler and faster.
+ */
+#define PM800_LDO(vreg, ereg, ebit, amax, ldo_volt_table) \
+{ \
+ .desc = { \
+ .name = #vreg, \
+ .ops = &pm800_volt_table_ops, \
+ .type = REGULATOR_VOLTAGE, \
+ .id = PM800_ID_##vreg, \
+ .owner = THIS_MODULE, \
+ .n_voltages = ARRAY_SIZE(ldo_volt_table), \
+ .vsel_reg = PM800_##vreg##_VOUT, \
+ .vsel_mask = 0x1f, \
+ .enable_reg = PM800_##ereg, \
+ .enable_mask = 1 << (ebit), \
+ .volt_table = ldo_volt_table, \
+ }, \
+ .max_ua = (amax), \
+}
+
+/* Ranges are sorted in ascending order. */
+static struct pm800_regulator_volt_range buck1_volt_range[] = {
+ {600000, 1587500, 12500, 0x0},
+ {1600000, 1800000, 50000, 0x50},
+};
+
+/* BUCK 2~5 have same ranges. */
+static struct pm800_regulator_volt_range buck2_5_volt_range[] = {
+ {600000, 1587500, 12500, 0x0},
+ {1600000, 3300000, 50000, 0x50},
+};
+
+static const unsigned int ldo1_volt_table[] = {
+ 600000, 650000, 700000, 750000, 800000, 850000, 900000, 950000,
+ 1000000, 1050000, 1100000, 1150000, 1200000, 1300000, 1400000, 1500000,
+};
+
+static const unsigned int ldo2_volt_table[] = {
+ 1700000, 1800000, 1900000, 2000000, 2100000, 2500000, 2700000, 2800000,
+};
+
+/* LDO 3~17 have same voltage table. */
+static const unsigned int ldo3_17_volt_table[] = {
+ 1200000, 1250000, 1700000, 1800000, 1850000, 1900000, 2500000, 2600000,
+ 2700000, 2750000, 2800000, 2850000, 2900000, 3000000, 3100000, 3300000,
+};
+
+/* LDO 18~19 have same voltage table. */
+static const unsigned int ldo18_19_volt_table[] = {
+ 1700000, 1800000, 1900000, 2500000, 2800000, 2900000, 3100000, 3300000,
+};
+
+static int pm800_get_current_limit(struct regulator_dev *rdev)
+{
+ struct pm800_regulator_info *info = rdev_get_drvdata(rdev);
+
+ return info->max_ua;
+}
+
+static int pm800_set_voltage(struct regulator_dev *rdev,
+ int min_uv, int max_uv, unsigned *selector)
+{
+ struct pm800_regulator_info *info = rdev_get_drvdata(rdev);
+ struct pm800_regulator_volt_range *range;
+ int i, best_index = -1;
+
+ if (info->volt == NULL)
+ return -EINVAL;
+
+ /*
+ * Ranges are sorted in ascending order. So if we found a best_uv
+ * in this range, we can break out.
+ */
+ for (i = 0; i < info->ranges; i++) {
+ range = &info->volt[i];
+
+ if (min_uv <= range->max_uv && max_uv >= range->min_uv) {
+ if (min_uv <= range->min_uv)
+ best_index = 0;
+ else
+ best_index = (min_uv - range->min_uv +
+ range->step_uv - 1) / range->step_uv;
+ break;
+ }
+ }
+
+ if (best_index == -1)
+ return -EINVAL;
+
+ *selector = best_index + range->min_val;
+
+ return regulator_set_voltage_sel_regmap(rdev, *selector);
+}
+
+static int pm800_get_voltage(struct regulator_dev *rdev)
+{
+ struct pm800_regulator_info *info = rdev_get_drvdata(rdev);
+ struct pm800_regulator_volt_range *range;
+ int i, val, max_val, volt = -EINVAL;
+
+ if (info->volt == NULL)
+ return -EINVAL;
+
+ val = regulator_get_voltage_sel_regmap(rdev);
+ if (val < 0)
+ return val;
+
+ /* get the voltage via the register value */
+ for (i = 0; i < info->ranges; i++) {
+ range = &info->volt[i];
+ max_val = (range->max_uv - range->min_uv) / range->step_uv;
+
+ if (val >= range->min_val && val <= max_val) {
+ volt = (val - range->min_val) * range->step_uv
+ + range->min_uv;
+ break;
+ }
+ }
+
+ return volt;
+}
+
+static struct regulator_ops pm800_volt_range_ops = {
+ .set_voltage = pm800_set_voltage,
+ .get_voltage = pm800_get_voltage,
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .get_current_limit = pm800_get_current_limit,
+};
+
+static struct regulator_ops pm800_volt_table_ops = {
+ .list_voltage = regulator_list_voltage_table,
+ .map_voltage = regulator_map_voltage_iterate,
+ .set_voltage_sel = regulator_set_voltage_sel_regmap,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .get_current_limit = pm800_get_current_limit,
+};
+
+/* The array is indexed by id(PM800_ID_XXX) */
+static struct pm800_regulator_info pm800_regulator_info[] = {
+ PM800_BUCK(BUCK1, BUCK_ENA, 0, 3000000, buck1_volt_range),
+ PM800_BUCK(BUCK2, BUCK_ENA, 1, 1200000, buck2_5_volt_range),
+ PM800_BUCK(BUCK3, BUCK_ENA, 2, 1200000, buck2_5_volt_range),
+ PM800_BUCK(BUCK4, BUCK_ENA, 3, 1200000, buck2_5_volt_range),
+ PM800_BUCK(BUCK5, BUCK_ENA, 4, 1200000, buck2_5_volt_range),
+
+ PM800_LDO(LDO1, LDO_ENA1_1, 0, 200000, ldo1_volt_table),
+ PM800_LDO(LDO2, LDO_ENA1_1, 1, 10000, ldo2_volt_table),
+ PM800_LDO(LDO3, LDO_ENA1_1, 2, 300000, ldo3_17_volt_table),
+ PM800_LDO(LDO4, LDO_ENA1_1, 3, 300000, ldo3_17_volt_table),
+ PM800_LDO(LDO5, LDO_ENA1_1, 4, 300000, ldo3_17_volt_table),
+ PM800_LDO(LDO6, LDO_ENA1_1, 5, 300000, ldo3_17_volt_table),
+ PM800_LDO(LDO7, LDO_ENA1_1, 6, 300000, ldo3_17_volt_table),
+ PM800_LDO(LDO8, LDO_ENA1_1, 7, 300000, ldo3_17_volt_table),
+ PM800_LDO(LDO9, LDO_ENA1_2, 0, 300000, ldo3_17_volt_table),
+ PM800_LDO(LDO10, LDO_ENA1_2, 1, 300000, ldo3_17_volt_table),
+ PM800_LDO(LDO11, LDO_ENA1_2, 2, 300000, ldo3_17_volt_table),
+ PM800_LDO(LDO12, LDO_ENA1_2, 3, 300000, ldo3_17_volt_table),
+ PM800_LDO(LDO13, LDO_ENA1_2, 4, 300000, ldo3_17_volt_table),
+ PM800_LDO(LDO14, LDO_ENA1_2, 5, 300000, ldo3_17_volt_table),
+ PM800_LDO(LDO15, LDO_ENA1_2, 6, 300000, ldo3_17_volt_table),
+ PM800_LDO(LDO16, LDO_ENA1_2, 7, 300000, ldo3_17_volt_table),
+ PM800_LDO(LDO17, LDO_ENA1_3, 0, 300000, ldo3_17_volt_table),
+ PM800_LDO(LDO18, LDO_ENA1_3, 1, 200000, ldo18_19_volt_table),
+ PM800_LDO(LDO19, LDO_ENA1_3, 2, 200000, ldo18_19_volt_table),
+};
+
+#define PM800_REGULATOR_OF_MATCH(_name, _id) \
+ [PM800_ID_##_id] = { \
+ .name = #_name, \
+ .driver_data = &pm800_regulator_info[PM800_ID_##_id], \
+ }
+
+static struct of_regulator_match pm800_regulator_matches[] = {
+ PM800_REGULATOR_OF_MATCH(buck1, BUCK1),
+ PM800_REGULATOR_OF_MATCH(buck2, BUCK2),
+ PM800_REGULATOR_OF_MATCH(buck3, BUCK3),
+ PM800_REGULATOR_OF_MATCH(buck4, BUCK4),
+ PM800_REGULATOR_OF_MATCH(buck5, BUCK5),
+ PM800_REGULATOR_OF_MATCH(ldo1, LDO1),
+ PM800_REGULATOR_OF_MATCH(ldo2, LDO2),
+ PM800_REGULATOR_OF_MATCH(ldo3, LDO3),
+ PM800_REGULATOR_OF_MATCH(ldo4, LDO4),
+ PM800_REGULATOR_OF_MATCH(ldo5, LDO5),
+ PM800_REGULATOR_OF_MATCH(ldo6, LDO6),
+ PM800_REGULATOR_OF_MATCH(ldo7, LDO7),
+ PM800_REGULATOR_OF_MATCH(ldo8, LDO8),
+ PM800_REGULATOR_OF_MATCH(ldo9, LDO9),
+ PM800_REGULATOR_OF_MATCH(ldo10, LDO10),
+ PM800_REGULATOR_OF_MATCH(ldo11, LDO11),
+ PM800_REGULATOR_OF_MATCH(ldo12, LDO12),
+ PM800_REGULATOR_OF_MATCH(ldo13, LDO13),
+ PM800_REGULATOR_OF_MATCH(ldo14, LDO14),
+ PM800_REGULATOR_OF_MATCH(ldo15, LDO15),
+ PM800_REGULATOR_OF_MATCH(ldo16, LDO16),
+ PM800_REGULATOR_OF_MATCH(ldo17, LDO17),
+ PM800_REGULATOR_OF_MATCH(ldo18, LDO18),
+ PM800_REGULATOR_OF_MATCH(ldo19, LDO19),
+};
+
+static int pm800_regulator_dt_init(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ int ret;
+
+ ret = of_regulator_match(&pdev->dev, np,
+ pm800_regulator_matches,
+ ARRAY_SIZE(pm800_regulator_matches));
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int pm800_regulator_probe(struct platform_device *pdev)
+{
+ struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
+ struct pm80x_platform_data *pdata = pdev->dev.parent->platform_data;
+ struct pm800_regulators *pm800_data;
+ struct pm800_regulator_info *info;
+ struct regulator_config config = { };
+ struct regulator_init_data *init_data;
+ int i, ret;
+
+ if (!pdata || pdata->num_regulators == 0) {
+ if (IS_ENABLED(CONFIG_OF)) {
+ ret = pm800_regulator_dt_init(pdev);
+ if (ret)
+ return ret;
+ } else {
+ return -ENODEV;
+ }
+ } else if (pdata->num_regulators) {
+ /* Check whether num_regulator is valid. */
+ unsigned int count = 0;
+ for (i = 0; pdata->regulators[i]; i++)
+ count++;
+ if (count != pdata->num_regulators)
+ return -EINVAL;
+ } else {
+ return -EINVAL;
+ }
+
+ pm800_data = devm_kzalloc(&pdev->dev, sizeof(*pm800_data),
+ GFP_KERNEL);
+ if (!pm800_data) {
+ dev_err(&pdev->dev, "Failed to allocate pm800_regualtors");
+ return -ENOMEM;
+ }
+
+ pm800_data->map = chip->subchip->regmap_power;
+ pm800_data->chip = chip;
+
+ platform_set_drvdata(pdev, pm800_data);
+
+ for (i = 0; i < PM800_ID_RG_MAX; i++) {
+ if (!pdata || pdata->num_regulators == 0)
+ init_data = pm800_regulator_matches[i].init_data;
+ else
+ init_data = pdata->regulators[i];
+ if (!init_data)
+ continue;
+ info = pm800_regulator_matches[i].driver_data;
+ config.dev = &pdev->dev;
+ config.init_data = init_data;
+ config.driver_data = info;
+ config.regmap = pm800_data->map;
+
+ pm800_data->regulators[i] =
+ regulator_register(&info->desc, &config);
+ if (IS_ERR(pm800_data->regulators[i])) {
+ ret = PTR_ERR(pm800_data->regulators[i]);
+ dev_err(&pdev->dev, "Failed to register %s\n",
+ info->desc.name);
+
+ while (--i >= 0 && pm800_data->regulators[i])
+ regulator_unregister(pm800_data->regulators[i]);
+
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static int pm800_regulator_remove(struct platform_device *pdev)
+{
+ struct pm800_regulators *pm800_data = platform_get_drvdata(pdev);
+ int i;
+
+ for (i = 0; pm800_data->regulators[i] && i < PM800_ID_RG_MAX; i++)
+ regulator_unregister(pm800_data->regulators[i]);
+
+ return 0;
+}
+
+static struct platform_driver pm800_regulator_driver = {
+ .driver = {
+ .name = "88pm80x-regulator",
+ .owner = THIS_MODULE,
+ },
+ .probe = pm800_regulator_probe,
+ .remove = pm800_regulator_remove,
+};
+
+static int __init pm800_regulator_init(void)
+{
+ return platform_driver_register(&pm800_regulator_driver);
+}
+subsys_initcall(pm800_regulator_init);
+
+static void __exit pm800_regulator_exit(void)
+{
+ platform_driver_unregister(&pm800_regulator_driver);
+}
+module_exit(pm800_regulator_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Joseph(Yossi) Hanin <[email protected]>");
+MODULE_DESCRIPTION("Regulator Driver for Marvell 88PM800 PMIC");
+MODULE_ALIAS("platform:88pm800-regulator");
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index a5d97ea..4521e34 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -514,5 +514,15 @@ config REGULATOR_AS3711
This driver provides support for the voltage regulators on the
AS3711 PMIC
+config REGULATOR_88PM800
+ bool "Marvell 88PM800 Power regulators"
+ depends on MFD_88PM800=y
+ help
+ This driver supports Marvell 88PM800 voltage regulator chips.
+ It delivers digitally programmable output,
+ the voltage is programmed via I2C interface.
+ It's suitable to support PXA988 chips to control VCC_MAIN and
+ various voltages.
+
endif
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 6e82503..015e5c2 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -70,6 +70,7 @@ obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o
obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o
+obj-$(CONFIG_REGULATOR_88PM800) += 88pm800.o
ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
--
1.7.4.1
On Thu, Jun 20, 2013 at 03:31:07AM -0400, Chao Xie wrote:
Looks pretty good, main thing here is that some of the functionality
here appears to be reproducing standard code.
> +static int pm800_set_voltage(struct regulator_dev *rdev,
> + int min_uv, int max_uv, unsigned *selector)
> +{
Use set_voltage_sel() and map_voltge_ascend()
> +static int pm800_get_voltage(struct regulator_dev *rdev)
> +{
> +
Just provide get_voltage_sel(), the core will do the mapping to voltages
using list_voltage().
> + } else if (pdata->num_regulators) {
> + /* Check whether num_regulator is valid. */
> + unsigned int count = 0;
> + for (i = 0; pdata->regulators[i]; i++)
> + count++;
> + if (count != pdata->num_regulators)
> + return -EINVAL;
This looks... odd.
> +static int __init pm800_regulator_init(void)
> +{
> + return platform_driver_register(&pm800_regulator_driver);
> +}
> +subsys_initcall(pm800_regulator_init);
> +
> +static void __exit pm800_regulator_exit(void)
> +{
> + platform_driver_unregister(&pm800_regulator_driver);
> +}
> +module_exit(pm800_regulator_exit);
With deferred probing you should just be able to use
module_platform_driver().
> +config REGULATOR_88PM800
> + bool "Marvell 88PM800 Power regulators"
> + depends on MFD_88PM800=y
> + help
> + This driver supports Marvell 88PM800 voltage regulator chips.
> + It delivers digitally programmable output,
> + the voltage is programmed via I2C interface.
> + It's suitable to support PXA988 chips to control VCC_MAIN and
> + various voltages.
> +
Keep this file sorted please.
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o
> obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
> obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
> obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o
> +obj-$(CONFIG_REGULATOR_88PM800) += 88pm800.o
Same here.
On Fri, Jun 21, 2013 at 11:24 PM, Mark Brown <[email protected]> wrote:
> On Thu, Jun 20, 2013 at 03:31:07AM -0400, Chao Xie wrote:
>
> Looks pretty good, main thing here is that some of the functionality
> here appears to be reproducing standard code.
>
>> +static int pm800_set_voltage(struct regulator_dev *rdev,
>> + int min_uv, int max_uv, unsigned *selector)
>> +{
>
> Use set_voltage_sel() and map_voltge_ascend()
>
>> +static int pm800_get_voltage(struct regulator_dev *rdev)
>> +{
>> +
>
> Just provide get_voltage_sel(), the core will do the mapping to voltages
> using list_voltage().
>
I am a little confused.
The BUCK voltage is not linear, and it contains a lot of voltages.
If we use map_voltage_ascend, it will looking a a suitable voltage
from begin to end one by one.
It will waist of time.
If we directly make use of set_voltage and get_voltage, we can
directly calculates the voltage which is
suitable, and do not go through all the voltags.
for example, now BUCK voltage table is
range 1 from 600000 to 1587500, each step is 12500
range 2 from 1600000 to 1800000, each step is 50000
When set_voltage(1680000, 1720000)
use map_voltage_ascend, it will go through all voltages range 1, and
then find a suitable voltage in range2
for our code, we direclty find the the voltage is in range2, all need
is (min_uv - range->min_uv)/range->step.
We can get the index, and plus the index with range->index_start, then
we can set the value to register.
>> + } else if (pdata->num_regulators) {
>> + /* Check whether num_regulator is valid. */
>> + unsigned int count = 0;
>> + for (i = 0; pdata->regulators[i]; i++)
>> + count++;
>> + if (count != pdata->num_regulators)
>> + return -EINVAL;
>
> This looks... odd.
>
It is just make sure that pdata has correct number of regulators.
It you think that it is redundant, i can remove it.
>> +static int __init pm800_regulator_init(void)
>> +{
>> + return platform_driver_register(&pm800_regulator_driver);
>> +}
>> +subsys_initcall(pm800_regulator_init);
>> +
>> +static void __exit pm800_regulator_exit(void)
>> +{
>> + platform_driver_unregister(&pm800_regulator_driver);
>> +}
>> +module_exit(pm800_regulator_exit);
>
> With deferred probing you should just be able to use
> module_platform_driver().
>
The regulator controlles some BUCK regulators.
These regulators may be used by application CPU or CP(communication
CPU) for telephony.
The CP may need different voltages if it goes deep initialization. if
we defer the setting later, it is too late for
CP initialization, and will impact the performance.
>> +config REGULATOR_88PM800
>> + bool "Marvell 88PM800 Power regulators"
>> + depends on MFD_88PM800=y
>> + help
>> + This driver supports Marvell 88PM800 voltage regulator chips.
>> + It delivers digitally programmable output,
>> + the voltage is programmed via I2C interface.
>> + It's suitable to support PXA988 chips to control VCC_MAIN and
>> + various voltages.
>> +
>
> Keep this file sorted please.
>
I will change it.
>> --- a/drivers/regulator/Makefile
>> +++ b/drivers/regulator/Makefile
>> @@ -70,6 +70,7 @@ obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o
>> obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
>> obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
>> obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o
>> +obj-$(CONFIG_REGULATOR_88PM800) += 88pm800.o
>
> Same here.
I will change it.
On Mon, Jun 24, 2013 at 10:01:39AM +0800, Chao Xie wrote:
> On Fri, Jun 21, 2013 at 11:24 PM, Mark Brown <[email protected]> wrote:
> > Just provide get_voltage_sel(), the core will do the mapping to voltages
> > using list_voltage().
> I am a little confused.
> The BUCK voltage is not linear, and it contains a lot of voltages.
> If we use map_voltage_ascend, it will looking a a suitable voltage
> from begin to end one by one.
What does this have to do with get_voltage()? And note that you can
write your own mapping function for a reason...
> If we directly make use of set_voltage and get_voltage, we can
> directly calculates the voltage which is
> suitable, and do not go through all the voltags.
> for example, now BUCK voltage table is
> range 1 from 600000 to 1587500, each step is 12500
> range 2 from 1600000 to 1800000, each step is 50000
No, this is nothing at all to do with using the selector versions of the
API. Think about what the API is doing and take a look at the code.
> >> + } else if (pdata->num_regulators) {
> >> + /* Check whether num_regulator is valid. */
> >> + unsigned int count = 0;
> >> + for (i = 0; pdata->regulators[i]; i++)
> >> + count++;
> >> + if (count != pdata->num_regulators)
> >> + return -EINVAL;
> > This looks... odd.
> It is just make sure that pdata has correct number of regulators.
> It you think that it is redundant, i can remove it.
If you really need to have platform data for all the regulators then
just embed the array inside the platform data so there's no possibility
of any confusion.
> > With deferred probing you should just be able to use
> > module_platform_driver().
> The regulator controlles some BUCK regulators.
> These regulators may be used by application CPU or CP(communication
> CPU) for telephony.
> The CP may need different voltages if it goes deep initialization. if
> we defer the setting later, it is too late for
> CP initialization, and will impact the performance.
If your kernel startup is taking long enough for this to be an issue it
seems like there's much bigger problems here and things are going to be
very fragile anyway, it's going to be better to figure out what the root
issue is.
On Mon, Jun 24, 2013 at 6:14 PM, Mark Brown <[email protected]> wrote:
> On Mon, Jun 24, 2013 at 10:01:39AM +0800, Chao Xie wrote:
>> On Fri, Jun 21, 2013 at 11:24 PM, Mark Brown <[email protected]> wrote:
>
>> > Just provide get_voltage_sel(), the core will do the mapping to voltages
>> > using list_voltage().
>
>> I am a little confused.
>> The BUCK voltage is not linear, and it contains a lot of voltages.
>> If we use map_voltage_ascend, it will looking a a suitable voltage
>> from begin to end one by one.
>
> What does this have to do with get_voltage()? And note that you can
> write your own mapping function for a reason...
>
>> If we directly make use of set_voltage and get_voltage, we can
>> directly calculates the voltage which is
>> suitable, and do not go through all the voltags.
>> for example, now BUCK voltage table is
>> range 1 from 600000 to 1587500, each step is 12500
>> range 2 from 1600000 to 1800000, each step is 50000
>
> No, this is nothing at all to do with using the selector versions of the
> API. Think about what the API is doing and take a look at the code.
>
OK. I will try to define my own mapping function and use the sel APIs.
>> >> + } else if (pdata->num_regulators) {
>> >> + /* Check whether num_regulator is valid. */
>> >> + unsigned int count = 0;
>> >> + for (i = 0; pdata->regulators[i]; i++)
>> >> + count++;
>> >> + if (count != pdata->num_regulators)
>> >> + return -EINVAL;
>
>> > This looks... odd.
>
>> It is just make sure that pdata has correct number of regulators.
>> It you think that it is redundant, i can remove it.
>
> If you really need to have platform data for all the regulators then
> just embed the array inside the platform data so there's no possibility
> of any confusion.
>
Some regulators will not be exported to kernel depending on boards which are
for special usage. So i do not use a array inside the platform data.
>> > With deferred probing you should just be able to use
>> > module_platform_driver().
>
>> The regulator controlles some BUCK regulators.
>> These regulators may be used by application CPU or CP(communication
>> CPU) for telephony.
>> The CP may need different voltages if it goes deep initialization. if
>> we defer the setting later, it is too late for
>> CP initialization, and will impact the performance.
>
> If your kernel startup is taking long enough for this to be an issue it
> seems like there's much bigger problems here and things are going to be
> very fragile anyway, it's going to be better to figure out what the root
> issue is.
I see. I will use module_platform_driver
On Tue, Jun 25, 2013 at 10:13:38AM +0800, Chao Xie wrote:
> On Mon, Jun 24, 2013 at 6:14 PM, Mark Brown <[email protected]> wrote:
> > If you really need to have platform data for all the regulators then
> > just embed the array inside the platform data so there's no possibility
> > of any confusion.
> Some regulators will not be exported to kernel depending on boards which are
> for special usage. So i do not use a array inside the platform data.
Remember that the regulator API won't do anything except read from a
regulator unless it's got constraints saying it's OK to do so. Are you
saying that even reading the hardware state is a problem?
On Tue, Jun 25, 2013 at 5:12 PM, Mark Brown <[email protected]> wrote:
> On Tue, Jun 25, 2013 at 10:13:38AM +0800, Chao Xie wrote:
>> On Mon, Jun 24, 2013 at 6:14 PM, Mark Brown <[email protected]> wrote:
>
>> > If you really need to have platform data for all the regulators then
>> > just embed the array inside the platform data so there's no possibility
>> > of any confusion.
>
>> Some regulators will not be exported to kernel depending on boards which are
>> for special usage. So i do not use a array inside the platform data.
>
> Remember that the regulator API won't do anything except read from a
> regulator unless it's got constraints saying it's OK to do so. Are you
> saying that even reading the hardware state is a problem?
The regulator driver does not only work for 88pm805 but also for the
next lite version 88pm822.
88pm822 only have 14 LDOes. So i defined array of points in pdata.
When user define pdata for 88pm805, it can fill all 19 LDOes, while
for 88pm822, it only fill 14 LDOes, and set
others to be NULL, and do not need fill some dummy data into the left 5 LDOes.
The 88pm822 is just tape out, and we are debugging at it. So the
detection of 88pm822 is not added to this driver yet.
On Wed, Jun 26, 2013 at 09:35:01AM +0800, Chao Xie wrote:
> The regulator driver does not only work for 88pm805 but also for the
> next lite version 88pm822.
> 88pm822 only have 14 LDOes. So i defined array of points in pdata.
> When user define pdata for 88pm805, it can fill all 19 LDOes, while
> for 88pm822, it only fill 14 LDOes, and set
> others to be NULL, and do not need fill some dummy data into the left 5 LDOes.
> The 88pm822 is just tape out, and we are debugging at it. So the
> detection of 88pm822 is not added to this driver yet.
Handling of multiple devices like that would normally be done by using
the device name that is registered.