[PATCH 1/4] power: Add an axp20x-ac-power driver

[PATCH 1/4] power: Add an axp20x-ac-power driver

[Michael Haas]

This adds a driver for the ac power_supply bits of the axp20x
PMICs.

This submission is taken directly from Bruno Prémonts 2015 RFC [0].
The original RFC contains drivers for AC, battery and backup
battery. This commit only adds the AC driver for now.

My only change to Bruno's axp20x_ac_power.c is the additional
check on a possible short-circuit between ACIN and VBUS. In this
case, axp20x-ac-power-supply refuses to attach itself to the device
and axp20x-usb-power-supply must be used.

[0] http://permalink.gmane.org/gmane.comp.hardware.netbook.arm.sunxi/17980

Signed-off-by: Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org>
---
 drivers/mfd/axp20x.c            |  11 +++
 drivers/power/Makefile          |   2 +-
 drivers/power/axp20x_ac_power.c | 212 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 224 insertions(+), 1 deletion(-)
 create mode 100644 drivers/power/axp20x_ac_power.c

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index a57d6e9..9351c0e 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -178,6 +178,12 @@ static struct resource axp288_power_button_resources[] = {
 	},
 };
 
+static struct resource axp20x_ac_power_supply_resources[] = {
+	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_PLUGIN, "ACIN_PLUGIN"),
+	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_REMOVAL, "ACIN_REMOVAL"),
+	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_OVER_V, "ACIN_OVER_V"),
+};
+
 static struct resource axp288_fuel_gauge_resources[] = {
 	{
 		.start = AXP288_IRQ_QWBTU,
@@ -440,6 +446,11 @@ static struct mfd_cell axp20x_cells[] = {
 		.of_compatible	= "x-powers,axp202-usb-power-supply",
 		.num_resources	= ARRAY_SIZE(axp20x_usb_power_supply_resources),
 		.resources	= axp20x_usb_power_supply_resources,
+	}, {
+		.name		= "axp20x-ac-power-supply",
+		.of_compatible	= "x-powers,axp202-ac-power-supply",
+		.num_resources	= ARRAY_SIZE(axp20x_ac_power_supply_resources),
+		.resources	= axp20x_ac_power_supply_resources,
 	},
 };
 
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index e46b75d..3a785cc 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -9,7 +9,7 @@ obj-$(CONFIG_GENERIC_ADC_BATTERY)	+= generic-adc-battery.o
 
 obj-$(CONFIG_PDA_POWER)		+= pda_power.o
 obj-$(CONFIG_APM_POWER)		+= apm_power.o
-obj-$(CONFIG_AXP20X_POWER)	+= axp20x_usb_power.o
+obj-$(CONFIG_AXP20X_POWER)	+= axp20x_usb_power.o axp20x_ac_power.o
 obj-$(CONFIG_MAX8925_POWER)	+= max8925_power.o
 obj-$(CONFIG_WM831X_BACKUP)	+= wm831x_backup.o
 obj-$(CONFIG_WM831X_POWER)	+= wm831x_power.o
diff --git a/drivers/power/axp20x_ac_power.c b/drivers/power/axp20x_ac_power.c
new file mode 100644
index 0000000..c5bcbeb
--- /dev/null
+++ b/drivers/power/axp20x_ac_power.c
@@ -0,0 +1,212 @@
+/*
+ * AXP20x PMIC AC power driver
+ *
+ * Copyright 2014-2015 Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under  the terms of the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/mfd/axp20x.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define DRVNAME "axp20x-ac-power"
+/* Fields of AXP20X_PWR_INPUT_STATUS */
+#define AXP20X_PWR_STATUS_AC_PRESENT     BIT(7)
+#define AXP20X_PWR_STATUS_AC_AVAILABLE   BIT(6)
+#define AXP20X_PWR_STATUS_AC_VBUS_SHORT  BIT(1)
+#define AXP20X_PWR_STATUS_AC_VBUS_SEL    BIT(0)
+
+/* Fields of AXP20X_ADC_EN1 */
+#define AXP20X_ADC_EN1_ACIN_VOLT BIT(5)
+#define AXP20X_ADC_EN1_ACIN_CURR BIT(4)
+
+struct axp20x_ac_power {
+	struct regmap *regmap;
+	struct power_supply *supply;
+};
+
+static int axp20x_ac_power_get_property(struct power_supply *psy,
+	enum power_supply_property psp, union power_supply_propval *val)
+{
+	struct axp20x_ac_power *power = power_supply_get_drvdata(psy);
+	unsigned int input;
+	int r;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		r = axp20x_read_variable_width(power->regmap,
+					       AXP20X_ACIN_V_ADC_H, 12);
+		if (r < 0)
+			return r;
+
+		val->intval = r * 1700; /* 1 step = 1.7 mV */
+		return 0;
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+		r = axp20x_read_variable_width(power->regmap,
+					       AXP20X_ACIN_I_ADC_H, 12);
+		if (r < 0)
+			return r;
+
+		val->intval = r * 375; /* 1 step = 0.375 mA */
+		return 0;
+	default:
+		break;
+	}
+
+	/* All the properties below need the input-status reg value */
+	r = regmap_read(power->regmap, AXP20X_PWR_INPUT_STATUS, &input);
+	if (r)
+		return r;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = !!(input & AXP20X_PWR_STATUS_AC_PRESENT);
+		break;
+	case POWER_SUPPLY_PROP_ONLINE:
+		val->intval = !!(input & AXP20X_PWR_STATUS_AC_AVAILABLE);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static enum power_supply_property axp20x_ac_power_properties[] = {
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+};
+
+static const struct power_supply_desc axp20x_ac_power_desc = {
+	.name = "axp20x-ac",
+	.type = POWER_SUPPLY_TYPE_MAINS,
+	.properties = axp20x_ac_power_properties,
+	.num_properties = ARRAY_SIZE(axp20x_ac_power_properties),
+	.get_property = axp20x_ac_power_get_property,
+};
+
+static irqreturn_t axp20x_irq_ac_over_v(int irq, void *devid)
+{
+	struct axp20x_ac_power *power = devid;
+
+	dev_warn(&power->supply->dev, "IRQ#%d AC over voltage\n", irq);
+	power_supply_changed(power->supply);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t axp20x_irq_ac_plugin(int irq, void *devid)
+{
+	struct axp20x_ac_power *power = devid;
+
+	dev_info(&power->supply->dev, "IRQ#%d AC connected\n", irq);
+	power_supply_changed(power->supply);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t axp20x_irq_ac_removal(int irq, void *devid)
+{
+	struct axp20x_ac_power *power = devid;
+
+	dev_info(&power->supply->dev, "IRQ#%d AC disconnected\n", irq);
+	power_supply_changed(power->supply);
+
+	return IRQ_HANDLED;
+}
+
+static int axp20x_ac_power_probe(struct platform_device *pdev)
+{
+	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
+	struct power_supply_config psy_cfg = {};
+	struct axp20x_ac_power *power;
+	static const char * const irq_names[] = { "ACIN_PLUGIN",
+		"ACIN_REMOVAL", "ACIN_OVER_V" };
+	irqreturn_t (*irq_funcs[])(int, void *) = { axp20x_irq_ac_plugin,
+		axp20x_irq_ac_removal, axp20x_irq_ac_over_v };
+	int i, irq, r, input;
+
+	if (!of_device_is_available(pdev->dev.of_node))
+		return -ENODEV;
+
+	power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
+	if (!power)
+		return -ENOMEM;
+
+	power->regmap = axp20x->regmap;
+
+	r = regmap_read(power->regmap, AXP20X_PWR_INPUT_STATUS, &input);
+	if (r < 0)
+		return r;
+
+	if (!!(input & AXP20X_PWR_STATUS_AC_VBUS_SHORT)) {
+		dev_err(&pdev->dev, "AC is connected to VBUS. Use axp20x_usb_power-supply driver instead!");
+		return -ENODEV;
+	}
+
+	/* Enable ac voltage and current measurement */
+	r = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
+			AXP20X_ADC_EN1_ACIN_CURR | AXP20X_ADC_EN1_ACIN_VOLT,
+			AXP20X_ADC_EN1_ACIN_CURR | AXP20X_ADC_EN1_ACIN_VOLT);
+	if (r)
+		return r;
+
+	psy_cfg.of_node = pdev->dev.of_node;
+	psy_cfg.drv_data = power;
+
+	power->supply = devm_power_supply_register(&pdev->dev,
+					&axp20x_ac_power_desc, &psy_cfg);
+	if (IS_ERR(power->supply))
+		return PTR_ERR(power->supply);
+
+	/* Request irqs after registering, as irqs may trigger immediately */
+	for (i = 0; i < ARRAY_SIZE(irq_names); i++) {
+		irq = platform_get_irq_byname(pdev, irq_names[i]);
+		if (irq < 0) {
+			dev_warn(&pdev->dev, "No IRQ for %s: %d\n",
+				 irq_names[i], irq);
+			continue;
+		}
+		irq = regmap_irq_get_virq(axp20x->regmap_irqc, irq);
+		r = devm_request_any_context_irq(&pdev->dev, irq,
+				irq_funcs[i], 0, DRVNAME, power);
+		if (r < 0)
+			dev_warn(&pdev->dev, "Error requesting %s IRQ: %d\n",
+				 irq_names[i], r);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id axp20x_ac_power_match[] = {
+	{ .compatible = "x-powers,axp202-ac-power-supply" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, axp20x_ac_power_match);
+
+static struct platform_driver axp20x_ac_power_driver = {
+	.probe = axp20x_ac_power_probe,
+	.driver = {
+		.name = DRVNAME,
+		.of_match_table = axp20x_ac_power_match,
+	},
+};
+
+module_platform_driver(axp20x_ac_power_driver);
+
+MODULE_AUTHOR("Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>");
+MODULE_DESCRIPTION("AXP20x PMIC AC power supply status driver");
+MODULE_LICENSE("GPL");
-- 
2.8.0

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[PATCH 2/4] ARM: dts: axp209: Add ac_power_supply child node to the ax209 node

[Michael Haas]

Add a node representing the ac power supply part of the axp209 pmic.

Signed-off-by: Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org>
---
 arch/arm/boot/dts/axp209.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/axp209.dtsi b/arch/arm/boot/dts/axp209.dtsi
index 051ab3b..9046f0a 100644
--- a/arch/arm/boot/dts/axp209.dtsi
+++ b/arch/arm/boot/dts/axp209.dtsi
@@ -94,4 +94,10 @@
 		compatible = "x-powers,axp202-usb-power-supply";
 		status = "disabled";
 	};
+
+	ac_power_supply: ac_power_supply {
+		compatible = "x-powers,axp202-ac-power-supply";
+		status = "disabled";
+	};
+
 };
-- 
2.8.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] ARM: dts: Add binding documentation for AXP20x pmic ac power supply

[Michael Haas]

Add binding documentation for the ac power supply part of the AXP20x
pmic.

Signed-off-by: Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org>
---
 .../bindings/power_supply/axp20x_ac_power.txt      | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power_supply/axp20x_ac_power.txt

diff --git a/Documentation/devicetree/bindings/power_supply/axp20x_ac_power.txt b/Documentation/devicetree/bindings/power_supply/axp20x_ac_power.txt
new file mode 100644
index 0000000..1cbdcfb
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_supply/axp20x_ac_power.txt
@@ -0,0 +1,34 @@
+AXP20x AC power supply
+
+Required Properties:
+-compatible: "x-powers,axp202-ac-power-supply"
+
+This node is a subnode of the axp20x PMIC.
+
+Example:
+
+axp209: pmic@34 {
+	compatible = "x-powers,axp209";
+	reg = <0x34>;
+	interrupt-parent = <&nmi_intc>;
+	interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
+	interrupt-controller;
+	#interrupt-cells = <1>;
+
+	regulators {
+		x-powers,dcdc-freq = <1500>;
+
+		vdd_cpu: dcdc2 {
+			regulator-always-on;
+			regulator-min-microvolt = <1000000>;
+			regulator-max-microvolt = <1450000>;
+			regulator-name = "vdd-cpu";
+		};
+
+		...
+	};
+
+	ac-power-supply: ac-power-supply {
+		compatible = "x-powers,axp202-ac-power-supply";
+	};
+};
-- 
2.8.0

[PATCH 4/4] ARM: dts: sunxi: add ac-power to A20 OLinuXino Lime2 board

[Michael Haas]

The A20-olinuxino-lime2 has an AC power input.
This patch enables support for current and voltage monitoring
via the axp20x-ac-power driver.

Signed-off-by: Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org>
---
 arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts b/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
index 9303635..ed83365 100644
--- a/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
+++ b/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
@@ -166,6 +166,10 @@
 		usb_power_supply: usb_power_supply {
 			compatible = "x-powers,axp202-usb-power-supply";
 		};
+		ac_power_supply: ac_power_supply {
+			status = "okay";
+			compatible = "x-powers,axp202-ac-power-supply";
+		};
 	};
 };
 
