[PATCH v3 1/4] ARM: dts: Add binding documentation for AXP20x pmic usb power supply

[PATCH v3 1/4] ARM: dts: Add binding documentation for AXP20x pmic usb power supply

[Hans de Goede]

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

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Split out into a separate patch from the actual driver commit
---
 .../bindings/power_supply/axp20x_usb_power.txt     | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power_supply/axp20x_usb_power.txt

diff --git a/Documentation/devicetree/bindings/power_supply/axp20x_usb_power.txt b/Documentation/devicetree/bindings/power_supply/axp20x_usb_power.txt
new file mode 100644
index 0000000..a84d801
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_supply/axp20x_usb_power.txt
@@ -0,0 +1,34 @@
+AXP20x USB power supply
+
+Required Properties:
+-compatible: "x-powers,axp202-usb-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";
+		};
+
+		...
+	};
+
+	usb_power_supply: usb_power_supply {
+		compatible = "x-powers,axp202-usb-power-supply";
+	};
+};
-- 
2.4.3

[PATCH v3 1/4] ARM: dts: Add binding documentation for AXP20x pmic usb power supply

[Hans de Goede]

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

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Split out into a separate patch from the actual driver commit
---
 .../bindings/power_supply/axp20x_usb_power.txt     | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power_supply/axp20x_usb_power.txt

diff --git a/Documentation/devicetree/bindings/power_supply/axp20x_usb_power.txt b/Documentation/devicetree/bindings/power_supply/axp20x_usb_power.txt
new file mode 100644
index 0000000..a84d801
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_supply/axp20x_usb_power.txt
@@ -0,0 +1,34 @@
+AXP20x USB power supply
+
+Required Properties:
+-compatible: "x-powers,axp202-usb-power-supply"
+
+This node is a subnode of the axp20x PMIC.
+
+Example:
+
+axp209: pmic at 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";
+		};
+
+		...
+	};
+
+	usb_power_supply: usb_power_supply {
+		compatible = "x-powers,axp202-usb-power-supply";
+	};
+};
-- 
2.4.3

[PATCH v3 2/4] mfd: axp20x: Add missing registers, and mark more registers volatile

[Hans de Goede]

From: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>

Add an extra set of registers which is necessary tu support the PMICs
battery charger function, and mark registers which contain status bits,
gpio status, and adc readings as volatile.

