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

[PATCH v3 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.

I have enabled the AC and USB power supply drivers in axp209.dtsi as
suggested by Maxime Ripard and Chen-Yu Tsai

Changes in v3:

* Move register definitions back from MFD header to driver itself
* Globally enable AC power supply driver in axp209.dtsi
* Additionally enable USB power supply driver in axp209.dtsi
* Fix formatting and ordering issues noted by Chen-Yu Tsai
* Drop Rob Herring's ACKED-BY on the binding documentation patch as I have
  simplified the example

Changes in v2:

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

[PATCH v3 2/5] 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@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..aa6ef08 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -128,6 +128,12 @@ static struct resource axp152_pek_resources[] = {
 	DEFINE_RES_IRQ_NAMED(AXP152_IRQ_PEK_FAL_EDGE, "PEK_DBF"),
 };
 
+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 axp20x_pek_resources[] = {
 	{
 		.name	= "PEK_DBR",
@@ -436,6 +442,11 @@ static struct mfd_cell axp20x_cells[] = {
 	}, {
 		.name		= "axp20x-regulator",
 	}, {
+		.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,
+	}, {
 		.name		= "axp20x-usb-power-supply",
 		.of_compatible	= "x-powers,axp202-usb-power-supply",
 		.num_resources	= ARRAY_SIZE(axp20x_usb_power_supply_resources),
-- 
2.8.2

Re: [PATCH v3 2/5] mfd: axp20x: Add a cell for the ac power_supply part of the axp20x PMICs

[Chen-Yu Tsai]

On Thu, May 5, 2016 at 6:35 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>

Acked-by: Chen-Yu Tsai <wens@csie.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..aa6ef08 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -128,6 +128,12 @@ static struct resource axp152_pek_resources[] = {
>         DEFINE_RES_IRQ_NAMED(AXP152_IRQ_PEK_FAL_EDGE, "PEK_DBF"),
>  };
>
> +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 axp20x_pek_resources[] = {
>         {
>                 .name   = "PEK_DBR",
> @@ -436,6 +442,11 @@ static struct mfd_cell axp20x_cells[] = {
>         }, {
>                 .name           = "axp20x-regulator",
>         }, {
> +               .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,
> +       }, {
>                 .name           = "axp20x-usb-power-supply",
>                 .of_compatible  = "x-powers,axp202-usb-power-supply",
>                 .num_resources  = ARRAY_SIZE(axp20x_usb_power_supply_resources),
> --
> 2.8.2
>

[PATCH v3 3/5] 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>
---
 .../bindings/power_supply/axp20x_ac_power.txt           | 17 +++++++++++++++++
 1 file changed, 17 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..1cebe35
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_supply/axp20x_ac_power.txt
@@ -0,0 +1,17 @@
+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 {
+
+	ac-power-supply: ac-power-supply {
+		compatible = "x-powers,axp202-ac-power-supply";
+	};
+
+	...
+};
-- 
2.8.2

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

[Rob Herring]

On Thu, May 05, 2016 at 12:35:06PM +0200, Michael Haas wrote:
> Add binding documentation for the ac power supply part of the AXP20x
> pmic.
> 
> Signed-off-by: Michael Haas <haas@computerlinguist.org>
> ---
>  .../bindings/power_supply/axp20x_ac_power.txt           | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power_supply/axp20x_ac_power.txt

Acked-by: Rob Herring <robh@kernel.org>

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

[Chen-Yu Tsai]

On Fri, May 6, 2016 at 6:30 AM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Thu, May 05, 2016 at 12:35:06PM +0200, Michael Haas wrote:
>> Add binding documentation for the ac power supply part of the AXP20x
>> pmic.
>>
>> Signed-off-by: Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org>
>> ---
>>  .../bindings/power_supply/axp20x_ac_power.txt           | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power_supply/axp20x_ac_power.txt
>
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

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

[PATCH v3 1/5] 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 | 181 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 182 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..0d1ca0e
--- /dev/null
+++ b/drivers/power/axp20x_ac_power.c
@@ -0,0 +1,181 @@
+/*
+ * AXP20x PMIC AC power driver
+ *
+ * Copyright 2014-2015 Bruno Prémont <bonbons-ud5FBsm0p/xEiooADzr8i9i2O/JbrIOy@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under  the terms of the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/mfd/axp20x.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define DRVNAME "axp20x-ac-power"
+
+
+/* Fields of AXP20X_PWR_INPUT_STATUS */
+#define AXP20X_PWR_STATUS_AC_PRESENT     BIT(7)
+#define AXP20X_PWR_STATUS_AC_AVAILABLE   BIT(6)
+#define AXP20X_PWR_STATUS_AC_VBUS_SHORT  BIT(1)
+#define AXP20X_PWR_STATUS_AC_VBUS_SEL    BIT(0)
+
+/* Fields of AXP20X_ADC_EN1 */
+#define AXP20X_ADC_EN1_ACIN_VOLT BIT(5)
+#define AXP20X_ADC_EN1_ACIN_CURR BIT(4)
+
+
+struct axp20x_ac_power {
+	struct regmap *regmap;
+	struct power_supply *supply;
+};
+
+static int axp20x_ac_power_get_property(struct power_supply *psy,
+					enum power_supply_property psp,
+					union power_supply_propval *val)
+{
+	struct axp20x_ac_power *power = power_supply_get_drvdata(psy);
+	unsigned int input;
+	int r;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		r = axp20x_read_variable_width(power->regmap,
+					       AXP20X_ACIN_V_ADC_H, 12);
+		if (r < 0)
+			return r;
+
+		val->intval = r * 1700; /* 1 step = 1.7 mV */
+		return 0;
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+		r = axp20x_read_variable_width(power->regmap,
+					       AXP20X_ACIN_I_ADC_H, 12);
+		if (r < 0)
+			return r;
+
+		val->intval = r * 375; /* 1 step = 0.375 mA */
+		return 0;
+	default:
+		break;
+	}
+
+	/* All the properties below need the input-status reg value */
+	r = regmap_read(power->regmap, AXP20X_PWR_INPUT_STATUS, &input);
+	if (r)
+		return r;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = !!(input & AXP20X_PWR_STATUS_AC_PRESENT);
+		break;
+	case POWER_SUPPLY_PROP_ONLINE:
+		val->intval = !!(input & AXP20X_PWR_STATUS_AC_AVAILABLE);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static enum power_supply_property axp20x_ac_power_properties[] = {
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+};
+
+static const struct power_supply_desc axp20x_ac_power_desc = {
+	.name = "axp20x-ac",
+	.type = POWER_SUPPLY_TYPE_MAINS,
+	.properties = axp20x_ac_power_properties,
+	.num_properties = ARRAY_SIZE(axp20x_ac_power_properties),
+	.get_property = axp20x_ac_power_get_property,
+};
+
+static irqreturn_t axp20x_irq_ac_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.2

-- 
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/5] 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.

This node is enabled by default. A device is likely to have an AC power
connection. If the AC power is indeed absent, the ac power driver
will simply report the power input as offline.

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..7deb7d9 100644
--- a/arch/arm/boot/dts/axp209.dtsi
+++ b/arch/arm/boot/dts/axp209.dtsi
@@ -90,8 +90,14 @@
 		};
 	};
 
+	ac_power_supply: ac_power_supply {
+		compatible = "x-powers,axp202-ac-power-supply";
+		status = "okay";
+	};
+
 	usb_power_supply: usb_power_supply {
 		compatible = "x-powers,axp202-usb-power-supply";
 		status = "disabled";
 	};
+
 };
-- 
2.8.2

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

[Hans de Goede]

Hi,

On 05-05-16 12:35, Michael Haas wrote:
> Add a node representing the ac power supply part of the axp209 pmic.
>
> This node is enabled by default. A device is likely to have an AC power
> connection. If the AC power is indeed absent, the ac power driver
> will simply report the power input as offline.
>
> 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..7deb7d9 100644
> --- a/arch/arm/boot/dts/axp209.dtsi
> +++ b/arch/arm/boot/dts/axp209.dtsi
> @@ -90,8 +90,14 @@
>  		};
>  	};
>
> +	ac_power_supply: ac_power_supply {
> +		compatible = "x-powers,axp202-ac-power-supply";
> +		status = "okay";
> +	};
> +

Default status needs to be disabled, many tablets have only
a micro/mini usb-b connector which is also used for charging
and do not have a separate ac 5V in connector.

Regards,

Hans


>  	usb_power_supply: usb_power_supply {
>  		compatible = "x-powers,axp202-usb-power-supply";
>  		status = "disabled";
>  	};
> +
>  };
>
--
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 v3 5/5] ARM: dts: axp209: Enable usb_power_supply by default

