Message-ID: <c2b5e0c8-651f-426e-ae96-13857313997a@t-8ch.de>
Date: Sun, 22 Dec 2024 13:09:28 +0100
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Joshua Grisham <josh@...huagrisham.com>
Cc: Hans de Goede <hdegoede@...hat.com>, ilpo.jarvinen@...ux.intel.com, 
	W_Armin@....de, 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 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@...hat.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.

> 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

Powered by blists - more mailing lists