From: Sebastian Reichel <sre@kernel.org>
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Linus Walleij <linus.walleij@linaro.org>,
Dmitry Torokhov <dmitry.torokhov@gmail.com>,
Jacek Anaszewski <jacek.anaszewski@gmail.com>,
Pavel Machek <pavel@ucw.cz>, Lee Jones <lee.jones@linaro.org>,
Liam Girdwood <lgirdwood@gmail.com>,
Mark Brown <broonie@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, linux-input@vger.kernel.org,
linux-leds@vger.kernel.org, linux-pm@vger.kernel.org,
Bartosz Golaszewski <bgolaszewski@baylibre.com>
Subject: Re: [PATCH 09/13] power: supply: max77650: add support for battery charger
Date: Wed, 23 Jan 2019 19:27:38 +0100 [thread overview]
Message-ID: <20190123182738.wtskerasrzmrufrn@earth.universe> (raw)
In-Reply-To: <20190118134244.22253-10-brgl@bgdev.pl>
[-- Attachment #1: Type: text/plain, Size: 13205 bytes --]
Hi Bartosz,
Looks mostly ok, I have a few comments inline towards the end.
On Fri, Jan 18, 2019 at 02:42:40PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Add basic support for the battery charger for max77650 PMIC.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> drivers/power/supply/Kconfig | 7 +
> drivers/power/supply/Makefile | 1 +
> drivers/power/supply/max77650-charger.c | 347 ++++++++++++++++++++++++
> 3 files changed, 355 insertions(+)
> create mode 100644 drivers/power/supply/max77650-charger.c
>
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index e901b9879e7e..0230c96fa94d 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -499,6 +499,13 @@ config CHARGER_DETECTOR_MAX14656
> Revision 1.2 and can be found e.g. in Kindle 4/5th generation
> readers and certain LG devices.
>
> +config CHARGER_MAX77650
> + tristate "Maxim MAX77650 battery charger driver"
> + depends on MFD_MAX77650
> + help
> + Say Y to enable support for the battery charger control of MAX77650
> + PMICs.
> +
> config CHARGER_MAX77693
> tristate "Maxim MAX77693 battery charger driver"
> depends on MFD_MAX77693
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index b731c2a9b695..b73eb8c5c1a9 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_CHARGER_MANAGER) += charger-manager.o
> obj-$(CONFIG_CHARGER_LTC3651) += ltc3651-charger.o
> obj-$(CONFIG_CHARGER_MAX14577) += max14577_charger.o
> obj-$(CONFIG_CHARGER_DETECTOR_MAX14656) += max14656_charger_detector.o
> +obj-$(CONFIG_CHARGER_MAX77650) += max77650-charger.o
> obj-$(CONFIG_CHARGER_MAX77693) += max77693_charger.o
> obj-$(CONFIG_CHARGER_MAX8997) += max8997_charger.o
> obj-$(CONFIG_CHARGER_MAX8998) += max8998_charger.o
> diff --git a/drivers/power/supply/max77650-charger.c b/drivers/power/supply/max77650-charger.c
> new file mode 100644
> index 000000000000..fb9d8f933174
> --- /dev/null
> +++ b/drivers/power/supply/max77650-charger.c
> @@ -0,0 +1,347 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 BayLibre SAS
> + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> + *
> + * Battery charger driver for MAXIM 77650/77651 charger/power-supply.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/max77650.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/regmap.h>
> +
> +#define MAX77650_CHARGER_ENABLED BIT(0)
> +#define MAX77650_CHARGER_DISABLED 0x00
> +#define MAX77650_CHARGER_CHG_EN_MASK BIT(0)
> +
> +#define MAX77650_CHARGER_CHG_DTLS_MASK GENMASK(7, 4)
> +#define MAX77650_CHARGER_CHG_DTLS_BITS(_reg) \
> + (((_reg) & MAX77650_CHARGER_CHG_DTLS_MASK) >> 4)
> +
> +#define MAX77650_CHARGER_CHG_OFF 0x00
> +#define MAX77650_CHARGER_CHG_PREQ 0x01
> +#define MAX77650_CHARGER_CHG_ON_CURR 0x02
> +#define MAX77650_CHARGER_CHG_ON_JCURR 0x03
> +#define MAX77650_CHARGER_CHG_ON_VOLT 0x04
> +#define MAX77650_CHARGER_CHG_ON_JVOLT 0x05
> +#define MAX77650_CHARGER_CHG_ON_TOPOFF 0x06
> +#define MAX77650_CHARGER_CHG_ON_JTOPOFF 0x07
> +#define MAX77650_CHARGER_CHG_DONE 0x08
> +#define MAX77650_CHARGER_CHG_JDONE 0x09
> +#define MAX77650_CHARGER_CHG_SUSP_PF 0x0a
> +#define MAX77650_CHARGER_CHG_SUSP_FCF 0x0b
> +#define MAX77650_CHARGER_CHG_SUSP_BTF 0x0c
> +
> +#define MAX77650_CHARGER_CHGIN_DTLS_MASK GENMASK(3, 2)
> +#define MAX77650_CHARGER_CHGIN_DTLS_BITS(_reg) \
> + (((_reg) & MAX77650_CHARGER_CHGIN_DTLS_MASK) >> 2)
> +
> +#define MAX77650_CHARGER_CHGIN_UVL 0x00
> +#define MAX77650_CHARGER_CHGIN_OVL 0x01
> +#define MAX77650_CHARGER_CHGIN_OKAY 0x11
> +
> +#define MAX77650_CHARGER_CHG_MASK BIT(1)
> +#define MAX77650_CHARGER_CHG_CHARGING(_reg) \
> + (((_reg) & MAX77650_CHARGER_CHG_MASK) > 1)
> +
> +#define MAX77650_CHARGER_VCHGIN_MIN_MASK 0xc0
> +#define MAX77650_CHARGER_VCHGIN_MIN_SHIFT(_val) ((_val) << 5)
> +
> +#define MAX77650_CHARGER_ICHGIN_LIM_MASK 0x1c
> +#define MAX77650_CHARGER_ICHGIN_LIM_SHIFT(_val) ((_val) << 2)
> +
> +struct max77650_charger_data {
> + struct regmap *map;
> + struct device *dev;
> +};
> +
> +static enum power_supply_property max77650_charger_properties[] = {
> + POWER_SUPPLY_PROP_STATUS,
> + POWER_SUPPLY_PROP_ONLINE,
> + POWER_SUPPLY_PROP_CHARGE_TYPE
> +};
> +
> +static const unsigned int max77650_charger_vchgin_min_table[] = {
> + 4000000, 4100000, 4200000, 4300000, 4400000, 4500000, 4600000, 4700000
> +};
> +
> +static const unsigned int max77650_charger_ichgin_lim_table[] = {
> + 95000, 190000, 285000, 380000, 475000
> +};
> +
> +static int max77650_charger_set_vchgin_min(struct max77650_charger_data *chg,
> + unsigned int val)
> +{
> + int i, rv;
> +
> + for (i = 0; i < ARRAY_SIZE(max77650_charger_vchgin_min_table); i++) {
> + if (val == max77650_charger_vchgin_min_table[i]) {
> + rv = regmap_update_bits(chg->map,
> + MAX77650_REG_CNFG_CHG_B,
> + MAX77650_CHARGER_VCHGIN_MIN_MASK,
> + MAX77650_CHARGER_VCHGIN_MIN_SHIFT(i));
> + if (rv)
> + return rv;
> +
> + return 0;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int max77650_charger_set_ichgin_lim(struct max77650_charger_data *chg,
> + unsigned int val)
> +{
> + int i, rv;
> +
> + for (i = 0; i < ARRAY_SIZE(max77650_charger_ichgin_lim_table); i++) {
> + if (val == max77650_charger_ichgin_lim_table[i]) {
> + rv = regmap_update_bits(chg->map,
> + MAX77650_REG_CNFG_CHG_B,
> + MAX77650_CHARGER_ICHGIN_LIM_MASK,
> + MAX77650_CHARGER_ICHGIN_LIM_SHIFT(i));
> + if (rv)
> + return rv;
> +
> + return 0;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +static void max77650_charger_enable(struct max77650_charger_data *chg)
> +{
> + int rv;
> +
> + rv = regmap_update_bits(chg->map,
> + MAX77650_REG_CNFG_CHG_B,
> + MAX77650_CHARGER_CHG_EN_MASK,
> + MAX77650_CHARGER_ENABLED);
> + if (rv)
> + dev_err(chg->dev, "unable to enable the charger: %d\n", rv);
> +}
> +
> +static void max77650_charger_disable(struct max77650_charger_data *chg)
> +{
> + int rv;
> +
> + rv = regmap_update_bits(chg->map,
> + MAX77650_REG_CNFG_CHG_B,
> + MAX77650_CHARGER_CHG_EN_MASK,
> + MAX77650_CHARGER_DISABLED);
> + if (rv)
> + dev_err(chg->dev, "unable to disable the charger: %d\n", rv);
> +}
> +
> +static irqreturn_t max77650_charger_check_status(int irq, void *data)
> +{
> + struct max77650_charger_data *chg = data;
> + int rv, reg;
> +
> + rv = regmap_read(chg->map, MAX77650_REG_STAT_CHG_B, ®);
> + if (rv) {
> + dev_err(chg->dev,
> + "unable to read the charger status: %d\n", rv);
> + return IRQ_HANDLED;
> + }
> +
> + switch (MAX77650_CHARGER_CHGIN_DTLS_BITS(reg)) {
> + case MAX77650_CHARGER_CHGIN_UVL:
> + dev_err(chg->dev, "undervoltage lockout detected, disabling charger\n");
> + max77650_charger_disable(chg);
> + break;
> + case MAX77650_CHARGER_CHGIN_OVL:
> + dev_err(chg->dev, "overvoltage lockout detected, disabling charger\n");
> + max77650_charger_disable(chg);
> + break;
> + case MAX77650_CHARGER_CHGIN_OKAY:
> + max77650_charger_enable(chg);
> + break;
> + default:
> + /* May be 0x10 - debouncing */
> + break;
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int max77650_charger_get_property(struct power_supply *psy,
> + enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + struct max77650_charger_data *chg = power_supply_get_drvdata(psy);
> + int rv, reg;
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_STATUS:
> + rv = regmap_read(chg->map, MAX77650_REG_STAT_CHG_B, ®);
> + if (rv)
> + return rv;
> +
> + if (MAX77650_CHARGER_CHG_CHARGING(reg)) {
> + val->intval = POWER_SUPPLY_STATUS_CHARGING;
> + break;
> + }
> +
> + switch (MAX77650_CHARGER_CHG_DTLS_BITS(reg)) {
> + case MAX77650_CHARGER_CHG_OFF:
> + case MAX77650_CHARGER_CHG_SUSP_PF:
> + case MAX77650_CHARGER_CHG_SUSP_FCF:
> + case MAX77650_CHARGER_CHG_SUSP_BTF:
> + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> + break;
> + case MAX77650_CHARGER_CHG_PREQ:
> + case MAX77650_CHARGER_CHG_ON_CURR:
> + case MAX77650_CHARGER_CHG_ON_JCURR:
> + case MAX77650_CHARGER_CHG_ON_VOLT:
> + case MAX77650_CHARGER_CHG_ON_JVOLT:
> + case MAX77650_CHARGER_CHG_ON_TOPOFF:
> + case MAX77650_CHARGER_CHG_ON_JTOPOFF:
> + val->intval = POWER_SUPPLY_STATUS_CHARGING;
> + break;
> + case MAX77650_CHARGER_CHG_DONE:
> + val->intval = POWER_SUPPLY_STATUS_FULL;
> + break;
> + default:
> + val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
> + }
> + break;
> + case POWER_SUPPLY_PROP_ONLINE:
> + rv = regmap_read(chg->map, MAX77650_REG_STAT_CHG_B, ®);
> + if (rv)
> + return rv;
> +
> + val->intval = MAX77650_CHARGER_CHG_CHARGING(reg);
> + break;
> + case POWER_SUPPLY_PROP_CHARGE_TYPE:
> + rv = regmap_read(chg->map, MAX77650_REG_STAT_CHG_B, ®);
> + if (rv)
> + return rv;
> +
> + if (!MAX77650_CHARGER_CHG_CHARGING(reg)) {
> + val->intval = POWER_SUPPLY_CHARGE_TYPE_NONE;
> + break;
> + }
> +
> + switch (MAX77650_CHARGER_CHG_DTLS_BITS(reg)) {
> + case MAX77650_CHARGER_CHG_PREQ:
> + case MAX77650_CHARGER_CHG_ON_CURR:
> + case MAX77650_CHARGER_CHG_ON_JCURR:
> + case MAX77650_CHARGER_CHG_ON_VOLT:
> + case MAX77650_CHARGER_CHG_ON_JVOLT:
> + val->intval = POWER_SUPPLY_CHARGE_TYPE_FAST;
> + break;
> + case MAX77650_CHARGER_CHG_ON_TOPOFF:
> + case MAX77650_CHARGER_CHG_ON_JTOPOFF:
> + val->intval = POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
> + break;
> + default:
> + val->intval = POWER_SUPPLY_CHARGE_TYPE_UNKNOWN;
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static const struct power_supply_desc max77650_battery_desc = {
> + .name = "max77650",
> + .type = POWER_SUPPLY_TYPE_BATTERY,
This is a charger, not a battery. BATTERY type is for the fuel
gauges. You should be using POWER_SUPPLY_TYPE_USB for USB based
battery chargers and POWER_SUPPLY_TYPE_MAINS otherwise.
> + .get_property = max77650_charger_get_property,
> + .properties = max77650_charger_properties,
> + .num_properties = ARRAY_SIZE(max77650_charger_properties),
> +};
> +
> +static int max77650_charger_probe(struct platform_device *pdev)
> +{
> + struct regmap_irq_chip_data *irq_data;
> + struct power_supply_config pscfg = {};
> + struct max77650_charger_data *chg;
> + struct power_supply *battery;
> + struct device *dev, *parent;
> + int rv, chg_irq, chgin_irq;
> + struct i2c_client *i2c;
> + unsigned int prop;
> +
> + dev = &pdev->dev;
> + parent = dev->parent;
> + i2c = to_i2c_client(parent);
> + irq_data = i2c_get_clientdata(i2c);
> +
> + chg = devm_kzalloc(dev, sizeof(*chg), GFP_KERNEL);
> + if (!chg)
> + return -ENOMEM;
> +
> + chg->map = dev_get_regmap(parent, NULL);
> + if (!chg->map)
> + return -ENODEV;
> +
> + chg->dev = dev;
> +
> + pscfg.of_node = dev->of_node;
> + pscfg.drv_data = chg;
> +
> + chg_irq = regmap_irq_get_virq(irq_data, MAX77650_INT_CHG);
> + if (chg_irq <= 0)
> + return -EINVAL;
> +
> + chgin_irq = regmap_irq_get_virq(irq_data, MAX77650_INT_CHGIN);
> + if (chgin_irq <= 0)
> + return -EINVAL;
> +
> + rv = devm_request_threaded_irq(dev, chg_irq, NULL,
> + max77650_charger_check_status,
> + IRQF_ONESHOT, "chg", chg);
> + if (rv)
> + return rv;
> +
> + rv = devm_request_threaded_irq(dev, chgin_irq, NULL,
> + max77650_charger_check_status,
> + IRQF_ONESHOT, "chgin", chg);
> + if (rv)
> + return rv;
> +
> + battery = devm_power_supply_register(dev,
> + &max77650_battery_desc, &pscfg);
> + if (IS_ERR(battery))
> + return PTR_ERR(battery);
> +
> + rv = of_property_read_u32(dev->of_node, "maxim,vchgin-min", &prop);
> + if (rv == 0) {
> + rv = max77650_charger_set_vchgin_min(chg, prop);
> + if (rv)
> + return rv;
> + }
> +
> + rv = of_property_read_u32(dev->of_node, "maxim,ichgin-lim", &prop);
> + if (rv == 0) {
> + rv = max77650_charger_set_ichgin_lim(chg, prop);
> + if (rv)
> + return rv;
> + }
> +
> + return regmap_update_bits(chg->map,
> + MAX77650_REG_CNFG_CHG_B,
> + MAX77650_CHARGER_CHG_EN_MASK,
> + MAX77650_CHARGER_ENABLED);
should this be disabled on module remove?
> +}
> +
> +static struct platform_driver max77650_charger_driver = {
> + .driver = {
> + .name = "max77650-charger",
> + },
> + .probe = max77650_charger_probe,
> +};
> +module_platform_driver(max77650_charger_driver);
> +
> +MODULE_DESCRIPTION("MAXIM 77650/77651 charger driver");
> +MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
> +MODULE_LICENSE("GPL");
This is GPL2 or later, but "SPDX-License-Identifier: GPL-2.0" is not.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2019-01-24 9:58 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-18 13:42 [PATCH 00/13] mfd: add support for max77650 PMIC Bartosz Golaszewski
2019-01-18 13:42 ` [PATCH 01/13] dt-bindings: mfd: add DT bindings for max77650 Bartosz Golaszewski
2019-01-18 13:42 ` [PATCH 02/13] dt-bindings: regulator: " Bartosz Golaszewski
2019-01-18 13:42 ` [PATCH 03/13] dt-bindings: power: supply: " Bartosz Golaszewski
2019-01-18 13:42 ` [PATCH 04/13] dt-bindings: gpio: " Bartosz Golaszewski
2019-01-21 14:04 ` Linus Walleij
2019-01-18 13:42 ` [PATCH 05/13] dt-bindings: leds: " Bartosz Golaszewski
2019-01-20 16:28 ` Jacek Anaszewski
2019-01-18 13:42 ` [PATCH 06/13] dt-bindings: input: " Bartosz Golaszewski
2019-01-18 13:42 ` [PATCH 07/13] mfd: max77650: new core mfd driver Bartosz Golaszewski
2019-01-18 13:42 ` [PATCH 08/13] regulator: max77650: add regulator support Bartosz Golaszewski
2019-01-18 18:01 ` Mark Brown
2019-01-18 18:13 ` Bartosz Golaszewski
2019-01-18 18:36 ` Mark Brown
2019-01-18 18:18 ` Bartosz Golaszewski
2019-01-18 13:42 ` [PATCH 09/13] power: supply: max77650: add support for battery charger Bartosz Golaszewski
2019-01-23 18:27 ` Sebastian Reichel [this message]
2019-01-18 13:42 ` [PATCH 10/13] gpio: max77650: add GPIO support Bartosz Golaszewski
2019-01-21 14:20 ` Linus Walleij
2019-01-21 17:07 ` Bartosz Golaszewski
2019-01-24 10:30 ` Linus Walleij
2019-01-29 11:00 ` Bartosz Golaszewski
2019-01-29 13:22 ` Bartosz Golaszewski
2019-01-18 13:42 ` [PATCH 11/13] leds: max77650: add LEDs support Bartosz Golaszewski
2019-01-20 16:39 ` Jacek Anaszewski
2019-01-18 13:42 ` [PATCH 12/13] input: max77650: add onkey support Bartosz Golaszewski
2019-01-19 9:03 ` Dmitry Torokhov
2019-01-21 10:52 ` Bartosz Golaszewski
2019-01-28 19:22 ` Dmitry Torokhov
2019-02-12 20:34 ` Lee Jones
2019-02-13 7:30 ` Dmitry Torokhov
2019-02-14 9:42 ` Lee Jones
2019-01-18 13:42 ` [PATCH 13/13] MAINTAINERS: add an entry for max77650 mfd driver Bartosz Golaszewski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190123182738.wtskerasrzmrufrn@earth.universe \
--to=sre@kernel.org \
--cc=bgolaszewski@baylibre.com \
--cc=brgl@bgdev.pl \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.torokhov@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=jacek.anaszewski@gmail.com \
--cc=lee.jones@linaro.org \
--cc=lgirdwood@gmail.com \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=pavel@ucw.cz \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).