This patch series starts to add support for the twl6025 chip to the
twl driver. This series contains patches for the MFD device and the
regulator device to support the twl6025.
Graeme
The charger fault IRQs from the twl will in future patches be handled
by a seperate IRQ handler in the charger driver than the general charger
IRQ. Give them different IRQ numbers now to allow the charger driver to
be merged in the future.
Signed-off-by: Graeme Gregory <[email protected]>
---
drivers/mfd/twl6030-irq.c | 4 ++--
include/linux/i2c/twl.h | 1 +
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
index dfbae34..eb3b5f8 100644
--- a/drivers/mfd/twl6030-irq.c
+++ b/drivers/mfd/twl6030-irq.c
@@ -76,8 +76,8 @@ static int twl6030_interrupt_mapping[24] = {
USBOTG_INTR_OFFSET, /* Bit 18 ID */
USB_PRES_INTR_OFFSET, /* Bit 19 VBUS */
CHARGER_INTR_OFFSET, /* Bit 20 CHRG_CTRL */
- CHARGER_INTR_OFFSET, /* Bit 21 EXT_CHRG */
- CHARGER_INTR_OFFSET, /* Bit 22 INT_CHRG */
+ CHARGERFAULT_INTR_OFFSET, /* Bit 21 EXT_CHRG */
+ CHARGERFAULT_INTR_OFFSET, /* Bit 22 INT_CHRG */
RSV_INTR_OFFSET, /* Bit 23 Reserved */
};
/*----------------------------------------------------------------------*/
diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
index f85f0ed..4323d3a 100644
--- a/include/linux/i2c/twl.h
+++ b/include/linux/i2c/twl.h
@@ -91,6 +91,7 @@
#define BCI_INTR_OFFSET 2
#define MADC_INTR_OFFSET 3
#define USB_INTR_OFFSET 4
+#define CHARGERFAULT_INTR_OFFSET 5
#define BCI_PRES_INTR_OFFSET 9
#define USB_PRES_INTR_OFFSET 10
#define RTC_INTR_OFFSET 11
--
1.7.4.1
The twl6025 uses a different regulator for USB than the 6030 so select
the correct regulator name depending on the subclass of device.
Signed-off-by: Graeme Gregory <[email protected]>
---
drivers/usb/otg/twl6030-usb.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/otg/twl6030-usb.c b/drivers/usb/otg/twl6030-usb.c
index 6e920de..3d82419 100644
--- a/drivers/usb/otg/twl6030-usb.c
+++ b/drivers/usb/otg/twl6030-usb.c
@@ -190,6 +190,12 @@ static int twl6030_phy_suspend(struct otg_transceiver *x, int suspend)
static int twl6030_usb_ldo_init(struct twl6030_usb *twl)
{
+ char *regulator_name;
+
+ if (twl_features() & TWL6025_SUBCLASS)
+ regulator_name = "ldousb";
+ else
+ regulator_name = "vusb";
/* Set to OTG_REV 1.3 and turn on the ID_WAKEUP_COMP */
twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x1, TWL6030_BACKUP_REG);
@@ -200,7 +206,7 @@ static int twl6030_usb_ldo_init(struct twl6030_usb *twl)
/* Program MISC2 register and set bit VUSB_IN_VBAT */
twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x10, TWL6030_MISC2);
- twl->usb3v3 = regulator_get(twl->dev, "vusb");
+ twl->usb3v3 = regulator_get(twl->dev, regulator_name);
if (IS_ERR(twl->usb3v3))
return -ENODEV;
--
1.7.4.1
Adding support for the twl6025. Major difference in the twl6025 is the
group functionality has been removed from the chip so this affects how
regulators are enabled and disabled.
The names of the regulators also changed.
The DCDCs of the 6025 are software controllable as well.
Signed-off-by: Graeme Gregory <[email protected]>
---
drivers/regulator/twl-regulator.c | 447 ++++++++++++++++++++++++++++++++++---
1 files changed, 412 insertions(+), 35 deletions(-)
diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index 2a808c2..c08a0de 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -51,6 +51,8 @@ struct twlreg_info {
u16 min_mV;
u16 max_mV;
+ u8 flags;
+
/* used by regulator core */
struct regulator_desc desc;
};
@@ -70,6 +72,7 @@ struct twlreg_info {
#define VREG_TRANS 1
#define VREG_STATE 2
#define VREG_VOLTAGE 3
+#define VREG_VOLTAGE_DCDC 4
/* TWL6030 Misc register offsets */
#define VREG_BC_ALL 1
#define VREG_BC_REF 2
@@ -87,6 +90,17 @@ struct twlreg_info {
#define TWL6030_CFG_STATE_APP(v) (((v) & TWL6030_CFG_STATE_APP_MASK) >>\
TWL6030_CFG_STATE_APP_SHIFT)
+/* Flags for DCDC Voltage reading */
+#define DCDC_OFFSET_EN BIT(0)
+#define DCDC_EXTENDED_EN BIT(1)
+
+/* twl6025 SMPS EPROM values */
+#define TWL6030_SMPS_OFFSET 0xB0
+#define TWL6030_SMPS_MULT 0xB3
+#define SMPS_MULTOFFSET_SMPS4 BIT(0)
+#define SMPS_MULTOFFSET_VIO BIT(1)
+#define SMPS_MULTOFFSET_SMPS3 BIT(6)
+
static inline int
twlreg_read(struct twlreg_info *info, unsigned slave_subgp, unsigned offset)
{
@@ -144,11 +158,14 @@ static int twl6030reg_is_enabled(struct regulator_dev *rdev)
struct twlreg_info *info = rdev_get_drvdata(rdev);
int grp, val;
- grp = twlreg_read(info, TWL_MODULE_PM_RECEIVER, VREG_GRP);
- if (grp < 0)
- return grp;
+ if (!(twl_features() & TWL6025_SUBCLASS)) {
+ grp = twlreg_read(info, TWL_MODULE_PM_RECEIVER, VREG_GRP);
+ if (grp < 0)
+ return grp;
- grp &= P1_GRP_6030;
+ grp &= P1_GRP_6030;
+ } else
+ grp = 1;
val = twlreg_read(info, TWL_MODULE_PM_RECEIVER, VREG_STATE);
val = TWL6030_CFG_STATE_APP(val);
@@ -159,19 +176,22 @@ static int twl6030reg_is_enabled(struct regulator_dev *rdev)
static int twlreg_enable(struct regulator_dev *rdev)
{
struct twlreg_info *info = rdev_get_drvdata(rdev);
- int grp;
- int ret;
+ int grp = 0;
+ int ret = 0;
- grp = twlreg_read(info, TWL_MODULE_PM_RECEIVER, VREG_GRP);
- if (grp < 0)
- return grp;
+ if (!(twl_class_is_6030() && (twl_features() & TWL6025_SUBCLASS))) {
+ grp = twlreg_read(info, TWL_MODULE_PM_RECEIVER, VREG_GRP);
+ if (grp < 0)
+ return grp;
- if (twl_class_is_4030())
- grp |= P1_GRP_4030;
- else
- grp |= P1_GRP_6030;
+ if (twl_class_is_4030())
+ grp |= P1_GRP_4030;
+ else
+ grp |= P1_GRP_6030;
- ret = twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_GRP, grp);
+ ret = twlreg_write(info, TWL_MODULE_PM_RECEIVER,
+ VREG_GRP, grp);
+ }
if (!ret && twl_class_is_6030())
ret = twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_STATE,
@@ -186,29 +206,34 @@ static int twlreg_enable(struct regulator_dev *rdev)
static int twlreg_disable(struct regulator_dev *rdev)
{
struct twlreg_info *info = rdev_get_drvdata(rdev);
- int grp;
- int ret;
-
- grp = twlreg_read(info, TWL_MODULE_PM_RECEIVER, VREG_GRP);
- if (grp < 0)
- return grp;
-
- /* For 6030, set the off state for all grps enabled */
- if (twl_class_is_6030()) {
- ret = twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_STATE,
- (grp & (P1_GRP_6030 | P2_GRP_6030 | P3_GRP_6030)) <<
- TWL6030_CFG_STATE_GRP_SHIFT |
- TWL6030_CFG_STATE_OFF);
+ int grp = 0;
+ int ret = 0;
+
+ if (!(twl_class_is_6030() && (twl_features() & TWL6025_SUBCLASS))) {
+ grp = twlreg_read(info, TWL_MODULE_PM_RECEIVER, VREG_GRP);
+ if (grp < 0)
+ return grp;
+
+ /* For 6030, set the off state for all grps enabled */
+ if (twl_class_is_6030()) {
+ ret = twlreg_write(info, TWL_MODULE_PM_RECEIVER,
+ VREG_STATE,
+ (grp & (P1_GRP_6030 | P2_GRP_6030 |
+ P3_GRP_6030)) <<
+ TWL6030_CFG_STATE_GRP_SHIFT |
+ TWL6030_CFG_STATE_OFF);
if (ret)
return ret;
- }
+ }
- if (twl_class_is_4030())
- grp &= ~(P1_GRP_4030 | P2_GRP_4030 | P3_GRP_4030);
- else
- grp &= ~(P1_GRP_6030 | P2_GRP_6030 | P3_GRP_6030);
+ if (twl_class_is_4030())
+ grp &= ~(P1_GRP_4030 | P2_GRP_4030 | P3_GRP_4030);
+ else
+ grp &= ~(P1_GRP_6030 | P2_GRP_6030 | P3_GRP_6030);
- ret = twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_GRP, grp);
+ ret = twlreg_write(info, TWL_MODULE_PM_RECEIVER,
+ VREG_GRP, grp);
+ }
/* Next, associate cleared grp in state register */
if (!ret && twl_class_is_6030())
@@ -299,10 +324,11 @@ static int twl4030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
static int twl6030reg_set_mode(struct regulator_dev *rdev, unsigned mode)
{
struct twlreg_info *info = rdev_get_drvdata(rdev);
- int grp;
+ int grp = 0;
int val;
- grp = twlreg_read(info, TWL_MODULE_PM_RECEIVER, VREG_GRP);
+ if (!(twl_class_is_6030() && (twl_features() & TWL6025_SUBCLASS)))
+ grp = twlreg_read(info, TWL_MODULE_PM_RECEIVER, VREG_GRP);
if (grp < 0)
return grp;
@@ -594,6 +620,270 @@ static struct regulator_ops twl6030_fixed_resource = {
.get_status = twl6030reg_get_status,
};
+/*
+ * DCDC status and control
+ */
+
+static int twl6030dcdc_list_voltage(struct regulator_dev *rdev, unsigned index)
+{
+ struct twlreg_info *info = rdev_get_drvdata(rdev);
+
+ int voltage = 0;
+
+ switch (info->flags) {
+ case 0:
+ if (index == 0)
+ voltage = 0;
+ else if (index < 58)
+ voltage = (600000 + (12500 * (index - 1)));
+ else if (index == 58)
+ voltage = 1350 * 1000;
+ else if (index == 59)
+ voltage = 1500 * 1000;
+ else if (index == 60)
+ voltage = 1800 * 1000;
+ else if (index == 61)
+ voltage = 1900 * 1000;
+ else if (index == 62)
+ voltage = 2100 * 1000;
+ break;
+ case DCDC_OFFSET_EN:
+ if (index == 0)
+ voltage = 0;
+ else if (index < 58)
+ voltage = (700000 + (12500 * (index - 1)));
+ else if (index == 58)
+ voltage = 1350 * 1000;
+ else if (index == 59)
+ voltage = 1500 * 1000;
+ else if (index == 60)
+ voltage = 1800 * 1000;
+ else if (index == 61)
+ voltage = 1900 * 1000;
+ else if (index == 62)
+ voltage = 2100 * 1000;
+ break;
+ case DCDC_EXTENDED_EN:
+ if (index == 0)
+ voltage = 0;
+ else if (index < 58)
+ voltage = (1852000 + (38600 * (index - 1)));
+ else if (index == 58)
+ voltage = 2084 * 1000;
+ else if (index == 59)
+ voltage = 2315 * 1000;
+ else if (index == 60)
+ voltage = 2778 * 1000;
+ else if (index == 61)
+ voltage = 2932 * 1000;
+ else if (index == 62)
+ voltage = 3241 * 1000;
+ break;
+ case DCDC_OFFSET_EN|DCDC_EXTENDED_EN:
+ if (index == 0)
+ voltage = 0;
+ else if (index < 58)
+ voltage = (2161000 + (38600 * (index - 1)));
+ else if (index == 58)
+ voltage = 4167 * 1000;
+ else if (index == 59)
+ voltage = 2315 * 1000;
+ else if (index == 60)
+ voltage = 2778 * 1000;
+ else if (index == 61)
+ voltage = 2932 * 1000;
+ else if (index == 62)
+ voltage = 3241 * 1000;
+ break;
+ }
+
+ return voltage;
+}
+
+static int
+twl6030dcdc_set_voltage(struct regulator_dev *rdev, int min_uV, int max_uV,
+ unsigned int *selector)
+{
+ struct twlreg_info *info = rdev_get_drvdata(rdev);
+ int vsel = 0;
+
+ switch (info->flags) {
+ case 0:
+ if (min_uV == 0)
+ vsel = 0;
+ else if ((min_uV >= 600000) && (max_uV <= 1300000)) {
+ vsel = (min_uV - 600000) / 125;
+ if (vsel % 100)
+ vsel += 100;
+ vsel /= 100;
+ vsel++;
+ }
+ /* Values 1..57 for vsel are linear and can be calculated
+ * values 58..62 are non linear.
+ */
+ else if ((min_uV > 1900000) && (max_uV >= 2100000))
+ vsel = 62;
+ else if ((min_uV > 1800000) && (max_uV >= 1900000))
+ vsel = 61;
+ else if ((min_uV > 1500000) && (max_uV >= 1800000))
+ vsel = 60;
+ else if ((min_uV > 1350000) && (max_uV >= 1500000))
+ vsel = 59;
+ else if ((min_uV > 1300000) && (max_uV >= 1350000))
+ vsel = 58;
+ else
+ return -EINVAL;
+ break;
+ case DCDC_OFFSET_EN:
+ if (min_uV == 0)
+ vsel = 0;
+ else if ((min_uV >= 700000) && (max_uV <= 1420000)) {
+ vsel = (min_uV - 600000) / 125;
+ if (vsel % 100)
+ vsel += 100;
+ vsel /= 100;
+ vsel++;
+ }
+ /* Values 1..57 for vsel are linear and can be calculated
+ * values 58..62 are non linear.
+ */
+ else if ((min_uV > 1900000) && (max_uV >= 2100000))
+ vsel = 62;
+ else if ((min_uV > 1800000) && (max_uV >= 1900000))
+ vsel = 61;
+ else if ((min_uV > 1350000) && (max_uV >= 1800000))
+ vsel = 60;
+ else if ((min_uV > 1350000) && (max_uV >= 1500000))
+ vsel = 59;
+ else if ((min_uV > 1300000) && (max_uV >= 1350000))
+ vsel = 58;
+ else
+ return -EINVAL;
+ break;
+ case DCDC_EXTENDED_EN:
+ if (min_uV == 0)
+ vsel = 0;
+ else if ((min_uV >= 1852000) && (max_uV <= 4013600)) {
+ vsel = (min_uV - 1852000) / 386;
+ if (vsel % 100)
+ vsel += 100;
+ vsel /= 100;
+ vsel++;
+ }
+ break;
+ case DCDC_OFFSET_EN|DCDC_EXTENDED_EN:
+ if (min_uV == 0)
+ vsel = 0;
+ else if ((min_uV >= 2161000) && (max_uV <= 4321000)) {
+ vsel = (min_uV - 1852000) / 386;
+ if (vsel % 100)
+ vsel += 100;
+ vsel /= 100;
+ vsel++;
+ }
+ break;
+ }
+
+ *selector = vsel;
+
+ return twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_VOLTAGE_DCDC,
+ vsel);
+}
+
+static int twl6030dcdc_get_voltage(struct regulator_dev *rdev)
+{
+ struct twlreg_info *info = rdev_get_drvdata(rdev);
+ int vsel = twlreg_read(info, TWL_MODULE_PM_RECEIVER,
+ VREG_VOLTAGE_DCDC);
+ int voltage = 0;
+
+ if (vsel < 0)
+ return vsel;
+ switch (info->flags) {
+ case 0:
+ if (vsel == 0)
+ voltage = 0;
+ else if (vsel < 58)
+ voltage = (600000 + (12500 * (vsel - 1)));
+ else if (vsel == 58)
+ voltage = 1350 * 1000;
+ else if (vsel == 59)
+ voltage = 1500 * 1000;
+ else if (vsel == 60)
+ voltage = 1800 * 1000;
+ else if (vsel == 61)
+ voltage = 1900 * 1000;
+ else if (vsel == 62)
+ voltage = 2100 * 1000;
+ break;
+ case DCDC_OFFSET_EN:
+ if (vsel == 0)
+ voltage = 0;
+ else if (vsel < 58)
+ voltage = (700000 + (12500 * (vsel - 1)));
+ else if (vsel == 58)
+ voltage = 1350 * 1000;
+ else if (vsel == 59)
+ voltage = 1500 * 1000;
+ else if (vsel == 60)
+ voltage = 1800 * 1000;
+ else if (vsel == 61)
+ voltage = 1900 * 1000;
+ else if (vsel == 62)
+ voltage = 2100 * 1000;
+ break;
+ case DCDC_EXTENDED_EN:
+ if (vsel == 0)
+ voltage = 0;
+ else if (vsel < 58)
+ voltage = (1852000 + (38600 * (vsel - 1)));
+ else if (vsel == 58)
+ voltage = 2084 * 1000;
+ else if (vsel == 59)
+ voltage = 2315 * 1000;
+ else if (vsel == 60)
+ voltage = 2778 * 1000;
+ else if (vsel == 61)
+ voltage = 2932 * 1000;
+ else if (vsel == 62)
+ voltage = 3241 * 1000;
+ break;
+ case DCDC_EXTENDED_EN|DCDC_OFFSET_EN:
+ if (vsel == 0)
+ voltage = 0;
+ else if (vsel < 58)
+ voltage = (2161000 + (38600 * (vsel - 1)));
+ else if (vsel == 58)
+ voltage = 4167 * 1000;
+ else if (vsel == 59)
+ voltage = 2315 * 1000;
+ else if (vsel == 60)
+ voltage = 2778 * 1000;
+ else if (vsel == 61)
+ voltage = 2932 * 1000;
+ else if (vsel == 62)
+ voltage = 3241 * 1000;
+ break;
+ }
+
+ return voltage;
+}
+
+static struct regulator_ops twldcdc_ops = {
+ .list_voltage = twl6030dcdc_list_voltage,
+
+ .set_voltage = twl6030dcdc_set_voltage,
+ .get_voltage = twl6030dcdc_get_voltage,
+
+ .enable = twlreg_enable,
+ .disable = twlreg_disable,
+ .is_enabled = twl6030reg_is_enabled,
+
+ .set_mode = twl6030reg_set_mode,
+
+ .get_status = twl6030reg_get_status,
+};
+
/*----------------------------------------------------------------------*/
#define TWL4030_FIXED_LDO(label, offset, mVolts, num, turnon_delay, \
@@ -636,6 +926,22 @@ static struct regulator_ops twl6030_fixed_resource = {
}, \
}
+#define TWL6025_ADJUSTABLE_LDO(label, offset, min_mVolts, max_mVolts, num, \
+ remap_conf) { \
+ .base = offset, \
+ .id = num, \
+ .min_mV = min_mVolts, \
+ .max_mV = max_mVolts, \
+ .remap = remap_conf, \
+ .desc = { \
+ .name = #label, \
+ .id = TWL6025_REG_##label, \
+ .n_voltages = ((max_mVolts - min_mVolts)/100) + 1, \
+ .ops = &twl6030ldo_ops, \
+ .type = REGULATOR_VOLTAGE, \
+ .owner = THIS_MODULE, \
+ }, \
+ }
#define TWL_FIXED_LDO(label, offset, mVolts, num, turnon_delay, remap_conf, \
family, operations) { \
@@ -667,6 +973,23 @@ static struct regulator_ops twl6030_fixed_resource = {
}, \
}
+#define TWL6025_ADJUSTABLE_DCDC(label, offset, num, \
+ remap_conf) { \
+ .base = offset, \
+ .id = num, \
+ .min_mV = 600, \
+ .max_mV = 2100, \
+ .remap = remap_conf, \
+ .desc = { \
+ .name = #label, \
+ .id = TWL6025_REG_##label, \
+ .n_voltages = 63, \
+ .ops = &twldcdc_ops, \
+ .type = REGULATOR_VOLTAGE, \
+ .owner = THIS_MODULE, \
+ }, \
+ }
+
/*
* We list regulators here if systems need some level of
* software control over them after boot.
@@ -708,8 +1031,41 @@ static struct twlreg_info twl_regs[] = {
TWL6030_FIXED_LDO(VDAC, 0x64, 1800, 17, 0),
TWL6030_FIXED_LDO(VUSB, 0x70, 3300, 18, 0),
TWL6030_FIXED_RESOURCE(CLK32KG, 0x8C, 48, 0),
+
+ /* 6025 are renamed compared to 6030 versions */
+ TWL6025_ADJUSTABLE_LDO(LDO2, 0x54, 1000, 3300, 1, 0x21),
+ TWL6025_ADJUSTABLE_LDO(LDO4, 0x58, 1000, 3300, 2, 0x21),
+ TWL6025_ADJUSTABLE_LDO(LDO3, 0x5c, 1000, 3300, 3, 0x21),
+ TWL6025_ADJUSTABLE_LDO(LDO5, 0x68, 1000, 3300, 4, 0x21),
+ TWL6025_ADJUSTABLE_LDO(LDO1, 0x6c, 1000, 3300, 5, 0x21),
+ TWL6025_ADJUSTABLE_LDO(LDO7, 0x74, 1000, 3300, 7, 0x21),
+ TWL6025_ADJUSTABLE_LDO(LDO6, 0x60, 1000, 3300, 16, 0x21),
+ TWL6025_ADJUSTABLE_LDO(LDOLN, 0x64, 1000, 3300, 17, 0x21),
+ TWL6025_ADJUSTABLE_LDO(LDOUSB, 0x70, 1000, 3300, 18, 0x21),
+
+ TWL6025_ADJUSTABLE_DCDC(SMPS3, 0x34, 1, 0x21),
+ TWL6025_ADJUSTABLE_DCDC(SMPS4, 0x10, 2, 0x21),
+ TWL6025_ADJUSTABLE_DCDC(VIO, 0x16, 3, 0x21),
};
+static u8 twl_get_smps_offset(void)
+{
+ u8 value;
+
+ twl_i2c_read_u8(TWL_MODULE_PM_RECEIVER, &value,
+ TWL6030_SMPS_OFFSET);
+ return value;
+}
+
+static u8 twl_get_smps_mult(void)
+{
+ u8 value;
+
+ twl_i2c_read_u8(TWL_MODULE_PM_RECEIVER, &value,
+ TWL6030_SMPS_MULT);
+ return value;
+}
+
static int __devinit twlreg_probe(struct platform_device *pdev)
{
int i;
@@ -753,6 +1109,27 @@ static int __devinit twlreg_probe(struct platform_device *pdev)
break;
}
+ switch (pdev->id) {
+ case TWL6025_REG_SMPS3:
+ if (twl_get_smps_mult() & SMPS_MULTOFFSET_SMPS3)
+ info->flags |= DCDC_EXTENDED_EN;
+ if (twl_get_smps_offset() & SMPS_MULTOFFSET_SMPS3)
+ info->flags |= DCDC_OFFSET_EN;
+ break;
+ case TWL6025_REG_SMPS4:
+ if (twl_get_smps_mult() & SMPS_MULTOFFSET_SMPS4)
+ info->flags |= DCDC_EXTENDED_EN;
+ if (twl_get_smps_offset() & SMPS_MULTOFFSET_SMPS4)
+ info->flags |= DCDC_OFFSET_EN;
+ break;
+ case TWL6025_REG_VIO:
+ if (twl_get_smps_mult() & SMPS_MULTOFFSET_VIO)
+ info->flags |= DCDC_EXTENDED_EN;
+ if (twl_get_smps_offset() & SMPS_MULTOFFSET_VIO)
+ info->flags |= DCDC_OFFSET_EN;
+ break;
+ }
+
rdev = regulator_register(&info->desc, &pdev->dev, initdata, info);
if (IS_ERR(rdev)) {
dev_err(&pdev->dev, "can't register %s, %ld\n",
--
1.7.4.1
Phoenix Lite is based on the twl6030 family of PMICs. It has mostly the
same feature set of twl6030 but with small changes. The codec block has
also been removed. It also has a new charger block and new features in
its ADC block. VUSB handling also differs.
Signed-off-by: Graeme Gregory <[email protected]>
---
drivers/mfd/twl-core.c | 103 ++++++++++++++++++++++++++++++++++++++++++-----
include/linux/i2c/twl.h | 38 +++++++++++++++++-
2 files changed, 130 insertions(+), 11 deletions(-)
diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index 960b5be..6b9562a 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -198,6 +198,7 @@
#define TWL6030_BASEADD_GASGAUGE 0x00C0
#define TWL6030_BASEADD_PIH 0x00D0
#define TWL6030_BASEADD_CHARGER 0x00E0
+#define TWL6025_BASEADD_CHARGER 0x00DA
/* subchip/slave 2 0x4A - DFT */
#define TWL6030_BASEADD_DIEID 0x00C0
@@ -236,6 +237,17 @@ unsigned int twl_rev(void)
}
EXPORT_SYMBOL(twl_rev);
+/* Export a function so child drivers of this mfd can tell which subclass
+ * of the chip is being used. eg regulator needs to know that it is a
+ * 6025 variant.
+ */
+static unsigned int twl_feat;
+unsigned int twl_features(void)
+{
+ return twl_feat;
+}
+EXPORT_SYMBOL(twl_features);
+
/* Structure for each TWL4030/TWL6030 Slave */
struct twl_client {
struct i2c_client *client;
@@ -328,6 +340,7 @@ static struct twl_mapping twl6030_map[] = {
{ SUB_CHIP_ID0, TWL6030_BASEADD_RTC },
{ SUB_CHIP_ID0, TWL6030_BASEADD_MEM },
+ { SUB_CHIP_ID1, TWL6025_BASEADD_CHARGER },
};
/*----------------------------------------------------------------------*/
@@ -685,9 +698,8 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
}
if (twl_has_usb() && pdata->usb && twl_class_is_6030()) {
- static struct regulator_consumer_supply usb3v3 = {
- .supply = "vusb",
- };
+ static struct regulator_consumer_supply usb3v3;
+ int regulator;
if (twl_has_regulator()) {
/* this is a template that gets copied */
@@ -700,8 +712,15 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
| REGULATOR_CHANGE_STATUS,
};
- child = add_regulator_linked(TWL6030_REG_VUSB,
- &usb_fixed, &usb3v3, 1);
+ if (features & TWL6025_SUBCLASS) {
+ usb3v3.supply = "ldousb";
+ regulator = TWL6025_REG_LDOUSB;
+ } else {
+ usb3v3.supply = "vusb";
+ regulator = TWL6030_REG_VUSB;
+ }
+ child = add_regulator_linked(regulator, &usb_fixed,
+ &usb3v3, 1);
if (IS_ERR(child))
return PTR_ERR(child);
}
@@ -718,7 +737,15 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
/* we need to connect regulators to this transceiver */
if (twl_has_regulator() && child)
usb3v3.dev = child;
+ } else if (twl_has_regulator() && twl_class_is_6030()) {
+ if (features & TWL6025_SUBCLASS)
+ child = add_regulator(TWL6025_REG_LDOUSB,
+ pdata->ldousb);
+ else
+ child = add_regulator(TWL6030_REG_VUSB, pdata->vusb);
+ if (IS_ERR(child))
+ return PTR_ERR(child);
}
if (twl_has_watchdog() && twl_class_is_4030()) {
@@ -828,7 +855,8 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
}
/* twl6030 regulators */
- if (twl_has_regulator() && twl_class_is_6030()) {
+ if (twl_has_regulator() && twl_class_is_6030() &&
+ !(features & TWL6025_SUBCLASS)) {
child = add_regulator(TWL6030_REG_VMMC, pdata->vmmc);
if (IS_ERR(child))
return PTR_ERR(child);
@@ -841,10 +869,6 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
if (IS_ERR(child))
return PTR_ERR(child);
- child = add_regulator(TWL6030_REG_VANA, pdata->vana);
- if (IS_ERR(child))
- return PTR_ERR(child);
-
child = add_regulator(TWL6030_REG_VCXIO, pdata->vcxio);
if (IS_ERR(child))
return PTR_ERR(child);
@@ -870,6 +894,62 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
return PTR_ERR(child);
}
+ /* 6030 and 6025 share this regulator */
+ if (twl_has_regulator() && twl_class_is_6030()) {
+ child = add_regulator(TWL6030_REG_VANA, pdata->vana);
+ if (IS_ERR(child))
+ return PTR_ERR(child);
+ }
+
+ /* twl6025 regulators */
+ if (twl_has_regulator() && twl_class_is_6030() &&
+ (features & TWL6025_SUBCLASS)) {
+ child = add_regulator(TWL6025_REG_LDO5, pdata->ldo5);
+ if (IS_ERR(child))
+ return PTR_ERR(child);
+
+ child = add_regulator(TWL6025_REG_LDO1, pdata->ldo1);
+ if (IS_ERR(child))
+ return PTR_ERR(child);
+
+ child = add_regulator(TWL6025_REG_LDO7, pdata->ldo7);
+ if (IS_ERR(child))
+ return PTR_ERR(child);
+
+ child = add_regulator(TWL6025_REG_LDO6, pdata->ldo6);
+ if (IS_ERR(child))
+ return PTR_ERR(child);
+
+ child = add_regulator(TWL6025_REG_LDOLN, pdata->ldoln);
+ if (IS_ERR(child))
+ return PTR_ERR(child);
+
+ child = add_regulator(TWL6025_REG_LDO2, pdata->ldo2);
+ if (IS_ERR(child))
+ return PTR_ERR(child);
+
+ child = add_regulator(TWL6025_REG_LDO4, pdata->ldo4);
+ if (IS_ERR(child))
+ return PTR_ERR(child);
+
+ child = add_regulator(TWL6025_REG_LDO3, pdata->ldo3);
+ if (IS_ERR(child))
+ return PTR_ERR(child);
+
+ child = add_regulator(TWL6025_REG_SMPS3, pdata->smps3);
+ if (IS_ERR(child))
+ return PTR_ERR(child);
+
+ child = add_regulator(TWL6025_REG_SMPS4, pdata->smps4);
+ if (IS_ERR(child))
+ return PTR_ERR(child);
+
+ child = add_regulator(TWL6025_REG_VIO, pdata->vio6025);
+ if (IS_ERR(child))
+ return PTR_ERR(child);
+
+ }
+
if (twl_has_bci() && pdata->bci &&
!(features & (TPS_SUBSET | TWL5031))) {
child = add_child(3, "twl4030_bci",
@@ -1093,6 +1173,8 @@ twl_probe(struct i2c_client *client, const struct i2c_device_id *id)
twl_i2c_write_u8(TWL4030_MODULE_INTBR, temp, REG_GPPUPDCTR1);
}
+ twl_feat = id->driver_data;
+
status = add_children(pdata, id->driver_data);
fail:
if (status < 0)
@@ -1108,6 +1190,7 @@ static const struct i2c_device_id twl_ids[] = {
{ "tps65930", TPS_SUBSET }, /* fewer LDOs and DACs; no charger */
{ "tps65920", TPS_SUBSET }, /* fewer LDOs; no codec or charger */
{ "twl6030", TWL6030_CLASS }, /* "Phoenix power chip" */
+ { "twl6025", TWL6030_CLASS | TWL6025_SUBCLASS }, /* "Phoenix lite" */
{ /* end of list */ },
};
MODULE_DEVICE_TABLE(i2c, twl_ids);
diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
index 0c0d1ae..f85f0ed 100644
--- a/include/linux/i2c/twl.h
+++ b/include/linux/i2c/twl.h
@@ -165,6 +165,11 @@ static inline int twl_class_is_ ##class(void) \
TWL_CLASS_IS(4030, TWL4030_CLASS_ID)
TWL_CLASS_IS(6030, TWL6030_CLASS_ID)
+#define TWL6025_SUBCLASS BIT(4) /* TWL6025 has changed registers */
+
+/* So we can recover the features in other parts of twl stack */
+unsigned int twl_features(void);
+
/*
* Read and write single 8-bit registers
*/
@@ -698,7 +703,21 @@ struct twl4030_platform_data {
struct regulator_init_data *vana;
struct regulator_init_data *vcxio;
struct regulator_init_data *vusb;
- struct regulator_init_data *clk32kg;
+ struct regulator_init_data *clk32kg;
+ /* TWL6025 LDO regulators */
+ struct regulator_init_data *ldo1;
+ struct regulator_init_data *ldo2;
+ struct regulator_init_data *ldo3;
+ struct regulator_init_data *ldo4;
+ struct regulator_init_data *ldo5;
+ struct regulator_init_data *ldo6;
+ struct regulator_init_data *ldo7;
+ struct regulator_init_data *ldoln;
+ struct regulator_init_data *ldousb;
+ /* TWL6025 DCDC regulators */
+ struct regulator_init_data *smps3;
+ struct regulator_init_data *smps4;
+ struct regulator_init_data *vio6025;
};
/*----------------------------------------------------------------------*/
@@ -780,4 +799,21 @@ static inline int twl4030charger_usb_en(int enable) { return 0; }
#define TWL6030_REG_VRTC 47
#define TWL6030_REG_CLK32KG 48
+/* LDOs on 6025 have different names */
+#define TWL6025_REG_LDO2 49
+#define TWL6025_REG_LDO4 50
+#define TWL6025_REG_LDO3 51
+#define TWL6025_REG_LDO5 52
+#define TWL6025_REG_LDO1 53
+#define TWL6025_REG_LDO7 54
+#define TWL6025_REG_LDO6 55
+#define TWL6025_REG_LDOLN 56
+#define TWL6025_REG_LDOUSB 57
+
+/* 6025 DCDC supplies */
+#define TWL6025_REG_SMPS3 58
+#define TWL6025_REG_SMPS4 59
+#define TWL6025_REG_VIO 60
+
+
#endif /* End of __TWL4030_H */
--
1.7.4.1
On Wed, Apr 27, 2011 at 10:39:48AM +0100, Graeme Gregory wrote:
> Phoenix Lite is based on the twl6030 family of PMICs. It has mostly the
> same feature set of twl6030 but with small changes. The codec block has
> also been removed. It also has a new charger block and new features in
> its ADC block. VUSB handling also differs.
>
> Signed-off-by: Graeme Gregory <[email protected]>
> ---
> drivers/mfd/twl-core.c | 103 ++++++++++++++++++++++++++++++++++++++++++-----
> include/linux/i2c/twl.h | 38 +++++++++++++++++-
> 2 files changed, 130 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> index 960b5be..6b9562a 100644
> --- a/drivers/mfd/twl-core.c
> +++ b/drivers/mfd/twl-core.c
> @@ -198,6 +198,7 @@
> #define TWL6030_BASEADD_GASGAUGE 0x00C0
> #define TWL6030_BASEADD_PIH 0x00D0
> #define TWL6030_BASEADD_CHARGER 0x00E0
> +#define TWL6025_BASEADD_CHARGER 0x00DA
>
> /* subchip/slave 2 0x4A - DFT */
> #define TWL6030_BASEADD_DIEID 0x00C0
> @@ -236,6 +237,17 @@ unsigned int twl_rev(void)
> }
> EXPORT_SYMBOL(twl_rev);
>
> +/* Export a function so child drivers of this mfd can tell which subclass
> + * of the chip is being used. eg regulator needs to know that it is a
> + * 6025 variant.
> + */
> +static unsigned int twl_feat;
> +unsigned int twl_features(void)
> +{
> + return twl_feat;
> +}
> +EXPORT_SYMBOL(twl_features);
Why do you need other parts of the stack to access features ? It would
be better to use features to initialize proper fields in the TWL
structure and use that for runtime checking.
> @@ -328,6 +340,7 @@ static struct twl_mapping twl6030_map[] = {
>
> { SUB_CHIP_ID0, TWL6030_BASEADD_RTC },
> { SUB_CHIP_ID0, TWL6030_BASEADD_MEM },
> + { SUB_CHIP_ID1, TWL6025_BASEADD_CHARGER },
> };
>
> /*----------------------------------------------------------------------*/
> @@ -685,9 +698,8 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
> }
> if (twl_has_usb() && pdata->usb && twl_class_is_6030()) {
>
> - static struct regulator_consumer_supply usb3v3 = {
> - .supply = "vusb",
> - };
> + static struct regulator_consumer_supply usb3v3;
> + int regulator;
>
> if (twl_has_regulator()) {
> /* this is a template that gets copied */
> @@ -700,8 +712,15 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
> | REGULATOR_CHANGE_STATUS,
> };
>
> - child = add_regulator_linked(TWL6030_REG_VUSB,
> - &usb_fixed, &usb3v3, 1);
> + if (features & TWL6025_SUBCLASS) {
> + usb3v3.supply = "ldousb";
> + regulator = TWL6025_REG_LDOUSB;
> + } else {
> + usb3v3.supply = "vusb";
> + regulator = TWL6030_REG_VUSB;
> + }
that's just a name. Why don't you use the same name for both variants ?
> @@ -718,7 +737,15 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
> /* we need to connect regulators to this transceiver */
> if (twl_has_regulator() && child)
> usb3v3.dev = child;
> + } else if (twl_has_regulator() && twl_class_is_6030()) {
> + if (features & TWL6025_SUBCLASS)
> + child = add_regulator(TWL6025_REG_LDOUSB,
> + pdata->ldousb);
> + else
> + child = add_regulator(TWL6030_REG_VUSB, pdata->vusb);
see, then you need to add more fields to the platform_data structure
just because of a string.
> @@ -1093,6 +1173,8 @@ twl_probe(struct i2c_client *client, const struct i2c_device_id *id)
> twl_i2c_write_u8(TWL4030_MODULE_INTBR, temp, REG_GPPUPDCTR1);
> }
>
> + twl_feat = id->driver_data;
NACK. Avoid globals as much as possible.
> diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
> index 0c0d1ae..f85f0ed 100644
> --- a/include/linux/i2c/twl.h
> +++ b/include/linux/i2c/twl.h
> @@ -698,7 +703,21 @@ struct twl4030_platform_data {
> struct regulator_init_data *vana;
> struct regulator_init_data *vcxio;
> struct regulator_init_data *vusb;
> - struct regulator_init_data *clk32kg;
> + struct regulator_init_data *clk32kg;
here you converted TABs into spaces. Fix it.
> + /* TWL6025 LDO regulators */
> + struct regulator_init_data *ldo1;
> + struct regulator_init_data *ldo2;
> + struct regulator_init_data *ldo3;
> + struct regulator_init_data *ldo4;
> + struct regulator_init_data *ldo5;
> + struct regulator_init_data *ldo6;
> + struct regulator_init_data *ldo7;
> + struct regulator_init_data *ldoln;
> + struct regulator_init_data *ldousb;
> + /* TWL6025 DCDC regulators */
> + struct regulator_init_data *smps3;
> + struct regulator_init_data *smps4;
> + struct regulator_init_data *vio6025;
this is just becoming really really ugly. You need a more clever way of
handling this. Maybe passing an array of regulators and the array size
instead of continuously adding fields to this structure.
> @@ -780,4 +799,21 @@ static inline int twl4030charger_usb_en(int enable) { return 0; }
> #define TWL6030_REG_VRTC 47
> #define TWL6030_REG_CLK32KG 48
>
> +/* LDOs on 6025 have different names */
> +#define TWL6025_REG_LDO2 49
> +#define TWL6025_REG_LDO4 50
> +#define TWL6025_REG_LDO3 51
> +#define TWL6025_REG_LDO5 52
> +#define TWL6025_REG_LDO1 53
> +#define TWL6025_REG_LDO7 54
> +#define TWL6025_REG_LDO6 55
> +#define TWL6025_REG_LDOLN 56
> +#define TWL6025_REG_LDOUSB 57
> +
> +/* 6025 DCDC supplies */
> +#define TWL6025_REG_SMPS3 58
> +#define TWL6025_REG_SMPS4 59
> +#define TWL6025_REG_VIO 60
> +
> +
one blank line only.
--
balbi
Hi,
On Wed, Apr 27, 2011 at 10:39:50AM +0100, Graeme Gregory wrote:
> Adding support for the twl6025. Major difference in the twl6025 is the
> group functionality has been removed from the chip so this affects how
> regulators are enabled and disabled.
>
> The names of the regulators also changed.
>
> The DCDCs of the 6025 are software controllable as well.
>
> Signed-off-by: Graeme Gregory <[email protected]>
> ---
> drivers/regulator/twl-regulator.c | 447 ++++++++++++++++++++++++++++++++++---
> 1 files changed, 412 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
> index 2a808c2..c08a0de 100644
> --- a/drivers/regulator/twl-regulator.c
> +++ b/drivers/regulator/twl-regulator.c
> @@ -144,11 +158,14 @@ static int twl6030reg_is_enabled(struct regulator_dev *rdev)
> struct twlreg_info *info = rdev_get_drvdata(rdev);
> int grp, val;
>
> - grp = twlreg_read(info, TWL_MODULE_PM_RECEIVER, VREG_GRP);
> - if (grp < 0)
> - return grp;
> + if (!(twl_features() & TWL6025_SUBCLASS)) {
instead, why don't you pass some flag as platform_data to this driver,
which gets used to initialize a fiel in struct twlreg_info, then you use
that to do the checking.
> + grp = twlreg_read(info, TWL_MODULE_PM_RECEIVER, VREG_GRP);
> + if (grp < 0)
> + return grp;
>
> - grp &= P1_GRP_6030;
> + grp &= P1_GRP_6030;
> + } else
> + grp = 1;
if one branch has {} add it to both.
--
balbi
Hi,
On Wed, Apr 27, 2011 at 10:39:51AM +0100, Graeme Gregory wrote:
> The twl6025 uses a different regulator for USB than the 6030 so select
> the correct regulator name depending on the subclass of device.
>
> Signed-off-by: Graeme Gregory <[email protected]>
I don't see the point of this patch. It's just a string. Use the same
name and add a comment saying that on datasheet/TRM/documentation the
name LDO is actually referred to as LDOUSB. It's the same functionality
anyway.
--
balbi
On Wed, Apr 27, 2011 at 10:39:50AM +0100, Graeme Gregory wrote:
> + switch (info->flags) {
> + case 0:
> + if (index == 0)
> + voltage = 0;
> + else if (index < 58)
> + voltage = (600000 + (12500 * (index - 1)));
> + else if (index == 58)
> + voltage = 1350 * 1000;
> + else if (index == 59)
> + voltage = 1500 * 1000;
> + else if (index == 60)
> + voltage = 1800 * 1000;
> + else if (index == 61)
> + voltage = 1900 * 1000;
> + else if (index == 62)
> + voltage = 2100 * 1000;
This reads like a switch statement with a default: case to me.
> +static int twl6030dcdc_get_voltage(struct regulator_dev *rdev)
> +{
> + struct twlreg_info *info = rdev_get_drvdata(rdev);
> + int vsel = twlreg_read(info, TWL_MODULE_PM_RECEIVER,
> + VREG_VOLTAGE_DCDC);
> + int voltage = 0;
> +
> + if (vsel < 0)
> + return vsel;
> + switch (info->flags) {
This should just call list_voltage() to do the mapping into a voltage or
better yet should be converted into a get_voltage_sel() which will mean
that the core can call list_voltage() for you. Either way you save
duplicating the code to map from selector to voltage.
On Wed, 2011-04-27 at 13:45 +0300, Felipe Balbi wrote:
> Hi,
>
> On Wed, Apr 27, 2011 at 10:39:51AM +0100, Graeme Gregory wrote:
> > The twl6025 uses a different regulator for USB than the 6030 so select
> > the correct regulator name depending on the subclass of device.
> >
> > Signed-off-by: Graeme Gregory <[email protected]>
>
> I don't see the point of this patch. It's just a string. Use the same
> name and add a comment saying that on datasheet/TRM/documentation the
> name LDO is actually referred to as LDOUSB. It's the same functionality
> anyway.
>
I think for the avoidance of any doubt, it's probably best to use the
TWL6025 string name here as it will importantly match the TWL6025 TRM
and any schematics using the TWL6025. Getting this wrong during TWL6025
board integration has the potential for hardware damage.
Liam
On Wed, 2011-04-27 at 13:40 +0300, Felipe Balbi wrote:
> On Wed, Apr 27, 2011 at 10:39:48AM +0100, Graeme Gregory wrote:
> > + /* TWL6025 LDO regulators */
> > + struct regulator_init_data *ldo1;
> > + struct regulator_init_data *ldo2;
> > + struct regulator_init_data *ldo3;
> > + struct regulator_init_data *ldo4;
> > + struct regulator_init_data *ldo5;
> > + struct regulator_init_data *ldo6;
> > + struct regulator_init_data *ldo7;
> > + struct regulator_init_data *ldoln;
> > + struct regulator_init_data *ldousb;
> > + /* TWL6025 DCDC regulators */
> > + struct regulator_init_data *smps3;
> > + struct regulator_init_data *smps4;
> > + struct regulator_init_data *vio6025;
>
> this is just becoming really really ugly. You need a more clever way of
> handling this. Maybe passing an array of regulators and the array size
> instead of continuously adding fields to this structure.
>
Ok, I agree that optimising the platform data here is desirable, but I
think we will have to stick with this atm as the twl driver has some
rather annoying limitations that make optimising things like this a pita
atm.
I guess we should look at fixing the twl driver within TI in order to
make it more adaptable (i.e. support future twl ICs) and also a non
singleton device.
Liam
Hi,
On Sun, May 08, 2011 at 04:08:37PM +0100, Liam Girdwood wrote:
> On Wed, 2011-04-27 at 13:45 +0300, Felipe Balbi wrote:
> > Hi,
> >
> > On Wed, Apr 27, 2011 at 10:39:51AM +0100, Graeme Gregory wrote:
> > > The twl6025 uses a different regulator for USB than the 6030 so select
> > > the correct regulator name depending on the subclass of device.
> > >
> > > Signed-off-by: Graeme Gregory <[email protected]>
> >
> > I don't see the point of this patch. It's just a string. Use the same
> > name and add a comment saying that on datasheet/TRM/documentation the
> > name LDO is actually referred to as LDOUSB. It's the same functionality
> > anyway.
> >
>
> I think for the avoidance of any doubt, it's probably best to use the
> TWL6025 string name here as it will importantly match the TWL6025 TRM
> and any schematics using the TWL6025. Getting this wrong during TWL6025
> board integration has the potential for hardware damage.
I would rather have something that doesn't depend on a correct string
and matches based on the device pointer instead. I agree that having the
correct string makes it easier to reference schematics/trm and the like,
but making the SW depend on the correct spelling of a simple string, is
too much for me :-(
Specially when getting it wrong "has the potential for hardware damage"
:-)
--
balbi
On Mon, 2011-05-09 at 12:03 +0300, Felipe Balbi wrote:
> Hi,
>
> On Sun, May 08, 2011 at 04:08:37PM +0100, Liam Girdwood wrote:
> > On Wed, 2011-04-27 at 13:45 +0300, Felipe Balbi wrote:
> > > Hi,
> > >
> > > On Wed, Apr 27, 2011 at 10:39:51AM +0100, Graeme Gregory wrote:
> > > > The twl6025 uses a different regulator for USB than the 6030 so select
> > > > the correct regulator name depending on the subclass of device.
> > > >
> > > > Signed-off-by: Graeme Gregory <[email protected]>
> > >
> > > I don't see the point of this patch. It's just a string. Use the same
> > > name and add a comment saying that on datasheet/TRM/documentation the
> > > name LDO is actually referred to as LDOUSB. It's the same functionality
> > > anyway.
> > >
> >
> > I think for the avoidance of any doubt, it's probably best to use the
> > TWL6025 string name here as it will importantly match the TWL6025 TRM
> > and any schematics using the TWL6025. Getting this wrong during TWL6025
> > board integration has the potential for hardware damage.
>
> I would rather have something that doesn't depend on a correct string
> and matches based on the device pointer instead. I agree that having the
> correct string makes it easier to reference schematics/trm and the like,
> but making the SW depend on the correct spelling of a simple string, is
> too much for me :-(
>
> Specially when getting it wrong "has the potential for hardware damage"
> :-)
>
I think it's the lesser evil though, especially for device integrators.
They will just match the regulator name from the schematics together
with the TRM name when creating their regulator constraints and having
different names here will definitely cause confusion.
Liam
Hi,
On Mon, May 09, 2011 at 12:43:49PM +0100, Liam Girdwood wrote:
> On Mon, 2011-05-09 at 12:03 +0300, Felipe Balbi wrote:
> > Hi,
> >
> > On Sun, May 08, 2011 at 04:08:37PM +0100, Liam Girdwood wrote:
> > > On Wed, 2011-04-27 at 13:45 +0300, Felipe Balbi wrote:
> > > > Hi,
> > > >
> > > > On Wed, Apr 27, 2011 at 10:39:51AM +0100, Graeme Gregory wrote:
> > > > > The twl6025 uses a different regulator for USB than the 6030 so select
> > > > > the correct regulator name depending on the subclass of device.
> > > > >
> > > > > Signed-off-by: Graeme Gregory <[email protected]>
> > > >
> > > > I don't see the point of this patch. It's just a string. Use the same
> > > > name and add a comment saying that on datasheet/TRM/documentation the
> > > > name LDO is actually referred to as LDOUSB. It's the same functionality
> > > > anyway.
> > > >
> > >
> > > I think for the avoidance of any doubt, it's probably best to use the
> > > TWL6025 string name here as it will importantly match the TWL6025 TRM
> > > and any schematics using the TWL6025. Getting this wrong during TWL6025
> > > board integration has the potential for hardware damage.
> >
> > I would rather have something that doesn't depend on a correct string
> > and matches based on the device pointer instead. I agree that having the
> > correct string makes it easier to reference schematics/trm and the like,
> > but making the SW depend on the correct spelling of a simple string, is
> > too much for me :-(
> >
> > Specially when getting it wrong "has the potential for hardware damage"
> > :-)
> >
>
> I think it's the lesser evil though, especially for device integrators.
> They will just match the regulator name from the schematics together
> with the TRM name when creating their regulator constraints and having
> different names here will definitely cause confusion.
Any chance Device Tree could be used to pass that data to kernel
instead, together with regulator names and all needed data for each one
of them ?
--
balbi
On Mon, May 09, 2011 at 03:16:03PM +0300, Felipe Balbi wrote:
> On Mon, May 09, 2011 at 12:43:49PM +0100, Liam Girdwood wrote:
> > I think it's the lesser evil though, especially for device integrators.
> > They will just match the regulator name from the schematics together
> > with the TRM name when creating their regulator constraints and having
> > different names here will definitely cause confusion.
> Any chance Device Tree could be used to pass that data to kernel
> instead, together with regulator names and all needed data for each one
> of them ?
I'm pretty sure that as soon as we have viable device tree support for
relevant platforms in mainline we'll have regulator support, though I'm
not sure that this will help too much with the naming as you'll still
have to figure out what the consumer requests. We shouldn't be passing
in the consumer supply names from device tree (at least not a board
specific one) as this breaks the model that the supply names correspond
to the chip pins.
Hi,
On Mon, May 09, 2011 at 02:29:41PM +0200, Mark Brown wrote:
> On Mon, May 09, 2011 at 03:16:03PM +0300, Felipe Balbi wrote:
> > On Mon, May 09, 2011 at 12:43:49PM +0100, Liam Girdwood wrote:
>
> > > I think it's the lesser evil though, especially for device integrators.
> > > They will just match the regulator name from the schematics together
> > > with the TRM name when creating their regulator constraints and having
> > > different names here will definitely cause confusion.
>
> > Any chance Device Tree could be used to pass that data to kernel
> > instead, together with regulator names and all needed data for each one
> > of them ?
>
> I'm pretty sure that as soon as we have viable device tree support for
> relevant platforms in mainline we'll have regulator support, though I'm
> not sure that this will help too much with the naming as you'll still
> have to figure out what the consumer requests. We shouldn't be passing
> in the consumer supply names from device tree (at least not a board
> specific one) as this breaks the model that the supply names correspond
> to the chip pins.
The thing is. We already had problems in the past with the Clock FW
because drivers were passing clock names on platform_data and I really
want to avoid the same on regulators. We need something clever.
We pass in the data of how regulators are wired at the board level and
drivers would regulator_get(my_dev->dev, "whatever fancy name I want as
it doesn't matter at all");
If a driver has >1 regulator, you can distinguish them by which
functionality they provide, no matter the name, they will be connected
to a particular ball on the IC package which has a certain definition
on the TRM. In other words, we should match on the functionality they
provide, not on the name.
Let's try to thing some 5 - 6 years ahead. With the current trend, we
will be working on support for the PMIC on OMAP10, imagine if each one
of those revisions decide to change the name of the regulator, because
this new HW engineer working on the PMIC design doesn't like the old
name. We will have X regulator pointers and X regulator name pointers in
our platform_data. It will be really cumbersome and prone to errors if
people continue on copy&pasting old code to "generate" a new driver.
What I was expecting to see, was a mechanism where we define how those
things are wired (it doesn't matter if the data uses DeviceTree or what)
at the HW level and we ask the framework to give us the regulator
connected to device X which provides a certain functionality. Just like
clkdev has managed to get close to. Currently drivers will:
clk_get(dev, "ick");
clk_get(dev, "fck");
etc...
and the name really doesn't matter. Clkdev still isn't perfect as it
uses device names, then when we want/need to change the device/driver
name (due to some re-factoring for example) we end up breaking things.
IMHO, struct device should point to its dependencies, be it a clock, be
it a regulator.
--
balbi
On Mon, 2011-05-09 at 15:16 +0300, Felipe Balbi wrote:
> Hi,
>
> On Mon, May 09, 2011 at 12:43:49PM +0100, Liam Girdwood wrote:
> > On Mon, 2011-05-09 at 12:03 +0300, Felipe Balbi wrote:
> > > Hi,
> > >
> > > On Sun, May 08, 2011 at 04:08:37PM +0100, Liam Girdwood wrote:
> > > > On Wed, 2011-04-27 at 13:45 +0300, Felipe Balbi wrote:
> > > > > Hi,
> > > > >
> > > > > On Wed, Apr 27, 2011 at 10:39:51AM +0100, Graeme Gregory wrote:
> > > > > > The twl6025 uses a different regulator for USB than the 6030 so select
> > > > > > the correct regulator name depending on the subclass of device.
> > > > > >
> > > > > > Signed-off-by: Graeme Gregory <[email protected]>
> > > > >
> > > > > I don't see the point of this patch. It's just a string. Use the same
> > > > > name and add a comment saying that on datasheet/TRM/documentation the
> > > > > name LDO is actually referred to as LDOUSB. It's the same functionality
> > > > > anyway.
> > > > >
> > > >
> > > > I think for the avoidance of any doubt, it's probably best to use the
> > > > TWL6025 string name here as it will importantly match the TWL6025 TRM
> > > > and any schematics using the TWL6025. Getting this wrong during TWL6025
> > > > board integration has the potential for hardware damage.
> > >
> > > I would rather have something that doesn't depend on a correct string
> > > and matches based on the device pointer instead. I agree that having the
> > > correct string makes it easier to reference schematics/trm and the like,
> > > but making the SW depend on the correct spelling of a simple string, is
> > > too much for me :-(
> > >
> > > Specially when getting it wrong "has the potential for hardware damage"
> > > :-)
> > >
> >
> > I think it's the lesser evil though, especially for device integrators.
> > They will just match the regulator name from the schematics together
> > with the TRM name when creating their regulator constraints and having
> > different names here will definitely cause confusion.
>
> Any chance Device Tree could be used to pass that data to kernel
> instead, together with regulator names and all needed data for each one
> of them ?
>
Yes, I think this should be the long term aim, but atm we will have to
stick with current implementation until regulator has device tree
support.
Liam
On Mon, May 09, 2011 at 04:07:07PM +0300, Felipe Balbi wrote:
> The thing is. We already had problems in the past with the Clock FW
> because drivers were passing clock names on platform_data and I really
> want to avoid the same on regulators. We need something clever.
Right, which is why you should *never* do that. Any time I see anyone
requesting a regulator based on anything other than a fixed string in
the driver and the struct device for the consumer I push back very hard,
and so should everyone else.
> We pass in the data of how regulators are wired at the board level and
> drivers would regulator_get(my_dev->dev, "whatever fancy name I want as
> it doesn't matter at all");
No, not at all. Consumer drivers should be hard coding the strings they
request and the strings used should correspond to the pin names used on
the device.
> If a driver has >1 regulator, you can distinguish them by which
> functionality they provide, no matter the name, they will be connected
> to a particular ball on the IC package which has a certain definition
Having more than one supply is the common case for devices; relatively
few devices have a single supply.
> on the TRM. In other words, we should match on the functionality they
> provide, not on the name.
As Liam said this will just make the situation worse. Users will have
to figure out what the names the driver authors assigned to the various
device supplies map onto in the physical system via an additional layer
of indirection which will at best be written down in comments in the
driver.
> Let's try to thing some 5 - 6 years ahead. With the current trend, we
> will be working on support for the PMIC on OMAP10, imagine if each one
> of those revisions decide to change the name of the regulator, because
> this new HW engineer working on the PMIC design doesn't like the old
> name. We will have X regulator pointers and X regulator name pointers in
> our platform_data. It will be really cumbersome and prone to errors if
> people continue on copy&pasting old code to "generate" a new driver.
Here you're apparently talking about something different - you're
talking about how we pass in the regulator init data for the regulators
supplied by the chip. The patch being discussed changes the consumer
side for USB, not the regulator driver side. The naming on the driver
side is a particular problem for TWL devices since unlike most PMICs the
regulators aren't simply numbered but are instead assigned individual
names so you can't just use a big array indexed by number.
> What I was expecting to see, was a mechanism where we define how those
> things are wired (it doesn't matter if the data uses DeviceTree or what)
> at the HW level and we ask the framework to give us the regulator
> connected to device X which provides a certain functionality. Just like
> clkdev has managed to get close to. Currently drivers will:
This is preceisely what is happening. A well written consumer driver
asks the regulator core for the regulator supplying a given supply, with
the supplies named in the same way as they are named in the datasheet.
You appear to be arging strongly for us to adopt the model which is
already in use.
On Mon, May 09, 2011 at 03:35:43PM +0200, Mark Brown wrote:
> On Mon, May 09, 2011 at 04:07:07PM +0300, Felipe Balbi wrote:
>
> > The thing is. We already had problems in the past with the Clock FW
> > because drivers were passing clock names on platform_data and I really
> > want to avoid the same on regulators. We need something clever.
>
> Right, which is why you should *never* do that. Any time I see anyone
> requesting a regulator based on anything other than a fixed string in
> the driver and the struct device for the consumer I push back very hard,
> and so should everyone else.
ok, so let's go back to the original patch:
> @@ -190,6 +190,12 @@ static int twl6030_phy_suspend(struct otg_transceiver *x, int suspend)
>
> static int twl6030_usb_ldo_init(struct twl6030_usb *twl)
> {
> + char *regulator_name;
> +
> + if (twl_features() & TWL6025_SUBCLASS)
> + regulator_name = "ldousb";
> + else
> + regulator_name = "vusb";
>
> /* Set to OTG_REV 1.3 and turn on the ID_WAKEUP_COMP */
> twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x1, TWL6030_BACKUP_REG);
> @@ -200,7 +206,7 @@ static int twl6030_usb_ldo_init(struct twl6030_usb *twl)
> /* Program MISC2 register and set bit VUSB_IN_VBAT */
> twl6030_writeb(twl, TWL6030_MODULE_ID0 , 0x10, TWL6030_MISC2);
>
> - twl->usb3v3 = regulator_get(twl->dev, "vusb");
> + twl->usb3v3 = regulator_get(twl->dev, regulator_name);
> if (IS_ERR(twl->usb3v3))
> return -ENODEV;
then, imagine 5 years from now how that branch will look. How long do
you think it'll take until someone decides to pass that name via
platform_data to avoid growing that conditional ?
> > We pass in the data of how regulators are wired at the board level and
> > drivers would regulator_get(my_dev->dev, "whatever fancy name I want as
> > it doesn't matter at all");
>
> No, not at all. Consumer drivers should be hard coding the strings they
> request and the strings used should correspond to the pin names used on
> the device.
but that's the thing. Why do they need to depend on that string ?
> > on the TRM. In other words, we should match on the functionality they
> > provide, not on the name.
>
> As Liam said this will just make the situation worse. Users will have
> to figure out what the names the driver authors assigned to the various
> device supplies map onto in the physical system via an additional layer
> of indirection which will at best be written down in comments in the
> driver.
My point is that the "name" shouldn't matter. Instead of matching a
"name" match a "reference". Have something earlier in the boot assing
regulators to devices, or something like that, so that drivers don't
need to care about "names".
> > Let's try to thing some 5 - 6 years ahead. With the current trend, we
> > will be working on support for the PMIC on OMAP10, imagine if each one
> > of those revisions decide to change the name of the regulator, because
> > this new HW engineer working on the PMIC design doesn't like the old
> > name. We will have X regulator pointers and X regulator name pointers in
> > our platform_data. It will be really cumbersome and prone to errors if
> > people continue on copy&pasting old code to "generate" a new driver.
>
> Here you're apparently talking about something different - you're
> talking about how we pass in the regulator init data for the regulators
> supplied by the chip. The patch being discussed changes the consumer
> side for USB, not the regulator driver side. The naming on the driver
see above, how long do you think it'll take for someone to try and pass
that name via pdata ? On the next name change, this is already likely to
happen to prevent that conditional from growing.
> side is a particular problem for TWL devices since unlike most PMICs the
> regulators aren't simply numbered but are instead assigned individual
> names so you can't just use a big array indexed by number.
why not ? The name shouldn't matter. In anycase, if you look into
/sys/class/regulator/ all directories are regulator.<index>:
dev_set_name(&rdev->dev, "regulator.%d",
atomic_inc_return(®ulator_no) - 1);
> > What I was expecting to see, was a mechanism where we define how those
> > things are wired (it doesn't matter if the data uses DeviceTree or what)
> > at the HW level and we ask the framework to give us the regulator
> > connected to device X which provides a certain functionality. Just like
> > clkdev has managed to get close to. Currently drivers will:
>
> This is preceisely what is happening. A well written consumer driver
> asks the regulator core for the regulator supplying a given supply, with
> the supplies named in the same way as they are named in the datasheet.
>
> You appear to be arging strongly for us to adopt the model which is
> already in use.
I guess I hasn't been clear enough then, my whole point is that drivers
shouldn't depend on strings at all in order to fetch the correct
regulator. How many:
if (xxx_features() & XXX_CLASS_IS_YYYY)
regulator_name = "my_first_name";
else
regulator_name = "my_second_name";
will have to pop in drivers until we figure that it won't scale ?
--
balbi
On Mon, May 09, 2011 at 04:51:26PM +0300, Felipe Balbi wrote:
> On Mon, May 09, 2011 at 03:35:43PM +0200, Mark Brown wrote:
> > - twl->usb3v3 = regulator_get(twl->dev, "vusb");
> > + twl->usb3v3 = regulator_get(twl->dev, regulator_name);
> > if (IS_ERR(twl->usb3v3))
> > return -ENODEV;
> then, imagine 5 years from now how that branch will look. How long do
> you think it'll take until someone decides to pass that name via
> platform_data to avoid growing that conditional ?
Probably way more than five years; frankly someone needs to yell at
whoever wrote the documentation and point out to them that VBUS is a
well known name that came from the spec.
In this particular case we actually shouldn't be worry about this at all
as it turns out that the consumer and supply are both internal to the
chip and hard wired together with no external users (the
regulator_init_data is part of the twl core not the board) so we
shouldn't be making this visible outside the drivers in the first place,
the name is totally irrelevant to anything except the twl drivers. The
name is much more important if someone mapping out the regulators on a
board has to worry about it.
> > > We pass in the data of how regulators are wired at the board level and
> > > drivers would regulator_get(my_dev->dev, "whatever fancy name I want as
> > > it doesn't matter at all");
> > No, not at all. Consumer drivers should be hard coding the strings they
> > request and the strings used should correspond to the pin names used on
> > the device.
> but that's the thing. Why do they need to depend on that string ?
They have to depend on *something* textual. Even if you change it into
program code it's still going to have to be written down somewhere.
> > As Liam said this will just make the situation worse. Users will have
> > to figure out what the names the driver authors assigned to the various
> > device supplies map onto in the physical system via an additional layer
> > of indirection which will at best be written down in comments in the
> > driver.
> My point is that the "name" shouldn't matter. Instead of matching a
> "name" match a "reference". Have something earlier in the boot assing
> regulators to devices, or something like that, so that drivers don't
> need to care about "names".
Unfortunately we write computer programs using text and that does rather
mean that things need names eventually, and that's going to have to
include the driver since whatever else does the mapping is going to need
to figure out what the driver is using to refer to a given regulator.
At some point these magic references are going to have to come from a
human in some form the human has a hope of comprehending. The names
aren't really for the benefit of the driver, they're there so that a
human reading a schematic can work out which regulator to attach to
which supply and produce a mapping which other people can read and
understand.
> > Here you're apparently talking about something different - you're
> > talking about how we pass in the regulator init data for the regulators
> > supplied by the chip. The patch being discussed changes the consumer
> > side for USB, not the regulator driver side. The naming on the driver
> see above, how long do you think it'll take for someone to try and pass
> that name via pdata ? On the next name change, this is already likely to
> happen to prevent that conditional from growing.
People try to pass things as platform data all the time, usually as the
first thing they think of because they are having a hard time
distinguishing between the label the rail was given on the board and the
label the chip uses for the supply. This isn't a problem, we just tell
them not to do that so that people can use the driver on other boards.
> > side is a particular problem for TWL devices since unlike most PMICs the
> > regulators aren't simply numbered but are instead assigned individual
> > names so you can't just use a big array indexed by number.
> why not ? The name shouldn't matter. In anycase, if you look into
> /sys/class/regulator/ all directories are regulator.<index>:
> dev_set_name(&rdev->dev, "regulator.%d",
> atomic_inc_return(®ulator_no) - 1);
That's not terribly useful for describing the relationships between
devices as the names come on a first come first served basis and are
therefore not at all stable. We could try assiging base numbers to the
various regulator drivers but then you end up with a massive legibility
problem as nobody numbers the supplies on devices.
> > > What I was expecting to see, was a mechanism where we define how those
> shouldn't depend on strings at all in order to fetch the correct
> regulator. How many:
> if (xxx_features() & XXX_CLASS_IS_YYYY)
> regulator_name = "my_first_name";
> else
> regulator_name = "my_second_name";
> will have to pop in drivers until we figure that it won't scale ?
Note that the combination of _features and _CLASS tests here is already
fairly unusual for chips, even before you add on the random name
changes. I really think you're generalising too much from a rare case
here, and that it's more likely that for most cases where you get a name
change it'll be but one of many changes that need to be accounted for or
you'll just be able to do something fairly straightforward and table
based.
The upshot is that I'm not terribly concerned about this for the chips
we're actually seeing (this is the first time I've seen a device which
just randomly renames one supply at all). Besides, it's also not like
there's any alternatives on offer here.
Having said all that I do have a patch in the works which should mean
that noddy consumers (not this one) don't need any code or data at all
and can just key off device lifetime and PM events transparently