Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1479917 > unrolled thread

[PATCH] power: gpio_charger: switch to using GPIO descriptors

Back to article view | Back to linux.kernel

Contents

  [PATCH] power: gpio_charger: switch to using GPIO descriptors Linus Walleij <linus.walleij@linaro.org> - 2016-09-09 13:50 +0200
    Re: [PATCH] power: gpio_charger: switch to using GPIO descriptors Sebastian Reichel <sre@kernel.org> - 2016-09-19 21:20 +0200
      Re: [PATCH] power: gpio_charger: switch to using GPIO descriptors Linus Walleij <linus.walleij@linaro.org> - 2016-09-19 21:40 +0200

#1479917 — [PATCH] power: gpio_charger: switch to using GPIO descriptors

The GPIO charger is using a mix of the legacy GPIO interface
and <linux/of_gpio.h> which is not the modern way to use GPIOs.

Refactor like this:

- Use a GPIO descriptor for the GPIO line used to monitor the
  charger.
- Fetch the descriptor with devm_gpiod_get() as the first
  method.
- If this fails and we are *not* using device tree, then
  start looking to see if we can use platform data instead.
- After looking up and requesting a GPIO number with the
  legacy API, convert it to a descriptor.

This way we can later isolate and drop the legacy code as
more platforms move over to using descriptors.

Cc: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
I would be very happy if some people can test this. I think
only the chromebook use it from device tree, but I'm equally
interested in the legacy usecase to work as nice as before.
---
 drivers/power/gpio-charger.c | 84 ++++++++++++++++++++++++++------------------
 1 file changed, 50 insertions(+), 34 deletions(-)

diff --git a/drivers/power/gpio-charger.c b/drivers/power/gpio-charger.c
index c5869b1941ac..001731e88718 100644
--- a/drivers/power/gpio-charger.c
+++ b/drivers/power/gpio-charger.c
@@ -14,7 +14,7 @@
  */
 
 #include <linux/device.h>
-#include <linux/gpio.h>
+#include <linux/gpio.h> /* For legacy platform data */
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
@@ -23,7 +23,7 @@
 #include <linux/power_supply.h>
 #include <linux/slab.h>
 #include <linux/of.h>
-#include <linux/of_gpio.h>
+#include <linux/gpio/consumer.h>
 
 #include <linux/power/gpio-charger.h>
 
@@ -34,6 +34,8 @@ struct gpio_charger {
 
 	struct power_supply *charger;
 	struct power_supply_desc charger_desc;
+	struct gpio_desc *gpiod;
+	bool legacy_gpio_requested;
 };
 
 static irqreturn_t gpio_charger_irq(int irq, void *devid)
@@ -58,7 +60,8 @@ static int gpio_charger_get_property(struct power_supply *psy,
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_ONLINE:
-		val->intval = !!gpio_get_value_cansleep(pdata->gpio);
+		val->intval = gpiod_get_value_cansleep(gpio_charger->gpiod);
+		/* This xor is only ever used with legacy pdata GPIO */
 		val->intval ^= pdata->gpio_active_low;
 		break;
 	default:
@@ -78,7 +81,6 @@ struct gpio_charger_platform_data *gpio_charger_parse_dt(struct device *dev)
 	struct device_node *np = dev->of_node;
 	struct gpio_charger_platform_data *pdata;
 	const char *chargetype;
-	enum of_gpio_flags flags;
 	int ret;
 
 	if (!np)
@@ -89,16 +91,6 @@ struct gpio_charger_platform_data *gpio_charger_parse_dt(struct device *dev)
 		return ERR_PTR(-ENOMEM);
 
 	pdata->name = np->name;
-
-	pdata->gpio = of_get_gpio_flags(np, 0, &flags);
-	if (pdata->gpio < 0) {
-		if (pdata->gpio != -EPROBE_DEFER)
-			dev_err(dev, "could not get charger gpio\n");
-		return ERR_PTR(pdata->gpio);
-	}
-
-	pdata->gpio_active_low = !!(flags & OF_GPIO_ACTIVE_LOW);
-
 	pdata->type = POWER_SUPPLY_TYPE_UNKNOWN;
 	ret = of_property_read_string(np, "charger-type", &chargetype);
 	if (ret >= 0) {
@@ -144,11 +136,6 @@ static int gpio_charger_probe(struct platform_device *pdev)
 		}
 	}
 
