Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
MIME-Version: 1.0
Date:   Fri, 08 Dec 2023 15:31:27 +0500
From:   Nikita Travkin <nikita@trvn.ru>
To:     Konrad Dybcio <konrad.dybcio@linaro.org>
Cc:     Sebastian Reichel <sre@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        cros-qcom-dts-watchers@chromium.org,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH 2/3] power: supply: Add Acer Aspire 1 embedded controller
 driver
In-Reply-To: <71459bab-05b9-41f6-bb32-2b744736487d@linaro.org>
References: <20231207-aspire1-ec-v1-0-ba9e1c227007@trvn.ru>
 <20231207-aspire1-ec-v1-2-ba9e1c227007@trvn.ru>
 <71459bab-05b9-41f6-bb32-2b744736487d@linaro.org>
Message-ID: <8fe5cb8cecf92d98f2768b811deb3ea0@trvn.ru>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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