[PATCH v2 0/6] Add axp20x-ac power driver

[PATCH v2 0/6] Add axp20x-ac power driver

[Michael Haas]

This version of the axp20x-ac driver hopefully cleans up all issues
found in version 1.

Major changes:

* Remove check for shortcut between AC and USB
* Remove logging in interrupt handler

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

[Michael Haas]

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

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

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

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

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

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

[PATCH v2 2/6] mfd: axp20x: Add register bits for axp20x-ac-power

[Michael Haas]

This change adds some register bit definitions used by the
axp20x-ac-power driver.

Signed-off-by: Michael Haas <haas@computerlinguist.org>
---
 include/linux/mfd/axp20x.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index d82e7d5..c4c6dfa 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -90,6 +90,17 @@ enum {
 #define AXP22X_ALDO3_V_OUT		0x2a
 #define AXP22X_CHRG_CTRL3		0x35
 
+
+/* Fields of AXP20X_PWR_INPUT_STATUS */
+#define AXP20X_PWR_STATUS_AC_PRESENT     BIT(7)
+#define AXP20X_PWR_STATUS_AC_AVAILABLE   BIT(6)
+#define AXP20X_PWR_STATUS_AC_VBUS_SHORT  BIT(1)
+#define AXP20X_PWR_STATUS_AC_VBUS_SEL    BIT(0)
+
+/* Fields of AXP20X_ADC_EN1 */
+#define AXP20X_ADC_EN1_ACIN_VOLT BIT(5)
+#define AXP20X_ADC_EN1_ACIN_CURR BIT(4)
+
 /* Interrupt */
 #define AXP152_IRQ1_EN			0x40
 #define AXP152_IRQ2_EN			0x41
-- 
2.8.0

[PATCH v2 3/6] mfd: axp20x: Add a cell for the ac power_supply part of the axp20x PMICs

[Michael Haas]

As a counterpart to the usb power_supply cell, this commit adds an AC
power_supply cell to the axp20x driver.

Still missing are the RTC backup battery and the main battery charger
cells.

Signed-off-by: Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org>
---
 drivers/mfd/axp20x.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index a57d6e9..9351c0e 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -178,6 +178,12 @@ static struct resource axp288_power_button_resources[] = {
 	},
 };
 
+static struct resource axp20x_ac_power_supply_resources[] = {
+	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_PLUGIN, "ACIN_PLUGIN"),
+	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_REMOVAL, "ACIN_REMOVAL"),
+	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_OVER_V, "ACIN_OVER_V"),
+};
+
 static struct resource axp288_fuel_gauge_resources[] = {
 	{
 		.start = AXP288_IRQ_QWBTU,
@@ -440,6 +446,11 @@ static struct mfd_cell axp20x_cells[] = {
 		.of_compatible	= "x-powers,axp202-usb-power-supply",
 		.num_resources	= ARRAY_SIZE(axp20x_usb_power_supply_resources),
 		.resources	= axp20x_usb_power_supply_resources,
+	}, {
+		.name		= "axp20x-ac-power-supply",
+		.of_compatible	= "x-powers,axp202-ac-power-supply",
+		.num_resources	= ARRAY_SIZE(axp20x_ac_power_supply_resources),
+		.resources	= axp20x_ac_power_supply_resources,
 	},
 };
 
-- 
2.8.0

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

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

[Michael Haas]

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

Signed-off-by: Michael Haas <haas@computerlinguist.org>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../bindings/power_supply/axp20x_ac_power.txt      | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power_supply/axp20x_ac_power.txt

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

[PATCH v2 5/6] ARM: dts: axp209: Add ac_power_supply child node to the ax209 node

[Michael Haas]

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

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

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

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

[Michael Haas]

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

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

diff --git a/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts b/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
index d5c796c..bcd4a89 100644
--- a/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
+++ b/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
@@ -163,6 +163,10 @@
 				regulator-always-on;
 			};
 		};
+		ac_power_supply: ac_power_supply {
+			status = "okay";
+			compatible = "x-powers,axp202-ac-power-supply";
+		};
 	};
 };
 
-- 
2.8.0

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

[Chen-Yu Tsai]

Hi,