[Michael Haas]

This node should be enabled by default. A device is likely to have an USB power
connection. If USB power is indeed absent, the USB power driver
will simply report the power input as offline.

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

diff --git a/arch/arm/boot/dts/axp209.dtsi b/arch/arm/boot/dts/axp209.dtsi
index 7deb7d9..4f16be0 100644
--- a/arch/arm/boot/dts/axp209.dtsi
+++ b/arch/arm/boot/dts/axp209.dtsi
@@ -97,7 +97,7 @@
 
 	usb_power_supply: usb_power_supply {
 		compatible = "x-powers,axp202-usb-power-supply";
-		status = "disabled";
+		status = "okay";
 	};
 
 };
-- 
2.8.2

Re: [PATCH v3 5/5] ARM: dts: axp209: Enable usb_power_supply by default

[Hans de Goede]

Hi,

On 05-05-16 12:35, Michael Haas wrote:
> This node should be enabled by default. A device is likely to have an USB power
> connection. If USB power is indeed absent, the USB power driver
> will simply report the power input as offline.

Nack, as Maxime already said we do not want to enable any optional
features by default. Many top set boxes do not use the usb power supply,
and we don't want some userspace power control panel applet showing
the supply as offline, we want the supply to simply not be there.

Regards,

Hans



>
> Signed-off-by: Michael Haas <haas@computerlinguist.org>
> ---
>  arch/arm/boot/dts/axp209.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/axp209.dtsi b/arch/arm/boot/dts/axp209.dtsi
> index 7deb7d9..4f16be0 100644
> --- a/arch/arm/boot/dts/axp209.dtsi
> +++ b/arch/arm/boot/dts/axp209.dtsi
> @@ -97,7 +97,7 @@
>
>  	usb_power_supply: usb_power_supply {
>  		compatible = "x-powers,axp202-usb-power-supply";
> -		status = "disabled";
> +		status = "okay";
>  	};
>
>  };
>

