Re: [PATCH v3] platform/x86: samsung-galaxybook: Add samsung-galaxybook driver

[Armin Wolf <W_Armin@gmx.de>]

Am 22.12.24 um 13:09 schrieb Thomas Weißschuh:

> Hi Joshua,
>
> On 2024-12-19 18:31:22+0100, Joshua Grisham wrote:
>> Thank you both Thomas and Hans for your review and comments! I am
>> working on a v4 of the patch but had a few questions which I wanted to
>> clarify (they can also come after in a v5 etc in case I managed to get
>> this ready to go before anyone has the time to confirm and/or clarify
>> some things!).
> Keep them coming :-)
>
>> Den tis 17 dec. 2024 kl 15:23 skrev Hans de Goede <hdegoede@redhat.com>:
>>> On 16-Dec-24 5:46 PM, Thomas Weißschuh wrote:
>>>>> +Various hardware settings can be controlled by the following sysfs attributes:
>>>>> +
>>>>> +- ``allow_recording`` (allows or blocks usage of built-in camera and microphone)
>>>>> +- ``start_on_lid_open`` (power on automatically when opening the lid)
>>>>> +- ``usb_charge`` (allows USB ports to provide power even when device is off)
>>>> Non-standard sysfs attributes should be avoided where possible.
>>>> Userspace will need bespoke code to handle them.
>>>> This looks like it could be handled by the standard firmware_attributes
>>>> interface.
>>>> This would standardize discovery and usage.
>>> Ack this really feels like firmware-attributes. I would not be surprised
>>> if there are matching BIOS settings and if changing those also changes
>>> the sysfs files and likewise if the sysfs settings persist over reboot.
>>>
>> Yes 2 of these (not this "allow_recording" I think) are available via
>> BIOS and all 3 of them persist over restarts.
>>
>> Just so I am 100% clear what you mean here -- these type of attributes
>> should be created using the utilities available in
>> drivers/platform/x86/firmware_attributes_class.h so that they are
>> created under the path /sys/class/firmware-attributes/*/attributes/*/
>> ?
> Yes.
>
>> What exactly should they be named (any preference?) and should I also
>> add some documentation for them in
>> Documentation/ABI/testing/sysfs-class-firmware-attributes ?
> I think they are meant to be named consistently with what the native
> UEFI setup interface calls them.
> And yes, they should be documented.
>
>> I am fairly sure I understand the concept and can agree that it kind
>> of makes a lot of sense to be able to standardize the userspace
>> interface, especially for attributes which do the exact same thing
>> across different vendors/devices (unless it just as easily possible to
>> go based on some pattern matching e.g. like is done in udev and upower
>> with "*kbd_backlight*" etc) but as of now it looks like the only
>> examples implemented are for thinklmi, dell-wmi, and hp-bioscfg that I
>> can see so far?
> The firmware-attributes don't really have a standardized semantic.
> Here the standardization is more about the discovery and interaction.
> Somebody can build a generic UI to change these settings, without the UI
> knowing anything about what the setting actually does.
>
> If the setting maps to a another, more specific interface, that should
> be used.
>
>> Before, I had tried to look through all of the various platform/x86
>> drivers and harmonize which names I picked for these sysfs attributes
>> (that is how I landed on "usb_charge" and "start_on_lid_open" as I
>> recall correctly) but I am not aware of any existing userspace tools
>> which are looking for anything like these (apart for
>> driver/vendor-specific utilities). Any recommendation from the very
>> wise people here would certainly be appreciated for these :)
> [..] Snip, I don't feel qualify to comment on the input bits.

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.

>
>> Other notifications that I am wondering what the "right" way to handle
>> / using the right interface:
> [..]
>
>> - When the battery charge control end threshold is reached, there is
>> an ACPI notification on this device as well that is the one I have
>> marked "ACPI_NOTIFY_BATTERY_STATE_CHANGED" -- the Samsung background
>> apps pop up a custom OSD that basically says something to the effect
>> that their "Battery saver is protecting the battery by stopping
>> charging" (can't remember the exact verbiage) and they change the
>> battery icon, but without doing anything else in my driver currently
>> the battery still reports state of "charging" even though it just sits
>> constantly at the percentage (and has the charging icon in GNOME etc).
>> I have seen the event come and go occasionally when I did not expect
>> it, but my working theory is that maybe it is if/when the battery
>> starts charging again if it dips too far below the target "end
>> threshold" and then notifies again when the threshold has been
>> reached. Armin also mentioned this before in a different mail; I guess
>> I would hope/expect there is an event or a function I could call to
>> have the state reflected correctly but I would not want that it
>> negatively impacts the normal behavior of charging the battery itself
>> (just that the state/icon would change would be ideal! as it functions
>> perfectly, it is just that the state and icon are not accurate).
> Optimally the ACPI event would integrate with the ACPI battery driver.
> See the handling of POWER_SUPPLY_STATUS_NOT_CHARGING in
> drivers/acpi/battery.c.
> Does the battery report the current rate as 0 when limiting?
> Then something like acpi_battery_handle_discharging() could be used.
>
>
> Thomas

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

Thanks,
Armin Wolf