The patch sets add support for Hi6220 PMIC Hi655x MFD core and its
regulator driver.
Current testing and support board is Hikey which is one of 96boards.
It is an arm64 open source board. For more information about this board,
please access https://www.96boards.org.
This is hardware layout for access PMIC Hi655x from AP SoC Hi6220.
Between PMIC Hi655x and Hi6220, the physical signal channel is SSI.
We can use memory-mapped I/O to communicate.
+----------------+ +-------------+
| | | |
| Hi6220 | SSI bus | Hi655x |
| |-------------| |
| |(REGMAP_MMIO)| |
+----------------+ +-------------+
V2: Code refactoring of regulator
V3: Drop mtcmos from this patch and use regmap-irq
The patch sets are based on 4.4-rc5.
Chen Feng (5):
doc: bindings: Add document for mfd hi665x PMIC
doc: bindings: Document for hi655x regulator driver
mfd: hi655x: Add hi665x pmic driver
regulator: add regulator driver of hi655x PMIC
hisilicon/dts: Add hi655x pmic dts node
.../devicetree/bindings/mfd/hisilicon,hi655x.txt | 17 ++
.../regulator/hisilicon,hi655x-regulator.txt | 43 ++++
arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 178 +++++++++++++++
drivers/mfd/Kconfig | 9 +
drivers/mfd/Makefile | 1 +
drivers/mfd/hi655x-pmic.c | 170 ++++++++++++++
drivers/regulator/Kconfig | 8 +
drivers/regulator/Makefile | 1 +
drivers/regulator/hi655x-regulator.c | 246 +++++++++++++++++++++
include/linux/mfd/hi655x-pmic.h | 56 +++++
include/linux/regulator/hi655x-regulator.h | 41 ++++
11 files changed, 770 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
create mode 100644 Documentation/devicetree/bindings/regulator/hisilicon,hi655x-regulator.txt
create mode 100644 drivers/mfd/hi655x-pmic.c
create mode 100644 drivers/regulator/hi655x-regulator.c
create mode 100644 include/linux/mfd/hi655x-pmic.h
create mode 100644 include/linux/regulator/hi655x-regulator.h
--
1.9.1
Add document for mfd driver hi655x pmic driver
Signed-off-by: Chen Feng <[email protected]>
Signed-off-by: Fei Wang <[email protected]>
Tested-by: Xinwei Kong <[email protected]>
---
.../devicetree/bindings/mfd/hisilicon,hi655x.txt | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
diff --git a/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt b/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
new file mode 100644
index 0000000..d1a2fa0
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
@@ -0,0 +1,17 @@
+Hisilicon hi655x Power Management Integrated Circuit (PMIC)
+
+Required properties:
+- compatible: Should be "hisilicon,hi655x-pmic"
+- reg: Base address of PMIC on hi6220 soc
+- interrupt-controller: Hi655x has internal IRQs (has own IRQ domain).
+- pmic-gpios: The gpio used by pmic irq.
+
+Example:
+ pmic: pmic@f8000000 {
+ compatible = "hisilicon,hi655x-pmic";
+ reg = <0x0 0xf8000000 0x0 0x1000>;
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ pmic-gpios = <&gpio_pmu_irq_n>;
+ status = "okay";
+ }
--
1.9.1
Add Document for hi655x pmic regulator driver
Signed-off-by: Chen Feng <[email protected]>
Signed-off-by: Fei Wang <[email protected]>
Tested-by: Xinwei Kong <[email protected]>
---
.../regulator/hisilicon,hi655x-regulator.txt | 43 ++++++++++++++++++++++
1 file changed, 43 insertions(+)
create mode 100644 Documentation/devicetree/bindings/regulator/hisilicon,hi655x-regulator.txt
diff --git a/Documentation/devicetree/bindings/regulator/hisilicon,hi655x-regulator.txt b/Documentation/devicetree/bindings/regulator/hisilicon,hi655x-regulator.txt
new file mode 100644
index 0000000..2eaebae
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/hisilicon,hi655x-regulator.txt
@@ -0,0 +1,43 @@
+Hisilicon Hi655x Voltage regulators
+
+Note:
+The hi655x regulator control is managed by hi655x Power IC.
+So the node of this regulator must be child node of hi655x
+pmic node.
+
+The driver uses the regulator core framework, so please also
+take the bindings of regulator.txt for reference.
+
+Required properties:
+- compatible: Must be "hisilicon,hi655x-regulator-pmic";
+- regulator-ctrl-regs: Registers offset of control register,
+ In turn with enable disable and status register offset.
+- regulator-ctrl-mask: The control mask bit of the register.
+- regulator-vset-regs: Voltage set register offset.
+- regulator-vset-mask: voltage set control mask.
+- regulator-n-vol: The num of support voltages.
+- regulator-vset-table: The table of support voltages.
+
+Example:
+ pmic: pmic@f8000000 {
+ compatible = "hisilicon,hi655x-pmic";
+ ...
+ ldo7: regulator@a26 {
+ compatible = "hisilicon,hi655x-regulator-pmic";
+ regulator-name = "ldo7";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-valid-modes-mask = <0x0a>;
+ regulator-enable-ramp-delay = <120>;
+ regulator-ctrl-regs = <0x029 0x02a 0x02b>;
+ regulator-ctrl-mask = <0x6>;
+ regulator-vset-regs = <0x078>;
+ regulator-vset-mask = <0x7>;
+ regulator-n-vol = <8>;
+ regulator-vset-table = <1800000>,<1850000>,
+ <2850000>,<2900000>,
+ <3000000>,<3100000>,
+ <3200000>,<3300000>;
+ };
+ ...
+ }
--
1.9.1
Add mfd driver to support hisilicon hi665x PMIC.
Signed-off-by: Chen Feng <[email protected]>
Signed-off-by: Fei Wang <[email protected]>
Tested-by: Xinwei Kong <[email protected]>
---
drivers/mfd/Kconfig | 9 +++
drivers/mfd/Makefile | 1 +
drivers/mfd/hi655x-pmic.c | 170 ++++++++++++++++++++++++++++++++++++++++
include/linux/mfd/hi655x-pmic.h | 56 +++++++++++++
4 files changed, 236 insertions(+)
create mode 100644 drivers/mfd/hi655x-pmic.c
create mode 100644 include/linux/mfd/hi655x-pmic.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 4d92df6..cb70b53 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -284,6 +284,15 @@ config MFD_HI6421_PMIC
menus in order to enable them.
We communicate with the Hi6421 via memory-mapped I/O.
+config MFD_HI655X_PMIC
+ tristate "HiSilicon Hi655X series PMU/Codec IC"
+ depends on OF
+ select MFD_CORE
+ select REGMAP_MMIO
+ select REGMAP_IRQ
+ help
+ Select this option to enable Hisilicon hi655x series pmic driver.
+
config HTC_EGPIO
bool "HTC EGPIO support"
depends on GPIOLIB && ARM
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index a8b76b8..6a7b0e1 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -186,6 +186,7 @@ obj-$(CONFIG_MFD_STW481X) += stw481x.o
obj-$(CONFIG_MFD_IPAQ_MICRO) += ipaq-micro.o
obj-$(CONFIG_MFD_MENF21BMC) += menf21bmc.o
obj-$(CONFIG_MFD_HI6421_PMIC) += hi6421-pmic-core.o
+obj-$(CONFIG_MFD_HI655X_PMIC) += hi655x-pmic.o
obj-$(CONFIG_MFD_DLN2) += dln2.o
obj-$(CONFIG_MFD_RT5033) += rt5033.o
obj-$(CONFIG_MFD_SKY81452) += sky81452.o
diff --git a/drivers/mfd/hi655x-pmic.c b/drivers/mfd/hi655x-pmic.c
new file mode 100644
index 0000000..2ee455e
--- /dev/null
+++ b/drivers/mfd/hi655x-pmic.c
@@ -0,0 +1,170 @@
+/*
+ * Device driver for PMIC DRIVER in HI655X IC
+ *
+ * Copyright (c) 2015 Hisilicon Co. Ltd
+ *
+ * Chen Feng <[email protected]>
+ * Fei Wang <[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/io.h>
+#include <linux/irq.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/mfd/hi655x-pmic.h>
+#include <linux/regmap.h>
+
+
+static const struct of_device_id of_hi655x_pmic_child_match_tbl[] = {
+ { .compatible = "hisilicon,hi655x-regulator-pmic", },
+ {},
+};
+
+static const struct of_device_id of_hi655x_pmic_match_tbl[] = {
+ { .compatible = "hisilicon,hi655x-pmic", },
+ {},
+};
+
+static const struct regmap_irq hi655x_irqs[] = {
+ { .reg_offset = 0, .mask = OTMP_D1R_INT },
+ { .reg_offset = 0, .mask = VSYS_2P5_R_INT },
+ { .reg_offset = 0, .mask = VSYS_UV_D3R_INT },
+ { .reg_offset = 0, .mask = VSYS_6P0_D200UR_INT },
+ { .reg_offset = 0, .mask = PWRON_D4SR_INT },
+ { .reg_offset = 0, .mask = PWRON_D20F_INT },
+ { .reg_offset = 0, .mask = PWRON_D20R_INT },
+ { .reg_offset = 0, .mask = RESERVE_INT },
+};
+
+static const struct regmap_irq_chip hi655x_irq_chip = {
+ .name = "hi655x-pmic",
+ .irqs = hi655x_irqs,
+ .num_regs = 1,
+ .num_irqs = ARRAY_SIZE(hi655x_irqs),
+ .status_base = HI655X_IRQ_STAT_BASE,
+ .mask_base = HI655X_IRQ_MASK_BASE,
+};
+
+static unsigned int hi655x_pmic_get_version(struct hi655x_pmic *pmic)
+{
+ u32 val;
+
+ regmap_read(pmic->regmap,
+ HI655X_BUS_ADDR(HI655X_VER_REG), &val);
+
+ return val;
+}
+
+static struct regmap_config hi655x_regmap_config = {
+ .reg_bits = 32,
+ .reg_stride = HI655X_STRIDE,
+ .val_bits = 8,
+ .max_register = HI655X_BUS_ADDR(0xFFF),
+};
+
+static void hi655x_local_irq_clear(struct regmap *map)
+{
+ unsigned int i;
+
+ regmap_write(map, HI655X_ANA_IRQM_BASE, HI655X_IRQ_CLR);
+ for (i = 0; i < HI655X_IRQ_ARRAY; i++) {
+ regmap_write(map, HI655X_IRQ_STAT_BASE + i * HI655X_STRIDE,
+ HI655X_IRQ_CLR);
+ }
+}
+
+static int hi655x_pmic_probe(struct platform_device *pdev)
+{
+ int ret;
+ struct hi655x_pmic *pmic;
+ struct device_node *gpio_np;
+
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ void __iomem *base;
+
+ pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
+ pmic->dev = dev;
+
+ pmic->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!pmic->res) {
+ dev_err(dev, "platform_get_resource err\n");
+ return -ENOENT;
+ }
+ base = devm_ioremap_resource(&pdev->dev, pmic->res);
+ if (!base) {
+ dev_err(dev, "cannot map register memory\n");
+ return -ENOMEM;
+ }
+ pmic->regmap = devm_regmap_init_mmio_clk(dev, NULL, base,
+ &hi655x_regmap_config);
+
+ pmic->ver = hi655x_pmic_get_version(pmic);
+ if ((pmic->ver < PMU_VER_START) || (pmic->ver > PMU_VER_END)) {
+ dev_warn(dev, "it is wrong pmu version\n");
+ return -EINVAL;
+ }
+
+ hi655x_local_irq_clear(pmic->regmap);
+
+ gpio_np = of_parse_phandle(np, "pmic-gpios", 0);
+ if (!gpio_np) {
+ dev_err(dev, "can't parse property\n");
+ ret = -EPROBE_DEFER;
+ return ret;
+ }
+
+ ret = gpio_request_one(pmic->gpio, GPIOF_IN, "hi655x_pmic_irq");
+ if (ret < 0) {
+ dev_err(dev, "failed to request gpio %d ret = %d\n",
+ pmic->gpio, ret);
+ return ret;
+ }
+ pmic->irq = gpio_to_irq(pmic->gpio);
+
+ ret = regmap_add_irq_chip(pmic->regmap, pmic->irq,
+ IRQF_TRIGGER_LOW | IRQF_NO_SUSPEND, 0,
+ &hi655x_irq_chip, &pmic->irq_data);
+ if (ret) {
+ gpio_free(pmic->gpio);
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, pmic);
+
+ /**
+ * populate sub nodes
+ */
+ ret = of_platform_populate(np, of_hi655x_pmic_child_match_tbl,
+ NULL, dev);
+ if (ret) {
+ gpio_free(pmic->gpio);
+ regmap_del_irq_chip(pmic->irq, pmic->irq_data);
+ return ret;
+ }
+
+ return 0;
+}
+
+static struct platform_driver hi655x_pmic_driver = {
+ .driver = {
+ .name = "hi655x-pmic",
+ .owner = THIS_MODULE,
+ .of_match_table = of_hi655x_pmic_match_tbl,
+ },
+ .probe = hi655x_pmic_probe,
+};
+module_platform_driver(hi655x_pmic_driver);
+
+MODULE_AUTHOR("Chen Feng <[email protected]>");
+MODULE_DESCRIPTION("Hisilicon hi655x pmic driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/hi655x-pmic.h b/include/linux/mfd/hi655x-pmic.h
new file mode 100644
index 0000000..9b677f92
--- /dev/null
+++ b/include/linux/mfd/hi655x-pmic.h
@@ -0,0 +1,56 @@
+/*
+ * Header file for device driver Hi655X PMIC
+ *
+ * Copyright (C) 2015 Hisilicon Co. Ltd.
+ *
+ * Chen Feng <[email protected]>
+ * Fei Wang <[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.
+ */
+
+#ifndef __HI655X_PMIC_H
+#define __HI655X_PMIC_H
+
+/* Hi655x registers are mapped to memory bus in 4 bytes stride */
+#define HI655X_STRIDE (4)
+#define HI655X_BUS_ADDR(x) ((x) << 2)
+
+#define HI655X_BITS (8)
+
+#define HI655X_NR_IRQ (32)
+
+#define HI655X_IRQ_STAT_BASE (0x003 << 2)
+#define HI655X_IRQ_MASK_BASE (0x007 << 2)
+#define HI655X_ANA_IRQM_BASE (0x1b5 << 2)
+#define HI655X_IRQ_ARRAY (4)
+#define HI655X_IRQ_MASK (0xFF)
+#define HI655X_IRQ_CLR (0xFF)
+#define HI655X_VER_REG (0x00)
+
+#define PMU_VER_START (0x10)
+#define PMU_VER_END (0x38)
+
+#define RESERVE_INT (BIT(7))
+#define PWRON_D20R_INT (BIT(6))
+#define PWRON_D20F_INT (BIT(5))
+#define PWRON_D4SR_INT (BIT(4))
+#define VSYS_6P0_D200UR_INT (BIT(3))
+#define VSYS_UV_D3R_INT (BIT(2))
+#define VSYS_2P5_R_INT (BIT(1))
+#define OTMP_D1R_INT (BIT(0))
+
+struct hi655x_pmic {
+ struct resource *res;
+ struct device *dev;
+ struct regmap *regmap;
+ struct clk *clk;
+ int irq;
+ int gpio;
+ unsigned int ver;
+ struct regmap_irq_chip_data *irq_data;
+};
+
+#endif
--
1.9.1
Add driver support for HiSilicon Hi655x voltage regulators.
Signed-off-by: Chen Feng <[email protected]>
Signed-off-by: Fei Wang <[email protected]>
Tested-by: Xinwei Kong <[email protected]>
---
drivers/regulator/Kconfig | 8 +
drivers/regulator/Makefile | 1 +
drivers/regulator/hi655x-regulator.c | 246 +++++++++++++++++++++++++++++
include/linux/regulator/hi655x-regulator.h | 41 +++++
4 files changed, 296 insertions(+)
create mode 100644 drivers/regulator/hi655x-regulator.c
create mode 100644 include/linux/regulator/hi655x-regulator.h
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 8df0b0e..c6ecfd7 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -261,6 +261,14 @@ config REGULATOR_HI6421
21 general purpose LDOs, 3 dedicated LDOs, and 5 BUCKs. All
of them come with support to either ECO (idle) or sleep mode.
+config REGULATOR_HI655X
+ tristate "Hisilicon HI655X PMIC regulators support"
+ depends on ARCH_HISI
+ depends on MFD_HI655X_PMIC && OF
+ help
+ This driver provides support for the voltage regulators of the
+ Hisilicon Hi655x PMIC device.
+
config REGULATOR_ISL9305
tristate "Intersil ISL9305 regulator"
depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 0f81749..8e4db96 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_REGULATOR_DB8500_PRCMU) += db8500-prcmu.o
obj-$(CONFIG_REGULATOR_FAN53555) += fan53555.o
obj-$(CONFIG_REGULATOR_GPIO) += gpio-regulator.o
obj-$(CONFIG_REGULATOR_HI6421) += hi6421-regulator.o
+obj-$(CONFIG_REGULATOR_HI655X) += hi655x-regulator.o
obj-$(CONFIG_REGULATOR_ISL6271A) += isl6271a-regulator.o
obj-$(CONFIG_REGULATOR_ISL9305) += isl9305.o
obj-$(CONFIG_REGULATOR_LP3971) += lp3971.o
diff --git a/drivers/regulator/hi655x-regulator.c b/drivers/regulator/hi655x-regulator.c
new file mode 100644
index 0000000..e222ee7
--- /dev/null
+++ b/drivers/regulator/hi655x-regulator.c
@@ -0,0 +1,246 @@
+/*
+ * Device driver for regulators in hi655x IC
+ *
+ * Copyright (c) 2015 Hisilicon.
+ *
+ * Chen Feng <[email protected]>
+ * Fei Wang <[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/slab.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/delay.h>
+#include <linux/time.h>
+#include <linux/regulator/hi655x-regulator.h>
+#include <linux/mfd/hi655x-pmic.h>
+#include <linux/regmap.h>
+#include <linux/bitops.h>
+#include <linux/string.h>
+
+static int hi655x_is_enabled(struct regulator_dev *rdev)
+{
+ unsigned int value = 0;
+
+ struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
+ struct hi655x_regulator_ctrl_regs *ctrl_regs = ®ulator->ctrl_regs;
+
+ regmap_read(rdev->regmap, ctrl_regs->status_reg, &value);
+ return (value & BIT(regulator->ctrl_mask));
+}
+
+static int hi655x_disable(struct regulator_dev *rdev)
+{
+ int ret = 0;
+
+ struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
+ struct hi655x_regulator_ctrl_regs *ctrl_regs = ®ulator->ctrl_regs;
+
+ ret = regmap_write(rdev->regmap, ctrl_regs->disable_reg,
+ BIT(regulator->ctrl_mask));
+ return ret;
+}
+
+static int hi655x_get_voltage(struct regulator_dev *rdev)
+{
+ unsigned int value = 0;
+
+ struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
+
+ regmap_read(rdev->regmap, regulator->vset_reg, &value);
+ value &= regulator->vset_mask;
+
+ return regulator->rdesc.volt_table[value];
+}
+
+static int hi655x_set_voltage(struct regulator_dev *rdev,
+ int min_uV, int max_uV, unsigned *selector)
+{
+ int i = 0;
+ int vol = 0;
+
+ struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
+
+ /**
+ * search the matched vol and get its index
+ */
+ for (i = 0; i < regulator->rdesc.n_voltages; i++) {
+ vol = regulator->rdesc.volt_table[i];
+ if ((vol >= min_uV) && (vol <= max_uV))
+ break;
+ }
+
+ if (i == regulator->rdesc.n_voltages)
+ return -EINVAL;
+
+ regmap_update_bits(rdev->regmap, regulator->vset_reg,
+ regulator->vset_mask, i);
+
+ *selector = i;
+
+ return 0;
+}
+
+static struct regulator_ops hi655x_regulator_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = hi655x_disable,
+ .is_enabled = hi655x_is_enabled,
+ .list_voltage = regulator_list_voltage_table,
+ .get_voltage = hi655x_get_voltage,
+ .set_voltage = hi655x_set_voltage,
+};
+
+static const struct of_device_id of_hi655x_regulator_match_tbl[] = {
+ {
+ .compatible = "hisilicon,hi655x-regulator-pmic",
+ },
+};
+MODULE_DEVICE_TABLE(of, of_hi655x_regulator_match_tbl);
+
+/**
+ * get the hi655x specific data from dt node.
+ */
+static int of_get_hi655x_ctr(struct hi655x_regulator *regulator,
+ struct device *dev, struct device_node *np)
+{
+ int ret;
+ u32 *vset_table;
+ u32 vset_reg;
+ u32 ctrl_mask;
+ u32 vset_mask;
+ u32 vol_numb;
+ struct hi655x_regulator_ctrl_regs ctrl_reg;
+ struct hi655x_regulator_ctrl_regs *t_ctrl;
+
+ ret = of_property_read_u32_array(np, "regulator-ctrl-regs",
+ (u32 *)&ctrl_reg, 0x3);
+ if (!ret) {
+ t_ctrl = ®ulator->ctrl_regs;
+ t_ctrl->enable_reg = HI655X_BUS_ADDR(ctrl_reg.enable_reg);
+ t_ctrl->disable_reg = HI655X_BUS_ADDR(ctrl_reg.disable_reg);
+ t_ctrl->status_reg = HI655X_BUS_ADDR(ctrl_reg.status_reg);
+ } else
+ goto error;
+
+ ret = of_property_read_u32(np, "regulator-ctrl-mask", &ctrl_mask);
+ if (!ret)
+ regulator->ctrl_mask = ctrl_mask;
+ else
+ goto error;
+
+ regulator->rdesc.enable_reg = t_ctrl->enable_reg;
+ regulator->rdesc.enable_val = ctrl_mask;
+ regulator->rdesc.enable_mask = ctrl_mask;
+
+ ret = of_property_read_u32(np, "regulator-vset-regs", &vset_reg);
+ if (!ret)
+ regulator->vset_reg = HI655X_BUS_ADDR(vset_reg);
+ else
+ goto error;
+
+ ret = of_property_read_u32(np, "regulator-vset-mask", &vset_mask);
+ if (!ret)
+ regulator->vset_mask = vset_mask;
+ else
+ goto error;
+
+ ret = of_property_read_u32(np, "regulator-n-vol", &vol_numb);
+ if (!ret)
+ regulator->rdesc.n_voltages = vol_numb;
+ else
+ goto error;
+
+ vset_table = devm_kzalloc(dev, vol_numb * sizeof(int), GFP_KERNEL);
+ if (!vset_table)
+ return -ENOMEM;
+
+ ret = of_property_read_u32_array(np, "regulator-vset-table",
+ vset_table,
+ vol_numb);
+
+ if (!ret)
+ regulator->rdesc.volt_table = vset_table;
+
+ return 0;
+error:
+ dev_err(dev, "get from dts node error!\n");
+ return -ENODEV;
+}
+
+static int hi655x_regulator_probe(struct platform_device *pdev)
+{
+ int ret;
+ struct hi655x_regulator *regulator;
+ struct hi655x_pmic *pmic;
+ struct regulator_init_data *init_data;
+ struct regulator_config config = { };
+ struct device_node *np = pdev->dev.of_node;
+ struct device *dev = &pdev->dev;
+
+ pmic = dev_get_drvdata(pdev->dev.parent);
+ if (!pmic) {
+ dev_err(dev, "no pmic in the regulator parent node\n");
+ return -ENODEV;
+ }
+
+ regulator = devm_kzalloc(dev, sizeof(*regulator), GFP_KERNEL);
+ if (!regulator)
+ return -ENOMEM;
+
+ regulator->rdesc.type = REGULATOR_VOLTAGE;
+ regulator->rdesc.owner = THIS_MODULE;
+ regulator->rdesc.ops = &hi655x_regulator_ops;
+ init_data = of_get_regulator_init_data(dev, dev->of_node,
+ ®ulator->rdesc);
+ if (!init_data)
+ return -EINVAL;
+
+ config.dev = &pdev->dev;
+ config.init_data = init_data;
+ config.driver_data = regulator;
+ config.regmap = pmic->regmap;
+ config.of_node = pdev->dev.of_node;
+ regulator->rdesc.name = init_data->constraints.name;
+
+ ret = of_get_hi655x_ctr(regulator, dev, np);
+ if (ret) {
+ dev_err(dev, "get param from dts error!\n");
+ return ret;
+ }
+
+ regulator->regdev = devm_regulator_register(dev,
+ ®ulator->rdesc,
+ &config);
+ if (IS_ERR(regulator->regdev))
+ return PTR_ERR(regulator->regdev);
+
+ platform_set_drvdata(pdev, regulator);
+
+ return 0;
+}
+
+static struct platform_driver hi655x_regulator_driver = {
+ .driver = {
+ .name = "hi655x_regulator",
+ .of_match_table = of_hi655x_regulator_match_tbl,
+ },
+ .probe = hi655x_regulator_probe,
+};
+module_platform_driver(hi655x_regulator_driver);
+
+MODULE_AUTHOR("Chen Feng <[email protected]>");
+MODULE_DESCRIPTION("Hisi hi655x PMIC driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/regulator/hi655x-regulator.h b/include/linux/regulator/hi655x-regulator.h
new file mode 100644
index 0000000..8c76fc5
--- /dev/null
+++ b/include/linux/regulator/hi655x-regulator.h
@@ -0,0 +1,41 @@
+/*
+ * Head file for hi655x regulator
+ *
+ * Copyright (c) 2015 Hisilicon.
+ *
+ * Chen Feng <[email protected]>
+ * Fei Wang <[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.
+ */
+#ifndef __HISI_HI655X_REGULATOR_H__
+#define __HISI_HI655X_REGULATOR_H__
+
+struct hi655x_regulator_ctrl_regs {
+ u32 enable_reg;
+ u32 disable_reg;
+ u32 status_reg;
+};
+
+struct hi655x_regulator_ctrl_data {
+ int shift;
+ unsigned int val;
+};
+
+struct hi655x_regulator_vset_data {
+ int shift;
+ unsigned int mask;
+};
+
+struct hi655x_regulator {
+ struct hi655x_regulator_ctrl_regs ctrl_regs;
+ u32 vset_reg;
+ u32 ctrl_mask;
+ u32 vset_mask;
+ struct regulator_desc rdesc;
+ struct regulator_dev *regdev;
+};
+
+#endif
--
1.9.1
Add dts node for hi665x MFD and regulator driver
Signed-off-by: Chen Feng <[email protected]>
Signed-off-by: Fei Wang <[email protected]>
Tested-by: Xinwei Kong <[email protected]>
---
arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 178 ++++++++++++++++++++++++++++++
1 file changed, 178 insertions(+)
diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
index 82d2488..5f98a72 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
@@ -208,5 +208,183 @@
clock-names = "uartclk", "apb_pclk";
status = "disabled";
};
+ pmic: pmic@F8000000 {
+ compatible = "hisilicon,hi655x-pmic";
+ reg = <0x0 0xf8000000 0x0 0x1000>;
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ pmic-gpios = <&gpio_pmu_irq_n>;
+ status = "okay";
+ ldo2: regulator@a21 {
+ compatible = "hisilicon,hi655x-regulator-pmic";
+ regulator-name = "ldo2";
+ regulator-min-microvolt = <2500000>;
+ regulator-max-microvolt = <3200000>;
+ regulator-valid-modes-mask = <0x02>;
+ regulator-enable-ramp-delay = <120>;
+ regulator-ctrl-regs = <0x029 0x02a 0x02b>;
+ regulator-ctrl-mask = <0x1>;
+ regulator-vset-regs = <0x072>;
+ regulator-vset-mask = <0x7>;
+ regulator-n-vol = <8>;
+ regulator-vset-table = <2500000>,<2600000>,
+ <2700000>,<2800000>,
+ <2900000>,<3000000>,
+ <3100000>,<3200000>;
+ };
+ ldo7: regulator@a26 {
+ compatible = "hisilicon,hi655x-regulator-pmic";
+ regulator-name = "ldo7";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-valid-modes-mask = <0x0a>;
+ regulator-enable-ramp-delay = <120>;
+ regulator-ctrl-regs = <0x029 0x02a 0x02b>;
+ regulator-ctrl-mask = <0x6>;
+ regulator-vset-regs = <0x078>;
+ regulator-vset-mask = <0x7>;
+ regulator-n-vol = <8>;
+ regulator-vset-table = <1800000>,<1850000>,
+ <2850000>,<2900000>,
+ <3000000>,<3100000>,
+ <3200000>,<3300000>;
+ };
+ ldo10: regulator@a29 {
+ compatible = "hisilicon,hi655x-regulator-pmic";
+ regulator-name = "ldo10";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <3000000>;
+ regulator-valid-modes-mask = <0x0a>;
+ regulator-enable-ramp-delay = <360>;
+ regulator-ctrl-regs = <0x02c 0x02d 0x02e>;
+ regulator-ctrl-mask = <0x1>;
+ regulator-vset-regs = <0x07b>;
+ regulator-vset-mask = <0x7>;
+ regulator-n-vol = <8>;
+ regulator-vset-table = <1800000>,<1850000>,
+ <1900000>,<2750000>,
+ <2800000>,<2850000>,
+ <2900000>,<3000000>;
+ };
+ ldo13: regulator@a32 {
+ compatible = "hisilicon,hi655x-regulator-pmic";
+ regulator-name = "ldo13";
+ regulator-min-microvolt = <1600000>;
+ regulator-max-microvolt = <1950000>;
+ regulator-enable-ramp-delay = <120>;
+ regulator-ctrl-regs = <0x02c 0x02d 0x02e>;
+ regulator-ctrl-mask = <0x4>;
+ regulator-vset-regs = <0x07e>;
+ regulator-vset-mask = <0x7>;
+ regulator-n-vol = <8>;
+ regulator-vset-table = <1600000>,<1650000>,
+ <1700000>,<1750000>,
+ <1800000>,<1850000>,
+ <1900000>,<1950000>;
+ };
+ ldo14: regulator@a33 {
+ compatible = "hisilicon,hi655x-regulator-pmic";
+ regulator-name = "ldo14";
+ regulator-min-microvolt = <2500000>;
+ regulator-max-microvolt = <3200000>;
+ regulator-enable-ramp-delay = <120>;
+ regulator-ctrl-regs = <0x02c 0x02d 0x02e>;
+ regulator-ctrl-mask = <0x5>;
+ regulator-vset-regs = <0x07f>;
+ regulator-vset-mask = <0x7>;
+ regulator-n-vol = <8>;
+ regulator-vset-table = <2500000>,<2600000>,
+ <2700000>,<2800000>,
+ <2900000>,<3000000>,
+ <3100000>,<3200000>;
+ };
+ ldo15: regulator@a34 {
+ compatible = "hisilicon,hi655x-regulator-pmic";
+ regulator-name = "ldo15";
+ regulator-min-microvolt = <1600000>;
+ regulator-max-microvolt = <1950000>;
+ regulator-boot-on;
+ regulator-always-on;
+ regulator-enable-ramp-delay = <120>;
+ regulator-ctrl-regs = <0x02c 0x02d 0x02e>;
+ regulator-ctrl-mask = <0x6>;
+ regulator-vset-regs = <0x080>;
+ regulator-vset-mask = <0x7>;
+ regulator-n-vol = <8>;
+ regulator-vset-table = <1600000>,<1650000>,
+ <1700000>,<1750000>,
+ <1800000>,<1850000>,
+ <1900000>,<1950000>;
+ };
+ ldo17: regulator@a36 {
+ compatible = "hisilicon,hi655x-regulator-pmic";
+ regulator-name = "ldo17";
+ regulator-min-microvolt = <2500000>;
+ regulator-max-microvolt = <3200000>;
+ regulator-enable-ramp-delay = <120>;
+ regulator-ctrl-regs = <0x02f 0x030 0x031>;
+ regulator-ctrl-mask = <0x0>;
+ regulator-vset-regs = <0x082>;
+ regulator-vset-mask = <0x7>;
+ regulator-n-vol = <8>;
+ regulator-vset-table = <2500000>,<2600000>,
+ <2700000>,<2800000>,
+ <2900000>,<3000000>,
+ <3100000>,<3200000>;
+ };
+ ldo19: regulator@a38 {
+ compatible = "hisilicon,hi655x-regulator-pmic";
+ regulator-name = "ldo19";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <3000000>;
+ regulator-enable-ramp-delay = <360>;
+ regulator-ctrl-regs = <0x02f 0x030 0x031>;
+ regulator-ctrl-mask = <0x2>;
+ regulator-vset-regs = <0x084>;
+ regulator-vset-mask = <0x7>;
+ regulator-n-vol = <8>;
+ regulator-vset-table = <1800000>,<1850000>,
+ <1900000>,<2750000>,
+ <2800000>,<2850000>,
+ <2900000>,<3000000>;
+ };
+ ldo21: regulator@a40 {
+ compatible = "hisilicon,hi655x-regulator-pmic";
+ regulator-name = "ldo21";
+ regulator-min-microvolt = <1650000>;
+ regulator-max-microvolt = <2000000>;
+ regulator-always-on;
+ regulator-valid-modes-mask = <0x02>;
+ regulator-enable-ramp-delay = <120>;
+ regulator-ctrl-regs = <0x02f 0x030 0x031>;
+ regulator-ctrl-mask = <0x4>;
+ regulator-vset-regs = <0x086>;
+ regulator-vset-mask = <0x7>;
+ regulator-n-vol = <8>;
+ regulator-vset-table = <1650000>,<1700000>,
+ <1750000>,<1800000>,
+ <1850000>,<1900000>,
+ <1950000>,<2000000>;
+ };
+ ldo22: regulator@a41 {
+ compatible = "hisilicon,hi655x-regulator-pmic";
+ regulator-name = "ldo22";
+ regulator-min-microvolt = <900000>;
+ regulator-max-microvolt = <1200000>;
+ regulator-boot-on;
+ regulator-always-on;
+ regulator-valid-modes-mask = <0x02>;
+ regulator-enable-ramp-delay = <120>;
+ regulator-ctrl-regs = <0x02f 0x030 0x031>;
+ regulator-ctrl-mask = <0x5>;
+ regulator-vset-regs = <0x087>;
+ regulator-vset-mask = <0x7>;
+ regulator-n-vol = <8>;
+ regulator-vset-table = <900000>,<1000000>,
+ <1050000>,<1100000>,
+ <1150000>,<1175000>,
+ <1185000>,<1200000>;
+ };
+ };
};
};
--
1.9.1
drivers/mfd/hi655x-pmic.c:161:3-8: No need to set .owner here. The core will do it.
Remove .owner field if calls are used which set it automatically
Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci
CC: Chen Feng <[email protected]>
Signed-off-by: Fengguang Wu <[email protected]>
---
hi655x-pmic.c | 1 -
1 file changed, 1 deletion(-)
--- a/drivers/mfd/hi655x-pmic.c
+++ b/drivers/mfd/hi655x-pmic.c
@@ -158,7 +158,6 @@ static int hi655x_pmic_probe(struct plat
static struct platform_driver hi655x_pmic_driver = {
.driver = {
.name = "hi655x-pmic",
- .owner = THIS_MODULE,
.of_match_table = of_hi655x_pmic_match_tbl,
},
.probe = hi655x_pmic_probe,
Hi Chen,
[auto build test WARNING on ljones-mfd/for-mfd-next]
[also build test WARNING on v4.4-rc5 next-20151214]
url: https://github.com/0day-ci/linux/commits/Chen-Feng/Add-Support-for-Hi6220-PMIC-Hi6553-MFD-Core/20151215-210113
base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
coccinelle warnings: (new ones prefixed by >>)
>> drivers/mfd/hi655x-pmic.c:161:3-8: No need to set .owner here. The core will do it.
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Tue, Dec 15, 2015 at 08:54:15PM +0800, Chen Feng wrote:
> +config REGULATOR_HI655X
> + tristate "Hisilicon HI655X PMIC regulators support"
> + depends on ARCH_HISI
> + depends on MFD_HI655X_PMIC && OF
> + help
> + This driver provides support for the voltage regulators of the
> + Hisilicon Hi655x PMIC device.
> +
On the previous version of this patch I said:
| For both of these we should have an || COMPILE_TEST and there's no need
| for either to be bool I can see, they should be tristate.
I see you have made this a tristate which is good but you've not enabled
COMPILE_TEST or indicated why - there may be a very good reason for
doing this but nobody has said what it is. Please don't ignore review
comments, people are generally making them for a reason and are likely
to have the same concerns if issues remain unaddressed. Having to
repeat the same comments can get repetitive and make people question the
value of time spent reviewing. If you disagree with the review comments
that's fine but you need to reply and discuss your concerns so that the
reviewer can understand your decisions.
> +static int hi655x_is_enabled(struct regulator_dev *rdev)
> +{
> + unsigned int value = 0;
> +
> + struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
> + struct hi655x_regulator_ctrl_regs *ctrl_regs = ®ulator->ctrl_regs;
The style here is not what we normally do - normally the struct lookups
would be first, then any other variables and there wouldn't be any blank
lines in the variable declarations. The same applies to quite a few
functions.
> +static int hi655x_set_voltage(struct regulator_dev *rdev,
> + int min_uV, int max_uV, unsigned *selector)
> +{
As I commented on the previous version of this driver:
| Use the standard helpers, including one of the map_voltage()s and
| set_voltage_sel_regmap(), don't open code them.
You need to at least split the map and set_voltage_sel operations.
I've stopped reviewing here.
Mark,
Thanks very much for your review.
On 2015/12/17 3:16, Mark Brown wrote:
> On Tue, Dec 15, 2015 at 08:54:15PM +0800, Chen Feng wrote:
>
>> +config REGULATOR_HI655X
>> + tristate "Hisilicon HI655X PMIC regulators support"
>> + depends on ARCH_HISI
>> + depends on MFD_HI655X_PMIC && OF
>> + help
>> + This driver provides support for the voltage regulators of the
>> + Hisilicon Hi655x PMIC device.
>> +
>
> On the previous version of this patch I said:
>
> | For both of these we should have an || COMPILE_TEST and there's no need
> | for either to be bool I can see, they should be tristate.
>
> I see you have made this a tristate which is good but you've not enabled
> COMPILE_TEST or indicated why - there may be a very good reason for
> doing this but nobody has said what it is. Please don't ignore review
> comments, people are generally making them for a reason and are likely
> to have the same concerns if issues remain unaddressed. Having to
> repeat the same comments can get repetitive and make people question the
> value of time spent reviewing. If you disagree with the review comments
> that's fine but you need to reply and discuss your concerns so that the
> reviewer can understand your decisions.
>
Sorry for this,I am busy with other issue these days.
Really appreciate your comments.
I will add the COMPILE_TEST next version.
>> +static int hi655x_is_enabled(struct regulator_dev *rdev)
>> +{
>> + unsigned int value = 0;
>> +
>> + struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
>> + struct hi655x_regulator_ctrl_regs *ctrl_regs = ®ulator->ctrl_regs;
>
> The style here is not what we normally do - normally the struct lookups
> would be first, then any other variables and there wouldn't be any blank
> lines in the variable declarations. The same applies to quite a few
> functions.
>
ok, I will fix the style problem.
>> +static int hi655x_set_voltage(struct regulator_dev *rdev,
>> + int min_uV, int max_uV, unsigned *selector)
>> +{
>
> As I commented on the previous version of this driver:
>
> | Use the standard helpers, including one of the map_voltage()s and
> | set_voltage_sel_regmap(), don't open code them.
>
I change the regulator_enable method to regulator_enable_regmap this version.
But,since the hi655x PMIC use status register to ensure the regulator is okay or not.
Three different register offset in these PMIC series.
enable register.
disable register.
status register.
I can't use regulator_is_enabled_regmap regulator_disable_regmap in helpers.c
int regulator_disable_regmap(struct regulator_dev *rdev)
{
...
return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
rdev->desc->enable_mask, val);
...
}
Only enable_reg in the desc struct.
I will change the set voltage to regulator_set_voltage_sel_regmap next version.
Thanks again.
> You need to at least split the map and set_voltage_sel operations.
>
> I've stopped reviewing here.
>
Mark,
+- regulator-vset-regs: Voltage set register offset.
+- regulator-vset-mask: voltage set control mask.
+- regulator-n-vol: The num of support voltages.
+- regulator-vset-table: The table of support voltages.
> Why is this in the binding? This is a binding for a specific device,
> there is no point in putting all these data tables in the DT - it just
> bloats the DT and makes it harder for us to enhance our support for this
> device in the future.
You mentioned in previous version,I I have some questions for it.
This regulator-vset-regs etc are vendor specific describe. The hi655x PMIC
is a series of chips. They all have this value, but the offset may be different.
And we can generate the dts file from excel which is defined by SOC.
I think the dts is designed to distinguish different platform. If we hard code this
in files, it may be also different to use as common in next chip version.
I will appreciate it if you can give me some suggestions.
On 2015/12/15 20:54, Chen Feng wrote:
> Add dts node for hi665x MFD and regulator driver
>
> Signed-off-by: Chen Feng <[email protected]>
> Signed-off-by: Fei Wang <[email protected]>
> Tested-by: Xinwei Kong <[email protected]>
> ---
> arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 178 ++++++++++++++++++++++++++++++
> 1 file changed, 178 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> index 82d2488..5f98a72 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> @@ -208,5 +208,183 @@
> clock-names = "uartclk", "apb_pclk";
> status = "disabled";
> };
> + pmic: pmic@F8000000 {
> + compatible = "hisilicon,hi655x-pmic";
> + reg = <0x0 0xf8000000 0x0 0x1000>;
> + #interrupt-cells = <2>;
> + interrupt-controller;
> + pmic-gpios = <&gpio_pmu_irq_n>;
> + status = "okay";
> + ldo2: regulator@a21 {
> + compatible = "hisilicon,hi655x-regulator-pmic";
> + regulator-name = "ldo2";
> + regulator-min-microvolt = <2500000>;
> + regulator-max-microvolt = <3200000>;
> + regulator-valid-modes-mask = <0x02>;
> + regulator-enable-ramp-delay = <120>;
> + regulator-ctrl-regs = <0x029 0x02a 0x02b>;
> + regulator-ctrl-mask = <0x1>;
> + regulator-vset-regs = <0x072>;
> + regulator-vset-mask = <0x7>;
> + regulator-n-vol = <8>;
> + regulator-vset-table = <2500000>,<2600000>,
> + <2700000>,<2800000>,
> + <2900000>,<3000000>,
> + <3100000>,<3200000>;
> + };
> + ldo7: regulator@a26 {
> + compatible = "hisilicon,hi655x-regulator-pmic";
> + regulator-name = "ldo7";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-valid-modes-mask = <0x0a>;
> + regulator-enable-ramp-delay = <120>;
> + regulator-ctrl-regs = <0x029 0x02a 0x02b>;
> + regulator-ctrl-mask = <0x6>;
> + regulator-vset-regs = <0x078>;
> + regulator-vset-mask = <0x7>;
> + regulator-n-vol = <8>;
> + regulator-vset-table = <1800000>,<1850000>,
> + <2850000>,<2900000>,
> + <3000000>,<3100000>,
> + <3200000>,<3300000>;
> + };
> + ldo10: regulator@a29 {
> + compatible = "hisilicon,hi655x-regulator-pmic";
> + regulator-name = "ldo10";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <3000000>;
> + regulator-valid-modes-mask = <0x0a>;
> + regulator-enable-ramp-delay = <360>;
> + regulator-ctrl-regs = <0x02c 0x02d 0x02e>;
> + regulator-ctrl-mask = <0x1>;
> + regulator-vset-regs = <0x07b>;
> + regulator-vset-mask = <0x7>;
> + regulator-n-vol = <8>;
> + regulator-vset-table = <1800000>,<1850000>,
> + <1900000>,<2750000>,
> + <2800000>,<2850000>,
> + <2900000>,<3000000>;
> + };
> + ldo13: regulator@a32 {
> + compatible = "hisilicon,hi655x-regulator-pmic";
> + regulator-name = "ldo13";
> + regulator-min-microvolt = <1600000>;
> + regulator-max-microvolt = <1950000>;
> + regulator-enable-ramp-delay = <120>;
> + regulator-ctrl-regs = <0x02c 0x02d 0x02e>;
> + regulator-ctrl-mask = <0x4>;
> + regulator-vset-regs = <0x07e>;
> + regulator-vset-mask = <0x7>;
> + regulator-n-vol = <8>;
> + regulator-vset-table = <1600000>,<1650000>,
> + <1700000>,<1750000>,
> + <1800000>,<1850000>,
> + <1900000>,<1950000>;
> + };
> + ldo14: regulator@a33 {
> + compatible = "hisilicon,hi655x-regulator-pmic";
> + regulator-name = "ldo14";
> + regulator-min-microvolt = <2500000>;
> + regulator-max-microvolt = <3200000>;
> + regulator-enable-ramp-delay = <120>;
> + regulator-ctrl-regs = <0x02c 0x02d 0x02e>;
> + regulator-ctrl-mask = <0x5>;
> + regulator-vset-regs = <0x07f>;
> + regulator-vset-mask = <0x7>;
> + regulator-n-vol = <8>;
> + regulator-vset-table = <2500000>,<2600000>,
> + <2700000>,<2800000>,
> + <2900000>,<3000000>,
> + <3100000>,<3200000>;
> + };
> + ldo15: regulator@a34 {
> + compatible = "hisilicon,hi655x-regulator-pmic";
> + regulator-name = "ldo15";
> + regulator-min-microvolt = <1600000>;
> + regulator-max-microvolt = <1950000>;
> + regulator-boot-on;
> + regulator-always-on;
> + regulator-enable-ramp-delay = <120>;
> + regulator-ctrl-regs = <0x02c 0x02d 0x02e>;
> + regulator-ctrl-mask = <0x6>;
> + regulator-vset-regs = <0x080>;
> + regulator-vset-mask = <0x7>;
> + regulator-n-vol = <8>;
> + regulator-vset-table = <1600000>,<1650000>,
> + <1700000>,<1750000>,
> + <1800000>,<1850000>,
> + <1900000>,<1950000>;
> + };
> + ldo17: regulator@a36 {
> + compatible = "hisilicon,hi655x-regulator-pmic";
> + regulator-name = "ldo17";
> + regulator-min-microvolt = <2500000>;
> + regulator-max-microvolt = <3200000>;
> + regulator-enable-ramp-delay = <120>;
> + regulator-ctrl-regs = <0x02f 0x030 0x031>;
> + regulator-ctrl-mask = <0x0>;
> + regulator-vset-regs = <0x082>;
> + regulator-vset-mask = <0x7>;
> + regulator-n-vol = <8>;
> + regulator-vset-table = <2500000>,<2600000>,
> + <2700000>,<2800000>,
> + <2900000>,<3000000>,
> + <3100000>,<3200000>;
> + };
> + ldo19: regulator@a38 {
> + compatible = "hisilicon,hi655x-regulator-pmic";
> + regulator-name = "ldo19";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <3000000>;
> + regulator-enable-ramp-delay = <360>;
> + regulator-ctrl-regs = <0x02f 0x030 0x031>;
> + regulator-ctrl-mask = <0x2>;
> + regulator-vset-regs = <0x084>;
> + regulator-vset-mask = <0x7>;
> + regulator-n-vol = <8>;
> + regulator-vset-table = <1800000>,<1850000>,
> + <1900000>,<2750000>,
> + <2800000>,<2850000>,
> + <2900000>,<3000000>;
> + };
> + ldo21: regulator@a40 {
> + compatible = "hisilicon,hi655x-regulator-pmic";
> + regulator-name = "ldo21";
> + regulator-min-microvolt = <1650000>;
> + regulator-max-microvolt = <2000000>;
> + regulator-always-on;
> + regulator-valid-modes-mask = <0x02>;
> + regulator-enable-ramp-delay = <120>;
> + regulator-ctrl-regs = <0x02f 0x030 0x031>;
> + regulator-ctrl-mask = <0x4>;
> + regulator-vset-regs = <0x086>;
> + regulator-vset-mask = <0x7>;
> + regulator-n-vol = <8>;
> + regulator-vset-table = <1650000>,<1700000>,
> + <1750000>,<1800000>,
> + <1850000>,<1900000>,
> + <1950000>,<2000000>;
> + };
> + ldo22: regulator@a41 {
> + compatible = "hisilicon,hi655x-regulator-pmic";
> + regulator-name = "ldo22";
> + regulator-min-microvolt = <900000>;
> + regulator-max-microvolt = <1200000>;
> + regulator-boot-on;
> + regulator-always-on;
> + regulator-valid-modes-mask = <0x02>;
> + regulator-enable-ramp-delay = <120>;
> + regulator-ctrl-regs = <0x02f 0x030 0x031>;
> + regulator-ctrl-mask = <0x5>;
> + regulator-vset-regs = <0x087>;
> + regulator-vset-mask = <0x7>;
> + regulator-n-vol = <8>;
> + regulator-vset-table = <900000>,<1000000>,
> + <1050000>,<1100000>,
> + <1150000>,<1175000>,
> + <1185000>,<1200000>;
> + };
> + };
> };
> };
>
On Thu, Dec 17, 2015 at 11:27:27AM +0800, chenfeng wrote:
> +- regulator-vset-regs: Voltage set register offset.
> +- regulator-vset-mask: voltage set control mask.
> +- regulator-n-vol: The num of support voltages.
> +- regulator-vset-table: The table of support voltages.
> > Why is this in the binding? This is a binding for a specific device,
> > there is no point in putting all these data tables in the DT - it just
> > bloats the DT and makes it harder for us to enhance our support for this
> > device in the future.
> You mentioned in previous version,I I have some questions for it.
> This regulator-vset-regs etc are vendor specific describe. The hi655x PMIC
There's nothing vendor specific about the way this is written...
> is a series of chips. They all have this value, but the offset may be different.
> And we can generate the dts file from excel which is defined by SOC.
> I think the dts is designed to distinguish different platform. If we hard code this
> in files, it may be also different to use as common in next chip version.
If your tooling can generate DT files it can generate C code just as
well and it seems unlikely you're going to be able to build new boards
without being able to do firmware updates here. Especially for the
sorts of systems that use DT the set of scenarios where you're able to
update the DT but not the kernel seems like it will be extremely
limited. I don't really buy the argument that there's any practical
difference in the ability to update the kernel and DT and to the extent
there is one it seems better to keep the ABI we have to support smaller
by having the DT be minimal.
This also allows us to map things more efficiently than we can with just
a table of voltages. For example a good selection of the regulators in
your example DT appear to be linear ranges and so should be mapped as
such so we can do direct calcuations rather than having to iterate
through a table to map voltages into selectors. That gets especially
serious for higher resolution regulators like most DCDCs (and modern
LDOs for that matter).
Mark,
On 2015/12/19 1:58, Mark Brown wrote:
> On Thu, Dec 17, 2015 at 11:27:27AM +0800, chenfeng wrote:
>
>> +- regulator-vset-regs: Voltage set register offset.
>> +- regulator-vset-mask: voltage set control mask.
>> +- regulator-n-vol: The num of support voltages.
>> +- regulator-vset-table: The table of support voltages.
>
>>> Why is this in the binding? This is a binding for a specific device,
>>> there is no point in putting all these data tables in the DT - it just
>>> bloats the DT and makes it harder for us to enhance our support for this
>>> device in the future.
>
>> You mentioned in previous version,I I have some questions for it.
>
>> This regulator-vset-regs etc are vendor specific describe. The hi655x PMIC
>
> There's nothing vendor specific about the way this is written...
>
>> is a series of chips. They all have this value, but the offset may be different.
>> And we can generate the dts file from excel which is defined by SOC.
>
>> I think the dts is designed to distinguish different platform. If we hard code this
>> in files, it may be also different to use as common in next chip version.
>
> If your tooling can generate DT files it can generate C code just as
> well and it seems unlikely you're going to be able to build new boards
> without being able to do firmware updates here. Especially for the
> sorts of systems that use DT the set of scenarios where you're able to
> update the DT but not the kernel seems like it will be extremely
> limited. I don't really buy the argument that there's any practical
> difference in the ability to update the kernel and DT and to the extent
> there is one it seems better to keep the ABI we have to support smaller
> by having the DT be minimal.
>
> This also allows us to map things more efficiently than we can with just
> a table of voltages. For example a good selection of the regulators in
> your example DT appear to be linear ranges and so should be mapped as
> such so we can do direct calcuations rather than having to iterate
> through a table to map voltages into selectors. That gets especially
> serious for higher resolution regulators like most DCDCs (and modern
> LDOs for that matter).
>
Thanks,
I see, I will change the table of voltages into driver.
like this,
static const unsigned int voltages[] = {
1500000, 1800000, 2400000, 2500000,
2600000, 2700000, 2850000, 3000000,
};
And there will be two open-code function for is-enable and disable in the regulator driver.
Since we need use the status and disable register on PM chip. Only enable reg in the regulator desc.
Do you agree with this?
On 2015/12/21 11:01, chenfeng wrote:
> Mark,
>
> On 2015/12/19 1:58, Mark Brown wrote:
>> On Thu, Dec 17, 2015 at 11:27:27AM +0800, chenfeng wrote:
>>
>>> +- regulator-vset-regs: Voltage set register offset.
>>> +- regulator-vset-mask: voltage set control mask.
>>> +- regulator-n-vol: The num of support voltages.
>>> +- regulator-vset-table: The table of support voltages.
>>
>>>> Why is this in the binding? This is a binding for a specific device,
>>>> there is no point in putting all these data tables in the DT - it just
>>>> bloats the DT and makes it harder for us to enhance our support for this
>>>> device in the future.
>>
>>> You mentioned in previous version,I I have some questions for it.
>>
>>> This regulator-vset-regs etc are vendor specific describe. The hi655x PMIC
>>
>> There's nothing vendor specific about the way this is written...
>>
>>> is a series of chips. They all have this value, but the offset may be different.
>>> And we can generate the dts file from excel which is defined by SOC.
>>
>>> I think the dts is designed to distinguish different platform. If we hard code this
>>> in files, it may be also different to use as common in next chip version.
>>
>> If your tooling can generate DT files it can generate C code just as
>> well and it seems unlikely you're going to be able to build new boards
>> without being able to do firmware updates here. Especially for the
>> sorts of systems that use DT the set of scenarios where you're able to
>> update the DT but not the kernel seems like it will be extremely
>> limited. I don't really buy the argument that there's any practical
>> difference in the ability to update the kernel and DT and to the extent
>> there is one it seems better to keep the ABI we have to support smaller
>> by having the DT be minimal.
>>
>> This also allows us to map things more efficiently than we can with just
>> a table of voltages. For example a good selection of the regulators in
>> your example DT appear to be linear ranges and so should be mapped as
>> such so we can do direct calcuations rather than having to iterate
>> through a table to map voltages into selectors. That gets especially
>> serious for higher resolution regulators like most DCDCs (and modern
>> LDOs for that matter).
>>
> Thanks,
> I see, I will change the table of voltages into driver.
> like this,
> static const unsigned int voltages[] = {
> 1500000, 1800000, 2400000, 2500000,
> 2600000, 2700000, 2850000, 3000000,
> };
>
> And there will be two open-code function for is-enable and disable in the regulator driver.
> Since we need use the status and disable register on PM chip. Only enable reg in the regulator desc.
>
> Do you agree with this?
>
While doing this in driver code, I found that it seems all the vendor chip have
the voltage table. So I am wondering can we add this into the regulator framework.
We can add in the function of_get_regulation_constraints to get the vset table.
I am not sure this is right or not.
>
>
On Mon, Dec 21, 2015 at 11:01:37AM +0800, chenfeng wrote:
Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns. Doing this makes your messages much
easier to read and reply to.
> I see, I will change the table of voltages into driver.
> like this,
> static const unsigned int voltages[] = {
> 1500000, 1800000, 2400000, 2500000,
> 2600000, 2700000, 2850000, 3000000,
> };
> And there will be two open-code function for is-enable and disable in the regulator driver.
> Since we need use the status and disable register on PM chip. Only enable reg in the regulator desc.
> Do you agree with this?
Yes.
On Mon, Dec 21, 2015 at 02:20:16PM +0800, chenfeng wrote:
Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns. Doing this makes your messages much
easier to read and reply to.
> While doing this in driver code, I found that it seems all the vendor
> chip have the voltage table. So I am wondering can we add this into
> the regulator framework.
> We can add in the function of_get_regulation_constraints to get the
> vset table.
> I am not sure this is right or not.
I'm just not convinced it's a good pattern to move this data out to DT,
like I said in my other mail it's making the ABI bigger and I'm not sure
I see much upside over putting the data in a table in DT rather than in
C code. It's more parsing code and more things we really shouldn't
change in future.
On 2015/12/23 8:46, Mark Brown wrote:
> On Mon, Dec 21, 2015 at 02:20:16PM +0800, chenfeng wrote:
>
> Please fix your mail client to word wrap within paragraphs at something
> substantially less than 80 columns. Doing this makes your messages much
> easier to read and reply to.
>
ok, thanks for your advice.
>> While doing this in driver code, I found that it seems all the vendor
>> chip have the voltage table. So I am wondering can we add this into
>> the regulator framework.
>
>> We can add in the function of_get_regulation_constraints to get the
>> vset table.
>
>> I am not sure this is right or not.
>
> I'm just not convinced it's a good pattern to move this data out to DT,
> like I said in my other mail it's making the ABI bigger and I'm not sure
> I see much upside over putting the data in a table in DT rather than in
> C code. It's more parsing code and more things we really shouldn't
> change in future.
>
ok. I will send the new version soon.