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

[Dan Murphy]

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