Re: [PATCH v3 5/5] ARM: dts: axp209: Enable usb_power_supply by default

[Michael Haas]

Hi Hans,

On 05/05/2016 12:39 PM, Hans de Goede wrote:
> Hi,
> 
> On 05-05-16 12:35, Michael Haas wrote:
>> This node should be enabled by default. A device is likely to have an
>> USB power
>> connection. If USB power is indeed absent, the USB power driver
>> will simply report the power input as offline.
> 
> Nack, as Maxime already said we do not want to enable any optional
> features by default. Many top set boxes do not use the usb power supply,
> and we don't want some userspace power control panel applet showing
> the supply as offline, we want the supply to simply not be there.

it was my understanding that Maxime [0] and ChenYu [1] indicated they
would prefer it to be enabled globally.

Best,

Michael

[0] https://groups.google.com/d/msg/linux-sunxi/cHAlhoIw74g/CbBeoX23AAAJ
[1] https://groups.google.com/d/msg/linux-sunxi/Ee7i8DVI4F8/0b0TBrRkAAAJ

>>
>> Signed-off-by: Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/axp209.dtsi | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/axp209.dtsi
>> b/arch/arm/boot/dts/axp209.dtsi
>> index 7deb7d9..4f16be0 100644
>> --- a/arch/arm/boot/dts/axp209.dtsi
>> +++ b/arch/arm/boot/dts/axp209.dtsi
>> @@ -97,7 +97,7 @@
>>
>>      usb_power_supply: usb_power_supply {
>>          compatible = "x-powers,axp202-usb-power-supply";
>> -        status = "disabled";
>> +        status = "okay";
>>      };
>>
>>  };
>>

Re: [linux-sunxi] Re: [PATCH v3 5/5] ARM: dts: axp209: Enable usb_power_supply by default

[Hans de Goede]

Hi,

On 05-05-16 12:46, Michael Haas wrote:
> Hi Hans,
>
> On 05/05/2016 12:39 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 05-05-16 12:35, Michael Haas wrote:
>>> This node should be enabled by default. A device is likely to have an
>>> USB power
>>> connection. If USB power is indeed absent, the USB power driver
>>> will simply report the power input as offline.
>>
>> Nack, as Maxime already said we do not want to enable any optional
>> features by default. Many top set boxes do not use the usb power supply,
>> and we don't want some userspace power control panel applet showing
>> the supply as offline, we want the supply to simply not be there.
>
> it was my understanding that Maxime [0] and ChenYu [1] indicated they
> would prefer it to be enabled globally.

I believe you're misreading what Maxime is saying, let me
pair things up as I believe they are meant to be interpreted:

You say: "do you have any preference for this being in the AXP209 dtsi? ..."

Maxime says: "Yes, I'd prefer that a lot :)"


You say: "I've been thinking about it and it makes sense to enable the power
supply nodes for all devices using the AXP209."

Maxime says: "It avoids enabling it on all the boards."

IOW having the "status = disabled" in the dtsi avoids enabling it on all
the boards".

I'm pretty sure this is what Maxime's intentions are.


It seems that Chen-Yu does agree with you:

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

Chen-Yu, I disagree with this, IIRC we've had this discussion multiple
times, and so far the rule has always been that we do not want negatives
in the per board dts files. IOW we do not want people to have to add
status = "disabled" to dts files when the usb or ac power supply is not
used, since they are likely to forgot and it just makes the dts files
harder to read in general.

And boards without usb-power (many A10 / A20 / A31 set-top boxes), or
without ac-power (quite a few tablets) are more common then you think.

Regards,

Hans