-	if (!gpio_is_valid(pdata->gpio)) {
-		dev_err(&pdev->dev, "Invalid gpio pin\n");
-		return -EINVAL;
-	}
-
 	gpio_charger = devm_kzalloc(&pdev->dev, sizeof(*gpio_charger),
 					GFP_KERNEL);
 	if (!gpio_charger) {
@@ -156,6 +143,45 @@ static int gpio_charger_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	/*
+	 * This will fetch a GPIO descriptor from device tree, ACPI or
+	 * boardfile descriptor tables. It's good to try this first.
+	 */
+	gpio_charger->gpiod = devm_gpiod_get(&pdev->dev, NULL, GPIOD_IN);
+
+	/*
+	 * If this fails and we're not using device tree, try the
+	 * legacy platform data method.
+	 */
+	if (IS_ERR(gpio_charger->gpiod) && !pdev->dev.of_node) {
+		/* Non-DT: use legacy GPIO numbers */
+		if (!gpio_is_valid(pdata->gpio)) {
+			dev_err(&pdev->dev, "Invalid gpio pin in pdata\n");
+			return -EINVAL;
+		}
+		ret = gpio_request(pdata->gpio, dev_name(&pdev->dev));
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to request gpio pin: %d\n",
+				ret);
+			return ret;
+		}
+		gpio_charger->legacy_gpio_requested = true;
+		ret = gpio_direction_input(pdata->gpio);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to set gpio to input: %d\n",
+				ret);
+			goto err_gpio_free;
+		}
+		/* Then convert this to gpiod for now */
+		gpio_charger->gpiod = gpio_to_desc(pdata->gpio);
+	} else if (IS_ERR(gpio_charger->gpiod)) {
+		/* Just try again if this happens */
+		if (PTR_ERR(gpio_charger->gpiod) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		dev_err(&pdev->dev, "error getting GPIO descriptor\n");
+		return PTR_ERR(gpio_charger->gpiod);
+	}
+
 	charger_desc = &gpio_charger->charger_desc;
 
 	charger_desc->name = pdata->name ? pdata->name : "gpio-charger";
@@ -169,17 +195,6 @@ static int gpio_charger_probe(struct platform_device *pdev)
 	psy_cfg.of_node = pdev->dev.of_node;
 	psy_cfg.drv_data = gpio_charger;
 
-	ret = gpio_request(pdata->gpio, dev_name(&pdev->dev));
-	if (ret) {
-		dev_err(&pdev->dev, "Failed to request gpio pin: %d\n", ret);
-		goto err_free;
-	}
-	ret = gpio_direction_input(pdata->gpio);
-	if (ret) {
-		dev_err(&pdev->dev, "Failed to set gpio to input: %d\n", ret);
-		goto err_gpio_free;
-	}
-
 	gpio_charger->pdata = pdata;
 
 	gpio_charger->charger = power_supply_register(&pdev->dev,
@@ -191,7 +206,7 @@ static int gpio_charger_probe(struct platform_device *pdev)
 		goto err_gpio_free;
 	}
 
-	irq = gpio_to_irq(pdata->gpio);
+	irq = gpiod_to_irq(gpio_charger->gpiod);
 	if (irq > 0) {
 		ret = request_any_context_irq(irq, gpio_charger_irq,
 				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
@@ -209,8 +224,8 @@ static int gpio_charger_probe(struct platform_device *pdev)
 	return 0;
 
 err_gpio_free:
-	gpio_free(pdata->gpio);
-err_free:
+	if (gpio_charger->legacy_gpio_requested)
+		gpio_free(pdata->gpio);
 	return ret;
 }
 
@@ -223,7 +238,8 @@ static int gpio_charger_remove(struct platform_device *pdev)
 
 	power_supply_unregister(gpio_charger->charger);
 
-	gpio_free(gpio_charger->pdata->gpio);
+	if (gpio_charger->legacy_gpio_requested)
+		gpio_free(gpio_charger->pdata->gpio);
 
 	return 0;
 }
-- 
2.7.4

[toc] | [next] | [standalone]

#1486782

[Multipart message — attachments visible in raw view] — view raw

Hi Linus,

On Fri, Sep 09, 2016 at 01:40:04PM +0200, Linus Walleij wrote:
> The GPIO charger is using a mix of the legacy GPIO interface
> and <linux/of_gpio.h> which is not the modern way to use GPIOs.
> 
> Refactor like this:
> 
> - Use a GPIO descriptor for the GPIO line used to monitor the
>   charger.
> - Fetch the descriptor with devm_gpiod_get() as the first
>   method.
> - If this fails and we are *not* using device tree, then
>   start looking to see if we can use platform data instead.
> - After looking up and requesting a GPIO number with the
>   legacy API, convert it to a descriptor.
> 
> This way we can later isolate and drop the legacy code as
> more platforms move over to using descriptors.
>
> Cc: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> I would be very happy if some people can test this. I think
> only the chromebook use it from device tree, but I'm equally
> interested in the legacy usecase to work as nice as before.

Since this is not tested on any hardware and we're already
at rc7, I would prefer to take this after the merge window.
Any reason this should go into v4.9?

>  drivers/power/gpio-charger.c | 84 ++++++++++++++++++++++++++------------------
>  1 file changed, 50 insertions(+), 34 deletions(-)

This is obviously not based on power-supply's next branch, since
I moved all drivers to drivers/power/supply.

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1486794

On Mon, Sep 19, 2016 at 9:18 PM, Sebastian Reichel <sre@kernel.org> wrote:
> On Fri, Sep 09, 2016 at 01:40:04PM +0200, Linus Walleij wrote:
>> The GPIO charger is using a mix of the legacy GPIO interface
>> and <linux/of_gpio.h> which is not the modern way to use GPIOs.
>>
>> Refactor like this:
>>
>> - Use a GPIO descriptor for the GPIO line used to monitor the
>>   charger.
>> - Fetch the descriptor with devm_gpiod_get() as the first
>>   method.
>> - If this fails and we are *not* using device tree, then
>>   start looking to see if we can use platform data instead.
>> - After looking up and requesting a GPIO number with the
>>   legacy API, convert it to a descriptor.
>>
>> This way we can later isolate and drop the legacy code as
>> more platforms move over to using descriptors.
>>
>> Cc: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>> Cc: Heiko Stuebner <heiko@sntech.de>
>> Cc: Lars-Peter Clausen <lars@metafoo.de>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>> I would be very happy if some people can test this. I think
>> only the chromebook use it from device tree, but I'm equally
>> interested in the legacy usecase to work as nice as before.
>
> Since this is not tested on any hardware and we're already
> at rc7, I would prefer to take this after the merge window.
> Any reason this should go into v4.9?

No, there is no hurry.

>>  drivers/power/gpio-charger.c | 84 ++++++++++++++++++++++++++------------------
>>  1 file changed, 50 insertions(+), 34 deletions(-)
>
> This is obviously not based on power-supply's next branch, since
> I moved all drivers to drivers/power/supply.

I will rebase and resend after v4.9-rc1 then.

Yours,
Linus Walleij

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web