Bryan O'Donoghue писал(а) 12.03.2024 17:44: > 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. > Hm, ack, will add an entry in MAINTAINERS for this. >>>> + 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 > (Adding anyway given Ilpo also thinks it's better than not) >>>> + 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. > (Adding a macro for this) >>>> + >>>> + 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. > Ah, well... the 3 comes from a big if/elseif table in the decompiled dsdt which starts at 3... I will add a small comment near it. >> >>> 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> Ack, given there is also >0, will add str_index variable for the offset index. Thanks! Nikita
