Add driver for support max77686 regulator.
MAX77686 provides LDOs[1~26] and BUCKs[1~9]. It support to set or get the
volatege of regulator on max77686 chip with using regmap.
v4
- Register Crystal oscillator's clocks with generic clock API.
v3
- Remove voltage_map_desc structure and replace minimum voltage and voltage
step into regulator_desc.
- Use only regulator common APIs for regulator_ops.
(No more use driver's own funcions)
This patch is based on broonie/regulator/for-next branch.
v2
- Convert get_voltage to get_voltage_sel
- Convert set_voltage to set_voltage_sel
- Implement set_voltage_time_sel
- Register regulators unconditionally
- kzalloc -> devm_kzalloc
- Remove unneccessary printk
- Keep probing whether pdata exists or not
- Use regmap API to access PMIC register.
Signed-off-by: Chiwoong Byun <[email protected]>
Signed-off-by: Jonghwa Lee <[email protected]>
Signed-off-by: Myungjoo Ham <[email protected]>
Signed-off-by: Kyungmin Park <[email protected]>
---
drivers/regulator/Kconfig | 8 +
drivers/regulator/Makefile | 1 +
drivers/regulator/max77686.c | 540 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 549 insertions(+), 0 deletions(-)
create mode 100644 drivers/regulator/max77686.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 36db5a4..8e2ebad 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -195,6 +195,14 @@ config REGULATOR_MAX8998
via I2C bus. The provided regulator is suitable for S3C6410
and S5PC1XX chips to control VCC_CORE and VCC_USIM voltages.
+config REGULATOR_MAX77686
+ tristate "Maxim 77686 regulator"
+ depends on MFD_MAX77686
+ help
+ This driver controls a Maxim 77686 regulator
+ via I2C bus. The provided regulator is suitable for
+ Exynos-4 chips to control VARM and VINT voltages.
+
config REGULATOR_PCAP
tristate "Motorola PCAP2 regulator driver"
depends on EZX_PCAP
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 94b5274..008931b 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_REGULATOR_MAX8925) += max8925-regulator.o
obj-$(CONFIG_REGULATOR_MAX8952) += max8952.o
obj-$(CONFIG_REGULATOR_MAX8997) += max8997.o
obj-$(CONFIG_REGULATOR_MAX8998) += max8998.o
+obj-$(CONFIG_REGULATOR_MAX77686) += max77686.o
obj-$(CONFIG_REGULATOR_MC13783) += mc13783-regulator.o
obj-$(CONFIG_REGULATOR_MC13892) += mc13892-regulator.o
obj-$(CONFIG_REGULATOR_MC13XXX_CORE) += mc13xxx-regulator-core.o
diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
new file mode 100644
index 0000000..f412094
--- /dev/null
+++ b/drivers/regulator/max77686.c
@@ -0,0 +1,540 @@
+/*
+ * max77686.c - Regulator driver for the Maxim 77686
+ *
+ * Copyright (C) 2012 Samsung Electronics
+ * Chiwoong Byun <[email protected]>
+ * Jonghwa Lee <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * This driver is based on max8997.c
+ */
+
+#include <linux/kernel.h>
+#include <linux/bug.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/mfd/max77686.h>
+#include <linux/mfd/max77686-private.h>
+
+#ifdef CONFIG_COMMON_CLK
+#include <linux/clk-private.h>
+#endif
+
+#define MAX77686_LDO_MINUV 800000
+#define MAX77686_LDO_UVSTEP 50000
+#define MAX77686_LDO_LOW_MINUV 800000
+#define MAX77686_LDO_LOW_UVSTEP 25000
+#define MAX77686_BUCK_MINUV 750000
+#define MAX77686_BUCK_UVSTEP 50000
+#define MAX77686_DVS_MINUV 600000
+#define MAX77686_DVS_UVSTEP 12500
+
+#define MAX77686_OPMODE_SHIFT 6
+#define MAX77686_OPMODE_BUCK234_SHIFT 4
+#define MAX77686_OPMODE_MASK 0x3
+
+#define MAX77686_VSEL_MASK 0x3f
+#define MAX77686_DVS_VSEL_MASK 0xff
+
+#define MAX77686_RAMP_RATE 100
+
+#define MAX77686_REGULATORS MAX77686_REG_MAX
+#define MAX77686_LDOS 26
+
+struct max77686_data {
+ struct device *dev;
+ struct max77686_dev *iodev;
+ int num_regulators;
+ struct regulator_dev **rdev;
+ int ramp_delay; /* in mV/us */
+
+#ifdef CONFIG_COMMON_CLK
+ struct clk clk32khz_ap;
+ struct clk clk32khz_cp;
+ struct clk clk32khz_pmic;
+#endif
+ struct max77686_opmode_data *opmode_data;
+
+ u8 buck2_vol[8];
+ u8 buck3_vol[8];
+ u8 buck4_vol[8];
+};
+
+#ifdef CONFIG_COMMON_CLK
+
+#define to_max77686_data(__name) container_of(hw->clk, \
+ struct max77686_data, __name)
+
+static struct clk_gate clk32khz_ap_hw = {
+ .ret = MAX77686_REG_32KH,
+ .bit_idx = 0,
+ .flags = 0,
+ .lock = clk_lock,
+};
+
+static struct clk_gate clk32khz_cp_hw = {
+ .ret = MAX77686_REG_32KH,
+ .bit_idx = 1,
+ .flags = 0,
+ .lock = clk_lock,
+};
+
+static struct clk_gate clk32khz_pmic_hw = {
+ .ret = MAX77686_REG_32KH,
+ .bit_idx = 2,
+ .flags = 0,
+ .lock = clk_lock,
+};
+
+static int *clk_enable(struct clk_hw *hw);
+static void *clk_disable(struct clk_hw *hw);
+static int *clk_is_enable(struct clk_hw *hw);
+
+static struct clk_ops clk32khz_ops {
+ .enable = clk_enable,
+ .disable = clk_disable,
+ .is_enable = cli_is_enable,
+};
+#endif
+
+static int max77686_set_voltage_time_sel(struct regulator_dev *rdev,
+ unsigned int old_selector, unsigned int new_selector)
+{
+ struct max77686_data *max77686 = rdev_get_drvdata(rdev);
+ int rid = rdev_get_id(rdev);
+
+ switch (rid) {
+ case MAX77686_BUCK2 ... MAX77686_BUCK4:
+ return (DIV_ROUND_UP(rdev->desc->uV_step
+ * abs(new_selector - old_selector),
+ max77686->ramp_delay * 1000));
+ }
+ /* Unconditionally 100 mV/us */
+ return (DIV_ROUND_UP(rdev->desc->uV_step
+ * abs(new_selector - old_selector), 100000));
+}
+
+static struct regulator_ops max77686_ldo_ops = {
+ .list_voltage = regulator_list_voltage_linear,
+ .is_enabled = regulator_is_enabled_regmap,
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+ .set_voltage_sel = regulator_set_voltage_sel_regmap,
+ .set_voltage_time_sel = max77686_set_voltage_time_sel,
+};
+
+static struct regulator_ops max77686_buck_ops = {
+ .list_voltage = regulator_list_voltage_linear,
+ .is_enabled = regulator_is_enabled_regmap,
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+ .set_voltage_sel = regulator_set_voltage_sel_regmap,
+ .set_voltage_time_sel = max77686_set_voltage_time_sel,
+};
+
+#define regulator_desc_ldo(num) { \
+ .name = "LDO"#num, \
+ .id = MAX77686_LDO##num, \
+ .ops = &max77686_ldo_ops, \
+ .type = REGULATOR_VOLTAGE, \
+ .owner = THIS_MODULE, \
+ .min_uV = MAX77686_LDO_MINUV, \
+ .uV_step = MAX77686_LDO_UVSTEP, \
+ .n_voltages = MAX77686_VSEL_MASK + 1, \
+ .vsel_reg = MAX77686_REG_LDO1CTRL1 + num - 1, \
+ .vsel_mask = MAX77686_VSEL_MASK, \
+ .enable_reg = MAX77686_REG_LDO1CTRL1 + num - 1, \
+ .enable_mask = MAX77686_OPMODE_MASK \
+ << MAX77686_OPMODE_SHIFT, \
+}
+#define regulator_desc_ldo_low(num) { \
+ .name = "LDO"#num, \
+ .id = MAX77686_LDO##num, \
+ .ops = &max77686_ldo_ops, \
+ .type = REGULATOR_VOLTAGE, \
+ .owner = THIS_MODULE, \
+ .min_uV = MAX77686_LDO_LOW_MINUV, \
+ .uV_step = MAX77686_LDO_LOW_UVSTEP, \
+ .n_voltages = MAX77686_VSEL_MASK + 1, \
+ .vsel_reg = MAX77686_REG_LDO1CTRL1 + num - 1, \
+ .vsel_mask = MAX77686_VSEL_MASK, \
+ .enable_reg = MAX77686_REG_LDO1CTRL1 + num - 1, \
+ .enable_mask = MAX77686_OPMODE_MASK \
+ << MAX77686_OPMODE_SHIFT, \
+}
+#define regulator_desc_buck(num) { \
+ .name = "BUCK"#num, \
+ .id = MAX77686_BUCK##num, \
+ .ops = &max77686_buck_ops, \
+ .type = REGULATOR_VOLTAGE, \
+ .owner = THIS_MODULE, \
+ .min_uV = MAX77686_BUCK_MINUV, \
+ .uV_step = MAX77686_BUCK_UVSTEP, \
+ .n_voltages = MAX77686_VSEL_MASK + 1, \
+ .vsel_reg = MAX77686_REG_BUCK5OUT + (num - 5) * 2, \
+ .vsel_mask = MAX77686_VSEL_MASK, \
+ .enable_reg = MAX77686_REG_BUCK5CTRL + (num - 5) * 2, \
+ .enable_mask = MAX77686_OPMODE_MASK, \
+}
+#define regulator_desc_buck1(num) { \
+ .name = "BUCK"#num, \
+ .id = MAX77686_BUCK##num, \
+ .ops = &max77686_buck_ops, \
+ .type = REGULATOR_VOLTAGE, \
+ .owner = THIS_MODULE, \
+ .min_uV = MAX77686_BUCK_MINUV, \
+ .uV_step = MAX77686_BUCK_UVSTEP, \
+ .n_voltages = MAX77686_VSEL_MASK + 1, \
+ .vsel_reg = MAX77686_REG_BUCK1OUT, \
+ .vsel_mask = MAX77686_VSEL_MASK, \
+ .enable_reg = MAX77686_REG_BUCK1CTRL, \
+ .enable_mask = MAX77686_OPMODE_MASK, \
+}
+#define regulator_desc_buck_dvs(num) { \
+ .name = "BUCK"#num, \
+ .id = MAX77686_BUCK##num, \
+ .ops = &max77686_buck_ops, \
+ .type = REGULATOR_VOLTAGE, \
+ .owner = THIS_MODULE, \
+ .min_uV = MAX77686_DVS_MINUV, \
+ .uV_step = MAX77686_DVS_UVSTEP, \
+ .n_voltages = MAX77686_DVS_VSEL_MASK + 1, \
+ .vsel_reg = MAX77686_REG_BUCK2DVS1 + (num - 2) * 10, \
+ .vsel_mask = MAX77686_DVS_VSEL_MASK, \
+ .enable_reg = MAX77686_REG_BUCK2CTRL1 + (num - 2) * 10, \
+ .enable_mask = MAX77686_OPMODE_MASK \
+ << MAX77686_OPMODE_BUCK234_SHIFT, \
+}
+
+static struct regulator_desc regulators[] = {
+ regulator_desc_ldo_low(1),
+ regulator_desc_ldo_low(2),
+ regulator_desc_ldo(3),
+ regulator_desc_ldo(4),
+ regulator_desc_ldo(5),
+ regulator_desc_ldo_low(6),
+ regulator_desc_ldo_low(7),
+ regulator_desc_ldo_low(8),
+ regulator_desc_ldo(9),
+ regulator_desc_ldo(10),
+ regulator_desc_ldo(11),
+ regulator_desc_ldo(12),
+ regulator_desc_ldo(13),
+ regulator_desc_ldo(14),
+ regulator_desc_ldo_low(15),
+ regulator_desc_ldo(16),
+ regulator_desc_ldo(17),
+ regulator_desc_ldo(18),
+ regulator_desc_ldo(19),
+ regulator_desc_ldo(20),
+ regulator_desc_ldo(21),
+ regulator_desc_ldo(22),
+ regulator_desc_ldo(23),
+ regulator_desc_ldo(24),
+ regulator_desc_ldo(25),
+ regulator_desc_ldo(26),
+ regulator_desc_buck1(1),
+ regulator_desc_buck_dvs(2),
+ regulator_desc_buck_dvs(3),
+ regulator_desc_buck_dvs(4),
+ regulator_desc_buck(5),
+ regulator_desc_buck(6),
+ regulator_desc_buck(7),
+ regulator_desc_buck(8),
+ regulator_desc_buck(9),
+};
+
+#ifdef CONFIG_COMMON_CLK
+static int *clk_enable(struct clk_hw *hw)
+{
+ struct max77686_data *max77686 = NULL;
+ struct regmap *regmap;
+ int mask;
+
+ if (hw == &clk32khz_ap_hw) {
+ max77686 = to_max77686_dev(clk32khz_ap);
+ mask = (1 << MAX77686_32KH_AP);
+ } else if (hw == &clk32khz_cp_hw) {
+ max77686 = to_max77686_dev(clk32khz_cp);
+ mask = (1 << MAX77686_32KH_CP);
+ } else if (hw == &clk32khz_pmic_hw) {
+ max77686 = to_max77686_dev(clk32khz_pmic);
+ maske = (1 << MAX77686_P32KH);
+ }
+
+ if (!max77686)
+ return -ENOMEM;
+
+ return regmap_update_bits(regmap, MAX77686_REG_32KHZ, mask, 1);
+}
+
+static void *clk_disable(struct clk_hw *hw)
+{
+ struct max77686_data *max77686 = NULL;
+ struct regmap *regmap;
+ int mask;
+
+ if (hw == &clk32khz_ap_hw) {
+ max77686 = to_max77686_dev(clk32khz_ap);
+ mask = (1 << MAX77686_32KH_AP);
+ } else if (hw == &clk32khz_cp_hw) {
+ max77686 = to_max77686_dev(clk32khz_cp);
+ mask = (1 << MAX77686_32KH_CP);
+ } else if (hw == &clk32khz_pmic_hw) {
+ max77686 = to_max77686_dev(clk32khz_pmic);
+ maske = (1 << MAX77686_P32KH);
+ }
+
+ if (!max77686)
+ return -ENOMEM;
+
+ return regmap_update_bits(regmap, MAX77686_REG_32KHZ, mask, 0);
+}
+
+static int *clk_is_enable(struct clk_hw *hw)
+{
+ struct max77686_data *max77686 = NULL;
+ struct regmap *regmap;
+ int mask, ret;
+ u8 val;
+
+ if (hw == &clk32khz_ap_hw) {
+ max77686 = to_max77686_dev(clk32khz_ap);
+ mask = (1 << MAX77686_32KH_AP);
+ } else if (hw == &clk32khz_cp_hw) {
+ max77686 = to_max77686_dev(clk32khz_cp);
+ mask = (1 << MAX77686_32KH_CP);
+ } else if (hw == &clk32khz_pmic_hw) {
+ max77686 = to_max77686_dev(clk32khz_pmic);
+ maske = (1 << MAX77686_P32KH);
+ }
+
+ if (!max77686)
+ return -ENOMEM;
+
+ ret = regmap_read(regamp, MAX77686_REG_32KHZ, &val);
+ if (ret < 0)
+ return -EINVAL;
+
+ return val & mask;
+}
+
+static int set_max77686_clk_init(int cid, struct clk *clk)
+{
+ switch (cid) {
+ case MAX77686_32KH_AP:
+ clk->name = "32khap";
+ clk->hw = &clk32khz_ap_hw;
+ break;
+ case MAX77686_32KH_CP:
+ clk->name = "32khcp";
+ clk->hw = &clk32khz_cp_hw;
+ break;
+ case MAX77686_P32KH:
+ clk->name = "p32kh";
+ clk->hw = &clk32khz_pmic_hw;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ clk->ops = &clk_ops;
+ clk->flags = CLK_IS_ROOT;
+ clk->hw->clk = clk;
+ return 0;
+}
+#endif /* CONFIG_COMMON_CLK */
+
+static __devinit int max77686_pmic_probe(struct platform_device *pdev)
+{
+ struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent);
+ struct max77686_platform_data *pdata = dev_get_platdata(iodev->dev);
+ struct regulator_dev **rdev;
+ struct max77686_data *max77686;
+ struct regulator_init_data **init_data;
+ int i, size;
+ int ret = 0;
+ struct regulator_config config;
+
+ dev_dbg(&pdev->dev, "%s\n", __func__);
+
+ max77686 = devm_kzalloc(&pdev->dev, sizeof(struct max77686_data),
+ GFP_KERNEL);
+ if (!max77686)
+ return -ENOMEM;
+
+ size = sizeof(struct regulator_dev *) * MAX77686_REGULATORS;
+ max77686->rdev = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
+ if (!max77686->rdev)
+ return -ENOMEM;
+
+ rdev = max77686->rdev;
+ max77686->dev = &pdev->dev;
+ max77686->iodev = iodev;
+ if (pdata)
+ max77686->num_regulators = pdata->num_regulators;
+ platform_set_drvdata(pdev, max77686);
+ init_data = devm_kzalloc(&pdev->dev,
+ sizeof(struct regulator_init_data *)
+ * MAX77686_REGULATORS, GFP_KERNEL);
+ if (!init_data)
+ return -ENOMEM;
+
+ config.dev = max77686->dev;
+ config.regmap = iodev->regmap;
+ config.driver_data = max77686;
+
+ if (pdata)
+ max77686->opmode_data = pdata->opmode_data;
+
+ max77686->ramp_delay = MAX77686_RAMP_RATE; /* Set 0x3 for RAMP */
+ regmap_update_bits(max77686->iodev->regmap,
+ MAX77686_REG_BUCK2CTRL1, 0xC0, 0xC0);
+ regmap_update_bits(max77686->iodev->regmap,
+ MAX77686_REG_BUCK3CTRL1, 0xC0, 0xC0);
+ regmap_update_bits(max77686->iodev->regmap,
+ MAX77686_REG_BUCK4CTRL1, 0xC0, 0xC0);
+
+ for (i = 0; i < MAX77686_REGULATORS; i++) {
+ if (pdata)
+ init_data[pdata->regulators[i].id] =
+ pdata->regulators[i].initdata;
+
+ config.init_data = init_data[i];
+ rdev[i] = regulator_register(®ulators[i], &config);
+
+ if (IS_ERR(rdev[i])) {
+ ret = PTR_ERR(rdev[i]);
+ dev_err(max77686->dev,
+ "regulator init failed for %d\n", i);
+ rdev[i] = NULL;
+ goto err;
+ }
+ }
+
+#ifdef CONFIG_COMMON_CLK
+ set_max77686_clk_init(MAX77686_32KH_AP, &max77686->clk32khz_ap);
+ set_max77686_clk_init(MAX77686_32KH_CP, &max77686->clk32khz_cp);
+ set_max77686_clk_init(MAX77686_P32KH, &max77686->clk32khz_pmic);
+ __clk_init(&pdev->dev, &max77686->clk32khz_ap);
+ __clk_init(&pdev->dev, &max77686->clk32khz_cp);
+ __clk_init(&pdev->dev, &max77686->clk32khz_pmic);
+#endif
+
+ for (i = 0; i < 8; i++) {
+ if (pdata->buck2_voltage[i] > 0)
+ ret = regulator_map_voltage_linear(
+ rdev[MAX77686_LDOS+1],
+ pdata->buck2_voltage[i],
+ pdata->buck2_voltage[i]
+ + MAX77686_DVS_UVSTEP);
+ /* 1.1V as default for safety */
+ if (pdata->buck2_voltage[i] <= 0 || ret < 0)
+ max77686->buck2_vol[i] = 0x28;
+ else
+ max77686->buck2_vol[i] = ret;
+ regmap_write(max77686->iodev->regmap,
+ MAX77686_REG_BUCK2DVS1 + i, max77686->buck2_vol[i]);
+
+ if (pdata->buck3_voltage[i] > 0)
+ ret = regulator_map_voltage_linear(
+ rdev[MAX77686_LDOS+1],
+ pdata->buck3_voltage[i],
+ pdata->buck3_voltage[i]
+ + MAX77686_DVS_UVSTEP);
+ /* 1.1V as default for safety */
+ if (pdata->buck3_voltage[i] <= 0 || ret < 0)
+ max77686->buck3_vol[i] = 0x28;
+ else
+ max77686->buck3_vol[i] = ret;
+ regmap_write(max77686->iodev->regmap,
+ MAX77686_REG_BUCK3DVS1 + i, max77686->buck3_vol[i]);
+
+ if (pdata->buck4_voltage[i] > 0)
+ ret = regulator_map_voltage_linear(
+ rdev[MAX77686_LDOS+1],
+ pdata->buck4_voltage[i],
+ pdata->buck4_voltage[i]
+ + MAX77686_DVS_UVSTEP);
+ /* 1.1V as default for safety */
+ if (pdata->buck4_voltage[i] <= 0 || ret < 0)
+ max77686->buck4_vol[i] = 0x28;
+ else
+ max77686->buck4_vol[i] = ret;
+ regmap_write(max77686->iodev->regmap,
+ MAX77686_REG_BUCK4DVS1 + i, max77686->buck4_vol[i]);
+ }
+
+ return 0;
+err:
+ for (i = 0; i < MAX77686_REGULATORS; i++) {
+ if (rdev[i])
+ regulator_unregister(rdev[i]);
+ }
+ return ret;
+}
+
+static int __devexit max77686_pmic_remove(struct platform_device *pdev)
+{
+ struct max77686_data *max77686 = platform_get_drvdata(pdev);
+ struct regulator_dev **rdev = max77686->rdev;
+ int i;
+
+ for (i = 0; i < MAX77686_REGULATORS; i++)
+ if (rdev[i])
+ regulator_unregister(rdev[i]);
+
+ return 0;
+}
+
+static const struct platform_device_id max77686_pmic_id[] = {
+ { "max77686-pmic", 0},
+ { },
+};
+MODULE_DEVICE_TABLE(platform, max77686_pmic_id);
+
+static struct platform_driver max77686_pmic_driver = {
+ .driver = {
+ .name = "max77686-pmic",
+ .owner = THIS_MODULE,
+ },
+ .probe = max77686_pmic_probe,
+ .remove = __devexit_p(max77686_pmic_remove),
+ .id_table = max77686_pmic_id,
+};
+
+static int __init max77686_pmic_init(void)
+{
+ return platform_driver_register(&max77686_pmic_driver);
+}
+subsys_initcall(max77686_pmic_init);
+
+static void __exit max77686_pmic_cleanup(void)
+{
+ platform_driver_unregister(&max77686_pmic_driver);
+}
+module_exit(max77686_pmic_cleanup);
+
+MODULE_DESCRIPTION("MAXIM 77686 Regulator Driver");
+MODULE_AUTHOR("Chiwoong Byun <[email protected]>");
+MODULE_LICENSE("GPL");
--
1.7.4.1
Hi Jonghwa,
> +
> + ? ? ? if (pdata)
> + ? ? ? ? ? ? ? max77686->opmode_data = pdata->opmode_data;
I think this is unused(unwanted) now.
> +
> + ? ? ? for (i = 0; i < MAX77686_REGULATORS; i++) {
> + ? ? ? ? ? ? ? if (pdata)
> + ? ? ? ? ? ? ? ? ? ? ? init_data[pdata->regulators[i].id] =
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?pdata->regulators[i].initdata;
I think we can directly use pdata->regulators[i].initdata instead of
init_data[i].
In case if pdata is not their we can use same instance of
init_data(default) for all regulators.
> +
> + ? ? ? ? ? ? ? config.init_data = init_data[i];
> + ? ? ? ? ? ? ? rdev[i] = regulator_register(®ulators[i], &config);
> +
> +
> + ? ? ? for (i = 0; i < 8; i++) {
> + ? ? ? ? ? ? ? if (pdata->buck2_voltage[i] > 0)
> + ? ? ? ? ? ? ? ? ? ? ? ret = regulator_map_voltage_linear(
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? rdev[MAX77686_LDOS+1],
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pdata->buck2_voltage[i],
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pdata->buck2_voltage[i]
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? + MAX77686_DVS_UVSTEP);
> + ? ? ? ? ? ? ? /* 1.1V as default for safety */
> + ? ? ? ? ? ? ? if (pdata->buck2_voltage[i] <= 0 || ret < 0)
> + ? ? ? ? ? ? ? ? ? ? ? max77686->buck2_vol[i] = 0x28;
> + ? ? ? ? ? ? ? else
> + ? ? ? ? ? ? ? ? ? ? ? max77686->buck2_vol[i] = ret;
> + ? ? ? ? ? ? ? regmap_write(max77686->iodev->regmap,
> + ? ? ? ? ? ? ? ? ? ? ? ?MAX77686_REG_BUCK2DVS1 + i, max77686->buck2_vol[i]);
> +
> + ? ? ? ? ? ? ? if (pdata->buck3_voltage[i] > 0)
> + ? ? ? ? ? ? ? ? ? ? ? ret = regulator_map_voltage_linear(
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? rdev[MAX77686_LDOS+1],
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pdata->buck3_voltage[i],
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pdata->buck3_voltage[i]
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? + MAX77686_DVS_UVSTEP);
> + ? ? ? ? ? ? ? /* 1.1V as default for safety */
> + ? ? ? ? ? ? ? if (pdata->buck3_voltage[i] <= 0 || ret < 0)
> + ? ? ? ? ? ? ? ? ? ? ? max77686->buck3_vol[i] = 0x28;
> + ? ? ? ? ? ? ? else
> + ? ? ? ? ? ? ? ? ? ? ? max77686->buck3_vol[i] = ret;
> + ? ? ? ? ? ? ? regmap_write(max77686->iodev->regmap,
> + ? ? ? ? ? ? ? ? ? ? ? ?MAX77686_REG_BUCK3DVS1 + i, max77686->buck3_vol[i]);
> +
> + ? ? ? ? ? ? ? if (pdata->buck4_voltage[i] > 0)
> + ? ? ? ? ? ? ? ? ? ? ? ret = regulator_map_voltage_linear(
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? rdev[MAX77686_LDOS+1],
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pdata->buck4_voltage[i],
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pdata->buck4_voltage[i]
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? + MAX77686_DVS_UVSTEP);
> + ? ? ? ? ? ? ? /* 1.1V as default for safety */
> + ? ? ? ? ? ? ? if (pdata->buck4_voltage[i] <= 0 || ret < 0)
> + ? ? ? ? ? ? ? ? ? ? ? max77686->buck4_vol[i] = 0x28;
> + ? ? ? ? ? ? ? else
> + ? ? ? ? ? ? ? ? ? ? ? max77686->buck4_vol[i] = ret;
> + ? ? ? ? ? ? ? regmap_write(max77686->iodev->regmap,
> + ? ? ? ? ? ? ? ? ? ? ? ?MAX77686_REG_BUCK4DVS1 + i, max77686->buck4_vol[i]);
> + ? ? ? }
Why do we need to initialize the 8 voltage registers of BUCK2/3/4 ?
Regards,
Yadwinder.
Hi, Yadwinder,
On 2012년 05월 25일 21:27, Yadwinder Singh Brar wrote:
> Hi Jonghwa,
>
>> +
>> + if (pdata)
>> + max77686->opmode_data = pdata->opmode_data;
>
> I think this is unused(unwanted) now.
>
Sorry, i missed to remove it out. I'll remove it.
>> +
>> + for (i = 0; i < MAX77686_REGULATORS; i++) {
>> + if (pdata)
>> + init_data[pdata->regulators[i].id] =
>> + pdata->regulators[i].initdata;
>
> I think we can directly use pdata->regulators[i].initdata instead of
> init_data[i].
> In case if pdata is not their we can use same instance of
> init_data(default) for all regulators.
>
This if for some situation that pdata's initdata doensn't line up. When
user sets only initdata considered it being used, there may be
regulators not having initdata, also its order is not clear. So for
those state, i think just using temporary array which satisfies
regulator's id order is fine while it can't use pdata's initdata directly.
>> +
>> + config.init_data = init_data[i];
>> + rdev[i] = regulator_register(®ulators[i], &config);
>> +
>
>> +
>> + for (i = 0; i < 8; i++) {
>> + if (pdata->buck2_voltage[i] > 0)
>> + ret = regulator_map_voltage_linear(
>> + rdev[MAX77686_LDOS+1],
>> + pdata->buck2_voltage[i],
>> + pdata->buck2_voltage[i]
>> + + MAX77686_DVS_UVSTEP);
>> + /* 1.1V as default for safety */
>> + if (pdata->buck2_voltage[i] <= 0 || ret < 0)
>> + max77686->buck2_vol[i] = 0x28;
>> + else
>> + max77686->buck2_vol[i] = ret;
>> + regmap_write(max77686->iodev->regmap,
>> + MAX77686_REG_BUCK2DVS1 + i, max77686->buck2_vol[i]);
>> +
>> + if (pdata->buck3_voltage[i] > 0)
>> + ret = regulator_map_voltage_linear(
>> + rdev[MAX77686_LDOS+1],
>> + pdata->buck3_voltage[i],
>> + pdata->buck3_voltage[i]
>> + + MAX77686_DVS_UVSTEP);
>> + /* 1.1V as default for safety */
>> + if (pdata->buck3_voltage[i] <= 0 || ret < 0)
>> + max77686->buck3_vol[i] = 0x28;
>> + else
>> + max77686->buck3_vol[i] = ret;
>> + regmap_write(max77686->iodev->regmap,
>> + MAX77686_REG_BUCK3DVS1 + i, max77686->buck3_vol[i]);
>> +
>> + if (pdata->buck4_voltage[i] > 0)
>> + ret = regulator_map_voltage_linear(
>> + rdev[MAX77686_LDOS+1],
>> + pdata->buck4_voltage[i],
>> + pdata->buck4_voltage[i]
>> + + MAX77686_DVS_UVSTEP);
>> + /* 1.1V as default for safety */
>> + if (pdata->buck4_voltage[i] <= 0 || ret < 0)
>> + max77686->buck4_vol[i] = 0x28;
>> + else
>> + max77686->buck4_vol[i] = ret;
>> + regmap_write(max77686->iodev->regmap,
>> + MAX77686_REG_BUCK4DVS1 + i, max77686->buck4_vol[i]);
>> + }
>
> Why do we need to initialize the 8 voltage registers of BUCK2/3/4 ?
>
Yes you're right. It is useless because we don't support DVS mode at
this moment. I'll remove it either.
>
> Regards,
> Yadwinder.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
Thanks ,
Best Regards.
On Tue, May 29, 2012 at 7:40 AM, <[email protected]> wrote:
Hi Jonghwa,
>>> +
>>> + ? ? ? for (i = 0; i < MAX77686_REGULATORS; i++) {
>>> + ? ? ? ? ? ? ? if (pdata)
>>> + ? ? ? ? ? ? ? ? ? ? ? init_data[pdata->regulators[i].id] =
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?pdata->regulators[i].initdata;
>>
>> I ?think we can directly use ?pdata->regulators[i].initdata instead of
>> init_data[i].
>> In case if pdata is not their we can use same instance of
>> init_data(default) ?for all regulators.
>>
>
>
> This if for some situation that pdata's initdata doensn't line up. When
Ok, but I think this not right place for sorting (sorting is not taking
place.) You have to sort it before entering in loop for registering
regulators.
> user sets only initdata considered it being used, there may be
> regulators not having initdata, also its order is not clear. So for
Ok, I think this is a bug in present driver also, because
without checking pdata->num_regulators, you are running in
loop for (i = 0; i < MAX77686_REGULATORS; i++)
where MAX77686_REGULATORS should be equal to
pdata->num_regulators for this driver to work fine.
If we consider a case pdata->num_regulators is
equal to MAX77686_REGULATORS and initdata is
not their(i.e. NULL) than I think it will initialise
init_data[pdata->regulators[i].id to NULL, which again will be a bug.
> those state, i think just using temporary array which satisfies
> regulator's id order is fine while it can't use pdata's initdata directly.
>
If I am not wrong, I think we can also sort pdata's initdata also using
kernel's sort api and use one instance of (default)initdata for
all unused or uninitialized regulators in platform file.
>>> +
>>> + ? ? ? ? ? ? ? config.init_data = init_data[i];
>>> + ? ? ? ? ? ? ? rdev[i] = regulator_register(®ulators[i], &config);
>>> +
>>
Regards,
Yadwinder.
Hi Yadwinder,
I'm sorry for late reply. I understand the problem you pointed out, but
i don't agree with you all.
On 2012년 05월 29일 13:20, Yadwinder Singh Brar wrote:
> On Tue, May 29, 2012 at 7:40 AM, <[email protected]> wrote:
> Hi Jonghwa,
>
>>>> +
>>>> + for (i = 0; i < MAX77686_REGULATORS; i++) {
>>>> + if (pdata)
>>>> + init_data[pdata->regulators[i].id] =
>>>> + pdata->regulators[i].initdata;
>>>
>>> I think we can directly use pdata->regulators[i].initdata instead of
>>> init_data[i].
>>> In case if pdata is not their we can use same instance of
>>> init_data(default) for all regulators.
>>>
>>
>>
>> This if for some situation that pdata's initdata doensn't line up. When
>
> Ok, but I think this not right place for sorting (sorting is not taking
> place.) You have to sort it before entering in loop for registering
> regulators.
>
>> user sets only initdata considered it being used, there may be
>> regulators not having initdata, also its order is not clear. So for
>
> Ok, I think this is a bug in present driver also, because
> without checking pdata->num_regulators, you are running in
> loop for (i = 0; i < MAX77686_REGULATORS; i++)
> where MAX77686_REGULATORS should be equal to
> pdata->num_regulators for this driver to work fine.
>
I think we have same variable num_regulators but use differently. In my
code, it represents number of regulators to be used actually, but in
your code it equals to total number of regulators. Since it has
different meaning, it doesn't have to same with MAX77686_REGULATORS.
MAX77686_REGULATORS is macro which indicates total number of regulators
in max77686, and it equals to ARRAY_SIZE(regulators). Even if they are
not same, it's not a bug because we want to register all regulators
whether it will be used or not.
> If we consider a case pdata->num_regulators is
> equal to MAX77686_REGULATORS and initdata is
> not their(i.e. NULL) than I think it will initialise
> init_data[pdata->regulators[i].id to NULL, which again will be a bug.
>
>> those state, i think just using temporary array which satisfies
>> regulator's id order is fine while it can't use pdata's initdata directly.
>>
>
> If I am not wrong, I think we can also sort pdata's initdata also using
> kernel's sort api and use one instance of (default)initdata for
> all unused or uninitialized regulators in platform file.
>
If init_data references to NULL, it will be ignored while
register_regulators() does initialize. Thus it doesn't make any problem.
I'm afraid of using Kernel's sort API because of its overhead. Do you
think it will be better to use them? If you mind that init_data has been
dynamic allocated, it can be modified to a static pointer array.
>>>> +
>>>> + config.init_data = init_data[i];
>>>> + rdev[i] = regulator_register(®ulators[i], &config);
>>>> +
>>>
>
>
> Regards,
> Yadwinder.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
Thanks,
Hi Jonghwa,
On Wed, May 30, 2012 at 4:07 PM, <[email protected]> wrote:
> Hi Yadwinder,
>
> I'm sorry for late reply. I understand the problem you pointed out, but
> i don't agree with you all.
Sorry,I think you didn't get my points. Lets forget my code and talk
about this code now.
>>>>> +
>>>>> + ? ? ? for (i = 0; i < MAX77686_REGULATORS; i++) {
>>>>> + ? ? ? ? ? ? ? if (pdata)
>>>>> + ? ? ? ? ? ? ? ? ? ? ? init_data[pdata->regulators[i].id] =
>>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?pdata->regulators[i].initdata;
In case we have a list of 5 regulators only in pdata, than what will
happen here when i > 5 ???
>>>>
>>>> I ?think we can directly use ?pdata->regulators[i].initdata instead of
>>>> init_data[i].
>>>> In case if pdata is not their we can use same instance of
>>>> init_data(default) ?for all regulators.
>>>>
>>>
>>>
>>> This if for some situation that pdata's initdata doensn't line up. When
>>>>> + config.init_data = init_data[i];
>>>>> + rdev[i] = regulator_register(®ulators[i], &config);
In case pdata->regulators[0] is not the first regulator(i.e id > 0), then
will it get proper initdata for regulators[0] before registering ????
>>
>> Ok, but I think this not right place for sorting (sorting is not taking
>> place.) You have to sort it before entering in loop for registering
>> regulators.
>>
>>> user sets only initdata considered it being used, there may be
>>> regulators not having initdata, also its order is not clear. So for
>>
>> Ok, I think this is a bug in present driver also, because
>> without checking pdata->num_regulators, you are running in
>> loop ?for (i = 0; i < MAX77686_REGULATORS; i++)
>> where MAX77686_REGULATORS should be equal to
>> pdata->num_regulators for this driver to work fine.
>>
>
>
> I think we have same variable num_regulators but use differently. In my
> code, it represents number of regulators to be used actually, but in
> your code it equals to total number of regulators. Since it has
not exactly.
> different meaning, it doesn't have to same with MAX77686_REGULATORS.
> MAX77686_REGULATORS is macro which indicates total number of regulators
> in max77686, and it equals to ARRAY_SIZE(regulators). Even if they are
> not same, it's not a bug because we want to register all regulators
> whether it will be used or not.
>
>
>> If we consider a case pdata->num_regulators is
>> equal to MAX77686_REGULATORS and initdata is
>> not their(i.e. NULL) than I think it will initialise
>> init_data[pdata->regulators[i].id to NULL, which again will be a bug.
>>
>>> those state, i think just using temporary array which satisfies
>>> regulator's id order is fine while it can't use pdata's initdata directly.
>>>
>>
>> If I am not wrong, I think we can also sort pdata's initdata also using
>> kernel's sort api and use one instance of (default)initdata for
>> all unused or uninitialized regulators in platform file.
>>
>
>
> If init_data references to NULL, it will be ignored while
> register_regulators() does initialize. Thus it doesn't make any problem.
>
> I'm afraid of using Kernel's sort API because of its overhead. Do you
I don't think it's overhead will matter more than that of allocating a
new array and than
sorting it here.
> think it will be better to use them? If you mind that init_data has been
> dynamic allocated, it can be modified to a static pointer array.
>
No, their is no problem with dynamic.
Anyways, I had just suggested you to use pdata->regulators[i].initdata.
Regards,
?Yadwinder.
On 2012년 05월 30일 21:08, Yadwinder Singh Brar wrote:
> Hi Jonghwa,
>
> On Wed, May 30, 2012 at 4:07 PM, <[email protected]> wrote:
>> Hi Yadwinder,
>>
>> I'm sorry for late reply. I understand the problem you pointed out, but
>> i don't agree with you all.
>
> Sorry,I think you didn't get my points. Lets forget my code and talk
> about this code now.
>
>>>>>> +
>>>>>> + for (i = 0; i < MAX77686_REGULATORS; i++) {
>>>>>> + if (pdata)
>>>>>> + init_data[pdata->regulators[i].id] =
>>>>>> + pdata->regulators[i].initdata;
>
> In case we have a list of 5 regulators only in pdata, than what will
> happen here when i > 5 ???
>
You're right, it has bug. How do you think that change the condition to
(pdata && i < pdata->num_regulators)?
>>>>>
>>>>> I think we can directly use pdata->regulators[i].initdata instead of
>>>>> init_data[i].
>>>>> In case if pdata is not their we can use same instance of
>>>>> init_data(default) for all regulators.
>>>>>
>>>>
>>>>
>>>> This if for some situation that pdata's initdata doensn't line up. When
>
>>>>>> + config.init_data = init_data[i];
>>>>>> + rdev[i] = regulator_register(®ulators[i], &config);
>
> In case pdata->regulators[0] is not the first regulator(i.e id > 0), then
> will it get proper initdata for regulators[0] before registering ????
>
Yes, because above code replaces pdata->regalator's initdata to proper
position of initdata array referencing regulator's id.
>>>
>>> Ok, but I think this not right place for sorting (sorting is not taking
>>> place.) You have to sort it before entering in loop for registering
>>> regulators.
>>>
>>>> user sets only initdata considered it being used, there may be
>>>> regulators not having initdata, also its order is not clear. So for
>>>
>>> Ok, I think this is a bug in present driver also, because
>>> without checking pdata->num_regulators, you are running in
>>> loop for (i = 0; i < MAX77686_REGULATORS; i++)
>>> where MAX77686_REGULATORS should be equal to
>>> pdata->num_regulators for this driver to work fine.
>>>
>>
>>
>> I think we have same variable num_regulators but use differently. In my
>> code, it represents number of regulators to be used actually, but in
>> your code it equals to total number of regulators. Since it has
>
> not exactly.
>
>> different meaning, it doesn't have to same with MAX77686_REGULATORS.
>> MAX77686_REGULATORS is macro which indicates total number of regulators
>> in max77686, and it equals to ARRAY_SIZE(regulators). Even if they are
>> not same, it's not a bug because we want to register all regulators
>> whether it will be used or not.
>>
>>
>>> If we consider a case pdata->num_regulators is
>>> equal to MAX77686_REGULATORS and initdata is
>>> not their(i.e. NULL) than I think it will initialise
>>> init_data[pdata->regulators[i].id to NULL, which again will be a bug.
>>>
>>>> those state, i think just using temporary array which satisfies
>>>> regulator's id order is fine while it can't use pdata's initdata directly.
>>>>
>>>
>>> If I am not wrong, I think we can also sort pdata's initdata also using
>>> kernel's sort api and use one instance of (default)initdata for
>>> all unused or uninitialized regulators in platform file.
>>>
>>
>>
>> If init_data references to NULL, it will be ignored while
>> register_regulators() does initialize. Thus it doesn't make any problem.
>>
>> I'm afraid of using Kernel's sort API because of its overhead. Do you
>
> I don't think it's overhead will matter more than that of allocating a
> new array and than
> sorting it here.
>
>> think it will be better to use them? If you mind that init_data has been
>> dynamic allocated, it can be modified to a static pointer array.
>>
>
> No, their is no problem with dynamic.
> Anyways, I had just suggested you to use pdata->regulators[i].initdata.
>
So, to sum up to this, you think it is better to sort pdata->regulators
by its id before entering loop and just use pdata->regulators directly,
right? Okay, I'll do modify it.
> Regards,
> Yadwinder.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Thu, May 31, 2012 at 12:26 PM, <[email protected]> wrote:
> On 2012년 05월 30일 21:08, Yadwinder Singh Brar wrote:
>
>> Hi Jonghwa,
>>
>> On Wed, May 30, 2012 at 4:07 PM, <[email protected]> wrote:
>>> Hi Yadwinder,
>>>
>>> I'm sorry for late reply. I understand the problem you pointed out, but
>>> i don't agree with you all.
>>
>> Sorry,I think you didn't get my points. Lets forget my code and talk
>> about this code now.
>>
>>>>>>> +
>>>>>>> + for (i = 0; i < MAX77686_REGULATORS; i++) {
>>>>>>> + if (pdata)
>>>>>>> + init_data[pdata->regulators[i].id] =
>>>>>>> + pdata->regulators[i].initdata;
>>
>> In case we have a list of 5 regulators only in pdata, than what will
>> happen here when i > 5 ???
>>
>
>
> You're right, it has bug. How do you think that change the condition to
> (pdata && i < pdata->num_regulators)?
>
I think this stuff is not at right place, It should be moved out of this loop
to solve this both problems.
Please try to do all this stuff rel
>>>>>>
>>>>>> I think we can directly use pdata->regulators[i].initdata instead of
>>>>>> init_data[i].
>>>>>> In case if pdata is not their we can use same instance of
>>>>>> init_data(default) for all regulators.
>>>>>>
>>>>>
>>>>>
>>>>> This if for some situation that pdata's initdata doensn't line up. When
>>
>>>>>>> + config.init_data = init_data[i];
>>>>>>> + rdev[i] = regulator_register(®ulators[i], &config);
>>
>> In case pdata->regulators[0] is not the first regulator(i.e id > 0), then
>> will it get proper initdata for regulators[0] before registering ????
>>
>
>
> Yes, because above code replaces pdata->regalator's initdata to proper
> position of initdata array referencing regulator's id.
>
No, sorry i think you again missed the point.
Anyways, moving above stuff out of this loop
will also sove this problem.
>>>>
>>>> Ok, but I think this not right place for sorting (sorting is not taking
>>>> place.) You have to sort it before entering in loop for registering
>>>> regulators.
>>>>
>>>>> user sets only initdata considered it being used, there may be
>>>>> regulators not having initdata, also its order is not clear. So for
>>>>
>>>> Ok, I think this is a bug in present driver also, because
>>>> without checking pdata->num_regulators, you are running in
>>>> loop for (i = 0; i < MAX77686_REGULATORS; i++)
>>>> where MAX77686_REGULATORS should be equal to
>>>> pdata->num_regulators for this driver to work fine.
>>>>
>>>
>>>
>>> I think we have same variable num_regulators but use differently. In my
>>> code, it represents number of regulators to be used actually, but in
>>> your code it equals to total number of regulators. Since it has
>>
>> not exactly.
>>
>>> different meaning, it doesn't have to same with MAX77686_REGULATORS.
>>> MAX77686_REGULATORS is macro which indicates total number of regulators
>>> in max77686, and it equals to ARRAY_SIZE(regulators). Even if they are
>>> not same, it's not a bug because we want to register all regulators
>>> whether it will be used or not.
>>>
>>>
>
> So, to sum up to this, you think it is better to sort pdata->regulators
> by its id before entering loop and just use pdata->regulators directly,
> right? Okay, I'll do modify it.
>
Yes, this will allow us to add device tree support without any
modifications to this code in future and keeping our code simple
and compact.
In my opinion
>> Regards,
>> Yadwinder.
PLEASE
On Thu, May 31, 2012 at 12:26 PM, <[email protected]> wrote:
> On 2012년 05월 30일 21:08, Yadwinder Singh Brar wrote:
>
>> Hi Jonghwa,
>>
>> On Wed, May 30, 2012 at 4:07 PM, <[email protected]> wrote:
>>> Hi Yadwinder,
>>>
>>> I'm sorry for late reply. I understand the problem you pointed out, but
>>> i don't agree with you all.
>>
>> Sorry,I think you didn't get my points. Lets forget my code and talk
>> about this code now.
>>
>>>>>>> +
>>>>>>> + for (i = 0; i < MAX77686_REGULATORS; i++) {
>>>>>>> + if (pdata)
>>>>>>> + init_data[pdata->regulators[i].id] =
>>>>>>> + pdata->regulators[i].initdata;
>>
>> In case we have a list of 5 regulators only in pdata, than what will
>> happen here when i > 5 ???
>>
>
>
> You're right, it has bug. How do you think that change the condition to
> (pdata && i < pdata->num_regulators)?
>
>>>>>>
>>>>>> I think we can directly use pdata->regulators[i].initdata instead of
>>>>>> init_data[i].
>>>>>> In case if pdata is not their we can use same instance of
>>>>>> init_data(default) for all regulators.
>>>>>>
>>>>>
>>>>>
>>>>> This if for some situation that pdata's initdata doensn't line up. When
>>
>>>>>>> + config.init_data = init_data[i];
>>>>>>> + rdev[i] = regulator_register(®ulators[i], &config);
>>
>> In case pdata->regulators[0] is not the first regulator(i.e id > 0), then
>> will it get proper initdata for regulators[0] before registering ????
>>
>
>
> Yes, because above code replaces pdata->regalator's initdata to proper
> position of initdata array referencing regulator's id.
>
>>>>
>>>> Ok, but I think this not right place for sorting (sorting is not taking
>>>> place.) You have to sort it before entering in loop for registering
>>>> regulators.
>>>>
>>>>> user sets only initdata considered it being used, there may be
>>>>> regulators not having initdata, also its order is not clear. So for
>>>>
>>>> Ok, I think this is a bug in present driver also, because
>>>> without checking pdata->num_regulators, you are running in
>>>> loop for (i = 0; i < MAX77686_REGULATORS; i++)
>>>> where MAX77686_REGULATORS should be equal to
>>>> pdata->num_regulators for this driver to work fine.
>>>>
>>>
>>>
>>> I think we have same variable num_regulators but use differently. In my
>>> code, it represents number of regulators to be used actually, but in
>>> your code it equals to total number of regulators. Since it has
>>
>> not exactly.
>>
>>> different meaning, it doesn't have to same with MAX77686_REGULATORS.
>>> MAX77686_REGULATORS is macro which indicates total number of regulators
>>> in max77686, and it equals to ARRAY_SIZE(regulators). Even if they are
>>> not same, it's not a bug because we want to register all regulators
>>> whether it will be used or not.
>>>
>>>
>>>> If we consider a case pdata->num_regulators is
>>>> equal to MAX77686_REGULATORS and initdata is
>>>> not their(i.e. NULL) than I think it will initialise
>>>> init_data[pdata->regulators[i].id to NULL, which again will be a bug.
>>>>
>>>>> those state, i think just using temporary array which satisfies
>>>>> regulator's id order is fine while it can't use pdata's initdata directly.
>>>>>
>>>>
>>>> If I am not wrong, I think we can also sort pdata's initdata also using
>>>> kernel's sort api and use one instance of (default)initdata for
>>>> all unused or uninitialized regulators in platform file.
>>>>
>>>
>>>
>>> If init_data references to NULL, it will be ignored while
>>> register_regulators() does initialize. Thus it doesn't make any problem.
>>>
>>> I'm afraid of using Kernel's sort API because of its overhead. Do you
>>
>> I don't think it's overhead will matter more than that of allocating a
>> new array and than
>> sorting it here.
>>
>>> think it will be better to use them? If you mind that init_data has been
>>> dynamic allocated, it can be modified to a static pointer array.
>>>
>>
>> No, their is no problem with dynamic.
>> Anyways, I had just suggested you to use pdata->regulators[i].initdata.
>>
>
>
> So, to sum up to this, you think it is better to sort pdata->regulators
> by its id before entering loop and just use pdata->regulators directly,
> right? Okay, I'll do modify it.
>
>
>> Regards,
>> Yadwinder.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>>
>
>
PLEASE ignore previous incomplete mails. Sorry for noise by mistake.
On Thu, May 31, 2012 at 6:16 PM, Yadwinder Singh Brar
<[email protected]> wrote:
> On Thu, May 31, 2012 at 12:26 PM, <[email protected]> wrote:
>> On 2012년 05월 30일 21:08, Yadwinder Singh Brar wrote:
>>
>>> Hi Jonghwa,
>>>
>>> On Wed, May 30, 2012 at 4:07 PM, <[email protected]> wrote:
>>>> Hi Yadwinder,
>>>>
>>>> I'm sorry for late reply. I understand the problem you pointed out, but
>>>> i don't agree with you all.
>>>
>>> Sorry,I think you didn't get my points. Lets forget my code and talk
>>> about this code now.
>>>
>>>>>>>> +
>>>>>>>> + for (i = 0; i < MAX77686_REGULATORS; i++) {
>>>>>>>> + if (pdata)
>>>>>>>> + init_data[pdata->regulators[i].id] =
>>>>>>>> + pdata->regulators[i].initdata;
>>>
>>> In case we have a list of 5 regulators only in pdata, than what will
>>> happen here when i > 5 ???
>>>
>>
>>
>> You're right, it has bug. How do you think that change the condition to
>> (pdata && i < pdata->num_regulators)?
>>
>
I think this stuff is not at right place, It should be moved out of this loop
to solve this both problems.
Please try to do all this stuff related to platform file at starting while
checking if(pdata) {...}
>
>>>>>>>
>>>>>>> I think we can directly use pdata->regulators[i].initdata instead of
>>>>>>> init_data[i].
>>>>>>> In case if pdata is not their we can use same instance of
>>>>>>> init_data(default) for all regulators.
>>>>>>>
>>>>>>
>>>>>>
>>>>>> This if for some situation that pdata's initdata doensn't line up. When
>>>
>>>>>>>> + config.init_data = init_data[i];
>>>>>>>> + rdev[i] = regulator_register(®ulators[i], &config);
>>>
>>> In case pdata->regulators[0] is not the first regulator(i.e id > 0), then
>>> will it get proper initdata for regulators[0] before registering ????
>>>
>>
>>
>> Yes, because above code replaces pdata->regalator's initdata to proper
>> position of initdata array referencing regulator's id.
>>
>
> No, sorry i think you again missed the point.
> Anyways, moving above stuff out of this loop
> will also sove this problem.
>
>>>>>
>>>>> Ok, but I think this not right place for sorting (sorting is not taking
>>>>> place.) You have to sort it before entering in loop for registering
>>>>> regulators.
>>>>>
>>>>>> user sets only initdata considered it being used, there may be
>>>>>> regulators not having initdata, also its order is not clear. So for
>>>>>
>>>>> Ok, I think this is a bug in present driver also, because
>>>>> without checking pdata->num_regulators, you are running in
>>>>> loop for (i = 0; i < MAX77686_REGULATORS; i++)
>>>>> where MAX77686_REGULATORS should be equal to
>>>>> pdata->num_regulators for this driver to work fine.
>>>>>
>> So, to sum up to this, you think it is better to sort pdata->regulators
>> by its id before entering loop and just use pdata->regulators directly,
>> right? Okay, I'll do modify it.
>>
>
Yes, this will allow us to add device tree support without any
modifications to this code in future and keeping our code simple
and compact.
In my opinion since we are unconditionally registering all the regulators
to keep our code simple, as mark said, we should also populate also
all regulators in pdata list in platform file in sorted order to keep our code
simple here.
So that here we need to verify only if (pdata->num_regulators ==
MAX77686_REGULATORS).
>>> Regards,
>>> Yadwinder.