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! > > - 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). > Regard, > > Hans > > Thanks again and hopefully I will try to post v5 of this patch soon! Best, Joshua
