From: Sebastian Reichel <sre@kernel.org>
To: Iskren Chernev <iskren.chernev@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>,
linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
~postmarketos/upstreaming@lists.sr.ht,
Jonathan Bakker <xc-racer2@live.ca>,
Vladimir Barinov <vladimir.barinov@cogentembedded.com>
Subject: Re: [PATCH v3 1/6] power: supply: max17040: Use regmap i2c
Date: Fri, 28 Aug 2020 19:44:02 +0200 [thread overview]
Message-ID: <20200828174402.6fymgrib6qtnyn5v@earth.universe> (raw)
In-Reply-To: <20200624155633.3557401-2-iskren.chernev@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 13739 bytes --]
Hi,
On Wed, Jun 24, 2020 at 06:56:28PM +0300, Iskren Chernev wrote:
> Rewrite i2c operations from i2c client read/write to regmap i2c. As
> a result, most private functions now accept the private driver data
> instead of an i2c client pointer.
>
> Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com>
> ---
This still does two things: converting the driver to devm_
and using regmap. Please split into two patches. Otherwise
the changes look good, but the patchset needs to be rebased.
-- Sebastian
> drivers/power/supply/Kconfig | 1 +
> drivers/power/supply/max17040_battery.c | 250 +++++++++++-------------
> 2 files changed, 110 insertions(+), 141 deletions(-)
>
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 44d3c8512fb8d..12ca79952768f 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -367,6 +367,7 @@ config AXP288_FUEL_GAUGE
> config BATTERY_MAX17040
> tristate "Maxim MAX17040 Fuel Gauge"
> depends on I2C
> + select REGMAP_I2C
> help
> MAX17040 is fuel-gauge systems for lithium-ion (Li+) batteries
> in handheld and portable equipment. The MAX17040 is configured
> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> index 48aa44665e2f1..678241fcc2548 100644
> --- a/drivers/power/supply/max17040_battery.c
> +++ b/drivers/power/supply/max17040_battery.c
> @@ -16,32 +16,30 @@
> #include <linux/interrupt.h>
> #include <linux/power_supply.h>
> #include <linux/max17040_battery.h>
> +#include <linux/regmap.h>
> #include <linux/slab.h>
>
> #define MAX17040_VCELL 0x02
> #define MAX17040_SOC 0x04
> #define MAX17040_MODE 0x06
> #define MAX17040_VER 0x08
> -#define MAX17040_RCOMP 0x0C
> +#define MAX17040_CONFIG 0x0C
> #define MAX17040_CMD 0xFE
>
>
> #define MAX17040_DELAY 1000
> #define MAX17040_BATTERY_FULL 95
>
> -#define MAX17040_ATHD_MASK 0xFFC0
> +#define MAX17040_ATHD_MASK 0x3f
> #define MAX17040_ATHD_DEFAULT_POWER_UP 4
>
> struct max17040_chip {
> struct i2c_client *client;
> + struct regmap *regmap;
> struct delayed_work work;
> struct power_supply *battery;
> struct max17040_platform_data *pdata;
>
> - /* State Of Connect */
> - int online;
> - /* battery voltage */
> - int vcell;
> /* battery capacity */
> int soc;
> /* State Of Charge */
> @@ -50,135 +48,68 @@ struct max17040_chip {
> u32 low_soc_alert;
> };
>
> -static int max17040_get_property(struct power_supply *psy,
> - enum power_supply_property psp,
> - union power_supply_propval *val)
> -{
> - struct max17040_chip *chip = power_supply_get_drvdata(psy);
> -
> - switch (psp) {
> - case POWER_SUPPLY_PROP_STATUS:
> - val->intval = chip->status;
> - break;
> - case POWER_SUPPLY_PROP_ONLINE:
> - val->intval = chip->online;
> - break;
> - case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> - val->intval = chip->vcell;
> - break;
> - case POWER_SUPPLY_PROP_CAPACITY:
> - val->intval = chip->soc;
> - break;
> - default:
> - return -EINVAL;
> - }
> - return 0;
> -}
> -
> -static int max17040_write_reg(struct i2c_client *client, int reg, u16 value)
> -{
> - int ret;
> -
> - ret = i2c_smbus_write_word_swapped(client, reg, value);
> -
> - if (ret < 0)
> - dev_err(&client->dev, "%s: err %d\n", __func__, ret);
> -
> - return ret;
> -}
> -
> -static int max17040_read_reg(struct i2c_client *client, int reg)
> -{
> - int ret;
> -
> - ret = i2c_smbus_read_word_swapped(client, reg);
> -
> - if (ret < 0)
> - dev_err(&client->dev, "%s: err %d\n", __func__, ret);
> -
> - return ret;
> -}
> -
> -static void max17040_reset(struct i2c_client *client)
> +static int max17040_reset(struct max17040_chip *chip)
> {
> - max17040_write_reg(client, MAX17040_CMD, 0x0054);
> + return regmap_write(chip->regmap, MAX17040_CMD, 0x0054);
> }
>
> -static int max17040_set_low_soc_alert(struct i2c_client *client, u32 level)
> +static int max17040_set_low_soc_alert(struct max17040_chip *chip, u32 level)
> {
> - int ret;
> - u16 data;
> -
> level = 32 - level;
> - data = max17040_read_reg(client, MAX17040_RCOMP);
> - /* clear the alrt bit and set LSb 5 bits */
> - data &= MAX17040_ATHD_MASK;
> - data |= level;
> - ret = max17040_write_reg(client, MAX17040_RCOMP, data);
> -
> - return ret;
> + return regmap_update_bits(chip->regmap, MAX17040_CONFIG,
> + MAX17040_ATHD_MASK, level);
> }
>
> -static void max17040_get_vcell(struct i2c_client *client)
> +static int max17040_get_vcell(struct max17040_chip *chip)
> {
> - struct max17040_chip *chip = i2c_get_clientdata(client);
> - u16 vcell;
> + u32 vcell;
>
> - vcell = max17040_read_reg(client, MAX17040_VCELL);
> + regmap_read(chip->regmap, MAX17040_VCELL, &vcell);
>
> - chip->vcell = (vcell >> 4) * 1250;
> + return (vcell >> 4) * 1250;
> }
>
> -static void max17040_get_soc(struct i2c_client *client)
> +static int max17040_get_soc(struct max17040_chip *chip)
> {
> - struct max17040_chip *chip = i2c_get_clientdata(client);
> - u16 soc;
> + u32 soc;
>
> - soc = max17040_read_reg(client, MAX17040_SOC);
> + regmap_read(chip->regmap, MAX17040_SOC, &soc);
>
> - chip->soc = (soc >> 8);
> + return soc >> 8;
> }
>
> -static void max17040_get_version(struct i2c_client *client)
> +static int max17040_get_version(struct max17040_chip *chip)
> {
> - u16 version;
> + int ret;
> + u32 version;
>
> - version = max17040_read_reg(client, MAX17040_VER);
> + ret = regmap_read(chip->regmap, MAX17040_VER, &version);
>
> - dev_info(&client->dev, "MAX17040 Fuel-Gauge Ver 0x%x\n", version);
> + return ret ? ret : version;
> }
>
> -static void max17040_get_online(struct i2c_client *client)
> +static int max17040_get_online(struct max17040_chip *chip)
> {
> - struct max17040_chip *chip = i2c_get_clientdata(client);
> -
> - if (chip->pdata && chip->pdata->battery_online)
> - chip->online = chip->pdata->battery_online();
> - else
> - chip->online = 1;
> + return chip->pdata && chip->pdata->battery_online ?
> + chip->pdata->battery_online() : 1;
> }
>
> -static void max17040_get_status(struct i2c_client *client)
> +static int max17040_get_status(struct max17040_chip *chip)
> {
> - struct max17040_chip *chip = i2c_get_clientdata(client);
> -
> if (!chip->pdata || !chip->pdata->charger_online
> - || !chip->pdata->charger_enable) {
> - chip->status = POWER_SUPPLY_STATUS_UNKNOWN;
> - return;
> - }
> + || !chip->pdata->charger_enable)
> + return POWER_SUPPLY_STATUS_UNKNOWN;
> +
> + if (max17040_get_soc(chip) > MAX17040_BATTERY_FULL)
> + return POWER_SUPPLY_STATUS_FULL;
>
> - if (chip->pdata->charger_online()) {
> + if (chip->pdata->charger_online())
> if (chip->pdata->charger_enable())
> - chip->status = POWER_SUPPLY_STATUS_CHARGING;
> + return POWER_SUPPLY_STATUS_CHARGING;
> else
> - chip->status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> - } else {
> - chip->status = POWER_SUPPLY_STATUS_DISCHARGING;
> - }
> -
> - if (chip->soc > MAX17040_BATTERY_FULL)
> - chip->status = POWER_SUPPLY_STATUS_FULL;
> + return POWER_SUPPLY_STATUS_NOT_CHARGING;
> + else
> + return POWER_SUPPLY_STATUS_DISCHARGING;
> }
>
> static int max17040_get_of_data(struct max17040_chip *chip)
> @@ -190,18 +121,31 @@ static int max17040_get_of_data(struct max17040_chip *chip)
> "maxim,alert-low-soc-level",
> &chip->low_soc_alert);
>
> - if (chip->low_soc_alert <= 0 || chip->low_soc_alert >= 33)
> + if (chip->low_soc_alert <= 0 || chip->low_soc_alert >= 33) {
> + dev_err(dev, "maxim,alert-low-soc-level out of bounds\n");
> return -EINVAL;
> + }
>
> return 0;
> }
>
> -static void max17040_check_changes(struct i2c_client *client)
> +static void max17040_check_changes(struct max17040_chip *chip)
> +{
> + chip->soc = max17040_get_soc(chip);
> + chip->status = max17040_get_status(chip);
> +}
> +
> +static void max17040_queue_work(struct max17040_chip *chip)
> +{
> + queue_delayed_work(system_power_efficient_wq, &chip->work,
> + MAX17040_DELAY);
> +}
> +
> +static void max17040_stop_work(void *data)
> {
> - max17040_get_vcell(client);
> - max17040_get_soc(client);
> - max17040_get_online(client);
> - max17040_get_status(client);
> + struct max17040_chip *chip = data;
> +
> + cancel_delayed_work_sync(&chip->work);
> }
>
> static void max17040_work(struct work_struct *work)
> @@ -214,30 +158,29 @@ static void max17040_work(struct work_struct *work)
> /* store SOC and status to check changes */
> last_soc = chip->soc;
> last_status = chip->status;
> - max17040_check_changes(chip->client);
> + max17040_check_changes(chip);
>
> /* check changes and send uevent */
> if (last_soc != chip->soc || last_status != chip->status)
> power_supply_changed(chip->battery);
>
> - queue_delayed_work(system_power_efficient_wq, &chip->work,
> - MAX17040_DELAY);
> + max17040_queue_work(chip);
> }
>
> static irqreturn_t max17040_thread_handler(int id, void *dev)
> {
> struct max17040_chip *chip = dev;
> - struct i2c_client *client = chip->client;
>
> - dev_warn(&client->dev, "IRQ: Alert battery low level");
> + dev_warn(&chip->client->dev, "IRQ: Alert battery low level");
> +
> /* read registers */
> - max17040_check_changes(chip->client);
> + max17040_check_changes(chip);
>
> /* send uevent */
> power_supply_changed(chip->battery);
>
> /* reset alert bit */
> - max17040_set_low_soc_alert(client, chip->low_soc_alert);
> + max17040_set_low_soc_alert(chip, chip->low_soc_alert);
>
> return IRQ_HANDLED;
> }
> @@ -256,6 +199,38 @@ static int max17040_enable_alert_irq(struct max17040_chip *chip)
> return ret;
> }
>
> +static int max17040_get_property(struct power_supply *psy,
> + enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + struct max17040_chip *chip = power_supply_get_drvdata(psy);
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_STATUS:
> + val->intval = max17040_get_status(chip);
> + break;
> + case POWER_SUPPLY_PROP_ONLINE:
> + val->intval = max17040_get_online(chip);
> + break;
> + case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> + val->intval = max17040_get_vcell(chip);
> + break;
> + case POWER_SUPPLY_PROP_CAPACITY:
> + val->intval = max17040_get_soc(chip);
> + break;
> + default:
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static const struct regmap_config max17040_regmap = {
> + .reg_bits = 8,
> + .reg_stride = 2,
> + .val_bits = 16,
> + .val_format_endian = REGMAP_ENDIAN_BIG,
> +};
> +
> static enum power_supply_property max17040_battery_props[] = {
> POWER_SUPPLY_PROP_STATUS,
> POWER_SUPPLY_PROP_ONLINE,
> @@ -287,31 +262,33 @@ static int max17040_probe(struct i2c_client *client,
> return -ENOMEM;
>
> chip->client = client;
> + chip->regmap = devm_regmap_init_i2c(client, &max17040_regmap);
> chip->pdata = client->dev.platform_data;
> ret = max17040_get_of_data(chip);
> - if (ret) {
> - dev_err(&client->dev,
> - "failed: low SOC alert OF data out of bounds\n");
> + if (ret)
> return ret;
> - }
>
> i2c_set_clientdata(client, chip);
> psy_cfg.drv_data = chip;
>
> - chip->battery = power_supply_register(&client->dev,
> + chip->battery = devm_power_supply_register(&client->dev,
> &max17040_battery_desc, &psy_cfg);
> if (IS_ERR(chip->battery)) {
> dev_err(&client->dev, "failed: power supply register\n");
> return PTR_ERR(chip->battery);
> }
>
> - max17040_reset(client);
> - max17040_get_version(client);
> + ret = max17040_get_version(chip);
> + if (ret < 0)
> + return ret;
> + dev_dbg(&chip->client->dev, "MAX17040 Fuel-Gauge Ver 0x%x\n", ret);
> +
> + max17040_reset(chip);
>
> /* check interrupt */
> if (client->irq && of_device_is_compatible(client->dev.of_node,
> "maxim,max77836-battery")) {
> - ret = max17040_set_low_soc_alert(client, chip->low_soc_alert);
> + ret = max17040_set_low_soc_alert(chip, chip->low_soc_alert);
> if (ret) {
> dev_err(&client->dev,
> "Failed to set low SOC alert: err %d\n", ret);
> @@ -327,18 +304,11 @@ static int max17040_probe(struct i2c_client *client,
> }
>
> INIT_DEFERRABLE_WORK(&chip->work, max17040_work);
> - queue_delayed_work(system_power_efficient_wq, &chip->work,
> - MAX17040_DELAY);
> -
> - return 0;
> -}
> -
> -static int max17040_remove(struct i2c_client *client)
> -{
> - struct max17040_chip *chip = i2c_get_clientdata(client);
> + ret = devm_add_action(&client->dev, max17040_stop_work, chip);
> + if (ret)
> + return ret;
> + max17040_queue_work(chip);
>
> - power_supply_unregister(chip->battery);
> - cancel_delayed_work(&chip->work);
> return 0;
> }
>
> @@ -362,12 +332,11 @@ static int max17040_resume(struct device *dev)
> struct i2c_client *client = to_i2c_client(dev);
> struct max17040_chip *chip = i2c_get_clientdata(client);
>
> - queue_delayed_work(system_power_efficient_wq, &chip->work,
> - MAX17040_DELAY);
> -
> if (client->irq && device_may_wakeup(dev))
> disable_irq_wake(client->irq);
>
> + max17040_queue_work(chip);
> +
> return 0;
> }
>
> @@ -401,7 +370,6 @@ static struct i2c_driver max17040_i2c_driver = {
> .pm = MAX17040_PM_OPS,
> },
> .probe = max17040_probe,
> - .remove = max17040_remove,
> .id_table = max17040_id,
> };
> module_i2c_driver(max17040_i2c_driver);
> --
> 2.27.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2020-08-28 17:44 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-24 15:56 [PATCH v3 0/6] power: supply: max17040 support compatible devices Iskren Chernev
2020-06-24 15:56 ` [PATCH v3 1/6] power: supply: max17040: Use regmap i2c Iskren Chernev
2020-08-28 17:44 ` Sebastian Reichel [this message]
2020-06-24 15:56 ` [PATCH v3 2/6] dt-bindings: power: supply: Extend max17040 compatibility Iskren Chernev
2020-07-13 19:03 ` Rob Herring
2020-07-14 8:49 ` Iskren Chernev
2020-07-14 21:08 ` Rob Herring
2020-06-24 15:56 ` [PATCH v3 3/6] power: supply: max17040: Support compatible devices Iskren Chernev
2020-06-24 15:56 ` [PATCH v3 4/6] dt-bindings: power: supply: max17040: Add maxim,rcomp Iskren Chernev
2020-07-13 19:04 ` Rob Herring
2020-06-24 15:56 ` [PATCH v3 5/6] power: supply: max17040: Support setting rcomp Iskren Chernev
2020-06-24 15:56 ` [PATCH v3 6/6] power: supply: max17040: Support soc alert Iskren Chernev
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=20200828174402.6fymgrib6qtnyn5v@earth.universe \
--to=sre@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=iskren.chernev@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=vladimir.barinov@cogentembedded.com \
--cc=xc-racer2@live.ca \
--cc=~postmarketos/upstreaming@lists.sr.ht \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.