linux-kernel - Re: [PATCH v4 2/2] power: supply: Add support for Maxim MAX8971 charger

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAPVz0n1HG2qw6qgtEe0ok+9MVkzVhHkaO2vkoELwQAGpm-045w@mail.gmail.com>
Date: Wed, 12 Mar 2025 14:28:04 +0200
From: Svyatoslav Ryhel <clamor95@...il.com>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: linux-pm@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/2] power: supply: Add support for Maxim MAX8971 charger

ср, 12 бер. 2025 р. о 11:15 Svyatoslav Ryhel <clamor95@...il.com> пише:
>
> ср, 12 бер. 2025 р. о 10:50 Sebastian Reichel
> <sebastian.reichel@...labora.com> пише:
> >
> > 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], &regval);
> > > +     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@...il.com>");
> > > +MODULE_DESCRIPTION("MAX8971 Charger Driver");
> > > +MODULE_LICENSE("GPL");
> >
> > Otherwise LGTM.
> >
>
> Thank you for your suggestions, I will apply and test them.
>
> > -- Sebastian