mfd: AXP20x: Add support for AXP202 and AXP209

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       | 240 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/axp20x.h | 180 ++++++++++++++++++++++++++++++++++
 4 files changed, 433 insertions(+)
 create mode 100644 drivers/mfd/axp20x.c
 create mode 100644 include/linux/mfd/axp20x.h

drivers/mfd/Kconfig +1 -0

index 49bb445..24ba61a 100644
@@ -59,6 +59,18 @@ config MFD_AAT2870_CORE
 	  additional drivers must be enabled in order to use the
 	  functionality of the device.
 
+config MFD_AXP20X

Quoted 27 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]>

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

drivers/mfd/Kconfig +0 -0

index 49bb445..24ba61a 100644
@@ -59,6 +59,18 @@ config MFD_AAT2870_CORE
 	  additional drivers must be enabled in order to use the
 	  functionality of the device.
 

drivers/mfd/Kconfig config MFD_AAT2870_CORE · line 72

Quoted 11 lines

drivers/mfd/Kconfig +11 -0

+config MFD_AXP20X
+	bool "X-Powers AXP20X"
+	select MFD_CORE
+	select REGMAP_I2C
+	select REGMAP_IRQ
+	depends on I2C=y
+	help
+	  If you say Y here you get support for the AXP20X.
+	  This driver provides common support for accessing the device,
+	  additional drivers must be enabled in order to use the
+	  functionality of the device.

Please tell us what this device is and what sub-devices are available?

[...]

drivers/mfd/axp20x.c line 2

Quoted 8 lines

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

