[PATCH 1/3] power: axp20x_usb: Add support for usb power-supply on axp22x pmics

[PATCH 1/3] power: axp20x_usb: Add support for usb power-supply on axp22x pmics

[Hans de Goede]

The usb power-supply on the axp22x pmics is mostly identical to the
one on the axp20x pmics. One significant difference is that it cannot
measure / monitor the usb voltage / current.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/power/axp20x_usb_power.c | 99 +++++++++++++++++++++++++++++++---------
 1 file changed, 77 insertions(+), 22 deletions(-)

diff --git a/drivers/power/axp20x_usb_power.c b/drivers/power/axp20x_usb_power.c
index 421a90b..b3c1d8e 100644
--- a/drivers/power/axp20x_usb_power.c
+++ b/drivers/power/axp20x_usb_power.c
@@ -42,6 +42,7 @@
 #define AXP20X_VBUS_MON_VBUS_VALID	BIT(3)
 
 struct axp20x_usb_power {
+	struct axp20x_dev *axp20x;
 	struct regmap *regmap;
 	struct power_supply *supply;
 };
@@ -85,7 +86,15 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
 
 		switch (v & AXP20X_VBUS_CLIMIT_MASK) {
 		case AXP20X_VBUC_CLIMIT_100mA:
-			val->intval = 100000;
+			switch (power->axp20x->variant) {
+			case AXP202_ID:
+			case AXP209_ID:
+				val->intval = 100000;
+				break;
+			default:
+				val->intval = -1; /* No 100mA limit */
+				break;
+			}
 			break;
 		case AXP20X_VBUC_CLIMIT_500mA:
 			val->intval = 500000;
@@ -122,16 +131,23 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
 			break;
 		}
 
-		ret = regmap_read(power->regmap, AXP20X_USB_OTG_STATUS, &v);
-		if (ret)
-			return ret;
+		val->intval = POWER_SUPPLY_HEALTH_GOOD;
 
-		if (!(v & AXP20X_USB_STATUS_VBUS_VALID)) {
-			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
+		switch (power->axp20x->variant) {
+		case AXP202_ID:
+		case AXP209_ID:
+			ret = regmap_read(power->regmap,
+					  AXP20X_USB_OTG_STATUS, &v);
+			if (ret)
+				return ret;
+
+			if (!(v & AXP20X_USB_STATUS_VBUS_VALID))
+				val->intval =
+					POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
+			break;
+		default:
 			break;
 		}
-
-		val->intval = POWER_SUPPLY_HEALTH_GOOD;
 		break;
 	case POWER_SUPPLY_PROP_PRESENT:
 		val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_PRESENT);
@@ -156,6 +172,14 @@ static enum power_supply_property axp20x_usb_power_properties[] = {
 	POWER_SUPPLY_PROP_CURRENT_NOW,
 };
 
+static enum power_supply_property axp22x_usb_power_properties[] = {
+	POWER_SUPPLY_PROP_HEALTH,
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_VOLTAGE_MIN,
+	POWER_SUPPLY_PROP_CURRENT_MAX,
+};
+
 static const struct power_supply_desc axp20x_usb_power_desc = {
 	.name = "axp20x-usb",
 	.type = POWER_SUPPLY_TYPE_USB,
@@ -164,13 +188,25 @@ static const struct power_supply_desc axp20x_usb_power_desc = {
 	.get_property = axp20x_usb_power_get_property,
 };
 
+static const struct power_supply_desc axp22x_usb_power_desc = {
+	.name = "axp20x-usb",
+	.type = POWER_SUPPLY_TYPE_USB,
+	.properties = axp22x_usb_power_properties,
+	.num_properties = ARRAY_SIZE(axp22x_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" };
+	static const char * const axp20x_irq_names[] = { "VBUS_PLUGIN",
+		"VBUS_REMOVAL", "VBUS_VALID", "VBUS_NOT_VALID", NULL };
+	static const char * const axp22x_irq_names[] = {
+		"VBUS_PLUGIN", "VBUS_REMOVAL", NULL };
+	static const char * const *irq_names;
+	const struct power_supply_desc *usb_power_desc;
 	int i, irq, ret;
 
 	if (!of_device_is_available(pdev->dev.of_node))
@@ -185,31 +221,50 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 	if (!power)
 		return -ENOMEM;
 
+	power->axp20x = axp20x;
 	power->regmap = axp20x->regmap;
 
-	/* Enable vbus valid checking */
-	ret = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
-		    AXP20X_VBUS_MON_VBUS_VALID, AXP20X_VBUS_MON_VBUS_VALID);
-	if (ret)
-		return ret;
+	switch (power->axp20x->variant) {
+	case AXP202_ID:
+	case AXP209_ID:
+		/* Enable vbus valid checking */
+		ret = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
+					 AXP20X_VBUS_MON_VBUS_VALID,
+					 AXP20X_VBUS_MON_VBUS_VALID);
+		if (ret)
+			return ret;
 
-	/* Enable vbus voltage and current measurement */
-	ret = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
+		/* Enable vbus voltage and current measurement */
+		ret = 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 (ret)
-		return ret;
+		if (ret)
+			return ret;
+
+		usb_power_desc = &axp20x_usb_power_desc;
+		irq_names = axp20x_irq_names;
+		break;
+	case AXP221_ID:
+	case AXP223_ID:
+		usb_power_desc = &axp22x_usb_power_desc;
+		irq_names = axp22x_irq_names;
+		break;
+	default:
+		dev_err(&pdev->dev, "Unsupported AXP variant: %ld\n",
+			axp20x->variant);
+		return -EINVAL;
+	}
 
 	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);
