Re: [PATCH v4 3/4] platform: arm64: Add Acer Aspire 1 embedded controller driver

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

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@trvn.ru>
>>> ---
>>>    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@linaro.org>