On 12/03/2024 12:23, Nikita Travkin wrote:
Bryan O'Donoghue писал(а) 12.03.2024 16:58:
On 12/03/2024 08:42, Nikita Travkin wrote:
Acer Aspire 1 is a Snapdragon 7c based laptop. It uses an embedded
controller to perform a set of various functions, such as:
- Battery and charger monitoring;
- Keyboard layout control (i.e. fn_lock settings);
- USB Type-C DP alt mode HPD notifications;
- Laptop lid status.
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@xxxxxxx>
---
drivers/platform/arm64/Kconfig | 16 +
drivers/platform/arm64/Makefile | 2 +
drivers/platform/arm64/acer-aspire1-ec.c | 555 +++++++++++++++++++++++++++++++
You should be listing yourself as a maintainer for a driver you contribute.
I always believed that being in the AUTHOR() at the bottom of the driver
would guarantee me being in CC for patches, which so far worked great,
thus I was always hesitent adding extra entries in MAINTAINERS.
There's no such rule that I'm aware of there.
scripts/get_maintainer.pl won't list a driver author for the CC list
This is a substantial body of code, you should own it upstream.
+ case ASPIRE_EC_EVENT_FG_INF_CHG:
+ /* Notify (\_SB.I2C3.BAT1, 0x81) // Information Change */
fallthrough;
Hm I believe this would not warn since it's just two values for the same
code, just with an extra comment inbetween?
True
+ case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+ val->intval = le16_to_cpu(ddat.voltage_now) * 1000;
+ break;
+
+ case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+ val->intval = le16_to_cpu(sdat.voltage_design) * 1000;
+ break;
+
+ case POWER_SUPPLY_PROP_CHARGE_NOW:
+ val->intval = le16_to_cpu(ddat.capacity_now) * 1000;
+ break;
+
+ case POWER_SUPPLY_PROP_CHARGE_FULL:
+ val->intval = le16_to_cpu(sdat.capacity_full) * 1000;
+ break;
You could stick this "* 1000" stuff in a macro
acpi/battery.c also explicitly sets the multiplier so I think it's the
"common" way to do this.
common != nice
Purely aesthetics but anyway consider decomposing the replication down.
+
+ case POWER_SUPPLY_PROP_CAPACITY:
+ val->intval = le16_to_cpu(ddat.capacity_now) * 100;
+ val->intval /= le16_to_cpu(sdat.capacity_full);
+ break;
+
+ case POWER_SUPPLY_PROP_CURRENT_NOW:
+ val->intval = (s16)le16_to_cpu(ddat.current_now) * 1000;
+ break;
+
+ case POWER_SUPPLY_PROP_PRESENT:
+ val->intval = !!(ddat.flags & ASPIRE_EC_FG_FLAG_PRESENT);
+ break;
+
+ case POWER_SUPPLY_PROP_SCOPE:
+ val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
+ break;
+
+ case POWER_SUPPLY_PROP_MODEL_NAME:
+ if (sdat.model_id - 1 < ARRAY_SIZE(aspire_ec_bat_psy_battery_model))
+ val->strval = aspire_ec_bat_psy_battery_model[sdat.model_id - 1];
+ else
+ val->strval = "Unknown";
+ break;
+
+ case POWER_SUPPLY_PROP_MANUFACTURER:
+ if (sdat.vendor_id - 3 < ARRAY_SIZE(aspire_ec_bat_psy_battery_vendor))
+ val->strval = aspire_ec_bat_psy_battery_vendor[sdat.vendor_id - 3];
How does this -3 offset not underflow ?
vendor_id here is unsigned so the if check would actually overflow,
though explaining that I guess it's better to be explicit there and let
the compiler optimize that check away anyway... I will update the if
condition with an extra (id >= 3).
What's the "3" about though, that's what's not jumping out at me here.
Seems a bit dodgy to me - can you add a comment to the code to explain ? Its not immediately obvious the -3 is OK.
Also could you take an index instead of replicating the -value stepdown each time ?
int myindex = sdat.model_id - 1;
if (myindex < someconstraint)
strval = somearry[myindex];
I decided against adding a dedicated index variable since there is only
one actual use for each, so it's easy to see where it goes.
But you do it twice which is why I'm suggesting take an index and do it
once.
Then add
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
