mbox series

[v7,0/2] Introduce the BQ256XX family of chargers

Message ID 20201230230116.29697-1-r-rivera-matos@ti.com
Headers show
Series Introduce the BQ256XX family of chargers | expand

Message

Ricardo Rivera-Matos Dec. 30, 2020, 11:01 p.m. UTC 
Hello,

This patchset introduces the bq256xx family of charging ICs. The bq256xx
ICs are highly integrated, buck, switching chargers intended for use in 
smartphones, tablets, and portable electronics.

Ricardo Rivera-Matos (2):
  dt-bindings: power: Add the bq256xx dt bindings
  power: supply: bq256xx: Introduce the BQ256XX charger driver

 .../bindings/power/supply/bq256xx.yaml        |  110 ++
 drivers/power/supply/Kconfig                  |   11 +
 drivers/power/supply/Makefile                 |    1 +
 drivers/power/supply/bq256xx_charger.c        | 1747 +++++++++++++++++
 4 files changed, 1869 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/bq256xx.yaml
 create mode 100644 drivers/power/supply/bq256xx_charger.c

Comments

Sebastian Reichel Jan. 3, 2021, 1:26 a.m. UTC | #1 
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.

>  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.

> [...]
> +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!

> +	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.

> [...]

-- Sebastian
Ricardo Rivera-Matos Jan. 3, 2021, 9:26 p.m. UTC | #2 
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