ср, 12 бер. 2025 р. о 11:15 Svyatoslav Ryhel <clamor95@xxxxxxxxx> пише:
>
> ср, 12 бер. 2025 р. о 10:50 Sebastian Reichel
> <sebastian.reichel@xxxxxxxxxxxxx> пише:
> >
> > Hi,
> >
> > I have a couple of comments inline.
> >
> > On Mon, Mar 10, 2025 at 10:02:37AM +0200, Svyatoslav Ryhel wrote:
> > > The MAX8971 is a compact, high-frequency, high-efficiency switch-mode
> > > charger for a one-cell lithium-ion (Li+) battery.
> > >
> ...
> > > +
> > > +static int max8971_get_health(struct max8971_data *priv, int *val)
> > > +{
> > > + u32 regval;
> > > + int err;
> > > +
> > > + err = regmap_field_read(priv->rfield[THM_DTLS], ®val);
> > > + if (err)
> > > + return err;
> > > +
> > > + switch (regval) {
> > > + case MAX8971_HEALTH_COLD:
> > > + *val = POWER_SUPPLY_HEALTH_COLD;
> > > + break;
> > > + case MAX8971_HEALTH_COOL:
> > > + *val = POWER_SUPPLY_HEALTH_COOL;
> > > + break;
> > > + case MAX8971_HEALTH_WARM:
> > > + *val = POWER_SUPPLY_HEALTH_GOOD;
> > > + break;
> > > + case MAX8971_HEALTH_HOT:
> > > + *val = POWER_SUPPLY_HEALTH_HOT;
> > > + break;
> > > + case MAX8971_HEALTH_OVERHEAT:
> > > + *val = POWER_SUPPLY_HEALTH_OVERHEAT;
> > > + break;
> > > + case MAX8971_HEALTH_UNKNOWN:
> > > + default:
> > > + *val = POWER_SUPPLY_HEALTH_UNKNOWN;
> > > + }
> >
> > I guess it makes sense to report POWER_SUPPLY_HEALTH_DEAD
> > for MAX8971_CHARGING_DEAD_BATTERY?
> >
>
> It seems that I have missed that, thank you for pointing.
>
MAX8971_CHARGING_DEAD_BATTERY is not same as POWER_SUPPLY_HEALTH_DEAD
so we cannot use MAX8971_CHARGING_DEAD_BATTERY here. DEAD_BATTERY in
charging context is state of deep discharge not the battery health
overall. max8971_get_health returns charger state, not battery.
Battery state is monitored by dedicated controller.
> > > +
> > > + return 0;
> > > +}
> > > +
> ...
> > > +
> > > +static DEVICE_ATTR_RW(fast_charge_timer);
> > > +static DEVICE_ATTR_RW(top_off_threshold_current);
> > > +static DEVICE_ATTR_RW(top_off_timer);
> > > +
> > > +static struct attribute *max8971_attributes[] = {
> > > + &dev_attr_fast_charge_timer.attr,
> > > + &dev_attr_top_off_threshold_current.attr,
> > > + &dev_attr_top_off_timer.attr,
> > > + NULL
> > > +};
> >
> > Missing ABI documentation. Also wondering if we can instead just
> > configure sensible values without exposing them to userspace. For
> > debugging things there is always the regmap debugfs API.
> >
>
> These values are exposed like in the other maxim charger
> (max77693_charger to be exact) so I inspired with that plus dt
> maintainers were extremely against these in the tree.
>
> Would be nice to have those configurable at least in some way, if not
> by tree then by userspace. I assume ABI documentation should be a
> separate patch.
>
> > > +static const struct attribute_group max8971_attr_group = {
> > > + .attrs = max8971_attributes,
> > > +};
> > > +
> ...
> > > + err = devm_device_add_group(dev, &max8971_attr_group);
> > > + if (err)
> > > + return dev_err_probe(dev, err, "failed to create sysfs attributes\n");
> >
> > Iff we need the custom properties they should be at least registered
> > automatically at device creation time via 'cfg.attr_grp'.
> >
>
> I actually did not know this was an option. Thanks for pointing out.
>
> > > + err = devm_request_threaded_irq(dev, client->irq, NULL, &max8971_interrupt,
> > > + IRQF_ONESHOT | IRQF_SHARED, client->name, priv);
> > > + if (err)
> > > + return dev_err_probe(dev, err, "failed to register IRQ %d\n", client->irq);
> > > +
> > > + /* Extcon support is not vital for the charger to work */
> >
> > The comment is a bit missleading, because the current code seems to
> > make extcon support mandatory as far as I can tell.
> >
>
> Extcon is optional and charger interrupt will work fine without it,
> but this charger can only detect the fact of plug event, not the type
> of plug. So without extcon charging will always be done like from usb2
> pc port (default mode). Hence I have added extcon support here (which
> my device has and uses) to be able to set higher current if a
> dedicated charger is connected.
>
> > > + connector = of_parse_phandle(dev->of_node, "maxim,usb-connector", 0);
> > > + extcon = of_get_parent(connector);
> > > + of_node_put(connector);
> > > +
> > > + priv->edev = extcon_find_edev_by_node(extcon);
> > > + of_node_put(extcon);
> > > + if (IS_ERR(priv->edev))
> > > + return 0;
> > > +
> > > + err = devm_delayed_work_autocancel(dev, &priv->extcon_work,
> > > + max8971_extcon_evt_worker);
> > > + if (err)
> > > + return dev_err_probe(dev, err, "failed to add extcon evt stop action\n");
> > > +
> > > + priv->extcon_nb.notifier_call = extcon_get_charger_type;
> > > +
> > > + err = devm_extcon_register_notifier_all(dev, priv->edev, &priv->extcon_nb);
> > > + if (err)
> > > + return dev_err_probe(dev, err, "failed to register notifier\n");
> > > +
> > > + /* Initial configuration work with 1 sec delay */
> > > + schedule_delayed_work(&priv->extcon_work, msecs_to_jiffies(1000));
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int __maybe_unused max8971_resume(struct device *dev)
> > > +{
> > > + struct i2c_client *client = to_i2c_client(dev);
> > > + struct max8971_data *priv = i2c_get_clientdata(client);
> > > +
> > > + irq_wake_thread(client->irq, priv);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static SIMPLE_DEV_PM_OPS(max8971_pm_ops, NULL, max8971_resume);
> > > +
> > > +static const struct of_device_id max8971_match_ids[] = {
> > > + { .compatible = "maxim,max8971" },
> > > + { /* sentinel */ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, max8971_match_ids);
> > > +
> > > +static const struct i2c_device_id max8971_i2c_id[] = {
> > > + { "max8971" },
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, max8971_i2c_id);
> > > +
> > > +static struct i2c_driver max8971_driver = {
> > > + .driver = {
> > > + .name = "max8971-charger",
> > > + .of_match_table = max8971_match_ids,
> > > + .pm = &max8971_pm_ops,
> > > + },
> > > + .probe = max8971_probe,
> > > + .id_table = max8971_i2c_id,
> > > +};
> > > +module_i2c_driver(max8971_driver);
> > > +
> > > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@xxxxxxxxx>");
> > > +MODULE_DESCRIPTION("MAX8971 Charger Driver");
> > > +MODULE_LICENSE("GPL");
> >
> > Otherwise LGTM.
> >
>
> Thank you for your suggestions, I will apply and test them.
>
> > -- Sebastian
