All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V1] regulator: pv88080: Update regulator for PV88080 BB silicon support
@ 2016-09-21  4:44 Eric Jeong
  2016-09-24 18:22 ` Mark Brown
  0 siblings, 1 reply; 4+ messages in thread
From: Eric Jeong @ 2016-09-21  4:44 UTC (permalink / raw)
  To: LINUXKERNEL, Liam Girdwood, Mark Brown; +Cc: Support Opensource


From: Eric Jeong <eric.jeong.opensource@diasemi.com>

Three files are modified, the driver, header file and the binding document.

Updates for the regulator source file include and .of_match_table entry
and node match checking in the probe() function for a compatible pv88080 
silicon type. A new "HVBUCK" is added in source file and added 
regsiter definition in header file for pv88080 bb silicion.
The binding documentation changes have been made to reflect these updates.

Signed-off-by: Eric Jeong <eric.jeong.opensource@diasemi.com>

---
Changes
- Added register definition for PV88080 BB silicon.
- Updated compatible ID table.
- Added "HVBUCK" regulator.
- Updated binding documentation.

This patch applies against linux-next and next-20160920 


 .../devicetree/bindings/regulator/pv88080.txt      |   23 +-
 drivers/regulator/pv88080-regulator.c              |  257 +++++++++++++++++---
 drivers/regulator/pv88080-regulator.h              |  114 +++++----
 3 files changed, 316 insertions(+), 78 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/pv88080.txt b/Documentation/devicetree/bindings/regulator/pv88080.txt
index 38a6142..e6e4b9c8 100644
--- a/Documentation/devicetree/bindings/regulator/pv88080.txt
+++ b/Documentation/devicetree/bindings/regulator/pv88080.txt
@@ -1,22 +1,28 @@
 * Powerventure Semiconductor PV88080 Voltage Regulator
 
 Required properties:
-- compatible: "pvs,pv88080".
-- reg: I2C slave address, usually 0x49.
+- compatible: Must be one of the following, depending on the
+  silicon version:
+	- "pvs,pv88080" (DEPRECATED)
+
+	- "pvs,pv88080-aa" for PV88080 AA or AB silicon
+	- "pvs,pv88080-ba" for PV88080 BA or BB silicon
+  NOTE: The use of the compatibles with no silicon version is deprecated.
+- reg: I2C slave address, usually 0x49
 - interrupts: the interrupt outputs of the controller
 - regulators: A node that houses a sub-node for each regulator within the
   device. Each sub-node is identified using the node's name, with valid
   values listed below. The content of each sub-node is defined by the
   standard binding for regulators; see regulator.txt.
-  BUCK1, BUCK2, and BUCK3.
+  BUCK1, BUCK2, BUCK3 and HVBUCK.
 
 Optional properties:
 - Any optional property defined in regulator.txt
 
-Example
+Example:
 
 	pmic: pv88080@49 {
-		compatible = "pvs,pv88080";
+		compatible = "pvs,pv88080-ba";
 		reg = <0x49>;
 		interrupt-parent = <&gpio>;
 		interrupts = <24 24>;
@@ -45,5 +51,12 @@ Example
 				regulator-min-microamp 	= <1496000>;
 				regulator-max-microamp 	= <4189000>;
 			};
+
+			HVBUCK {
+				regulator-name = "hvbuck";
+				regulator-min-microvolt = <   5000>;
+				regulator-max-microvolt = <1275000>;
+ 			};
 		};
 	};
+
diff --git a/drivers/regulator/pv88080-regulator.c b/drivers/regulator/pv88080-regulator.c
index 81950bd..9d97e3e 100644
--- a/drivers/regulator/pv88080-regulator.c
+++ b/drivers/regulator/pv88080-regulator.c
@@ -16,6 +16,7 @@
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/regulator/driver.h>
@@ -26,7 +27,7 @@
 #include <linux/regulator/of_regulator.h>
 #include "pv88080-regulator.h"
 
-#define PV88080_MAX_REGULATORS	3
+#define PV88080_MAX_REGULATORS	4
 
 /* PV88080 REGULATOR IDs */
 enum {
@@ -34,6 +35,12 @@ enum {
 	PV88080_ID_BUCK1,
 	PV88080_ID_BUCK2,
 	PV88080_ID_BUCK3,
+	PV88080_ID_HVBUCK,
+};
+
+enum pv88080_types {
+	TYPE_PV88080_AA,
+	TYPE_PV88080_BA,
 };
 
 struct pv88080_regulator {
@@ -42,7 +49,8 @@ struct pv88080_regulator {
 	unsigned int n_current_limits;
 	const int *current_limits;
 	unsigned int limit_mask;
-	unsigned int conf;
+	unsigned int mode_reg;
+	unsigned int limit_reg;
 	unsigned int conf2;
 	unsigned int conf5;
 };
@@ -51,6 +59,8 @@ struct pv88080 {
 	struct device *dev;
 	struct regmap *regmap;
 	struct regulator_dev *rdev[PV88080_MAX_REGULATORS];
+	unsigned long type;
+	const struct pv88080_compatible_regmap *regmap_config;
 };
 
 struct pv88080_buck_voltage {
@@ -59,6 +69,30 @@ struct pv88080_buck_voltage {
 	int uV_step;
 };
 
+struct pv88080_buck_regmap {
+	/* REGS */
+	int buck_enable_reg;
+	int buck_vsel_reg;
+	int buck_mode_reg;
+	int buck_limit_reg;
+	int buck_vdac_range_reg;
+	int buck_vrange_gain_reg;
+	/* MASKS */
+	int buck_enable_mask;
+	int buck_vsel_mask;
+	int buck_limit_mask;
+};
+
+struct pv88080_compatible_regmap {
+	/* BUCK1, 2, 3 */
+	struct pv88080_buck_regmap buck_regmap[PV88080_MAX_REGULATORS-1];
+	/* HVBUCK */
+	int hvbuck_enable_reg;
+	int hvbuck_vsel_reg;
+	int hvbuck_enable_mask;
+	int hvbuck_vsel_mask;
+};
+
 static const struct regmap_config pv88080_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -89,13 +123,110 @@ struct pv88080_buck_voltage {
 	},
 };
 
+static const struct pv88080_compatible_regmap pv88080_aa_regs = {
+	/* BUCK1 */
+	.buck_regmap[0] = {
+		.buck_enable_reg      = PV88080AA_REG_BUCK1_CONF0,
+		.buck_vsel_reg        = PV88080AA_REG_BUCK1_CONF0,
+		.buck_mode_reg        = PV88080AA_REG_BUCK1_CONF1,
+		.buck_limit_reg       = PV88080AA_REG_BUCK1_CONF1,
+		.buck_vdac_range_reg  = PV88080AA_REG_BUCK1_CONF2,
+		.buck_vrange_gain_reg = PV88080AA_REG_BUCK1_CONF5,
+		.buck_enable_mask     = PV88080_BUCK1_EN,
+		.buck_vsel_mask       = PV88080_VBUCK1_MASK,
+		.buck_limit_mask      = PV88080_BUCK1_ILIM_MASK,
+	},
+	/* BUCK2 */
+	.buck_regmap[1] = {
+		.buck_enable_reg      = PV88080AA_REG_BUCK2_CONF0,
+		.buck_vsel_reg        = PV88080AA_REG_BUCK2_CONF0,
+		.buck_mode_reg        = PV88080AA_REG_BUCK2_CONF1,
+		.buck_limit_reg	      = PV88080AA_REG_BUCK2_CONF1,
+		.buck_vdac_range_reg  = PV88080AA_REG_BUCK2_CONF2,
+		.buck_vrange_gain_reg = PV88080AA_REG_BUCK2_CONF5,
+		.buck_enable_mask	  = PV88080_BUCK2_EN,
+		.buck_vsel_mask       = PV88080_VBUCK2_MASK,
+		.buck_limit_mask      = PV88080_BUCK2_ILIM_MASK,
+	},
+	/* BUCK3 */
+	.buck_regmap[2] = {
+		.buck_enable_reg	  = PV88080AA_REG_BUCK3_CONF0,
+		.buck_vsel_reg        = PV88080AA_REG_BUCK3_CONF0,
+		.buck_mode_reg        = PV88080AA_REG_BUCK3_CONF1,
+		.buck_limit_reg	      = PV88080AA_REG_BUCK3_CONF1,
+		.buck_vdac_range_reg  = PV88080AA_REG_BUCK3_CONF2,
+		.buck_vrange_gain_reg = PV88080AA_REG_BUCK3_CONF5,
+		.buck_enable_mask	  = PV88080_BUCK3_EN,
+		.buck_vsel_mask       = PV88080_VBUCK3_MASK,
+		.buck_limit_mask      = PV88080_BUCK3_ILIM_MASK,
+	},
+	/* HVBUCK */
+	.hvbuck_enable_reg	      = PV88080AA_REG_HVBUCK_CONF2,
+	.hvbuck_vsel_reg          = PV88080AA_REG_HVBUCK_CONF1,
+	.hvbuck_enable_mask       = PV88080_HVBUCK_EN,
+	.hvbuck_vsel_mask         = PV88080_VHVBUCK_MASK,
+};
+
+static const struct pv88080_compatible_regmap pv88080_ba_regs = {
+	/* BUCK1 */
+	.buck_regmap[0] = {
+		.buck_enable_reg	  = PV88080BA_REG_BUCK1_CONF0,
+		.buck_vsel_reg        = PV88080BA_REG_BUCK1_CONF0,
+		.buck_mode_reg        = PV88080BA_REG_BUCK1_CONF1,
+		.buck_limit_reg       = PV88080BA_REG_BUCK1_CONF1,
+		.buck_vdac_range_reg  = PV88080BA_REG_BUCK1_CONF2,
+		.buck_vrange_gain_reg = PV88080BA_REG_BUCK1_CONF5,
+		.buck_enable_mask     = PV88080_BUCK1_EN,
+		.buck_vsel_mask       = PV88080_VBUCK1_MASK,
+		.buck_limit_mask	  = PV88080_BUCK1_ILIM_MASK,
+	},
+	/* BUCK2 */
+	.buck_regmap[1] = {
+		.buck_enable_reg	  = PV88080BA_REG_BUCK2_CONF0,
+		.buck_vsel_reg        = PV88080BA_REG_BUCK2_CONF0,
+		.buck_mode_reg        = PV88080BA_REG_BUCK2_CONF1,
+		.buck_limit_reg	      = PV88080BA_REG_BUCK2_CONF1,
+		.buck_vdac_range_reg  = PV88080BA_REG_BUCK2_CONF2,
+		.buck_vrange_gain_reg = PV88080BA_REG_BUCK2_CONF5,
+		.buck_enable_mask	  = PV88080_BUCK2_EN,
+		.buck_vsel_mask       = PV88080_VBUCK2_MASK,
+		.buck_limit_mask	  = PV88080_BUCK2_ILIM_MASK,
+	},
+	/* BUCK3 */
+	.buck_regmap[2] = {
+		.buck_enable_reg	  = PV88080BA_REG_BUCK3_CONF0,
+		.buck_vsel_reg        = PV88080BA_REG_BUCK3_CONF0,
+		.buck_mode_reg        = PV88080BA_REG_BUCK3_CONF1,
+		.buck_limit_reg	      = PV88080BA_REG_BUCK3_CONF1,
+		.buck_vdac_range_reg  = PV88080BA_REG_BUCK3_CONF2,
+		.buck_vrange_gain_reg = PV88080BA_REG_BUCK3_CONF5,
+		.buck_enable_mask	  = PV88080_BUCK3_EN,
+		.buck_vsel_mask       = PV88080_VBUCK3_MASK,
+		.buck_limit_mask	  = PV88080_BUCK3_ILIM_MASK,
+	},
+	/* HVBUCK */
+	.hvbuck_enable_reg	      = PV88080BA_REG_HVBUCK_CONF2,
+	.hvbuck_vsel_reg          = PV88080BA_REG_HVBUCK_CONF1,
+	.hvbuck_enable_mask       = PV88080_HVBUCK_EN,
+	.hvbuck_vsel_mask		  = PV88080_VHVBUCK_MASK,
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id pv88080_dt_ids[] = {
+	{ .compatible = "pvs,pv88080-aa", .data = (void *)TYPE_PV88080_AA },
+	{ .compatible = "pvs,pv88080-ba", .data = (void *)TYPE_PV88080_BA },
+	{},
+};
+MODULE_DEVICE_TABLE(of, pv88080_dt_ids);
+#endif
+
 static unsigned int pv88080_buck_get_mode(struct regulator_dev *rdev)
 {
 	struct pv88080_regulator *info = rdev_get_drvdata(rdev);
 	unsigned int data;
 	int ret, mode = 0;
 
-	ret = regmap_read(rdev->regmap, info->conf, &data);
+	ret = regmap_read(rdev->regmap, info->mode_reg, &data);
 	if (ret < 0)
 		return ret;
 
@@ -136,7 +267,7 @@ static int pv88080_buck_set_mode(struct regulator_dev *rdev,
 		return -EINVAL;
 	}
 
-	return regmap_update_bits(rdev->regmap, info->conf,
+	return regmap_update_bits(rdev->regmap, info->mode_reg,
 					PV88080_BUCK1_MODE_MASK, val);
 }
 
@@ -151,7 +282,7 @@ static int pv88080_set_current_limit(struct regulator_dev *rdev, int min,
 		if (min <= info->current_limits[i]
 			&& max >= info->current_limits[i]) {
 				return regmap_update_bits(rdev->regmap,
-					info->conf,
+					info->limit_reg,
 					info->limit_mask,
 					i << PV88080_BUCK1_ILIM_SHIFT);
 		}
@@ -166,7 +297,7 @@ static int pv88080_get_current_limit(struct regulator_dev *rdev)
 	unsigned int data;
 	int ret;
 
-	ret = regmap_read(rdev->regmap, info->conf, &data);
+	ret = regmap_read(rdev->regmap, info->limit_reg, &data);
 	if (ret < 0)
 		return ret;
 
@@ -187,6 +318,15 @@ static int pv88080_get_current_limit(struct regulator_dev *rdev)
 	.get_current_limit = pv88080_get_current_limit,
 };
 
+static struct regulator_ops pv88080_hvbuck_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.is_enabled = regulator_is_enabled_regmap,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.list_voltage = regulator_list_voltage_linear,
+};
+
 #define PV88080_BUCK(chip, regl_name, min, step, max, limits_array) \
 {\
 	.desc	=	{\
@@ -200,17 +340,25 @@ static int pv88080_get_current_limit(struct regulator_dev *rdev)
 		.min_uV = min, \
 		.uV_step = step, \
 		.n_voltages = ((max) - (min))/(step) + 1, \
-		.enable_reg = PV88080_REG_##regl_name##_CONF0, \
-		.enable_mask = PV88080_##regl_name##_EN, \
-		.vsel_reg = PV88080_REG_##regl_name##_CONF0, \
-		.vsel_mask = PV88080_V##regl_name##_MASK, \
 	},\
 	.current_limits = limits_array, \
 	.n_current_limits = ARRAY_SIZE(limits_array), \
-	.limit_mask = PV88080_##regl_name##_ILIM_MASK, \
-	.conf = PV88080_REG_##regl_name##_CONF1, \
-	.conf2 = PV88080_REG_##regl_name##_CONF2, \
-	.conf5 = PV88080_REG_##regl_name##_CONF5, \
+}
+
+#define PV88080_HVBUCK(chip, regl_name, min, step, max) \
+{\
+	.desc	=	{\
+		.id = chip##_ID_##regl_name,\
+		.name = __stringify(chip##_##regl_name),\
+		.of_match = of_match_ptr(#regl_name),\
+		.regulators_node = of_match_ptr("regulators"),\
+		.type = REGULATOR_VOLTAGE,\
+		.owner = THIS_MODULE,\
+		.ops = &pv88080_hvbuck_ops,\
+		.min_uV = min, \
+		.uV_step = step, \
+		.n_voltages = ((max) - (min))/(step) + 1, \
+	},\
 }
 
 static struct pv88080_regulator pv88080_regulator_info[] = {
@@ -220,6 +368,7 @@ static int pv88080_get_current_limit(struct regulator_dev *rdev)
 		pv88080_buck23_limits),
 	PV88080_BUCK(PV88080, BUCK3, 600000, 6250, 1393750,
 		pv88080_buck23_limits),
+	PV88080_HVBUCK(PV88080, HVBUCK, 0, 5000, 1275000),
 };
 
 static irqreturn_t pv88080_irq_handler(int irq, void *data)
@@ -280,6 +429,8 @@ static int pv88080_i2c_probe(struct i2c_client *i2c,
 {
 	struct regulator_init_data *init_data = dev_get_platdata(&i2c->dev);
 	struct pv88080 *chip;
+	const struct pv88080_compatible_regmap *regmap_config;
+	const struct of_device_id *match;
 	struct regulator_config config = { };
 	int i, error, ret;
 	unsigned int conf2, conf5;
@@ -297,6 +448,17 @@ static int pv88080_i2c_probe(struct i2c_client *i2c,
 		return error;
 	}
 
+	if (i2c->dev.of_node) {
+		match = of_match_node(pv88080_dt_ids, i2c->dev.of_node);
+		if (!match) {
+			dev_err(chip->dev, "Failed to get of_match_node\n");
+			return -EINVAL;
+		}
+		chip->type = (unsigned long)match->data;
+	} else {
+		chip->type = id->driver_data;
+	}
+
 	i2c_set_clientdata(i2c, chip);
 
 	if (i2c->irq != 0) {
@@ -336,31 +498,54 @@ static int pv88080_i2c_probe(struct i2c_client *i2c,
 				"Failed to update mask reg: %d\n", ret);
 			return ret;
 		}
-
 	} else {
 		dev_warn(chip->dev, "No IRQ configured\n");
 	}
 
+	if (chip->type == TYPE_PV88080_AA)
+		chip->regmap_config = &pv88080_aa_regs;
+	else
+		chip->regmap_config = &pv88080_ba_regs;
+
+	regmap_config = chip->regmap_config;
 	config.dev = chip->dev;
 	config.regmap = chip->regmap;
 
-	for (i = 0; i < PV88080_MAX_REGULATORS; i++) {
+	/* Registeration for BUCK1, 2, 3 */
+	for (i = 0; i < PV88080_MAX_REGULATORS-1; i++) {
 		if (init_data)
 			config.init_data = &init_data[i];
 
+		pv88080_regulator_info[i].limit_reg
+			= regmap_config->buck_regmap[i].buck_limit_reg;
+		pv88080_regulator_info[i].limit_mask
+			= regmap_config->buck_regmap[i].buck_limit_mask;
+		pv88080_regulator_info[i].mode_reg
+			= regmap_config->buck_regmap[i].buck_mode_reg;
+		pv88080_regulator_info[i].conf2
+			= regmap_config->buck_regmap[i].buck_vdac_range_reg;
+		pv88080_regulator_info[i].conf5
+			= regmap_config->buck_regmap[i].buck_vrange_gain_reg;
+		pv88080_regulator_info[i].desc.enable_reg
+			= regmap_config->buck_regmap[i].buck_enable_reg;
+		pv88080_regulator_info[i].desc.enable_mask
+			= regmap_config->buck_regmap[i].buck_enable_mask;
+		pv88080_regulator_info[i].desc.vsel_reg
+			= regmap_config->buck_regmap[i].buck_vsel_reg;
+		pv88080_regulator_info[i].desc.vsel_mask
+			= regmap_config->buck_regmap[i].buck_vsel_mask;
+
 		ret = regmap_read(chip->regmap,
-			pv88080_regulator_info[i].conf2, &conf2);
+				pv88080_regulator_info[i].conf2, &conf2);
 		if (ret < 0)
 			return ret;
-
 		conf2 = ((conf2 >> PV88080_BUCK_VDAC_RANGE_SHIFT) &
 			PV88080_BUCK_VDAC_RANGE_MASK);
 
 		ret = regmap_read(chip->regmap,
-			pv88080_regulator_info[i].conf5, &conf5);
+				pv88080_regulator_info[i].conf5, &conf5);
 		if (ret < 0)
 			return ret;
-
 		conf5 = ((conf5 >> PV88080_BUCK_VRANGE_GAIN_SHIFT) &
 			PV88080_BUCK_VRANGE_GAIN_MASK);
 
@@ -383,23 +568,37 @@ static int pv88080_i2c_probe(struct i2c_client *i2c,
 		}
 	}
 
+	pv88080_regulator_info[PV88080_ID_HVBUCK].desc.enable_reg
+		= regmap_config->hvbuck_enable_reg;
+	pv88080_regulator_info[PV88080_ID_HVBUCK].desc.enable_mask
+		= regmap_config->hvbuck_enable_mask;
+	pv88080_regulator_info[PV88080_ID_HVBUCK].desc.vsel_reg
+		= regmap_config->hvbuck_vsel_reg;
+	pv88080_regulator_info[PV88080_ID_HVBUCK].desc.vsel_mask
+		= regmap_config->hvbuck_vsel_mask;
+
+	/* Registeration for HVBUCK */
+	if (init_data)
+		config.init_data = &init_data[PV88080_ID_HVBUCK];
+
+	config.driver_data = (void *)&pv88080_regulator_info[PV88080_ID_HVBUCK];
+	chip->rdev[PV88080_ID_HVBUCK] = devm_regulator_register(chip->dev,
+		&pv88080_regulator_info[PV88080_ID_HVBUCK].desc, &config);
+	if (IS_ERR(chip->rdev[PV88080_ID_HVBUCK])) {
+		dev_err(chip->dev, "Failed to register PV88080 regulator\n");
+		return PTR_ERR(chip->rdev[PV88080_ID_HVBUCK]);
+	}
+
 	return 0;
 }
 
 static const struct i2c_device_id pv88080_i2c_id[] = {
-	{"pv88080", 0},
+	{ "pv88080-aa", TYPE_PV88080_AA },
+	{ "pv88080-ba", TYPE_PV88080_BA },
 	{},
 };
 MODULE_DEVICE_TABLE(i2c, pv88080_i2c_id);
 
-#ifdef CONFIG_OF
-static const struct of_device_id pv88080_dt_ids[] = {
-	{ .compatible = "pvs,pv88080", .data = &pv88080_i2c_id[0] },
-	{},
-};
-MODULE_DEVICE_TABLE(of, pv88080_dt_ids);
-#endif
-
 static struct i2c_driver pv88080_regulator_driver = {
 	.driver = {
 		.name = "pv88080",
diff --git a/drivers/regulator/pv88080-regulator.h b/drivers/regulator/pv88080-regulator.h
index 5e9afde..ae25ff3 100644
--- a/drivers/regulator/pv88080-regulator.h
+++ b/drivers/regulator/pv88080-regulator.h
@@ -17,55 +17,75 @@
 #define __PV88080_REGISTERS_H__
 
 /* System Control and Event Registers */
-#define	PV88080_REG_EVENT_A			0x04
-#define	PV88080_REG_MASK_A			0x09
-#define	PV88080_REG_MASK_B			0x0a
-#define	PV88080_REG_MASK_C			0x0b
-
-/* Regulator Registers */
-#define	PV88080_REG_BUCK1_CONF0			0x27
-#define	PV88080_REG_BUCK1_CONF1			0x28
-#define	PV88080_REG_BUCK1_CONF2			0x59
-#define	PV88080_REG_BUCK1_CONF5			0x5c
-#define	PV88080_REG_BUCK2_CONF0			0x29
-#define	PV88080_REG_BUCK2_CONF1			0x2a
-#define	PV88080_REG_BUCK2_CONF2			0x61
-#define	PV88080_REG_BUCK2_CONF5			0x64
-#define	PV88080_REG_BUCK3_CONF0			0x2b
-#define	PV88080_REG_BUCK3_CONF1			0x2c
-#define	PV88080_REG_BUCK3_CONF2			0x69
-#define	PV88080_REG_BUCK3_CONF5			0x6c
+#define	PV88080_REG_EVENT_A				0x04
+#define	PV88080_REG_MASK_A				0x09
+#define	PV88080_REG_MASK_B				0x0A
+#define	PV88080_REG_MASK_C				0x0B
+
+/* Regulator Registers - rev. AA */
+#define PV88080AA_REG_HVBUCK_CONF1		0x2D
+#define PV88080AA_REG_HVBUCK_CONF2		0x2E
+#define	PV88080AA_REG_BUCK1_CONF0		0x27
+#define	PV88080AA_REG_BUCK1_CONF1		0x28
+#define	PV88080AA_REG_BUCK1_CONF2		0x59
+#define	PV88080AA_REG_BUCK1_CONF5		0x5C
+#define	PV88080AA_REG_BUCK2_CONF0		0x29
+#define	PV88080AA_REG_BUCK2_CONF1		0x2A
+#define	PV88080AA_REG_BUCK2_CONF2		0x61
+#define	PV88080AA_REG_BUCK2_CONF5		0x64
+#define	PV88080AA_REG_BUCK3_CONF0		0x2B
+#define	PV88080AA_REG_BUCK3_CONF1		0x2C
+#define	PV88080AA_REG_BUCK3_CONF2		0x69
+#define	PV88080AA_REG_BUCK3_CONF5		0x6C
+
+/* Regulator Registers - rev. BA */
+#define	PV88080BA_REG_HVBUCK_CONF1		0x33
+#define	PV88080BA_REG_HVBUCK_CONF2		0x34
+#define	PV88080BA_REG_BUCK1_CONF0		0x2A
+#define	PV88080BA_REG_BUCK1_CONF1		0x2C
+#define	PV88080BA_REG_BUCK1_CONF2		0x5A
+#define	PV88080BA_REG_BUCK1_CONF5		0x5D
+#define	PV88080BA_REG_BUCK2_CONF0		0x2D
+#define	PV88080BA_REG_BUCK2_CONF1		0x2F
+#define	PV88080BA_REG_BUCK2_CONF2		0x63
+#define	PV88080BA_REG_BUCK2_CONF5		0x66
+#define	PV88080BA_REG_BUCK3_CONF0		0x30
+#define	PV88080BA_REG_BUCK3_CONF1		0x32
+#define	PV88080BA_REG_BUCK3_CONF2		0x6C
+#define	PV88080BA_REG_BUCK3_CONF5		0x6F
 
 /* PV88080_REG_EVENT_A (addr=0x04) */
 #define	PV88080_E_VDD_FLT				0x01
-#define	PV88080_E_OVER_TEMP			0x02
+#define	PV88080_E_OVER_TEMP				0x02
 
 /* PV88080_REG_MASK_A (addr=0x09) */
 #define	PV88080_M_VDD_FLT				0x01
-#define	PV88080_M_OVER_TEMP			0x02
+#define	PV88080_M_OVER_TEMP				0x02
 
-/* PV88080_REG_BUCK1_CONF0 (addr=0x27) */
+/* PV88080_REG_BUCK1_CONF0 (addr=0x27|0x2A) */
 #define	PV88080_BUCK1_EN				0x80
-#define PV88080_VBUCK1_MASK			0x7F
-/* PV88080_REG_BUCK2_CONF0 (addr=0x29) */
+#define PV88080_VBUCK1_MASK				0x7F
+
+/* PV88080_REG_BUCK2_CONF0 (addr=0x29|0x2D) */
 #define	PV88080_BUCK2_EN				0x80
-#define PV88080_VBUCK2_MASK			0x7F
-/* PV88080_REG_BUCK3_CONF0 (addr=0x2b) */
+#define PV88080_VBUCK2_MASK				0x7F
+
+/* PV88080_REG_BUCK3_CONF0 (addr=0x2B|0x30) */
 #define	PV88080_BUCK3_EN				0x80
-#define PV88080_VBUCK3_MASK			0x7F
+#define PV88080_VBUCK3_MASK				0x7F
 
-/* PV88080_REG_BUCK1_CONF1 (addr=0x28) */
-#define PV88080_BUCK1_ILIM_SHIFT			2
+/* PV88080_REG_BUCK1_CONF1 (addr=0x28|0x2C) */
+#define PV88080_BUCK1_ILIM_SHIFT		2
 #define PV88080_BUCK1_ILIM_MASK			0x0C
 #define PV88080_BUCK1_MODE_MASK			0x03
 
-/* PV88080_REG_BUCK2_CONF1 (addr=0x2a) */
-#define PV88080_BUCK2_ILIM_SHIFT			2
+/* PV88080_REG_BUCK2_CONF1 (addr=0x2A|0x2F) */
+#define PV88080_BUCK2_ILIM_SHIFT		2
 #define PV88080_BUCK2_ILIM_MASK			0x0C
 #define PV88080_BUCK2_MODE_MASK			0x03
 
-/* PV88080_REG_BUCK3_CONF1 (addr=0x2c) */
-#define PV88080_BUCK3_ILIM_SHIFT			2
+/* PV88080_REG_BUCK3_CONF1 (addr=0x2C|0x32) */
+#define PV88080_BUCK3_ILIM_SHIFT		2
 #define PV88080_BUCK3_ILIM_MASK			0x0C
 #define PV88080_BUCK3_MODE_MASK			0x03
 
@@ -73,20 +93,26 @@
 #define	PV88080_BUCK_MODE_AUTO			0x01
 #define	PV88080_BUCK_MODE_SYNC			0x02
 
-/* PV88080_REG_BUCK2_CONF2 (addr=0x61) */
-/* PV88080_REG_BUCK3_CONF2 (addr=0x69) */
-#define PV88080_BUCK_VDAC_RANGE_SHIFT			7
-#define PV88080_BUCK_VDAC_RANGE_MASK			0x01
+/* PV88080_REG_HVBUCK_CONF1 (addr=0x2D|0x33) */
+#define PV88080_VHVBUCK_MASK			0xFF
+
+/* PV88080_REG_HVBUCK_CONF1 (addr=0x2E|0x34) */
+#define PV88080_HVBUCK_EN				0x01
+
+/* PV88080_REG_BUCK2_CONF2 (addr=0x61|0x63) */
+/* PV88080_REG_BUCK3_CONF2 (addr=0x69|0x6C) */
+#define PV88080_BUCK_VDAC_RANGE_SHIFT	7
+#define PV88080_BUCK_VDAC_RANGE_MASK	0x01
 
-#define PV88080_BUCK_VDAC_RANGE_1			0x00
-#define PV88080_BUCK_VDAC_RANGE_2			0x01
+#define PV88080_BUCK_VDAC_RANGE_1		0x00
+#define PV88080_BUCK_VDAC_RANGE_2		0x01
 
-/* PV88080_REG_BUCK2_CONF5 (addr=0x64) */
-/* PV88080_REG_BUCK3_CONF5 (addr=0x6c) */
-#define PV88080_BUCK_VRANGE_GAIN_SHIFT			0
-#define PV88080_BUCK_VRANGE_GAIN_MASK			0x01
+/* PV88080_REG_BUCK2_CONF5 (addr=0x64|0x66) */
+/* PV88080_REG_BUCK3_CONF5 (addr=0x6C|0x6F) */
+#define PV88080_BUCK_VRANGE_GAIN_SHIFT	0
+#define PV88080_BUCK_VRANGE_GAIN_MASK	0x01
 
-#define PV88080_BUCK_VRANGE_GAIN_1			0x00
-#define PV88080_BUCK_VRANGE_GAIN_2			0x01
+#define PV88080_BUCK_VRANGE_GAIN_1		0x00
+#define PV88080_BUCK_VRANGE_GAIN_2		0x01
 
 #endif	/* __PV88080_REGISTERS_H__ */
-- 
end-of-patch for PATCH V1

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH V1] regulator: pv88080: Update regulator for PV88080 BB silicon support
  2016-09-21  4:44 [PATCH V1] regulator: pv88080: Update regulator for PV88080 BB silicon support Eric Jeong
@ 2016-09-24 18:22 ` Mark Brown
  2016-09-26  3:03   ` Eric Hyeung Dong Jeong
  0 siblings, 1 reply; 4+ messages in thread
From: Mark Brown @ 2016-09-24 18:22 UTC (permalink / raw)
  To: Eric Jeong; +Cc: LINUXKERNEL, Liam Girdwood, Support Opensource

[-- Attachment #1: Type: text/plain, Size: 1754 bytes --]

On Wed, Sep 21, 2016 at 01:44:39PM +0900, Eric Jeong wrote:

> +#ifdef CONFIG_OF
> +static const struct of_device_id pv88080_dt_ids[] = {
> +	{ .compatible = "pvs,pv88080-aa", .data = (void *)TYPE_PV88080_AA },
> +	{ .compatible = "pvs,pv88080-ba", .data = (void *)TYPE_PV88080_BA },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, pv88080_dt_ids);
> +#endif

This doesn't list the old compatible string and therefore ends up
breaking since the code defaults to assuming something it hasn't heard
of is BB when it seems more likely that old DTs using the old compatible
string will be for boards that also use the old silicon.  It would be
much better practice to explicitly list the old compatible string and
how we handle it, that won't break in future and avoids this kind of
error.

Is it really not possible to read the chip revision from the device
using ID registers?  That'd be even better.

> +	if (i2c->dev.of_node) {
> +		match = of_match_node(pv88080_dt_ids, i2c->dev.of_node);
> +		if (!match) {
> +			dev_err(chip->dev, "Failed to get of_match_node\n");
> +			return -EINVAL;
> +		}
> +		chip->type = (unsigned long)match->data;

This will generate the warning for all existing DTs as a result of the
above which isn't great.

> +	if (chip->type == TYPE_PV88080_AA)
> +		chip->regmap_config = &pv88080_aa_regs;
> +	else
> +		chip->regmap_config = &pv88080_ba_regs;

Use a switch statement here, it is more extensible if we get future chip
versions and will improve error handling since we won't just silently
treat unrecognized values as BB.

>  static const struct i2c_device_id pv88080_i2c_id[] = {
> -	{"pv88080", 0},
> +	{ "pv88080-aa", TYPE_PV88080_AA },
> +	{ "pv88080-ba", TYPE_PV88080_BA },

Same thing as with the compatible string here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread
* RE: [PATCH V1] regulator: pv88080: Update regulator for PV88080 BB silicon support
  2016-09-24 18:22 ` Mark Brown
