Hi Joshua, On 7-Jan-25 12:36 PM, Joshua Grisham wrote: > Den mån 6 jan. 2025 kl 12:50 skrev Hans de Goede <hdegoede@xxxxxxxxxx>: >> >> Hi, >> >> Sorry for the very slow reply. >> > > Hi Hans, not to worry and appreciate that you take the time! I have > been in good and capable hands with several eager and helpful > reviewers who are helping to push me in the right direction :) I > acknowledge everything from your message but will respond to only > certain points below: > >>> What exactly should they be named (any preference?) >> >> No preference for the naming, the firmware-attributes API just >> specifies how userspace can find out if something is >> an int/string/enumand valid values / range. Not naming of >> the attributes. >> > > Now that I have had some time to get over jet lag and craziness from > the last few weeks, I think this has finally sunk in and I am with you > all on firmware-attributes :) I have decided to revert the naming a > bit on what I had most recently called "camera_lens_cover" to what > Samsung device users will be familiar with: "block_recording"; the > rest of the attributes within the enumeration type group will exist so > hopefully it will be pretty self-explanatory and also help to soothe > some of the "unexpected side effects" confusion. It will still report > SW_CAMERA_LENS_COVER to its own "Camera Lens Cover" input device, but > the firmware-attribute itself under samsung-galaxybook will be called > "block_recording". > >>> Other notifications that I am wondering what the "right" way to handle >>> / using the right interface: >>> >>> - Are there better events to use for these which these devices are >>> reporting for "ACPI_NOTIFY_DEVICE_ON_TABLE" and >>> "ACPI_NOTIFY_DEVICE_OFF_TABLE" , i.e. some kind of standard >>> "switch"-like notification that the motion sensor in the device has >>> detected that it has been placed or lifted from a flat surface? >> >> The thinkpad_apci driver has /sys/bus/platform/devices/thinkpad_acpi/dytc_lapmode >> which will read 1 when the laptop thinks it is not on a table (and thus will >> limit max temperatures a bit to avoid someone getting a hot lap when >> actually having the laptop on their lap. >> >> I'm not aware of any other drivers having something similar. I do think >> that power-profiles-daemon checks the dytc_lapmode thing, so it might >> be good to have some standard interface for this, but that would need >> to be designed / decided upon . >> >> For v1 of your patch I would just dev_dbg() log these events and >> otherwise do nothing. >> > > What I have landed on here is to forward along / generate acpi netlink > events against the platform driver name (samsung-galaxybook) for these > events, so for now users should be able to use acpid or similar > userspace tools if they really want to act on this, but otherwise > nothing else is being done by the driver. Please let me know if this > sounds like an ok approach or not and I can adjust accordingly. Also, > of course, if there is a different direction in the future where a > more formalized "userspace interface" for this is established, this > can be changed! Generating acpi netlink events for this sounds good to me. >>> - 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). >> >> ATM there is no userspace API to communicate e.g. "charging stopped >> due to charge end threshold" and this is the first time I hear about >> us getting events from the hw for this. >> >> So for this one too I would say just dev_dbg() log the event and >> otherwise do nothing. >> >> We can always add an API later if we have an idea how userspace >> could / will use this. >> > > Similar to above is where I landed for this one as well: what I have > done for now is forward along these notifications as acpi netlink > events on samsung-galaxybook, so users should be able to see and act > on them if they really want to, but otherwise they are completely > "new" / custom events that should not disturb anything (and as said > before, this can be changed later if/when any formalized userpace > interface is established for this kind of notification event). Same as above, generating acpi netlink events for this sounds good to me. Regard, Hans