-- 
2.8.0

Re: [PATCH 1/4] power: Add an axp20x-ac-power driver

[Maxime Ripard]

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

Hi,

On Sun, Apr 03, 2016 at 03:15:04PM +0200, Michael Haas wrote:
> This adds a driver for the ac power_supply bits of the axp20x
> PMICs.
> 
> This submission is taken directly from Bruno Prémonts 2015 RFC [0].
> The original RFC contains drivers for AC, battery and backup
> battery. This commit only adds the AC driver for now.
> 
> My only change to Bruno's axp20x_ac_power.c is the additional
> check on a possible short-circuit between ACIN and VBUS. In this
> case, axp20x-ac-power-supply refuses to attach itself to the device
> and axp20x-usb-power-supply must be used.
> 
> [0] http://permalink.gmane.org/gmane.comp.hardware.netbook.arm.sunxi/17980
> 
> Signed-off-by: Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org>
> ---
>  drivers/mfd/axp20x.c            |  11 +++
>  drivers/power/Makefile          |   2 +-
>  drivers/power/axp20x_ac_power.c | 212 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 224 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/power/axp20x_ac_power.c
> 
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index a57d6e9..9351c0e 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -178,6 +178,12 @@ static struct resource axp288_power_button_resources[] = {
>  	},
>  };
>  
> +static struct resource axp20x_ac_power_supply_resources[] = {
> +	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_PLUGIN, "ACIN_PLUGIN"),
> +	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_REMOVAL, "ACIN_REMOVAL"),
> +	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_OVER_V, "ACIN_OVER_V"),
> +};
> +
>  static struct resource axp288_fuel_gauge_resources[] = {
>  	{
>  		.start = AXP288_IRQ_QWBTU,
> @@ -440,6 +446,11 @@ static struct mfd_cell axp20x_cells[] = {
>  		.of_compatible	= "x-powers,axp202-usb-power-supply",
>  		.num_resources	= ARRAY_SIZE(axp20x_usb_power_supply_resources),
>  		.resources	= axp20x_usb_power_supply_resources,
> +	}, {
> +		.name		= "axp20x-ac-power-supply",
> +		.of_compatible	= "x-powers,axp202-ac-power-supply",
> +		.num_resources	= ARRAY_SIZE(axp20x_ac_power_supply_resources),
> +		.resources	= axp20x_ac_power_supply_resources,
>  	},
>  };
>

These changes should be in a separate patch.

> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index e46b75d..3a785cc 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -9,7 +9,7 @@ obj-$(CONFIG_GENERIC_ADC_BATTERY)	+= generic-adc-battery.o
>  
>  obj-$(CONFIG_PDA_POWER)		+= pda_power.o
>  obj-$(CONFIG_APM_POWER)		+= apm_power.o
> -obj-$(CONFIG_AXP20X_POWER)	+= axp20x_usb_power.o
> +obj-$(CONFIG_AXP20X_POWER)	+= axp20x_usb_power.o axp20x_ac_power.o
>  obj-$(CONFIG_MAX8925_POWER)	+= max8925_power.o
>  obj-$(CONFIG_WM831X_BACKUP)	+= wm831x_backup.o
>  obj-$(CONFIG_WM831X_POWER)	+= wm831x_power.o
> diff --git a/drivers/power/axp20x_ac_power.c b/drivers/power/axp20x_ac_power.c
> new file mode 100644
> index 0000000..c5bcbeb
> --- /dev/null
> +++ b/drivers/power/axp20x_ac_power.c
> @@ -0,0 +1,212 @@
> +/*
> + * AXP20x PMIC AC power driver
> + *
> + * Copyright 2014-2015 Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under  the terms of the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/mfd/axp20x.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define DRVNAME "axp20x-ac-power"
> +/* Fields of AXP20X_PWR_INPUT_STATUS */
> +#define AXP20X_PWR_STATUS_AC_PRESENT     BIT(7)
> +#define AXP20X_PWR_STATUS_AC_AVAILABLE   BIT(6)
> +#define AXP20X_PWR_STATUS_AC_VBUS_SHORT  BIT(1)
> +#define AXP20X_PWR_STATUS_AC_VBUS_SEL    BIT(0)
> +
> +/* Fields of AXP20X_ADC_EN1 */
> +#define AXP20X_ADC_EN1_ACIN_VOLT BIT(5)
> +#define AXP20X_ADC_EN1_ACIN_CURR BIT(4)

That should probably be defined in the global header, next to the
registers.

> +struct axp20x_ac_power {
> +	struct regmap *regmap;
> +	struct power_supply *supply;
> +};
> +
> +static int axp20x_ac_power_get_property(struct power_supply *psy,
> +	enum power_supply_property psp, union power_supply_propval *val)

The alignment is supposed to be on the opening parentheses

> +{
> +	struct axp20x_ac_power *power = power_supply_get_drvdata(psy);
> +	unsigned int input;
> +	int r;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		r = axp20x_read_variable_width(power->regmap,
> +					       AXP20X_ACIN_V_ADC_H, 12);
> +		if (r < 0)
> +			return r;
> +
> +		val->intval = r * 1700; /* 1 step = 1.7 mV */
> +		return 0;
> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +		r = axp20x_read_variable_width(power->regmap,
> +					       AXP20X_ACIN_I_ADC_H, 12);
> +		if (r < 0)
> +			return r;
> +
> +		val->intval = r * 375; /* 1 step = 0.375 mA */
> +		return 0;
> +	default:
> +		break;
> +	}
> +
> +	/* All the properties below need the input-status reg value */
> +	r = regmap_read(power->regmap, AXP20X_PWR_INPUT_STATUS, &input);
> +	if (r)
> +		return r;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_PRESENT:
> +		val->intval = !!(input & AXP20X_PWR_STATUS_AC_PRESENT);
> +		break;
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		val->intval = !!(input & AXP20X_PWR_STATUS_AC_AVAILABLE);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static enum power_supply_property axp20x_ac_power_properties[] = {
> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +};
> +
> +static const struct power_supply_desc axp20x_ac_power_desc = {
> +	.name = "axp20x-ac",
> +	.type = POWER_SUPPLY_TYPE_MAINS,
> +	.properties = axp20x_ac_power_properties,
> +	.num_properties = ARRAY_SIZE(axp20x_ac_power_properties),
> +	.get_property = axp20x_ac_power_get_property,
> +};
> +
> +static irqreturn_t axp20x_irq_ac_over_v(int irq, void *devid)
> +{
> +	struct axp20x_ac_power *power = devid;
> +
> +	dev_warn(&power->supply->dev, "IRQ#%d AC over voltage\n", irq);
> +	power_supply_changed(power->supply);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t axp20x_irq_ac_plugin(int irq, void *devid)
> +{
> +	struct axp20x_ac_power *power = devid;
> +
> +	dev_info(&power->supply->dev, "IRQ#%d AC connected\n", irq);
> +	power_supply_changed(power->supply);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t axp20x_irq_ac_removal(int irq, void *devid)
> +{
> +	struct axp20x_ac_power *power = devid;
> +
> +	dev_info(&power->supply->dev, "IRQ#%d AC disconnected\n", irq);
> +	power_supply_changed(power->supply);
> +
> +	return IRQ_HANDLED;
> +}

Logging in the interrupt handler is usually a bad idea, for several
reasons:
   - If you have a console, it's going to be output on the console,
     which might take quite some time. And you don't want to take
     quite some time in the interrupt handler.
   - printk might not even work in the interrupt context in some
     scenarios.

Removing that handler, you can register the same interrupt handler on
all the interrupts.

> +static int axp20x_ac_power_probe(struct platform_device *pdev)
> +{
> +	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> +	struct power_supply_config psy_cfg = {};
> +	struct axp20x_ac_power *power;
> +	static const char * const irq_names[] = { "ACIN_PLUGIN",
> +		"ACIN_REMOVAL", "ACIN_OVER_V" };
> +	irqreturn_t (*irq_funcs[])(int, void *) = { axp20x_irq_ac_plugin,
> +		axp20x_irq_ac_removal, axp20x_irq_ac_over_v };
> +	int i, irq, r, input;
> +
> +	if (!of_device_is_available(pdev->dev.of_node))
> +		return -ENODEV;

That's useless. If the device is not available, you're not going to be
probed in the first place.

> +	power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
> +	if (!power)
> +		return -ENOMEM;
> +
> +	power->regmap = axp20x->regmap;
> +
> +	r = regmap_read(power->regmap, AXP20X_PWR_INPUT_STATUS, &input);
> +	if (r < 0)
> +		return r;
> +
> +	if (!!(input & AXP20X_PWR_STATUS_AC_VBUS_SHORT)) {
> +		dev_err(&pdev->dev, "AC is connected to VBUS. Use axp20x_usb_power-supply driver instead!");
> +		return -ENODEV;
> +	}

Can't that change over time? Can't we support both drivers at the same time?

> +	/* Enable ac voltage and current measurement */
> +	r = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
> +			AXP20X_ADC_EN1_ACIN_CURR | AXP20X_ADC_EN1_ACIN_VOLT,
> +			AXP20X_ADC_EN1_ACIN_CURR | AXP20X_ADC_EN1_ACIN_VOLT);
> +	if (r)
> +		return r;
> +
> +	psy_cfg.of_node = pdev->dev.of_node;
> +	psy_cfg.drv_data = power;
> +
> +	power->supply = devm_power_supply_register(&pdev->dev,
> +					&axp20x_ac_power_desc, &psy_cfg);
> +	if (IS_ERR(power->supply))
> +		return PTR_ERR(power->supply);
> +
> +	/* Request irqs after registering, as irqs may trigger immediately */
> +	for (i = 0; i < ARRAY_SIZE(irq_names); i++) {
> +		irq = platform_get_irq_byname(pdev, irq_names[i]);
> +		if (irq < 0) {
> +			dev_warn(&pdev->dev, "No IRQ for %s: %d\n",
> +				 irq_names[i], irq);
> +			continue;
> +		}
> +		irq = regmap_irq_get_virq(axp20x->regmap_irqc, irq);
> +		r = devm_request_any_context_irq(&pdev->dev, irq,
> +				irq_funcs[i], 0, DRVNAME, power);
> +		if (r < 0)
> +			dev_warn(&pdev->dev, "Error requesting %s IRQ: %d\n",
> +				 irq_names[i], r);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id axp20x_ac_power_match[] = {
> +	{ .compatible = "x-powers,axp202-ac-power-supply" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, axp20x_ac_power_match);
> +
> +static struct platform_driver axp20x_ac_power_driver = {
> +	.probe = axp20x_ac_power_probe,
> +	.driver = {
> +		.name = DRVNAME,
> +		.of_match_table = axp20x_ac_power_match,
> +	},
> +};
> +
> +module_platform_driver(axp20x_ac_power_driver);
> +
> +MODULE_AUTHOR("Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>");
> +MODULE_DESCRIPTION("AXP20x PMIC AC power supply status driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.8.0

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/4] power: Add an axp20x-ac-power driver

[Michael Haas]

[-- Attachment #1.1: Type: text/plain, Size: 6394 bytes --]

Hi Maxime,

thanks for taking the time to review this.

On 04/04/2016 11:38 PM, Maxime Ripard wrote:
> Hi,
>>
>> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>> index a57d6e9..9351c0e 100644
>> --- a/drivers/mfd/axp20x.c
>> +++ b/drivers/mfd/axp20x.c
>> @@ -178,6 +178,12 @@ static struct resource axp288_power_button_resources[] = {
>>  	},
>>  };
>>  
>> +static struct resource axp20x_ac_power_supply_resources[] = {
>> +	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_PLUGIN, "ACIN_PLUGIN"),
>> +	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_REMOVAL, "ACIN_REMOVAL"),
>> +	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_OVER_V, "ACIN_OVER_V"),
>> +};
>> +
>>  static struct resource axp288_fuel_gauge_resources[] = {
>>  	{
>>  		.start = AXP288_IRQ_QWBTU,
>> @@ -440,6 +446,11 @@ static struct mfd_cell axp20x_cells[] = {
>>  		.of_compatible	= "x-powers,axp202-usb-power-supply",
>>  		.num_resources	= ARRAY_SIZE(axp20x_usb_power_supply_resources),
>>  		.resources	= axp20x_usb_power_supply_resources,
>> +	}, {
>> +		.name		= "axp20x-ac-power-supply",
>> +		.of_compatible	= "x-powers,axp202-ac-power-supply",
>> +		.num_resources	= ARRAY_SIZE(axp20x_ac_power_supply_resources),
>> +		.resources	= axp20x_ac_power_supply_resources,
>>  	},
>>  };
>>
> 
> These changes should be in a separate patch.

Will do!
> 

>> +
>> +static irqreturn_t axp20x_irq_ac_removal(int irq, void *devid)
>> +{
>> +	struct axp20x_ac_power *power = devid;
>> +
>> +	dev_info(&power->supply->dev, "IRQ#%d AC disconnected\n", irq);
>> +	power_supply_changed(power->supply);
>> +
>> +	return IRQ_HANDLED;
>> +}
> 
> Logging in the interrupt handler is usually a bad idea, for several
> reasons:
>    - If you have a console, it's going to be output on the console,
>      which might take quite some time. And you don't want to take
>      quite some time in the interrupt handler.
>    - printk might not even work in the interrupt context in some
>      scenarios.
> 
> Removing that handler, you can register the same interrupt handler on
> all the interrupts.
> 

Oops. Yes, I will fix that!

>> +static int axp20x_ac_power_probe(struct platform_device *pdev)
>> +{
>> +	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
>> +	struct power_supply_config psy_cfg = {};
>> +	struct axp20x_ac_power *power;
>> +	static const char * const irq_names[] = { "ACIN_PLUGIN",
>> +		"ACIN_REMOVAL", "ACIN_OVER_V" };
>> +	irqreturn_t (*irq_funcs[])(int, void *) = { axp20x_irq_ac_plugin,
>> +		axp20x_irq_ac_removal, axp20x_irq_ac_over_v };
>> +	int i, irq, r, input;
>> +
>> +	if (!of_device_is_available(pdev->dev.of_node))
>> +		return -ENODEV;
> 
> That's useless. If the device is not available, you're not going to be
> probed in the first place.

Ok.

> 
>> +	power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
>> +	if (!power)
>> +		return -ENOMEM;
>> +
>> +	power->regmap = axp20x->regmap;
>> +
>> +	r = regmap_read(power->regmap, AXP20X_PWR_INPUT_STATUS, &input);
>> +	if (r < 0)
>> +		return r;
>> +
>> +	if (!!(input & AXP20X_PWR_STATUS_AC_VBUS_SHORT)) {
>> +		dev_err(&pdev->dev, "AC is connected to VBUS. Use axp20x_usb_power-supply driver instead!");
>> +		return -ENODEV;
>> +	}
> 
> Can't that change over time? Can't we support both drivers at the same time?

Both drivers are supported at the same time. I did even try with both
AC and USB connected and both return meaningful values, at least for
voltage. The AXP20x can drawn power from both sources at the same time.

The check above fires in a specific scenario where ACIN and VBUS are
physically connected on the PCB. The Datasheet states for that
particular register:

"Indicates whether ACIN/VBUS short circuits on PCB or not"

So, assuming the chip detects the condition correctly, this does not
change over that. But you can switch from ACIN to VBUS and vice-versa
just fine if they are not short-circuited. If they are connected
together, I'd prefer the DTS to only enable the usb driver. The check
above is a last resort there.

Then again, with the quality of those data sheets, one never knows.

> 
>> +	/* Enable ac voltage and current measurement */
>> +	r = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
>> +			AXP20X_ADC_EN1_ACIN_CURR | AXP20X_ADC_EN1_ACIN_VOLT,
>> +			AXP20X_ADC_EN1_ACIN_CURR | AXP20X_ADC_EN1_ACIN_VOLT);
>> +	if (r)
>> +		return r;
>> +
>> +	psy_cfg.of_node = pdev->dev.of_node;
>> +	psy_cfg.drv_data = power;
>> +
>> +	power->supply = devm_power_supply_register(&pdev->dev,
>> +					&axp20x_ac_power_desc, &psy_cfg);
>> +	if (IS_ERR(power->supply))
>> +		return PTR_ERR(power->supply);
>> +
>> +	/* Request irqs after registering, as irqs may trigger immediately */
>> +	for (i = 0; i < ARRAY_SIZE(irq_names); i++) {
>> +		irq = platform_get_irq_byname(pdev, irq_names[i]);
>> +		if (irq < 0) {
>> +			dev_warn(&pdev->dev, "No IRQ for %s: %d\n",
>> +				 irq_names[i], irq);
>> +			continue;
>> +		}
>> +		irq = regmap_irq_get_virq(axp20x->regmap_irqc, irq);
>> +		r = devm_request_any_context_irq(&pdev->dev, irq,
>> +				irq_funcs[i], 0, DRVNAME, power);
>> +		if (r < 0)
>> +			dev_warn(&pdev->dev, "Error requesting %s IRQ: %d\n",
>> +				 irq_names[i], r);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id axp20x_ac_power_match[] = {
>> +	{ .compatible = "x-powers,axp202-ac-power-supply" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, axp20x_ac_power_match);
>> +
>> +static struct platform_driver axp20x_ac_power_driver = {
>> +	.probe = axp20x_ac_power_probe,
>> +	.driver = {
>> +		.name = DRVNAME,
>> +		.of_match_table = axp20x_ac_power_match,
>> +	},
>> +};
>> +
>> +module_platform_driver(axp20x_ac_power_driver);
>> +
>> +MODULE_AUTHOR("Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>");
>> +MODULE_DESCRIPTION("AXP20x PMIC AC power supply status driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.8.0
> 
> Thanks!
> Maxime
> 


-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/4] power: Add an axp20x-ac-power driver

[Chen-Yu Tsai]

Hi,

On Mon, Apr 4, 2016 at 10:11 PM, Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org> wrote:
> Hi Maxime,
>
> thanks for taking the time to review this.
>
> On 04/04/2016 11:38 PM, Maxime Ripard wrote:
>> Hi,
>>>
>>> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>>> index a57d6e9..9351c0e 100644
>>> --- a/drivers/mfd/axp20x.c
>>> +++ b/drivers/mfd/axp20x.c
>>> @@ -178,6 +178,12 @@ static struct resource axp288_power_button_resources[] = {
>>>      },
>>>  };
>>>
>>> +static struct resource axp20x_ac_power_supply_resources[] = {
>>> +    DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_PLUGIN, "ACIN_PLUGIN"),
>>> +    DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_REMOVAL, "ACIN_REMOVAL"),
>>> +    DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_OVER_V, "ACIN_OVER_V"),
>>> +};
>>> +
>>>  static struct resource axp288_fuel_gauge_resources[] = {
>>>      {
>>>              .start = AXP288_IRQ_QWBTU,
>>> @@ -440,6 +446,11 @@ static struct mfd_cell axp20x_cells[] = {
>>>              .of_compatible  = "x-powers,axp202-usb-power-supply",
>>>              .num_resources  = ARRAY_SIZE(axp20x_usb_power_supply_resources),
>>>              .resources      = axp20x_usb_power_supply_resources,
>>> +    }, {
>>> +            .name           = "axp20x-ac-power-supply",
>>> +            .of_compatible  = "x-powers,axp202-ac-power-supply",
>>> +            .num_resources  = ARRAY_SIZE(axp20x_ac_power_supply_resources),
>>> +            .resources      = axp20x_ac_power_supply_resources,
>>>      },
>>>  };
>>>
>>
>> These changes should be in a separate patch.
>
> Will do!
>>
>
>>> +
>>> +static irqreturn_t axp20x_irq_ac_removal(int irq, void *devid)
>>> +{
>>> +    struct axp20x_ac_power *power = devid;
>>> +
>>> +    dev_info(&power->supply->dev, "IRQ#%d AC disconnected\n", irq);
>>> +    power_supply_changed(power->supply);
>>> +
>>> +    return IRQ_HANDLED;
>>> +}
>>
>> Logging in the interrupt handler is usually a bad idea, for several
>> reasons:
>>    - If you have a console, it's going to be output on the console,
>>      which might take quite some time. And you don't want to take
>>      quite some time in the interrupt handler.
>>    - printk might not even work in the interrupt context in some
>>      scenarios.
>>
>> Removing that handler, you can register the same interrupt handler on
>> all the interrupts.
>>
>
> Oops. Yes, I will fix that!
>
>>> +static int axp20x_ac_power_probe(struct platform_device *pdev)
>>> +{
>>> +    struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
>>> +    struct power_supply_config psy_cfg = {};
>>> +    struct axp20x_ac_power *power;
>>> +    static const char * const irq_names[] = { "ACIN_PLUGIN",
>>> +            "ACIN_REMOVAL", "ACIN_OVER_V" };
>>> +    irqreturn_t (*irq_funcs[])(int, void *) = { axp20x_irq_ac_plugin,
>>> +            axp20x_irq_ac_removal, axp20x_irq_ac_over_v };
>>> +    int i, irq, r, input;
>>> +
>>> +    if (!of_device_is_available(pdev->dev.of_node))
>>> +            return -ENODEV;
>>
>> That's useless. If the device is not available, you're not going to be
>> probed in the first place.
>
> Ok.
>
>>
>>> +    power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
>>> +    if (!power)
>>> +            return -ENOMEM;
>>> +
>>> +    power->regmap = axp20x->regmap;
>>> +
>>> +    r = regmap_read(power->regmap, AXP20X_PWR_INPUT_STATUS, &input);
>>> +    if (r < 0)
>>> +            return r;
>>> +
>>> +    if (!!(input & AXP20X_PWR_STATUS_AC_VBUS_SHORT)) {
>>> +            dev_err(&pdev->dev, "AC is connected to VBUS. Use axp20x_usb_power-supply driver instead!");
>>> +            return -ENODEV;
>>> +    }
>>
>> Can't that change over time? Can't we support both drivers at the same time?
>
> Both drivers are supported at the same time. I did even try with both
> AC and USB connected and both return meaningful values, at least for
> voltage. The AXP20x can drawn power from both sources at the same time.
>
> The check above fires in a specific scenario where ACIN and VBUS are
> physically connected on the PCB. The Datasheet states for that
> particular register:
>
> "Indicates whether ACIN/VBUS short circuits on PCB or not"
>
> So, assuming the chip detects the condition correctly, this does not
> change over that. But you can switch from ACIN to VBUS and vice-versa
> just fine if they are not short-circuited. If they are connected
> together, I'd prefer the DTS to only enable the usb driver. The check
> above is a last resort there.
>
> Then again, with the quality of those data sheets, one never knows.

If they are short circuited, does that mean the PMIC will only
draw power from one source? If both are still used, then the
current and voltage readings still make sense. Then it makes
sense to have both drivers enabled, no?

Or would something bad happen?

ChenYu

>>
>>> +    /* Enable ac voltage and current measurement */
>>> +    r = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
>>> +                    AXP20X_ADC_EN1_ACIN_CURR | AXP20X_ADC_EN1_ACIN_VOLT,
>>> +                    AXP20X_ADC_EN1_ACIN_CURR | AXP20X_ADC_EN1_ACIN_VOLT);
>>> +    if (r)
>>> +            return r;
>>> +
>>> +    psy_cfg.of_node = pdev->dev.of_node;
>>> +    psy_cfg.drv_data = power;
>>> +
>>> +    power->supply = devm_power_supply_register(&pdev->dev,
>>> +                                    &axp20x_ac_power_desc, &psy_cfg);
>>> +    if (IS_ERR(power->supply))
>>> +            return PTR_ERR(power->supply);
>>> +
>>> +    /* Request irqs after registering, as irqs may trigger immediately */
>>> +    for (i = 0; i < ARRAY_SIZE(irq_names); i++) {
>>> +            irq = platform_get_irq_byname(pdev, irq_names[i]);
>>> +            if (irq < 0) {
>>> +                    dev_warn(&pdev->dev, "No IRQ for %s: %d\n",
>>> +                             irq_names[i], irq);
>>> +                    continue;
>>> +            }
>>> +            irq = regmap_irq_get_virq(axp20x->regmap_irqc, irq);
>>> +            r = devm_request_any_context_irq(&pdev->dev, irq,
>>> +                            irq_funcs[i], 0, DRVNAME, power);
>>> +            if (r < 0)
>>> +                    dev_warn(&pdev->dev, "Error requesting %s IRQ: %d\n",
>>> +                             irq_names[i], r);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct of_device_id axp20x_ac_power_match[] = {
>>> +    { .compatible = "x-powers,axp202-ac-power-supply" },
>>> +    { }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, axp20x_ac_power_match);
>>> +
>>> +static struct platform_driver axp20x_ac_power_driver = {
>>> +    .probe = axp20x_ac_power_probe,
>>> +    .driver = {
>>> +            .name = DRVNAME,
>>> +            .of_match_table = axp20x_ac_power_match,
>>> +    },
>>> +};
>>> +
>>> +module_platform_driver(axp20x_ac_power_driver);
>>> +
>>> +MODULE_AUTHOR("Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>");
>>> +MODULE_DESCRIPTION("AXP20x PMIC AC power supply status driver");
>>> +MODULE_LICENSE("GPL");
>>> --
>>> 2.8.0
>>
>> Thanks!
>> Maxime
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: Re: [PATCH 1/4] power: Add an axp20x-ac-power driver

[Michael Haas]

On 04/05/2016 08:05 AM, Chen-Yu Tsai wrote:
> Hi,
> 
> On Mon, Apr 4, 2016 at 10:11 PM, Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org> wrote:
>> Hi Maxime,
>>
>> thanks for taking the time to review this.
>>
>> On 04/04/2016 11:38 PM, Maxime Ripard wrote:
>>> Hi,
>>>>
>>>> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>>>> index a57d6e9..9351c0e 100644
>>>> --- a/drivers/mfd/axp20x.c
>>>> +++ b/drivers/mfd/axp20x.c
>>>> @@ -178,6 +178,12 @@ static struct resource axp288_power_button_resources[] = {
>>>>      },
>>>>  };
>>>>
>>>> +static struct resource axp20x_ac_power_supply_resources[] = {
>>>> +    DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_PLUGIN, "ACIN_PLUGIN"),
>>>> +    DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_REMOVAL, "ACIN_REMOVAL"),
>>>> +    DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_OVER_V, "ACIN_OVER_V"),
>>>> +};
>>>> +
>>>>  static struct resource axp288_fuel_gauge_resources[] = {
>>>>      {
>>>>              .start = AXP288_IRQ_QWBTU,
>>>> @@ -440,6 +446,11 @@ static struct mfd_cell axp20x_cells[] = {
>>>>              .of_compatible  = "x-powers,axp202-usb-power-supply",
>>>>              .num_resources  = ARRAY_SIZE(axp20x_usb_power_supply_resources),
>>>>              .resources      = axp20x_usb_power_supply_resources,
>>>> +    }, {
>>>> +            .name           = "axp20x-ac-power-supply",
>>>> +            .of_compatible  = "x-powers,axp202-ac-power-supply",
>>>> +            .num_resources  = ARRAY_SIZE(axp20x_ac_power_supply_resources),
>>>> +            .resources      = axp20x_ac_power_supply_resources,
>>>>      },
>>>>  };
>>>>
>>>
>>> These changes should be in a separate patch.
>>
>> Will do!
>>>
>>
>>>> +
>>>> +static irqreturn_t axp20x_irq_ac_removal(int irq, void *devid)
>>>> +{
>>>> +    struct axp20x_ac_power *power = devid;
>>>> +
>>>> +    dev_info(&power->supply->dev, "IRQ#%d AC disconnected\n", irq);
>>>> +    power_supply_changed(power->supply);
>>>> +
>>>> +    return IRQ_HANDLED;
>>>> +}
>>>
>>> Logging in the interrupt handler is usually a bad idea, for several
>>> reasons:
>>>    - If you have a console, it's going to be output on the console,
>>>      which might take quite some time. And you don't want to take
>>>      quite some time in the interrupt handler.
>>>    - printk might not even work in the interrupt context in some
>>>      scenarios.
>>>
>>> Removing that handler, you can register the same interrupt handler on
>>> all the interrupts.
>>>
>>
>> Oops. Yes, I will fix that!
>>
>>>> +static int axp20x_ac_power_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
>>>> +    struct power_supply_config psy_cfg = {};
>>>> +    struct axp20x_ac_power *power;
>>>> +    static const char * const irq_names[] = { "ACIN_PLUGIN",
>>>> +            "ACIN_REMOVAL", "ACIN_OVER_V" };
>>>> +    irqreturn_t (*irq_funcs[])(int, void *) = { axp20x_irq_ac_plugin,
>>>> +            axp20x_irq_ac_removal, axp20x_irq_ac_over_v };
>>>> +    int i, irq, r, input;
>>>> +
>>>> +    if (!of_device_is_available(pdev->dev.of_node))
>>>> +            return -ENODEV;
>>>
>>> That's useless. If the device is not available, you're not going to be
>>> probed in the first place.
>>
>> Ok.
>>
>>>
>>>> +    power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
>>>> +    if (!power)
>>>> +            return -ENOMEM;
>>>> +
>>>> +    power->regmap = axp20x->regmap;
>>>> +
>>>> +    r = regmap_read(power->regmap, AXP20X_PWR_INPUT_STATUS, &input);
>>>> +    if (r < 0)
>>>> +            return r;
>>>> +
>>>> +    if (!!(input & AXP20X_PWR_STATUS_AC_VBUS_SHORT)) {
>>>> +            dev_err(&pdev->dev, "AC is connected to VBUS. Use axp20x_usb_power-supply driver instead!");
>>>> +            return -ENODEV;
>>>> +    }
>>>
>>> Can't that change over time? Can't we support both drivers at the same time?
>>
>> Both drivers are supported at the same time. I did even try with both
>> AC and USB connected and both return meaningful values, at least for
>> voltage. The AXP20x can drawn power from both sources at the same time.
>>
>> The check above fires in a specific scenario where ACIN and VBUS are
>> physically connected on the PCB. The Datasheet states for that
>> particular register:
>>
>> "Indicates whether ACIN/VBUS short circuits on PCB or not"
>>
>> So, assuming the chip detects the condition correctly, this does not
>> change over that. But you can switch from ACIN to VBUS and vice-versa
>> just fine if they are not short-circuited. If they are connected
>> together, I'd prefer the DTS to only enable the usb driver. The check
>> above is a last resort there.
>>
>> Then again, with the quality of those data sheets, one never knows.
> 
> If they are short circuited, does that mean the PMIC will only
> draw power from one source? If both are still used, then the
> current and voltage readings still make sense. Then it makes
> sense to have both drivers enabled, no?
> 
> Or would something bad happen?
> 
> ChenYu
> 
>>>

Hi ChenYu,

I can think of two possible cases here where ACIN/VBUS are
short-circuited on the PCB.

1) Only one of ACIN xor VBUS can be physically connected and the AXP209
cannot distinguish between the two. Since there is only one input, it
makes sense to only expose one power supply node. Here, we don't
necessarily know which one.
2) Both ACIN and VBUS are connected but the AXP209 cannot distinguish
one from the other. The AXP209 may still choose to draw power over both
its ACIN and VBUS pins so we may get two readings. The sum of both
readings then yields the total power consumption of the system, even if
we don't know the individual contribution of ACIN or VBUS.

I just went over the data sheet again and I did not find a further
clarification of the short-circuit register. But I get your point now:
as I described above, we currently have no way of knowing if only one
(which one?) or both inputs will be used on the chip even if the short
circuit condition is detected.

I guess I could take a look at the driver provided by Allwinner
themselves in their old kernel drop. But for now, it may make sense to
ignore the condition alltogether.

Thanks for your input!

Michael

Re: [PATCH 3/4] ARM: dts: Add binding documentation for AXP20x pmic ac power supply

[Rob Herring]

On Sun, Apr 03, 2016 at 03:15:06PM +0200, Michael Haas wrote:
> Add binding documentation for the ac power supply part of the AXP20x
> pmic.
> 
> Signed-off-by: Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org>
> ---
>  .../bindings/power_supply/axp20x_ac_power.txt      | 34 ++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power_supply/axp20x_ac_power.txt

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>