On Sun, May 1, 2016 at 4:57 PM, Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org> wrote:
> Add binding documentation for the ac power supply part of the AXP20x
> pmic.
>
> Signed-off-by: Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org>
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  .../bindings/power_supply/axp20x_ac_power.txt      | 34 ++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power_supply/axp20x_ac_power.txt
>
> diff --git a/Documentation/devicetree/bindings/power_supply/axp20x_ac_power.txt b/Documentation/devicetree/bindings/power_supply/axp20x_ac_power.txt
> new file mode 100644
> index 0000000..1cbdcfb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power_supply/axp20x_ac_power.txt
> @@ -0,0 +1,34 @@
> +AXP20x AC power supply
> +
> +Required Properties:
> +-compatible: "x-powers,axp202-ac-power-supply"
> +
> +This node is a subnode of the axp20x PMIC.
> +
> +Example:
> +
> +axp209: pmic@34 {
> +       compatible = "x-powers,axp209";
> +       reg = <0x34>;
> +       interrupt-parent = <&nmi_intc>;
> +       interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> +       interrupt-controller;
> +       #interrupt-cells = <1>;
> +
> +       regulators {
> +               x-powers,dcdc-freq = <1500>;
> +
> +               vdd_cpu: dcdc2 {
> +                       regulator-always-on;
> +                       regulator-min-microvolt = <1000000>;
> +                       regulator-max-microvolt = <1450000>;
> +                       regulator-name = "vdd-cpu";
> +               };
> +
> +               ...
> +       };

You don't need to include this in the example. Just a "..." would suffice.

> +
> +       ac-power-supply: ac-power-supply {
> +               compatible = "x-powers,axp202-ac-power-supply";
> +       };

Also this should come first in alphabetic order.

Otherwise,

Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>

> +};
> --
> 2.8.0
>

Re: [PATCH v2 2/6] mfd: axp20x: Add register bits for axp20x-ac-power

[Chen-Yu Tsai]

Hi,

On Sun, May 1, 2016 at 4:57 PM, Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org> wrote:
> This change adds some register bit definitions used by the
> axp20x-ac-power driver.
>
> Signed-off-by: Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org>
> ---
>  include/linux/mfd/axp20x.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index d82e7d5..c4c6dfa 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -90,6 +90,17 @@ enum {
>  #define AXP22X_ALDO3_V_OUT             0x2a
>  #define AXP22X_CHRG_CTRL3              0x35
>
> +
> +/* Fields of AXP20X_PWR_INPUT_STATUS */
> +#define AXP20X_PWR_STATUS_AC_PRESENT     BIT(7)
> +#define AXP20X_PWR_STATUS_AC_AVAILABLE   BIT(6)
> +#define AXP20X_PWR_STATUS_AC_VBUS_SHORT  BIT(1)
> +#define AXP20X_PWR_STATUS_AC_VBUS_SEL    BIT(0)
> +
> +/* Fields of AXP20X_ADC_EN1 */
> +#define AXP20X_ADC_EN1_ACIN_VOLT BIT(5)
> +#define AXP20X_ADC_EN1_ACIN_CURR BIT(4)
> +

We keep the bit definitions of each register in each separate driver.
The drivers only define the ones they use.

ChenYu

>  /* Interrupt */
>  #define AXP152_IRQ1_EN                 0x40
>  #define AXP152_IRQ2_EN                 0x41
> --
> 2.8.0
>

Re: [PATCH v2 5/6] ARM: dts: axp209: Add ac_power_supply child node to the ax209 node

[Chen-Yu Tsai]

Hi,

On Sun, May 1, 2016 at 4:57 PM, Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org> wrote:
> Add a node representing the ac power supply part of the axp209 pmic.
>
> Signed-off-by: Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org>
> ---
>  arch/arm/boot/dts/axp209.dtsi | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm/boot/dts/axp209.dtsi b/arch/arm/boot/dts/axp209.dtsi
> index 051ab3b..9046f0a 100644
> --- a/arch/arm/boot/dts/axp209.dtsi
> +++ b/arch/arm/boot/dts/axp209.dtsi
> @@ -94,4 +94,10 @@
>                 compatible = "x-powers,axp202-usb-power-supply";
>                 status = "disabled";
>         };
> +
> +       ac_power_supply: ac_power_supply {

Please keep them alphabetically sorted.

> +               compatible = "x-powers,axp202-ac-power-supply";
> +               status = "disabled";

I see no reason why we shouldn't just enable it by default.
It is almost always going to be used. Same for the VBUS power supply.

> +       };
> +

Extra line here.


ChenYu

>  };
> --
> 2.8.0
>

Re: [PATCH v2 3/6] mfd: axp20x: Add a cell for the ac power_supply part of the axp20x PMICs

[Chen-Yu Tsai]

Hi,

