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