* [PATCH 0/2] max8903: Add device tree support and logic fixup
@ 2016-06-07 4:02 chris
2016-06-07 4:02 ` [PATCH 1/2] max8903: adds support for initiation via device tree chris
2016-06-07 4:02 ` [PATCH 2/2] max8903: cleans up confusing relationship between dc_valid, dok and dcm chris
0 siblings, 2 replies; 8+ messages in thread
From: chris @ 2016-06-07 4:02 UTC (permalink / raw)
To: linux-pm, dwmw2, dbaryshkov, sre; +Cc: Chris Lapa
From: Chris Lapa <chris@lapa.com.au>
This patch set adds device tree support for the MAX8903 battery charger
and also cleans up the logic with the dc_valid, dok and dcm pins.
I verified these patches work on a board I have here, which uses the
DC power side (not the USB portition) of the MAX8903.
Chris Lapa (2):
max8903: adds support for initiation via device tree.
max8903: cleans up confusing relationship between dc_valid, dok and
dcm.
.../devicetree/bindings/power/max8903-charger.txt | 28 ++
drivers/power/max8903_charger.c | 287 ++++++++++++++++-----
include/linux/power/max8903_charger.h | 6 +-
3 files changed, 250 insertions(+), 71 deletions(-)
create mode 100644 Documentation/devicetree/bindings/power/max8903-charger.txt
--
1.9.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/2] max8903: adds support for initiation via device tree. 2016-06-07 4:02 [PATCH 0/2] max8903: Add device tree support and logic fixup chris @ 2016-06-07 4:02 ` chris 2016-06-07 4:02 ` [PATCH 2/2] max8903: cleans up confusing relationship between dc_valid, dok and dcm chris 1 sibling, 0 replies; 8+ messages in thread From: chris @ 2016-06-07 4:02 UTC (permalink / raw) To: linux-pm, dwmw2, dbaryshkov, sre; +Cc: Chris Lapa From: Chris Lapa <chris@lapa.com.au> This commit also adds requesting gpio's via devm_gpio_request() to ensure the gpio is available for usage by the driver. Signed-off-by: Chris Lapa <chris@lapa.com.au> --- .../devicetree/bindings/power/max8903-charger.txt | 28 ++ drivers/power/max8903_charger.c | 281 ++++++++++++++++----- 2 files changed, 250 insertions(+), 59 deletions(-) create mode 100644 Documentation/devicetree/bindings/power/max8903-charger.txt diff --git a/Documentation/devicetree/bindings/power/max8903-charger.txt b/Documentation/devicetree/bindings/power/max8903-charger.txt new file mode 100644 index 0000000..7207731 --- /dev/null +++ b/Documentation/devicetree/bindings/power/max8903-charger.txt @@ -0,0 +1,28 @@ +Maxim Semiconductor MAX8903 Battery Charger bindings + +Required properties: +- compatible: "max8903-charger" for MAX8903 Battery Charger +- dc_valid: + - dok: DC power OK pin +- usb_valid: + - uok: USB power OK pin + +Optional properties: +- cen: Charge enable pin +- chg: Charger status pin +- flt: Fault pin +- dcm: Current limit mode setting (DC or USB) +- usus: USB suspend pin + + +Example: + + max8903-charger { + compatible = "max8903-charger"; + dok = <&gpio2 3 GPIO_ACTIVE_LOW>; + flt = <&gpio2 2 GPIO_ACTIVE_LOW>; + chg = <&gpio3 15 GPIO_ACTIVE_LOW>; + cen = <&gpio2 5 GPIO_ACTIVE_LOW>; + dc_valid; + status = "okay"; + }; diff --git a/drivers/power/max8903_charger.c b/drivers/power/max8903_charger.c index 17876ca..1989c10 100644 --- a/drivers/power/max8903_charger.c +++ b/drivers/power/max8903_charger.c @@ -23,13 +23,16 @@ #include <linux/gpio.h> #include <linux/interrupt.h> #include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_gpio.h> #include <linux/slab.h> #include <linux/power_supply.h> #include <linux/platform_device.h> #include <linux/power/max8903_charger.h> struct max8903_data { - struct max8903_pdata pdata; + struct max8903_pdata *pdata; struct device *dev; struct power_supply *psy; struct power_supply_desc psy_desc; @@ -53,8 +56,8 @@ static int max8903_get_property(struct power_supply *psy, switch (psp) { case POWER_SUPPLY_PROP_STATUS: val->intval = POWER_SUPPLY_STATUS_UNKNOWN; - if (data->pdata.chg) { - if (gpio_get_value(data->pdata.chg) == 0) + if (data->pdata->chg) { + if (gpio_get_value(data->pdata->chg) == 0) val->intval = POWER_SUPPLY_STATUS_CHARGING; else if (data->usb_in || data->ta_in) val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; @@ -75,13 +78,14 @@ static int max8903_get_property(struct power_supply *psy, default: return -EINVAL; } + return 0; } static irqreturn_t max8903_dcin(int irq, void *_data) { struct max8903_data *data = _data; - struct max8903_pdata *pdata = &data->pdata; + struct max8903_pdata *pdata = data->pdata; bool ta_in; enum power_supply_type old_type; @@ -122,7 +126,7 @@ static irqreturn_t max8903_dcin(int irq, void *_data) static irqreturn_t max8903_usbin(int irq, void *_data) { struct max8903_data *data = _data; - struct max8903_pdata *pdata = &data->pdata; + struct max8903_pdata *pdata = data->pdata; bool usb_in; enum power_supply_type old_type; @@ -161,7 +165,7 @@ static irqreturn_t max8903_usbin(int irq, void *_data) static irqreturn_t max8903_fault(int irq, void *_data) { struct max8903_data *data = _data; - struct max8903_pdata *pdata = &data->pdata; + struct max8903_pdata *pdata = data->pdata; bool fault; fault = gpio_get_value(pdata->flt) ? false : true; @@ -179,34 +183,135 @@ static irqreturn_t max8903_fault(int irq, void *_data) return IRQ_HANDLED; } +static struct max8903_pdata *max8903_parse_dt_data( + struct device *dev) +{ + struct device_node *of_node = dev->of_node; + struct max8903_pdata *pdata = NULL; + + if (!of_node) { + return pdata; + } + + pdata = devm_kzalloc(dev, sizeof(struct max8903_pdata), + GFP_KERNEL); + if (!pdata) { + return pdata; + } + + if (of_get_property(of_node, "dc_valid", NULL)) { + pdata->dc_valid = true; + } + + if (of_get_property(of_node, "usb_valid", NULL)) { + pdata->usb_valid = true; + } + + pdata->cen = of_get_named_gpio(of_node, "cen", 0); + if (!gpio_is_valid(pdata->cen)) { + pdata->cen = 0; + } + + pdata->chg = of_get_named_gpio(of_node, "chg", 0); + if (!gpio_is_valid(pdata->chg)) { + pdata->chg = 0; + } + + pdata->flt = of_get_named_gpio(of_node, "flt", 0); + if (!gpio_is_valid(pdata->flt)) { + pdata->flt = 0; + } + + pdata->usus = of_get_named_gpio(of_node, "usus", 0); + if (!gpio_is_valid(pdata->usus)) { + pdata->usus = 0; + } + + pdata->dcm = of_get_named_gpio(of_node, "dcm", 0); + if (!gpio_is_valid(pdata->dcm)) { + pdata->dcm = 0; + } + + pdata->dok = of_get_named_gpio(of_node, "dok", 0); + if (!gpio_is_valid(pdata->dok)) { + pdata->dok = 0; + } + + pdata->uok = of_get_named_gpio(of_node, "uok", 0); + if (!gpio_is_valid(pdata->uok)) { + pdata->uok = 0; + } + + return pdata; +} + static int max8903_probe(struct platform_device *pdev) { - struct max8903_data *data; + struct max8903_data *charger; struct device *dev = &pdev->dev; - struct max8903_pdata *pdata = pdev->dev.platform_data; struct power_supply_config psy_cfg = {}; int ret = 0; int gpio; int ta_in = 0; int usb_in = 0; - data = devm_kzalloc(dev, sizeof(struct max8903_data), GFP_KERNEL); - if (data == NULL) { + charger = devm_kzalloc(dev, sizeof(struct max8903_data), GFP_KERNEL); + if (charger == NULL) { dev_err(dev, "Cannot allocate memory.\n"); return -ENOMEM; } - memcpy(&data->pdata, pdata, sizeof(struct max8903_pdata)); - data->dev = dev; - platform_set_drvdata(pdev, data); - if (pdata->dc_valid == false && pdata->usb_valid == false) { + charger->pdata = pdev->dev.platform_data; + if (IS_ENABLED(CONFIG_OF) && !charger->pdata && dev->of_node) { + charger->pdata = max8903_parse_dt_data(dev); + if (!charger->pdata) + return -EINVAL; + } + + charger->dev = dev; + + platform_set_drvdata(pdev, charger); + + charger->fault = false; + charger->ta_in = ta_in; + charger->usb_in = usb_in; + + charger->psy_desc.name = "max8903_charger"; + charger->psy_desc.type = (ta_in) ? POWER_SUPPLY_TYPE_MAINS : + ((usb_in) ? POWER_SUPPLY_TYPE_USB : + POWER_SUPPLY_TYPE_BATTERY); + charger->psy_desc.get_property = max8903_get_property; + charger->psy_desc.properties = max8903_charger_props; + charger->psy_desc.num_properties = ARRAY_SIZE(max8903_charger_props); + + if (charger->pdata->dc_valid == false && charger->pdata->usb_valid == false) { dev_err(dev, "No valid power sources.\n"); return -EINVAL; } - if (pdata->dc_valid) { - if (pdata->dok && gpio_is_valid(pdata->dok) && - pdata->dcm && gpio_is_valid(pdata->dcm)) { + if (charger->pdata->dc_valid) { + if (charger->pdata->dok && gpio_is_valid(charger->pdata->dok) && + charger->pdata->dcm && gpio_is_valid(charger->pdata->dcm)) { + ret = devm_gpio_request(dev, + charger->pdata->dok, + charger->psy_desc.name); + if (ret) { + dev_err(dev, + "Failed GPIO request for dok: %d err %d\n", + charger->pdata->dok, ret); + return -EINVAL; + } + + ret = devm_gpio_request(dev, + charger->pdata->dcm, + charger->psy_desc.name); + if (ret) { + dev_err(dev, + "Failed GPIO request for dcm: %d err %d\n", + charger->pdata->dcm, ret); + return -EINVAL; + } + gpio = pdata->dok; /* PULL_UPed Interrupt */ ta_in = gpio_get_value(gpio) ? 0 : 1; @@ -219,18 +324,38 @@ static int max8903_probe(struct platform_device *pdev) } } else { if (pdata->dcm) { - if (gpio_is_valid(pdata->dcm)) + if (gpio_is_valid(pdata->dcm)) { + ret = devm_gpio_request(dev, + charger->pdata->dcm, + charger->psy_desc.name); + if (ret) { + dev_err(dev, + "Failed GPIO request for dcm: %d err %d\n", + charger->pdata->dcm, ret); + return -EINVAL; + } + gpio_set_value(pdata->dcm, 0); - else { + } else { dev_err(dev, "Invalid pin: dcm.\n"); return -EINVAL; } } } - if (pdata->usb_valid) { - if (pdata->uok && gpio_is_valid(pdata->uok)) { - gpio = pdata->uok; + if (charger->pdata->usb_valid) { + if (gpio_is_valid(charger->pdata->uok)) { + ret = devm_gpio_request(dev, + charger->pdata->uok, + charger->psy_desc.name); + if (ret) { + dev_err(dev, + "Failed GPIO request for uok: %d err %d\n", + charger->pdata->uok, ret); + return -EINVAL; + } + + gpio = charger->pdata->uok; usb_in = gpio_get_value(gpio) ? 0 : 1; } else { dev_err(dev, "When USB is wired, UOK should be wired." @@ -239,91 +364,122 @@ static int max8903_probe(struct platform_device *pdev) } } - if (pdata->cen) { - if (gpio_is_valid(pdata->cen)) { - gpio_set_value(pdata->cen, (ta_in || usb_in) ? 0 : 1); + if (charger->pdata->cen) { + if (gpio_is_valid(charger->pdata->cen)) { + ret = devm_gpio_request(dev, + charger->pdata->cen, + charger->psy_desc.name); + if (ret) { + dev_err(dev, + "Failed GPIO request for cen: %d err %d\n", + charger->pdata->cen, ret); + return -EINVAL; + } + + gpio_set_value(charger->pdata->cen, (ta_in || usb_in) ? 0 : 1); } else { dev_err(dev, "Invalid pin: cen.\n"); return -EINVAL; } } - if (pdata->chg) { - if (!gpio_is_valid(pdata->chg)) { + if (charger->pdata->chg) { + if (gpio_is_valid(charger->pdata->chg)) { + ret = devm_gpio_request(dev, + charger->pdata->chg, + charger->psy_desc.name); + if (ret) { + dev_err(dev, + "Failed GPIO request for chg: %d err %d\n", + charger->pdata->chg, ret); + return -EINVAL; + } + } else { dev_err(dev, "Invalid pin: chg.\n"); return -EINVAL; } } - if (pdata->flt) { - if (!gpio_is_valid(pdata->flt)) { + if (charger->pdata->flt) { + if (gpio_is_valid(charger->pdata->flt)) { + ret = devm_gpio_request(dev, + charger->pdata->flt, + charger->psy_desc.name); + if (ret) { + dev_err(dev, + "Failed GPIO request for flt: %d err %d\n", + charger->pdata->flt, ret); + return -EINVAL; + } + } else { dev_err(dev, "Invalid pin: flt.\n"); return -EINVAL; } } - if (pdata->usus) { - if (!gpio_is_valid(pdata->usus)) { + if (charger->pdata->usus) { + if (gpio_is_valid(charger->pdata->usus)) { + ret = devm_gpio_request(dev, + charger->pdata->usus, + charger->psy_desc.name); + if (ret) { + dev_err(dev, + "Failed GPIO request for usus: %d err %d\n", + charger->pdata->usus, ret); + return -EINVAL; + } + } else { dev_err(dev, "Invalid pin: usus.\n"); return -EINVAL; } } - data->fault = false; - data->ta_in = ta_in; - data->usb_in = usb_in; - - data->psy_desc.name = "max8903_charger"; - data->psy_desc.type = (ta_in) ? POWER_SUPPLY_TYPE_MAINS : - ((usb_in) ? POWER_SUPPLY_TYPE_USB : - POWER_SUPPLY_TYPE_BATTERY); - data->psy_desc.get_property = max8903_get_property; - data->psy_desc.properties = max8903_charger_props; - data->psy_desc.num_properties = ARRAY_SIZE(max8903_charger_props); - - psy_cfg.drv_data = data; + psy_cfg.supplied_to = NULL; + psy_cfg.num_supplicants = 0; + psy_cfg.of_node = dev->of_node; + psy_cfg.drv_data = charger; - data->psy = devm_power_supply_register(dev, &data->psy_desc, &psy_cfg); - if (IS_ERR(data->psy)) { + charger->psy = devm_power_supply_register(dev, &charger->psy_desc, &psy_cfg); + if (IS_ERR(charger->psy)) { dev_err(dev, "failed: power supply register.\n"); - return PTR_ERR(data->psy); + return PTR_ERR(charger->psy); } - if (pdata->dc_valid) { - ret = devm_request_threaded_irq(dev, gpio_to_irq(pdata->dok), + if (charger->pdata->dc_valid) { + ret = devm_request_threaded_irq(dev, gpio_to_irq(charger->pdata->dok), NULL, max8903_dcin, IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING | IRQF_ONESHOT, - "MAX8903 DC IN", data); + "MAX8903 DC IN", charger); if (ret) { dev_err(dev, "Cannot request irq %d for DC (%d)\n", - gpio_to_irq(pdata->dok), ret); + gpio_to_irq(charger->pdata->dok), ret); return ret; } } - if (pdata->usb_valid) { - ret = devm_request_threaded_irq(dev, gpio_to_irq(pdata->uok), + if (charger->pdata->usb_valid) { + ret = devm_request_threaded_irq(dev, gpio_to_irq(charger->pdata->uok), NULL, max8903_usbin, IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING | IRQF_ONESHOT, - "MAX8903 USB IN", data); + "MAX8903 USB IN", charger); if (ret) { dev_err(dev, "Cannot request irq %d for USB (%d)\n", - gpio_to_irq(pdata->uok), ret); + gpio_to_irq(charger->pdata->uok), ret); return ret; } } - if (pdata->flt) { - ret = devm_request_threaded_irq(dev, gpio_to_irq(pdata->flt), + if (charger->pdata->flt) { + ret = devm_request_threaded_irq(dev, gpio_to_irq(charger->pdata->flt), NULL, max8903_fault, IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING | IRQF_ONESHOT, - "MAX8903 Fault", data); + "MAX8903 Fault", charger); if (ret) { dev_err(dev, "Cannot request irq %d for Fault (%d)\n", - gpio_to_irq(pdata->flt), ret); + gpio_to_irq(charger->pdata->flt), ret); return ret; } } @@ -331,10 +487,17 @@ static int max8903_probe(struct platform_device *pdev) return 0; } +static const struct of_device_id max8903_match_ids[] = { + { .compatible = "max8903-charger", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, max8903_match_ids); + static struct platform_driver max8903_driver = { .probe = max8903_probe, .driver = { .name = "max8903-charger", + .of_match_table = max8903_match_ids }, }; -- 1.9.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] max8903: cleans up confusing relationship between dc_valid, dok and dcm. 2016-06-07 4:02 [PATCH 0/2] max8903: Add device tree support and logic fixup chris 2016-06-07 4:02 ` [PATCH 1/2] max8903: adds support for initiation via device tree chris @ 2016-06-07 4:02 ` chris 1 sibling, 0 replies; 8+ messages in thread From: chris @ 2016-06-07 4:02 UTC (permalink / raw) To: linux-pm, dwmw2, dbaryshkov, sre; +Cc: Chris Lapa From: Chris Lapa <chris@lapa.com.au> The max8903_charger.h file indicated that dcm and dok were not optional when dc_valid is set. It makes sense to have dok as a compulsory pin when dc_valid is given. However dcm can be optionally wired to a fixed level especially when the circuit is configured for dc power exclusively. The previous implementation already allowed for this somewhat, however no error was given if dok wasn't given whilst dc_valid was. The new implementation enforces dok presence when dc_valid is given. Whilst allowing dcm to be optional. Signed-off-by: Chris Lapa <chris@lapa.com.au> --- drivers/power/max8903_charger.c | 40 ++++++++++++----------------------- include/linux/power/max8903_charger.h | 6 +++--- 2 files changed, 17 insertions(+), 29 deletions(-) diff --git a/drivers/power/max8903_charger.c b/drivers/power/max8903_charger.c index 1989c10..d3c09f9 100644 --- a/drivers/power/max8903_charger.c +++ b/drivers/power/max8903_charger.c @@ -290,8 +290,7 @@ static int max8903_probe(struct platform_device *pdev) } if (charger->pdata->dc_valid) { - if (charger->pdata->dok && gpio_is_valid(charger->pdata->dok) && - charger->pdata->dcm && gpio_is_valid(charger->pdata->dcm)) { + if (charger->pdata->dok && gpio_is_valid(charger->pdata->dok)) { ret = devm_gpio_request(dev, charger->pdata->dok, charger->psy_desc.name); @@ -302,6 +301,17 @@ static int max8903_probe(struct platform_device *pdev) return -EINVAL; } + gpio = charger->pdata->dok; /* PULL_UPed Interrupt */ + ta_in = gpio_get_value(gpio) ? 0 : 1; + } else { + dev_err(dev, "When DC is wired, DOK should" + " be wired as well.\n"); + return -EINVAL; + } + } + + if (charger->pdata->dcm) { + if (gpio_is_valid(charger->pdata->dcm)) { ret = devm_gpio_request(dev, charger->pdata->dcm, charger->psy_desc.name); @@ -312,35 +322,13 @@ static int max8903_probe(struct platform_device *pdev) return -EINVAL; } - gpio = pdata->dok; /* PULL_UPed Interrupt */ - ta_in = gpio_get_value(gpio) ? 0 : 1; - gpio = pdata->dcm; /* Output */ + gpio = charger->pdata->dcm; /* Output */ gpio_set_value(gpio, ta_in); } else { - dev_err(dev, "When DC is wired, DOK and DCM should" - " be wired as well.\n"); + dev_err(dev, "Invalid pin: dcm.\n"); return -EINVAL; } - } else { - if (pdata->dcm) { - if (gpio_is_valid(pdata->dcm)) { - ret = devm_gpio_request(dev, - charger->pdata->dcm, - charger->psy_desc.name); - if (ret) { - dev_err(dev, - "Failed GPIO request for dcm: %d err %d\n", - charger->pdata->dcm, ret); - return -EINVAL; - } - - gpio_set_value(pdata->dcm, 0); - } else { - dev_err(dev, "Invalid pin: dcm.\n"); - return -EINVAL; - } - } } if (charger->pdata->usb_valid) { diff --git a/include/linux/power/max8903_charger.h b/include/linux/power/max8903_charger.h index 24f51db..89d3f1c 100644 --- a/include/linux/power/max8903_charger.h +++ b/include/linux/power/max8903_charger.h @@ -26,8 +26,8 @@ struct max8903_pdata { /* * GPIOs - * cen, chg, flt, and usus are optional. - * dok, dcm, and uok are not optional depending on the status of + * cen, chg, flt, dcm and usus are optional. + * dok and uok are not optional depending on the status of * dc_valid and usb_valid. */ int cen; /* Charger Enable input */ @@ -41,7 +41,7 @@ struct max8903_pdata { /* * DC(Adapter/TA) is wired * When dc_valid is true, - * dok and dcm should be valid. + * dok should be valid. * * At least one of dc_valid or usb_valid should be true. */ -- 1.9.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 0/2] max8903: Add device tree support and logic fixup
@ 2016-06-02 6:44 chris
2016-06-02 6:44 ` [PATCH 1/2] max8903: adds support for initiation via device tree chris
0 siblings, 1 reply; 8+ messages in thread
From: chris @ 2016-06-02 6:44 UTC (permalink / raw)
To: linux-kernel, linux-pm; +Cc: Chris Lapa
From: Chris Lapa <chris@lapa.com.au>
This patch set adds device tree support for the MAX8903 battery charger
and also cleans up the logic with the dc_valid, dok and dcm pins.
I verified these patches work on a board I have here, which uses the
DC power side (not the USB portition) of the MAX8903.
Chris Lapa (2):
max8903: adds support for initiation via device tree.
max8903: cleans up confusing relationship between dc_valid, dok and
dcm.
.../devicetree/bindings/power/max8903-charger.txt | 28 ++
drivers/power/max8903_charger.c | 287 ++++++++++++++++-----
include/linux/power/max8903_charger.h | 6 +-
3 files changed, 250 insertions(+), 71 deletions(-)
create mode 100644 Documentation/devicetree/bindings/power/max8903-charger.txt
--
1.9.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/2] max8903: adds support for initiation via device tree. 2016-06-02 6:44 [PATCH 0/2] max8903: Add device tree support and logic fixup chris @ 2016-06-02 6:44 ` chris 2016-06-09 10:35 ` Krzysztof Kozlowski 0 siblings, 1 reply; 8+ messages in thread From: chris @ 2016-06-02 6:44 UTC (permalink / raw) To: linux-kernel, linux-pm; +Cc: Chris Lapa From: Chris Lapa <chris@lapa.com.au> This commit also adds requesting gpio's via devm_gpio_request() to ensure the gpio is available for usage by the driver. Signed-off-by: Chris Lapa <chris@lapa.com.au> --- .../devicetree/bindings/power/max8903-charger.txt | 28 ++ drivers/power/max8903_charger.c | 281 ++++++++++++++++----- 2 files changed, 250 insertions(+), 59 deletions(-) create mode 100644 Documentation/devicetree/bindings/power/max8903-charger.txt diff --git a/Documentation/devicetree/bindings/power/max8903-charger.txt b/Documentation/devicetree/bindings/power/max8903-charger.txt new file mode 100644 index 0000000..7207731 --- /dev/null +++ b/Documentation/devicetree/bindings/power/max8903-charger.txt @@ -0,0 +1,28 @@ +Maxim Semiconductor MAX8903 Battery Charger bindings + +Required properties: +- compatible: "max8903-charger" for MAX8903 Battery Charger +- dc_valid: + - dok: DC power OK pin +- usb_valid: + - uok: USB power OK pin + +Optional properties: +- cen: Charge enable pin +- chg: Charger status pin +- flt: Fault pin +- dcm: Current limit mode setting (DC or USB) +- usus: USB suspend pin + + +Example: + + max8903-charger { + compatible = "max8903-charger"; + dok = <&gpio2 3 GPIO_ACTIVE_LOW>; + flt = <&gpio2 2 GPIO_ACTIVE_LOW>; + chg = <&gpio3 15 GPIO_ACTIVE_LOW>; + cen = <&gpio2 5 GPIO_ACTIVE_LOW>; + dc_valid; + status = "okay"; + }; diff --git a/drivers/power/max8903_charger.c b/drivers/power/max8903_charger.c index 17876ca..1989c10 100644 --- a/drivers/power/max8903_charger.c +++ b/drivers/power/max8903_charger.c @@ -23,13 +23,16 @@ #include <linux/gpio.h> #include <linux/interrupt.h> #include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_gpio.h> #include <linux/slab.h> #include <linux/power_supply.h> #include <linux/platform_device.h> #include <linux/power/max8903_charger.h> struct max8903_data { - struct max8903_pdata pdata; + struct max8903_pdata *pdata; struct device *dev; struct power_supply *psy; struct power_supply_desc psy_desc; @@ -53,8 +56,8 @@ static int max8903_get_property(struct power_supply *psy, switch (psp) { case POWER_SUPPLY_PROP_STATUS: val->intval = POWER_SUPPLY_STATUS_UNKNOWN; - if (data->pdata.chg) { - if (gpio_get_value(data->pdata.chg) == 0) + if (data->pdata->chg) { + if (gpio_get_value(data->pdata->chg) == 0) val->intval = POWER_SUPPLY_STATUS_CHARGING; else if (data->usb_in || data->ta_in) val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; @@ -75,13 +78,14 @@ static int max8903_get_property(struct power_supply *psy, default: return -EINVAL; } + return 0; } static irqreturn_t max8903_dcin(int irq, void *_data) { struct max8903_data *data = _data; - struct max8903_pdata *pdata = &data->pdata; + struct max8903_pdata *pdata = data->pdata; bool ta_in; enum power_supply_type old_type; @@ -122,7 +126,7 @@ static irqreturn_t max8903_dcin(int irq, void *_data) static irqreturn_t max8903_usbin(int irq, void *_data) { struct max8903_data *data = _data; - struct max8903_pdata *pdata = &data->pdata; + struct max8903_pdata *pdata = data->pdata; bool usb_in; enum power_supply_type old_type; @@ -161,7 +165,7 @@ static irqreturn_t max8903_usbin(int irq, void *_data) static irqreturn_t max8903_fault(int irq, void *_data) { struct max8903_data *data = _data; - struct max8903_pdata *pdata = &data->pdata; + struct max8903_pdata *pdata = data->pdata; bool fault; fault = gpio_get_value(pdata->flt) ? false : true; @@ -179,34 +183,135 @@ static irqreturn_t max8903_fault(int irq, void *_data) return IRQ_HANDLED; } +static struct max8903_pdata *max8903_parse_dt_data( + struct device *dev) +{ + struct device_node *of_node = dev->of_node; + struct max8903_pdata *pdata = NULL; + + if (!of_node) { + return pdata; + } + + pdata = devm_kzalloc(dev, sizeof(struct max8903_pdata), + GFP_KERNEL); + if (!pdata) { + return pdata; + } + + if (of_get_property(of_node, "dc_valid", NULL)) { + pdata->dc_valid = true; + } + + if (of_get_property(of_node, "usb_valid", NULL)) { + pdata->usb_valid = true; + } + + pdata->cen = of_get_named_gpio(of_node, "cen", 0); + if (!gpio_is_valid(pdata->cen)) { + pdata->cen = 0; + } + + pdata->chg = of_get_named_gpio(of_node, "chg", 0); + if (!gpio_is_valid(pdata->chg)) { + pdata->chg = 0; + } + + pdata->flt = of_get_named_gpio(of_node, "flt", 0); + if (!gpio_is_valid(pdata->flt)) { + pdata->flt = 0; + } + + pdata->usus = of_get_named_gpio(of_node, "usus", 0); + if (!gpio_is_valid(pdata->usus)) { + pdata->usus = 0; + } + + pdata->dcm = of_get_named_gpio(of_node, "dcm", 0); + if (!gpio_is_valid(pdata->dcm)) { + pdata->dcm = 0; + } + + pdata->dok = of_get_named_gpio(of_node, "dok", 0); + if (!gpio_is_valid(pdata->dok)) { + pdata->dok = 0; + } + + pdata->uok = of_get_named_gpio(of_node, "uok", 0); + if (!gpio_is_valid(pdata->uok)) { + pdata->uok = 0; + } + + return pdata; +} + static int max8903_probe(struct platform_device *pdev) { - struct max8903_data *data; + struct max8903_data *charger; struct device *dev = &pdev->dev; - struct max8903_pdata *pdata = pdev->dev.platform_data; struct power_supply_config psy_cfg = {}; int ret = 0; int gpio; int ta_in = 0; int usb_in = 0; - data = devm_kzalloc(dev, sizeof(struct max8903_data), GFP_KERNEL); - if (data == NULL) { + charger = devm_kzalloc(dev, sizeof(struct max8903_data), GFP_KERNEL); + if (charger == NULL) { dev_err(dev, "Cannot allocate memory.\n"); return -ENOMEM; } - memcpy(&data->pdata, pdata, sizeof(struct max8903_pdata)); - data->dev = dev; - platform_set_drvdata(pdev, data); - if (pdata->dc_valid == false && pdata->usb_valid == false) { + charger->pdata = pdev->dev.platform_data; + if (IS_ENABLED(CONFIG_OF) && !charger->pdata && dev->of_node) { + charger->pdata = max8903_parse_dt_data(dev); + if (!charger->pdata) + return -EINVAL; + } + + charger->dev = dev; + + platform_set_drvdata(pdev, charger); + + charger->fault = false; + charger->ta_in = ta_in; + charger->usb_in = usb_in; + + charger->psy_desc.name = "max8903_charger"; + charger->psy_desc.type = (ta_in) ? POWER_SUPPLY_TYPE_MAINS : + ((usb_in) ? POWER_SUPPLY_TYPE_USB : + POWER_SUPPLY_TYPE_BATTERY); + charger->psy_desc.get_property = max8903_get_property; + charger->psy_desc.properties = max8903_charger_props; + charger->psy_desc.num_properties = ARRAY_SIZE(max8903_charger_props); + + if (charger->pdata->dc_valid == false && charger->pdata->usb_valid == false) { dev_err(dev, "No valid power sources.\n"); return -EINVAL; } - if (pdata->dc_valid) { - if (pdata->dok && gpio_is_valid(pdata->dok) && - pdata->dcm && gpio_is_valid(pdata->dcm)) { + if (charger->pdata->dc_valid) { + if (charger->pdata->dok && gpio_is_valid(charger->pdata->dok) && + charger->pdata->dcm && gpio_is_valid(charger->pdata->dcm)) { + ret = devm_gpio_request(dev, + charger->pdata->dok, + charger->psy_desc.name); + if (ret) { + dev_err(dev, + "Failed GPIO request for dok: %d err %d\n", + charger->pdata->dok, ret); + return -EINVAL; + } + + ret = devm_gpio_request(dev, + charger->pdata->dcm, + charger->psy_desc.name); + if (ret) { + dev_err(dev, + "Failed GPIO request for dcm: %d err %d\n", + charger->pdata->dcm, ret); + return -EINVAL; + } + gpio = pdata->dok; /* PULL_UPed Interrupt */ ta_in = gpio_get_value(gpio) ? 0 : 1; @@ -219,18 +324,38 @@ static int max8903_probe(struct platform_device *pdev) } } else { if (pdata->dcm) { - if (gpio_is_valid(pdata->dcm)) + if (gpio_is_valid(pdata->dcm)) { + ret = devm_gpio_request(dev, + charger->pdata->dcm, + charger->psy_desc.name); + if (ret) { + dev_err(dev, + "Failed GPIO request for dcm: %d err %d\n", + charger->pdata->dcm, ret); + return -EINVAL; + } + gpio_set_value(pdata->dcm, 0); - else { + } else { dev_err(dev, "Invalid pin: dcm.\n"); return -EINVAL; } } } - if (pdata->usb_valid) { - if (pdata->uok && gpio_is_valid(pdata->uok)) { - gpio = pdata->uok; + if (charger->pdata->usb_valid) { + if (gpio_is_valid(charger->pdata->uok)) { + ret = devm_gpio_request(dev, + charger->pdata->uok, + charger->psy_desc.name); + if (ret) { + dev_err(dev, + "Failed GPIO request for uok: %d err %d\n", + charger->pdata->uok, ret); + return -EINVAL; + } + + gpio = charger->pdata->uok; usb_in = gpio_get_value(gpio) ? 0 : 1; } else { dev_err(dev, "When USB is wired, UOK should be wired." @@ -239,91 +364,122 @@ static int max8903_probe(struct platform_device *pdev) } } - if (pdata->cen) { - if (gpio_is_valid(pdata->cen)) { - gpio_set_value(pdata->cen, (ta_in || usb_in) ? 0 : 1); + if (charger->pdata->cen) { + if (gpio_is_valid(charger->pdata->cen)) { + ret = devm_gpio_request(dev, + charger->pdata->cen, + charger->psy_desc.name); + if (ret) { + dev_err(dev, + "Failed GPIO request for cen: %d err %d\n", + charger->pdata->cen, ret); + return -EINVAL; + } + + gpio_set_value(charger->pdata->cen, (ta_in || usb_in) ? 0 : 1); } else { dev_err(dev, "Invalid pin: cen.\n"); return -EINVAL; } } - if (pdata->chg) { - if (!gpio_is_valid(pdata->chg)) { + if (charger->pdata->chg) { + if (gpio_is_valid(charger->pdata->chg)) { + ret = devm_gpio_request(dev, + charger->pdata->chg, + charger->psy_desc.name); + if (ret) { + dev_err(dev, + "Failed GPIO request for chg: %d err %d\n", + charger->pdata->chg, ret); + return -EINVAL; + } + } else { dev_err(dev, "Invalid pin: chg.\n"); return -EINVAL; } } - if (pdata->flt) { - if (!gpio_is_valid(pdata->flt)) { + if (charger->pdata->flt) { + if (gpio_is_valid(charger->pdata->flt)) { + ret = devm_gpio_request(dev, + charger->pdata->flt, + charger->psy_desc.name); + if (ret) { + dev_err(dev, + "Failed GPIO request for flt: %d err %d\n", + charger->pdata->flt, ret); + return -EINVAL; + } + } else { dev_err(dev, "Invalid pin: flt.\n"); return -EINVAL; } } - if (pdata->usus) { - if (!gpio_is_valid(pdata->usus)) { + if (charger->pdata->usus) { + if (gpio_is_valid(charger->pdata->usus)) { + ret = devm_gpio_request(dev, + charger->pdata->usus, + charger->psy_desc.name); + if (ret) { + dev_err(dev, + "Failed GPIO request for usus: %d err %d\n", + charger->pdata->usus, ret); + return -EINVAL; + } + } else { dev_err(dev, "Invalid pin: usus.\n"); return -EINVAL; } } - data->fault = false; - data->ta_in = ta_in; - data->usb_in = usb_in; - - data->psy_desc.name = "max8903_charger"; - data->psy_desc.type = (ta_in) ? POWER_SUPPLY_TYPE_MAINS : - ((usb_in) ? POWER_SUPPLY_TYPE_USB : - POWER_SUPPLY_TYPE_BATTERY); - data->psy_desc.get_property = max8903_get_property; - data->psy_desc.properties = max8903_charger_props; - data->psy_desc.num_properties = ARRAY_SIZE(max8903_charger_props); - - psy_cfg.drv_data = data; + psy_cfg.supplied_to = NULL; + psy_cfg.num_supplicants = 0; + psy_cfg.of_node = dev->of_node; + psy_cfg.drv_data = charger; - data->psy = devm_power_supply_register(dev, &data->psy_desc, &psy_cfg); - if (IS_ERR(data->psy)) { + charger->psy = devm_power_supply_register(dev, &charger->psy_desc, &psy_cfg); + if (IS_ERR(charger->psy)) { dev_err(dev, "failed: power supply register.\n"); - return PTR_ERR(data->psy); + return PTR_ERR(charger->psy); } - if (pdata->dc_valid) { - ret = devm_request_threaded_irq(dev, gpio_to_irq(pdata->dok), + if (charger->pdata->dc_valid) { + ret = devm_request_threaded_irq(dev, gpio_to_irq(charger->pdata->dok), NULL, max8903_dcin, IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING | IRQF_ONESHOT, - "MAX8903 DC IN", data); + "MAX8903 DC IN", charger); if (ret) { dev_err(dev, "Cannot request irq %d for DC (%d)\n", - gpio_to_irq(pdata->dok), ret); + gpio_to_irq(charger->pdata->dok), ret); return ret; } } - if (pdata->usb_valid) { - ret = devm_request_threaded_irq(dev, gpio_to_irq(pdata->uok), + if (charger->pdata->usb_valid) { + ret = devm_request_threaded_irq(dev, gpio_to_irq(charger->pdata->uok), NULL, max8903_usbin, IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING | IRQF_ONESHOT, - "MAX8903 USB IN", data); + "MAX8903 USB IN", charger); if (ret) { dev_err(dev, "Cannot request irq %d for USB (%d)\n", - gpio_to_irq(pdata->uok), ret); + gpio_to_irq(charger->pdata->uok), ret); return ret; } } - if (pdata->flt) { - ret = devm_request_threaded_irq(dev, gpio_to_irq(pdata->flt), + if (charger->pdata->flt) { + ret = devm_request_threaded_irq(dev, gpio_to_irq(charger->pdata->flt), NULL, max8903_fault, IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING | IRQF_ONESHOT, - "MAX8903 Fault", data); + "MAX8903 Fault", charger); if (ret) { dev_err(dev, "Cannot request irq %d for Fault (%d)\n", - gpio_to_irq(pdata->flt), ret); + gpio_to_irq(charger->pdata->flt), ret); return ret; } } @@ -331,10 +487,17 @@ static int max8903_probe(struct platform_device *pdev) return 0; } +static const struct of_device_id max8903_match_ids[] = { + { .compatible = "max8903-charger", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, max8903_match_ids); + static struct platform_driver max8903_driver = { .probe = max8903_probe, .driver = { .name = "max8903-charger", + .of_match_table = max8903_match_ids }, }; -- 1.9.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] max8903: adds support for initiation via device tree. 2016-06-02 6:44 ` [PATCH 1/2] max8903: adds support for initiation via device tree chris @ 2016-06-09 10:35 ` Krzysztof Kozlowski 2016-06-10 5:35 ` Chris Lapa 0 siblings, 1 reply; 8+ messages in thread From: Krzysztof Kozlowski @ 2016-06-09 10:35 UTC (permalink / raw) To: chris; +Cc: linux-kernel, linux-pm Hi, Thanks for your contribution. Few comments below: On Thu, Jun 2, 2016 at 8:44 AM, <chris@lapa.com.au> wrote: > From: Chris Lapa <chris@lapa.com.au> > > This commit also adds requesting gpio's via devm_gpio_request() to ensure > the gpio is available for usage by the driver. > > Signed-off-by: Chris Lapa <chris@lapa.com.au> > --- > .../devicetree/bindings/power/max8903-charger.txt | 28 ++ > drivers/power/max8903_charger.c | 281 ++++++++++++++++----- > 2 files changed, 250 insertions(+), 59 deletions(-) > create mode 100644 Documentation/devicetree/bindings/power/max8903-charger.txt Please put the bindings documentation in separate, first patch. > > diff --git a/Documentation/devicetree/bindings/power/max8903-charger.txt b/Documentation/devicetree/bindings/power/max8903-charger.txt > new file mode 100644 > index 0000000..7207731 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/max8903-charger.txt > @@ -0,0 +1,28 @@ > +Maxim Semiconductor MAX8903 Battery Charger bindings > + > +Required properties: > +- compatible: "max8903-charger" for MAX8903 Battery Charger Needs a 'maxim,' prefix. > +- dc_valid: > + - dok: DC power OK pin > +- usb_valid: > + - uok: USB power OK pin I don't understand the explanation of them - dok/uok. What do you want to say here? > + > +Optional properties: > +- cen: Charge enable pin > +- chg: Charger status pin > +- flt: Fault pin > +- dcm: Current limit mode setting (DC or USB) > +- usus: USB suspend pin Each gpio should be suffixed with '-gpios' (see Documentation/devicetree/bindings/gpio/gpio.txt). > + > + > +Example: > + > + max8903-charger { > + compatible = "max8903-charger"; > + dok = <&gpio2 3 GPIO_ACTIVE_LOW>; > + flt = <&gpio2 2 GPIO_ACTIVE_LOW>; > + chg = <&gpio3 15 GPIO_ACTIVE_LOW>; > + cen = <&gpio2 5 GPIO_ACTIVE_LOW>; > + dc_valid; > + status = "okay"; > + }; > diff --git a/drivers/power/max8903_charger.c b/drivers/power/max8903_charger.c > index 17876ca..1989c10 100644 > --- a/drivers/power/max8903_charger.c > +++ b/drivers/power/max8903_charger.c > @@ -23,13 +23,16 @@ > #include <linux/gpio.h> > #include <linux/interrupt.h> > #include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/of_gpio.h> > #include <linux/slab.h> > #include <linux/power_supply.h> > #include <linux/platform_device.h> > #include <linux/power/max8903_charger.h> > > struct max8903_data { > - struct max8903_pdata pdata; > + struct max8903_pdata *pdata; > struct device *dev; > struct power_supply *psy; > struct power_supply_desc psy_desc; > @@ -53,8 +56,8 @@ static int max8903_get_property(struct power_supply *psy, > switch (psp) { > case POWER_SUPPLY_PROP_STATUS: > val->intval = POWER_SUPPLY_STATUS_UNKNOWN; > - if (data->pdata.chg) { > - if (gpio_get_value(data->pdata.chg) == 0) > + if (data->pdata->chg) { > + if (gpio_get_value(data->pdata->chg) == 0) > val->intval = POWER_SUPPLY_STATUS_CHARGING; > else if (data->usb_in || data->ta_in) > val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > @@ -75,13 +78,14 @@ static int max8903_get_property(struct power_supply *psy, > default: > return -EINVAL; > } > + > return 0; > } > > static irqreturn_t max8903_dcin(int irq, void *_data) > { > struct max8903_data *data = _data; > - struct max8903_pdata *pdata = &data->pdata; > + struct max8903_pdata *pdata = data->pdata; > bool ta_in; > enum power_supply_type old_type; > > @@ -122,7 +126,7 @@ static irqreturn_t max8903_dcin(int irq, void *_data) > static irqreturn_t max8903_usbin(int irq, void *_data) > { > struct max8903_data *data = _data; > - struct max8903_pdata *pdata = &data->pdata; > + struct max8903_pdata *pdata = data->pdata; > bool usb_in; > enum power_supply_type old_type; > > @@ -161,7 +165,7 @@ static irqreturn_t max8903_usbin(int irq, void *_data) > static irqreturn_t max8903_fault(int irq, void *_data) > { > struct max8903_data *data = _data; > - struct max8903_pdata *pdata = &data->pdata; > + struct max8903_pdata *pdata = data->pdata; > bool fault; > > fault = gpio_get_value(pdata->flt) ? false : true; > @@ -179,34 +183,135 @@ static irqreturn_t max8903_fault(int irq, void *_data) > return IRQ_HANDLED; > } > > +static struct max8903_pdata *max8903_parse_dt_data( > + struct device *dev) > +{ > + struct device_node *of_node = dev->of_node; > + struct max8903_pdata *pdata = NULL; > + > + if (!of_node) { > + return pdata; > + } Run a scripts/checkpatch.pl. In general the {} are not needed for single statements. > + > + pdata = devm_kzalloc(dev, sizeof(struct max8903_pdata), > + GFP_KERNEL); > + if (!pdata) { > + return pdata; ditto > + } > + > + if (of_get_property(of_node, "dc_valid", NULL)) { > + pdata->dc_valid = true; > + } > + > + if (of_get_property(of_node, "usb_valid", NULL)) { > + pdata->usb_valid = true; > + } > + > + pdata->cen = of_get_named_gpio(of_node, "cen", 0); > + if (!gpio_is_valid(pdata->cen)) { > + pdata->cen = 0; > + } > + > + pdata->chg = of_get_named_gpio(of_node, "chg", 0); > + if (!gpio_is_valid(pdata->chg)) { > + pdata->chg = 0; > + } > + > + pdata->flt = of_get_named_gpio(of_node, "flt", 0); > + if (!gpio_is_valid(pdata->flt)) { > + pdata->flt = 0; > + } > + > + pdata->usus = of_get_named_gpio(of_node, "usus", 0); > + if (!gpio_is_valid(pdata->usus)) { > + pdata->usus = 0; > + } > + > + pdata->dcm = of_get_named_gpio(of_node, "dcm", 0); > + if (!gpio_is_valid(pdata->dcm)) { > + pdata->dcm = 0; > + } > + > + pdata->dok = of_get_named_gpio(of_node, "dok", 0); > + if (!gpio_is_valid(pdata->dok)) { > + pdata->dok = 0; > + } > + > + pdata->uok = of_get_named_gpio(of_node, "uok", 0); > + if (!gpio_is_valid(pdata->uok)) { > + pdata->uok = 0; > + } > + > + return pdata; > +} > + > static int max8903_probe(struct platform_device *pdev) > { > - struct max8903_data *data; > + struct max8903_data *charger; > struct device *dev = &pdev->dev; > - struct max8903_pdata *pdata = pdev->dev.platform_data; > struct power_supply_config psy_cfg = {}; > int ret = 0; > int gpio; > int ta_in = 0; > int usb_in = 0; > > - data = devm_kzalloc(dev, sizeof(struct max8903_data), GFP_KERNEL); > - if (data == NULL) { > + charger = devm_kzalloc(dev, sizeof(struct max8903_data), GFP_KERNEL); > + if (charger == NULL) { > dev_err(dev, "Cannot allocate memory.\n"); In a separate patch - you can get rid of error message. I didn't perform a thorough review. First fix some easy ones. :) Best regards, Krzysztof ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] max8903: adds support for initiation via device tree. 2016-06-09 10:35 ` Krzysztof Kozlowski @ 2016-06-10 5:35 ` Chris Lapa 2016-06-10 6:13 ` Krzysztof Kozlowski 0 siblings, 1 reply; 8+ messages in thread From: Chris Lapa @ 2016-06-10 5:35 UTC (permalink / raw) To: Krzysztof Kozlowski; +Cc: linux-kernel, linux-pm Hi Krzysztof, Thanks for the review. I'm working on those changes now. However just so I know for the future. Why no error checking on devm_kzalloc() result? Looking through the source for devm_kzalloc() it looks like NULL isn't caught anywhere else. Thanks, Chris On 9/06/2016 8:35 PM, Krzysztof Kozlowski wrote: > Hi, > > Thanks for your contribution. Few comments below: > > On Thu, Jun 2, 2016 at 8:44 AM, <chris@lapa.com.au> wrote: >> From: Chris Lapa <chris@lapa.com.au> >> >> This commit also adds requesting gpio's via devm_gpio_request() to ensure >> the gpio is available for usage by the driver. >> >> Signed-off-by: Chris Lapa <chris@lapa.com.au> >> --- >> .../devicetree/bindings/power/max8903-charger.txt | 28 ++ >> drivers/power/max8903_charger.c | 281 ++++++++++++++++----- >> 2 files changed, 250 insertions(+), 59 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/power/max8903-charger.txt > > Please put the bindings documentation in separate, first patch. > >> >> diff --git a/Documentation/devicetree/bindings/power/max8903-charger.txt b/Documentation/devicetree/bindings/power/max8903-charger.txt >> new file mode 100644 >> index 0000000..7207731 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/power/max8903-charger.txt >> @@ -0,0 +1,28 @@ >> +Maxim Semiconductor MAX8903 Battery Charger bindings >> + >> +Required properties: >> +- compatible: "max8903-charger" for MAX8903 Battery Charger > > Needs a 'maxim,' prefix. > >> +- dc_valid: >> + - dok: DC power OK pin >> +- usb_valid: >> + - uok: USB power OK pin > > I don't understand the explanation of them - dok/uok. What do you want > to say here? > >> + >> +Optional properties: >> +- cen: Charge enable pin >> +- chg: Charger status pin >> +- flt: Fault pin >> +- dcm: Current limit mode setting (DC or USB) >> +- usus: USB suspend pin > > Each gpio should be suffixed with '-gpios' (see > Documentation/devicetree/bindings/gpio/gpio.txt). > >> + >> + >> +Example: >> + >> + max8903-charger { >> + compatible = "max8903-charger"; >> + dok = <&gpio2 3 GPIO_ACTIVE_LOW>; >> + flt = <&gpio2 2 GPIO_ACTIVE_LOW>; >> + chg = <&gpio3 15 GPIO_ACTIVE_LOW>; >> + cen = <&gpio2 5 GPIO_ACTIVE_LOW>; >> + dc_valid; >> + status = "okay"; >> + }; >> diff --git a/drivers/power/max8903_charger.c b/drivers/power/max8903_charger.c >> index 17876ca..1989c10 100644 >> --- a/drivers/power/max8903_charger.c >> +++ b/drivers/power/max8903_charger.c >> @@ -23,13 +23,16 @@ >> #include <linux/gpio.h> >> #include <linux/interrupt.h> >> #include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> >> +#include <linux/of_gpio.h> >> #include <linux/slab.h> >> #include <linux/power_supply.h> >> #include <linux/platform_device.h> >> #include <linux/power/max8903_charger.h> >> >> struct max8903_data { >> - struct max8903_pdata pdata; >> + struct max8903_pdata *pdata; >> struct device *dev; >> struct power_supply *psy; >> struct power_supply_desc psy_desc; >> @@ -53,8 +56,8 @@ static int max8903_get_property(struct power_supply *psy, >> switch (psp) { >> case POWER_SUPPLY_PROP_STATUS: >> val->intval = POWER_SUPPLY_STATUS_UNKNOWN; >> - if (data->pdata.chg) { >> - if (gpio_get_value(data->pdata.chg) == 0) >> + if (data->pdata->chg) { >> + if (gpio_get_value(data->pdata->chg) == 0) >> val->intval = POWER_SUPPLY_STATUS_CHARGING; >> else if (data->usb_in || data->ta_in) >> val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; >> @@ -75,13 +78,14 @@ static int max8903_get_property(struct power_supply *psy, >> default: >> return -EINVAL; >> } >> + >> return 0; >> } >> >> static irqreturn_t max8903_dcin(int irq, void *_data) >> { >> struct max8903_data *data = _data; >> - struct max8903_pdata *pdata = &data->pdata; >> + struct max8903_pdata *pdata = data->pdata; >> bool ta_in; >> enum power_supply_type old_type; >> >> @@ -122,7 +126,7 @@ static irqreturn_t max8903_dcin(int irq, void *_data) >> static irqreturn_t max8903_usbin(int irq, void *_data) >> { >> struct max8903_data *data = _data; >> - struct max8903_pdata *pdata = &data->pdata; >> + struct max8903_pdata *pdata = data->pdata; >> bool usb_in; >> enum power_supply_type old_type; >> >> @@ -161,7 +165,7 @@ static irqreturn_t max8903_usbin(int irq, void *_data) >> static irqreturn_t max8903_fault(int irq, void *_data) >> { >> struct max8903_data *data = _data; >> - struct max8903_pdata *pdata = &data->pdata; >> + struct max8903_pdata *pdata = data->pdata; >> bool fault; >> >> fault = gpio_get_value(pdata->flt) ? false : true; >> @@ -179,34 +183,135 @@ static irqreturn_t max8903_fault(int irq, void *_data) >> return IRQ_HANDLED; >> } >> >> +static struct max8903_pdata *max8903_parse_dt_data( >> + struct device *dev) >> +{ >> + struct device_node *of_node = dev->of_node; >> + struct max8903_pdata *pdata = NULL; >> + >> + if (!of_node) { >> + return pdata; >> + } > > Run a scripts/checkpatch.pl. In general the {} are not needed for > single statements. > >> + >> + pdata = devm_kzalloc(dev, sizeof(struct max8903_pdata), >> + GFP_KERNEL); >> + if (!pdata) { >> + return pdata; > > ditto > >> + } >> + >> + if (of_get_property(of_node, "dc_valid", NULL)) { >> + pdata->dc_valid = true; >> + } >> + >> + if (of_get_property(of_node, "usb_valid", NULL)) { >> + pdata->usb_valid = true; >> + } >> + >> + pdata->cen = of_get_named_gpio(of_node, "cen", 0); >> + if (!gpio_is_valid(pdata->cen)) { >> + pdata->cen = 0; >> + } >> + >> + pdata->chg = of_get_named_gpio(of_node, "chg", 0); >> + if (!gpio_is_valid(pdata->chg)) { >> + pdata->chg = 0; >> + } >> + >> + pdata->flt = of_get_named_gpio(of_node, "flt", 0); >> + if (!gpio_is_valid(pdata->flt)) { >> + pdata->flt = 0; >> + } >> + >> + pdata->usus = of_get_named_gpio(of_node, "usus", 0); >> + if (!gpio_is_valid(pdata->usus)) { >> + pdata->usus = 0; >> + } >> + >> + pdata->dcm = of_get_named_gpio(of_node, "dcm", 0); >> + if (!gpio_is_valid(pdata->dcm)) { >> + pdata->dcm = 0; >> + } >> + >> + pdata->dok = of_get_named_gpio(of_node, "dok", 0); >> + if (!gpio_is_valid(pdata->dok)) { >> + pdata->dok = 0; >> + } >> + >> + pdata->uok = of_get_named_gpio(of_node, "uok", 0); >> + if (!gpio_is_valid(pdata->uok)) { >> + pdata->uok = 0; >> + } >> + >> + return pdata; >> +} >> + >> static int max8903_probe(struct platform_device *pdev) >> { >> - struct max8903_data *data; >> + struct max8903_data *charger; >> struct device *dev = &pdev->dev; >> - struct max8903_pdata *pdata = pdev->dev.platform_data; >> struct power_supply_config psy_cfg = {}; >> int ret = 0; >> int gpio; >> int ta_in = 0; >> int usb_in = 0; >> >> - data = devm_kzalloc(dev, sizeof(struct max8903_data), GFP_KERNEL); >> - if (data == NULL) { >> + charger = devm_kzalloc(dev, sizeof(struct max8903_data), GFP_KERNEL); >> + if (charger == NULL) { >> dev_err(dev, "Cannot allocate memory.\n"); > > In a separate patch - you can get rid of error message. > > I didn't perform a thorough review. First fix some easy ones. :) > > Best regards, > Krzysztof > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] max8903: adds support for initiation via device tree. 2016-06-10 5:35 ` Chris Lapa @ 2016-06-10 6:13 ` Krzysztof Kozlowski 2016-06-10 10:23 ` Krzysztof Kozlowski 0 siblings, 1 reply; 8+ messages in thread From: Krzysztof Kozlowski @ 2016-06-10 6:13 UTC (permalink / raw) To: chris; +Cc: linux-kernel, linux-pm On 06/10/2016 07:35 AM, Chris Lapa wrote: > Hi Krzysztof, > > Thanks for the review. I'm working on those changes now. > > However just so I know for the future. Why no error checking on > devm_kzalloc() result? Looking through the source for devm_kzalloc() it > looks like NULL isn't caught anywhere else. Error checking is necessary. Just do not print the error message. The kernel core will print one with full back trace. if (charger == NULL) return -ENOMEM; Best regards, Krzysztof ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] max8903: adds support for initiation via device tree. 2016-06-10 6:13 ` Krzysztof Kozlowski @ 2016-06-10 10:23 ` Krzysztof Kozlowski 0 siblings, 0 replies; 8+ messages in thread From: Krzysztof Kozlowski @ 2016-06-10 10:23 UTC (permalink / raw) To: chris; +Cc: linux-kernel, linux-pm On 06/10/2016 08:13 AM, Krzysztof Kozlowski wrote: > On 06/10/2016 07:35 AM, Chris Lapa wrote: >> Hi Krzysztof, >> >> Thanks for the review. I'm working on those changes now. >> >> However just so I know for the future. Why no error checking on >> devm_kzalloc() result? Looking through the source for devm_kzalloc() it >> looks like NULL isn't caught anywhere else. > > Error checking is necessary. Just do not print the error message. The > kernel core will print one with full back trace. > > if (charger == NULL) > return -ENOMEM; ... and while at it just convert it to simpler: if (!charger) return -ENOMEM; BR, Krzysztof ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-06-10 10:23 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-06-07 4:02 [PATCH 0/2] max8903: Add device tree support and logic fixup chris 2016-06-07 4:02 ` [PATCH 1/2] max8903: adds support for initiation via device tree chris 2016-06-07 4:02 ` [PATCH 2/2] max8903: cleans up confusing relationship between dc_valid, dok and dcm chris -- strict thread matches above, loose matches on Subject: below -- 2016-06-02 6:44 [PATCH 0/2] max8903: Add device tree support and logic fixup chris 2016-06-02 6:44 ` [PATCH 1/2] max8903: adds support for initiation via device tree chris 2016-06-09 10:35 ` Krzysztof Kozlowski 2016-06-10 5:35 ` Chris Lapa 2016-06-10 6:13 ` Krzysztof Kozlowski 2016-06-10 10:23 ` Krzysztof Kozlowski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).