Date: Tue, 12 Mar 2024 19:09:39 +0500
From: Nikita Travkin <nikita@...n.ru>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
Cc: Hans de Goede <hdegoede@...hat.com>, Ilpo Järvinen
 <ilpo.jarvinen@...ux.intel.com>, Sebastian Reichel <sre@...nel.org>, Rob
 Herring <robh+dt@...nel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
 cros-qcom-dts-watchers@...omium.org, Andy Gross <agross@...nel.org>, Bjorn
 Andersson <andersson@...nel.org>, Konrad Dybcio <konrad.dybcio@...aro.org>,
 Rob Herring <robh@...nel.org>, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
 platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v4 3/4] platform: arm64: Add Acer Aspire 1 embedded
 controller driver

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@...n.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.
> 

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@...aro.org>

Ack, given there is also >0, will add str_index variable for the offset
index.

Thanks!
Nikita

Powered by blists - more mailing lists