Re: [PATCH 3/3] power: supply: bq2515x: Introduce the bq2515x family

[Dan Murphy <dmurphy@ti.com>]

Sebastian

On 10/20/19 7:15 AM, Sebastian Reichel wrote:
> Hi Dan,
>
> On Mon, Sep 30, 2019 at 09:31:37AM -0500, Dan Murphy wrote:
[...]
>> +
>> +static int bq2515x_power_supply_register(struct bq2515x_device *bq2515x)
>> +{
>> +	struct power_supply_config psy_cfg = { .drv_data = bq2515x, };
>> +	int ret = -EINVAL;
>> +
>> +	bq2515x->mains = devm_power_supply_register(bq2515x->dev,
>> +						    &bq2515x_mains_desc,
>> +						    &psy_cfg);
>> +	if (IS_ERR(bq2515x->mains))
>> +		return ret;
>> +
>> +	bq2515x->battery = devm_power_supply_register(bq2515x->dev,
>> +						      &bq2515x_battery_desc,
>> +						      &psy_cfg);
>> +	if (IS_ERR(bq2515x->battery)) {
>> +		power_supply_unregister(bq2515x->mains);
> you registered the mains power-supply with devm_ prefix, so it
> will be removed automatically.
Ack
>
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int bq2515x_hw_init(struct bq2515x_device *bq2515x)
>> +{
>> +	int ret = 0;
>> +
>> +	if (bq2515x->init_data.ichg)
>> +		ret = bq2515x_set_ilim_lvl(bq2515x, bq2515x->init_data.ichg);
>> +
>> +	if (bq2515x->init_data.vreg)
>> +		ret = bq2515x_set_batt_reg(bq2515x, bq2515x->init_data.vreg);
> This throws away potential error code from bq2515x_set_ilim_lvl().
Ack
>> +	return ret;
>> +}
>> +
>> +static int bq2515x_read_properties(struct bq2515x_device *bq2515x)
>> +{
>> +	int ret;
>> +
>> +	ret = device_property_read_u8(bq2515x->dev, "ti,charge-current",
>> +				      &bq2515x->init_data.ichg);
>> +	if (ret)
>> +		goto fail;
>> +
>> +	ret = device_property_read_u8(bq2515x->dev,
>> +				      "ti,battery-regulation-voltage",
>> +				      &bq2515x->init_data.vreg);
>> +	if (ret)
>> +		goto fail;
> The above properties are marked as optional in the DT bindings
> document.
ACK.  Will just set them to the data sheet default values
>
>> +	bq2515x->pg_gpio = devm_gpiod_get_optional(bq2515x->dev,
>> +						   "pg", GPIOD_IN);
>> +	if (IS_ERR(bq2515x->pg_gpio))
>> +		dev_info(bq2515x->dev, "PG GPIO not defined");
>> +
>> +	bq2515x->reset_gpio = devm_gpiod_get_optional(bq2515x->dev,
>> +						   "reset", GPIOD_OUT_LOW);
>> +	if (IS_ERR(bq2515x->reset_gpio))
>> +		dev_info(bq2515x->dev, "reset GPIO not defined");
>> +
>> +	bq2515x->lp_gpio = devm_gpiod_get_optional(bq2515x->dev, "low-power",
>> +						   GPIOD_OUT_LOW);
>> +	if (IS_ERR(bq2515x->lp_gpio))
>> +		dev_info(bq2515x->dev, "LP GPIO not defined");
>> +
>> +	bq2515x->ce_gpio = devm_gpiod_get_optional(bq2515x->dev,
>> +						   "charge-enable",
>> +						   GPIOD_OUT_HIGH);
>> +	if (IS_ERR(bq2515x->ce_gpio))
>> +		dev_info(bq2515x->dev, "Charge enable GPIO not defined");
> The GPIO errors should not be ignored, especially since you
> might get a -EPROBE_DEFER.

ACK


>> +fail:
>> +	return ret;
>> +}
>> +
>> +static const struct regmap_config bq2515x_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +
>> +	.max_register = BQ2515X_DEVICE_ID,
>> +	.reg_defaults     = bq2515x_reg_defs,
>> +	.num_reg_defaults = ARRAY_SIZE(bq2515x_reg_defs),
>> +	.cache_type	  = REGCACHE_RBTREE,
>> +};
>> +
>> +static int bq2515x_probe(struct i2c_client *client,
>> +			 const struct i2c_device_id *id)
>> +{
>> +	struct device *dev = &client->dev;
>> +	struct bq2515x_device *bq;
>> +	int ret;
>> +
>> +	bq = devm_kzalloc(dev, sizeof(*bq), GFP_KERNEL);
>> +	if (!bq)
>> +		return -ENOMEM;
>> +
>> +	bq->client = client;
>> +	bq->dev = dev;
>> +
>> +	mutex_init(&bq->lock);
>> +
>> +	bq->regmap = devm_regmap_init_i2c(client, &bq2515x_regmap_config);
>> +	if (IS_ERR(bq->regmap)) {
>> +		dev_err(dev, "failed to allocate register map\n");
>> +		return PTR_ERR(bq->regmap);
>> +	}
>> +
>> +	strncpy(bq->model_name, id->name, I2C_NAME_SIZE);
>> +
>> +	i2c_set_clientdata(client, bq);
>> +
>> +	ret = bq2515x_read_properties(bq);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to register power supply\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = bq2515x_hw_init(bq);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Cannot initialize the chip.\n");
>> +		return ret;
>> +	}
>> +
>> +	return bq2515x_power_supply_register(bq);
>> +}
>> +
>> +static const struct i2c_device_id bq2515x_i2c_ids[] = {
>> +	{ "bq25150", 0 },
>> +	{ "bq25155", 1 },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(i2c, bq2515x_i2c_ids);
>> +
>> +static const struct of_device_id bq2515x_of_match[] = {
>> +	{ .compatible = "ti,bq25150", },
>> +	{ .compatible = "ti,bq25155", },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, bq2515x_of_match);
>> +
>> +static struct i2c_driver bq2515x_driver = {
>> +	.driver = {
>> +		.name = "bq2515x-charger",
>> +		.of_match_table = of_match_ptr(bq2515x_of_match),
> You need to do one of those:
>
>   * remove of_match_ptr and specify bq2515x_of_match directly
>   * put the OF table in #ifdef CONFIG_OF
>   * mark the OF table as __maybe_unused

Probably just do the first one.

Dan