Message-ID: <CANpmGNvBvasft9U3_=G2=B9VF1Q0P7aG8QsdLF7H+NJH9oTfKw@mail.gmail.com>
Date: Thu, 15 Jan 2026 17:35:20 +1100
From: Michael Reeves <michael.reeves077@...il.com>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: Sven Peter <sven@...nel.org>, Janne Grunau <j@...nau.net>, Neal Gompa <neal@...pa.dev>, 
	Lee Jones <lee@...nel.org>, linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org, 
	asahi@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org, 
	Hector Martin <marcan@...can.st>
Subject: 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
<sebastian.reichel@...labora.com> wrote:
> > +     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.

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

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

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

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

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

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

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

> > +                     /* 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.

> > +     /* 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.

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

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

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

Powered by blists - more mailing lists