Re: [PATCH v2 1/2] power: supply: Add macsmc-power driver for Apple Silicon

Hi Sebastian,

Thanks for the review.

On Wed, Jan 14, 2026 at 1:10 PM Sebastian Reichel
[off-list ref] wrote:

quoted

+     case POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE:
+             if (power->has_chte)
+                     return apple_smc_write_u32(power->smc, SMC_KEY(CHTE), 1);
+             else if (power->has_ch0c)
+                     return apple_smc_write_u8(power->smc, SMC_KEY(CH0C), 1);
+             else
+                     return -EOPNOTSUPP;
+     break;

Indentation?

Fixed.

quoted

+     case POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE:
+             if (!power->has_ch0i)
+                     return -EOPNOTSUPP;
+     return apple_smc_write_u8(power->smc, SMC_KEY(CH0I), 1);

Indentation?

Fixed.

quoted

+     case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+             /* Calculate total max design voltage from per-cell nominal voltage */

Using the nominal voltage here doesn't make much sense. As the
comment above mentions not having nominal voltage data for the
battery, I suppose the comment is wrong and this uses the per-cell
**maximum** voltage?

You are correct. I've checked again just to confirm the comment is
definitely incorrect, and not the code, and the key reports ~4.4V.
Updated now to reflect that it is using the per-cell maximum voltage,
I think I got my words mixed up when writing the comment.

quoted

+     case POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD:
+             /*
+              * Read-only reflection of end threshold logic.
+              * Allowed to be written to avoid userspace confusion, but ignored.
+              */
+             return 0;

I think it's better to fix userspace and expose the value read-only.
In any case this creates an ABI and requires proper documentation in
Documentation/ABI/testing/sysfs-class-power. My suggestion would be
to drop this for now and handle it in a separate patchset, so that
ABI discussion don't block the remaining driver.

Agreed. I have dropped the threshold properties for now to avoid ABI churn.

quoted

+static int macsmc_power_probe(struct platform_device *pdev)
+{
+     struct apple_smc *smc = dev_get_drvdata(pdev->dev.parent);

You have a bunch of &pdev->dev further ahead, so I think it's
sensible to create 'struct dev = &pdev->dev;' and use it everywhere
in the probe function.

Done.

quoted

+             power->batt_desc.properties = power->batt_props;

It seems 'power->batt_props' is not used anywhere and you can just
use 'power->batt_desc.properties' directly?

Fixed. I've removed the struct members and assigned directly to desc.properties.

quoted

+             power->batt = devm_power_supply_register(&pdev->dev, &power->batt_desc, &psy_cfg);
+             if (IS_ERR(power->batt)) {
+                     ret = dev_err_probe(&pdev->dev, PTR_ERR(power->batt),
+                                         "Failed to register battery\n");

No need to assign ret; it is unused. But it becomes necessary with
a follow-up suggestion from me.

Fixed.

quoted

+             power->ac_desc.properties = power->ac_props;

Just like 'power->batt_props': You can drop power->ac_props and use
power->ac_desc.properties directly.

Done.

quoted

+                     /* If battery also failed or didn't exist, this is a fatal error */
+                     if (!power->batt)
+                             return ret;

You can just drop this check and instead rely on the "Final check"

Done.

quoted

+     /* Final check: did we register anything? */
+     if (!power->batt && !power->ac)
+             return -ENODEV;

You can just return ret here, if you assign -ENODEV as initial
value. In that case the correct error code will be returned in all
cases.

Done. I initialised ret to -ENODEV and updated the flow to return it.

quoted

+     power->nb.notifier_call = macsmc_power_event;
+     blocking_notifier_chain_register(&smc->event_handlers, &power->nb);
+
+     INIT_WORK(&power->critical_work, macsmc_power_critical_work);

This must happen before registering the event handler, which in
theory might instant-trigger an event creating a race condition. You
already have the correct order in the remove handler (which also
hints about this problem :)). After reordering you can further
simplify by just using devm_work_autocancel().

Fixed. Moved INIT_WORK to the top of probe and switched to devm_work_autocancel.

quoted

+static struct platform_driver macsmc_power_driver = {
+     .driver = {
+             .name = "macsmc-power",
+             .owner = THIS_MODULE,

There is no need to set platform_driver.driver.owner manually. It is
handled automatically, so please drop.

Dropped.

quoted

+MODULE_ALIAS("platform:macsmc-power");

Drop MODULE_ALIAS and instead add a platform_device_id table with
MODULE_DEVICE_TABLE(platform, <NAME>);

Done.

I will send v3 shortly.

Best regards,
Michael