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

[Samuel Holland]

Hi,

On 2/11/22 3:22 PM, Sebastian Reichel wrote:
> 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.

The chip takes a 5V USB input, charges a battery, and then boosts the battery
voltage back up to 5V on the output side (or passes the input voltage, if
present, directly through to the output).

Currently only POWER_SUPPLY_PROP_ONLINE describes the output side of the chip.
All of the other properties here describe the battery. For example,
POWER_SUPPLY_PROP_VOLTAGE/CURRENT_NOW is the battery voltage/current, not the
output voltage/current. That doesn't match what I would expect from
POWER_SUPPLY_TYPE_USB.

Should there really be two separate power supplies, one for the battery of type
POWER_SUPPLY_TYPE_BATTERY, and a second one for the output side of type
POWER_SUPPLY_TYPE_USB, with the battery supplying the boost output stage? I'll
send v2 with this idea; please let me know what you think.

Since these two supplies would share the same device and OF node, how would I
link them together?

>> +	.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;

I'll fix this in v2.

Regards,
Samuel

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