| Message ID | 20231207-aspire1-ec-v1-0-ba9e1c227007@trvn.ru |
|---|---|
| Headers | show |
| Series | power: supply: Acer Aspire 1 embedded controller | expand |
On 07/12/2023 12:20, Nikita Travkin wrote: > Add binding for the EC found in the Acer Aspire 1 laptop. > > Signed-off-by: Nikita Travkin <nikita@trvn.ru> > --- > .../bindings/power/supply/acer,aspire1-ec.yaml | 73 ++++++++++++++++++++++ > 1 file changed, 73 insertions(+) > > + > + acer,media-keys-on-top: > + description: Configure the keyboard layout to use media features of > + the fn row when the fn key is not pressed. The firmware may choose > + to add this property when user selects the fn mode in the firmware > + setup utility. > + type: boolean > + > + connector: > + $ref: /schemas/connector/usb-connector.yaml# > + > + properties: > + reg: > + maxItems: 1 You cannot have it... Add it to the example and see the results. > + > + unevaluatedProperties: false It should be fine to drop this as well, so you don't need anything else than $ref. Best regards, Krzysztof
Krzysztof Kozlowski писал(а) 07.12.2023 21:56: > On 07/12/2023 12:20, Nikita Travkin wrote: >> Add binding for the EC found in the Acer Aspire 1 laptop. >> >> Signed-off-by: Nikita Travkin <nikita@trvn.ru> >> --- >> .../bindings/power/supply/acer,aspire1-ec.yaml | 73 ++++++++++++++++++++++ >> 1 file changed, 73 insertions(+) >> > > >> + >> + acer,media-keys-on-top: >> + description: Configure the keyboard layout to use media features of >> + the fn row when the fn key is not pressed. The firmware may choose >> + to add this property when user selects the fn mode in the firmware >> + setup utility. >> + type: boolean >> + >> + connector: >> + $ref: /schemas/connector/usb-connector.yaml# >> + >> + properties: >> + reg: >> + maxItems: 1 > > You cannot have it... Add it to the example and see the results. > Gah, I had connector@0 at first with all the num-cells etc, later simplified it, but missed this part... >> + >> + unevaluatedProperties: false > > It should be fine to drop this as well, so you don't need anything else > than $ref. > Will drop this too then. Thanks! Nikita > > > Best regards, > Krzysztof
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
Konrad Dybcio писал(а) 08.12.2023 00:24: > 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 Hm you're right, this did look a bit ugly to me when I split the line (it was 101/100), Will probably use /= to make it nicer in v2. > > [...] > >> + 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? > The original ACPI code returns "Unknown" like this when the value is not in the table. I suppose I could warn here but not sure how useful it would be... And since this is a rather "hot" path, would need to warn only once, so extra complexity for a very unlikely situation IMO. >> + 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 > Yeah I agree here, missed this. Will add in v2. >> + 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? Not sure, it seems like when we exit suspend, this handler gets triggered before geni (or it's dependencies?) is considered "awake" (my guess is when the clocks are still off): [ 119.246867] PM: suspend entry (s2idle) (...) [ 119.438052] printk: Suspending console(s) (use no_console_suspend to debug) [ 119.942498] geni_i2c 888000.i2c: error turning SE resources:-13 [ 119.942550] aspire-ec 2-0076: Failed to read event id: -EACCES (...) [ 119.942657] geni_i2c 888000.i2c: error turning SE resources:-13 [ 119.942666] aspire-ec 2-0076: Failed to read event id: -EACCES (...) [ 120.881452] PM: suspend exit FWIW it doesn't seem to be a big problem since this is a level interrupt, so it will be retried until the event can be cleared, but since ACPI also has the sleep, I'm happy to inherit in and suppress a couple of red lines :) > > [...] > >> + switch (id) { >> + case 0x0: /* No event */ >> + break; > Is this a NOP/watchdog sort of thing? > This is a NOP, yes. I think I was hitting spurious interrupts once or twice so I suppressed this. > [...] > >> + >> +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 > All the resources I allocate are devm_ so I believe I shouldn't need to clean anything up on remove... Thanks for the review! Nikita > Konrad
The laptop contains an embedded controller that provides a set of features: - Battery and charger monitoring - USB Type-C DP alt mode HPD monitoring - Lid status detection - Small amount of keyboard configuration* [*] The keyboard is handled by the same EC but it has a dedicated i2c bus and is already enabled. This port only provides fn key behavior configuration. Unfortunately, while all this functionality is implemented in ACPI, it's currently not possible to use ACPI to boot Linux on such Qualcomm devices. Thus this series implements and enables a new driver that provides support for the EC features. The EC would be one of the last pieces to get almost full support for the Acer Aspire 1 laptop in the upstream Linux kernel. This series is similar to the EC driver for Lenovo Yoga C630, proposed in [1] but seemingly never followed up... [1] https://lore.kernel.org/all/20230205152809.2233436-1-dmitry.baryshkov@linaro.org/ Signed-off-by: Nikita Travkin <nikita@trvn.ru> --- Nikita Travkin (3): dt-bindings: power: supply: Add Acer Aspire 1 EC power: supply: Add Acer Aspire 1 embedded controller driver arm64: dts: qcom: acer-aspire1: Add embedded controller .../bindings/power/supply/acer,aspire1-ec.yaml | 73 ++++ arch/arm64/boot/dts/qcom/sc7180-acer-aspire1.dts | 40 +- drivers/power/supply/Kconfig | 14 + drivers/power/supply/Makefile | 1 + drivers/power/supply/acer-aspire1-ec.c | 432 +++++++++++++++++++++ 5 files changed, 559 insertions(+), 1 deletion(-) --- base-commit: 8e00ce02066e8f6f1ad5eab49a2ede7bf7a5ef64 change-id: 20231206-aspire1-ec-6b3d2cac1a72 Best regards,