Re: [PATCH v2 2/3] power: gpio-charger: add device tree support
From: Doug Anderson <dianders@chromium.org>
Date: 2014-09-22 16:48:38
Also in:
linux-pm, lkml
Heiko On Sun, Sep 21, 2014 at 1:05 PM, Heiko Stuebner [off-list ref] wrote:
quoted hunk ↗ jump to hunk
From: Heiko Stuebner <redacted> Add the ability to parse gpio-charger data from a devicetree node. Signed-off-by: Heiko Stuebner <redacted> --- drivers/power/gpio-charger.c | 66 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 63 insertions(+), 3 deletions(-)diff --git a/drivers/power/gpio-charger.c b/drivers/power/gpio-charger.c index a0024b2..5fe6879 100644 --- a/drivers/power/gpio-charger.c +++ b/drivers/power/gpio-charger.c@@ -22,6 +22,8 @@ #include <linux/platform_device.h> #include <linux/power_supply.h> #include <linux/slab.h> +#include <linux/of.h> +#include <linux/of_gpio.h> #include <linux/power/gpio-charger.h>@@ -69,17 +71,68 @@ static enum power_supply_property gpio_charger_properties[] = { POWER_SUPPLY_PROP_ONLINE, }; +static +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) + return ERR_PTR(-ENOENT); + + pdata = devm_kzalloc(dev, sizeof(struct gpio_charger_platform_data),
nit: sizeof(*pdata) is smaller / cleaner.
+ GFP_KERNEL); + if (!pdata) + return ERR_PTR(-ENOMEM); + + pdata->name = np->name; + + pdata->gpio = of_get_gpio_flags(np, 0, &flags);
It would be nice to check the gpio for errors here, including handling the possibility of EPROBE_DEFER.
+ 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) {
+ if (!strncmp("unknown", chargetype, 7))
+ pdata->type = POWER_SUPPLY_TYPE_UNKNOWN;
+ else if (!strncmp("battery", chargetype, 7))
+ pdata->type = POWER_SUPPLY_TYPE_BATTERY;
+ else if (!strncmp("ups", chargetype, 3))
+ pdata->type = POWER_SUPPLY_TYPE_UPS;
+ else if (!strncmp("mains", chargetype, 5))
+ pdata->type = POWER_SUPPLY_TYPE_MAINS;
+ else if (!strncmp("usb-sdp", chargetype, 7))
+ pdata->type = POWER_SUPPLY_TYPE_USB;
+ else if (!strncmp("usb-dcp", chargetype, 7))
+ pdata->type = POWER_SUPPLY_TYPE_USB_DCP;
+ else if (!strncmp("usb-cdp", chargetype, 7))
+ pdata->type = POWER_SUPPLY_TYPE_USB_CDP;
+ else if (!strncmp("usb-aca", chargetype, 7))
+ pdata->type = POWER_SUPPLY_TYPE_USB_ACA;
+ else
+ dev_warn(dev, "unknown charger type %s\n", chargetype);
+ }
+
+ return pdata;
+}
+
static int gpio_charger_probe(struct platform_device *pdev)
{
- const struct gpio_charger_platform_data *pdata = pdev->dev.platform_data;
+ struct gpio_charger_platform_data *pdata = pdev->dev.platform_data;Do you really need to remove const? If I remember correctly the const here is not saying that the variable "pdata" won't change but that the data pointed to by pdata won't change. I think that's still the case (from the perspective of this function).
struct gpio_charger *gpio_charger;
struct power_supply *charger;
int ret;
int irq;
if (!pdata) {
- dev_err(&pdev->dev, "No platform data\n");
- return -EINVAL;
+ pdata = gpio_charger_parse_dt(&pdev->dev);
+ if (IS_ERR(pdata)) {
+ dev_err(&pdev->dev, "No platform data\n");
+ return -EINVAL;Probably should pass back the error code since it might be EPROBE_DEFER. ...and <sigh> I guess you need the special case of "don't print if it's EPROBE_DEFER". I almost wish we had a helper function for that. ;)
quoted hunk ↗ jump to hunk
+ } } if (!gpio_is_valid(pdata->gpio)) {@@ -189,6 +242,12 @@ static int gpio_charger_resume(struct device *dev) static SIMPLE_DEV_PM_OPS(gpio_charger_pm_ops, gpio_charger_suspend, gpio_charger_resume); +static const struct of_device_id gpio_charger_match[] = { + { .compatible = "gpio-charger" }, + { } +}; +MODULE_DEVICE_TABLE(of, gpio_charger_match); + static struct platform_driver gpio_charger_driver = { .probe = gpio_charger_probe, .remove = gpio_charger_remove,@@ -196,6 +255,7 @@ static struct platform_driver gpio_charger_driver = { .name = "gpio-charger", .owner = THIS_MODULE, .pm = &gpio_charger_pm_ops, + .of_match_table = of_match_ptr(gpio_charger_match),
Given that you don't have any #ifdefs with "CONFIG_OF", I think gpio_charger_match will always exist. It seems like you should remove the of_match_ptr or add some #ifdefs. I can't quite keep up with what the currently suggested best practice is here, though. -Doug