>
> Best,
>
> Michael
>
> [0] https://groups.google.com/d/msg/linux-sunxi/cHAlhoIw74g/CbBeoX23AAAJ
> [1] https://groups.google.com/d/msg/linux-sunxi/Ee7i8DVI4F8/0b0TBrRkAAAJ
>
>>>
>>> Signed-off-by: Michael Haas <haas-bdq14YP6qtSV9CzYT+GlPGD2FQJk+8+b@public.gmane.org>
>>> ---
>>>  arch/arm/boot/dts/axp209.dtsi | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/boot/dts/axp209.dtsi
>>> b/arch/arm/boot/dts/axp209.dtsi
>>> index 7deb7d9..4f16be0 100644
>>> --- a/arch/arm/boot/dts/axp209.dtsi
>>> +++ b/arch/arm/boot/dts/axp209.dtsi
>>> @@ -97,7 +97,7 @@
>>>
>>>      usb_power_supply: usb_power_supply {
>>>          compatible = "x-powers,axp202-usb-power-supply";
>>> -        status = "disabled";
>>> +        status = "okay";
>>>      };
>>>
>>>  };
>>>
>
--
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

Re: [linux-sunxi] Re: [PATCH v3 5/5] ARM: dts: axp209: Enable usb_power_supply by default

[Chen-Yu Tsai]

On Fri, May 6, 2016 at 1:24 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 05-05-16 12:46, Michael Haas wrote:
>>
>> Hi Hans,
>>
>> On 05/05/2016 12:39 PM, Hans de Goede wrote:
>>>
>>> Hi,
>>>
>>> On 05-05-16 12:35, Michael Haas wrote:
>>>>
>>>> This node should be enabled by default. A device is likely to have an
>>>> USB power
>>>> connection. If USB power is indeed absent, the USB power driver
>>>> will simply report the power input as offline.
>>>
>>>
>>> Nack, as Maxime already said we do not want to enable any optional
>>> features by default. Many top set boxes do not use the usb power supply,
>>> and we don't want some userspace power control panel applet showing
>>> the supply as offline, we want the supply to simply not be there.
>>
>>
>> it was my understanding that Maxime [0] and ChenYu [1] indicated they
>> would prefer it to be enabled globally.
>
>
> I believe you're misreading what Maxime is saying, let me
> pair things up as I believe they are meant to be interpreted:
>
> You say: "do you have any preference for this being in the AXP209 dtsi? ..."
>
> Maxime says: "Yes, I'd prefer that a lot :)"
>
>
> You say: "I've been thinking about it and it makes sense to enable the power
> supply nodes for all devices using the AXP209."
>
> Maxime says: "It avoids enabling it on all the boards."
>
> IOW having the "status = disabled" in the dtsi avoids enabling it on all
> the boards".
>
> I'm pretty sure this is what Maxime's intentions are.
>
>
> It seems that Chen-Yu does agree with you:
>
> "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."
>
> Chen-Yu, I disagree with this, IIRC we've had this discussion multiple
> times, and so far the rule has always been that we do not want negatives
> in the per board dts files. IOW we do not want people to have to add
> status = "disabled" to dts files when the usb or ac power supply is not
> used, since they are likely to forgot and it just makes the dts files
> harder to read in general.
>
> And boards without usb-power (many A10 / A20 / A31 set-top boxes), or
> without ac-power (quite a few tablets) are more common then you think.

I see. Both are very valid points. I'll keep that in mind.

Michael, please drop this one and also set status = "disabled" for
the ac power supply in axp209.dtsi.

Sorry for the noise.

ChenYu

>
> Regards,
>
> Hans
>
>
>
>
>
>
>>
>> Best,
>>
>> Michael
>>
>> [0] https://groups.google.com/d/msg/linux-sunxi/cHAlhoIw74g/CbBeoX23AAAJ
>> [1] https://groups.google.com/d/msg/linux-sunxi/Ee7i8DVI4F8/0b0TBrRkAAAJ
>>
>>>>
>>>> Signed-off-by: Michael Haas <haas@computerlinguist.org>
>>>> ---
>>>>  arch/arm/boot/dts/axp209.dtsi | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm/boot/dts/axp209.dtsi
>>>> b/arch/arm/boot/dts/axp209.dtsi
>>>> index 7deb7d9..4f16be0 100644
>>>> --- a/arch/arm/boot/dts/axp209.dtsi
>>>> +++ b/arch/arm/boot/dts/axp209.dtsi
>>>> @@ -97,7 +97,7 @@
>>>>
>>>>      usb_power_supply: usb_power_supply {
>>>>          compatible = "x-powers,axp202-usb-power-supply";
>>>> -        status = "disabled";
>>>> +        status = "okay";
>>>>      };
>>>>
>>>>  };
>>>>
>>
>