mbox series

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

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

Message

Ricardo Rivera-Matos Sept. 23, 2020, 3:24 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        | 1769 +++++++++++++++++
 4 files changed, 1891 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/bq256xx.yaml
 create mode 100644 drivers/power/supply/bq256xx_charger.c

Comments

Sebastian Reichel Sept. 30, 2020, 11:47 p.m. UTC | #1 
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
Ricardo Rivera-Matos Oct. 1, 2020, 4:47 p.m. UTC | #2 
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