@ 2016-09-26  3:03   ` Eric Hyeung Dong Jeong
  2016-09-26 15:58     ` Mark Brown
  0 siblings, 1 reply; 4+ messages in thread
From: Eric Hyeung Dong Jeong @ 2016-09-26  3:03 UTC (permalink / raw)
  To: Mark Brown, Eric Hyeung Dong Jeong
  Cc: LINUXKERNEL, Liam Girdwood, Support Opensource

On September 25, 2016 3:23 AM, Mark Brown wrote:

> On Wed, Sep 21, 2016 at 01:44:39PM +0900, Eric Jeong wrote:
> 
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id pv88080_dt_ids[] = {
> > +	{ .compatible = "pvs,pv88080-aa", .data = (void *)TYPE_PV88080_AA },
> > +	{ .compatible = "pvs,pv88080-ba", .data = (void *)TYPE_PV88080_BA },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, pv88080_dt_ids); #endif
> 
> This doesn't list the old compatible string and therefore ends up breaking
> since the code defaults to assuming something it hasn't heard of is BB when
> it seems more likely that old DTs using the old compatible string will be for
> boards that also use the old silicon.  It would be much better practice to
> explicitly list the old compatible string and how we handle it, that won't break
> in future and avoids this kind of error.
> 
> Is it really not possible to read the chip revision from the device using ID
> registers?  That'd be even better.

