mfd: AXP20x: Add support for AXP202 and AXP209

drivers/mfd/axp20x.c new file +8 -0

+#define AXP20X_IRQ(_irq, _off, _mask) \
+       [AXP20X_IRQ_##_irq] = { .reg_offset = (_off), .mask = BIT(_mask) }
+
+static const struct regmap_irq axp20x_regmap_irqs[] = {
+       AXP20X_IRQ(ACIN_OVER_V,         0, 7),
+       AXP20X_IRQ(ACIN_PLUGIN,         0, 6),
+       AXP20X_IRQ(ACIN_REMOVAL,        0, 5),
+       AXP20X_IRQ(VBUS_OVER_V,         0, 4),

Quoted 8 lines · On Friday 11 April 2014 11:38:05 Carlo Caione wrote:

drivers/mfd/axp20x.c new file +8 -0

+#define AXP20X_IRQ(_irq, _off, _mask) \
+       [AXP20X_IRQ_##_irq] = { .reg_offset = (_off), .mask = BIT(_mask) }
+
+static const struct regmap_irq axp20x_regmap_irqs[] = {
+       AXP20X_IRQ(ACIN_OVER_V,         0, 7),
+       AXP20X_IRQ(ACIN_PLUGIN,         0, 6),
+       AXP20X_IRQ(ACIN_REMOVAL,        0, 5),
+       AXP20X_IRQ(VBUS_OVER_V,         0, 4),

drivers/mfd/axp20x.c line 90

Quoted 10 lines

drivers/mfd/axp20x.c new file +10 -0

+       AXP20X_IRQ(VBUS_PLUGIN,         0, 3),
+       AXP20X_IRQ(VBUS_REMOVAL,        0, 2),
+       AXP20X_IRQ(VBUS_V_LOW,          0, 1),
+       AXP20X_IRQ(BATT_PLUGIN,         1, 7),
+       AXP20X_IRQ(BATT_REMOVAL,        1, 6),
+       AXP20X_IRQ(BATT_ENT_ACT_MODE,   1, 5),
+       AXP20X_IRQ(BATT_EXIT_ACT_MODE,  1, 4),
+       AXP20X_IRQ(CHARG,               1, 3),
+       AXP20X_IRQ(CHARG_DONE,          1, 2),
+       AXP20X_IRQ(BATT_TEMP_HIGH,      1, 1),

Why do you have to enumerate the interrupts here? Can't you just put all the numbers into the DT nodes of the devices using them?

In general, I would say that the mfd driver should not care about what is connected to it.

Why do you have to enumerate the interrupts here? Can't you just put all the numbers into the DT nodes of the devices using them?

In general, I would say that the mfd driver should not care about what is connected to it.

Quoted 5 lines · Arnd Bergmann

Why do you have to enumerate the interrupts here? Can't you just put all the numbers into the DT nodes of the devices using them?

In general, I would say that the mfd driver should not care about what is connected to it.

This then means that all the machines using the device need to define the interrupt table and have the MFD cells represented in the DT which means encoding Linux abstractions into the DT.

In cases where the device is also used with ACPI or platform data that's a definite issue since they have different idioms. That applies less to PMICs tightly bound to particular SoCs but is an issue in general, not all the world is DT.

There's also issues here with us changing our subsystems. Things like clocks are a bit indistinct at present, they're sort of floating between clock and other subsystems. We've also done things like invent extcon, making completely new subdevices. Keeping the data out of DT avoids problems when this happens. The balance changes a bit if there are clearly reusable IPs within the device but sadly hardware designers don't always give us that and even then sometimes we don't want to use them like that.

This patch introduces the preliminary support for PMICs X-Powers AXP202 and AXP209. The AXP209 and AXP202 are the PMUs (Power Management Unit) used by A10, A13 and A20 SoCs and developed by X-Powers, a sister company of Allwinner.

The core enables support for two subsystems:
- PEK (Power Enable Key)
- Regulators

Signed-off-by Carlo Caione <[email protected]>

---
 drivers/mfd/Kconfig        |  12 +++
 drivers/mfd/Makefile       |   1 +
 drivers/mfd/axp20x.c       | 249 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/axp20x.h | 180 ++++++++++++++++++++++++++++++++
 4 files changed, 442 insertions(+)
 create mode 100644 drivers/mfd/axp20x.c
 create mode 100644 include/linux/mfd/axp20x.h

Quoted 10 lines

This patch introduces the preliminary support for PMICs X-Powers AXP202 and AXP209. The AXP209 and AXP202 are the PMUs (Power Management Unit) used by A10, A13 and A20 SoCs and developed by X-Powers, a sister company of Allwinner.

The core enables support for two subsystems:
- PEK (Power Enable Key)
- Regulators

Signed-off-by Carlo Caione <[email protected]>

Quoted 8 lines

---
 drivers/mfd/Kconfig        |  12 +++
 drivers/mfd/Makefile       |   1 +
 drivers/mfd/axp20x.c       | 249 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/axp20x.h | 180 ++++++++++++++++++++++++++++++++
 4 files changed, 442 insertions(+)
 create mode 100644 drivers/mfd/axp20x.c
 create mode 100644 include/linux/mfd/axp20x.h

This looks good to me know.

Once you have all the other Acks you need I'd be happy to create an immutable branch for the other maintainers to pull from.

This patch add support for the Power Enable Key found on MFD AXP202 and AXP209. Besides the basic support for the button, the driver adds two entries in sysfs to configure the time delay for power on/off.

Signed-off-by Carlo Caione <[email protected]>

---
 drivers/input/misc/Kconfig      |  11 ++
 drivers/input/misc/Makefile     |   1 +
 drivers/input/misc/axp20x-pek.c | 261 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 273 insertions(+)
 create mode 100644 drivers/input/misc/axp20x-pek.c

drivers/input/misc/Kconfig +11 -0

index 7904ab0..87244fb 100644
@@ -393,6 +393,17 @@ config INPUT_RETU_PWRBUTTON
 	  To compile this driver as a module, choose M here. The module will
 	  be called retu-pwrbutton.
 
+config INPUT_AXP20X_PEK
+	tristate "X-Powers AXP20X power button driver"
+	depends on MFD_AXP20X
+	help
+	  Say Y here if you want to enable power key reporting via the
+	  AXP20X PMIC.
+
+	  To compile this driver as a module, choose M here. The module will
+	  be called axp20x-pek.
+
+
 config INPUT_TWL4030_PWRBUTTON
 	tristate "TWL4030 Power button Driver"
 	depends on TWL4030_CORE

drivers/input/misc/Makefile +1 -0

index cda71fc..624abf5 100644
@@ -50,6 +50,7 @@ obj-$(CONFIG_INPUT_POWERMATE)		+= powermate.o
 obj-$(CONFIG_INPUT_PWM_BEEPER)		+= pwm-beeper.o
 obj-$(CONFIG_INPUT_RB532_BUTTON)	+= rb532_button.o
 obj-$(CONFIG_INPUT_RETU_PWRBUTTON)	+= retu-pwrbutton.o
+obj-$(CONFIG_INPUT_AXP20X_PEK)		+= axp20x-pek.o
 obj-$(CONFIG_INPUT_GPIO_ROTARY_ENCODER)	+= rotary_encoder.o
 obj-$(CONFIG_INPUT_SGI_BTNS)		+= sgi_btns.o
 obj-$(CONFIG_INPUT_SIRFSOC_ONKEY)	+= sirfsoc-onkey.o

drivers/input/misc/axp20x-pek.c new file +86 -0

new file mode 100644
index 0000000..a9112bd
@@ -0,0 +1,261 @@
+/*
+ * axp20x power button driver.
+ *
+ * Copyright (C) 2013 Carlo Caione <[email protected]>
+ *
+ * This file is subject to the terms and conditions of the GNU General
+ * Public License. See the file "COPYING" in the main directory of this
+ * archive for more details.
+ *
+ * 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.
+ */
+
+#include <linux/errno.h>
+#include <linux/irq.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mfd/axp20x.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define AXP20X_PEK_STARTUP_MASK		(0xc0)
+#define AXP20X_PEK_SHUTDOWN_MASK	(0x03)
+
+struct axp20x_pek {
+	struct axp20x_dev *axp20x;
+	struct input_dev *input;
+	int irq_dbr;
+	int irq_dbf;
+};
+
+struct axp20x_time {
+	unsigned int time;
+	unsigned int idx;
+};
+
+static const struct axp20x_time startup_time[] = {
+	{ .time = 128,  .idx = 0 },
+	{ .time = 1000, .idx = 2 },
+	{ .time = 3000, .idx = 1 },
+	{ .time = 2000, .idx = 3 },
+};
+
+static const struct axp20x_time shutdown_time[] = {
+	{ .time = 4000,  .idx = 0 },
+	{ .time = 6000,  .idx = 1 },
+	{ .time = 8000,  .idx = 2 },
+	{ .time = 10000, .idx = 3 },
+};
+
+struct axp20x_pek_ext_attr {
+	const struct axp20x_time *p_time;
+	unsigned int mask;
+};
+
+static struct axp20x_pek_ext_attr axp20x_pek_startup_ext_attr = {
+	.p_time	= startup_time,
+	.mask	= AXP20X_PEK_STARTUP_MASK,
+};
+
+static struct axp20x_pek_ext_attr axp20x_pek_shutdown_ext_attr = {
+	.p_time	= shutdown_time,
+	.mask	= AXP20X_PEK_SHUTDOWN_MASK,
+};
+
+static struct axp20x_pek_ext_attr *get_axp_ext_attr(struct device_attribute *attr)
+{
+	return container_of(attr, struct dev_ext_attribute, attr)->var;
+}
+
+static ssize_t axp20x_show_ext_attr(struct device *dev, struct device_attribute *attr,
+				    char *buf)
+{
+	struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
+	struct axp20x_pek_ext_attr *axp20x_ea = get_axp_ext_attr(attr);
+	unsigned int val;
+	int ret, i;
+
+	ret = regmap_read(axp20x_pek->axp20x->regmap, AXP20X_PEK_KEY, &val);
+	if (ret != 0)

drivers/input/misc/axp20x-pek.c new file +131 -0

+}
+
+static ssize_t axp20x_store_ext_attr(struct device *dev, struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
+	struct axp20x_pek_ext_attr *axp20x_ea = get_axp_ext_attr(attr);
+	char val_str[20];
+	size_t len;
+	int ret, i;
+	unsigned int val, idx = 0;
+	unsigned int best_err = UINT_MAX;
+
+	val_str[sizeof(val_str) - 1] = '\0';
+	strncpy(val_str, buf, sizeof(val_str) - 1);
+	len = strlen(val_str);
+
+	if (len && val_str[len - 1] == '\n')
+		val_str[len - 1] = '\0';
+
+	ret = kstrtouint(val_str, 10, &val);
+	if (ret)
+		return ret;
+
+	for (i = 3; i >= 0; i--) {
+		unsigned int err;
+
+		err = abs(axp20x_ea->p_time[i].time - val);
+		if (err < best_err) {
+			best_err = err;
+			idx = axp20x_ea->p_time[i].idx;
+		}
+
+		if (!err)
+			break;
+	}
+
+	idx <<= ffs(axp20x_ea->mask) - 1;
+	ret = regmap_update_bits(axp20x_pek->axp20x->regmap,
+				 AXP20X_PEK_KEY,
+				 axp20x_ea->mask, idx);
+	if (ret != 0)
+		return -EINVAL;
+	return count;
+}
+
+static struct dev_ext_attribute axp20x_dev_attr_startup = {
+	.attr	= __ATTR(startup, 0644, axp20x_show_ext_attr, axp20x_store_ext_attr),
+	.var	= &axp20x_pek_startup_ext_attr
+};
+
+static struct dev_ext_attribute axp20x_dev_attr_shutdown = {
+	.attr	= __ATTR(shutdown, 0644, axp20x_show_ext_attr, axp20x_store_ext_attr),
+	.var	= &axp20x_pek_shutdown_ext_attr
+};
+
+static irqreturn_t axp20x_pek_irq(int irq, void *pwr)
+{
+	struct input_dev *idev = pwr;
+	struct axp20x_pek *axp20x_pek = input_get_drvdata(idev);
+
+	if (irq == axp20x_pek->irq_dbr)
+		input_report_key(idev, KEY_POWER, true);
+	else if (irq == axp20x_pek->irq_dbf)
+		input_report_key(idev, KEY_POWER, false);
+
+	input_sync(idev);
+
+	return IRQ_HANDLED;
+}
+
+static int axp20x_pek_probe(struct platform_device *pdev)
+{
+	struct axp20x_pek *axp20x_pek;
+	struct axp20x_dev *axp20x;
+	struct input_dev *idev;
+	int error;
+
+	axp20x_pek = devm_kzalloc(&pdev->dev, sizeof(struct axp20x_pek),
+				  GFP_KERNEL);
+	if (!axp20x_pek)
+		return -ENOMEM;
+
+	axp20x_pek->axp20x = dev_get_drvdata(pdev->dev.parent);
+	axp20x = axp20x_pek->axp20x;
+
+	axp20x_pek->irq_dbr = platform_get_irq_byname(pdev, "PEK_DBR");
+	if (axp20x_pek->irq_dbr < 0) {
+		dev_err(&pdev->dev, "No IRQ for PEK_DBR, error=%d\n",
+				axp20x_pek->irq_dbr);
+		return axp20x_pek->irq_dbr;
+	}
+	axp20x_pek->irq_dbr = regmap_irq_get_virq(axp20x->regmap_irqc,
+						  axp20x_pek->irq_dbr);
+
+	axp20x_pek->irq_dbf = platform_get_irq_byname(pdev, "PEK_DBF");
+	if (axp20x_pek->irq_dbf < 0) {
+		dev_err(&pdev->dev, "No IRQ for PEK_DBF, error=%d\n",
+				axp20x_pek->irq_dbf);
+		return axp20x_pek->irq_dbf;
+	}
+	axp20x_pek->irq_dbf = regmap_irq_get_virq(axp20x->regmap_irqc,
+						  axp20x_pek->irq_dbf);
+
+	axp20x_pek->input = devm_input_allocate_device(&pdev->dev);
+	if (!axp20x_pek->input)
+		return -ENOMEM;
+
+	idev = axp20x_pek->input;
+
+	idev->name = "axp20x-pek";
+	idev->phys = "m1kbd/input2";
+	idev->dev.parent = &pdev->dev;
+
+	input_set_capability(idev, EV_KEY, KEY_POWER);
+
+	input_set_drvdata(idev, axp20x_pek);
+
+	error = devm_request_any_context_irq(&pdev->dev, axp20x_pek->irq_dbr,
+					  axp20x_pek_irq, 0,
+					  "axp20x-pek-dbr", idev);
+	if (error < 0) {
+		dev_err(axp20x->dev, "Failed to request dbr IRQ#%d: %d\n",
+			axp20x_pek->irq_dbr, error);
+
+		return error;
+	}
+
+	error = devm_request_any_context_irq(&pdev->dev, axp20x_pek->irq_dbf,
+					  axp20x_pek_irq, 0,
+					  "axp20x-pek-dbf", idev);

drivers/input/misc/axp20x-pek.c new file +24 -0

+
+	error = input_register_device(idev);
+	if (error) {
+		dev_err(axp20x->dev, "Can't register input device: %d\n", error);
+		return error;
+	}
+
+	platform_set_drvdata(pdev, axp20x_pek);
+
+	return 0;
+}
+
+static struct platform_driver axp20x_pek_driver = {
+	.probe		= axp20x_pek_probe,
+	.driver		= {
+		.name		= "axp20x-pek",
+		.owner		= THIS_MODULE,
+	},
+};
+module_platform_driver(axp20x_pek_driver);
+
+MODULE_DESCRIPTION("axp20x Power Button");
+MODULE_AUTHOR("Carlo Caione <[email protected]>");
+MODULE_LICENSE("GPL");

Signature · 1 line

1.8.3.2

Quoted 15 lines

drivers/input/misc/axp20x-pek.c new file +15 -0

+static ssize_t axp20x_show_ext_attr(struct device *dev, struct device_attribute *attr,
+                                 char *buf)
+{
+     struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
+     struct axp20x_pek_ext_attr *axp20x_ea = get_axp_ext_attr(attr);
+     unsigned int val;
+     int ret, i;
+
+     ret = regmap_read(axp20x_pek->axp20x->regmap, AXP20X_PEK_KEY, &val);
+     if (ret != 0)
+             return ret;
+
+     val &= axp20x_ea->mask;
+     val >>= ffs(axp20x_ea->mask) - 1;
+

drivers/input/misc/axp20x-pek.c line 96

Quoted 5 lines

drivers/input/misc/axp20x-pek.c new file +5 -0

+     for (i = 0; i < 4; i++)
+             if (val == axp20x_ea->p_time[i].idx)
+                     val = axp20x_ea->p_time[i].time;
+
+     return sprintf(buf, "%ums\n", val);

Please do not print ums and instead document the units in sysfs ABI docs.

drivers/input/misc/axp20x-pek.c line 237

Quoted 3 lines

drivers/input/misc/axp20x-pek.c new file +3 -0

+     if (device_create_file(&pdev->dev, &axp20x_dev_attr_startup.attr) ||
+         device_create_file(&pdev->dev, &axp20x_dev_attr_shutdown.attr))
+             return -ENODEV;

I think you still want to use attribute group here. You also need to clean up the attributes when unbinding device. Also, why returning -ENODEV instead of the proper error code?

AXP202 and AXP209 come with two synchronous step-down DC-DCs and five LDOs. This patch introduces basic support for those regulators.

Quoted 2 lines · Carlo Caione

AXP202 and AXP209 come with two synchronous step-down DC-DCs and five LDOs. This patch introduces basic support for those regulators.

Applied, thanks, but I had to resolve some trivial add/add conflicts with the Broadcom regulator driver Makefile and Kconfig additions - please remember to submit patches against current code.

AXP202 and AXP209 come with two synchronous step-down DC-DCs and five LDOs. This patch introduces basic support for those regulators.

Signed-off-by Carlo Caione <[email protected]>

---
 drivers/regulator/Kconfig            |   7 +
 drivers/regulator/Makefile           |   1 +

I know I'm late, and this patch has already been applied, but shouldn't we use of_get_child_by_name instead of of_find_node_by_name. This might lead to wrong regulators node parsing if other regulators are defined in the DT after the axp20x node, because, AFAIK, this function searches for DT nodes defined after the specified np node, but not necessarely children of the np node.

Hello Carlo,

Quoted 7 lines · Carlo Caione

AXP202 and AXP209 come with two synchronous step-down DC-DCs and five LDOs. This patch introduces basic support for those regulators.

Signed-off-by Carlo Caione <[email protected]>

---
 drivers/regulator/Kconfig            |   7 +
 drivers/regulator/Makefile           |   1 +

[...]

drivers/regulator/axp20x-regulator.c line 191

Quoted 4 lines

drivers/regulator/axp20x-regulator.c new file +4 -0

+     if (!np)
+             return 0;
+
+     regulators = of_find_node_by_name(np, "regulators");

I know I'm late, and this patch has already been applied, but shouldn't we use of_get_child_by_name instead of of_find_node_by_name. This might lead to wrong regulators node parsing if other regulators are defined in the DT after the axp20x node, because, AFAIK, this function searches for DT nodes defined after the specified np node, but not necessarely children of the np node.

In all the DTs the min and max microvolt allowed for each regulator are actually the min and max voltage possible for the regulator itself. This is not safe but we do not have the ranges allowed for each board and the original Allwinner driver does exactly this way.

Quoted 4 lines · Carlo Caione

In all the DTs the min and max microvolt allowed for each regulator are actually the min and max voltage possible for the regulator itself. This is not safe but we do not have the ranges allowed for each board and the original Allwinner driver does exactly this way.

Is there any code in their kernel which varies the supply voltages? If there isn't then simply omitting the voltage ranges is the best option, leaving the supplies fixed. If there is then the range it uses is a good starting point. In general supplies will be fixed voltage on a given board unless there is a specific reason to vary them.

Review

Quoted 2 lines

(quoted diff) +2 -0

+                             regulators {
+                                     compatible = "x-powers,axp20x-reg";

This compatible isn't part of the driver.

Quoted 2 lines · Mark Brown

Quoted 2 lines

(quoted diff) +2 -0

+                             regulators {
+                                     compatible = "x-powers,axp20x-reg";

Quoted 1 line

This compatible isn't part of the driver.

Yes I know. The problem here is that in v4 I had to fill in the field .of_compatible of the mfd_cell with "x-powers,axp20x-reg". This because the regulator_dev_lookup() checks for dev->of_node when looking for the supply so I needed the compatible string in the DT to have the dev->of_node filled in by mfd_add_device().
What do you suggest? Modify the regulator driver?

Quoted 5 lines · Carlo Caione

Quoted 2 lines · Mark Brown

Quoted 2 lines

(quoted diff) +2 -0

+                             regulators {
+                                     compatible = "x-powers,axp20x-reg";

Quoted 8 lines

Quoted 1 line

This compatible isn't part of the driver.

Yes I know. The problem here is that in v4 I had to fill in the field .of_compatible of the mfd_cell with "x-powers,axp20x-reg". This because the regulator_dev_lookup() checks for dev->of_node when looking for the supply so I needed the compatible string in the DT to have the dev->of_node filled in by mfd_add_device().
What do you suggest? Modify the regulator driver?

You're looking for regulator_bulk_register_supply_alias() in the MFD driver (via parent_supplies in the MFD cell probably).

I'm fighting with a small issue when using the regulator_bulk_register_supply_alias(). Problem is that when using the .parent_supplies entry in the MFD driver, I hit the

WARN_ON(!list_empty(&dev->devres_head));

in linux/drivers/base/dd.c#L272, but, apart from the warning, everything seems to work correctly.
A possible explanation I gave myself is that in the mfd_add_device() we try to use the devm_* API when the regulator device is not bound to the driver yet (I found some information here
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/104442.html).
Is this the case?

Quoted 6 lines · Carlo Caione

I'm fighting with a small issue when using the regulator_bulk_register_supply_alias(). Problem is that when using the .parent_supplies entry in the MFD driver, I hit the

WARN_ON(!list_empty(&dev->devres_head));

Quoted 7 lines

in linux/drivers/base/dd.c#L272, but, apart from the warning, everything seems to work correctly.
A possible explanation I gave myself is that in the mfd_add_device() we try to use the devm_* API when the regulator device is not bound to the driver yet (I found some information here
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/104442.html).
Is this the case?

Without knowing more about the case you're hitting it's hard to say - I do run a board which exercises the API for a MFD (with the arizona drivers) regularly and haven't noticed an issue so there must be something different about what you're trying to do.

I'm having a really hard time with this problem, so any hint is welcome :) The small modification I'm using on top of the patches in this series is here: http://bpaste.net/show/228330/

Unfortunately as I said I got this when booting:
http://bpaste.net/show/nUhUTzELT32v9HNPathL/

Quoted 3 lines · Carlo Caione

I'm having a really hard time with this problem, so any hint is welcome :) The small modification I'm using on top of the patches in this series is here: http://bpaste.net/show/228330/

Quoted 2 lines

Unfortunately as I said I got this when booting:
http://bpaste.net/show/nUhUTzELT32v9HNPathL/

Huh, actually the arizona drivers do show this (it was being masked in my logs by another unrelated bug). I guess the Wolfson guys aren't working with upstream much (though Charles did write the orignal code here...).

The issue is the MFD core, it shouldn't be using managed allocations here - the error reported by the assert is entirely correct. If the CODEC driver is bound and unbound it'll not be possible to reload it as things stand.

Your driver is correct but the implementation needs to be fixed - possibly with an API change on free since at the minute the cells to be freed don't get passed back into the MFD core when deallocating.

Quoted 9 lines · Mark Brown

Quoted 6 lines · Carlo Caione

I'm having a really hard time with this problem, so any hint is welcome :) The small modification I'm using on top of the patches in this series is here: http://bpaste.net/show/228330/

Unfortunately as I said I got this when booting:
http://bpaste.net/show/nUhUTzELT32v9HNPathL/

Quoted 4 lines

Huh, actually the arizona drivers do show this (it was being masked in my logs by another unrelated bug). I guess the Wolfson guys aren't working with upstream much (though Charles did write the orignal code here...).

I run upstream fairly regularly here (although mostly on Arndale rather than Speyside). On closer inspection of my kernel log don't seem to have anything related to it, which is odd.

Ah ok seems I am getting an error but for some reason for me it shows up looking very unrelated to the supply mapping. In that it shows up much later in the log and doesn't seem to mention the MFD at all:

Signed-off-by Hans de Goede <[email protected]>

Signed-off-by Carlo Caione <[email protected]>

---

In all the DTs the min and max microvolt allowed for each regulator are actually the min and max voltage possible for the regulator itself. This is not safe but we do not have the ranges allowed for each board and the original Allwinner driver does exactly this way.

AXP20x has the so called Power Path Management (IPS) that can select the proper power supply according to the status of the external power and the Li-battery status. The output of the IPS block is usually a 5V fixed voltage used as input supply for all the other regulators. This fixed voltage is represented in the DT as a fixed voltage regulator in the "regulator" subnode.

 arch/arm/boot/dts/sun4i-a10-a1000.dts           | 69 +++++++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-cubieboard.dts      | 69 +++++++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-hackberry.dts       | 75 +++++++++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-inet97fv2.dts       | 69 +++++++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-mini-xplus.dts      | 75 +++++++++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-olinuxino-lime.dts  | 75 +++++++++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-pcduino.dts         | 69 +++++++++++++++++++++++
 arch/arm/boot/dts/sun7i-a20-cubieboard2.dts     | 70 +++++++++++++++++++++++
 arch/arm/boot/dts/sun7i-a20-cubietruck.dts      | 70 +++++++++++++++++++++++
 arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts | 70 +++++++++++++++++++++++

arch/arm/boot/dts/sun4i-a10-a1000.dts +7 -0

+			axp209: pmic at 34 {
+				compatible = "x-powers,axp209";
+				reg = <0x34>;
+				interrupts = <0>;
+
+				interrupt-controller;
+				#interrupt-cells = <1>;

arch/arm/boot/dts/sun4i-a10-a1000.dts +49 -0

+
+					x-powers,dcdc-freq = <1500>;
+
+					axp_ipsout_reg: axp_ipsout {
+						compatible = "regulator-fixed";
+						regulator-name = "axp-ipsout";
+						regulator-min-microvolt = <5000000>;
+						regulator-max-microvolt = <5000000>;
+						regulator-always-on;
+					};
+
+					axp_vcore_reg: dcdc2 {
+						regulator-min-microvolt = <700000>;
+						regulator-max-microvolt = <2275000>;
+						regulator-always-on;
+					};
+
+					axp_ddr_reg: dcdc3 {
+						regulator-min-microvolt = <700000>;
+						regulator-max-microvolt = <3500000>;
+						regulator-always-on;
+					};
+
+					axp_rtc_reg: ldo1 {
+						regulator-always-on;
+					};
+
+					axp_analog_reg: ldo2 {
+						regulator-min-microvolt = <1800000>;
+						regulator-max-microvolt = <3300000>;
+						regulator-always-on;
+					};
+
+					axp_pll_reg: ldo3 {
+						regulator-min-microvolt = <700000>;
+						regulator-max-microvolt = <3500000>;
+					};
+
+					axp_hdmi_reg: ldo4 {
+						regulator-min-microvolt = <1250000>;
+						regulator-max-microvolt = <3300000>;
+					};
+
+					axp_mic_reg: ldo5 {
+						regulator-min-microvolt = <1800000>;
+						regulator-max-microvolt = <3300000>;
+					};
+				};
+			};
 		};
 	};
 

arch/arm/boot/dts/sun4i-a10-cubieboard.dts +69 -0

index 4684cbe..635fd4b 100644
@@ -80,6 +80,75 @@
 			pinctrl-names = "default";
 			pinctrl-0 = <&i2c0_pins_a>;
 			status = "okay";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			axp209: pmic at 34 {
+				compatible = "x-powers,axp209";
+				reg = <0x34>;
+				interrupts = <0>;
+
+				interrupt-controller;
+				#interrupt-cells = <1>;
+
+				acin-supply = <&axp_ipsout_reg>;
+				vin2-supply = <&axp_ipsout_reg>;
+				vin3-supply = <&axp_ipsout_reg>;
+				ldo24in-supply = <&axp_ipsout_reg>;
+				ldo3in-supply = <&axp_ipsout_reg>;
+				ldo5in-supply = <&axp_ipsout_reg>;
+
+				regulators {
+					compatible = "x-powers,axp20x-reg";
+
+					x-powers,dcdc-freq = <1500>;
+
+					axp_ipsout_reg: axp_ipsout {
+						compatible = "regulator-fixed";
+						regulator-name = "axp-ipsout";
+						regulator-min-microvolt = <5000000>;
+						regulator-max-microvolt = <5000000>;
+						regulator-always-on;
+					};
+
+					axp_vcore_reg: dcdc2 {
+						regulator-min-microvolt = <700000>;
+						regulator-max-microvolt = <2275000>;
+						regulator-always-on;
+					};
+
+					axp_ddr_reg: dcdc3 {
+						regulator-min-microvolt = <700000>;
+						regulator-max-microvolt = <3500000>;
+						regulator-always-on;
+					};
+
+					axp_rtc_reg: ldo1 {
+						regulator-always-on;
+					};
+
+					axp_analog_reg: ldo2 {
+						regulator-min-microvolt = <1800000>;
+						regulator-max-microvolt = <3300000>;
+						regulator-always-on;
+					};
+
+					axp_pll_reg: ldo3 {
+						regulator-min-microvolt = <700000>;
+						regulator-max-microvolt = <3500000>;
+					};
+
+					axp_hdmi_reg: ldo4 {
+						regulator-min-microvolt = <1250000>;
+						regulator-max-microvolt = <3300000>;
+					};
+
+					axp_mic_reg: ldo5 {
+						regulator-min-microvolt = <1800000>;
+						regulator-max-microvolt = <3300000>;
+					};
+				};
+			};
 		};
 
 		i2c1: i2c at 01c2b000 {

arch/arm/boot/dts/sun4i-a10-hackberry.dts +0 -0

index d7c17e4..8f2db9c 100644

Hi Carlo,

Quoted 15 lines

Signed-off-by Hans de Goede <[email protected]>

Signed-off-by Carlo Caione <[email protected]>

---

In all the DTs the min and max microvolt allowed for each regulator are actually the min and max voltage possible for the regulator itself. This is not safe but we do not have the ranges allowed for each board and the original Allwinner driver does exactly this way.

AXP20x has the so called Power Path Management (IPS) that can select the proper power supply according to the status of the external power and the Li-battery status. The output of the IPS block is usually a 5V fixed voltage used as input supply for all the other regulators. This fixed voltage is represented in the DT as a fixed voltage regulator in the "regulator" subnode.

Quoted 10 lines

 arch/arm/boot/dts/sun4i-a10-a1000.dts           | 69 +++++++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-cubieboard.dts      | 69 +++++++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-hackberry.dts       | 75 +++++++++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-inet97fv2.dts       | 69 +++++++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-mini-xplus.dts      | 75 +++++++++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-olinuxino-lime.dts  | 75 +++++++++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-pcduino.dts         | 69 +++++++++++++++++++++++
 arch/arm/boot/dts/sun7i-a20-cubieboard2.dts     | 70 +++++++++++++++++++++++
 arch/arm/boot/dts/sun7i-a20-cubietruck.dts      | 70 +++++++++++++++++++++++
 arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts | 70 +++++++++++++++++++++++

That looks like a lot of them. Did you test all of them? Are all those regulators you define used on all these boards?

arch/arm/boot/dts/sun4i-a10-a1000.dts line 92

Quoted 12 lines

 10 files changed, 711 insertions(+)

arch/arm/boot/dts/sun4i-a10-a1000.dts +2 -0

index fa746aea..029a880 100644
@@ -88,6 +88,75 @@
                      pinctrl-names = "default";
                      pinctrl-0 = <&i2c0_pins_a>;
                      status = "okay";
+                     #address-cells = <1>;
+                     #size-cells = <0>;

That should be in the DTSI.

Quoted 12 lines

arch/arm/boot/dts/sun4i-a10-a1000.dts +12 -0

+                     axp209: pmic at 34 {
+                             compatible = "x-powers,axp209";
+                             reg = <0x34>;
+                             interrupts = <0>;
+
+                             interrupt-controller;
+                             #interrupt-cells = <1>;
+
+                             acin-supply = <&axp_ipsout_reg>;
+                             vin2-supply = <&axp_ipsout_reg>;
+                             vin3-supply = <&axp_ipsout_reg>;
+                             ldo24in-supply = <&axp_ipsout_reg>;

arch/arm/boot/dts/sun4i-a10-a1000.dts line 110

Quoted 5 lines

arch/arm/boot/dts/sun4i-a10-a1000.dts +5 -0

+                             ldo3in-supply = <&axp_ipsout_reg>;
+                             ldo5in-supply = <&axp_ipsout_reg>;
+
+                             regulators {
+                                     compatible = "x-powers,axp20x-reg";

I told you a few times already, but don't introduce pattern-matching compatibles.

Quoted 137 lines

arch/arm/boot/dts/sun4i-a10-a1000.dts +49 -0

+
+                                     x-powers,dcdc-freq = <1500>;
+
+                                     axp_ipsout_reg: axp_ipsout {
+                                             compatible = "regulator-fixed";
+                                             regulator-name = "axp-ipsout";
+                                             regulator-min-microvolt = <5000000>;
+                                             regulator-max-microvolt = <5000000>;
+                                             regulator-always-on;
+                                     };
+
+                                     axp_vcore_reg: dcdc2 {
+                                             regulator-min-microvolt = <700000>;
+                                             regulator-max-microvolt = <2275000>;
+                                             regulator-always-on;
+                                     };
+
+                                     axp_ddr_reg: dcdc3 {
+                                             regulator-min-microvolt = <700000>;
+                                             regulator-max-microvolt = <3500000>;
+                                             regulator-always-on;
+                                     };
+
+                                     axp_rtc_reg: ldo1 {
+                                             regulator-always-on;
+                                     };
+
+                                     axp_analog_reg: ldo2 {
+                                             regulator-min-microvolt = <1800000>;
+                                             regulator-max-microvolt = <3300000>;
+                                             regulator-always-on;
+                                     };
+
+                                     axp_pll_reg: ldo3 {
+                                             regulator-min-microvolt = <700000>;
+                                             regulator-max-microvolt = <3500000>;
+                                     };
+
+                                     axp_hdmi_reg: ldo4 {
+                                             regulator-min-microvolt = <1250000>;
+                                             regulator-max-microvolt = <3300000>;
+                                     };
+
+                                     axp_mic_reg: ldo5 {
+                                             regulator-min-microvolt = <1800000>;
+                                             regulator-max-microvolt = <3300000>;
+                                     };
+                             };
+                     };
              };
      };
 

arch/arm/boot/dts/sun4i-a10-cubieboard.dts +69 -0

index 4684cbe..635fd4b 100644
@@ -80,6 +80,75 @@
                      pinctrl-names = "default";
                      pinctrl-0 = <&i2c0_pins_a>;
                      status = "okay";
+                     #address-cells = <1>;
+                     #size-cells = <0>;
+
+                     axp209: pmic at 34 {
+                             compatible = "x-powers,axp209";
+                             reg = <0x34>;
+                             interrupts = <0>;
+
+                             interrupt-controller;
+                             #interrupt-cells = <1>;
+
+                             acin-supply = <&axp_ipsout_reg>;
+                             vin2-supply = <&axp_ipsout_reg>;
+                             vin3-supply = <&axp_ipsout_reg>;
+                             ldo24in-supply = <&axp_ipsout_reg>;
+                             ldo3in-supply = <&axp_ipsout_reg>;
+                             ldo5in-supply = <&axp_ipsout_reg>;
+
+                             regulators {
+                                     compatible = "x-powers,axp20x-reg";
+
+                                     x-powers,dcdc-freq = <1500>;
+
+                                     axp_ipsout_reg: axp_ipsout {
+                                             compatible = "regulator-fixed";
+                                             regulator-name = "axp-ipsout";
+                                             regulator-min-microvolt = <5000000>;
+                                             regulator-max-microvolt = <5000000>;
+                                             regulator-always-on;
+                                     };
+
+                                     axp_vcore_reg: dcdc2 {
+                                             regulator-min-microvolt = <700000>;
+                                             regulator-max-microvolt = <2275000>;
+                                             regulator-always-on;
+                                     };
+
+                                     axp_ddr_reg: dcdc3 {
+                                             regulator-min-microvolt = <700000>;
+                                             regulator-max-microvolt = <3500000>;
+                                             regulator-always-on;
+                                     };
+
+                                     axp_rtc_reg: ldo1 {
+                                             regulator-always-on;
+                                     };
+
+                                     axp_analog_reg: ldo2 {
+                                             regulator-min-microvolt = <1800000>;
+                                             regulator-max-microvolt = <3300000>;
+                                             regulator-always-on;
+                                     };
+
+                                     axp_pll_reg: ldo3 {
+                                             regulator-min-microvolt = <700000>;
+                                             regulator-max-microvolt = <3500000>;
+                                     };
+
+                                     axp_hdmi_reg: ldo4 {
+                                             regulator-min-microvolt = <1250000>;
+                                             regulator-max-microvolt = <3300000>;
+                                     };
+
+                                     axp_mic_reg: ldo5 {
+                                             regulator-min-microvolt = <1800000>;
+                                             regulator-max-microvolt = <3300000>;
+                                     };
+                             };
+                     };
              };
 
              i2c1: i2c at 01c2b000 {

arch/arm/boot/dts/sun4i-a10-hackberry.dts +0 -0

index d7c17e4..8f2db9c 100644

arch/arm/boot/dts/sun4i-a10-hackberry.dts line 89

Quoted 8 lines

arch/arm/boot/dts/sun4i-a10-hackberry.dts +5 -0

@@ -82,6 +82,81 @@
                      pinctrl-0 = <&uart0_pins_a>;
                      status = "okay";
              };
+
+             i2c0: i2c at 01c2ac00 {
+                     pinctrl-names = "default";
+                     pinctrl-0 = <&i2c0_pins_a>;
+                     status = "okay";

That should be in a separate patch.

These comments apply to most of the changes here. Make sure you edit all of them.

Quoted 1 line · Maxime Ripard

Hi Carlo,

Hi Maxime,

Quoted 27 lines · Carlo Caione

Quoted 25 lines

Signed-off-by Hans de Goede <[email protected]>

Signed-off-by Carlo Caione <[email protected]>

---

In all the DTs the min and max microvolt allowed for each regulator are actually the min and max voltage possible for the regulator itself. This is not safe but we do not have the ranges allowed for each board and the original Allwinner driver does exactly this way.

AXP20x has the so called Power Path Management (IPS) that can select the proper power supply according to the status of the external power and the Li-battery status. The output of the IPS block is usually a 5V fixed voltage used as input supply for all the other regulators. This fixed voltage is represented in the DT as a fixed voltage regulator in the "regulator" subnode.

 arch/arm/boot/dts/sun4i-a10-a1000.dts           | 69 +++++++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-cubieboard.dts      | 69 +++++++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-hackberry.dts       | 75 +++++++++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-inet97fv2.dts       | 69 +++++++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-mini-xplus.dts      | 75 +++++++++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-olinuxino-lime.dts  | 75 +++++++++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-pcduino.dts         | 69 +++++++++++++++++++++++
 arch/arm/boot/dts/sun7i-a20-cubieboard2.dts     | 70 +++++++++++++++++++++++
 arch/arm/boot/dts/sun7i-a20-cubietruck.dts      | 70 +++++++++++++++++++++++
 arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts | 70 +++++++++++++++++++++++

Quoted 2 lines

That looks like a lot of them. Did you test all of them? Are all those regulators you define used on all these boards?

I tested it only on cubieboard2, all the other boards are contributed by Hans. I'll double check with Hans.

Quoted 15 lines

arch/arm/boot/dts/sun4i-a10-a1000.dts line 92

Quoted 13 lines

 10 files changed, 711 insertions(+)

arch/arm/boot/dts/sun4i-a10-a1000.dts +2 -0

index fa746aea..029a880 100644
@@ -88,6 +88,75 @@
                      pinctrl-names = "default";
                      pinctrl-0 = <&i2c0_pins_a>;
                      status = "okay";
+                     #address-cells = <1>;
+                     #size-cells = <0>;

That should be in the DTSI.

Agree.

As Mark has also mentioned we should probably pin the regulators to a certain voltage, except for those which we expect to be controlled by a driver, so basically all of them should be pinned to a certain voltage except for DCDC2 which gets used for the cpu voltage which we will want to scale as soon as we've a cpufreq driver.

While testing the latest revision of your code I also noticed that the kernel ends up disabling LDO3 and LDO4, which could be fine on some boards and a problem on other boards.

I think we need to be careful here. For now it may be best to only add the DCDC2 regulator to the dts, as we know that dcdc2 is used for the cpu voltage everywhere, and we will actually want to control that later on.

For the others, for the boards where we've schematics (*) it would be good to add the other regulators with fixed voltages as specified in the schematics. For the rest it may be best to simply leave the regulators alone / at their default settings.