From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754616AbaIVQsi (ORCPT ); Mon, 22 Sep 2014 12:48:38 -0400 Received: from mail-vc0-f175.google.com ([209.85.220.175]:41970 "EHLO mail-vc0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754085AbaIVQsf (ORCPT ); Mon, 22 Sep 2014 12:48:35 -0400 MIME-Version: 1.0 In-Reply-To: <1411329923-15912-3-git-send-email-heiko@sntech.de> References: <1411329923-15912-1-git-send-email-heiko@sntech.de> <1411329923-15912-3-git-send-email-heiko@sntech.de> Date: Mon, 22 Sep 2014 09:48:33 -0700 X-Google-Sender-Auth: UB3mO71YDLIhYxqtYHwdRvG2ppY Message-ID: Subject: Re: [PATCH v2 2/3] power: gpio-charger: add device tree support From: Doug Anderson To: Heiko Stuebner Cc: sre@kernel.org, Dmitry Eremin-Solenikov , David Woodhouse , "linux-pm@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , Rob Herring , Mark Rutland , Pawel Moll , Ian Campbell , Kumar Gala , Heiko Stuebner Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Heiko On Sun, Sep 21, 2014 at 1:05 PM, Heiko Stuebner wrote: > From: Heiko Stuebner > > Add the ability to parse gpio-charger data from a devicetree node. > > Signed-off-by: Heiko Stuebner > --- > 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 > #include > #include > +#include > +#include > > #include > > @@ -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 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. ;) > + } > } > > 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