Messages in this thread | | | Date | Thu, 7 Dec 2023 20:24:53 +0100 | Subject | Re: [PATCH 2/3] power: supply: Add Acer Aspire 1 embedded controller driver | From | Konrad Dybcio <> |
| |
On 12/7/23 12:20, Nikita Travkin wrote: > Acer Aspire 1 is a Snapdragon 7c based laptop. It uses an embedded > controller to control the charging and battery management, as well as to > perform a set of misc functions. > > Unfortunately, while all this functionality is implemented in ACPI, it's > currently not possible to use ACPI to boot Linux on such Qualcomm > devices. To allow Linux to still support the features provided by EC, > this driver reimplments the relevant ACPI parts. This allows us to boot > the laptop with Device Tree and retain all the features. > > Signed-off-by: Nikita Travkin <nikita@trvn.ru> > --- [...]
> + case POWER_SUPPLY_PROP_CAPACITY: > + val->intval = le16_to_cpu(ddat.capacity_now) * 100 > + / le16_to_cpu(sdat.capacity_full); It may be just my OCD and im not the maintainer here, but I'd do /= here
[...]
> + case POWER_SUPPLY_PROP_MODEL_NAME: > + if (sdat.model_id - 1 < ARRAY_SIZE(aspire_ec_psy_battery_model)) > + val->strval = aspire_ec_psy_battery_model[sdat.model_id - 1]; > + else > + val->strval = "Unknown"; Would it make sense to print the model_id that's absent from the LUT here and similarly below?
> + break; > + > + case POWER_SUPPLY_PROP_MANUFACTURER: > + if (sdat.vendor_id - 3 < ARRAY_SIZE(aspire_ec_psy_battery_vendor)) > + val->strval = aspire_ec_psy_battery_vendor[sdat.vendor_id - 3]; > + else > + val->strval = "Unknown"; > + break; > + > + default: > + return -EINVAL; > + } Another ocd trip, i'd add a newline before return
> + return 0; > +} [...]
> + /* > + * The original ACPI firmware actually has a small sleep in the handler. > + * > + * It seems like in most cases it's not needed but when the device > + * just exits suspend, our i2c driver has a brief time where data > + * transfer is not possible yet. So this delay allows us to suppress > + * quite a bunch of spurious error messages in dmesg. Thus it's kept. Ouch.. do you think i2c-geni needs fixing on this part?
[...]
> + switch (id) { > + case 0x0: /* No event */ > + break; Is this a NOP/watchdog sort of thing?
[...]
> + > +static struct i2c_driver aspire_ec_driver = { > + .driver = { > + .name = "aspire-ec", > + .of_match_table = aspire_ec_of_match, > + .pm = pm_sleep_ptr(&aspire_ec_pm_ops), > + }, > + .probe = aspire_ec_probe, > + .id_table = aspire_ec_id, Since it's tristate, I'd expect an entry for .remove_new here
Konrad
| |