| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 1 Oct 2020 11:47:10 -0500
From: Ricardo Rivera-Matos <r-rivera-matos@...com>
To: Sebastian Reichel <sre@...nel.org>
CC: <robh+dt@...nel.org>, <linux-pm@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<dmurphy@...com>
Subject: Re: [EXTERNAL] Re: [PATCH v4 2/2] power: supply: bq256xx: Introduce
the BQ256XX charger driver
Sebastian
On 9/30/20 6:47 PM, Sebastian Reichel wrote:
> Hi,
>
> You are leaking some resources, otherwise LGTM.
ACK
>
> 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().
ACK
>
>> +
>> + 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().
ACK, I will insert devm_add_action_or_reset() just before ret =
bq256xx_power_supply_init()
>
>> [...]
> -- Sebastian
Ricardo
Powered by blists - more mailing lists