new file mode 100644
index 0000000..f77a663
@@ -0,0 +1,240 @@
+/*
+ * axp20x.c - MFD core driver for the X-Powers AXP202 and AXP209

... which consist of ...

[...]

drivers/mfd/axp20x.c line 58

Quoted 13 lines

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

+static struct resource axp20x_pek_resources[] = {
+	{
+		.name	= "PEK_DBR",
+		.start	= AXP20X_IRQ_PEK_RIS_EDGE,
+		.end	= AXP20X_IRQ_PEK_RIS_EDGE,
+		.flags	= IORESOURCE_IRQ,
+	}, {
+		.name	= "PEK_DBF",
+		.start	= AXP20X_IRQ_PEK_FAL_EDGE,
+		.end	= AXP20X_IRQ_PEK_FAL_EDGE,
+		.flags	= IORESOURCE_IRQ,
+	},
+};

Have you considered doing this in the Device Tree? It's a lot less code/overhead.

drivers/mfd/axp20x.c line 122

Quoted 4 lines

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

+static const struct i2c_device_id axp20x_i2c_id[] = {
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, axp20x_i2c_id);

We really should consider changing the I2C subsystem!

Can you add a comment here describing why we have to add this seemingly pointless empty struct please?

drivers/mfd/axp20x.c line 144

Quoted 9 lines

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

+static struct mfd_cell axp20x_cells[] = {
+	{
+		.name		= "axp20x-pek",
+		.num_resources	= ARRAY_SIZE(axp20x_pek_resources),
+		.resources	= axp20x_pek_resources,
+	}, {
+		.name		= "axp20x-regulator",
+	},
+};

Do these drivers don't look inside the DTB at all?

drivers/mfd/axp20x.c line 169

Quoted 6 lines

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

+	of_id = of_match_device(axp20x_of_match, &i2c->dev);
+	if (!of_id) {
+		dev_err(&i2c->dev, "Unable to setup AXP20X data\n");
+		return -ENODEV;
+	}
+	axp20x->variant = (int) of_id->data;

'variant' needs to be a (unsigned?) long or it will break on 64bit architectures.

Quoted 8 lines

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

+static struct resource axp20x_pek_resources[] = {
+     {
+             .name   = "PEK_DBR",
+             .start  = AXP20X_IRQ_PEK_RIS_EDGE,
+             .end    = AXP20X_IRQ_PEK_RIS_EDGE,
+             .flags  = IORESOURCE_IRQ,
+     }, {
+             .name   = "PEK_DBF",

drivers/mfd/axp20x.c line 58

Quoted 5 lines

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

+             .start  = AXP20X_IRQ_PEK_FAL_EDGE,
+             .end    = AXP20X_IRQ_PEK_FAL_EDGE,
+             .flags  = IORESOURCE_IRQ,
+     },
+};

Have you considered doing this in the Device Tree? It's a lot less code/overhead.

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 | 260 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 272 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 +22 -0

new file mode 100644
index 0000000..6698a69
@@ -0,0 +1,260 @@
+/*
+ * 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>

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

+
+struct axp20x_pek {
+	struct axp20x_dev *axp20x;
+	struct input_dev *input;
+	int irq_dbr;
+	int irq_dbf;
+};
+
+struct axp20x_pek_ext_attr {
+	const char const **str;
+	unsigned int mask;
+};
+
+static struct axp20x_pek_ext_attr axp20x_pek_startup_ext_attr = {
+	.str	= startup_time,
+	.mask	= AXP20X_PEK_STARTUP_MASK,
+};
+
+static struct axp20x_pek_ext_attr axp20x_pek_shutdown_ext_attr = {
+	.str	= 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;
+	int cnt = 0;
+
+	ret = regmap_read(axp20x_pek->axp20x->regmap, AXP20X_PEK_KEY, &val);
+	if (ret != 0)

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

+	}
+
+	cnt += sprintf(buf + cnt, "\n");
+
+	return cnt;
+}
+
+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];
+	int ret, i;
+	size_t len;
+
+	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';
+
+	for (i = 0; i < 4; i++) {
+		if (!strcmp(val_str, axp20x_ea->str[i])) {
+
+			i <<= ffs(axp20x_ea->mask) - 1;
+			ret = regmap_update_bits(axp20x_pek->axp20x->regmap,
+						 AXP20X_PEK_KEY,
+						 axp20x_ea->mask, i);
+			if (ret != 0)
+				return -EINVAL;
+			return count;
+		}
+	}
+
+	return -EINVAL;
+}
+
+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(shutdown, 0644, axp20x_show_ext_attr, axp20x_store_ext_attr),
+	&axp20x_pek_shutdown_ext_attr
+};
+
+static struct attribute *dev_attrs[] = {
+	&axp20x_dev_attr_startup.attr.attr,
+	&axp20x_dev_attr_shutdown.attr.attr,
+	NULL,
+};
+
+static struct attribute_group dev_attr_group = {
+	.attrs	= dev_attrs,
+};
+
+static const struct attribute_group *dev_attr_groups[] = {
+	&dev_attr_group,
+	NULL,
+};
+
+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";

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

+	if (error) {
+		dev_err(axp20x->dev, "Failed to request dbr IRQ#%d: %d\n",
+			axp20x_pek->irq_dbr, error);
+
+		return error;
+	}
+

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

+	error = input_register_device(idev);
+	if (error) {
+		dev_err(axp20x->dev, "Can't register input device: %d\n", error);
+		return error;
+	}
+

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

+
+	return 0;
+}
+
+static struct platform_driver axp20x_pek_driver = {
+	.probe		= axp20x_pek_probe,
+	.remove		= axp20x_pek_remove,
+	.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

Hi Carlo,

Quoted 12 lines

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

+#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>
+

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

Quoted 5 lines

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

+#define AXP20X_PEK_STARTUP_MASK		(0xc0)
+#define AXP20X_PEK_SHUTDOWN_MASK	(0x03)
+
+static const char const *startup_time[] = { "128ms", "3s" , "1s", "2s" };
+static const char const *shutdown_time[] = { "4s", "6s" , "8s", "10s" };

Why not have everything expressed in milliseconds and have sysfs attribute apply the closest one possible?

By the way, do you want to plumb these through device tree as well?

Quoted 43 lines

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

+
+struct axp20x_pek {
+	struct axp20x_dev *axp20x;
+	struct input_dev *input;
+	int irq_dbr;
+	int irq_dbf;
+};
+
+struct axp20x_pek_ext_attr {
+	const char const **str;
+	unsigned int mask;
+};
+
+static struct axp20x_pek_ext_attr axp20x_pek_startup_ext_attr = {
+	.str	= startup_time,
+	.mask	= AXP20X_PEK_STARTUP_MASK,
+};
+
+static struct axp20x_pek_ext_attr axp20x_pek_shutdown_ext_attr = {
+	.str	= 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;
+	int cnt = 0;
+
+	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 81

Quoted 6 lines

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

+
+	for (i = 0; i < 4; i++) {
+		if (val == i)
+			cnt += sprintf(buf + cnt, "[%s] ", axp20x_ea->str[i]);
+		else
+			cnt += sprintf(buf + cnt, "%s ", axp20x_ea->str[i]);

Please just return the current value; why do we need pretty-printing?

Quoted 123 lines

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

+	}
+
+	cnt += sprintf(buf + cnt, "\n");
+
+	return cnt;
+}
+
+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];
+	int ret, i;
+	size_t len;
+
+	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';
+
+	for (i = 0; i < 4; i++) {
+		if (!strcmp(val_str, axp20x_ea->str[i])) {
+
+			i <<= ffs(axp20x_ea->mask) - 1;
+			ret = regmap_update_bits(axp20x_pek->axp20x->regmap,
+						 AXP20X_PEK_KEY,
+						 axp20x_ea->mask, i);
+			if (ret != 0)
+				return -EINVAL;
+			return count;
+		}
+	}
+
+	return -EINVAL;
+}
+
+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(shutdown, 0644, axp20x_show_ext_attr, axp20x_store_ext_attr),
+	&axp20x_pek_shutdown_ext_attr
+};
+
+static struct attribute *dev_attrs[] = {
+	&axp20x_dev_attr_startup.attr.attr,
+	&axp20x_dev_attr_shutdown.attr.attr,
+	NULL,
+};
+
+static struct attribute_group dev_attr_group = {
+	.attrs	= dev_attrs,
+};
+
+static const struct attribute_group *dev_attr_groups[] = {
+	&dev_attr_group,
+	NULL,
+};
+
+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);

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

Quoted 6 lines

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

+
+	input_set_drvdata(idev, axp20x_pek);
+
+	error = devm_request_threaded_irq(&pdev->dev, axp20x_pek->irq_dbr,
+					  NULL, axp20x_pek_irq, 0,
+					  "axp20x-pek-dbr", idev);

Why does it have to be threaded IRQ?

Quoted 12 lines

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

+	if (error) {
+		dev_err(axp20x->dev, "Failed to request dbr IRQ#%d: %d\n",
+			axp20x_pek->irq_dbr, error);
+
+		return error;
+	}
+
+	error = devm_request_threaded_irq(&pdev->dev, axp20x_pek->irq_dbf,
+					  NULL, axp20x_pek_irq, 0,
+					  "axp20x-pek-dbf", idev);
+	if (error) {
+		dev_err(axp20x->dev, "Failed to request dbf IRQ#%d: %d\n",

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

Quoted 5 lines

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

+			axp20x_pek->irq_dbf, error);
+		return error;
+	}
+
+	idev->dev.groups = dev_attr_groups;

These are not generic input attributes so they should belong to the platform device, not input device.

Quoted 10 lines

drivers/input/misc/axp20x-pek.c new file +10 -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;
+}

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

Quoted 6 lines

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

+
+static int axp20x_pek_remove(struct platform_device *pdev)
+{
+	struct axp20x_pek *axp20x_pek = platform_get_drvdata(pdev);
+
+	input_unregister_device(axp20x_pek->input);

No need to call input_unregister_device() for managed input devices.

Quoted 14 lines

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

+
+	return 0;
+}
+
+static struct platform_driver axp20x_pek_driver = {
+	.probe		= axp20x_pek_probe,
+	.remove		= axp20x_pek_remove,
+	.driver		= {
+		.name		= "axp20x-pek",
+		.owner		= THIS_MODULE,
+	},
+};
+module_platform_driver(axp20x_pek_driver);
+

Quoted 6 lines

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

+MODULE_DESCRIPTION("axp20x Power Button");
+MODULE_AUTHOR("Carlo Caione <[email protected]>");
+MODULE_LICENSE("GPL");

Signature · 1 line

1.8.3.2

Thanks.

Quoted 1 line · Dmitry Torokhov

Hi Carlo,

Hi Dmitry,

Quoted 2 lines · Carlo Caione

Quoted 6 lines

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

Quoted 4 lines

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

+
+	error = devm_request_threaded_irq(&pdev->dev, axp20x_pek->irq_dbr,
+					  NULL, axp20x_pek_irq, 0,
+					  "axp20x-pek-dbr", idev);

Why does it have to be threaded IRQ?

Because this is already handled as a nested irq from a irq thread. The parent threaded irq is installed by regmap_add_irq_chip() in the MFD driver core.

Quoted 13 lines

Quoted 13 lines

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

+	if (error) {
+		dev_err(axp20x->dev, "Failed to request dbr IRQ#%d: %d\n",
+			axp20x_pek->irq_dbr, error);
+
+		return error;
+	}
+
+	error = devm_request_threaded_irq(&pdev->dev, axp20x_pek->irq_dbf,
+					  NULL, axp20x_pek_irq, 0,
+					  "axp20x-pek-dbf", idev);
+	if (error) {
+		dev_err(axp20x->dev, "Failed to request dbf IRQ#%d: %d\n",

Quoted 8 lines

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

Quoted 5 lines

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

+			axp20x_pek->irq_dbf, error);
+		return error;
+	}
+
+	idev->dev.groups = dev_attr_groups;

These are not generic input attributes so they should belong to the platform device, not input device.

Ok. Can I ask why they cannot be considered an input attributes?

drivers/regulator/axp20x-regulator.c line 96

Quoted 9 lines · Carlo Caione

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

+static int axp20x_set_suspend_voltage(struct regulator_dev *rdev, int uV)
+{
+	int sel = regulator_map_voltage_iterate(rdev, uV, uV);
+
+	if (sel < 0)
+		return sel;
+
+	return regulator_set_voltage_sel_regmap(rdev, sel);
+}

This is fairly obviously broken - it's overwriting the normal runtime value, this will disrupt the running system if we want the value we use on suspend is different to the value we want at runtime.

Think about it - if this was a sane thing to do the core would just do it without needing driver specific code, we already know how to set the voltage for the device.

Quoted 3 lines · Mark Brown

This is fairly obviously broken - it's overwriting the normal runtime value, this will disrupt the running system if we want the value we use on suspend is different to the value we want at runtime.

Ok, silly question: isn't it exactly what we want? Set the voltage for the regulator when the system is suspended?

Quoted 3 lines

Think about it - if this was a sane thing to do the core would just do it without needing driver specific code, we already know how to set the voltage for the device.

I thought it was because some regulators can have specific regs for managing the suspend mode.

BTW, but then what is the difference between my code and (i.e.) the same routine in da9055-regulator.c?

http://lxr.linux.no/linux+v3.13.5/drivers/regulator/da9055-regulator.c#L276

This dtsi describes the axp209 PMIC, and is to be included from inside the i2c controller node to which the axp209 is connected.

 arch/arm/boot/dts/x-powers-axp209.dtsi | 54 ++++++++++++++++++++++++++++++++++

Quoted 4 lines · Carlo Caione

This dtsi describes the axp209 PMIC, and is to be included from inside the i2c controller node to which the axp209 is connected.

 arch/arm/boot/dts/x-powers-axp209.dtsi | 54 ++++++++++++++++++++++++++++++++++

Something is wrong here. Either the changelog is inaccurate or this is broken, either way this should be cleaned up because this looks like very bad practice. If this is a common include file for boards derived from some reference design then the changelog is misleading and the DT should be clearer about the board tie ins. Otherwise the .dtsi is defining what should be board specific parameters.

The fact that the DT names everything after the regulator on the PMIC and not after the supply names on the board is especially suspicious and glancing at a couple of the regulators it looks like the constraints here are the maximum ranges the PMIC supports rather than anything to do with any board.

Please take a step back and think about what the regulator constraints are intended to do - they're about matching the regulator capabilities to the board since it is rare for boards to be able to do everything the regulator can support. Duplicating the basic device capabilities into the DT would be a pointless waste of time.

arch/arm/boot/dts/x-powers-axp209.dtsi line 21

Quoted 5 lines

arch/arm/boot/dts/x-powers-axp209.dtsi new file +5 -0

+	axp_dcdc2: dcdc2 {
+		regulator-min-microvolt = <700000>;
+		regulator-max-microvolt = <2275000>;
+		regulator-always-on;
+	};

What is this configuration actually for - what guarantee do we have that the above is safe for a given board using this regulator?

In general I'd expect to see anything specifying variability for a regulator to also include the relevant consumer node.

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

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

---
 arch/arm/boot/dts/sun4i-a10-a1000.dts           | 13 +++++++++++++
 arch/arm/boot/dts/sun4i-a10-cubieboard.dts      | 13 +++++++++++++
 arch/arm/boot/dts/sun4i-a10-hackberry.dts       | 19 +++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-inet97fv2.dts       | 13 +++++++++++++
 arch/arm/boot/dts/sun4i-a10-mini-xplus.dts      | 19 +++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-olinuxino-lime.dts  | 19 +++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-pcduino.dts         | 13 +++++++++++++
 arch/arm/boot/dts/sun7i-a20-cubieboard2.dts     | 14 ++++++++++++++
 arch/arm/boot/dts/sun7i-a20-cubietruck.dts      | 15 +++++++++++++++
 arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts | 14 ++++++++++++++
 10 files changed, 152 insertions(+)

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

index fa746aea..cf18c4d 100644

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

Quoted 12 lines

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

index fa746aea..cf18c4d 100644
@@ -88,6 +88,19 @@
                        pinctrl-names = "default";
                        pinctrl-0 = <&i2c0_pins_a>;
                        status = "okay";
+                       #address-cells = <1>;
+                       #size-cells = <0>;
+
+                       axp: axp20x at 34 {

Hi,

Sorry for spotting this just now. I suggest you change it to

                          axp209: pmic at 34 {

or

                          axp209_pmic at 34 {

to match the ePAPR policy that node names should be generic and describe functionality. The latter is what some DTs use.

Same for all the other files.

Thanks for working on this!

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

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

---
 arch/arm/boot/dts/sun4i-a10-a1000.dts           | 13 +++++++++++++
 arch/arm/boot/dts/sun4i-a10-cubieboard.dts      | 13 +++++++++++++
 arch/arm/boot/dts/sun4i-a10-hackberry.dts       | 19 +++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-inet97fv2.dts       | 13 +++++++++++++
 arch/arm/boot/dts/sun4i-a10-mini-xplus.dts      | 19 +++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-olinuxino-lime.dts  | 19 +++++++++++++++++++
 arch/arm/boot/dts/sun4i-a10-pcduino.dts         | 13 +++++++++++++
 arch/arm/boot/dts/sun7i-a20-cubieboard2.dts     | 14 ++++++++++++++
 arch/arm/boot/dts/sun7i-a20-cubietruck.dts      | 15 +++++++++++++++
 arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts | 14 ++++++++++++++
 10 files changed, 152 insertions(+)

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

index fa746aea..cf18c4d 100644
@@ -88,6 +88,19 @@
 			pinctrl-names = "default";
 			pinctrl-0 = <&i2c0_pins_a>;
 			status = "okay";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			axp: axp20x at 34 {
+				reg = <0x34>;

Here, you would just include the dtsi at the top of the file, and in the DTSI, you would have something like:

&axp209 {
   regulators {
      ...
   }
}

Quoted 2 lines · Maxime Ripard

Here, you would just include the dtsi at the top of the file, and in the DTSI, you would have something like:

Quoted 5 lines

&axp209 {
   regulators {
      ...
   }
}

Hang on, what is an include file setting up regulators for?

Quoted 1 line · Mark Brown

Hang on, what is an include file setting up regulators for?

Look at patch 7. To share the regulators declaration across all boards and avoid duplication.

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. AXP20x comprises an adaptive USB-Compatible PWM charger, 2 BUCK DC-DC converters, 5 LDOs, multiple 12-bit ADCs of voltage, current and temperature as well as 4 configurable GPIOs.

Carlo Caione (10):
mfd: AXP20x: Add mfd driver for AXP20x PMIC
dt-bindings: add vendor-prefix for X-Powers
mfd: AXP20x: Add bindings documentation
input: misc: Add driver for AXP20x Power Enable Key
input: misc: Add ABI docs for AXP20x PEK
regulator: AXP20x: Add support for regulators subsystem
ARM: sunxi: dt: Add x-powers-axp209.dtsi file
ARM: sun7i/sun4i: dt: Add AXP209 support to various boards
ARM: sunxi: Add AXP20x support in defconfig
ARM: sunxi: Add AXP20x support multi_v7_defconfig