lkml.org 
[lkml]   [2021]   [Jan]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [EXTERNAL] Re: [PATCH v7 2/2] power: supply: bq256xx: Introduce the BQ256XX charger driver
From
Date
Sebastian

On 1/2/21 7:26 PM, Sebastian Reichel wrote:
> Hi Ricardo,
>
> On Wed, Dec 30, 2020 at 05:01:16PM -0600, Ricardo Rivera-Matos wrote:
>> The BQ256XX family of devices are highly integrated buck chargers
>> for single cell batteries.
>>
>> Signed-off-by: Ricardo Rivera-Matos <r-rivera-matos@ti.com>
>>
>> v5 - adds power_supply_put_battery_info() and devm_add_action_or_rest() calls
>>
>> v6 - implements bq256xx_remove function
>>
>> v7 - applies various fixes
>>
>> - implements clamp() API
>>
>> - implements memcmp() API
>>
>> - changes cache_type to REGACHE_FLAT
>>
>> - changes bq256xx_probe to properly unregister device
>>
>> Signed-off-by: Ricardo Rivera-Matos <r-rivera-matos@ti.com>
>> ---
> Thanks, looks mostly good now.
Cool :)
>
>> drivers/power/supply/Kconfig | 11 +
>> drivers/power/supply/Makefile | 1 +
>> drivers/power/supply/bq256xx_charger.c | 1747 ++++++++++++++++++++++++
>> 3 files changed, 1759 insertions(+)
>> create mode 100644 drivers/power/supply/bq256xx_charger.c
>>
>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>> index 44d3c8512fb8..87d852914bc2 100644
>> --- a/drivers/power/supply/Kconfig
>> +++ b/drivers/power/supply/Kconfig
>> @@ -618,6 +618,17 @@ config CHARGER_BQ25890
>> help
>> Say Y to enable support for the TI BQ25890 battery charger.
>>
>> +config CHARGER_BQ256XX
>> + tristate "TI BQ256XX battery charger driver"
>> + depends on I2C
>> + depends on GPIOLIB || COMPILE_TEST
>> + select REGMAP_I2C
>> + help
>> + Say Y to enable support for the TI BQ256XX battery chargers. The
>> + BQ256XX family of devices are highly-integrated, switch-mode battery
>> + charge management and system power path management devices for single
>> + cell Li-ion and Li-polymer batteries.
>> +
>> config CHARGER_SMB347
>> tristate "Summit Microelectronics SMB347 Battery Charger"
>> depends on I2C
> Please rebase to current power-supply for-next branch, Kconfig and
> Makefile diff does not apply because of one additional BQ device.
ACK
>
>> [...]
>> +static void bq256xx_usb_work(struct work_struct *data)
>> +{
>> + struct bq256xx_device *bq =
>> + container_of(data, struct bq256xx_device, usb_work);
>> +
>> + switch (bq->usb_event) {
>> + case USB_EVENT_ID:
>> + break;
>> +
> spurious newline, please remove!
ACK
>
>> + case USB_EVENT_NONE:
>> + power_supply_changed(bq->charger);
>> + break;
>> + default:
>> + dev_err(bq->dev, "Error switching to charger mode.\n");
>> + break;
>> + }
>> +}
>> +
>> [...]
>> +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;
>> +
>> + power_supply_put_battery_info(bq->charger, &bat_info);
>> +
>> + return 0;
>> +
>> +err_out:
>> + return ret;
> please return error code directly instead of adding this useless
> goto.
ACK
>
>> [...]
> -- Sebastian
Ricardo

\
 
 \ /
  Last update: 2021-01-03 22:30    [W:0.078 / U:0.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site