Message-ID: <CAMF+KeYEarSLRyhS1o4AA7-Uy40_v0k2Bebhjzo9yKF=2k0jQQ@mail.gmail.com>
Date: Thu, 26 Dec 2024 17:08:13 +0100
From: Joshua Grisham <josh@...huagrisham.com>
To: Armin Wolf <W_Armin@....de>
Cc: Thomas Weißschuh <linux@...ssschuh.net>, 
	Joshua Grisham <josh@...huagrisham.com>, Hans de Goede <hdegoede@...hat.com>, 
	ilpo.jarvinen@...ux.intel.com, platform-driver-x86@...r.kernel.org, 
	corbet@....net, linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] platform/x86: samsung-galaxybook: Add
 samsung-galaxybook driver

Hi Armin,

Den ons 25 dec. 2024 kl 21:23 skrev Armin Wolf <W_Armin@....de>:
>
> Harmonization with other platform driver regarding the firmware attribute names will
> likely not result in any benefits. I am not aware of any utility profiting from such
> a thing.
>

I just posted a v4 of the patch where I have moved the device sysfs
attributes to firmware-attributes as requested by both Thomas and
Hans. With this change, I did try to "harmonize" the names as best as
I could anyway, recognizing that there are several device drivers in
the tree which have actually currently implemented these as device
sysfs attributes instead, but in theory they could be moved over to
use the same firmware attribute instead ? e.g. all of the ones that
have a usb_charge or usb_charging device attribute could be moved to
use this new usb_charging firmware-attribute if it is desired at a
later time?

Here were the examples I could find online for different devices that
drove my rationale for the names that I picked:

1) and maybe easiest to start with: for what I had as
"allow_recording" I thought would be better to change to be a "block"
as per Samsung's own documentation and implementation, but then to
also try and harmonize how it would be interpreted by other tools
including this switch input event, I chose to switch this entire
feature to the name "camera lens cover". Hopefully this is ok! The
only "weird" thing in my opinion is that on this particular device,
there is a side-effect that the microphone is also blocked as well
(which is not obviously indicated by this naming, but not totally hard
to understand, either).

2)  for what I had called "start on lid open", I found the following
vendors with various names for the same kind of feature:
HP: "Power On When Lid is Opened"
Dell/Alienware: "Power On Lid Open"
Huawei: "Auto Power On"
Samsung Galaxy Book: "Startup when Lid is Opened"
Lenovo: "Flip to Start"

So for this, I tweaked my driver's name slightly to try and fit the
middle ground between all of these, and went with the name
power_on_lid_open

3) for what I had called "usb charge":
Lenovo: Always On USB
Asus: USB power delivery in Soft Off state (S5)
Dell: USB PowerShare
Razer: USB Charge Function
Existing drivers for Chrome, LG, and samsung-laptop call it "USB
Charge" (long name "USB Charge in Sleep Mode")
Fujitsu Lifebook: "Anytime USB Charge"
Acer: "Power-off USB Charging"
HP: "USB Charging"
Samsung Galaxy Book series: "USB Charging"

In effort to make this as descriptive as possible and mostly fit all
of these, I went with the name usb_charging

All 3 of these I have added to the ABI-Testing documentation and
removed my local documentation for them.

Anyway I am hoping that all of this makes sense and helps, but please
feel free anyone to say if I got any of this wrong :)

> [...]
>
> Can you send me the acpidump of your machine (with the fan bug)? I can check how the
> ACPI battery handles this case internally.
>

I looked further into this one as well. What I see is that the battery
device itself actually also receives a notification when this happens
(so 3x events are generated for the same thing on this device; a
keypress on the keyboard device, an ACPI notification on their "SCAI"
settings device, and an ACPI notification on the battery device
itself), and based on what I can tell in the code in ACPI core is
already "updating" status based on what is read from _BST from the
battery device. I think then that the "real" problem is that my
device's _BST is not reporting these parts correctly as per
https://uefi.org/specs/ACPI/6.5_A/10_Power_Source_and_Power_Meter_Devices.html#bst-battery-status
and especially in regards to the part about reporting Battery Charge
Limiting state.

So for now I have made it so that this driver will aim that the
notification to the battery device is the one that "matters": the
keyboard event is now filtered away, and I removed any special
handling of the extra ACPI settings device notification (but do send
the notification along as an ACPI netlink event in case anyone really
wants to create their own notification or action from it). I thought
to also try to take this one up with Samsung; though I am skeptical
that they would fix it for existing devices like mine,  maybe it will
help with newer or future devices! In the end it is not too big of a
deal as the device works well, it is only the icon and status that are
not being updated correctly.

> Thanks,
> Armin Wolf
>

There are lots of other adjustments in the v4 of this patch in attempt
to address all of the issues brought up by everyone, so everyone is of
course welcome to take a look and see if I managed to catch everything
or not, and I can adjust from there!

Thanks again!

Best regards,
Joshua

Powered by blists - more mailing lists