Message-ID: <CAPVz0n10HskS+khCm58F7UUPu6XcYRs69dQtLPOQqoQS9MEL0g@mail.gmail.com>
Date: Wed, 12 Mar 2025 11:15:38 +0200
From: Svyatoslav Ryhel <clamor95@...il.com>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, 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 р. о 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.

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

Powered by blists - more mailing lists