On Sun, May 1, 2016 at 4:57 PM, Michael Haas <haas@computerlinguist.org> wrote:
> As a counterpart to the usb power_supply cell, this commit adds an AC
> power_supply cell to the axp20x driver.
>
> Still missing are the RTC backup battery and the main battery charger
> cells.
>
> Signed-off-by: Michael Haas <haas@computerlinguist.org>
> ---
>  drivers/mfd/axp20x.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index a57d6e9..9351c0e 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -178,6 +178,12 @@ static struct resource axp288_power_button_resources[] = {
>         },
>  };
>
> +static struct resource axp20x_ac_power_supply_resources[] = {
> +       DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_PLUGIN, "ACIN_PLUGIN"),
> +       DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_REMOVAL, "ACIN_REMOVAL"),
> +       DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_OVER_V, "ACIN_OVER_V"),
> +};
> +

Please group the resources by family, i.e. put this one above
axp20x_pek_resources.

>  static struct resource axp288_fuel_gauge_resources[] = {
>         {
>                 .start = AXP288_IRQ_QWBTU,
> @@ -440,6 +446,11 @@ static struct mfd_cell axp20x_cells[] = {
>                 .of_compatible  = "x-powers,axp202-usb-power-supply",
>                 .num_resources  = ARRAY_SIZE(axp20x_usb_power_supply_resources),
>                 .resources      = axp20x_usb_power_supply_resources,
> +       }, {
> +               .name           = "axp20x-ac-power-supply",
> +               .of_compatible  = "x-powers,axp202-ac-power-supply",
> +               .num_resources  = ARRAY_SIZE(axp20x_ac_power_supply_resources),
> +               .resources      = axp20x_ac_power_supply_resources,

I would put this before "x-powers,axp202-usb-power-supply", as it's the primary
power supply, but keeping the power supplies grouped. The list is already out of
alphabetic order anyway.

ChenYu

>         },
>  };
>
> --
> 2.8.0
>

Re: Re: [PATCH v2 2/6] mfd: axp20x: Add register bits for axp20x-ac-power

[Michael Haas]

On 05/01/2016 11:48 AM, Chen-Yu Tsai wrote:
> Hi,
> 
> On Sun, May 1, 2016 at 4:57 PM, Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org> wrote:
>> This change adds some register bit definitions used by the
>> axp20x-ac-power driver.
>>
>> Signed-off-by: Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org>
>> ---
>>  include/linux/mfd/axp20x.h | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
>> index d82e7d5..c4c6dfa 100644
>> --- a/include/linux/mfd/axp20x.h
>> +++ b/include/linux/mfd/axp20x.h
>> @@ -90,6 +90,17 @@ enum {
>>  #define AXP22X_ALDO3_V_OUT             0x2a
>>  #define AXP22X_CHRG_CTRL3              0x35
>>
>> +
>> +/* Fields of AXP20X_PWR_INPUT_STATUS */
>> +#define AXP20X_PWR_STATUS_AC_PRESENT     BIT(7)
>> +#define AXP20X_PWR_STATUS_AC_AVAILABLE   BIT(6)
>> +#define AXP20X_PWR_STATUS_AC_VBUS_SHORT  BIT(1)
>> +#define AXP20X_PWR_STATUS_AC_VBUS_SEL    BIT(0)
>> +
>> +/* Fields of AXP20X_ADC_EN1 */
>> +#define AXP20X_ADC_EN1_ACIN_VOLT BIT(5)
>> +#define AXP20X_ADC_EN1_ACIN_CURR BIT(4)
>> +
> 
> We keep the bit definitions of each register in each separate driver.
> The drivers only define the ones they use.
> 
> ChenYu

Hi ChenYu,

i believe Maxime Ripard requested that these defines be moved to the
header: https://groups.google.com/d/msg/linux-sunxi/nEUg87cV6KI/TvdB6MBZBAAJ

What do you think?

Thanks for the review!

Michael

Re: Re: [PATCH v2 2/6] mfd: axp20x: Add register bits for axp20x-ac-power

[Chen-Yu Tsai]

On Sun, May 1, 2016 at 10:24 PM, Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org> wrote:
> On 05/01/2016 11:48 AM, Chen-Yu Tsai wrote:
>> Hi,
>>
>> On Sun, May 1, 2016 at 4:57 PM, Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org> wrote:
>>> This change adds some register bit definitions used by the
>>> axp20x-ac-power driver.
>>>
>>> Signed-off-by: Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org>
>>> ---
>>>  include/linux/mfd/axp20x.h | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
>>> index d82e7d5..c4c6dfa 100644
>>> --- a/include/linux/mfd/axp20x.h
>>> +++ b/include/linux/mfd/axp20x.h
>>> @@ -90,6 +90,17 @@ enum {
>>>  #define AXP22X_ALDO3_V_OUT             0x2a
>>>  #define AXP22X_CHRG_CTRL3              0x35
>>>
>>> +
>>> +/* Fields of AXP20X_PWR_INPUT_STATUS */
>>> +#define AXP20X_PWR_STATUS_AC_PRESENT     BIT(7)
>>> +#define AXP20X_PWR_STATUS_AC_AVAILABLE   BIT(6)
>>> +#define AXP20X_PWR_STATUS_AC_VBUS_SHORT  BIT(1)
>>> +#define AXP20X_PWR_STATUS_AC_VBUS_SEL    BIT(0)
>>> +
>>> +/* Fields of AXP20X_ADC_EN1 */
>>> +#define AXP20X_ADC_EN1_ACIN_VOLT BIT(5)
>>> +#define AXP20X_ADC_EN1_ACIN_CURR BIT(4)
>>> +
>>
>> We keep the bit definitions of each register in each separate driver.
>> The drivers only define the ones they use.
>>
>> ChenYu
>
> Hi ChenYu,
>
> i believe Maxime Ripard requested that these defines be moved to the
> header: https://groups.google.com/d/msg/linux-sunxi/nEUg87cV6KI/TvdB6MBZBAAJ
>
> What do you think?

My argument is kind of weak, and really comes down to preference.

Currently the register bit definitions are scattered in various drivers,
which is fine given they are really specific to the part of hardware the
driver supports. Gathering them all together might increase the size of
the header file substantially. As I see it the chanses that bits from
one part are going to be used in another are rather small.

Some register address macros are shared, such as for the 2 power supply
drivers, and for the regmap definitions. So those would need to go in a
shared header anyway.


Regards
ChenYu

Re: Re: [PATCH v2 2/6] mfd: axp20x: Add register bits for axp20x-ac-power

[Maxime Ripard]

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

Hi,

On Mon, May 02, 2016 at 09:35:01AM +0800, Chen-Yu Tsai wrote:
> On Sun, May 1, 2016 at 10:24 PM, Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org> wrote:
> > On 05/01/2016 11:48 AM, Chen-Yu Tsai wrote:
> >> Hi,
> >>
> >> On Sun, May 1, 2016 at 4:57 PM, Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org> wrote:
> >>> This change adds some register bit definitions used by the
> >>> axp20x-ac-power driver.
> >>>
> >>> Signed-off-by: Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org>
> >>> ---
> >>>  include/linux/mfd/axp20x.h | 11 +++++++++++
> >>>  1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> >>> index d82e7d5..c4c6dfa 100644
> >>> --- a/include/linux/mfd/axp20x.h
> >>> +++ b/include/linux/mfd/axp20x.h
> >>> @@ -90,6 +90,17 @@ enum {
> >>>  #define AXP22X_ALDO3_V_OUT             0x2a
> >>>  #define AXP22X_CHRG_CTRL3              0x35
> >>>
> >>> +
> >>> +/* Fields of AXP20X_PWR_INPUT_STATUS */
> >>> +#define AXP20X_PWR_STATUS_AC_PRESENT     BIT(7)
> >>> +#define AXP20X_PWR_STATUS_AC_AVAILABLE   BIT(6)
> >>> +#define AXP20X_PWR_STATUS_AC_VBUS_SHORT  BIT(1)
> >>> +#define AXP20X_PWR_STATUS_AC_VBUS_SEL    BIT(0)
> >>> +
> >>> +/* Fields of AXP20X_ADC_EN1 */
> >>> +#define AXP20X_ADC_EN1_ACIN_VOLT BIT(5)
> >>> +#define AXP20X_ADC_EN1_ACIN_CURR BIT(4)
> >>> +
> >>
> >> We keep the bit definitions of each register in each separate driver.
> >> The drivers only define the ones they use.
> >>
> >> ChenYu
> >
> > Hi ChenYu,
> >
> > i believe Maxime Ripard requested that these defines be moved to the
> > header: https://groups.google.com/d/msg/linux-sunxi/nEUg87cV6KI/TvdB6MBZBAAJ
> >
> > What do you think?
> 
> My argument is kind of weak, and really comes down to preference.
> 
> Currently the register bit definitions are scattered in various drivers,
> which is fine given they are really specific to the part of hardware the
> driver supports. Gathering them all together might increase the size of
> the header file substantially. As I see it the chanses that bits from
> one part are going to be used in another are rather small.
> 
> Some register address macros are shared, such as for the 2 power supply
> drivers, and for the regmap definitions. So those would need to go in a
> shared header anyway.

Sorry for the misunderstanding. I was assuming that having all the
registers and associated bits would be better off in a common header
where all the drivers could refer to, but you're the maintainer on
that on, so it's up to you ;)

Maxime

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