Message-ID: <2786ed04-dfaf-4098-be21-7d32719e8c6b@gmx.de>
Date: Fri, 27 Dec 2024 00:35:15 +0100
From: Armin Wolf <W_Armin@....de>
To: Joshua Grisham <josh@...huagrisham.com>
Cc: Thomas Weißschuh <linux@...ssschuh.net>,
 Hans de Goede <hdegoede@...hat.com>, ilpo.jarvinen@...ux.intel.com,
 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

Am 26.12.24 um 17:08 schrieb Joshua Grisham:

> Hi Armin,
>
> Den ons 25 dec. 2024 kl 21:23 skrev Armin Wolf <W_Armin@....de>:
>> 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.
>>
> I just posted a v4 of the patch where I have moved the device sysfs
> attributes to firmware-attributes as requested by both Thomas and
> Hans. With this change, I did try to "harmonize" the names as best as
> I could anyway, recognizing that there are several device drivers in
> the tree which have actually currently implemented these as device
> sysfs attributes instead, but in theory they could be moved over to
> use the same firmware attribute instead ? e.g. all of the ones that
> have a usb_charge or usb_charging device attribute could be moved to
> use this new usb_charging firmware-attribute if it is desired at a
> later time?

Yes, but this should be done by the maintainers of those drivers. They know best how those
drivers work internally.

>
> Here were the examples I could find online for different devices that
> drove my rationale for the names that I picked:
>
> 1) and maybe easiest to start with: for what I had as
> "allow_recording" I thought would be better to change to be a "block"
> as per Samsung's own documentation and implementation, but then to
> also try and harmonize how it would be interpreted by other tools
> including this switch input event, I chose to switch this entire
> feature to the name "camera lens cover". Hopefully this is ok! The
> only "weird" thing in my opinion is that on this particular device,
> there is a side-effect that the microphone is also blocked as well
> (which is not obviously indicated by this naming, but not totally hard
> to understand, either).
>
> 2)  for what I had called "start on lid open", I found the following
> vendors with various names for the same kind of feature:
> HP: "Power On When Lid is Opened"
> Dell/Alienware: "Power On Lid Open"
> Huawei: "Auto Power On"
> Samsung Galaxy Book: "Startup when Lid is Opened"
> Lenovo: "Flip to Start"
>
> So for this, I tweaked my driver's name slightly to try and fit the
> middle ground between all of these, and went with the name
> power_on_lid_open

Sounds OK to me.

>
> 3) for what I had called "usb charge":
> Lenovo: Always On USB
> Asus: USB power delivery in Soft Off state (S5)
> Dell: USB PowerShare
> Razer: USB Charge Function
> Existing drivers for Chrome, LG, and samsung-laptop call it "USB
> Charge" (long name "USB Charge in Sleep Mode")
> Fujitsu Lifebook: "Anytime USB Charge"
> Acer: "Power-off USB Charging"
> HP: "USB Charging"
> Samsung Galaxy Book series: "USB Charging"
>
> In effort to make this as descriptive as possible and mostly fit all
> of these, I went with the name usb_charging

Sounds OK to me.

>
> All 3 of these I have added to the ABI-Testing documentation and
> removed my local documentation for them.
>
> Anyway I am hoping that all of this makes sense and helps, but please
> feel free anyone to say if I got any of this wrong :)
>
>> [...]
>>
>> Can you send me the acpidump of your machine (with the fan bug)? I can check how the
>> ACPI battery handles this case internally.
>>
> I looked further into this one as well. What I see is that the battery
> device itself actually also receives a notification when this happens
> (so 3x events are generated for the same thing on this device; a
> keypress on the keyboard device, an ACPI notification on their "SCAI"
> settings device, and an ACPI notification on the battery device
> itself), and based on what I can tell in the code in ACPI core is
> already "updating" status based on what is read from _BST from the
> battery device. I think then that the "real" problem is that my
> device's _BST is not reporting these parts correctly as per
> https://uefi.org/specs/ACPI/6.5_A/10_Power_Source_and_Power_Meter_Devices.html#bst-battery-status
> and especially in regards to the part about reporting Battery Charge
> Limiting state.
>
> So for now I have made it so that this driver will aim that the
> notification to the battery device is the one that "matters": the
> keyboard event is now filtered away, and I removed any special
> handling of the extra ACPI settings device notification (but do send
> the notification along as an ACPI netlink event in case anyone really
> wants to create their own notification or action from it). I thought
> to also try to take this one up with Samsung; though I am skeptical
> that they would fix it for existing devices like mine,  maybe it will
> help with newer or future devices! In the end it is not too big of a
> deal as the device works well, it is only the icon and status that are
> not being updated correctly.

I see, it could be that Windows does not support this charge limiting state, so vendors like Samsung
do not implement it.

Still i am OK with your approach here.

>> Thanks,
>> Armin Wolf
>>
> There are lots of other adjustments in the v4 of this patch in attempt
> to address all of the issues brought up by everyone, so everyone is of
> course welcome to take a look and see if I managed to catch everything
> or not, and I can adjust from there!

Thanks, i will take a look soon.

Armin Wolf

> Thanks again!
>
> Best regards,
> Joshua

Powered by blists - more mailing lists