From: Sebastian Reichel <sre@kernel.org>
To: Ricardo Rivera-Matos <r-rivera-matos@ti.com>
Cc: robh+dt@kernel.org, linux-pm@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
dmurphy@ti.com
Subject: Re: [PATCH v4 2/2] power: supply: bq256xx: Introduce the BQ256XX charger driver
Date: Thu, 1 Oct 2020 01:47:25 +0200 [thread overview]
Message-ID: <20200930234725.467aylfzokwzw72z@earth.universe> (raw)
In-Reply-To: <20200923152416.24822-3-r-rivera-matos@ti.com>
[-- Attachment #1: Type: text/plain, Size: 5540 bytes --]
Hi,
You are leaking some resources, otherwise LGTM.
On Wed, Sep 23, 2020 at 10:24:16AM -0500, Ricardo Rivera-Matos wrote:
> [...]
> +static int bq256xx_hw_init(struct bq256xx_device *bq)
> +{
> + struct power_supply_battery_info bat_info = { };
> + int wd_reg_val = BQ256XX_WATCHDOG_DIS;
> + int ret = 0;
> + int i;
> +
> + for (i = 0; i < BQ256XX_NUM_WD_VAL; i++) {
> + if (bq->watchdog_timer > bq256xx_watchdog_time[i] &&
> + bq->watchdog_timer < bq256xx_watchdog_time[i + 1])
> + wd_reg_val = i;
> + }
> + ret = regmap_update_bits(bq->regmap, BQ256XX_CHARGER_CONTROL_1,
> + BQ256XX_WATCHDOG_MASK, wd_reg_val <<
> + BQ256XX_WDT_BIT_SHIFT);
> +
> + ret = power_supply_get_battery_info(bq->charger, &bat_info);
> + if (ret) {
> + dev_warn(bq->dev, "battery info missing, default values will be applied\n");
> +
> + bat_info.constant_charge_current_max_ua =
> + bq->chip_info->bq256xx_def_ichg;
> +
> + bat_info.constant_charge_voltage_max_uv =
> + bq->chip_info->bq256xx_def_vbatreg;
> +
> + bat_info.precharge_current_ua =
> + bq->chip_info->bq256xx_def_iprechg;
> +
> + bat_info.charge_term_current_ua =
> + bq->chip_info->bq256xx_def_iterm;
> +
> + bq->init_data.ichg_max =
> + bq->chip_info->bq256xx_max_ichg;
> +
> + bq->init_data.vbatreg_max =
> + bq->chip_info->bq256xx_max_vbatreg;
> + } else {
> + bq->init_data.ichg_max =
> + bat_info.constant_charge_current_max_ua;
> +
> + bq->init_data.vbatreg_max =
> + bat_info.constant_charge_voltage_max_uv;
> + }
> +
> + ret = bq->chip_info->bq256xx_set_vindpm(bq, bq->init_data.vindpm);
> + if (ret)
> + goto err_out;
> +
> + ret = bq->chip_info->bq256xx_set_iindpm(bq, bq->init_data.iindpm);
> + if (ret)
> + goto err_out;
> +
> + ret = bq->chip_info->bq256xx_set_ichg(bq,
> + bat_info.constant_charge_current_max_ua);
> + if (ret)
> + goto err_out;
> +
> + ret = bq->chip_info->bq256xx_set_iprechg(bq,
> + bat_info.precharge_current_ua);
> + if (ret)
> + goto err_out;
> +
> + ret = bq->chip_info->bq256xx_set_vbatreg(bq,
> + bat_info.constant_charge_voltage_max_uv);
> + if (ret)
> + goto err_out;
> +
> + ret = bq->chip_info->bq256xx_set_iterm(bq,
> + bat_info.charge_term_current_ua);
> + if (ret)
> + goto err_out;
You need to power_supply_put_battery_info().
> +
> + return 0;
> +
> +err_out:
> + return ret;
> +}
> +
> +static int bq256xx_parse_dt(struct bq256xx_device *bq)
> +{
> + int ret = 0;
> +
> + ret = device_property_read_u32(bq->dev, "ti,watchdog-timeout-ms",
> + &bq->watchdog_timer);
> + if (ret)
> + bq->watchdog_timer = BQ256XX_WATCHDOG_DIS;
> +
> + if (bq->watchdog_timer > BQ256XX_WATCHDOG_MAX ||
> + bq->watchdog_timer < BQ256XX_WATCHDOG_DIS)
> + return -EINVAL;
> +
> + ret = device_property_read_u32(bq->dev,
> + "input-voltage-limit-microvolt",
> + &bq->init_data.vindpm);
> + if (ret)
> + bq->init_data.vindpm = bq->chip_info->bq256xx_def_vindpm;
> +
> + ret = device_property_read_u32(bq->dev,
> + "input-current-limit-microamp",
> + &bq->init_data.iindpm);
> + if (ret)
> + bq->init_data.iindpm = bq->chip_info->bq256xx_def_iindpm;
> +
> + return 0;
> +}
> +
> +static int bq256xx_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct device *dev = &client->dev;
> + struct bq256xx_device *bq;
> + int ret;
> +
> + bq = devm_kzalloc(dev, sizeof(*bq), GFP_KERNEL);
> + if (!bq)
> + return -ENOMEM;
> +
> + bq->client = client;
> + bq->dev = dev;
> + bq->chip_info = &bq256xx_chip_info_tbl[id->driver_data];
> +
> + mutex_init(&bq->lock);
> +
> + strncpy(bq->model_name, id->name, I2C_NAME_SIZE);
> +
> + bq->regmap = devm_regmap_init_i2c(client,
> + bq->chip_info->bq256xx_regmap_config);
> +
> + if (IS_ERR(bq->regmap)) {
> + dev_err(dev, "Failed to allocate register map\n");
> + return PTR_ERR(bq->regmap);
> + }
> +
> + i2c_set_clientdata(client, bq);
> +
> + ret = bq256xx_parse_dt(bq);
> + if (ret) {
> + dev_err(dev, "Failed to read device tree properties%d\n", ret);
> + return ret;
> + }
> +
> + /* OTG reporting */
> + bq->usb2_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> + if (!IS_ERR_OR_NULL(bq->usb2_phy)) {
> + INIT_WORK(&bq->usb_work, bq256xx_usb_work);
> + bq->usb_nb.notifier_call = bq256xx_usb_notifier;
> + usb_register_notifier(bq->usb2_phy, &bq->usb_nb);
> + }
> +
> + bq->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
> + if (!IS_ERR_OR_NULL(bq->usb3_phy)) {
> + INIT_WORK(&bq->usb_work, bq256xx_usb_work);
> + bq->usb_nb.notifier_call = bq256xx_usb_notifier;
> + usb_register_notifier(bq->usb3_phy, &bq->usb_nb);
> + }
> +
> + if (client->irq) {
> + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> + bq256xx_irq_handler_thread,
> + IRQF_TRIGGER_FALLING |
> + IRQF_ONESHOT,
> + dev_name(&client->dev), bq);
> + if (ret)
> + goto error_out;
> + }
> +
> + ret = bq256xx_power_supply_init(bq, dev);
> + if (ret) {
> + dev_err(dev, "Failed to register power supply\n");
> + goto error_out;
> + }
> +
> + ret = bq256xx_hw_init(bq);
> + if (ret) {
> + dev_err(dev, "Cannot initialize the chip.\n");
> + goto error_out;
> + }
> +
> + return ret;
> +
> +error_out:
> + if (!IS_ERR_OR_NULL(bq->usb2_phy))
> + usb_unregister_notifier(bq->usb2_phy, &bq->usb_nb);
> +
> + if (!IS_ERR_OR_NULL(bq->usb3_phy))
> + usb_unregister_notifier(bq->usb3_phy, &bq->usb_nb);
> + return ret;
This also needs to be called during driver removal. Probably
it's best to do this via devm_add_action_or_reset().
> [...]
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2020-09-30 23:47 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-23 15:24 [PATCH v4 0/2] Introduce the BQ256XX family of chargers Ricardo Rivera-Matos
2020-09-23 15:24 ` [PATCH v4 1/2] dt-bindings: power: Add the bq256xx dt bindings Ricardo Rivera-Matos
2020-09-28 18:47 ` Rob Herring
2020-09-23 15:24 ` [PATCH v4 2/2] power: supply: bq256xx: Introduce the BQ256XX charger driver Ricardo Rivera-Matos
2020-09-30 23:47 ` Sebastian Reichel [this message]
2020-10-01 16:47 ` [EXTERNAL] " Ricardo Rivera-Matos
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=20200930234725.467aylfzokwzw72z@earth.universe \
--to=sre@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dmurphy@ti.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=r-rivera-matos@ti.com \
--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).