The address of ID register is different between each chip revision.
So, It is difficult to read the chip revision from the device using ID register.
And, I will send a patch with old compatible.

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH V1] regulator: pv88080: Update regulator for PV88080 BB silicon support
  2016-09-26  3:03   ` Eric Hyeung Dong Jeong
@ 2016-09-26 15:58     ` Mark Brown
  0 siblings, 0 replies; 4+ messages in thread
From: Mark Brown @ 2016-09-26 15:58 UTC (permalink / raw)
  To: Eric Hyeung Dong Jeong; +Cc: LINUXKERNEL, Liam Girdwood, Support Opensource

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]

On Mon, Sep 26, 2016 at 03:03:51AM +0000, Eric Hyeung Dong Jeong wrote:

> The address of ID register is different between each chip revision.
> So, It is difficult to read the chip revision from the device using ID register.

Oh dear, seems the hardware team haven't fully understood the purpose of
ID registers :(  Hopefully this has been fed back to them and future
devices will be better.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-09-26 15:58 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-21  4:44 [PATCH V1] regulator: pv88080: Update regulator for PV88080 BB silicon support Eric Jeong
2016-09-24 18:22 ` Mark Brown
2016-09-26  3:03   ` Eric Hyeung Dong Jeong
2016-09-26 15:58     ` Mark Brown

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.