Cc: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
Signed-off-by: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
Changes in v2:
-Add a AXP20X_OCV_MAX define
Changes in v3:
-Add Bruno's S-o-b
---
 drivers/mfd/axp20x.c       | 8 +++++++-
 include/linux/mfd/axp20x.h | 6 ++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index 6df9155..f9a3c2d 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -39,10 +39,16 @@ static const char * const axp20x_model_names[] = {
 static const struct regmap_range axp20x_writeable_ranges[] = {
 	regmap_reg_range(AXP20X_DATACACHE(0), AXP20X_IRQ5_STATE),
 	regmap_reg_range(AXP20X_DCDC_MODE, AXP20X_FG_RES),
+	regmap_reg_range(AXP20X_RDC_H, AXP20X_OCV(15)),
 };
 
 static const struct regmap_range axp20x_volatile_ranges[] = {
+	regmap_reg_range(AXP20X_PWR_INPUT_STATUS, AXP20X_USB_OTG_STATUS),
+	regmap_reg_range(AXP20X_CHRG_CTRL1, AXP20X_CHRG_CTRL2),
 	regmap_reg_range(AXP20X_IRQ1_EN, AXP20X_IRQ5_STATE),
+	regmap_reg_range(AXP20X_ACIN_V_ADC_H, AXP20X_IPSOUT_V_HIGH_L),
+	regmap_reg_range(AXP20X_GPIO20_SS, AXP20X_GPIO3_CTRL),
+	regmap_reg_range(AXP20X_FG_RES, AXP20X_RDC_L),
 };
 
 static const struct regmap_access_table axp20x_writeable_table = {
@@ -159,7 +165,7 @@ static const struct regmap_config axp20x_regmap_config = {
 	.val_bits	= 8,
 	.wr_table	= &axp20x_writeable_table,
 	.volatile_table	= &axp20x_volatile_table,
-	.max_register	= AXP20X_FG_RES,
+	.max_register	= AXP20X_OCV(AXP20X_OCV_MAX),
 	.cache_type	= REGCACHE_RBTREE,
 };
 
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index 5275423..6d8b39a 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -151,6 +151,12 @@ enum {
 #define AXP20X_CC_CTRL			0xb8
 #define AXP20X_FG_RES			0xb9
 
+/* OCV */
+#define AXP20X_RDC_H			0xba
+#define AXP20X_RDC_L			0xbb
+#define AXP20X_OCV(m)			(0xc0 + (m))
+#define AXP20X_OCV_MAX			0xf
+
 /* AXP22X specific registers */
 #define AXP22X_BATLOW_THRES1		0xe6
 
-- 
2.4.3

-- 
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 v3 2/4] mfd: axp20x: Add missing registers, and mark more registers volatile

[Hans de Goede]

From: Bruno Pr?mont <bonbons@linux-vserver.org>

Add an extra set of registers which is necessary tu support the PMICs
battery charger function, and mark registers which contain status bits,
gpio status, and adc readings as volatile.

Cc: Bruno Pr?mont <bonbons@linux-vserver.org>
Signed-off-by: Bruno Pr?mont <bonbons@linux-vserver.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Add a AXP20X_OCV_MAX define
Changes in v3:
-Add Bruno's S-o-b
---
 drivers/mfd/axp20x.c       | 8 +++++++-
 include/linux/mfd/axp20x.h | 6 ++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index 6df9155..f9a3c2d 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -39,10 +39,16 @@ static const char * const axp20x_model_names[] = {
 static const struct regmap_range axp20x_writeable_ranges[] = {
 	regmap_reg_range(AXP20X_DATACACHE(0), AXP20X_IRQ5_STATE),
 	regmap_reg_range(AXP20X_DCDC_MODE, AXP20X_FG_RES),
+	regmap_reg_range(AXP20X_RDC_H, AXP20X_OCV(15)),
 };
 
 static const struct regmap_range axp20x_volatile_ranges[] = {
+	regmap_reg_range(AXP20X_PWR_INPUT_STATUS, AXP20X_USB_OTG_STATUS),
+	regmap_reg_range(AXP20X_CHRG_CTRL1, AXP20X_CHRG_CTRL2),
 	regmap_reg_range(AXP20X_IRQ1_EN, AXP20X_IRQ5_STATE),
+	regmap_reg_range(AXP20X_ACIN_V_ADC_H, AXP20X_IPSOUT_V_HIGH_L),
+	regmap_reg_range(AXP20X_GPIO20_SS, AXP20X_GPIO3_CTRL),
+	regmap_reg_range(AXP20X_FG_RES, AXP20X_RDC_L),
 };
 
 static const struct regmap_access_table axp20x_writeable_table = {
@@ -159,7 +165,7 @@ static const struct regmap_config axp20x_regmap_config = {
 	.val_bits	= 8,
 	.wr_table	= &axp20x_writeable_table,
 	.volatile_table	= &axp20x_volatile_table,
-	.max_register	= AXP20X_FG_RES,
+	.max_register	= AXP20X_OCV(AXP20X_OCV_MAX),
 	.cache_type	= REGCACHE_RBTREE,
 };
 
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index 5275423..6d8b39a 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -151,6 +151,12 @@ enum {
 #define AXP20X_CC_CTRL			0xb8
 #define AXP20X_FG_RES			0xb9
 
+/* OCV */
+#define AXP20X_RDC_H			0xba
+#define AXP20X_RDC_L			0xbb
+#define AXP20X_OCV(m)			(0xc0 + (m))
+#define AXP20X_OCV_MAX			0xf
+
 /* AXP22X specific registers */
 #define AXP22X_BATLOW_THRES1		0xe6
 
-- 
2.4.3

[PATCH v3 3/4] mfd: axp20x: Add a cell for the usb power_supply part of the axp20x PMICs

[Hans de Goede]

Add a cell for the usb power_supply part of the axp20x PMICs.

Note that this cell is only for the usb power_supply part and not the
ac-power / battery-charger / rtc-backup-bat-charger bits.

Depending on the board each of those must be enabled / disabled separately
in devicetree as most boards do not use all 4. So in dt each one needs its
own child-node of the axp20x node. Another reason for using separate child
nodes for each is so that other devicetree nodes can have a power-supply
property with a phandle referencing a node representing a single
power-supply.

The decision to use a separate devicetree node for each is reflected on
the kernel side by each getting its own mfd-cell / platform_device and
platform-driver.

Note this commit also makes some whitespace changes to the intialization
of existing cells in axp20x_cells, these are pure whitespace changes,
functionally nothing changes.

Cc: Bruno Prémont <bonbons@linux-vserver.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Use DEFINE_RES_IRQ_NAMED
-Change indentation of axp20x_cells initializers to avoid line wrapping
Changes in v3:
-Improve commit message
-Add Bruno's S-o-b
---
 drivers/mfd/axp20x.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index f9a3c2d..ca4a604 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -113,6 +113,13 @@ static struct resource axp20x_pek_resources[] = {
 	},
 };
 
+static struct resource axp20x_usb_power_supply_resources[] = {
+	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_PLUGIN, "VBUS_PLUGIN"),
+	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_REMOVAL, "VBUS_REMOVAL"),
+	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_VALID, "VBUS_VALID"),
+	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_NOT_VALID, "VBUS_NOT_VALID"),
+};
+
 static struct resource axp22x_pek_resources[] = {
 	{
 		.name   = "PEK_DBR",
@@ -363,11 +370,16 @@ static const struct regmap_irq_chip axp288_regmap_irq_chip = {
 
 static struct mfd_cell axp20x_cells[] = {
 	{
-		.name			= "axp20x-pek",
-		.num_resources		= ARRAY_SIZE(axp20x_pek_resources),
-		.resources		= axp20x_pek_resources,
+		.name		= "axp20x-pek",
+		.num_resources	= ARRAY_SIZE(axp20x_pek_resources),
+		.resources	= axp20x_pek_resources,
 	}, {
-		.name			= "axp20x-regulator",
+		.name		= "axp20x-regulator",
+	}, {
+		.name		= "axp20x-usb-power-supply",
+		.of_compatible	= "x-powers,axp202-usb-power-supply",
+		.num_resources	= ARRAY_SIZE(axp20x_usb_power_supply_resources),
+		.resources	= axp20x_usb_power_supply_resources,
 	},
 };
 
-- 
2.4.3

[PATCH v3 3/4] mfd: axp20x: Add a cell for the usb power_supply part of the axp20x PMICs

[Hans de Goede]

Add a cell for the usb power_supply part of the axp20x PMICs.

Note that this cell is only for the usb power_supply part and not the
ac-power / battery-charger / rtc-backup-bat-charger bits.

Depending on the board each of those must be enabled / disabled separately
in devicetree as most boards do not use all 4. So in dt each one needs its
own child-node of the axp20x node. Another reason for using separate child
nodes for each is so that other devicetree nodes can have a power-supply
property with a phandle referencing a node representing a single
power-supply.

The decision to use a separate devicetree node for each is reflected on
the kernel side by each getting its own mfd-cell / platform_device and
platform-driver.

Note this commit also makes some whitespace changes to the intialization
of existing cells in axp20x_cells, these are pure whitespace changes,
functionally nothing changes.

Cc: Bruno Pr?mont <bonbons@linux-vserver.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Bruno Pr?mont <bonbons@linux-vserver.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Use DEFINE_RES_IRQ_NAMED
-Change indentation of axp20x_cells initializers to avoid line wrapping
Changes in v3:
-Improve commit message
-Add Bruno's S-o-b
---
 drivers/mfd/axp20x.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index f9a3c2d..ca4a604 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -113,6 +113,13 @@ static struct resource axp20x_pek_resources[] = {
 	},
 };
 
+static struct resource axp20x_usb_power_supply_resources[] = {
+	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_PLUGIN, "VBUS_PLUGIN"),
+	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_REMOVAL, "VBUS_REMOVAL"),
+	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_VALID, "VBUS_VALID"),
+	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_NOT_VALID, "VBUS_NOT_VALID"),
+};
+
 static struct resource axp22x_pek_resources[] = {
 	{
 		.name   = "PEK_DBR",
@@ -363,11 +370,16 @@ static const struct regmap_irq_chip axp288_regmap_irq_chip = {
 
 static struct mfd_cell axp20x_cells[] = {
 	{
-		.name			= "axp20x-pek",
-		.num_resources		= ARRAY_SIZE(axp20x_pek_resources),
-		.resources		= axp20x_pek_resources,
+		.name		= "axp20x-pek",
+		.num_resources	= ARRAY_SIZE(axp20x_pek_resources),
+		.resources	= axp20x_pek_resources,
 	}, {
-		.name			= "axp20x-regulator",
+		.name		= "axp20x-regulator",
+	}, {
+		.name		= "axp20x-usb-power-supply",
+		.of_compatible	= "x-powers,axp202-usb-power-supply",
+		.num_resources	= ARRAY_SIZE(axp20x_usb_power_supply_resources),
+		.resources	= axp20x_usb_power_supply_resources,
 	},
 };
 
-- 
2.4.3

[PATCH v3 4/4] power: Add an axp20x-usb-power driver

[Hans de Goede]

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

I initially started writing my own driver, before coming aware of
Bruno Prémont's excellent earlier RFC with a driver for this.

My driver was lacking CURRENT_MAX and VOLTAGE_MIN support Bruno's
drvier has, so I've copied the code for those from his driver.

Note that the AC-power-supply and battery charger bits will need separate
drivers. Each one needs its own devictree child-node so that other
devicetree nodes can reference the right power-supply, and thus each one
will get its own mfd-cell / platform_device and platform-driver.

Cc: Bruno Prémont <bonbons@linux-vserver.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Split out the dt-bindings documentation into a separate patch
-Renamed axp20x_read_16bit to axp20x_read_variable_width
-Use better local variable names inside axp20x_read_variable_width
Changes in v3:
-Add Bruno's S-o-b
---
 drivers/power/Kconfig            |   7 ++
 drivers/power/Makefile           |   1 +
 drivers/power/axp20x_usb_power.c | 243 +++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/axp20x.h       |  24 ++++
 4 files changed, 275 insertions(+)
 create mode 100644 drivers/power/axp20x_usb_power.c

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 4091fb0..1fee60c 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -439,6 +439,13 @@ config BATTERY_RT5033
 	  The fuelgauge calculates and determines the battery state of charge
 	  according to battery open circuit voltage.
 
+config AXP20X_POWER
+	tristate "AXP20x power supply driver"
+	depends on MFD_AXP20X
+	help
+	  This driver provides support for the power supply features of
+	  AXP20x PMIC.
+
 source "drivers/power/reset/Kconfig"
 
 endif # POWER_SUPPLY
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index b7b0181..ae0f27d 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -9,6 +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_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_usb_power.c b/drivers/power/axp20x_usb_power.c
new file mode 100644
index 0000000..09f388e
--- /dev/null
+++ b/drivers/power/axp20x_usb_power.c
@@ -0,0 +1,243 @@
+/*
+ * AXP20x PMIC USB power supply status driver
+ *
+ * Copyright (C) 2015 Hans de Goede <hdegoede@redhat.com>
+ * Copyright (C) 2014 Bruno Prémont <bonbons@linux-vserver.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/device.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.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-usb-power-supply"
+
+#define AXP20X_PWR_STATUS_VBUS_PRESENT	BIT(5)
+#define AXP20X_PWR_STATUS_VBUS_USED	BIT(4)
+
+#define AXP20X_USB_STATUS_VBUS_VALID	BIT(2)
+
+#define AXP20X_VBUS_VHOLD_uV(b)		(4000000 + (((b) >> 3) & 7) * 100000)
+#define AXP20X_VBUS_CLIMIT_MASK		3
+#define AXP20X_VBUC_CLIMIT_900mA	0
+#define AXP20X_VBUC_CLIMIT_500mA	1
+#define AXP20X_VBUC_CLIMIT_100mA	2
+#define AXP20X_VBUC_CLIMIT_NONE		3
+
+#define AXP20X_ADC_EN1_VBUS_CURR	BIT(2)
+#define AXP20X_ADC_EN1_VBUS_VOLT	BIT(3)
+
+#define AXP20X_VBUS_MON_VBUS_VALID	BIT(3)
+
+struct axp20x_usb_power {
+	struct regmap *regmap;
+	struct power_supply *supply;
+};
+
+static irqreturn_t axp20x_usb_power_irq(int irq, void *devid)
+{
+	struct axp20x_usb_power *power = devid;
+
+	power_supply_changed(power->supply);
+
+	return IRQ_HANDLED;
+}
+
+static int axp20x_usb_power_get_property(struct power_supply *psy,
+	enum power_supply_property psp, union power_supply_propval *val)
+{
+	struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
+	unsigned int input, v;
+	int r;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
+		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
+		if (r)
+			return r;
+
+		val->intval = AXP20X_VBUS_VHOLD_uV(v);
+		return 0;
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		r = axp20x_read_variable_width(power->regmap,
+					       AXP20X_VBUS_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_MAX:
+		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
+		if (r)
+			return r;
+
+		switch (v & AXP20X_VBUS_CLIMIT_MASK) {
+		case AXP20X_VBUC_CLIMIT_100mA:
+			val->intval = 100000;
+			break;
+		case AXP20X_VBUC_CLIMIT_500mA:
+			val->intval = 500000;
+			break;
+		case AXP20X_VBUC_CLIMIT_900mA:
+			val->intval = 900000;
+			break;
+		case AXP20X_VBUC_CLIMIT_NONE:
+			val->intval = -1;
+			break;
+		}
+		return 0;
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+		r = axp20x_read_variable_width(power->regmap,
+					       AXP20X_VBUS_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_HEALTH:
+		if (!(input & AXP20X_PWR_STATUS_VBUS_PRESENT)) {
+			val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
+			break;
+		}
+
+		r = regmap_read(power->regmap, AXP20X_USB_OTG_STATUS, &v);
+		if (r)
+			return r;
+
+		if (!(v & AXP20X_USB_STATUS_VBUS_VALID)) {
+			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
+			break;
+		}
+
+		val->intval = POWER_SUPPLY_HEALTH_GOOD;
+		break;
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_PRESENT);
+		break;
+	case POWER_SUPPLY_PROP_ONLINE:
+		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_USED);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static enum power_supply_property axp20x_usb_power_properties[] = {
+	POWER_SUPPLY_PROP_HEALTH,
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_VOLTAGE_MIN,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_CURRENT_MAX,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+};
+
+static const struct power_supply_desc axp20x_usb_power_desc = {
+	.name = "axp20x-usb",
+	.type = POWER_SUPPLY_TYPE_USB,
+	.properties = axp20x_usb_power_properties,
+	.num_properties = ARRAY_SIZE(axp20x_usb_power_properties),
+	.get_property = axp20x_usb_power_get_property,
+};
+
+static int axp20x_usb_power_probe(struct platform_device *pdev)
+{
+	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
+	struct power_supply_config psy_cfg = {};
+	struct axp20x_usb_power *power;
+	static const char * const irq_names[] = { "VBUS_PLUGIN",
+		"VBUS_REMOVAL", "VBUS_VALID", "VBUS_NOT_VALID" };
+	int i, irq, r;
+
+	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;
+
+	/* Enable vbus valid checking */
+	r = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
+		    AXP20X_VBUS_MON_VBUS_VALID, AXP20X_VBUS_MON_VBUS_VALID);
+	if (r)
+		return r;
+
+	/* Enable vbus voltage and current measurement */
+	r = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
+			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_VOLT,
+			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_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_usb_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,
+				axp20x_usb_power_irq, 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_usb_power_match[] = {
+	{ .compatible = "x-powers,axp202-usb-power-supply" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, axp20x_usb_power_match);
+
+static struct platform_driver axp20x_usb_power_driver = {
+	.probe = axp20x_usb_power_probe,
+	.driver = {
+		.name = DRVNAME,
+		.of_match_table = axp20x_usb_power_match,
+	},
+};
+
+module_platform_driver(axp20x_usb_power_driver);
+
+MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
+MODULE_DESCRIPTION("AXP20x PMIC USB power supply status driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index 6d8b39a..8ec996e 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -11,6 +11,8 @@
 #ifndef __LINUX_MFD_AXP20X_H
 #define __LINUX_MFD_AXP20X_H
 
+#include <linux/regmap.h>
+
 enum {
 	AXP202_ID = 0,
 	AXP209_ID,
@@ -372,4 +374,26 @@ struct axp288_extcon_pdata {
 	struct gpio_desc *gpio_mux_cntl;
 };
 
+/* generic helper function for reading 9-16 bit wide regs */
+static inline int axp20x_read_variable_width(struct regmap *regmap,
+	unsigned int reg, unsigned int width)
+{
+	unsigned int reg_val, result;
+	int err;
+
+	err = regmap_read(regmap, reg, &reg_val);
+	if (err)
+		return err;
+
+	result = reg_val << (width - 8);
+
+	err = regmap_read(regmap, reg + 1, &reg_val);
+	if (err)
+		return err;
+
+	result |= reg_val;
+
+	return result;
+}
+
 #endif /* __LINUX_MFD_AXP20X_H */
-- 
2.4.3

[PATCH v3 4/4] power: Add an axp20x-usb-power driver

[Hans de Goede]

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

I initially started writing my own driver, before coming aware of
Bruno Pr?mont's excellent earlier RFC with a driver for this.

My driver was lacking CURRENT_MAX and VOLTAGE_MIN support Bruno's
drvier has, so I've copied the code for those from his driver.

Note that the AC-power-supply and battery charger bits will need separate
drivers. Each one needs its own devictree child-node so that other
devicetree nodes can reference the right power-supply, and thus each one
will get its own mfd-cell / platform_device and platform-driver.

Cc: Bruno Pr?mont <bonbons@linux-vserver.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Bruno Pr?mont <bonbons@linux-vserver.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Split out the dt-bindings documentation into a separate patch
-Renamed axp20x_read_16bit to axp20x_read_variable_width
-Use better local variable names inside axp20x_read_variable_width
Changes in v3:
-Add Bruno's S-o-b
---
 drivers/power/Kconfig            |   7 ++
 drivers/power/Makefile           |   1 +
 drivers/power/axp20x_usb_power.c | 243 +++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/axp20x.h       |  24 ++++
 4 files changed, 275 insertions(+)
 create mode 100644 drivers/power/axp20x_usb_power.c

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 4091fb0..1fee60c 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -439,6 +439,13 @@ config BATTERY_RT5033
 	  The fuelgauge calculates and determines the battery state of charge
 	  according to battery open circuit voltage.
 
+config AXP20X_POWER
+	tristate "AXP20x power supply driver"
+	depends on MFD_AXP20X
+	help
+	  This driver provides support for the power supply features of
+	  AXP20x PMIC.
+
 source "drivers/power/reset/Kconfig"
 
 endif # POWER_SUPPLY
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index b7b0181..ae0f27d 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -9,6 +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_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_usb_power.c b/drivers/power/axp20x_usb_power.c
new file mode 100644
index 0000000..09f388e
--- /dev/null
+++ b/drivers/power/axp20x_usb_power.c
@@ -0,0 +1,243 @@
+/*
+ * AXP20x PMIC USB power supply status driver
+ *
+ * Copyright (C) 2015 Hans de Goede <hdegoede@redhat.com>
+ * Copyright (C) 2014 Bruno Pr?mont <bonbons@linux-vserver.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/device.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.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-usb-power-supply"
+
+#define AXP20X_PWR_STATUS_VBUS_PRESENT	BIT(5)
+#define AXP20X_PWR_STATUS_VBUS_USED	BIT(4)
+
+#define AXP20X_USB_STATUS_VBUS_VALID	BIT(2)
+
+#define AXP20X_VBUS_VHOLD_uV(b)		(4000000 + (((b) >> 3) & 7) * 100000)
+#define AXP20X_VBUS_CLIMIT_MASK		3
+#define AXP20X_VBUC_CLIMIT_900mA	0
+#define AXP20X_VBUC_CLIMIT_500mA	1
+#define AXP20X_VBUC_CLIMIT_100mA	2
+#define AXP20X_VBUC_CLIMIT_NONE		3
+
+#define AXP20X_ADC_EN1_VBUS_CURR	BIT(2)
+#define AXP20X_ADC_EN1_VBUS_VOLT	BIT(3)
+
+#define AXP20X_VBUS_MON_VBUS_VALID	BIT(3)
+
+struct axp20x_usb_power {
+	struct regmap *regmap;
+	struct power_supply *supply;
+};
+
+static irqreturn_t axp20x_usb_power_irq(int irq, void *devid)
+{
+	struct axp20x_usb_power *power = devid;
+
+	power_supply_changed(power->supply);
+
+	return IRQ_HANDLED;
+}
+
+static int axp20x_usb_power_get_property(struct power_supply *psy,
+	enum power_supply_property psp, union power_supply_propval *val)
+{
+	struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
+	unsigned int input, v;
+	int r;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
+		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
+		if (r)
+			return r;
+
+		val->intval = AXP20X_VBUS_VHOLD_uV(v);
+		return 0;
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		r = axp20x_read_variable_width(power->regmap,
+					       AXP20X_VBUS_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_MAX:
+		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
+		if (r)
+			return r;
+
+		switch (v & AXP20X_VBUS_CLIMIT_MASK) {
+		case AXP20X_VBUC_CLIMIT_100mA:
+			val->intval = 100000;
+			break;
+		case AXP20X_VBUC_CLIMIT_500mA:
+			val->intval = 500000;
+			break;
+		case AXP20X_VBUC_CLIMIT_900mA:
+			val->intval = 900000;
+			break;
+		case AXP20X_VBUC_CLIMIT_NONE:
+			val->intval = -1;
+			break;
+		}
+		return 0;
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+		r = axp20x_read_variable_width(power->regmap,
+					       AXP20X_VBUS_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_HEALTH:
+		if (!(input & AXP20X_PWR_STATUS_VBUS_PRESENT)) {
+			val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
+			break;
+		}
+
+		r = regmap_read(power->regmap, AXP20X_USB_OTG_STATUS, &v);
+		if (r)
+			return r;
+
+		if (!(v & AXP20X_USB_STATUS_VBUS_VALID)) {
+			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
+			break;
+		}
+
+		val->intval = POWER_SUPPLY_HEALTH_GOOD;
+		break;
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_PRESENT);
+		break;
+	case POWER_SUPPLY_PROP_ONLINE:
+		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_USED);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static enum power_supply_property axp20x_usb_power_properties[] = {
+	POWER_SUPPLY_PROP_HEALTH,
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_VOLTAGE_MIN,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_CURRENT_MAX,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+};
+
+static const struct power_supply_desc axp20x_usb_power_desc = {
+	.name = "axp20x-usb",
+	.type = POWER_SUPPLY_TYPE_USB,
+	.properties = axp20x_usb_power_properties,
+	.num_properties = ARRAY_SIZE(axp20x_usb_power_properties),
+	.get_property = axp20x_usb_power_get_property,
+};
+
+static int axp20x_usb_power_probe(struct platform_device *pdev)
+{
+	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
+	struct power_supply_config psy_cfg = {};
+	struct axp20x_usb_power *power;
+	static const char * const irq_names[] = { "VBUS_PLUGIN",
+		"VBUS_REMOVAL", "VBUS_VALID", "VBUS_NOT_VALID" };
+	int i, irq, r;
+
+	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;
+
+	/* Enable vbus valid checking */
+	r = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
+		    AXP20X_VBUS_MON_VBUS_VALID, AXP20X_VBUS_MON_VBUS_VALID);
+	if (r)
+		return r;
+
+	/* Enable vbus voltage and current measurement */
+	r = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
+			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_VOLT,
+			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_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_usb_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,
+				axp20x_usb_power_irq, 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_usb_power_match[] = {
+	{ .compatible = "x-powers,axp202-usb-power-supply" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, axp20x_usb_power_match);
+
+static struct platform_driver axp20x_usb_power_driver = {
+	.probe = axp20x_usb_power_probe,
+	.driver = {
+		.name = DRVNAME,
+		.of_match_table = axp20x_usb_power_match,
+	},
+};
+
+module_platform_driver(axp20x_usb_power_driver);
+
+MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
+MODULE_DESCRIPTION("AXP20x PMIC USB power supply status driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index 6d8b39a..8ec996e 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -11,6 +11,8 @@
 #ifndef __LINUX_MFD_AXP20X_H
 #define __LINUX_MFD_AXP20X_H
 
+#include <linux/regmap.h>
+
 enum {
 	AXP202_ID = 0,
 	AXP209_ID,
@@ -372,4 +374,26 @@ struct axp288_extcon_pdata {
 	struct gpio_desc *gpio_mux_cntl;
 };
 
+/* generic helper function for reading 9-16 bit wide regs */
+static inline int axp20x_read_variable_width(struct regmap *regmap,
+	unsigned int reg, unsigned int width)
+{
+	unsigned int reg_val, result;
+	int err;
+
+	err = regmap_read(regmap, reg, &reg_val);
+	if (err)
+		return err;
+
+	result = reg_val << (width - 8);
+
+	err = regmap_read(regmap, reg + 1, &reg_val);
+	if (err)
+		return err;
+
+	result |= reg_val;
+
+	return result;
+}
+
 #endif /* __LINUX_MFD_AXP20X_H */
-- 
2.4.3

Re: [PATCH v3 4/4] power: Add an axp20x-usb-power driver

[Krzysztof Kozlowski]

W dniu 26.06.2015 o 19:59, Hans de Goede pisze:
> This adds a driver for the usb power_supply bits of the axp20x PMICs.
> 
> I initially started writing my own driver, before coming aware of
> Bruno Prémont's excellent earlier RFC with a driver for this.
> 
> My driver was lacking CURRENT_MAX and VOLTAGE_MIN support Bruno's
> drvier has, so I've copied the code for those from his driver.
> 
> Note that the AC-power-supply and battery charger bits will need separate
> drivers. Each one needs its own devictree child-node so that other
> devicetree nodes can reference the right power-supply, and thus each one
> will get its own mfd-cell / platform_device and platform-driver.
> 
> Cc: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> Changes in v2:
> -Split out the dt-bindings documentation into a separate patch
> -Renamed axp20x_read_16bit to axp20x_read_variable_width
> -Use better local variable names inside axp20x_read_variable_width
> Changes in v3:
> -Add Bruno's S-o-b

Hi,

Two minor comments below, the driver itself looks good.


> ---
>  drivers/power/Kconfig            |   7 ++
>  drivers/power/Makefile           |   1 +
>  drivers/power/axp20x_usb_power.c | 243 +++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/axp20x.h       |  24 ++++
>  4 files changed, 275 insertions(+)
>  create mode 100644 drivers/power/axp20x_usb_power.c
> 
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index 4091fb0..1fee60c 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -439,6 +439,13 @@ config BATTERY_RT5033
>  	  The fuelgauge calculates and determines the battery state of charge
>  	  according to battery open circuit voltage.
>  
> +config AXP20X_POWER
> +	tristate "AXP20x power supply driver"
> +	depends on MFD_AXP20X
> +	help
> +	  This driver provides support for the power supply features of
> +	  AXP20x PMIC.
> +
>  source "drivers/power/reset/Kconfig"
>  
>  endif # POWER_SUPPLY
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index b7b0181..ae0f27d 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -9,6 +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_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_usb_power.c b/drivers/power/axp20x_usb_power.c
> new file mode 100644
> index 0000000..09f388e
> --- /dev/null
> +++ b/drivers/power/axp20x_usb_power.c
> @@ -0,0 +1,243 @@
> +/*
> + * AXP20x PMIC USB power supply status driver
> + *
> + * Copyright (C) 2015 Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> + * Copyright (C) 2014 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/device.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.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-usb-power-supply"
> +
> +#define AXP20X_PWR_STATUS_VBUS_PRESENT	BIT(5)
> +#define AXP20X_PWR_STATUS_VBUS_USED	BIT(4)
> +
> +#define AXP20X_USB_STATUS_VBUS_VALID	BIT(2)
> +
> +#define AXP20X_VBUS_VHOLD_uV(b)		(4000000 + (((b) >> 3) & 7) * 100000)
> +#define AXP20X_VBUS_CLIMIT_MASK		3
> +#define AXP20X_VBUC_CLIMIT_900mA	0
> +#define AXP20X_VBUC_CLIMIT_500mA	1
> +#define AXP20X_VBUC_CLIMIT_100mA	2
> +#define AXP20X_VBUC_CLIMIT_NONE		3
> +
> +#define AXP20X_ADC_EN1_VBUS_CURR	BIT(2)
> +#define AXP20X_ADC_EN1_VBUS_VOLT	BIT(3)
> +
> +#define AXP20X_VBUS_MON_VBUS_VALID	BIT(3)
> +
> +struct axp20x_usb_power {
> +	struct regmap *regmap;
> +	struct power_supply *supply;
> +};
> +
> +static irqreturn_t axp20x_usb_power_irq(int irq, void *devid)
> +{
> +	struct axp20x_usb_power *power = devid;
> +
> +	power_supply_changed(power->supply);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int axp20x_usb_power_get_property(struct power_supply *psy,
> +	enum power_supply_property psp, union power_supply_propval *val)
> +{
> +	struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
> +	unsigned int input, v;
> +	int r;

Here and in probe function - it is strictly personal but I don't like
the 'r' variable. I got used to 'err' or 'ret' which are more
descriptive to me. 'r' may look like 'resource' for example.

> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
> +		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
> +		if (r)
> +			return r;
> +
> +		val->intval = AXP20X_VBUS_VHOLD_uV(v);
> +		return 0;
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		r = axp20x_read_variable_width(power->regmap,
> +					       AXP20X_VBUS_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_MAX:
> +		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
> +		if (r)
> +			return r;
> +
> +		switch (v & AXP20X_VBUS_CLIMIT_MASK) {
> +		case AXP20X_VBUC_CLIMIT_100mA:
> +			val->intval = 100000;
> +			break;
> +		case AXP20X_VBUC_CLIMIT_500mA:
> +			val->intval = 500000;
> +			break;
> +		case AXP20X_VBUC_CLIMIT_900mA:
> +			val->intval = 900000;
> +			break;
> +		case AXP20X_VBUC_CLIMIT_NONE:
> +			val->intval = -1;
> +			break;
> +		}
> +		return 0;
> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +		r = axp20x_read_variable_width(power->regmap,
> +					       AXP20X_VBUS_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_HEALTH:
> +		if (!(input & AXP20X_PWR_STATUS_VBUS_PRESENT)) {
> +			val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
> +			break;
> +		}
> +
> +		r = regmap_read(power->regmap, AXP20X_USB_OTG_STATUS, &v);
> +		if (r)
> +			return r;
> +
> +		if (!(v & AXP20X_USB_STATUS_VBUS_VALID)) {
> +			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
> +			break;
> +		}
> +
> +		val->intval = POWER_SUPPLY_HEALTH_GOOD;
> +		break;
> +	case POWER_SUPPLY_PROP_PRESENT:
> +		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_PRESENT);
> +		break;
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_USED);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static enum power_supply_property axp20x_usb_power_properties[] = {
> +	POWER_SUPPLY_PROP_HEALTH,
> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_VOLTAGE_MIN,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_MAX,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +};
> +
> +static const struct power_supply_desc axp20x_usb_power_desc = {
> +	.name = "axp20x-usb",
> +	.type = POWER_SUPPLY_TYPE_USB,
> +	.properties = axp20x_usb_power_properties,
> +	.num_properties = ARRAY_SIZE(axp20x_usb_power_properties),
> +	.get_property = axp20x_usb_power_get_property,
> +};
> +
> +static int axp20x_usb_power_probe(struct platform_device *pdev)
> +{
> +	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> +	struct power_supply_config psy_cfg = {};
> +	struct axp20x_usb_power *power;
> +	static const char * const irq_names[] = { "VBUS_PLUGIN",
> +		"VBUS_REMOVAL", "VBUS_VALID", "VBUS_NOT_VALID" };
> +	int i, irq, r;
> +
> +	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;
> +
> +	/* Enable vbus valid checking */
> +	r = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
> +		    AXP20X_VBUS_MON_VBUS_VALID, AXP20X_VBUS_MON_VBUS_VALID);
> +	if (r)
> +		return r;
> +
> +	/* Enable vbus voltage and current measurement */
> +	r = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
> +			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_VOLT,
> +			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_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_usb_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,
> +				axp20x_usb_power_irq, 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_usb_power_match[] = {
> +	{ .compatible = "x-powers,axp202-usb-power-supply" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, axp20x_usb_power_match);
> +
> +static struct platform_driver axp20x_usb_power_driver = {
> +	.probe = axp20x_usb_power_probe,
> +	.driver = {
> +		.name = DRVNAME,
> +		.of_match_table = axp20x_usb_power_match,
> +	},
> +};
> +
> +module_platform_driver(axp20x_usb_power_driver);
> +
> +MODULE_AUTHOR("Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>");
> +MODULE_DESCRIPTION("AXP20x PMIC USB power supply status driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index 6d8b39a..8ec996e 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -11,6 +11,8 @@
>  #ifndef __LINUX_MFD_AXP20X_H
>  #define __LINUX_MFD_AXP20X_H
>  
> +#include <linux/regmap.h>
> +
>  enum {
>  	AXP202_ID = 0,
>  	AXP209_ID,
> @@ -372,4 +374,26 @@ struct axp288_extcon_pdata {
>  	struct gpio_desc *gpio_mux_cntl;
>  };
>  
> +/* generic helper function for reading 9-16 bit wide regs */
> +static inline int axp20x_read_variable_width(struct regmap *regmap,
> +	unsigned int reg, unsigned int width)
> +{
> +	unsigned int reg_val, result;
> +	int err;
> +
> +	err = regmap_read(regmap, reg, &reg_val);
> +	if (err)
> +		return err;
> +
> +	result = reg_val << (width - 8);
> +
> +	err = regmap_read(regmap, reg + 1, &reg_val);
> +	if (err)
> +		return err;
> +
> +	result |= reg_val;
> +
> +	return result;
> +}
> +
>  #endif /* __LINUX_MFD_AXP20X_H */

Is this static inline function used outside of
drivers/power/axp20x_usb_power.c? Looks like it isn't, but maybe you
have a plan for it? If not, then it shouldn't be in header file.

Best regards,
Krzysztof

-- 
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 v3 4/4] power: Add an axp20x-usb-power driver

[Krzysztof Kozlowski]

W dniu 26.06.2015 o 19:59, Hans de Goede pisze:
> This adds a driver for the usb power_supply bits of the axp20x PMICs.
> 
> I initially started writing my own driver, before coming aware of
> Bruno Pr?mont's excellent earlier RFC with a driver for this.
> 
> My driver was lacking CURRENT_MAX and VOLTAGE_MIN support Bruno's
> drvier has, so I've copied the code for those from his driver.
> 
> Note that the AC-power-supply and battery charger bits will need separate
> drivers. Each one needs its own devictree child-node so that other
> devicetree nodes can reference the right power-supply, and thus each one
> will get its own mfd-cell / platform_device and platform-driver.
> 
> Cc: Bruno Pr?mont <bonbons@linux-vserver.org>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Bruno Pr?mont <bonbons@linux-vserver.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Split out the dt-bindings documentation into a separate patch
> -Renamed axp20x_read_16bit to axp20x_read_variable_width
> -Use better local variable names inside axp20x_read_variable_width
> Changes in v3:
> -Add Bruno's S-o-b

Hi,

Two minor comments below, the driver itself looks good.


> ---
>  drivers/power/Kconfig            |   7 ++
>  drivers/power/Makefile           |   1 +
>  drivers/power/axp20x_usb_power.c | 243 +++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/axp20x.h       |  24 ++++
>  4 files changed, 275 insertions(+)
>  create mode 100644 drivers/power/axp20x_usb_power.c
> 
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index 4091fb0..1fee60c 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -439,6 +439,13 @@ config BATTERY_RT5033
>  	  The fuelgauge calculates and determines the battery state of charge
>  	  according to battery open circuit voltage.
>  
> +config AXP20X_POWER
> +	tristate "AXP20x power supply driver"
> +	depends on MFD_AXP20X
> +	help
> +	  This driver provides support for the power supply features of
> +	  AXP20x PMIC.
> +
>  source "drivers/power/reset/Kconfig"
>  
>  endif # POWER_SUPPLY
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index b7b0181..ae0f27d 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -9,6 +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_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_usb_power.c b/drivers/power/axp20x_usb_power.c
> new file mode 100644
> index 0000000..09f388e
> --- /dev/null
> +++ b/drivers/power/axp20x_usb_power.c
> @@ -0,0 +1,243 @@
> +/*
> + * AXP20x PMIC USB power supply status driver
> + *
> + * Copyright (C) 2015 Hans de Goede <hdegoede@redhat.com>
> + * Copyright (C) 2014 Bruno Pr?mont <bonbons@linux-vserver.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/device.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.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-usb-power-supply"
> +
> +#define AXP20X_PWR_STATUS_VBUS_PRESENT	BIT(5)
> +#define AXP20X_PWR_STATUS_VBUS_USED	BIT(4)
> +
> +#define AXP20X_USB_STATUS_VBUS_VALID	BIT(2)
> +
> +#define AXP20X_VBUS_VHOLD_uV(b)		(4000000 + (((b) >> 3) & 7) * 100000)
> +#define AXP20X_VBUS_CLIMIT_MASK		3
> +#define AXP20X_VBUC_CLIMIT_900mA	0
> +#define AXP20X_VBUC_CLIMIT_500mA	1
> +#define AXP20X_VBUC_CLIMIT_100mA	2
> +#define AXP20X_VBUC_CLIMIT_NONE		3
> +
> +#define AXP20X_ADC_EN1_VBUS_CURR	BIT(2)
> +#define AXP20X_ADC_EN1_VBUS_VOLT	BIT(3)
> +
> +#define AXP20X_VBUS_MON_VBUS_VALID	BIT(3)
> +
> +struct axp20x_usb_power {
> +	struct regmap *regmap;
> +	struct power_supply *supply;
> +};
> +
> +static irqreturn_t axp20x_usb_power_irq(int irq, void *devid)
> +{
> +	struct axp20x_usb_power *power = devid;
> +
> +	power_supply_changed(power->supply);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int axp20x_usb_power_get_property(struct power_supply *psy,
> +	enum power_supply_property psp, union power_supply_propval *val)
> +{
> +	struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
> +	unsigned int input, v;
> +	int r;

Here and in probe function - it is strictly personal but I don't like
the 'r' variable. I got used to 'err' or 'ret' which are more
descriptive to me. 'r' may look like 'resource' for example.

> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
> +		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
> +		if (r)
> +			return r;
> +
> +		val->intval = AXP20X_VBUS_VHOLD_uV(v);
> +		return 0;
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		r = axp20x_read_variable_width(power->regmap,
> +					       AXP20X_VBUS_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_MAX:
> +		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
> +		if (r)
> +			return r;
> +
> +		switch (v & AXP20X_VBUS_CLIMIT_MASK) {
> +		case AXP20X_VBUC_CLIMIT_100mA:
> +			val->intval = 100000;
> +			break;
> +		case AXP20X_VBUC_CLIMIT_500mA:
> +			val->intval = 500000;
> +			break;
> +		case AXP20X_VBUC_CLIMIT_900mA:
> +			val->intval = 900000;
> +			break;
> +		case AXP20X_VBUC_CLIMIT_NONE:
> +			val->intval = -1;
> +			break;
> +		}
> +		return 0;
> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +		r = axp20x_read_variable_width(power->regmap,
> +					       AXP20X_VBUS_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_HEALTH:
> +		if (!(input & AXP20X_PWR_STATUS_VBUS_PRESENT)) {
> +			val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
> +			break;
> +		}
> +
> +		r = regmap_read(power->regmap, AXP20X_USB_OTG_STATUS, &v);
> +		if (r)
> +			return r;
> +
> +		if (!(v & AXP20X_USB_STATUS_VBUS_VALID)) {
> +			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
> +			break;
> +		}
> +
> +		val->intval = POWER_SUPPLY_HEALTH_GOOD;
> +		break;
> +	case POWER_SUPPLY_PROP_PRESENT:
> +		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_PRESENT);
> +		break;
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_USED);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static enum power_supply_property axp20x_usb_power_properties[] = {
> +	POWER_SUPPLY_PROP_HEALTH,
> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_VOLTAGE_MIN,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_MAX,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +};
> +
> +static const struct power_supply_desc axp20x_usb_power_desc = {
> +	.name = "axp20x-usb",
> +	.type = POWER_SUPPLY_TYPE_USB,
> +	.properties = axp20x_usb_power_properties,
> +	.num_properties = ARRAY_SIZE(axp20x_usb_power_properties),
> +	.get_property = axp20x_usb_power_get_property,
> +};
> +
> +static int axp20x_usb_power_probe(struct platform_device *pdev)
> +{
> +	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> +	struct power_supply_config psy_cfg = {};
> +	struct axp20x_usb_power *power;
> +	static const char * const irq_names[] = { "VBUS_PLUGIN",
> +		"VBUS_REMOVAL", "VBUS_VALID", "VBUS_NOT_VALID" };
> +	int i, irq, r;
> +
> +	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;
> +
> +	/* Enable vbus valid checking */
> +	r = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
> +		    AXP20X_VBUS_MON_VBUS_VALID, AXP20X_VBUS_MON_VBUS_VALID);
> +	if (r)
> +		return r;
> +
> +	/* Enable vbus voltage and current measurement */
> +	r = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
> +			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_VOLT,
> +			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_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_usb_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,
> +				axp20x_usb_power_irq, 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_usb_power_match[] = {
> +	{ .compatible = "x-powers,axp202-usb-power-supply" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, axp20x_usb_power_match);
> +
> +static struct platform_driver axp20x_usb_power_driver = {
> +	.probe = axp20x_usb_power_probe,
> +	.driver = {
> +		.name = DRVNAME,
> +		.of_match_table = axp20x_usb_power_match,
> +	},
> +};
> +
> +module_platform_driver(axp20x_usb_power_driver);
> +
> +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
> +MODULE_DESCRIPTION("AXP20x PMIC USB power supply status driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index 6d8b39a..8ec996e 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -11,6 +11,8 @@
>  #ifndef __LINUX_MFD_AXP20X_H
>  #define __LINUX_MFD_AXP20X_H
>  
> +#include <linux/regmap.h>
> +
>  enum {
>  	AXP202_ID = 0,
>  	AXP209_ID,
> @@ -372,4 +374,26 @@ struct axp288_extcon_pdata {
>  	struct gpio_desc *gpio_mux_cntl;
>  };
>  
> +/* generic helper function for reading 9-16 bit wide regs */
> +static inline int axp20x_read_variable_width(struct regmap *regmap,
> +	unsigned int reg, unsigned int width)
> +{
> +	unsigned int reg_val, result;
> +	int err;
> +
> +	err = regmap_read(regmap, reg, &reg_val);
> +	if (err)
> +		return err;
> +
> +	result = reg_val << (width - 8);
> +
> +	err = regmap_read(regmap, reg + 1, &reg_val);
> +	if (err)
> +		return err;
> +
> +	result |= reg_val;
> +
> +	return result;
> +}
> +
>  #endif /* __LINUX_MFD_AXP20X_H */

Is this static inline function used outside of
drivers/power/axp20x_usb_power.c? Looks like it isn't, but maybe you
have a plan for it? If not, then it shouldn't be in header file.

Best regards,
Krzysztof

Re: [PATCH v3 4/4] power: Add an axp20x-usb-power driver

[Bruno Prémont]

On Sat, 27 Jun 2015 14:28:40 Krzysztof Kozlowski wrote:
> W dniu 26.06.2015 o 19:59, Hans de Goede pisze:
> > This adds a driver for the usb power_supply bits of the axp20x PMICs.
> > 
> > I initially started writing my own driver, before coming aware of
> > Bruno Prémont's excellent earlier RFC with a driver for this.
> > 
> > My driver was lacking CURRENT_MAX and VOLTAGE_MIN support Bruno's
> > drvier has, so I've copied the code for those from his driver.
> > 
> > Note that the AC-power-supply and battery charger bits will need separate
> > drivers. Each one needs its own devictree child-node so that other
> > devicetree nodes can reference the right power-supply, and thus each one
> > will get its own mfd-cell / platform_device and platform-driver.
> > 
> > Cc: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
> > Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > Signed-off-by: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
> > Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> > Changes in v2:
> > -Split out the dt-bindings documentation into a separate patch
> > -Renamed axp20x_read_16bit to axp20x_read_variable_width
> > -Use better local variable names inside axp20x_read_variable_width
> > Changes in v3:
> > -Add Bruno's S-o-b
> 
> Hi,
> 
> Two minor comments below, the driver itself looks good.
> 
> 
> > ---
> >  drivers/power/Kconfig            |   7 ++
> >  drivers/power/Makefile           |   1 +
> >  drivers/power/axp20x_usb_power.c | 243 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/axp20x.h       |  24 ++++
> >  4 files changed, 275 insertions(+)
> >  create mode 100644 drivers/power/axp20x_usb_power.c
> > 
> > diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> > index 4091fb0..1fee60c 100644
> > --- a/drivers/power/Kconfig
> > +++ b/drivers/power/Kconfig
> > @@ -439,6 +439,13 @@ config BATTERY_RT5033
> >  	  The fuelgauge calculates and determines the battery state of charge
> >  	  according to battery open circuit voltage.
> >  
> > +config AXP20X_POWER
> > +	tristate "AXP20x power supply driver"
> > +	depends on MFD_AXP20X
> > +	help
> > +	  This driver provides support for the power supply features of
> > +	  AXP20x PMIC.
> > +
> >  source "drivers/power/reset/Kconfig"
> >  
> >  endif # POWER_SUPPLY
> > diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> > index b7b0181..ae0f27d 100644
> > --- a/drivers/power/Makefile
> > +++ b/drivers/power/Makefile
> > @@ -9,6 +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_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_usb_power.c b/drivers/power/axp20x_usb_power.c
> > new file mode 100644
> > index 0000000..09f388e
> > --- /dev/null
> > +++ b/drivers/power/axp20x_usb_power.c
> > @@ -0,0 +1,243 @@
> > +/*
> > + * AXP20x PMIC USB power supply status driver
> > + *
> > + * Copyright (C) 2015 Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > + * Copyright (C) 2014 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/device.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.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-usb-power-supply"
> > +
> > +#define AXP20X_PWR_STATUS_VBUS_PRESENT	BIT(5)
> > +#define AXP20X_PWR_STATUS_VBUS_USED	BIT(4)
> > +
> > +#define AXP20X_USB_STATUS_VBUS_VALID	BIT(2)
> > +
> > +#define AXP20X_VBUS_VHOLD_uV(b)		(4000000 + (((b) >> 3) & 7) * 100000)
> > +#define AXP20X_VBUS_CLIMIT_MASK		3
> > +#define AXP20X_VBUC_CLIMIT_900mA	0
> > +#define AXP20X_VBUC_CLIMIT_500mA	1
> > +#define AXP20X_VBUC_CLIMIT_100mA	2
> > +#define AXP20X_VBUC_CLIMIT_NONE		3
> > +
> > +#define AXP20X_ADC_EN1_VBUS_CURR	BIT(2)
> > +#define AXP20X_ADC_EN1_VBUS_VOLT	BIT(3)
> > +
> > +#define AXP20X_VBUS_MON_VBUS_VALID	BIT(3)
> > +
> > +struct axp20x_usb_power {
> > +	struct regmap *regmap;
> > +	struct power_supply *supply;
> > +};
> > +
> > +static irqreturn_t axp20x_usb_power_irq(int irq, void *devid)
> > +{
> > +	struct axp20x_usb_power *power = devid;
> > +
> > +	power_supply_changed(power->supply);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int axp20x_usb_power_get_property(struct power_supply *psy,
> > +	enum power_supply_property psp, union power_supply_propval *val)
> > +{
> > +	struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
> > +	unsigned int input, v;
> > +	int r;
> 
> Here and in probe function - it is strictly personal but I don't like
> the 'r' variable. I got used to 'err' or 'ret' which are more
> descriptive to me. 'r' may look like 'resource' for example.

'ret' might work but doesn't it imply return value for this function
instead of return value from a called function?

> > +
> > +	switch (psp) {
> > +	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
> > +		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
> > +		if (r)
> > +			return r;
> > +
> > +		val->intval = AXP20X_VBUS_VHOLD_uV(v);
> > +		return 0;
> > +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> > +		r = axp20x_read_variable_width(power->regmap,
> > +					       AXP20X_VBUS_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_MAX:
> > +		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
> > +		if (r)
> > +			return r;
> > +
> > +		switch (v & AXP20X_VBUS_CLIMIT_MASK) {
> > +		case AXP20X_VBUC_CLIMIT_100mA:
> > +			val->intval = 100000;
> > +			break;
> > +		case AXP20X_VBUC_CLIMIT_500mA:
> > +			val->intval = 500000;
> > +			break;
> > +		case AXP20X_VBUC_CLIMIT_900mA:
> > +			val->intval = 900000;
> > +			break;
> > +		case AXP20X_VBUC_CLIMIT_NONE:
> > +			val->intval = -1;
> > +			break;
> > +		}
> > +		return 0;
> > +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> > +		r = axp20x_read_variable_width(power->regmap,
> > +					       AXP20X_VBUS_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_HEALTH:
> > +		if (!(input & AXP20X_PWR_STATUS_VBUS_PRESENT)) {
> > +			val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
> > +			break;
> > +		}
> > +
> > +		r = regmap_read(power->regmap, AXP20X_USB_OTG_STATUS, &v);
> > +		if (r)
> > +			return r;
> > +
> > +		if (!(v & AXP20X_USB_STATUS_VBUS_VALID)) {
> > +			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
> > +			break;
> > +		}
> > +
> > +		val->intval = POWER_SUPPLY_HEALTH_GOOD;
> > +		break;
> > +	case POWER_SUPPLY_PROP_PRESENT:
> > +		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_PRESENT);
> > +		break;
> > +	case POWER_SUPPLY_PROP_ONLINE:
> > +		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_USED);
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static enum power_supply_property axp20x_usb_power_properties[] = {
> > +	POWER_SUPPLY_PROP_HEALTH,
> > +	POWER_SUPPLY_PROP_PRESENT,
> > +	POWER_SUPPLY_PROP_ONLINE,
> > +	POWER_SUPPLY_PROP_VOLTAGE_MIN,
> > +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> > +	POWER_SUPPLY_PROP_CURRENT_MAX,
> > +	POWER_SUPPLY_PROP_CURRENT_NOW,
> > +};
> > +
> > +static const struct power_supply_desc axp20x_usb_power_desc = {
> > +	.name = "axp20x-usb",
> > +	.type = POWER_SUPPLY_TYPE_USB,
> > +	.properties = axp20x_usb_power_properties,
> > +	.num_properties = ARRAY_SIZE(axp20x_usb_power_properties),
> > +	.get_property = axp20x_usb_power_get_property,
> > +};
> > +
> > +static int axp20x_usb_power_probe(struct platform_device *pdev)
> > +{
> > +	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> > +	struct power_supply_config psy_cfg = {};
> > +	struct axp20x_usb_power *power;
> > +	static const char * const irq_names[] = { "VBUS_PLUGIN",
> > +		"VBUS_REMOVAL", "VBUS_VALID", "VBUS_NOT_VALID" };
> > +	int i, irq, r;
> > +
> > +	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;
> > +
> > +	/* Enable vbus valid checking */
> > +	r = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
> > +		    AXP20X_VBUS_MON_VBUS_VALID, AXP20X_VBUS_MON_VBUS_VALID);
> > +	if (r)
> > +		return r;
> > +
> > +	/* Enable vbus voltage and current measurement */
> > +	r = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
> > +			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_VOLT,
> > +			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_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_usb_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,
> > +				axp20x_usb_power_irq, 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_usb_power_match[] = {
> > +	{ .compatible = "x-powers,axp202-usb-power-supply" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, axp20x_usb_power_match);
> > +
> > +static struct platform_driver axp20x_usb_power_driver = {
> > +	.probe = axp20x_usb_power_probe,
> > +	.driver = {
> > +		.name = DRVNAME,
> > +		.of_match_table = axp20x_usb_power_match,
> > +	},
> > +};
> > +
> > +module_platform_driver(axp20x_usb_power_driver);
> > +
> > +MODULE_AUTHOR("Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>");
> > +MODULE_DESCRIPTION("AXP20x PMIC USB power supply status driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> > index 6d8b39a..8ec996e 100644
> > --- a/include/linux/mfd/axp20x.h
> > +++ b/include/linux/mfd/axp20x.h
> > @@ -11,6 +11,8 @@
> >  #ifndef __LINUX_MFD_AXP20X_H
> >  #define __LINUX_MFD_AXP20X_H
> >  
> > +#include <linux/regmap.h>
> > +
> >  enum {
> >  	AXP202_ID = 0,
> >  	AXP209_ID,
> > @@ -372,4 +374,26 @@ struct axp288_extcon_pdata {
> >  	struct gpio_desc *gpio_mux_cntl;
> >  };
> >  
> > +/* generic helper function for reading 9-16 bit wide regs */
> > +static inline int axp20x_read_variable_width(struct regmap *regmap,
> > +	unsigned int reg, unsigned int width)
> > +{
> > +	unsigned int reg_val, result;
> > +	int err;
> > +
> > +	err = regmap_read(regmap, reg, &reg_val);
> > +	if (err)
> > +		return err;
> > +
> > +	result = reg_val << (width - 8);
> > +
> > +	err = regmap_read(regmap, reg + 1, &reg_val);
> > +	if (err)
> > +		return err;
> > +
> > +	result |= reg_val;
> > +
> > +	return result;
> > +}
> > +
> >  #endif /* __LINUX_MFD_AXP20X_H */
> 
> Is this static inline function used outside of
> drivers/power/axp20x_usb_power.c? Looks like it isn't, but maybe you
> have a plan for it? If not, then it shouldn't be in header file.

See comments for first iteration of this patch:
| | This is only used twice and only in one file.
| |
| | Do you know of any other uses of this call that will be upstreamed
| | shortly?  
| 
| Yes I plan to write drivers for the other 3 power_supply class
| cells (ac-power, battery-charger, rtc-bat-charger) in the axp209,
| and most of those need the same helper which I why I put it here.  

> Best regards,
> Krzysztof

Best regards,
Bruno

-- 
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 v3 4/4] power: Add an axp20x-usb-power driver

[Bruno Prémont]

On Sat, 27 Jun 2015 14:28:40 Krzysztof Kozlowski wrote:
> W dniu 26.06.2015 o 19:59, Hans de Goede pisze:
> > This adds a driver for the usb power_supply bits of the axp20x PMICs.
> > 
> > I initially started writing my own driver, before coming aware of
> > Bruno Pr?mont's excellent earlier RFC with a driver for this.
> > 
> > My driver was lacking CURRENT_MAX and VOLTAGE_MIN support Bruno's
> > drvier has, so I've copied the code for those from his driver.
> > 
> > Note that the AC-power-supply and battery charger bits will need separate
> > drivers. Each one needs its own devictree child-node so that other
> > devicetree nodes can reference the right power-supply, and thus each one
> > will get its own mfd-cell / platform_device and platform-driver.
> > 
> > Cc: Bruno Pr?mont <bonbons@linux-vserver.org>
> > Acked-by: Lee Jones <lee.jones@linaro.org>
> > Signed-off-by: Bruno Pr?mont <bonbons@linux-vserver.org>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> > Changes in v2:
> > -Split out the dt-bindings documentation into a separate patch
> > -Renamed axp20x_read_16bit to axp20x_read_variable_width
> > -Use better local variable names inside axp20x_read_variable_width
> > Changes in v3:
> > -Add Bruno's S-o-b
> 
> Hi,
> 
> Two minor comments below, the driver itself looks good.
> 
> 
> > ---
> >  drivers/power/Kconfig            |   7 ++
> >  drivers/power/Makefile           |   1 +
> >  drivers/power/axp20x_usb_power.c | 243 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/axp20x.h       |  24 ++++
> >  4 files changed, 275 insertions(+)
> >  create mode 100644 drivers/power/axp20x_usb_power.c
> > 
> > diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> > index 4091fb0..1fee60c 100644
> > --- a/drivers/power/Kconfig
> > +++ b/drivers/power/Kconfig
> > @@ -439,6 +439,13 @@ config BATTERY_RT5033
> >  	  The fuelgauge calculates and determines the battery state of charge
> >  	  according to battery open circuit voltage.
> >  
> > +config AXP20X_POWER
> > +	tristate "AXP20x power supply driver"
> > +	depends on MFD_AXP20X
> > +	help
> > +	  This driver provides support for the power supply features of
> > +	  AXP20x PMIC.
> > +
> >  source "drivers/power/reset/Kconfig"
> >  
> >  endif # POWER_SUPPLY
> > diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> > index b7b0181..ae0f27d 100644
> > --- a/drivers/power/Makefile
> > +++ b/drivers/power/Makefile
> > @@ -9,6 +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_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_usb_power.c b/drivers/power/axp20x_usb_power.c
> > new file mode 100644
> > index 0000000..09f388e
> > --- /dev/null
> > +++ b/drivers/power/axp20x_usb_power.c
> > @@ -0,0 +1,243 @@
> > +/*
> > + * AXP20x PMIC USB power supply status driver
> > + *
> > + * Copyright (C) 2015 Hans de Goede <hdegoede@redhat.com>
> > + * Copyright (C) 2014 Bruno Pr?mont <bonbons@linux-vserver.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/device.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.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-usb-power-supply"
> > +
> > +#define AXP20X_PWR_STATUS_VBUS_PRESENT	BIT(5)
> > +#define AXP20X_PWR_STATUS_VBUS_USED	BIT(4)
> > +
> > +#define AXP20X_USB_STATUS_VBUS_VALID	BIT(2)
> > +
> > +#define AXP20X_VBUS_VHOLD_uV(b)		(4000000 + (((b) >> 3) & 7) * 100000)
> > +#define AXP20X_VBUS_CLIMIT_MASK		3
> > +#define AXP20X_VBUC_CLIMIT_900mA	0
> > +#define AXP20X_VBUC_CLIMIT_500mA	1
> > +#define AXP20X_VBUC_CLIMIT_100mA	2
> > +#define AXP20X_VBUC_CLIMIT_NONE		3
> > +
> > +#define AXP20X_ADC_EN1_VBUS_CURR	BIT(2)
> > +#define AXP20X_ADC_EN1_VBUS_VOLT	BIT(3)
> > +
> > +#define AXP20X_VBUS_MON_VBUS_VALID	BIT(3)
> > +
> > +struct axp20x_usb_power {
> > +	struct regmap *regmap;
> > +	struct power_supply *supply;
> > +};
> > +
> > +static irqreturn_t axp20x_usb_power_irq(int irq, void *devid)
> > +{
> > +	struct axp20x_usb_power *power = devid;
> > +
> > +	power_supply_changed(power->supply);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int axp20x_usb_power_get_property(struct power_supply *psy,
> > +	enum power_supply_property psp, union power_supply_propval *val)
> > +{
> > +	struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
> > +	unsigned int input, v;
> > +	int r;
> 
> Here and in probe function - it is strictly personal but I don't like
> the 'r' variable. I got used to 'err' or 'ret' which are more
> descriptive to me. 'r' may look like 'resource' for example.

'ret' might work but doesn't it imply return value for this function
instead of return value from a called function?

> > +
> > +	switch (psp) {
> > +	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
> > +		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
> > +		if (r)
> > +			return r;
> > +
> > +		val->intval = AXP20X_VBUS_VHOLD_uV(v);
> > +		return 0;
> > +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> > +		r = axp20x_read_variable_width(power->regmap,
> > +					       AXP20X_VBUS_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_MAX:
> > +		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
> > +		if (r)
> > +			return r;
> > +
> > +		switch (v & AXP20X_VBUS_CLIMIT_MASK) {
> > +		case AXP20X_VBUC_CLIMIT_100mA:
> > +			val->intval = 100000;
> > +			break;
> > +		case AXP20X_VBUC_CLIMIT_500mA:
> > +			val->intval = 500000;
> > +			break;
> > +		case AXP20X_VBUC_CLIMIT_900mA:
> > +			val->intval = 900000;
> > +			break;
> > +		case AXP20X_VBUC_CLIMIT_NONE:
> > +			val->intval = -1;
> > +			break;
> > +		}
> > +		return 0;
> > +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> > +		r = axp20x_read_variable_width(power->regmap,
> > +					       AXP20X_VBUS_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_HEALTH:
> > +		if (!(input & AXP20X_PWR_STATUS_VBUS_PRESENT)) {
> > +			val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
> > +			break;
> > +		}
> > +
> > +		r = regmap_read(power->regmap, AXP20X_USB_OTG_STATUS, &v);
> > +		if (r)
> > +			return r;
> > +
> > +		if (!(v & AXP20X_USB_STATUS_VBUS_VALID)) {
> > +			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
> > +			break;
> > +		}
> > +
> > +		val->intval = POWER_SUPPLY_HEALTH_GOOD;
> > +		break;
> > +	case POWER_SUPPLY_PROP_PRESENT:
> > +		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_PRESENT);
> > +		break;
> > +	case POWER_SUPPLY_PROP_ONLINE:
> > +		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_USED);
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static enum power_supply_property axp20x_usb_power_properties[] = {
> > +	POWER_SUPPLY_PROP_HEALTH,
> > +	POWER_SUPPLY_PROP_PRESENT,
> > +	POWER_SUPPLY_PROP_ONLINE,
> > +	POWER_SUPPLY_PROP_VOLTAGE_MIN,
> > +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> > +	POWER_SUPPLY_PROP_CURRENT_MAX,
> > +	POWER_SUPPLY_PROP_CURRENT_NOW,
> > +};
> > +
> > +static const struct power_supply_desc axp20x_usb_power_desc = {
> > +	.name = "axp20x-usb",
> > +	.type = POWER_SUPPLY_TYPE_USB,
> > +	.properties = axp20x_usb_power_properties,
> > +	.num_properties = ARRAY_SIZE(axp20x_usb_power_properties),
> > +	.get_property = axp20x_usb_power_get_property,
> > +};
> > +
> > +static int axp20x_usb_power_probe(struct platform_device *pdev)
> > +{
> > +	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> > +	struct power_supply_config psy_cfg = {};
> > +	struct axp20x_usb_power *power;
> > +	static const char * const irq_names[] = { "VBUS_PLUGIN",
> > +		"VBUS_REMOVAL", "VBUS_VALID", "VBUS_NOT_VALID" };
> > +	int i, irq, r;
> > +
> > +	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;
> > +
> > +	/* Enable vbus valid checking */
> > +	r = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
> > +		    AXP20X_VBUS_MON_VBUS_VALID, AXP20X_VBUS_MON_VBUS_VALID);
> > +	if (r)
> > +		return r;
> > +
> > +	/* Enable vbus voltage and current measurement */
> > +	r = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
> > +			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_VOLT,
> > +			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_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_usb_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,
> > +				axp20x_usb_power_irq, 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_usb_power_match[] = {
> > +	{ .compatible = "x-powers,axp202-usb-power-supply" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, axp20x_usb_power_match);
> > +
> > +static struct platform_driver axp20x_usb_power_driver = {
> > +	.probe = axp20x_usb_power_probe,
> > +	.driver = {
> > +		.name = DRVNAME,
> > +		.of_match_table = axp20x_usb_power_match,
> > +	},
> > +};
> > +
> > +module_platform_driver(axp20x_usb_power_driver);
> > +
> > +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
> > +MODULE_DESCRIPTION("AXP20x PMIC USB power supply status driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> > index 6d8b39a..8ec996e 100644
> > --- a/include/linux/mfd/axp20x.h
> > +++ b/include/linux/mfd/axp20x.h
> > @@ -11,6 +11,8 @@
> >  #ifndef __LINUX_MFD_AXP20X_H
> >  #define __LINUX_MFD_AXP20X_H
> >  
> > +#include <linux/regmap.h>
> > +
> >  enum {
> >  	AXP202_ID = 0,
> >  	AXP209_ID,
> > @@ -372,4 +374,26 @@ struct axp288_extcon_pdata {
> >  	struct gpio_desc *gpio_mux_cntl;
> >  };
> >  
> > +/* generic helper function for reading 9-16 bit wide regs */
> > +static inline int axp20x_read_variable_width(struct regmap *regmap,
> > +	unsigned int reg, unsigned int width)
> > +{
> > +	unsigned int reg_val, result;
> > +	int err;
> > +
> > +	err = regmap_read(regmap, reg, &reg_val);
> > +	if (err)
> > +		return err;
> > +
> > +	result = reg_val << (width - 8);
> > +
> > +	err = regmap_read(regmap, reg + 1, &reg_val);
> > +	if (err)
> > +		return err;
> > +
> > +	result |= reg_val;
> > +
> > +	return result;
> > +}
> > +
> >  #endif /* __LINUX_MFD_AXP20X_H */
> 
> Is this static inline function used outside of
> drivers/power/axp20x_usb_power.c? Looks like it isn't, but maybe you
> have a plan for it? If not, then it shouldn't be in header file.

See comments for first iteration of this patch:
| | This is only used twice and only in one file.
| |
| | Do you know of any other uses of this call that will be upstreamed
| | shortly?  
| 
| Yes I plan to write drivers for the other 3 power_supply class
| cells (ac-power, battery-charger, rtc-bat-charger) in the axp209,
| and most of those need the same helper which I why I put it here.  

> Best regards,
> Krzysztof

Best regards,
Bruno

Re: [PATCH v3 4/4] power: Add an axp20x-usb-power driver

[Krzysztof Kozlowski]

W dniu 28.06.2015 o 05:25, Bruno Prémont pisze:
> On Sat, 27 Jun 2015 14:28:40 Krzysztof Kozlowski wrote:
>> W dniu 26.06.2015 o 19:59, Hans de Goede pisze:
>>> This adds a driver for the usb power_supply bits of the axp20x PMICs.
>>>
>>> I initially started writing my own driver, before coming aware of
>>> Bruno Prémont's excellent earlier RFC with a driver for this.
>>>
>>> My driver was lacking CURRENT_MAX and VOLTAGE_MIN support Bruno's
>>> drvier has, so I've copied the code for those from his driver.
>>>
>>> Note that the AC-power-supply and battery charger bits will need separate
>>> drivers. Each one needs its own devictree child-node so that other
>>> devicetree nodes can reference the right power-supply, and thus each one
>>> will get its own mfd-cell / platform_device and platform-driver.
>>>
>>> Cc: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
>>> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> Signed-off-by: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
>>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> ---
>>> Changes in v2:
>>> -Split out the dt-bindings documentation into a separate patch
>>> -Renamed axp20x_read_16bit to axp20x_read_variable_width
>>> -Use better local variable names inside axp20x_read_variable_width
>>> Changes in v3:
>>> -Add Bruno's S-o-b
>>
>> Hi,
>>
>> Two minor comments below, the driver itself looks good.
>>
>>
>>> ---
>>>  drivers/power/Kconfig            |   7 ++
>>>  drivers/power/Makefile           |   1 +
>>>  drivers/power/axp20x_usb_power.c | 243 +++++++++++++++++++++++++++++++++++++++
>>>  include/linux/mfd/axp20x.h       |  24 ++++
>>>  4 files changed, 275 insertions(+)
>>>  create mode 100644 drivers/power/axp20x_usb_power.c
>>>
>>> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
>>> index 4091fb0..1fee60c 100644
>>> --- a/drivers/power/Kconfig
>>> +++ b/drivers/power/Kconfig
>>> @@ -439,6 +439,13 @@ config BATTERY_RT5033
>>>  	  The fuelgauge calculates and determines the battery state of charge
>>>  	  according to battery open circuit voltage.
>>>  
>>> +config AXP20X_POWER
>>> +	tristate "AXP20x power supply driver"
>>> +	depends on MFD_AXP20X
>>> +	help
>>> +	  This driver provides support for the power supply features of
>>> +	  AXP20x PMIC.
>>> +
>>>  source "drivers/power/reset/Kconfig"
>>>  
>>>  endif # POWER_SUPPLY
>>> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
>>> index b7b0181..ae0f27d 100644
>>> --- a/drivers/power/Makefile
>>> +++ b/drivers/power/Makefile
>>> @@ -9,6 +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_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_usb_power.c b/drivers/power/axp20x_usb_power.c
>>> new file mode 100644
>>> index 0000000..09f388e
>>> --- /dev/null
>>> +++ b/drivers/power/axp20x_usb_power.c
>>> @@ -0,0 +1,243 @@
>>> +/*
>>> + * AXP20x PMIC USB power supply status driver
>>> + *
>>> + * Copyright (C) 2015 Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> + * Copyright (C) 2014 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/device.h>
>>> +#include <linux/init.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/kernel.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-usb-power-supply"
>>> +
>>> +#define AXP20X_PWR_STATUS_VBUS_PRESENT	BIT(5)
>>> +#define AXP20X_PWR_STATUS_VBUS_USED	BIT(4)
>>> +
>>> +#define AXP20X_USB_STATUS_VBUS_VALID	BIT(2)
>>> +
>>> +#define AXP20X_VBUS_VHOLD_uV(b)		(4000000 + (((b) >> 3) & 7) * 100000)
>>> +#define AXP20X_VBUS_CLIMIT_MASK		3
>>> +#define AXP20X_VBUC_CLIMIT_900mA	0
>>> +#define AXP20X_VBUC_CLIMIT_500mA	1
>>> +#define AXP20X_VBUC_CLIMIT_100mA	2
>>> +#define AXP20X_VBUC_CLIMIT_NONE		3
>>> +
>>> +#define AXP20X_ADC_EN1_VBUS_CURR	BIT(2)
>>> +#define AXP20X_ADC_EN1_VBUS_VOLT	BIT(3)
>>> +
>>> +#define AXP20X_VBUS_MON_VBUS_VALID	BIT(3)
>>> +
>>> +struct axp20x_usb_power {
>>> +	struct regmap *regmap;
>>> +	struct power_supply *supply;
>>> +};
>>> +
>>> +static irqreturn_t axp20x_usb_power_irq(int irq, void *devid)
>>> +{
>>> +	struct axp20x_usb_power *power = devid;
>>> +
>>> +	power_supply_changed(power->supply);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int axp20x_usb_power_get_property(struct power_supply *psy,
>>> +	enum power_supply_property psp, union power_supply_propval *val)
>>> +{
>>> +	struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
>>> +	unsigned int input, v;
>>> +	int r;
>>
>> Here and in probe function - it is strictly personal but I don't like
>> the 'r' variable. I got used to 'err' or 'ret' which are more
>> descriptive to me. 'r' may look like 'resource' for example.
> 
> 'ret' might work but doesn't it imply return value for this function
> instead of return value from a called function?

A little, but in this case it is indeed a return value from called
function. 'err' would fine as well.

> 
>>> +
>>> +	switch (psp) {
>>> +	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
>>> +		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
>>> +		if (r)
>>> +			return r;
>>> +
>>> +		val->intval = AXP20X_VBUS_VHOLD_uV(v);
>>> +		return 0;
>>> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>>> +		r = axp20x_read_variable_width(power->regmap,
>>> +					       AXP20X_VBUS_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_MAX:
>>> +		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
>>> +		if (r)
>>> +			return r;
>>> +
>>> +		switch (v & AXP20X_VBUS_CLIMIT_MASK) {
>>> +		case AXP20X_VBUC_CLIMIT_100mA:
>>> +			val->intval = 100000;
>>> +			break;
>>> +		case AXP20X_VBUC_CLIMIT_500mA:
>>> +			val->intval = 500000;
>>> +			break;
>>> +		case AXP20X_VBUC_CLIMIT_900mA:
>>> +			val->intval = 900000;
>>> +			break;
>>> +		case AXP20X_VBUC_CLIMIT_NONE:
>>> +			val->intval = -1;
>>> +			break;
>>> +		}
>>> +		return 0;
>>> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
>>> +		r = axp20x_read_variable_width(power->regmap,
>>> +					       AXP20X_VBUS_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_HEALTH:
>>> +		if (!(input & AXP20X_PWR_STATUS_VBUS_PRESENT)) {
>>> +			val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
>>> +			break;
>>> +		}
>>> +
>>> +		r = regmap_read(power->regmap, AXP20X_USB_OTG_STATUS, &v);
>>> +		if (r)
>>> +			return r;
>>> +
>>> +		if (!(v & AXP20X_USB_STATUS_VBUS_VALID)) {
>>> +			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
>>> +			break;
>>> +		}
>>> +
>>> +		val->intval = POWER_SUPPLY_HEALTH_GOOD;
>>> +		break;
>>> +	case POWER_SUPPLY_PROP_PRESENT:
>>> +		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_PRESENT);
>>> +		break;
>>> +	case POWER_SUPPLY_PROP_ONLINE:
>>> +		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_USED);
>>> +		break;
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static enum power_supply_property axp20x_usb_power_properties[] = {
>>> +	POWER_SUPPLY_PROP_HEALTH,
>>> +	POWER_SUPPLY_PROP_PRESENT,
>>> +	POWER_SUPPLY_PROP_ONLINE,
>>> +	POWER_SUPPLY_PROP_VOLTAGE_MIN,
>>> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
>>> +	POWER_SUPPLY_PROP_CURRENT_MAX,
>>> +	POWER_SUPPLY_PROP_CURRENT_NOW,
>>> +};
>>> +
>>> +static const struct power_supply_desc axp20x_usb_power_desc = {
>>> +	.name = "axp20x-usb",
>>> +	.type = POWER_SUPPLY_TYPE_USB,
>>> +	.properties = axp20x_usb_power_properties,
>>> +	.num_properties = ARRAY_SIZE(axp20x_usb_power_properties),
>>> +	.get_property = axp20x_usb_power_get_property,
>>> +};
>>> +
>>> +static int axp20x_usb_power_probe(struct platform_device *pdev)
>>> +{
>>> +	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
>>> +	struct power_supply_config psy_cfg = {};
>>> +	struct axp20x_usb_power *power;
>>> +	static const char * const irq_names[] = { "VBUS_PLUGIN",
>>> +		"VBUS_REMOVAL", "VBUS_VALID", "VBUS_NOT_VALID" };
>>> +	int i, irq, r;
>>> +
>>> +	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;
>>> +
>>> +	/* Enable vbus valid checking */
>>> +	r = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
>>> +		    AXP20X_VBUS_MON_VBUS_VALID, AXP20X_VBUS_MON_VBUS_VALID);
>>> +	if (r)
>>> +		return r;
>>> +
>>> +	/* Enable vbus voltage and current measurement */
>>> +	r = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
>>> +			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_VOLT,
>>> +			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_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_usb_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,
>>> +				axp20x_usb_power_irq, 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_usb_power_match[] = {
>>> +	{ .compatible = "x-powers,axp202-usb-power-supply" },
>>> +	{ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, axp20x_usb_power_match);
>>> +
>>> +static struct platform_driver axp20x_usb_power_driver = {
>>> +	.probe = axp20x_usb_power_probe,
>>> +	.driver = {
>>> +		.name = DRVNAME,
>>> +		.of_match_table = axp20x_usb_power_match,
>>> +	},
>>> +};
>>> +
>>> +module_platform_driver(axp20x_usb_power_driver);
>>> +
>>> +MODULE_AUTHOR("Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>");
>>> +MODULE_DESCRIPTION("AXP20x PMIC USB power supply status driver");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
>>> index 6d8b39a..8ec996e 100644
>>> --- a/include/linux/mfd/axp20x.h
>>> +++ b/include/linux/mfd/axp20x.h
>>> @@ -11,6 +11,8 @@
>>>  #ifndef __LINUX_MFD_AXP20X_H
>>>  #define __LINUX_MFD_AXP20X_H
>>>  
>>> +#include <linux/regmap.h>
>>> +
>>>  enum {
>>>  	AXP202_ID = 0,
>>>  	AXP209_ID,
>>> @@ -372,4 +374,26 @@ struct axp288_extcon_pdata {
>>>  	struct gpio_desc *gpio_mux_cntl;
>>>  };
>>>  
>>> +/* generic helper function for reading 9-16 bit wide regs */
>>> +static inline int axp20x_read_variable_width(struct regmap *regmap,
>>> +	unsigned int reg, unsigned int width)
>>> +{
>>> +	unsigned int reg_val, result;
>>> +	int err;
>>> +
>>> +	err = regmap_read(regmap, reg, &reg_val);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	result = reg_val << (width - 8);
>>> +
>>> +	err = regmap_read(regmap, reg + 1, &reg_val);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	result |= reg_val;
>>> +
>>> +	return result;
>>> +}
>>> +
>>>  #endif /* __LINUX_MFD_AXP20X_H */
>>
>> Is this static inline function used outside of
>> drivers/power/axp20x_usb_power.c? Looks like it isn't, but maybe you
>> have a plan for it? If not, then it shouldn't be in header file.
> 
> See comments for first iteration of this patch:
> | | This is only used twice and only in one file.
> | |
> | | Do you know of any other uses of this call that will be upstreamed
> | | shortly?  
> | 
> | Yes I plan to write drivers for the other 3 power_supply class
> | cells (ac-power, battery-charger, rtc-bat-charger) in the axp209,
> | and most of those need the same helper which I why I put it here.  

Thanks for explanation.

Best regards,
Krzysztof

-- 
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 v3 4/4] power: Add an axp20x-usb-power driver

[Krzysztof Kozlowski]

W dniu 28.06.2015 o 05:25, Bruno Pr?mont pisze:
> On Sat, 27 Jun 2015 14:28:40 Krzysztof Kozlowski wrote:
>> W dniu 26.06.2015 o 19:59, Hans de Goede pisze:
>>> This adds a driver for the usb power_supply bits of the axp20x PMICs.
>>>
>>> I initially started writing my own driver, before coming aware of
>>> Bruno Pr?mont's excellent earlier RFC with a driver for this.
>>>
>>> My driver was lacking CURRENT_MAX and VOLTAGE_MIN support Bruno's
>>> drvier has, so I've copied the code for those from his driver.
>>>
>>> Note that the AC-power-supply and battery charger bits will need separate
>>> drivers. Each one needs its own devictree child-node so that other
>>> devicetree nodes can reference the right power-supply, and thus each one
>>> will get its own mfd-cell / platform_device and platform-driver.
>>>
>>> Cc: Bruno Pr?mont <bonbons@linux-vserver.org>
>>> Acked-by: Lee Jones <lee.jones@linaro.org>
>>> Signed-off-by: Bruno Pr?mont <bonbons@linux-vserver.org>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> Changes in v2:
>>> -Split out the dt-bindings documentation into a separate patch
>>> -Renamed axp20x_read_16bit to axp20x_read_variable_width
>>> -Use better local variable names inside axp20x_read_variable_width
>>> Changes in v3:
>>> -Add Bruno's S-o-b
>>
>> Hi,
>>
>> Two minor comments below, the driver itself looks good.
>>
>>
>>> ---
>>>  drivers/power/Kconfig            |   7 ++
>>>  drivers/power/Makefile           |   1 +
>>>  drivers/power/axp20x_usb_power.c | 243 +++++++++++++++++++++++++++++++++++++++
>>>  include/linux/mfd/axp20x.h       |  24 ++++
>>>  4 files changed, 275 insertions(+)
>>>  create mode 100644 drivers/power/axp20x_usb_power.c
>>>
>>> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
>>> index 4091fb0..1fee60c 100644
>>> --- a/drivers/power/Kconfig
>>> +++ b/drivers/power/Kconfig
>>> @@ -439,6 +439,13 @@ config BATTERY_RT5033
>>>  	  The fuelgauge calculates and determines the battery state of charge
>>>  	  according to battery open circuit voltage.
>>>  
>>> +config AXP20X_POWER
>>> +	tristate "AXP20x power supply driver"
>>> +	depends on MFD_AXP20X
>>> +	help
>>> +	  This driver provides support for the power supply features of
>>> +	  AXP20x PMIC.
>>> +
>>>  source "drivers/power/reset/Kconfig"
>>>  
>>>  endif # POWER_SUPPLY
>>> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
>>> index b7b0181..ae0f27d 100644
>>> --- a/drivers/power/Makefile
>>> +++ b/drivers/power/Makefile
>>> @@ -9,6 +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_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_usb_power.c b/drivers/power/axp20x_usb_power.c
>>> new file mode 100644
>>> index 0000000..09f388e
>>> --- /dev/null
>>> +++ b/drivers/power/axp20x_usb_power.c
>>> @@ -0,0 +1,243 @@
>>> +/*
>>> + * AXP20x PMIC USB power supply status driver
>>> + *
>>> + * Copyright (C) 2015 Hans de Goede <hdegoede@redhat.com>
>>> + * Copyright (C) 2014 Bruno Pr?mont <bonbons@linux-vserver.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/device.h>
>>> +#include <linux/init.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/kernel.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-usb-power-supply"
>>> +
>>> +#define AXP20X_PWR_STATUS_VBUS_PRESENT	BIT(5)
>>> +#define AXP20X_PWR_STATUS_VBUS_USED	BIT(4)
>>> +
>>> +#define AXP20X_USB_STATUS_VBUS_VALID	BIT(2)
>>> +
>>> +#define AXP20X_VBUS_VHOLD_uV(b)		(4000000 + (((b) >> 3) & 7) * 100000)
>>> +#define AXP20X_VBUS_CLIMIT_MASK		3
>>> +#define AXP20X_VBUC_CLIMIT_900mA	0
>>> +#define AXP20X_VBUC_CLIMIT_500mA	1
>>> +#define AXP20X_VBUC_CLIMIT_100mA	2
>>> +#define AXP20X_VBUC_CLIMIT_NONE		3
>>> +
>>> +#define AXP20X_ADC_EN1_VBUS_CURR	BIT(2)
>>> +#define AXP20X_ADC_EN1_VBUS_VOLT	BIT(3)
>>> +
>>> +#define AXP20X_VBUS_MON_VBUS_VALID	BIT(3)
>>> +
>>> +struct axp20x_usb_power {
>>> +	struct regmap *regmap;
>>> +	struct power_supply *supply;
>>> +};
>>> +
>>> +static irqreturn_t axp20x_usb_power_irq(int irq, void *devid)
>>> +{
>>> +	struct axp20x_usb_power *power = devid;
>>> +
>>> +	power_supply_changed(power->supply);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int axp20x_usb_power_get_property(struct power_supply *psy,
>>> +	enum power_supply_property psp, union power_supply_propval *val)
>>> +{
>>> +	struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
>>> +	unsigned int input, v;
>>> +	int r;
>>
>> Here and in probe function - it is strictly personal but I don't like
>> the 'r' variable. I got used to 'err' or 'ret' which are more
>> descriptive to me. 'r' may look like 'resource' for example.
> 
> 'ret' might work but doesn't it imply return value for this function
> instead of return value from a called function?

A little, but in this case it is indeed a return value from called
function. 'err' would fine as well.

> 
>>> +
>>> +	switch (psp) {
>>> +	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
>>> +		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
>>> +		if (r)
>>> +			return r;
>>> +
>>> +		val->intval = AXP20X_VBUS_VHOLD_uV(v);
>>> +		return 0;
>>> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>>> +		r = axp20x_read_variable_width(power->regmap,
>>> +					       AXP20X_VBUS_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_MAX:
>>> +		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
>>> +		if (r)
>>> +			return r;
>>> +
>>> +		switch (v & AXP20X_VBUS_CLIMIT_MASK) {
>>> +		case AXP20X_VBUC_CLIMIT_100mA:
>>> +			val->intval = 100000;
>>> +			break;
>>> +		case AXP20X_VBUC_CLIMIT_500mA:
>>> +			val->intval = 500000;
>>> +			break;
>>> +		case AXP20X_VBUC_CLIMIT_900mA:
>>> +			val->intval = 900000;
>>> +			break;
>>> +		case AXP20X_VBUC_CLIMIT_NONE:
>>> +			val->intval = -1;
>>> +			break;
>>> +		}
>>> +		return 0;
>>> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
>>> +		r = axp20x_read_variable_width(power->regmap,
>>> +					       AXP20X_VBUS_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_HEALTH:
>>> +		if (!(input & AXP20X_PWR_STATUS_VBUS_PRESENT)) {
>>> +			val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
>>> +			break;
>>> +		}
>>> +
>>> +		r = regmap_read(power->regmap, AXP20X_USB_OTG_STATUS, &v);
>>> +		if (r)
>>> +			return r;
>>> +
>>> +		if (!(v & AXP20X_USB_STATUS_VBUS_VALID)) {
>>> +			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
>>> +			break;
>>> +		}
>>> +
>>> +		val->intval = POWER_SUPPLY_HEALTH_GOOD;
>>> +		break;
>>> +	case POWER_SUPPLY_PROP_PRESENT:
>>> +		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_PRESENT);
>>> +		break;
>>> +	case POWER_SUPPLY_PROP_ONLINE:
>>> +		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_USED);
>>> +		break;
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static enum power_supply_property axp20x_usb_power_properties[] = {
>>> +	POWER_SUPPLY_PROP_HEALTH,
>>> +	POWER_SUPPLY_PROP_PRESENT,
>>> +	POWER_SUPPLY_PROP_ONLINE,
>>> +	POWER_SUPPLY_PROP_VOLTAGE_MIN,
>>> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
>>> +	POWER_SUPPLY_PROP_CURRENT_MAX,
>>> +	POWER_SUPPLY_PROP_CURRENT_NOW,
>>> +};
>>> +
>>> +static const struct power_supply_desc axp20x_usb_power_desc = {
>>> +	.name = "axp20x-usb",
>>> +	.type = POWER_SUPPLY_TYPE_USB,
>>> +	.properties = axp20x_usb_power_properties,
>>> +	.num_properties = ARRAY_SIZE(axp20x_usb_power_properties),
>>> +	.get_property = axp20x_usb_power_get_property,
>>> +};
>>> +
>>> +static int axp20x_usb_power_probe(struct platform_device *pdev)
>>> +{
>>> +	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
>>> +	struct power_supply_config psy_cfg = {};
>>> +	struct axp20x_usb_power *power;
>>> +	static const char * const irq_names[] = { "VBUS_PLUGIN",
>>> +		"VBUS_REMOVAL", "VBUS_VALID", "VBUS_NOT_VALID" };
>>> +	int i, irq, r;
>>> +
>>> +	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;
>>> +
>>> +	/* Enable vbus valid checking */
>>> +	r = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
>>> +		    AXP20X_VBUS_MON_VBUS_VALID, AXP20X_VBUS_MON_VBUS_VALID);
>>> +	if (r)
>>> +		return r;
>>> +
>>> +	/* Enable vbus voltage and current measurement */
>>> +	r = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
>>> +			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_VOLT,
>>> +			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_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_usb_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,
>>> +				axp20x_usb_power_irq, 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_usb_power_match[] = {
>>> +	{ .compatible = "x-powers,axp202-usb-power-supply" },
>>> +	{ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, axp20x_usb_power_match);
>>> +
>>> +static struct platform_driver axp20x_usb_power_driver = {
>>> +	.probe = axp20x_usb_power_probe,
>>> +	.driver = {
>>> +		.name = DRVNAME,
>>> +		.of_match_table = axp20x_usb_power_match,
>>> +	},
>>> +};
>>> +
>>> +module_platform_driver(axp20x_usb_power_driver);
>>> +
>>> +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
>>> +MODULE_DESCRIPTION("AXP20x PMIC USB power supply status driver");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
>>> index 6d8b39a..8ec996e 100644
>>> --- a/include/linux/mfd/axp20x.h
>>> +++ b/include/linux/mfd/axp20x.h
>>> @@ -11,6 +11,8 @@
>>>  #ifndef __LINUX_MFD_AXP20X_H
>>>  #define __LINUX_MFD_AXP20X_H
>>>  
>>> +#include <linux/regmap.h>
>>> +
>>>  enum {
>>>  	AXP202_ID = 0,
>>>  	AXP209_ID,
>>> @@ -372,4 +374,26 @@ struct axp288_extcon_pdata {
>>>  	struct gpio_desc *gpio_mux_cntl;
>>>  };
>>>  
>>> +/* generic helper function for reading 9-16 bit wide regs */
>>> +static inline int axp20x_read_variable_width(struct regmap *regmap,
>>> +	unsigned int reg, unsigned int width)
>>> +{
>>> +	unsigned int reg_val, result;
>>> +	int err;
>>> +
>>> +	err = regmap_read(regmap, reg, &reg_val);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	result = reg_val << (width - 8);
>>> +
>>> +	err = regmap_read(regmap, reg + 1, &reg_val);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	result |= reg_val;
>>> +
>>> +	return result;
>>> +}
>>> +
>>>  #endif /* __LINUX_MFD_AXP20X_H */
>>
>> Is this static inline function used outside of
>> drivers/power/axp20x_usb_power.c? Looks like it isn't, but maybe you
>> have a plan for it? If not, then it shouldn't be in header file.
> 
> See comments for first iteration of this patch:
> | | This is only used twice and only in one file.
> | |
> | | Do you know of any other uses of this call that will be upstreamed
> | | shortly?  
> | 
> | Yes I plan to write drivers for the other 3 power_supply class
> | cells (ac-power, battery-charger, rtc-bat-charger) in the axp209,
> | and most of those need the same helper which I why I put it here.  

Thanks for explanation.

Best regards,
Krzysztof

Re: [PATCH v3 2/4] mfd: axp20x: Add missing registers, and mark more registers volatile

[Maxime Ripard]

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

On Fri, Jun 26, 2015 at 12:59:15PM +0200, Hans de Goede wrote:
> From: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
> 
> Add an extra set of registers which is necessary tu support the PMICs
> battery charger function, and mark registers which contain status bits,
> gpio status, and adc readings as volatile.
> 
> Cc: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
> Signed-off-by: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

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

[PATCH v3 2/4] mfd: axp20x: Add missing registers, and mark more registers volatile

[Maxime Ripard]

On Fri, Jun 26, 2015 at 12:59:15PM +0200, Hans de Goede wrote:
> From: Bruno Pr?mont <bonbons@linux-vserver.org>
> 
> Add an extra set of registers which is necessary tu support the PMICs
> battery charger function, and mark registers which contain status bits,
> gpio status, and adc readings as volatile.
> 
> Cc: Bruno Pr?mont <bonbons@linux-vserver.org>
> Signed-off-by: Bruno Pr?mont <bonbons@linux-vserver.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150706/94359d35/attachment.sig>

Re: [PATCH v3 3/4] mfd: axp20x: Add a cell for the usb power_supply part of the axp20x PMICs

[Maxime Ripard]

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

On Fri, Jun 26, 2015 at 12:59:16PM +0200, Hans de Goede wrote:
> Add a cell for the usb power_supply part of the axp20x PMICs.
> 
> Note that this cell is only for the usb power_supply part and not the
> ac-power / battery-charger / rtc-backup-bat-charger bits.
> 
> Depending on the board each of those must be enabled / disabled separately
> in devicetree as most boards do not use all 4. So in dt each one needs its
> own child-node of the axp20x node. Another reason for using separate child
> nodes for each is so that other devicetree nodes can have a power-supply
> property with a phandle referencing a node representing a single
> power-supply.
> 
> The decision to use a separate devicetree node for each is reflected on
> the kernel side by each getting its own mfd-cell / platform_device and
> platform-driver.
> 
> Note this commit also makes some whitespace changes to the intialization
> of existing cells in axp20x_cells, these are pure whitespace changes,
> functionally nothing changes.
> 
> Cc: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

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

[PATCH v3 3/4] mfd: axp20x: Add a cell for the usb power_supply part of the axp20x PMICs

[Maxime Ripard]

On Fri, Jun 26, 2015 at 12:59:16PM +0200, Hans de Goede wrote:
> Add a cell for the usb power_supply part of the axp20x PMICs.
> 
> Note that this cell is only for the usb power_supply part and not the
> ac-power / battery-charger / rtc-backup-bat-charger bits.
> 
> Depending on the board each of those must be enabled / disabled separately
> in devicetree as most boards do not use all 4. So in dt each one needs its
> own child-node of the axp20x node. Another reason for using separate child
> nodes for each is so that other devicetree nodes can have a power-supply
> property with a phandle referencing a node representing a single
> power-supply.
> 
> The decision to use a separate devicetree node for each is reflected on
> the kernel side by each getting its own mfd-cell / platform_device and
> platform-driver.
> 
> Note this commit also makes some whitespace changes to the intialization
> of existing cells in axp20x_cells, these are pure whitespace changes,
> functionally nothing changes.
> 
> Cc: Bruno Pr?mont <bonbons@linux-vserver.org>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Bruno Pr?mont <bonbons@linux-vserver.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150706/70906e2c/attachment.sig>

Re: [PATCH v3 4/4] power: Add an axp20x-usb-power driver

[Maxime Ripard]

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

Hi,

On Fri, Jun 26, 2015 at 12:59:17PM +0200, Hans de Goede wrote:
> This adds a driver for the usb power_supply bits of the axp20x PMICs.
> 
> I initially started writing my own driver, before coming aware of
> Bruno Prémont's excellent earlier RFC with a driver for this.
> 
> My driver was lacking CURRENT_MAX and VOLTAGE_MIN support Bruno's
> drvier has, so I've copied the code for those from his driver.
> 
> Note that the AC-power-supply and battery charger bits will need separate
> drivers. Each one needs its own devictree child-node so that other
> devicetree nodes can reference the right power-supply, and thus each one
> will get its own mfd-cell / platform_device and platform-driver.
> 
> Cc: Bruno Prémont <bonbons@linux-vserver.org>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Split out the dt-bindings documentation into a separate patch
> -Renamed axp20x_read_16bit to axp20x_read_variable_width
> -Use better local variable names inside axp20x_read_variable_width
> Changes in v3:
> -Add Bruno's S-o-b
> ---
>  drivers/power/Kconfig            |   7 ++
>  drivers/power/Makefile           |   1 +
>  drivers/power/axp20x_usb_power.c | 243 +++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/axp20x.h       |  24 ++++
>  4 files changed, 275 insertions(+)
>  create mode 100644 drivers/power/axp20x_usb_power.c
> 
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index 4091fb0..1fee60c 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -439,6 +439,13 @@ config BATTERY_RT5033
>  	  The fuelgauge calculates and determines the battery state of charge
>  	  according to battery open circuit voltage.
>  
> +config AXP20X_POWER
> +	tristate "AXP20x power supply driver"
> +	depends on MFD_AXP20X
> +	help
> +	  This driver provides support for the power supply features of
> +	  AXP20x PMIC.
> +
>  source "drivers/power/reset/Kconfig"
>  
>  endif # POWER_SUPPLY
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index b7b0181..ae0f27d 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -9,6 +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_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_usb_power.c b/drivers/power/axp20x_usb_power.c
> new file mode 100644
> index 0000000..09f388e
> --- /dev/null
> +++ b/drivers/power/axp20x_usb_power.c
> @@ -0,0 +1,243 @@
> +/*
> + * AXP20x PMIC USB power supply status driver
> + *
> + * Copyright (C) 2015 Hans de Goede <hdegoede@redhat.com>
> + * Copyright (C) 2014 Bruno Prémont <bonbons@linux-vserver.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/device.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.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-usb-power-supply"
> +
> +#define AXP20X_PWR_STATUS_VBUS_PRESENT	BIT(5)
> +#define AXP20X_PWR_STATUS_VBUS_USED	BIT(4)
> +
> +#define AXP20X_USB_STATUS_VBUS_VALID	BIT(2)
> +
> +#define AXP20X_VBUS_VHOLD_uV(b)		(4000000 + (((b) >> 3) & 7) * 100000)
> +#define AXP20X_VBUS_CLIMIT_MASK		3
> +#define AXP20X_VBUC_CLIMIT_900mA	0
> +#define AXP20X_VBUC_CLIMIT_500mA	1
> +#define AXP20X_VBUC_CLIMIT_100mA	2
> +#define AXP20X_VBUC_CLIMIT_NONE		3
> +
> +#define AXP20X_ADC_EN1_VBUS_CURR	BIT(2)
> +#define AXP20X_ADC_EN1_VBUS_VOLT	BIT(3)
> +
> +#define AXP20X_VBUS_MON_VBUS_VALID	BIT(3)
> +
> +struct axp20x_usb_power {
> +	struct regmap *regmap;
> +	struct power_supply *supply;
> +};
> +
> +static irqreturn_t axp20x_usb_power_irq(int irq, void *devid)
> +{
> +	struct axp20x_usb_power *power = devid;
> +
> +	power_supply_changed(power->supply);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int axp20x_usb_power_get_property(struct power_supply *psy,
> +	enum power_supply_property psp, union power_supply_propval *val)
> +{
> +	struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
> +	unsigned int input, v;
> +	int r;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
> +		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
> +		if (r)
> +			return r;
> +
> +		val->intval = AXP20X_VBUS_VHOLD_uV(v);
> +		return 0;
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		r = axp20x_read_variable_width(power->regmap,
> +					       AXP20X_VBUS_V_ADC_H, 12);
> +		if (r < 0)
> +			return r;
> +
> +		val->intval = r * 1700; /* 1 step = 1.7 mV */

Eventually, as we will have more users (the various power supplies,
the TS pin?), we should have an IIO driver for the internal ADC.

Do you know how close it is from the AXP288's ?


> +		return 0;
> +	case POWER_SUPPLY_PROP_CURRENT_MAX:
> +		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
> +		if (r)
> +			return r;
> +
> +		switch (v & AXP20X_VBUS_CLIMIT_MASK) {
> +		case AXP20X_VBUC_CLIMIT_100mA:
> +			val->intval = 100000;
> +			break;
> +		case AXP20X_VBUC_CLIMIT_500mA:
> +			val->intval = 500000;
> +			break;
> +		case AXP20X_VBUC_CLIMIT_900mA:
> +			val->intval = 900000;
> +			break;
> +		case AXP20X_VBUC_CLIMIT_NONE:
> +			val->intval = -1;
> +			break;
> +		}
> +		return 0;
> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +		r = axp20x_read_variable_width(power->regmap,
> +					       AXP20X_VBUS_I_ADC_H, 12);
> +		if (r < 0)
> +			return r;
> +
> +		val->intval = r * 375; /* 1 step = 0.375 mA */

And here too.

> +		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_HEALTH:
> +		if (!(input & AXP20X_PWR_STATUS_VBUS_PRESENT)) {
> +			val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
> +			break;
> +		}
> +
> +		r = regmap_read(power->regmap, AXP20X_USB_OTG_STATUS, &v);
> +		if (r)
> +			return r;
> +
> +		if (!(v & AXP20X_USB_STATUS_VBUS_VALID)) {
> +			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
> +			break;
> +		}
> +
> +		val->intval = POWER_SUPPLY_HEALTH_GOOD;
> +		break;
> +	case POWER_SUPPLY_PROP_PRESENT:
> +		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_PRESENT);
> +		break;
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_USED);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static enum power_supply_property axp20x_usb_power_properties[] = {
> +	POWER_SUPPLY_PROP_HEALTH,
> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_VOLTAGE_MIN,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_MAX,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +};
> +
> +static const struct power_supply_desc axp20x_usb_power_desc = {
> +	.name = "axp20x-usb",
> +	.type = POWER_SUPPLY_TYPE_USB,
> +	.properties = axp20x_usb_power_properties,
> +	.num_properties = ARRAY_SIZE(axp20x_usb_power_properties),
> +	.get_property = axp20x_usb_power_get_property,
> +};
> +
> +static int axp20x_usb_power_probe(struct platform_device *pdev)
> +{
> +	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> +	struct power_supply_config psy_cfg = {};
> +	struct axp20x_usb_power *power;
> +	static const char * const irq_names[] = { "VBUS_PLUGIN",
> +		"VBUS_REMOVAL", "VBUS_VALID", "VBUS_NOT_VALID" };
> +	int i, irq, r;
> +
> +	if (!of_device_is_available(pdev->dev.of_node))
> +		return -ENODEV;

That looks odd. Is it even 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;

It should probably be wise to test that the axp20x pointer is not
NULL.

> +
> +	/* Enable vbus valid checking */
> +	r = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
> +		    AXP20X_VBUS_MON_VBUS_VALID, AXP20X_VBUS_MON_VBUS_VALID);
> +	if (r)
> +		return r;
> +
> +	/* Enable vbus voltage and current measurement */
> +	r = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
> +			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_VOLT,
> +			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_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_usb_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,
> +				axp20x_usb_power_irq, 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_usb_power_match[] = {
> +	{ .compatible = "x-powers,axp202-usb-power-supply" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, axp20x_usb_power_match);
> +
> +static struct platform_driver axp20x_usb_power_driver = {
> +	.probe = axp20x_usb_power_probe,
> +	.driver = {
> +		.name = DRVNAME,
> +		.of_match_table = axp20x_usb_power_match,
> +	},
> +};
> +
> +module_platform_driver(axp20x_usb_power_driver);
> +
> +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
> +MODULE_DESCRIPTION("AXP20x PMIC USB power supply status driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index 6d8b39a..8ec996e 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -11,6 +11,8 @@
>  #ifndef __LINUX_MFD_AXP20X_H
>  #define __LINUX_MFD_AXP20X_H
>  
> +#include <linux/regmap.h>
> +
>  enum {
>  	AXP202_ID = 0,
>  	AXP209_ID,
> @@ -372,4 +374,26 @@ struct axp288_extcon_pdata {
>  	struct gpio_desc *gpio_mux_cntl;
>  };
>  
> +/* generic helper function for reading 9-16 bit wide regs */
> +static inline int axp20x_read_variable_width(struct regmap *regmap,
> +	unsigned int reg, unsigned int width)
> +{
> +	unsigned int reg_val, result;
> +	int err;
> +
> +	err = regmap_read(regmap, reg, &reg_val);
> +	if (err)
> +		return err;
> +
> +	result = reg_val << (width - 8);
> +
> +	err = regmap_read(regmap, reg + 1, &reg_val);
> +	if (err)
> +		return err;
> +
> +	result |= reg_val;
> +
> +	return result;
> +}
> +
>  #endif /* __LINUX_MFD_AXP20X_H */
> -- 
> 2.4.3
> 

Looks good otherwise, thanks!

Maxime

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

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

[PATCH v3 4/4] power: Add an axp20x-usb-power driver

[Maxime Ripard]

Hi,

On Fri, Jun 26, 2015 at 12:59:17PM +0200, Hans de Goede wrote:
> This adds a driver for the usb power_supply bits of the axp20x PMICs.
> 
> I initially started writing my own driver, before coming aware of
> Bruno Pr?mont's excellent earlier RFC with a driver for this.
> 
> My driver was lacking CURRENT_MAX and VOLTAGE_MIN support Bruno's
> drvier has, so I've copied the code for those from his driver.
> 
> Note that the AC-power-supply and battery charger bits will need separate
> drivers. Each one needs its own devictree child-node so that other
> devicetree nodes can reference the right power-supply, and thus each one
> will get its own mfd-cell / platform_device and platform-driver.
> 
> Cc: Bruno Pr?mont <bonbons@linux-vserver.org>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Bruno Pr?mont <bonbons@linux-vserver.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Split out the dt-bindings documentation into a separate patch
> -Renamed axp20x_read_16bit to axp20x_read_variable_width
> -Use better local variable names inside axp20x_read_variable_width
> Changes in v3:
> -Add Bruno's S-o-b
> ---
>  drivers/power/Kconfig            |   7 ++
>  drivers/power/Makefile           |   1 +
>  drivers/power/axp20x_usb_power.c | 243 +++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/axp20x.h       |  24 ++++
>  4 files changed, 275 insertions(+)
>  create mode 100644 drivers/power/axp20x_usb_power.c
> 
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index 4091fb0..1fee60c 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -439,6 +439,13 @@ config BATTERY_RT5033
>  	  The fuelgauge calculates and determines the battery state of charge
>  	  according to battery open circuit voltage.
>  
> +config AXP20X_POWER
> +	tristate "AXP20x power supply driver"
> +	depends on MFD_AXP20X
> +	help
> +	  This driver provides support for the power supply features of
> +	  AXP20x PMIC.
> +
>  source "drivers/power/reset/Kconfig"
>  
>  endif # POWER_SUPPLY
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index b7b0181..ae0f27d 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -9,6 +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_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_usb_power.c b/drivers/power/axp20x_usb_power.c
> new file mode 100644
> index 0000000..09f388e
> --- /dev/null
> +++ b/drivers/power/axp20x_usb_power.c
> @@ -0,0 +1,243 @@
> +/*
> + * AXP20x PMIC USB power supply status driver
> + *
> + * Copyright (C) 2015 Hans de Goede <hdegoede@redhat.com>
> + * Copyright (C) 2014 Bruno Pr?mont <bonbons@linux-vserver.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/device.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.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-usb-power-supply"
> +
> +#define AXP20X_PWR_STATUS_VBUS_PRESENT	BIT(5)
> +#define AXP20X_PWR_STATUS_VBUS_USED	BIT(4)
> +
> +#define AXP20X_USB_STATUS_VBUS_VALID	BIT(2)
> +
> +#define AXP20X_VBUS_VHOLD_uV(b)		(4000000 + (((b) >> 3) & 7) * 100000)
> +#define AXP20X_VBUS_CLIMIT_MASK		3
> +#define AXP20X_VBUC_CLIMIT_900mA	0
> +#define AXP20X_VBUC_CLIMIT_500mA	1
> +#define AXP20X_VBUC_CLIMIT_100mA	2
> +#define AXP20X_VBUC_CLIMIT_NONE		3
> +
> +#define AXP20X_ADC_EN1_VBUS_CURR	BIT(2)
> +#define AXP20X_ADC_EN1_VBUS_VOLT	BIT(3)
> +
> +#define AXP20X_VBUS_MON_VBUS_VALID	BIT(3)
> +
> +struct axp20x_usb_power {
> +	struct regmap *regmap;
> +	struct power_supply *supply;
> +};
> +
> +static irqreturn_t axp20x_usb_power_irq(int irq, void *devid)
> +{
> +	struct axp20x_usb_power *power = devid;
> +
> +	power_supply_changed(power->supply);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int axp20x_usb_power_get_property(struct power_supply *psy,
> +	enum power_supply_property psp, union power_supply_propval *val)
> +{
> +	struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
> +	unsigned int input, v;
> +	int r;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
> +		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
> +		if (r)
> +			return r;
> +
> +		val->intval = AXP20X_VBUS_VHOLD_uV(v);
> +		return 0;
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		r = axp20x_read_variable_width(power->regmap,
> +					       AXP20X_VBUS_V_ADC_H, 12);
> +		if (r < 0)
> +			return r;
> +
> +		val->intval = r * 1700; /* 1 step = 1.7 mV */

Eventually, as we will have more users (the various power supplies,
the TS pin?), we should have an IIO driver for the internal ADC.

Do you know how close it is from the AXP288's ?


> +		return 0;
> +	case POWER_SUPPLY_PROP_CURRENT_MAX:
> +		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
> +		if (r)
> +			return r;
> +
> +		switch (v & AXP20X_VBUS_CLIMIT_MASK) {
> +		case AXP20X_VBUC_CLIMIT_100mA:
> +			val->intval = 100000;
> +			break;
> +		case AXP20X_VBUC_CLIMIT_500mA:
> +			val->intval = 500000;
> +			break;
> +		case AXP20X_VBUC_CLIMIT_900mA:
> +			val->intval = 900000;
> +			break;
> +		case AXP20X_VBUC_CLIMIT_NONE:
> +			val->intval = -1;
> +			break;
> +		}
> +		return 0;
> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +		r = axp20x_read_variable_width(power->regmap,
> +					       AXP20X_VBUS_I_ADC_H, 12);
> +		if (r < 0)
> +			return r;
> +
> +		val->intval = r * 375; /* 1 step = 0.375 mA */

And here too.

> +		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_HEALTH:
> +		if (!(input & AXP20X_PWR_STATUS_VBUS_PRESENT)) {
> +			val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
> +			break;
> +		}
> +
> +		r = regmap_read(power->regmap, AXP20X_USB_OTG_STATUS, &v);
> +		if (r)
> +			return r;
> +
> +		if (!(v & AXP20X_USB_STATUS_VBUS_VALID)) {
> +			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
> +			break;
> +		}
> +
> +		val->intval = POWER_SUPPLY_HEALTH_GOOD;
> +		break;
> +	case POWER_SUPPLY_PROP_PRESENT:
> +		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_PRESENT);
> +		break;
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_USED);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static enum power_supply_property axp20x_usb_power_properties[] = {
> +	POWER_SUPPLY_PROP_HEALTH,
> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_VOLTAGE_MIN,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_MAX,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +};
> +
> +static const struct power_supply_desc axp20x_usb_power_desc = {
> +	.name = "axp20x-usb",
> +	.type = POWER_SUPPLY_TYPE_USB,
> +	.properties = axp20x_usb_power_properties,
> +	.num_properties = ARRAY_SIZE(axp20x_usb_power_properties),
> +	.get_property = axp20x_usb_power_get_property,
> +};
> +
> +static int axp20x_usb_power_probe(struct platform_device *pdev)
> +{
> +	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> +	struct power_supply_config psy_cfg = {};
> +	struct axp20x_usb_power *power;
> +	static const char * const irq_names[] = { "VBUS_PLUGIN",
> +		"VBUS_REMOVAL", "VBUS_VALID", "VBUS_NOT_VALID" };
> +	int i, irq, r;
> +
> +	if (!of_device_is_available(pdev->dev.of_node))
> +		return -ENODEV;

That looks odd. Is it even 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;

It should probably be wise to test that the axp20x pointer is not
NULL.

> +
> +	/* Enable vbus valid checking */
> +	r = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
> +		    AXP20X_VBUS_MON_VBUS_VALID, AXP20X_VBUS_MON_VBUS_VALID);
> +	if (r)
> +		return r;
> +
> +	/* Enable vbus voltage and current measurement */
> +	r = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
> +			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_VOLT,
> +			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_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_usb_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,
> +				axp20x_usb_power_irq, 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_usb_power_match[] = {
> +	{ .compatible = "x-powers,axp202-usb-power-supply" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, axp20x_usb_power_match);
> +
> +static struct platform_driver axp20x_usb_power_driver = {
> +	.probe = axp20x_usb_power_probe,
> +	.driver = {
> +		.name = DRVNAME,
> +		.of_match_table = axp20x_usb_power_match,
> +	},
> +};
> +
> +module_platform_driver(axp20x_usb_power_driver);
> +
> +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
> +MODULE_DESCRIPTION("AXP20x PMIC USB power supply status driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index 6d8b39a..8ec996e 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -11,6 +11,8 @@
>  #ifndef __LINUX_MFD_AXP20X_H
>  #define __LINUX_MFD_AXP20X_H
>  
> +#include <linux/regmap.h>
> +
>  enum {
>  	AXP202_ID = 0,
>  	AXP209_ID,
> @@ -372,4 +374,26 @@ struct axp288_extcon_pdata {
>  	struct gpio_desc *gpio_mux_cntl;
>  };
>  
> +/* generic helper function for reading 9-16 bit wide regs */
> +static inline int axp20x_read_variable_width(struct regmap *regmap,
> +	unsigned int reg, unsigned int width)
> +{
> +	unsigned int reg_val, result;
> +	int err;
> +
> +	err = regmap_read(regmap, reg, &reg_val);
> +	if (err)
> +		return err;
> +
> +	result = reg_val << (width - 8);
> +
> +	err = regmap_read(regmap, reg + 1, &reg_val);
> +	if (err)
> +		return err;
> +
> +	result |= reg_val;
> +
> +	return result;
> +}
> +
>  #endif /* __LINUX_MFD_AXP20X_H */
> -- 
> 2.4.3
> 

Looks good otherwise, thanks!

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150706/c0e06b65/attachment-0001.sig>

Re: Re: [PATCH v3 4/4] power: Add an axp20x-usb-power driver

[Hans de Goede]

Hi,

On 06-07-15 12:40, Maxime Ripard wrote:
> Hi,
>
> On Fri, Jun 26, 2015 at 12:59:17PM +0200, Hans de Goede wrote:
>> This adds a driver for the usb power_supply bits of the axp20x PMICs.
>>
>> I initially started writing my own driver, before coming aware of
>> Bruno Prémont's excellent earlier RFC with a driver for this.
>>
>> My driver was lacking CURRENT_MAX and VOLTAGE_MIN support Bruno's
>> drvier has, so I've copied the code for those from his driver.
>>
>> Note that the AC-power-supply and battery charger bits will need separate
>> drivers. Each one needs its own devictree child-node so that other
>> devicetree nodes can reference the right power-supply, and thus each one
>> will get its own mfd-cell / platform_device and platform-driver.
>>
>> Cc: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
>> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Signed-off-by: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>> Changes in v2:
>> -Split out the dt-bindings documentation into a separate patch
>> -Renamed axp20x_read_16bit to axp20x_read_variable_width
>> -Use better local variable names inside axp20x_read_variable_width
>> Changes in v3:
>> -Add Bruno's S-o-b
>> ---
>>   drivers/power/Kconfig            |   7 ++
>>   drivers/power/Makefile           |   1 +
>>   drivers/power/axp20x_usb_power.c | 243 +++++++++++++++++++++++++++++++++++++++
>>   include/linux/mfd/axp20x.h       |  24 ++++
>>   4 files changed, 275 insertions(+)
>>   create mode 100644 drivers/power/axp20x_usb_power.c
>>
>> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
>> index 4091fb0..1fee60c 100644
>> --- a/drivers/power/Kconfig
>> +++ b/drivers/power/Kconfig
>> @@ -439,6 +439,13 @@ config BATTERY_RT5033
>>   	  The fuelgauge calculates and determines the battery state of charge
>>   	  according to battery open circuit voltage.
>>
>> +config AXP20X_POWER
>> +	tristate "AXP20x power supply driver"
>> +	depends on MFD_AXP20X
>> +	help
>> +	  This driver provides support for the power supply features of
>> +	  AXP20x PMIC.
>> +
>>   source "drivers/power/reset/Kconfig"
>>
>>   endif # POWER_SUPPLY
>> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
>> index b7b0181..ae0f27d 100644
>> --- a/drivers/power/Makefile
>> +++ b/drivers/power/Makefile
>> @@ -9,6 +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_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_usb_power.c b/drivers/power/axp20x_usb_power.c
>> new file mode 100644
>> index 0000000..09f388e
>> --- /dev/null
>> +++ b/drivers/power/axp20x_usb_power.c
>> @@ -0,0 +1,243 @@
>> +/*
>> + * AXP20x PMIC USB power supply status driver
>> + *
>> + * Copyright (C) 2015 Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> + * Copyright (C) 2014 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/device.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.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-usb-power-supply"
>> +
>> +#define AXP20X_PWR_STATUS_VBUS_PRESENT	BIT(5)
>> +#define AXP20X_PWR_STATUS_VBUS_USED	BIT(4)
>> +
>> +#define AXP20X_USB_STATUS_VBUS_VALID	BIT(2)
>> +
>> +#define AXP20X_VBUS_VHOLD_uV(b)		(4000000 + (((b) >> 3) & 7) * 100000)
>> +#define AXP20X_VBUS_CLIMIT_MASK		3
>> +#define AXP20X_VBUC_CLIMIT_900mA	0
>> +#define AXP20X_VBUC_CLIMIT_500mA	1
>> +#define AXP20X_VBUC_CLIMIT_100mA	2
>> +#define AXP20X_VBUC_CLIMIT_NONE		3
>> +
>> +#define AXP20X_ADC_EN1_VBUS_CURR	BIT(2)
>> +#define AXP20X_ADC_EN1_VBUS_VOLT	BIT(3)
>> +
>> +#define AXP20X_VBUS_MON_VBUS_VALID	BIT(3)
>> +
>> +struct axp20x_usb_power {
>> +	struct regmap *regmap;
>> +	struct power_supply *supply;
>> +};
>> +
>> +static irqreturn_t axp20x_usb_power_irq(int irq, void *devid)
>> +{
>> +	struct axp20x_usb_power *power = devid;
>> +
>> +	power_supply_changed(power->supply);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int axp20x_usb_power_get_property(struct power_supply *psy,
>> +	enum power_supply_property psp, union power_supply_propval *val)
>> +{
>> +	struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
>> +	unsigned int input, v;
>> +	int r;
>> +
>> +	switch (psp) {
>> +	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
>> +		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
>> +		if (r)
>> +			return r;
>> +
>> +		val->intval = AXP20X_VBUS_VHOLD_uV(v);
>> +		return 0;
>> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>> +		r = axp20x_read_variable_width(power->regmap,
>> +					       AXP20X_VBUS_V_ADC_H, 12);
>> +		if (r < 0)
>> +			return r;
>> +
>> +		val->intval = r * 1700; /* 1 step = 1.7 mV */
>
> Eventually, as we will have more users (the various power supplies,
> the TS pin?), we should have an IIO driver for the internal ADC.

IMHO an IIO driver is only useful in case the TS or gpio pins are
used in ADC mode, and sofar we've not encountered any boards using that,

In my expereince the IIO framework is overly complicated and usually gets
in the way more then it helps.

> Do you know how close it is from the AXP288's ?

No I've not compared the 2.


>> +		return 0;
>> +	case POWER_SUPPLY_PROP_CURRENT_MAX:
>> +		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
>> +		if (r)
>> +			return r;
>> +
>> +		switch (v & AXP20X_VBUS_CLIMIT_MASK) {
>> +		case AXP20X_VBUC_CLIMIT_100mA:
>> +			val->intval = 100000;
>> +			break;
>> +		case AXP20X_VBUC_CLIMIT_500mA:
>> +			val->intval = 500000;
>> +			break;
>> +		case AXP20X_VBUC_CLIMIT_900mA:
>> +			val->intval = 900000;
>> +			break;
>> +		case AXP20X_VBUC_CLIMIT_NONE:
>> +			val->intval = -1;
>> +			break;
>> +		}
>> +		return 0;
>> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
>> +		r = axp20x_read_variable_width(power->regmap,
>> +					       AXP20X_VBUS_I_ADC_H, 12);
>> +		if (r < 0)
>> +			return r;
>> +
>> +		val->intval = r * 375; /* 1 step = 0.375 mA */
>
> And here too.
>
>> +		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_HEALTH:
>> +		if (!(input & AXP20X_PWR_STATUS_VBUS_PRESENT)) {
>> +			val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
>> +			break;
>> +		}
>> +
>> +		r = regmap_read(power->regmap, AXP20X_USB_OTG_STATUS, &v);
>> +		if (r)
>> +			return r;
>> +
>> +		if (!(v & AXP20X_USB_STATUS_VBUS_VALID)) {
>> +			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
>> +			break;
>> +		}
>> +
>> +		val->intval = POWER_SUPPLY_HEALTH_GOOD;
>> +		break;
>> +	case POWER_SUPPLY_PROP_PRESENT:
>> +		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_PRESENT);
>> +		break;
>> +	case POWER_SUPPLY_PROP_ONLINE:
>> +		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_USED);
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static enum power_supply_property axp20x_usb_power_properties[] = {
>> +	POWER_SUPPLY_PROP_HEALTH,
>> +	POWER_SUPPLY_PROP_PRESENT,
>> +	POWER_SUPPLY_PROP_ONLINE,
>> +	POWER_SUPPLY_PROP_VOLTAGE_MIN,
>> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
>> +	POWER_SUPPLY_PROP_CURRENT_MAX,
>> +	POWER_SUPPLY_PROP_CURRENT_NOW,
>> +};
>> +
>> +static const struct power_supply_desc axp20x_usb_power_desc = {
>> +	.name = "axp20x-usb",
>> +	.type = POWER_SUPPLY_TYPE_USB,
>> +	.properties = axp20x_usb_power_properties,
>> +	.num_properties = ARRAY_SIZE(axp20x_usb_power_properties),
>> +	.get_property = axp20x_usb_power_get_property,
>> +};
>> +
>> +static int axp20x_usb_power_probe(struct platform_device *pdev)
>> +{
>> +	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
>> +	struct power_supply_config psy_cfg = {};
>> +	struct axp20x_usb_power *power;
>> +	static const char * const irq_names[] = { "VBUS_PLUGIN",
>> +		"VBUS_REMOVAL", "VBUS_VALID", "VBUS_NOT_VALID" };
>> +	int i, irq, r;
>> +
>> +	if (!of_device_is_available(pdev->dev.of_node))
>> +		return -ENODEV;
>
> That looks odd. Is it even going to be probed in the first place?

Yes, the mfd framework will instantiate all cells listed for an mfd
device without checking there are compatible child-nodes present
in the dtb.

>> +
>> +	power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
>> +	if (!power)
>> +		return -ENOMEM;
>> +
>> +	power->regmap = axp20x->regmap;
>
> It should probably be wise to test that the axp20x pointer is not
> NULL.

The platform device we are probing has been instantiated by the mfd
framework after setting the parent device driver ptr, so this will never
be not NULL.

>> +
>> +	/* Enable vbus valid checking */
>> +	r = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
>> +		    AXP20X_VBUS_MON_VBUS_VALID, AXP20X_VBUS_MON_VBUS_VALID);
>> +	if (r)
>> +		return r;
>> +
>> +	/* Enable vbus voltage and current measurement */
>> +	r = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
>> +			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_VOLT,
>> +			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_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_usb_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,
>> +				axp20x_usb_power_irq, 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_usb_power_match[] = {
>> +	{ .compatible = "x-powers,axp202-usb-power-supply" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, axp20x_usb_power_match);
>> +
>> +static struct platform_driver axp20x_usb_power_driver = {
>> +	.probe = axp20x_usb_power_probe,
>> +	.driver = {
>> +		.name = DRVNAME,
>> +		.of_match_table = axp20x_usb_power_match,
>> +	},
>> +};
>> +
>> +module_platform_driver(axp20x_usb_power_driver);
>> +
>> +MODULE_AUTHOR("Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>");
>> +MODULE_DESCRIPTION("AXP20x PMIC USB power supply status driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
>> index 6d8b39a..8ec996e 100644
>> --- a/include/linux/mfd/axp20x.h
>> +++ b/include/linux/mfd/axp20x.h
>> @@ -11,6 +11,8 @@
>>   #ifndef __LINUX_MFD_AXP20X_H
>>   #define __LINUX_MFD_AXP20X_H
>>
>> +#include <linux/regmap.h>
>> +
>>   enum {
>>   	AXP202_ID = 0,
>>   	AXP209_ID,
>> @@ -372,4 +374,26 @@ struct axp288_extcon_pdata {
>>   	struct gpio_desc *gpio_mux_cntl;
>>   };
>>
>> +/* generic helper function for reading 9-16 bit wide regs */
>> +static inline int axp20x_read_variable_width(struct regmap *regmap,
>> +	unsigned int reg, unsigned int width)
>> +{
>> +	unsigned int reg_val, result;
>> +	int err;
>> +
>> +	err = regmap_read(regmap, reg, &reg_val);
>> +	if (err)
>> +		return err;
>> +
>> +	result = reg_val << (width - 8);
>> +
>> +	err = regmap_read(regmap, reg + 1, &reg_val);
>> +	if (err)
>> +		return err;
>> +
>> +	result |= reg_val;
>> +
>> +	return result;
>> +}
>> +
>>   #endif /* __LINUX_MFD_AXP20X_H */
>> --
>> 2.4.3

Regards,

Hans

-- 
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.

[linux-sunxi] Re: [PATCH v3 4/4] power: Add an axp20x-usb-power driver

[Hans de Goede]

Hi,

On 06-07-15 12:40, Maxime Ripard wrote:
> Hi,
>
> On Fri, Jun 26, 2015 at 12:59:17PM +0200, Hans de Goede wrote:
>> This adds a driver for the usb power_supply bits of the axp20x PMICs.
>>
>> I initially started writing my own driver, before coming aware of
>> Bruno Pr?mont's excellent earlier RFC with a driver for this.
>>
>> My driver was lacking CURRENT_MAX and VOLTAGE_MIN support Bruno's
>> drvier has, so I've copied the code for those from his driver.
>>
>> Note that the AC-power-supply and battery charger bits will need separate
>> drivers. Each one needs its own devictree child-node so that other
>> devicetree nodes can reference the right power-supply, and thus each one
>> will get its own mfd-cell / platform_device and platform-driver.
>>
>> Cc: Bruno Pr?mont <bonbons@linux-vserver.org>
>> Acked-by: Lee Jones <lee.jones@linaro.org>
>> Signed-off-by: Bruno Pr?mont <bonbons@linux-vserver.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -Split out the dt-bindings documentation into a separate patch
>> -Renamed axp20x_read_16bit to axp20x_read_variable_width
>> -Use better local variable names inside axp20x_read_variable_width
>> Changes in v3:
>> -Add Bruno's S-o-b
>> ---
>>   drivers/power/Kconfig            |   7 ++
>>   drivers/power/Makefile           |   1 +
>>   drivers/power/axp20x_usb_power.c | 243 +++++++++++++++++++++++++++++++++++++++
>>   include/linux/mfd/axp20x.h       |  24 ++++
>>   4 files changed, 275 insertions(+)
>>   create mode 100644 drivers/power/axp20x_usb_power.c
>>
>> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
>> index 4091fb0..1fee60c 100644
>> --- a/drivers/power/Kconfig
>> +++ b/drivers/power/Kconfig
>> @@ -439,6 +439,13 @@ config BATTERY_RT5033
>>   	  The fuelgauge calculates and determines the battery state of charge
>>   	  according to battery open circuit voltage.
>>
>> +config AXP20X_POWER
>> +	tristate "AXP20x power supply driver"
>> +	depends on MFD_AXP20X
>> +	help
>> +	  This driver provides support for the power supply features of
>> +	  AXP20x PMIC.
>> +
>>   source "drivers/power/reset/Kconfig"
>>
>>   endif # POWER_SUPPLY
>> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
>> index b7b0181..ae0f27d 100644
>> --- a/drivers/power/Makefile
>> +++ b/drivers/power/Makefile
>> @@ -9,6 +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_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_usb_power.c b/drivers/power/axp20x_usb_power.c
>> new file mode 100644
>> index 0000000..09f388e
>> --- /dev/null
>> +++ b/drivers/power/axp20x_usb_power.c
>> @@ -0,0 +1,243 @@
>> +/*
>> + * AXP20x PMIC USB power supply status driver
>> + *
>> + * Copyright (C) 2015 Hans de Goede <hdegoede@redhat.com>
>> + * Copyright (C) 2014 Bruno Pr?mont <bonbons@linux-vserver.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/device.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.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-usb-power-supply"
>> +
>> +#define AXP20X_PWR_STATUS_VBUS_PRESENT	BIT(5)
>> +#define AXP20X_PWR_STATUS_VBUS_USED	BIT(4)
>> +
>> +#define AXP20X_USB_STATUS_VBUS_VALID	BIT(2)
>> +
>> +#define AXP20X_VBUS_VHOLD_uV(b)		(4000000 + (((b) >> 3) & 7) * 100000)
>> +#define AXP20X_VBUS_CLIMIT_MASK		3
>> +#define AXP20X_VBUC_CLIMIT_900mA	0
>> +#define AXP20X_VBUC_CLIMIT_500mA	1
>> +#define AXP20X_VBUC_CLIMIT_100mA	2
>> +#define AXP20X_VBUC_CLIMIT_NONE		3
>> +
>> +#define AXP20X_ADC_EN1_VBUS_CURR	BIT(2)
>> +#define AXP20X_ADC_EN1_VBUS_VOLT	BIT(3)
>> +
>> +#define AXP20X_VBUS_MON_VBUS_VALID	BIT(3)
>> +
>> +struct axp20x_usb_power {
>> +	struct regmap *regmap;
>> +	struct power_supply *supply;
>> +};
>> +
>> +static irqreturn_t axp20x_usb_power_irq(int irq, void *devid)
>> +{
>> +	struct axp20x_usb_power *power = devid;
>> +
>> +	power_supply_changed(power->supply);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int axp20x_usb_power_get_property(struct power_supply *psy,
>> +	enum power_supply_property psp, union power_supply_propval *val)
>> +{
>> +	struct axp20x_usb_power *power = power_supply_get_drvdata(psy);
>> +	unsigned int input, v;
>> +	int r;
>> +
>> +	switch (psp) {
>> +	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
>> +		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
>> +		if (r)
>> +			return r;
>> +
>> +		val->intval = AXP20X_VBUS_VHOLD_uV(v);
>> +		return 0;
>> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>> +		r = axp20x_read_variable_width(power->regmap,
>> +					       AXP20X_VBUS_V_ADC_H, 12);
>> +		if (r < 0)
>> +			return r;
>> +
>> +		val->intval = r * 1700; /* 1 step = 1.7 mV */
>
> Eventually, as we will have more users (the various power supplies,
> the TS pin?), we should have an IIO driver for the internal ADC.

IMHO an IIO driver is only useful in case the TS or gpio pins are
used in ADC mode, and sofar we've not encountered any boards using that,

In my expereince the IIO framework is overly complicated and usually gets
in the way more then it helps.

> Do you know how close it is from the AXP288's ?

No I've not compared the 2.


>> +		return 0;
>> +	case POWER_SUPPLY_PROP_CURRENT_MAX:
>> +		r = regmap_read(power->regmap, AXP20X_VBUS_IPSOUT_MGMT, &v);
>> +		if (r)
>> +			return r;
>> +
>> +		switch (v & AXP20X_VBUS_CLIMIT_MASK) {
>> +		case AXP20X_VBUC_CLIMIT_100mA:
>> +			val->intval = 100000;
>> +			break;
>> +		case AXP20X_VBUC_CLIMIT_500mA:
>> +			val->intval = 500000;
>> +			break;
>> +		case AXP20X_VBUC_CLIMIT_900mA:
>> +			val->intval = 900000;
>> +			break;
>> +		case AXP20X_VBUC_CLIMIT_NONE:
>> +			val->intval = -1;
>> +			break;
>> +		}
>> +		return 0;
>> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
>> +		r = axp20x_read_variable_width(power->regmap,
>> +					       AXP20X_VBUS_I_ADC_H, 12);
>> +		if (r < 0)
>> +			return r;
>> +
>> +		val->intval = r * 375; /* 1 step = 0.375 mA */
>
> And here too.
>
>> +		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_HEALTH:
>> +		if (!(input & AXP20X_PWR_STATUS_VBUS_PRESENT)) {
>> +			val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
>> +			break;
>> +		}
>> +
>> +		r = regmap_read(power->regmap, AXP20X_USB_OTG_STATUS, &v);
>> +		if (r)
>> +			return r;
>> +
>> +		if (!(v & AXP20X_USB_STATUS_VBUS_VALID)) {
>> +			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
>> +			break;
>> +		}
>> +
>> +		val->intval = POWER_SUPPLY_HEALTH_GOOD;
>> +		break;
>> +	case POWER_SUPPLY_PROP_PRESENT:
>> +		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_PRESENT);
>> +		break;
>> +	case POWER_SUPPLY_PROP_ONLINE:
>> +		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_USED);
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static enum power_supply_property axp20x_usb_power_properties[] = {
>> +	POWER_SUPPLY_PROP_HEALTH,
>> +	POWER_SUPPLY_PROP_PRESENT,
>> +	POWER_SUPPLY_PROP_ONLINE,
>> +	POWER_SUPPLY_PROP_VOLTAGE_MIN,
>> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
>> +	POWER_SUPPLY_PROP_CURRENT_MAX,
>> +	POWER_SUPPLY_PROP_CURRENT_NOW,
>> +};
>> +
>> +static const struct power_supply_desc axp20x_usb_power_desc = {
>> +	.name = "axp20x-usb",
>> +	.type = POWER_SUPPLY_TYPE_USB,
>> +	.properties = axp20x_usb_power_properties,
>> +	.num_properties = ARRAY_SIZE(axp20x_usb_power_properties),
>> +	.get_property = axp20x_usb_power_get_property,
>> +};
>> +
>> +static int axp20x_usb_power_probe(struct platform_device *pdev)
>> +{
>> +	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
>> +	struct power_supply_config psy_cfg = {};
>> +	struct axp20x_usb_power *power;
>> +	static const char * const irq_names[] = { "VBUS_PLUGIN",
>> +		"VBUS_REMOVAL", "VBUS_VALID", "VBUS_NOT_VALID" };
>> +	int i, irq, r;
>> +
>> +	if (!of_device_is_available(pdev->dev.of_node))
>> +		return -ENODEV;
>
> That looks odd. Is it even going to be probed in the first place?

Yes, the mfd framework will instantiate all cells listed for an mfd
device without checking there are compatible child-nodes present
in the dtb.

>> +
>> +	power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
>> +	if (!power)
>> +		return -ENOMEM;
>> +
>> +	power->regmap = axp20x->regmap;
>
> It should probably be wise to test that the axp20x pointer is not
> NULL.

The platform device we are probing has been instantiated by the mfd
framework after setting the parent device driver ptr, so this will never
be not NULL.

>> +
>> +	/* Enable vbus valid checking */
>> +	r = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
>> +		    AXP20X_VBUS_MON_VBUS_VALID, AXP20X_VBUS_MON_VBUS_VALID);
>> +	if (r)
>> +		return r;
>> +
>> +	/* Enable vbus voltage and current measurement */
>> +	r = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
>> +			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_VOLT,
>> +			AXP20X_ADC_EN1_VBUS_CURR | AXP20X_ADC_EN1_VBUS_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_usb_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,
>> +				axp20x_usb_power_irq, 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_usb_power_match[] = {
>> +	{ .compatible = "x-powers,axp202-usb-power-supply" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, axp20x_usb_power_match);
>> +
>> +static struct platform_driver axp20x_usb_power_driver = {
>> +	.probe = axp20x_usb_power_probe,
>> +	.driver = {
>> +		.name = DRVNAME,
>> +		.of_match_table = axp20x_usb_power_match,
>> +	},
>> +};
>> +
>> +module_platform_driver(axp20x_usb_power_driver);
>> +
>> +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
>> +MODULE_DESCRIPTION("AXP20x PMIC USB power supply status driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
>> index 6d8b39a..8ec996e 100644
>> --- a/include/linux/mfd/axp20x.h
>> +++ b/include/linux/mfd/axp20x.h
>> @@ -11,6 +11,8 @@
>>   #ifndef __LINUX_MFD_AXP20X_H
>>   #define __LINUX_MFD_AXP20X_H
>>
>> +#include <linux/regmap.h>
>> +
>>   enum {
>>   	AXP202_ID = 0,
>>   	AXP209_ID,
>> @@ -372,4 +374,26 @@ struct axp288_extcon_pdata {
>>   	struct gpio_desc *gpio_mux_cntl;
>>   };
>>
>> +/* generic helper function for reading 9-16 bit wide regs */
>> +static inline int axp20x_read_variable_width(struct regmap *regmap,
>> +	unsigned int reg, unsigned int width)
>> +{
>> +	unsigned int reg_val, result;
>> +	int err;
>> +
>> +	err = regmap_read(regmap, reg, &reg_val);
>> +	if (err)
>> +		return err;
>> +
>> +	result = reg_val << (width - 8);
>> +
>> +	err = regmap_read(regmap, reg + 1, &reg_val);
>> +	if (err)
>> +		return err;
>> +
>> +	result |= reg_val;
>> +
>> +	return result;
>> +}
>> +
>>   #endif /* __LINUX_MFD_AXP20X_H */
>> --
>> 2.4.3

Regards,

Hans

Re: [PATCH v3 2/4] mfd: axp20x: Add missing registers, and mark more registers volatile

[Lee Jones]

On Fri, 26 Jun 2015, Hans de Goede wrote:

> From: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
> 
> Add an extra set of registers which is necessary tu support the PMICs
> battery charger function, and mark registers which contain status bits,
> gpio status, and adc readings as volatile.
> 
> Cc: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
> Signed-off-by: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> Changes in v2:
> -Add a AXP20X_OCV_MAX define
> Changes in v3:
> -Add Bruno's S-o-b
> ---
>  drivers/mfd/axp20x.c       | 8 +++++++-
>  include/linux/mfd/axp20x.h | 6 ++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)

For my own reference:
  Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Are these two patches tied to any other in the set, or can I apply
them separately?

> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 6df9155..f9a3c2d 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -39,10 +39,16 @@ static const char * const axp20x_model_names[] = {
>  static const struct regmap_range axp20x_writeable_ranges[] = {
>  	regmap_reg_range(AXP20X_DATACACHE(0), AXP20X_IRQ5_STATE),
>  	regmap_reg_range(AXP20X_DCDC_MODE, AXP20X_FG_RES),
> +	regmap_reg_range(AXP20X_RDC_H, AXP20X_OCV(15)),
>  };
>  
>  static const struct regmap_range axp20x_volatile_ranges[] = {
> +	regmap_reg_range(AXP20X_PWR_INPUT_STATUS, AXP20X_USB_OTG_STATUS),
> +	regmap_reg_range(AXP20X_CHRG_CTRL1, AXP20X_CHRG_CTRL2),
>  	regmap_reg_range(AXP20X_IRQ1_EN, AXP20X_IRQ5_STATE),
> +	regmap_reg_range(AXP20X_ACIN_V_ADC_H, AXP20X_IPSOUT_V_HIGH_L),
> +	regmap_reg_range(AXP20X_GPIO20_SS, AXP20X_GPIO3_CTRL),
> +	regmap_reg_range(AXP20X_FG_RES, AXP20X_RDC_L),
>  };
>  
>  static const struct regmap_access_table axp20x_writeable_table = {
> @@ -159,7 +165,7 @@ static const struct regmap_config axp20x_regmap_config = {
>  	.val_bits	= 8,
>  	.wr_table	= &axp20x_writeable_table,
>  	.volatile_table	= &axp20x_volatile_table,
> -	.max_register	= AXP20X_FG_RES,
> +	.max_register	= AXP20X_OCV(AXP20X_OCV_MAX),
>  	.cache_type	= REGCACHE_RBTREE,
>  };
>  
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index 5275423..6d8b39a 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -151,6 +151,12 @@ enum {
>  #define AXP20X_CC_CTRL			0xb8
>  #define AXP20X_FG_RES			0xb9
>  
> +/* OCV */
> +#define AXP20X_RDC_H			0xba
> +#define AXP20X_RDC_L			0xbb
> +#define AXP20X_OCV(m)			(0xc0 + (m))
> +#define AXP20X_OCV_MAX			0xf
> +
>  /* AXP22X specific registers */
>  #define AXP22X_BATLOW_THRES1		0xe6
>  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

-- 
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 v3 2/4] mfd: axp20x: Add missing registers, and mark more registers volatile

[Lee Jones]

On Fri, 26 Jun 2015, Hans de Goede wrote:

> From: Bruno Pr?mont <bonbons@linux-vserver.org>
> 
> Add an extra set of registers which is necessary tu support the PMICs
> battery charger function, and mark registers which contain status bits,
> gpio status, and adc readings as volatile.
> 
> Cc: Bruno Pr?mont <bonbons@linux-vserver.org>
> Signed-off-by: Bruno Pr?mont <bonbons@linux-vserver.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Add a AXP20X_OCV_MAX define
> Changes in v3:
> -Add Bruno's S-o-b
> ---
>  drivers/mfd/axp20x.c       | 8 +++++++-
>  include/linux/mfd/axp20x.h | 6 ++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)

For my own reference:
  Acked-by: Lee Jones <lee.jones@linaro.org>

Are these two patches tied to any other in the set, or can I apply
them separately?

> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 6df9155..f9a3c2d 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -39,10 +39,16 @@ static const char * const axp20x_model_names[] = {
>  static const struct regmap_range axp20x_writeable_ranges[] = {
>  	regmap_reg_range(AXP20X_DATACACHE(0), AXP20X_IRQ5_STATE),
>  	regmap_reg_range(AXP20X_DCDC_MODE, AXP20X_FG_RES),
> +	regmap_reg_range(AXP20X_RDC_H, AXP20X_OCV(15)),
>  };
>  
>  static const struct regmap_range axp20x_volatile_ranges[] = {
> +	regmap_reg_range(AXP20X_PWR_INPUT_STATUS, AXP20X_USB_OTG_STATUS),
> +	regmap_reg_range(AXP20X_CHRG_CTRL1, AXP20X_CHRG_CTRL2),
>  	regmap_reg_range(AXP20X_IRQ1_EN, AXP20X_IRQ5_STATE),
> +	regmap_reg_range(AXP20X_ACIN_V_ADC_H, AXP20X_IPSOUT_V_HIGH_L),
> +	regmap_reg_range(AXP20X_GPIO20_SS, AXP20X_GPIO3_CTRL),
> +	regmap_reg_range(AXP20X_FG_RES, AXP20X_RDC_L),
>  };
>  
>  static const struct regmap_access_table axp20x_writeable_table = {
> @@ -159,7 +165,7 @@ static const struct regmap_config axp20x_regmap_config = {
>  	.val_bits	= 8,
>  	.wr_table	= &axp20x_writeable_table,
>  	.volatile_table	= &axp20x_volatile_table,
> -	.max_register	= AXP20X_FG_RES,
> +	.max_register	= AXP20X_OCV(AXP20X_OCV_MAX),
>  	.cache_type	= REGCACHE_RBTREE,
>  };
>  
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index 5275423..6d8b39a 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -151,6 +151,12 @@ enum {
>  #define AXP20X_CC_CTRL			0xb8
>  #define AXP20X_FG_RES			0xb9
>  
> +/* OCV */
> +#define AXP20X_RDC_H			0xba
> +#define AXP20X_RDC_L			0xbb
> +#define AXP20X_OCV(m)			(0xc0 + (m))
> +#define AXP20X_OCV_MAX			0xf
> +
>  /* AXP22X specific registers */
>  #define AXP22X_BATLOW_THRES1		0xe6
>  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 3/4] mfd: axp20x: Add a cell for the usb power_supply part of the axp20x PMICs

[Lee Jones]

On Fri, 26 Jun 2015, Hans de Goede wrote:

> Add a cell for the usb power_supply part of the axp20x PMICs.
> 
> Note that this cell is only for the usb power_supply part and not the
> ac-power / battery-charger / rtc-backup-bat-charger bits.
> 
> Depending on the board each of those must be enabled / disabled separately
> in devicetree as most boards do not use all 4. So in dt each one needs its
> own child-node of the axp20x node. Another reason for using separate child
> nodes for each is so that other devicetree nodes can have a power-supply
> property with a phandle referencing a node representing a single
> power-supply.
> 
> The decision to use a separate devicetree node for each is reflected on
> the kernel side by each getting its own mfd-cell / platform_device and
> platform-driver.
> 
> Note this commit also makes some whitespace changes to the intialization
> of existing cells in axp20x_cells, these are pure whitespace changes,
> functionally nothing changes.
> 
> Cc: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> Changes in v2:
> -Use DEFINE_RES_IRQ_NAMED
> -Change indentation of axp20x_cells initializers to avoid line wrapping
> Changes in v3:
> -Improve commit message
> -Add Bruno's S-o-b
> ---
>  drivers/mfd/axp20x.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)

For my own reference:
  Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Are these two patches tied to any other in the set, or can I apply
them separately?

> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index f9a3c2d..ca4a604 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -113,6 +113,13 @@ static struct resource axp20x_pek_resources[] = {
>  	},
>  };
>  
> +static struct resource axp20x_usb_power_supply_resources[] = {
> +	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_PLUGIN, "VBUS_PLUGIN"),
> +	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_REMOVAL, "VBUS_REMOVAL"),
> +	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_VALID, "VBUS_VALID"),
> +	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_NOT_VALID, "VBUS_NOT_VALID"),
> +};
> +
>  static struct resource axp22x_pek_resources[] = {
>  	{
>  		.name   = "PEK_DBR",
> @@ -363,11 +370,16 @@ static const struct regmap_irq_chip axp288_regmap_irq_chip = {
>  
>  static struct mfd_cell axp20x_cells[] = {
>  	{
> -		.name			= "axp20x-pek",
> -		.num_resources		= ARRAY_SIZE(axp20x_pek_resources),
> -		.resources		= axp20x_pek_resources,
> +		.name		= "axp20x-pek",
> +		.num_resources	= ARRAY_SIZE(axp20x_pek_resources),
> +		.resources	= axp20x_pek_resources,
>  	}, {
> -		.name			= "axp20x-regulator",
> +		.name		= "axp20x-regulator",
> +	}, {
> +		.name		= "axp20x-usb-power-supply",
> +		.of_compatible	= "x-powers,axp202-usb-power-supply",
> +		.num_resources	= ARRAY_SIZE(axp20x_usb_power_supply_resources),
> +		.resources	= axp20x_usb_power_supply_resources,
>  	},
>  };
>  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

-- 
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 v3 3/4] mfd: axp20x: Add a cell for the usb power_supply part of the axp20x PMICs

[Lee Jones]

On Fri, 26 Jun 2015, Hans de Goede wrote:

> Add a cell for the usb power_supply part of the axp20x PMICs.
> 
> Note that this cell is only for the usb power_supply part and not the
> ac-power / battery-charger / rtc-backup-bat-charger bits.
> 
> Depending on the board each of those must be enabled / disabled separately
> in devicetree as most boards do not use all 4. So in dt each one needs its
> own child-node of the axp20x node. Another reason for using separate child
> nodes for each is so that other devicetree nodes can have a power-supply
> property with a phandle referencing a node representing a single
> power-supply.
> 
> The decision to use a separate devicetree node for each is reflected on
> the kernel side by each getting its own mfd-cell / platform_device and
> platform-driver.
> 
> Note this commit also makes some whitespace changes to the intialization
> of existing cells in axp20x_cells, these are pure whitespace changes,
> functionally nothing changes.
> 
> Cc: Bruno Pr?mont <bonbons@linux-vserver.org>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Bruno Pr?mont <bonbons@linux-vserver.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Use DEFINE_RES_IRQ_NAMED
> -Change indentation of axp20x_cells initializers to avoid line wrapping
> Changes in v3:
> -Improve commit message
> -Add Bruno's S-o-b
> ---
>  drivers/mfd/axp20x.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)

For my own reference:
  Acked-by: Lee Jones <lee.jones@linaro.org>

Are these two patches tied to any other in the set, or can I apply
them separately?

> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index f9a3c2d..ca4a604 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -113,6 +113,13 @@ static struct resource axp20x_pek_resources[] = {
>  	},
>  };
>  
> +static struct resource axp20x_usb_power_supply_resources[] = {
> +	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_PLUGIN, "VBUS_PLUGIN"),
> +	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_REMOVAL, "VBUS_REMOVAL"),
> +	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_VALID, "VBUS_VALID"),
> +	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_NOT_VALID, "VBUS_NOT_VALID"),
> +};
> +
>  static struct resource axp22x_pek_resources[] = {
>  	{
>  		.name   = "PEK_DBR",
> @@ -363,11 +370,16 @@ static const struct regmap_irq_chip axp288_regmap_irq_chip = {
>  
>  static struct mfd_cell axp20x_cells[] = {
>  	{
> -		.name			= "axp20x-pek",
> -		.num_resources		= ARRAY_SIZE(axp20x_pek_resources),
> -		.resources		= axp20x_pek_resources,
> +		.name		= "axp20x-pek",
> +		.num_resources	= ARRAY_SIZE(axp20x_pek_resources),
> +		.resources	= axp20x_pek_resources,
>  	}, {
> -		.name			= "axp20x-regulator",
> +		.name		= "axp20x-regulator",
> +	}, {
> +		.name		= "axp20x-usb-power-supply",
> +		.of_compatible	= "x-powers,axp202-usb-power-supply",
> +		.num_resources	= ARRAY_SIZE(axp20x_usb_power_supply_resources),
> +		.resources	= axp20x_usb_power_supply_resources,
>  	},
>  };
>  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: Re: [PATCH v3 3/4] mfd: axp20x: Add a cell for the usb power_supply part of the axp20x PMICs

[Hans de Goede]

Hi,

On 07-07-15 09:01, Lee Jones wrote:
> On Fri, 26 Jun 2015, Hans de Goede wrote:
>
>> Add a cell for the usb power_supply part of the axp20x PMICs.
>>
>> Note that this cell is only for the usb power_supply part and not the
>> ac-power / battery-charger / rtc-backup-bat-charger bits.
>>
>> Depending on the board each of those must be enabled / disabled separately
>> in devicetree as most boards do not use all 4. So in dt each one needs its
>> own child-node of the axp20x node. Another reason for using separate child
>> nodes for each is so that other devicetree nodes can have a power-supply
>> property with a phandle referencing a node representing a single
>> power-supply.
>>
>> The decision to use a separate devicetree node for each is reflected on
>> the kernel side by each getting its own mfd-cell / platform_device and
>> platform-driver.
>>
>> Note this commit also makes some whitespace changes to the intialization
>> of existing cells in axp20x_cells, these are pure whitespace changes,
>> functionally nothing changes.
>>
>> Cc: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
>> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Signed-off-by: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>> Changes in v2:
>> -Use DEFINE_RES_IRQ_NAMED
>> -Change indentation of axp20x_cells initializers to avoid line wrapping
>> Changes in v3:
>> -Improve commit message
>> -Add Bruno's S-o-b
>> ---
>>   drivers/mfd/axp20x.c | 20 ++++++++++++++++----
>>   1 file changed, 16 insertions(+), 4 deletions(-)
>
> For my own reference:
>    Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> Are these two patches tied to any other in the set, or can I apply
> them separately?

You can apply them seperately, thanks.

I'm wondering who will take care of upstreaming the first patch of this
set: "ARM: dts: Add binding documentation for AXP20x pmic usb power supply"

If you can take that one too that would be great.

Regards,

Hans

-- 
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.

[linux-sunxi] Re: [PATCH v3 3/4] mfd: axp20x: Add a cell for the usb power_supply part of the axp20x PMICs

[Hans de Goede]

Hi,

On 07-07-15 09:01, Lee Jones wrote:
> On Fri, 26 Jun 2015, Hans de Goede wrote:
>
>> Add a cell for the usb power_supply part of the axp20x PMICs.
>>
>> Note that this cell is only for the usb power_supply part and not the
>> ac-power / battery-charger / rtc-backup-bat-charger bits.
>>
>> Depending on the board each of those must be enabled / disabled separately
>> in devicetree as most boards do not use all 4. So in dt each one needs its
>> own child-node of the axp20x node. Another reason for using separate child
>> nodes for each is so that other devicetree nodes can have a power-supply
>> property with a phandle referencing a node representing a single
>> power-supply.
>>
>> The decision to use a separate devicetree node for each is reflected on
>> the kernel side by each getting its own mfd-cell / platform_device and
>> platform-driver.
>>
>> Note this commit also makes some whitespace changes to the intialization
>> of existing cells in axp20x_cells, these are pure whitespace changes,
>> functionally nothing changes.
>>
>> Cc: Bruno Pr?mont <bonbons@linux-vserver.org>
>> Acked-by: Lee Jones <lee.jones@linaro.org>
>> Signed-off-by: Bruno Pr?mont <bonbons@linux-vserver.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -Use DEFINE_RES_IRQ_NAMED
>> -Change indentation of axp20x_cells initializers to avoid line wrapping
>> Changes in v3:
>> -Improve commit message
>> -Add Bruno's S-o-b
>> ---
>>   drivers/mfd/axp20x.c | 20 ++++++++++++++++----
>>   1 file changed, 16 insertions(+), 4 deletions(-)
>
> For my own reference:
>    Acked-by: Lee Jones <lee.jones@linaro.org>
>
> Are these two patches tied to any other in the set, or can I apply
> them separately?

You can apply them seperately, thanks.

I'm wondering who will take care of upstreaming the first patch of this
set: "ARM: dts: Add binding documentation for AXP20x pmic usb power supply"

If you can take that one too that would be great.

Regards,

Hans

Re: Re: [PATCH v3 3/4] mfd: axp20x: Add a cell for the usb power_supply part of the axp20x PMICs

[Lee Jones]

On Tue, 07 Jul 2015, Hans de Goede wrote:
> On 07-07-15 09:01, Lee Jones wrote:
> >On Fri, 26 Jun 2015, Hans de Goede wrote:
> >
> >>Add a cell for the usb power_supply part of the axp20x PMICs.
> >>
> >>Note that this cell is only for the usb power_supply part and not the
> >>ac-power / battery-charger / rtc-backup-bat-charger bits.
> >>
> >>Depending on the board each of those must be enabled / disabled separately
> >>in devicetree as most boards do not use all 4. So in dt each one needs its
> >>own child-node of the axp20x node. Another reason for using separate child
> >>nodes for each is so that other devicetree nodes can have a power-supply
> >>property with a phandle referencing a node representing a single
> >>power-supply.
> >>
> >>The decision to use a separate devicetree node for each is reflected on
> >>the kernel side by each getting its own mfd-cell / platform_device and
> >>platform-driver.
> >>
> >>Note this commit also makes some whitespace changes to the intialization
> >>of existing cells in axp20x_cells, these are pure whitespace changes,
> >>functionally nothing changes.
> >>
> >>Cc: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
> >>Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>Signed-off-by: Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
> >>Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>---
> >>Changes in v2:
> >>-Use DEFINE_RES_IRQ_NAMED
> >>-Change indentation of axp20x_cells initializers to avoid line wrapping
> >>Changes in v3:
> >>-Improve commit message
> >>-Add Bruno's S-o-b
> >>---
> >>  drivers/mfd/axp20x.c | 20 ++++++++++++++++----
> >>  1 file changed, 16 insertions(+), 4 deletions(-)
> >
> >For my own reference:
> >   Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >
> >Are these two patches tied to any other in the set, or can I apply
> >them separately?
> 
> You can apply them seperately, thanks.
> 
> I'm wondering who will take care of upstreaming the first patch of this
> set: "ARM: dts: Add binding documentation for AXP20x pmic usb power supply"
> 
> If you can take that one too that would be great.

I can do, but I need an Ack from these guys:

POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS
M:      Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
M:      Dmitry Eremin-Solenikov <dbaryshkov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
M:      David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
L:      linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
T:      git git://git.infradead.org/battery-2.6.git
S:      Maintained
F:      include/linux/power_supply.h
F:      drivers/power/

... failing that, they should take it.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

-- 
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.

[linux-sunxi] Re: [PATCH v3 3/4] mfd: axp20x: Add a cell for the usb power_supply part of the axp20x PMICs

[Lee Jones]

On Tue, 07 Jul 2015, Hans de Goede wrote:
> On 07-07-15 09:01, Lee Jones wrote:
> >On Fri, 26 Jun 2015, Hans de Goede wrote:
> >
> >>Add a cell for the usb power_supply part of the axp20x PMICs.
> >>
> >>Note that this cell is only for the usb power_supply part and not the
> >>ac-power / battery-charger / rtc-backup-bat-charger bits.
> >>
> >>Depending on the board each of those must be enabled / disabled separately
> >>in devicetree as most boards do not use all 4. So in dt each one needs its
> >>own child-node of the axp20x node. Another reason for using separate child
> >>nodes for each is so that other devicetree nodes can have a power-supply
> >>property with a phandle referencing a node representing a single
> >>power-supply.
> >>
> >>The decision to use a separate devicetree node for each is reflected on
> >>the kernel side by each getting its own mfd-cell / platform_device and
> >>platform-driver.
> >>
> >>Note this commit also makes some whitespace changes to the intialization
> >>of existing cells in axp20x_cells, these are pure whitespace changes,
> >>functionally nothing changes.
> >>
> >>Cc: Bruno Pr?mont <bonbons@linux-vserver.org>
> >>Acked-by: Lee Jones <lee.jones@linaro.org>
> >>Signed-off-by: Bruno Pr?mont <bonbons@linux-vserver.org>
> >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>---
> >>Changes in v2:
> >>-Use DEFINE_RES_IRQ_NAMED
> >>-Change indentation of axp20x_cells initializers to avoid line wrapping
> >>Changes in v3:
> >>-Improve commit message
> >>-Add Bruno's S-o-b
> >>---
> >>  drivers/mfd/axp20x.c | 20 ++++++++++++++++----
> >>  1 file changed, 16 insertions(+), 4 deletions(-)
> >
> >For my own reference:
> >   Acked-by: Lee Jones <lee.jones@linaro.org>
> >
> >Are these two patches tied to any other in the set, or can I apply
> >them separately?
> 
> You can apply them seperately, thanks.
> 
> I'm wondering who will take care of upstreaming the first patch of this
> set: "ARM: dts: Add binding documentation for AXP20x pmic usb power supply"
> 
> If you can take that one too that would be great.

I can do, but I need an Ack from these guys:

POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS
M:      Sebastian Reichel <sre@kernel.org>
M:      Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
M:      David Woodhouse <dwmw2@infradead.org>
L:      linux-pm at vger.kernel.org
T:      git git://git.infradead.org/battery-2.6.git
S:      Maintained
F:      include/linux/power_supply.h
F:      drivers/power/

... failing that, they should take it.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 1/4] ARM: dts: Add binding documentation for AXP20x pmic usb power supply

[Sebastian Reichel]

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

Hi,

On Fri, Jun 26, 2015 at 12:59:14PM +0200, Hans de Goede wrote:
> Add binding documentation for the usb power supply part of the AXP20x pmic.
> [...]
> +Example:
> +
> [...]
> +
> +	usb_power_supply: usb_power_supply {
                      ^^^^^^^^^^^^^^^^
                      use usb-power-supply here.
> +		compatible = "x-powers,axp202-usb-power-supply";
> +	};
> +};

-- Sebastian

[PATCH v3 1/4] ARM: dts: Add binding documentation for AXP20x pmic usb power supply

[Sebastian Reichel]

Hi,

On Fri, Jun 26, 2015 at 12:59:14PM +0200, Hans de Goede wrote:
> Add binding documentation for the usb power supply part of the AXP20x pmic.
> [...]
> +Example:
> +
> [...]
> +
> +	usb_power_supply: usb_power_supply {
                      ^^^^^^^^^^^^^^^^
                      use usb-power-supply here.
> +		compatible = "x-powers,axp202-usb-power-supply";
> +	};
> +};

-- Sebastian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150724/45a5f13f/attachment.sig>

Re: [PATCH v3 4/4] power: Add an axp20x-usb-power driver

[Sebastian Reichel]

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

Hi,

On Fri, Jun 26, 2015 at 12:59:17PM +0200, Hans de Goede wrote:
> This adds a driver for the usb power_supply bits of the axp20x PMICs.
> 
> I initially started writing my own driver, before coming aware of
> Bruno Prémont's excellent earlier RFC with a driver for this.
> 
> My driver was lacking CURRENT_MAX and VOLTAGE_MIN support Bruno's
> drvier has, so I've copied the code for those from his driver.
> 
> Note that the AC-power-supply and battery charger bits will need separate
> drivers. Each one needs its own devictree child-node so that other
> devicetree nodes can reference the right power-supply, and thus each one
> will get its own mfd-cell / platform_device and platform-driver.

Once the other comments have been taken care of, the driver looks ok
to me.

-- Sebastian

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

[PATCH v3 4/4] power: Add an axp20x-usb-power driver

[Sebastian Reichel]

Hi,

On Fri, Jun 26, 2015 at 12:59:17PM +0200, Hans de Goede wrote:
> This adds a driver for the usb power_supply bits of the axp20x PMICs.
> 
> I initially started writing my own driver, before coming aware of
> Bruno Pr?mont's excellent earlier RFC with a driver for this.
> 
> My driver was lacking CURRENT_MAX and VOLTAGE_MIN support Bruno's
> drvier has, so I've copied the code for those from his driver.
> 
> Note that the AC-power-supply and battery charger bits will need separate
> drivers. Each one needs its own devictree child-node so that other
> devicetree nodes can reference the right power-supply, and thus each one
> will get its own mfd-cell / platform_device and platform-driver.

Once the other comments have been taken care of, the driver looks ok
to me.

-- Sebastian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150724/894209e1/attachment.sig>

Re: [PATCH v3 1/4] ARM: dts: Add binding documentation for AXP20x pmic usb power supply

[Hans de Goede]

Hi,

On 24-07-15 16:59, Sebastian Reichel wrote:
> Hi,
>
> On Fri, Jun 26, 2015 at 12:59:14PM +0200, Hans de Goede wrote:
>> Add binding documentation for the usb power supply part of the AXP20x pmic.
>> [...]
>> +Example:
>> +
>> [...]
>> +
>> +	usb_power_supply: usb_power_supply {
>                        ^^^^^^^^^^^^^^^^
>                        use usb-power-supply here.

Ok, fixed for the upcoming v4 of the power-supply part of this patch set.

Regards,

Hans

[PATCH v3 1/4] ARM: dts: Add binding documentation for AXP20x pmic usb power supply

[Hans de Goede]

Hi,

On 24-07-15 16:59, Sebastian Reichel wrote:
> Hi,
>
> On Fri, Jun 26, 2015 at 12:59:14PM +0200, Hans de Goede wrote:
>> Add binding documentation for the usb power supply part of the AXP20x pmic.
>> [...]
>> +Example:
>> +
>> [...]
>> +
>> +	usb_power_supply: usb_power_supply {
>                        ^^^^^^^^^^^^^^^^
>                        use usb-power-supply here.

Ok, fixed for the upcoming v4 of the power-supply part of this patch set.

Regards,

Hans

Re: [PATCH v3 4/4] power: Add an axp20x-usb-power driver

[Hans de Goede]

Hi,

On 24-07-15 17:10, Sebastian Reichel wrote:
> Hi,
>
> On Fri, Jun 26, 2015 at 12:59:17PM +0200, Hans de Goede wrote:
>> This adds a driver for the usb power_supply bits of the axp20x PMICs.
>>
>> I initially started writing my own driver, before coming aware of
>> Bruno Prémont's excellent earlier RFC with a driver for this.
>>
>> My driver was lacking CURRENT_MAX and VOLTAGE_MIN support Bruno's
>> drvier has, so I've copied the code for those from his driver.
>>
>> Note that the AC-power-supply and battery charger bits will need separate
>> drivers. Each one needs its own devictree child-node so that other
>> devicetree nodes can reference the right power-supply, and thus each one
>> will get its own mfd-cell / platform_device and platform-driver.
>
> Once the other comments have been taken care of, the driver looks ok
> to me.

Ok, I will do a v4 of the power-supply part of this patch set, with the
various remarks on this patch fixed.

Thanks & Regards,

Hans

-- 
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 v3 4/4] power: Add an axp20x-usb-power driver

[Hans de Goede]

Hi,

On 24-07-15 17:10, Sebastian Reichel wrote:
> Hi,
>
> On Fri, Jun 26, 2015 at 12:59:17PM +0200, Hans de Goede wrote:
>> This adds a driver for the usb power_supply bits of the axp20x PMICs.
>>
>> I initially started writing my own driver, before coming aware of
>> Bruno Pr?mont's excellent earlier RFC with a driver for this.
>>
>> My driver was lacking CURRENT_MAX and VOLTAGE_MIN support Bruno's
>> drvier has, so I've copied the code for those from his driver.
>>
>> Note that the AC-power-supply and battery charger bits will need separate
>> drivers. Each one needs its own devictree child-node so that other
>> devicetree nodes can reference the right power-supply, and thus each one
>> will get its own mfd-cell / platform_device and platform-driver.
>
> Once the other comments have been taken care of, the driver looks ok
> to me.

Ok, I will do a v4 of the power-supply part of this patch set, with the
various remarks on this patch fixed.

Thanks & Regards,

Hans

Re: [PATCH v3 4/4] power: Add an axp20x-usb-power driver

[Hans de Goede]

Hi Sebastian,

On 24-07-15 17:10, Sebastian Reichel wrote:
> Hi,
>
> On Fri, Jun 26, 2015 at 12:59:17PM +0200, Hans de Goede wrote:
>> This adds a driver for the usb power_supply bits of the axp20x PMICs.
>>
>> I initially started writing my own driver, before coming aware of
>> Bruno Prémont's excellent earlier RFC with a driver for this.
>>
>> My driver was lacking CURRENT_MAX and VOLTAGE_MIN support Bruno's
>> drvier has, so I've copied the code for those from his driver.
>>
>> Note that the AC-power-supply and battery charger bits will need separate
>> drivers. Each one needs its own devictree child-node so that other
>> devicetree nodes can reference the right power-supply, and thus each one
>> will get its own mfd-cell / platform_device and platform-driver.
>
> Once the other comments have been taken care of, the driver looks ok
> to me.

I send out a v4 taking care of the other comments a while back, any
news on this? Anything I need todo from my side to get things moving?

Thanks & Regards,

Hans

[PATCH v3 4/4] power: Add an axp20x-usb-power driver

[Hans de Goede]

Hi Sebastian,

On 24-07-15 17:10, Sebastian Reichel wrote:
> Hi,
>
> On Fri, Jun 26, 2015 at 12:59:17PM +0200, Hans de Goede wrote:
>> This adds a driver for the usb power_supply bits of the axp20x PMICs.
>>
>> I initially started writing my own driver, before coming aware of
>> Bruno Pr?mont's excellent earlier RFC with a driver for this.
>>
>> My driver was lacking CURRENT_MAX and VOLTAGE_MIN support Bruno's
>> drvier has, so I've copied the code for those from his driver.
>>
>> Note that the AC-power-supply and battery charger bits will need separate
>> drivers. Each one needs its own devictree child-node so that other
>> devicetree nodes can reference the right power-supply, and thus each one
>> will get its own mfd-cell / platform_device and platform-driver.
>
> Once the other comments have been taken care of, the driver looks ok
> to me.

I send out a v4 taking care of the other comments a while back, any
news on this? Anything I need todo from my side to get things moving?

Thanks & Regards,

Hans