| 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
| ||
|
Message-ID: <CANpmGNv6Yd30YXat1OAGN-Fs495DOAnttQEtOayq+wLpHMXS0w@mail.gmail.com>
Date: Sun, 18 Jan 2026 23:59:12 +1100
From: Michael Reeves <michael.reeves077@...il.com>
To: Janne Grunau <j@...nau.net>
Cc: Nick Chan <towinchenmi@...il.com>, Sebastian Reichel <sre@...nel.org>, Sven Peter <sven@...nel.org>,
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 v3 1/2] power: supply: Add macsmc-power driver for Apple Silicon
Hi Janne,
Thank you for the feedback.
On Sat, Jan 17, 2026 at 11:26 PM Janne Grunau <j@...nau.net> wrote:
>
> On Thu, Jan 15, 2026 at 06:08:15PM +1100, Michael Reeves via B4 Relay wrote:
> > From: Michael Reeves <michael.reeves077@...il.com>
>
> I think the driver is overall similar to the downstream AsahiLinux
> driver so please keep Hector as author.
>
Will change in v4!
> > This driver provides battery and AC status monitoring for Apple Silicon
> > Macs via the SMC (System Management Controller). It supports
> > reporting capacity, voltage, current, and charging status.
> >
> > Co-developed-by: Hector Martin <marcan@...can.st>
> > Signed-off-by: Hector Martin <marcan@...can.st>
>
> The downstream driver a quite a few more Co-developed-by:/Sobs. When I
> squashed the commits I decided to err on the safe side and included
> commit authors from incremental patches as Co-developed-by: Why did you
> drop those?
Oh sorry, the UI just displayed marcan because, as you said, the
commit was squashed to a single From: marcan commit in the main
upstream branch.
I see them now on closer inspection and will add SOBs/CDBs in v4,
except for the contributor who added unconditional available charge behaviour
reporting, because that code is not in the refactored upstream driver I've made
here - it's different and conditional now. Unless you think it's
fairer and safer to
also include that?
[...]
> > +
> > +struct macsmc_power {
> > + struct device *dev;
> > + struct apple_smc *smc;
> > +
> > + struct power_supply_desc ac_desc;
> > + struct power_supply_desc batt_desc;
> > +
> > + struct power_supply *batt;
> > + struct power_supply *ac;
> > +
> > + char model_name[MAX_STRING_LENGTH];
> > + char serial_number[MAX_STRING_LENGTH];
> > + char mfg_date[MAX_STRING_LENGTH];
> > +
> > + bool has_chwa;
> > + bool has_chls;
> > + bool has_ch0i;
> > + bool has_ch0c;
> > + bool has_chte;
>
> I think comments what these keys do and in which firmware version the
> appeared or vanished would be helpful here. I can Help with digging
> through the history.
>
Will add in v4, thank you for the suggestion. WIll let you know on IRC
if any help is needed but can see the full history now.
> > +
> > + u8 num_cells;
> > + int nominal_voltage_mv;
> > +
> > + struct notifier_block nb;
> > + struct work_struct critical_work;
> > + bool shutdown_started;
> > +};
> > +
> > +static int macsmc_battery_get_status(struct macsmc_power *power)
> > +{
> > + u64 nocharge_flags;
> > + u32 nopower_flags;
> > + u16 ac_current;
> > + int charge_limit = 0;
> > + bool limited = false;
> > + bool flag;
> > + int ret;
> > +
> > + /*
> > + * Fallbacks exist for keys that may disappear in future hardware.
> > + * CHCE/CHCC/BSFC/CHSC are considered fundamental; absence is an error.
> > + */
>
> Why did you rewrite this (and many other) comments? I don't think this
> is an improvement over the comment in the downstream driver.
>
I tried to make comments clearer / more concise where needed / make more sense
for upstream, so that it would be easier to understand for a wider audience, if
there's any changes you don't like, please let me know and I can restore the
original comment if that would make more sense.
[...]
> > +
> > +static int macsmc_battery_set_charge_behaviour(struct macsmc_power *power, int val)
> > +{
> > + int ret;
> > +
> > + /* First, reset all inhibitors to a known-good 'auto' state */
>
> why? I wouldn't expect this function to make any if `val` is not
> supported. If has_ch0i is not true
> POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE will reset inhibit charge
> and return -EOPNOTSUPP which is unexpected.
> Resetting the values first and then change them to the actual desired
> values doesn't make much sense either.
> Yes, it's a little annoying to make the same calls in multiple cases of
> the switch statement but you could move the `power->has_*` checks and
> the smc writes to a single line.
>
I'll refactor this to handle the specific writes within each switch
case for v4, thank you for the pickup!
> > + if (power->has_ch0i) {
> > + ret = apple_smc_write_u8(power->smc, SMC_KEY(CH0I), 0);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + if (power->has_chte) {
> > + ret = apple_smc_write_u32(power->smc, SMC_KEY(CHTE), 0);
> > + if (ret)
> > + return ret;
> > + } else if (power->has_ch0c) {
> > + ret = apple_smc_write_u8(power->smc, SMC_KEY(CH0C), 0);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + switch (val) {
> > + case POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO:
> > + return 0;
> > +
> > + 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;
> > +
> > + case POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE:
> > + if (!power->has_ch0i)
> > + return -EOPNOTSUPP;
> > + return apple_smc_write_u8(power->smc, SMC_KEY(CH0I), 1);
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int macsmc_battery_get_date(const char *s, int *out)
> > +{
> > + if (!isdigit(s[0]) || !isdigit(s[1]))
> > + return -EOPNOTSUPP;
> > +
> > + *out = (s[0] - '0') * 10 + s[1] - '0';
> > + return 0;
> > +}
> > +
> > +static int macsmc_battery_get_capacity_level(struct macsmc_power *power)
> > +{
> > + bool flag;
> > + u32 val;
> > + int ret;
> > +
> > + /* Check for emergency shutdown condition */
> > + if (apple_smc_read_u32(power->smc, SMC_KEY(BCF0), &val) >= 0 && val)
> > + return POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL;
> > +
> > + /* Check AC status for whether we could boot in this state */
> > + if (apple_smc_read_u32(power->smc, SMC_KEY(ACSt), &val) >= 0) {
> > + if (!(val & ACSt_CAN_BOOT_IBOOT))
> > + return POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL;
> > +
> > + if (!(val & ACSt_CAN_BOOT_AP))
> > + return POWER_SUPPLY_CAPACITY_LEVEL_LOW;
> > + }
> > +
> > + ret = apple_smc_read_flag(power->smc, SMC_KEY(BSFC), &flag);
>
> the comment in the downstream driver what the flag is helpful
>
Will add back (maybe with clarification/cleanup) in v4, thank you!
> > + .set_property = macsmc_battery_set_property,
> > + .property_is_writeable = macsmc_battery_property_is_writeable,
> > +};
> > +
> > +static int macsmc_ac_get_property(struct power_supply *psy,
> > + enum power_supply_property psp,
> > + union power_supply_propval *val)
> > +{
> > + struct macsmc_power *power = power_supply_get_drvdata(psy);
> > + int ret = 0;
> > + u16 vu16;
> > + u32 vu32;
> > +
> > + switch (psp) {
> > + case POWER_SUPPLY_PROP_ONLINE:
> > + ret = apple_smc_read_u32(power->smc, SMC_KEY(CHIS), &vu32);
> > + val->intval = !!vu32;
> > + break;
> > + case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> > + ret = apple_smc_read_u16(power->smc, SMC_KEY(AC-n), &vu16);
> > + val->intval = vu16 * 1000;
> > + break;
> > + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> > + ret = apple_smc_read_u16(power->smc, SMC_KEY(AC-i), &vu16);
> > + val->intval = vu16 * 1000;
> > + break;
> > + case POWER_SUPPLY_PROP_INPUT_POWER_LIMIT:
> > + ret = apple_smc_read_u32(power->smc, SMC_KEY(ACPW), &vu32);
> > + val->intval = vu32 * 1000;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static const struct power_supply_desc macsmc_ac_desc_template = {
> > + .name = "macsmc-ac",
> > + .type = POWER_SUPPLY_TYPE_MAINS,
> > + .get_property = macsmc_ac_get_property,
> > +};
> > +
> > +static void macsmc_power_critical_work(struct work_struct *wrk)
> > +{
> > + struct macsmc_power *power = container_of(wrk, struct macsmc_power, critical_work);
> > + u16 bitv, b0av;
> > + u32 bcf0;
> > +
> > + if (!power->batt)
> > + return;
> > +
> > + /*
> > + * EMERGENCY: Check voltage vs design minimum.
> > + * If we are below BITV, the battery is physically exhausted.
> > + * We must shut down NOW to protect the filesystem.
> > + */
> > + if (apple_smc_read_u16(power->smc, SMC_KEY(BITV), &bitv) >= 0 &&
> > + apple_smc_read_u16(power->smc, SMC_KEY(B0AV), &b0av) >= 0 &&
> > + b0av < bitv) {
> > + dev_emerg(power->dev,
> > + "Battery voltage (%d mV) below design minimum (%d mV)! Emergency shutdown.\n",
> > + b0av, bitv);
> > + kernel_power_off();
> > + return;
>
> I don't think we should return here. kernel_power_off() may do
> nothing in which case the orderly_poweroff(true) might save data.
> I would protect it as well via power->shutdown_started like in the
> downstream driver to avoid calling kernel_power_off() once per second
> until the battery dies or the system shutsdown.
>
Sounds good, will fix in v4. Note that the way orderly_poweroff(true) saves
data (if userspace fails to respond) is an emergency_sync() before
kernel_power_off() so we could also just add emergency_sync() to this
code path to achieve the same effect.
Re protection, it would probably be useful to, if we go down the route
of removing the return, have 2 separate flags for orderly vs non-orderly
shutdown started so we can "upgrade" a shutdown where needed as the comment
I added below clarifies.
> > + }
> > +
> > + /*
> > + * Avoid duplicate attempts at orderly shutdown.
> > + * Voltage check is above this as we may want to
> > + * "upgrade" an orderly shutdown to a critical power
> > + * off if voltage drops.
> > + */
> > + if (power->shutdown_started || system_state > SYSTEM_RUNNING)
> > + return;
> > +
> > + /*
> > + * Check if SMC flagged the battery as empty.
> > + * We trigger a graceful shutdown to let the OS save data.
> > + */
> > + if (apple_smc_read_u32(power->smc, SMC_KEY(BCF0), &bcf0) == 0 && bcf0 != 0) {
> > + power->shutdown_started = true;
> > + dev_crit(power->dev, "Battery critical (empty flag set). Triggering orderly shutdown.\n");
> > + orderly_poweroff(true);
> > + }
> > +}
> > +
[...]
> > +static int macsmc_power_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct apple_smc *smc = dev_get_drvdata(pdev->dev.parent);
> > + struct power_supply_config psy_cfg = {};
> > + struct macsmc_power *power;
> > + bool has_battery = false;
> > + bool has_ac_adapter = false;
> > + int ret = -ENODEV;
> > + bool flag;
> > + u16 vu16;
> > + u32 val32;
> > + enum power_supply_property *props;
> > + size_t nprops;
> > +
> > + if (!smc)
> > + return -ENODEV;
> > +
> > + power = devm_kzalloc(dev, sizeof(*power), GFP_KERNEL);
> > + if (!power)
> > + return -ENOMEM;
> > +
> > + power->dev = dev;
> > + power->smc = smc;
> > + dev_set_drvdata(dev, power);
> > +
> > + INIT_WORK(&power->critical_work, macsmc_power_critical_work);
> > + ret = devm_work_autocancel(dev, &power->critical_work, macsmc_power_critical_work);
> > + if (ret)
> > + return ret;
> > +
> > + /*
> > + * Check for battery presence.
> > + * B0AV is a fundamental key.
> > + */
> > + if (apple_smc_read_u16(power->smc, SMC_KEY(B0AV), &vu16) == 0 &&
> > + macsmc_battery_get_status(power) > POWER_SUPPLY_STATUS_UNKNOWN)
> > + has_battery = true;
> > +
> > + /*
> > + * Check for AC adapter presence.
> > + * CHIS is a fundamental key.
> > + */
> > + if (apple_smc_key_exists(smc, SMC_KEY(CHIS)))
> > + has_ac_adapter = true;
> > +
>
> I think a short circuit check for !(has_battery || has_ac_adapter) would
> make sense here. The setup code for props is quite long. It should
> return -ENODEV. ret is not -ENODEV. anymore since it was overwritten
> with the return value of devm_work_autocancel()
>
Thank you for the pickup, will fix in v4.
> > + if (has_battery) {
> > + power->batt_desc = macsmc_battery_desc_template;
> > + props = devm_kcalloc(dev, MACSMC_MAX_BATT_PROPS,
> > + sizeof(enum power_supply_property),
> > + GFP_KERNEL);
>
> I don't like the dynamic allocation for the props. I think we can
> currently get way with static property arrays and so we should do that.
> See my comments below
>
I think dynamic allocation is useful for backward and forward
compatibility in future (while I have been writing this Draft, Nick
Chan has sent a reply echoing this
from a pre-M1 bringup perspective), and is also cleaner / less hacky,
so would also prefer if it's kept, but happy to change to static if that's the
consensus that emerges.
> > + if (!props)
> > + return -ENOMEM;
> > +
> > + nprops = 0;
> > +
> > + /* Fundamental properties */
> > + props[nprops++] = POWER_SUPPLY_PROP_STATUS;
> > + props[nprops++] = POWER_SUPPLY_PROP_PRESENT;
> > + props[nprops++] = POWER_SUPPLY_PROP_VOLTAGE_NOW;
> > + props[nprops++] = POWER_SUPPLY_PROP_CURRENT_NOW;
> > + props[nprops++] = POWER_SUPPLY_PROP_POWER_NOW;
> > + props[nprops++] = POWER_SUPPLY_PROP_CAPACITY;
> > + props[nprops++] = POWER_SUPPLY_PROP_CAPACITY_LEVEL;
> > + props[nprops++] = POWER_SUPPLY_PROP_TEMP;
> > + props[nprops++] = POWER_SUPPLY_PROP_CYCLE_COUNT;
> > + props[nprops++] = POWER_SUPPLY_PROP_HEALTH;
> > + props[nprops++] = POWER_SUPPLY_PROP_SCOPE;
> > + props[nprops++] = POWER_SUPPLY_PROP_MODEL_NAME;
> > + props[nprops++] = POWER_SUPPLY_PROP_SERIAL_NUMBER;
> > + props[nprops++] = POWER_SUPPLY_PROP_MANUFACTURE_YEAR;
> > + props[nprops++] = POWER_SUPPLY_PROP_MANUFACTURE_MONTH;
> > + props[nprops++] = POWER_SUPPLY_PROP_MANUFACTURE_DAY;
> > +
> > + /* Extended properties usually present */
> > + props[nprops++] = POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW;
> > + props[nprops++] = POWER_SUPPLY_PROP_TIME_TO_FULL_NOW;
> > + props[nprops++] = POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN;
> > + props[nprops++] = POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN;
> > + props[nprops++] = POWER_SUPPLY_PROP_VOLTAGE_MIN;
> > + props[nprops++] = POWER_SUPPLY_PROP_VOLTAGE_MAX;
> > + props[nprops++] = POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT;
> > + props[nprops++] = POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX;
> > + props[nprops++] = POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE;
> > + props[nprops++] = POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN;
> > + props[nprops++] = POWER_SUPPLY_PROP_CHARGE_FULL;
> > + props[nprops++] = POWER_SUPPLY_PROP_CHARGE_NOW;
> > + props[nprops++] = POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN;
> > + props[nprops++] = POWER_SUPPLY_PROP_ENERGY_FULL;
> > + props[nprops++] = POWER_SUPPLY_PROP_ENERGY_NOW;
> > + props[nprops++] = POWER_SUPPLY_PROP_CHARGE_COUNTER;
> > +
> > + /* Detect features based on key availability */
> > + if (apple_smc_key_exists(smc, SMC_KEY(CHTE)))
> > + power->has_chte = true;
> > + if (apple_smc_key_exists(smc, SMC_KEY(CH0C)))
> > + power->has_ch0c = true;
> > + if (apple_smc_key_exists(smc, SMC_KEY(CH0I)))
> > + power->has_ch0i = true;
> > +
> > + /* Reset "Optimised Battery Charging" flags to default state */
> > + if (power->has_chte)
> > + apple_smc_write_u32(smc, SMC_KEY(CHTE), 0);
> > + else if (power->has_ch0c)
> > + apple_smc_write_u8(smc, SMC_KEY(CH0C), 0);
> > +
> > + if (power->has_ch0i)
> > + apple_smc_write_u8(smc, SMC_KEY(CH0I), 0);
> > +
> > + apple_smc_write_u8(smc, SMC_KEY(CH0K), 0);
> > + apple_smc_write_u8(smc, SMC_KEY(CH0B), 0);
> > +
> > + /* Configure charge behaviour if supported */
> > + if (power->has_ch0i || power->has_ch0c || power->has_chte) {
> > + props[nprops++] = POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR;
>
> This is the only dynamic battery prop. With a static array we can simply
> put it at the end of the array and set batt_desc.num_properties to array
> size - 1 if none of the keys are available.
>
See above.
> > +
> > + power->batt_desc.charge_behaviours =
> > + BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO);
> > +
> > + if (power->has_ch0i)
> > + power->batt_desc.charge_behaviours |=
> > + BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE);
> > +
> > + if (power->has_chte || power->has_ch0c)
> > + power->batt_desc.charge_behaviours |=
> > + BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE);
> > + }
> > +
> > + /* Detect charge limit method (CHWA vs CHLS) */
> > + if (apple_smc_read_flag(power->smc, SMC_KEY(CHWA), &flag) == 0)
> > + power->has_chwa = true;
> > + else if (apple_smc_read_u16(power->smc, SMC_KEY(CHLS), &vu16) >= 0)
> > + power->has_chls = true;
> > +
> > + power->batt_desc.properties = props;
> > + power->batt_desc.num_properties = nprops;
> > +
> > + /* Fetch identity strings */
> > + apple_smc_read(smc, SMC_KEY(BMDN), power->model_name,
> > + sizeof(power->model_name) - 1);
> > + apple_smc_read(smc, SMC_KEY(BMSN), power->serial_number,
> > + sizeof(power->serial_number) - 1);
> > + apple_smc_read(smc, SMC_KEY(BMDT), power->mfg_date,
> > + sizeof(power->mfg_date) - 1);
> > +
> > + apple_smc_read_u8(power->smc, SMC_KEY(BNCB), &power->num_cells);
> > + power->nominal_voltage_mv = MACSMC_NOMINAL_CELL_VOLTAGE_MV * power->num_cells;
> > +
> > + /* Enable critical shutdown notifications by reading status once */
> > + apple_smc_read_u32(power->smc, SMC_KEY(BCF0), &val32);
> > +
> > + psy_cfg.drv_data = power;
> > + power->batt = devm_power_supply_register(dev, &power->batt_desc, &psy_cfg);
> > + if (IS_ERR(power->batt)) {
> > + dev_err_probe(dev, PTR_ERR(power->batt),
> > + "Failed to register battery\n");
> > + /* Don't return failure yet; try AC registration first */
> > + power->batt = NULL;
> > + }
> > + } else {
> > + dev_dbg(dev, "No battery detected.\n");
>
> I would drop this
>
Will do in v4, thank you!
> > + }
> > +
> > + if (has_ac_adapter) {
> > + power->ac_desc = macsmc_ac_desc_template;
> > + props = devm_kcalloc(dev, MACSMC_MAX_AC_PROPS,
> > + sizeof(enum power_supply_property),
> > + GFP_KERNEL);
> > + if (!props)
> > + return -ENOMEM;
> > +
> > + nprops = 0;
> > +
> > + /* Online status is fundamental */
> > + props[nprops++] = POWER_SUPPLY_PROP_ONLINE;
> > +
> > + /* Input power limits are usually available */
> > + if (apple_smc_key_exists(power->smc, SMC_KEY(ACPW)))
> > + props[nprops++] = POWER_SUPPLY_PROP_INPUT_POWER_LIMIT;
> > +
> > + /* macOS 15.4+ firmware dropped legacy AC keys (AC-n, AC-i) */
> > + if (apple_smc_read_u16(power->smc, SMC_KEY(AC-n), &vu16) >= 0) {
> > + props[nprops++] = POWER_SUPPLY_PROP_VOLTAGE_NOW;
> > + props[nprops++] = POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT;
> > + }
>
> we can do the same trick here assuming AC-n and AC-i are only present if
> ACPW is present.
>
See above comment.
> > + power->ac_desc.properties = props;
> > + power->ac_desc.num_properties = nprops;
> > +
> > + psy_cfg.drv_data = power;
> > + power->ac = devm_power_supply_register(dev, &power->ac_desc, &psy_cfg);
> > + if (IS_ERR(power->ac)) {
> > + dev_err_probe(dev, PTR_ERR(power->ac),
> > + "Failed to register AC adapter\n");
> > + power->ac = NULL;
> > + }
> > + } else {
> > + dev_dbg(dev, "No A/C adapter detected.\n");
>
> same here
Will drop, thanks!
> > + }
> > +
> > + /* Final check: did we register anything? */
> > + if (!power->batt && !power->ac)
> > + return ret;
>
> See my comment about the changed value of ret above
>
Yes, will fix in v4, thank you again for the pickup!
[...]
Thank you so much for your time!
Best regards,
Michael
On Sun, Jan 18, 2026 at 11:49 PM Janne Grunau <j@...nau.net> wrote:
>
> On Sun, Jan 18, 2026 at 08:19:38PM +0800, Nick Chan wrote:
> >
> >
> > On 17/1/2026 20:26, Janne Grunau wrote:
> > > On Thu, Jan 15, 2026 at 06:08:15PM +1100, Michael Reeves via B4 Relay wrote:
> > >> From: Michael Reeves <michael.reeves077@...il.com>
> > >
> > > I think the driver is overall similar to the downstream AsahiLinux
> > > driver so please keep Hector as author.
> > >
> > >> This driver provides battery and AC status monitoring for Apple Silicon
> > >> Macs via the SMC (System Management Controller). It supports
> > >> reporting capacity, voltage, current, and charging status.
> > >>
> > >> Co-developed-by: Hector Martin <marcan@...can.st>
> > >> Signed-off-by: Hector Martin <marcan@...can.st>
> > >
> > > The downstream driver a quite a few more Co-developed-by:/Sobs. When I
> > > squashed the commits I decided to err on the safe side and included
> > > commit authors from incremental patches as Co-developed-by: Why did you
> > > drop those?
> > >
> > >> Reviewed-by: Neal Gompa <neal@...pa.dev>
> > >> Signed-off-by: Michael Reeves <michael.reeves077@...il.com>
> > >> ---
> > >> MAINTAINERS | 1 +
> > >> drivers/power/supply/Kconfig | 11 +
> > >> drivers/power/supply/Makefile | 1 +
> > >> drivers/power/supply/macsmc-power.c | 834 ++++++++++++++++++++++++++++++++++++
> > >> 4 files changed, 847 insertions(+)
> > >>
> > >> diff --git a/drivers/power/supply/macsmc-power.c b/drivers/power/supply/macsmc-power.c
> > >> new file mode 100644
> > >> index 000000000000..9b3faefe7a45
> > >> --- /dev/null
> > >> +++ b/drivers/power/supply/macsmc-power.c
> > >> @@ -0,0 +1,834 @@
>
> ...
>
> > >> +static int macsmc_power_probe(struct platform_device *pdev)
> > >> +{
> > >> + struct device *dev = &pdev->dev;
> > >> + struct apple_smc *smc = dev_get_drvdata(pdev->dev.parent);
> > >> + struct power_supply_config psy_cfg = {};
> > >> + struct macsmc_power *power;
> > >> + bool has_battery = false;
> > >> + bool has_ac_adapter = false;
> > >> + int ret = -ENODEV;
> > >> + bool flag;
> > >> + u16 vu16;
> > >> + u32 val32;
> > >> + enum power_supply_property *props;
> > >> + size_t nprops;
> > >> +
> > >> + if (!smc)
> > >> + return -ENODEV;
> > >> +
> > >> + power = devm_kzalloc(dev, sizeof(*power), GFP_KERNEL);
> > >> + if (!power)
> > >> + return -ENOMEM;
> > >> +
> > >> + power->dev = dev;
> > >> + power->smc = smc;
> > >> + dev_set_drvdata(dev, power);
> > >> +
> > >> + INIT_WORK(&power->critical_work, macsmc_power_critical_work);
> > >> + ret = devm_work_autocancel(dev, &power->critical_work, macsmc_power_critical_work);
> > >> + if (ret)
> > >> + return ret;
> > >> +
> > >> + /*
> > >> + * Check for battery presence.
> > >> + * B0AV is a fundamental key.
> > >> + */
> > >> + if (apple_smc_read_u16(power->smc, SMC_KEY(B0AV), &vu16) == 0 &&
> > >> + macsmc_battery_get_status(power) > POWER_SUPPLY_STATUS_UNKNOWN)
> > >> + has_battery = true;
> > >> +
> > >> + /*
> > >> + * Check for AC adapter presence.
> > >> + * CHIS is a fundamental key.
> > >> + */
> > >> + if (apple_smc_key_exists(smc, SMC_KEY(CHIS)))
> > >> + has_ac_adapter = true;
> > >> +
> > >
> > > I think a short circuit check for !(has_battery || has_ac_adapter) would
> > > make sense here. The setup code for props is quite long. It should
> > > return -ENODEV. ret is not -ENODEV. anymore since it was overwritten
> > > with the return value of devm_work_autocancel()
> > >
> > >> + if (has_battery) {
> > >> + power->batt_desc = macsmc_battery_desc_template;
> > >> + props = devm_kcalloc(dev, MACSMC_MAX_BATT_PROPS,
> > >> + sizeof(enum power_supply_property),
> > >> + GFP_KERNEL);
> > >
> > > I don't like the dynamic allocation for the props. I think we can
> > > currently get way with static property arrays and so we should do that.
> > > See my comments below
> >
> > Dynamic allocation is needed to properly add support pre-M1 support,
> > as the SMCs in them misses a few keys, yet in iOS 16 the charge limit
> > (CHWA) keys are added. In a similar way, there really is no sane way
> > to add any new properties for M1/M2/M3 with static allocation. So I
> > would prefer if dynamic allocation is kept.
>
> ack, I wasn't really thinking beyond the code in front of me. Let's go
> with dynamic allocations. A check that nprops doesn't exceed
> MACSMC_MAX_BATT_PROPS should be added then.
>
> Janne
Powered by blists - more mailing lists