+	power->supply = devm_power_supply_register(&pdev->dev, 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++) {
+	for (i = 0; irq_names[i]; i++) {
 		irq = platform_get_irq_byname(pdev, irq_names[i]);
 		if (irq < 0) {
 			dev_warn(&pdev->dev, "No IRQ for %s: %d\n",
-- 
2.7.4

[PATCH 2/3] mfd: axp20x: Extend axp22x_volatile_ranges

[Hans de Goede]

The axp22x pmic has a bunch of volatile registers besides the interrupt
ones, extend axp22x_volatile_ranges with these.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/mfd/axp20x.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index e4e3297..ca04361 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -93,7 +93,10 @@ static const struct regmap_range axp22x_writeable_ranges[] = {
 };
 
 static const struct regmap_range axp22x_volatile_ranges[] = {
+	regmap_reg_range(AXP20X_PWR_INPUT_STATUS, AXP20X_PWR_OP_MODE),
 	regmap_reg_range(AXP20X_IRQ1_EN, AXP20X_IRQ5_STATE),
+	regmap_reg_range(AXP22X_GPIO_STATE, AXP22X_GPIO_STATE),
+	regmap_reg_range(AXP20X_FG_RES, AXP20X_FG_RES),
 };
 
 static const struct regmap_access_table axp22x_writeable_table = {
-- 
2.7.4

Re: [PATCH 2/3] mfd: axp20x: Extend axp22x_volatile_ranges

[Chen-Yu Tsai]

On Sun, May 15, 2016 at 1:51 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> The axp22x pmic has a bunch of volatile registers besides the interrupt
> ones, extend axp22x_volatile_ranges with these.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Chen-Yu Tsai <wens@csie.org>

Re: [PATCH 2/3] mfd: axp20x: Extend axp22x_volatile_ranges

[Lee Jones]

On Sat, 14 May 2016, Hans de Goede wrote:

> The axp22x pmic has a bunch of volatile registers besides the interrupt
> ones, extend axp22x_volatile_ranges with these.
> 
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/mfd/axp20x.c | 3 +++
>  1 file changed, 3 insertions(+)

Applied, thanks.

> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index e4e3297..ca04361 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -93,7 +93,10 @@ static const struct regmap_range axp22x_writeable_ranges[] = {
>  };
>  
>  static const struct regmap_range axp22x_volatile_ranges[] = {
> +	regmap_reg_range(AXP20X_PWR_INPUT_STATUS, AXP20X_PWR_OP_MODE),
>  	regmap_reg_range(AXP20X_IRQ1_EN, AXP20X_IRQ5_STATE),
> +	regmap_reg_range(AXP22X_GPIO_STATE, AXP22X_GPIO_STATE),
> +	regmap_reg_range(AXP20X_FG_RES, AXP20X_FG_RES),
>  };
>  
>  static const struct regmap_access_table axp22x_writeable_table = {

-- 
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 3/3] mfd: axp20x: Add axp20x-usb-power-supply for axp22x pmics

[Hans de Goede]

Add axp20x-usb-power-supply for axp22x pmics.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/mfd/axp20x.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index ca04361..d62209d 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -160,6 +160,11 @@ static struct resource axp20x_usb_power_supply_resources[] = {
 	DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_NOT_VALID, "VBUS_NOT_VALID"),
 };
 
+static struct resource axp22x_usb_power_supply_resources[] = {
+	DEFINE_RES_IRQ_NAMED(AXP22X_IRQ_VBUS_PLUGIN, "VBUS_PLUGIN"),
+	DEFINE_RES_IRQ_NAMED(AXP22X_IRQ_VBUS_REMOVAL, "VBUS_REMOVAL"),
+};
+
 static struct resource axp22x_pek_resources[] = {
 	{
 		.name   = "PEK_DBR",
@@ -527,6 +532,11 @@ static struct mfd_cell axp22x_cells[] = {
 		.resources		= axp22x_pek_resources,
 	}, {
 		.name			= "axp20x-regulator",
+	}, {
+		.name		= "axp20x-usb-power-supply",
+		.of_compatible	= "x-powers,axp202-usb-power-supply",
+		.num_resources	= ARRAY_SIZE(axp22x_usb_power_supply_resources),
+		.resources	= axp22x_usb_power_supply_resources,
 	},
 };
 
-- 
2.7.4

--
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: [PATCH 3/3] mfd: axp20x: Add axp20x-usb-power-supply for axp22x pmics

[Chen-Yu Tsai]

Hi,

On Sun, May 15, 2016 at 1:51 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Add axp20x-usb-power-supply for axp22x pmics.
>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/mfd/axp20x.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index ca04361..d62209d 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -160,6 +160,11 @@ static struct resource axp20x_usb_power_supply_resources[] = {
>         DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_NOT_VALID, "VBUS_NOT_VALID"),
>  };
>
> +static struct resource axp22x_usb_power_supply_resources[] = {
> +       DEFINE_RES_IRQ_NAMED(AXP22X_IRQ_VBUS_PLUGIN, "VBUS_PLUGIN"),
> +       DEFINE_RES_IRQ_NAMED(AXP22X_IRQ_VBUS_REMOVAL, "VBUS_REMOVAL"),
> +};
> +
>  static struct resource axp22x_pek_resources[] = {
>         {
>                 .name   = "PEK_DBR",
> @@ -527,6 +532,11 @@ static struct mfd_cell axp22x_cells[] = {
>                 .resources              = axp22x_pek_resources,
>         }, {
>                 .name                   = "axp20x-regulator",
> +       }, {
> +               .name           = "axp20x-usb-power-supply",
> +               .of_compatible  = "x-powers,axp202-usb-power-supply",

As mentioned in the power supply patch, could you use a new compatible for the
axp22x variant? Thanks.

ChenYu

> +               .num_resources  = ARRAY_SIZE(axp22x_usb_power_supply_resources),
> +               .resources      = axp22x_usb_power_supply_resources,
>         },
>  };
>
> --
> 2.7.4
>

Re: [PATCH 1/3] power: axp20x_usb: Add support for usb power-supply on axp22x pmics

[Chen-Yu Tsai]

Hi,

On Sun, May 15, 2016 at 1:51 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> The usb power-supply on the axp22x pmics is mostly identical to the
> one on the axp20x pmics. One significant difference is that it cannot
> measure / monitor the usb voltage / current.
>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/power/axp20x_usb_power.c | 99 +++++++++++++++++++++++++++++++---------
>  1 file changed, 77 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/power/axp20x_usb_power.c b/drivers/power/axp20x_usb_power.c
> index 421a90b..b3c1d8e 100644
> --- a/drivers/power/axp20x_usb_power.c
> +++ b/drivers/power/axp20x_usb_power.c
> @@ -42,6 +42,7 @@
>  #define AXP20X_VBUS_MON_VBUS_VALID     BIT(3)
>
>  struct axp20x_usb_power {
> +       struct axp20x_dev *axp20x;
>         struct regmap *regmap;
>         struct power_supply *supply;
>  };
> @@ -85,7 +86,15 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
>
>                 switch (v & AXP20X_VBUS_CLIMIT_MASK) {
>                 case AXP20X_VBUC_CLIMIT_100mA:
> -                       val->intval = 100000;
> +                       switch (power->axp20x->variant) {
> +                       case AXP202_ID:
> +                       case AXP209_ID:
> +                               val->intval = 100000;
> +                               break;
> +                       default:
> +                               val->intval = -1; /* No 100mA limit */
> +                               break;
> +                       }

Given this hardware actually has DT bindings, I'd prefer matching against the
compatible string, and keeping the variant part in axp20x_usb_power. You could
still use the AXPXXX_ID constants, but just the ones used in the bindings. This
could potentially make adding support for newer PMICs easier by only needing a
compatible DT entry.

>                         break;
>                 case AXP20X_VBUC_CLIMIT_500mA:
>                         val->intval = 500000;
> @@ -122,16 +131,23 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
>                         break;
>                 }
>
> -               ret = regmap_read(power->regmap, AXP20X_USB_OTG_STATUS, &v);
> -               if (ret)
> -                       return ret;
> +               val->intval = POWER_SUPPLY_HEALTH_GOOD;
>
> -               if (!(v & AXP20X_USB_STATUS_VBUS_VALID)) {
> -                       val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
> +               switch (power->axp20x->variant) {
> +               case AXP202_ID:
> +               case AXP209_ID:
> +                       ret = regmap_read(power->regmap,
> +                                         AXP20X_USB_OTG_STATUS, &v);
> +                       if (ret)
> +                               return ret;
> +
> +                       if (!(v & AXP20X_USB_STATUS_VBUS_VALID))
> +                               val->intval =
> +                                       POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
> +                       break;
> +               default:

It seems like you'd return only POWER_SUPPLY_HEALTH_GOOD for AXP22x, even if
VBUS is actually disconnected.

>                         break;
>                 }
> -
> -               val->intval = POWER_SUPPLY_HEALTH_GOOD;
>                 break;
>         case POWER_SUPPLY_PROP_PRESENT:
>                 val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_PRESENT);
> @@ -156,6 +172,14 @@ static enum power_supply_property axp20x_usb_power_properties[] = {
>         POWER_SUPPLY_PROP_CURRENT_NOW,
>  };
>
> +static enum power_supply_property axp22x_usb_power_properties[] = {
> +       POWER_SUPPLY_PROP_HEALTH,
> +       POWER_SUPPLY_PROP_PRESENT,
> +       POWER_SUPPLY_PROP_ONLINE,
> +       POWER_SUPPLY_PROP_VOLTAGE_MIN,
> +       POWER_SUPPLY_PROP_CURRENT_MAX,
> +};
> +
>  static const struct power_supply_desc axp20x_usb_power_desc = {
>         .name = "axp20x-usb",
>         .type = POWER_SUPPLY_TYPE_USB,
> @@ -164,13 +188,25 @@ static const struct power_supply_desc axp20x_usb_power_desc = {
>         .get_property = axp20x_usb_power_get_property,
>  };
>
> +static const struct power_supply_desc axp22x_usb_power_desc = {
> +       .name = "axp20x-usb",
> +       .type = POWER_SUPPLY_TYPE_USB,
> +       .properties = axp22x_usb_power_properties,
> +       .num_properties = ARRAY_SIZE(axp22x_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" };
> +       static const char * const axp20x_irq_names[] = { "VBUS_PLUGIN",
> +               "VBUS_REMOVAL", "VBUS_VALID", "VBUS_NOT_VALID", NULL };
> +       static const char * const axp22x_irq_names[] = {
> +               "VBUS_PLUGIN", "VBUS_REMOVAL", NULL };
> +       static const char * const *irq_names;
> +       const struct power_supply_desc *usb_power_desc;
>         int i, irq, ret;
>
>         if (!of_device_is_available(pdev->dev.of_node))
> @@ -185,31 +221,50 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>         if (!power)
>                 return -ENOMEM;
>
> +       power->axp20x = axp20x;
>         power->regmap = axp20x->regmap;
>
> -       /* Enable vbus valid checking */
> -       ret = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
> -                   AXP20X_VBUS_MON_VBUS_VALID, AXP20X_VBUS_MON_VBUS_VALID);
> -       if (ret)
> -               return ret;
> +       switch (power->axp20x->variant) {
> +       case AXP202_ID:
> +       case AXP209_ID:
> +               /* Enable vbus valid checking */
> +               ret = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
> +                                        AXP20X_VBUS_MON_VBUS_VALID,
> +                                        AXP20X_VBUS_MON_VBUS_VALID);
> +               if (ret)
> +                       return ret;
>
> -       /* Enable vbus voltage and current measurement */
> -       ret = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
> +               /* Enable vbus voltage and current measurement */
> +               ret = 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 (ret)
> -               return ret;
> +               if (ret)
> +                       return ret;
> +
> +               usb_power_desc = &axp20x_usb_power_desc;
> +               irq_names = axp20x_irq_names;
> +               break;
> +       case AXP221_ID:
> +       case AXP223_ID:
> +               usb_power_desc = &axp22x_usb_power_desc;
> +               irq_names = axp22x_irq_names;
> +               break;
> +       default:
> +               dev_err(&pdev->dev, "Unsupported AXP variant: %ld\n",
> +                       axp20x->variant);
> +               return -EINVAL;
> +       }

As mentioned above, I'd prefer using the compatible string.

Regards
ChenYu

>
>         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);
> +       power->supply = devm_power_supply_register(&pdev->dev, 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++) {
> +       for (i = 0; irq_names[i]; i++) {
>                 irq = platform_get_irq_byname(pdev, irq_names[i]);
>                 if (irq < 0) {
>                         dev_warn(&pdev->dev, "No IRQ for %s: %d\n",
> --
> 2.7.4
>

Re: Re: [PATCH 1/3] power: axp20x_usb: Add support for usb power-supply on axp22x pmics

[Hans de Goede]

Hi,

On 16-05-16 09:07, Chen-Yu Tsai wrote:
> Hi,
>
> On Sun, May 15, 2016 at 1:51 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> The usb power-supply on the axp22x pmics is mostly identical to the
>> one on the axp20x pmics. One significant difference is that it cannot
>> measure / monitor the usb voltage / current.
>>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/power/axp20x_usb_power.c | 99 +++++++++++++++++++++++++++++++---------
>>  1 file changed, 77 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/power/axp20x_usb_power.c b/drivers/power/axp20x_usb_power.c
>> index 421a90b..b3c1d8e 100644
>> --- a/drivers/power/axp20x_usb_power.c
>> +++ b/drivers/power/axp20x_usb_power.c
>> @@ -42,6 +42,7 @@
>>  #define AXP20X_VBUS_MON_VBUS_VALID     BIT(3)
>>
>>  struct axp20x_usb_power {
>> +       struct axp20x_dev *axp20x;
>>         struct regmap *regmap;
>>         struct power_supply *supply;
>>  };
>> @@ -85,7 +86,15 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
>>
>>                 switch (v & AXP20X_VBUS_CLIMIT_MASK) {
>>                 case AXP20X_VBUC_CLIMIT_100mA:
>> -                       val->intval = 100000;
>> +                       switch (power->axp20x->variant) {
>> +                       case AXP202_ID:
>> +                       case AXP209_ID:
>> +                               val->intval = 100000;
>> +                               break;
>> +                       default:
>> +                               val->intval = -1; /* No 100mA limit */
>> +                               break;
>> +                       }
>
> Given this hardware actually has DT bindings, I'd prefer matching against the
> compatible string, and keeping the variant part in axp20x_usb_power. You could
> still use the AXPXXX_ID constants, but just the ones used in the bindings. This
> could potentially make adding support for newer PMICs easier by only needing a
> compatible DT entry.

Fixed for v2 of this patch-set.

>>                         break;
>>                 case AXP20X_VBUC_CLIMIT_500mA:
>>                         val->intval = 500000;
>> @@ -122,16 +131,23 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
>>                         break;
>>                 }
>>
>> -               ret = regmap_read(power->regmap, AXP20X_USB_OTG_STATUS, &v);
>> -               if (ret)
>> -                       return ret;
>> +               val->intval = POWER_SUPPLY_HEALTH_GOOD;
>>
>> -               if (!(v & AXP20X_USB_STATUS_VBUS_VALID)) {
>> -                       val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
>> +               switch (power->axp20x->variant) {
>> +               case AXP202_ID:
>> +               case AXP209_ID:
>> +                       ret = regmap_read(power->regmap,
>> +                                         AXP20X_USB_OTG_STATUS, &v);
>> +                       if (ret)
>> +                               return ret;
>> +
>> +                       if (!(v & AXP20X_USB_STATUS_VBUS_VALID))
>> +                               val->intval =
>> +                                       POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
>> +                       break;
>> +               default:
>
> It seems like you'd return only POWER_SUPPLY_HEALTH_GOOD for AXP22x, even if
> VBUS is actually disconnected.

No, the following sits just above the code in the patch context above:

         case POWER_SUPPLY_PROP_HEALTH:
                 if (!(input & AXP20X_PWR_STATUS_VBUS_PRESENT)) {
                         val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
                         break;
                 }

Actually the break from above is part of the context, but the relevant
bits are missing.



>
>>                         break;
>>                 }
>> -
>> -               val->intval = POWER_SUPPLY_HEALTH_GOOD;
>>                 break;
>>         case POWER_SUPPLY_PROP_PRESENT:
>>                 val->intval = !!(input & AXP20X_PWR_STATUS_VBUS_PRESENT);
>> @@ -156,6 +172,14 @@ static enum power_supply_property axp20x_usb_power_properties[] = {
>>         POWER_SUPPLY_PROP_CURRENT_NOW,
>>  };
>>
>> +static enum power_supply_property axp22x_usb_power_properties[] = {
>> +       POWER_SUPPLY_PROP_HEALTH,
>> +       POWER_SUPPLY_PROP_PRESENT,
>> +       POWER_SUPPLY_PROP_ONLINE,
>> +       POWER_SUPPLY_PROP_VOLTAGE_MIN,
>> +       POWER_SUPPLY_PROP_CURRENT_MAX,
>> +};
>> +
>>  static const struct power_supply_desc axp20x_usb_power_desc = {
>>         .name = "axp20x-usb",
>>         .type = POWER_SUPPLY_TYPE_USB,
>> @@ -164,13 +188,25 @@ static const struct power_supply_desc axp20x_usb_power_desc = {
>>         .get_property = axp20x_usb_power_get_property,
>>  };
>>
>> +static const struct power_supply_desc axp22x_usb_power_desc = {
>> +       .name = "axp20x-usb",
>> +       .type = POWER_SUPPLY_TYPE_USB,
>> +       .properties = axp22x_usb_power_properties,
>> +       .num_properties = ARRAY_SIZE(axp22x_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" };
>> +       static const char * const axp20x_irq_names[] = { "VBUS_PLUGIN",
>> +               "VBUS_REMOVAL", "VBUS_VALID", "VBUS_NOT_VALID", NULL };
>> +       static const char * const axp22x_irq_names[] = {
>> +               "VBUS_PLUGIN", "VBUS_REMOVAL", NULL };
>> +       static const char * const *irq_names;
>> +       const struct power_supply_desc *usb_power_desc;
>>         int i, irq, ret;
>>
>>         if (!of_device_is_available(pdev->dev.of_node))
>> @@ -185,31 +221,50 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>>         if (!power)
>>                 return -ENOMEM;
>>
>> +       power->axp20x = axp20x;
>>         power->regmap = axp20x->regmap;
>>
>> -       /* Enable vbus valid checking */
>> -       ret = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
>> -                   AXP20X_VBUS_MON_VBUS_VALID, AXP20X_VBUS_MON_VBUS_VALID);
>> -       if (ret)
>> -               return ret;
>> +       switch (power->axp20x->variant) {
>> +       case AXP202_ID:
>> +       case AXP209_ID:
>> +               /* Enable vbus valid checking */
>> +               ret = regmap_update_bits(power->regmap, AXP20X_VBUS_MON,
>> +                                        AXP20X_VBUS_MON_VBUS_VALID,
>> +                                        AXP20X_VBUS_MON_VBUS_VALID);
>> +               if (ret)
>> +                       return ret;
>>
>> -       /* Enable vbus voltage and current measurement */
>> -       ret = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
>> +               /* Enable vbus voltage and current measurement */
>> +               ret = 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 (ret)
>> -               return ret;
>> +               if (ret)
>> +                       return ret;
>> +
>> +               usb_power_desc = &axp20x_usb_power_desc;
>> +               irq_names = axp20x_irq_names;
>> +               break;
>> +       case AXP221_ID:
>> +       case AXP223_ID:
>> +               usb_power_desc = &axp22x_usb_power_desc;
>> +               irq_names = axp22x_irq_names;
>> +               break;
>> +       default:
>> +               dev_err(&pdev->dev, "Unsupported AXP variant: %ld\n",
>> +                       axp20x->variant);
>> +               return -EINVAL;
>> +       }
>
> As mentioned above, I'd prefer using the compatible string.

Fixed for v2.

Regards,

Hans



>
> Regards
> ChenYu
>
>>
>>         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);
>> +       power->supply = devm_power_supply_register(&pdev->dev, 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++) {
>> +       for (i = 0; irq_names[i]; i++) {
>>                 irq = platform_get_irq_byname(pdev, irq_names[i]);
>>                 if (irq < 0) {
>>                         dev_warn(&pdev->dev, "No IRQ for %s: %d\n",
>> --
>> 2.7.4
>>