Re: [EXTERNAL] Re: [PATCH v7 2/2] power: supply: bq256xx: Introduce the BQ256XX charger driver

[Ricardo Rivera-Matos <r-rivera-matos@xxxxxx>]

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@xxxxxx>
> >
> > 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@xxxxxx>
> > ---
> 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