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

[Hans de Goede <hdegoede@redhat.com>]

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@redhat.com>:
>>
>> 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