Re: [PATCH 3/3] power: supply: Add a driver for Injoinic power bank ICs

[Sebastian Reichel]

Hi,

On Sat, Jan 29, 2022 at 04:24:24PM -0600, Samuel Holland wrote:
> This driver supports several chip variants which all share the same I2C
> register interface. Since the chip will turn off and become inaccessible
> under conditions outside of software control (e.g. upon button press or
> input voltage removal), some special handling is needed to delay the
> initialization of the IC until it is accessible.
> 
> Signed-off-by: Samuel Holland <samuel@xxxxxxxxxxxx>
> ---

Thanks, driver looks mostly good to me. Just two things:

> [...]
> +static const struct power_supply_desc ip5xxx_charger_desc = {
> +	.name			= "ip5xxx-charger",
> +	.type			= POWER_SUPPLY_TYPE_BATTERY,

POWER_SUPPLY_TYPE_BATTERY is the wrong type for a charger.
Considering the chips want 5V on the input side as far as
I could see, a sensible type is POWER_SUPPLY_TYPE_USB.

> +	.properties		= ip5xxx_charger_properties,
> +	.num_properties		= ARRAY_SIZE(ip5xxx_charger_properties),
> +	.get_property		= ip5xxx_charger_get_property,
> +	.set_property		= ip5xxx_charger_set_property,
> +	.property_is_writeable	= ip5xxx_charger_property_is_writeable,
> +};
> +
> +static const struct regmap_config ip5xxx_regmap_config = {
> +	.reg_bits		= 8,
> +	.val_bits		= 8,
> +	.max_register		= IP5XXX_BATOCV_DAT1,
> +};
> +
> +static int ip5xxx_power_probe(struct i2c_client *client)
> +{
> +	struct power_supply_config psy_cfg = {};
> +	struct device *dev = &client->dev;
> +	struct power_supply *psy;
> +	struct ip5xxx *ip5xxx;
> +
> +	ip5xxx = devm_kzalloc(dev, sizeof(*ip5xxx), GFP_KERNEL);

if (!ip5xxx)
    return -ENOMEM;

-- Sebastian

> +	ip5xxx->regmap = devm_regmap_init_i2c(client, &ip5xxx_regmap_config);
> +	if (IS_ERR(ip5xxx->regmap))
> +		return PTR_ERR(ip5xxx->regmap);
> +
> +	psy_cfg.of_node = dev->of_node;
> +	psy_cfg.drv_data = ip5xxx;
> +
> +	psy = devm_power_supply_register(dev, &ip5xxx_charger_desc, &psy_cfg);
> +	if (IS_ERR(psy))
> +		return PTR_ERR(psy);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ip5xxx_power_of_match[] = {
> +	{ .compatible = "injoinic,ip5108" },
> +	{ .compatible = "injoinic,ip5109" },
> +	{ .compatible = "injoinic,ip5207" },
> +	{ .compatible = "injoinic,ip5209" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ip5xxx_power_of_match);
> +
> +static struct i2c_driver ip5xxx_power_driver = {
> +	.probe_new	= ip5xxx_power_probe,
> +	.driver		= {
> +		.name		= "ip5xxx-power",
> +		.of_match_table	= ip5xxx_power_of_match,
> +	}
> +};
> +module_i2c_driver(ip5xxx_power_driver);
> +
> +MODULE_AUTHOR("Samuel Holland <samuel@xxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Injoinic IP5xxx power bank IC driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.33.1
>

Attachment: signature.asc
Description: PGP signature