Adding a new platform driver samsung-galaxybook

Adding a new platform driver samsung-galaxybook

[Joshua Grisham]

Hello! I have created a platform driver for Samsung Galaxy Book series
notebooks which has now gone through several iterations and
contributions from several other community members. Based on stars and
community involvement I would guess that the usage of the driver is
more than at least 100 users (if not more?) across multiple different
generations of devices and many different distributions, so hopefully
we have ironed out a lot of issues by now!

The existing driver samsung-laptop is of course somewhat
famous/notorious for how it works, but on newer devices (primarily
Samsung Galaxy Book series devices but does include a few others),
Samsung is using a new ACPI device called "SCAI" which is what this
new driver is built on, and the functionality is totally different.
There are only a few ACPI methods on this device that then actually
control a lot of different features; the "magic" is in building
various payloads to steer all of these different functionalities even
though it is often using the same ACPI method.

It is my opinion that, as we now have achieved some level of stability
with this driver, it would be good to try and get it added to mainline
as having it in mainline will add a lot of benefits (even larger
number of users who will gain benefit from this, better quality and
standardization with involvement from maintainers and the larger
community, etc).

I have myself tried to adhere to many of the existing patterns that
exist within other pdx86 drivers and the community has helped to find
and ensure (and in some cases even directly contributed to that)
various features are using standard interfaces such as with the
battery extension, platform profile, etc, in a way that seems to be
unified with existing platform drivers as well.

The driver code is currently located here:
https://github.com/joshuagrisham/samsung-galaxybook-extras/blob/main/samsung-galaxybook.c

As there are a few variants of what features are supported on
different devices (even devices with the same ACPI device id) then one
of the key principles that I have tried to now follow with the driver
is that each feature tries to check that it works or not (receives an
error code in the payload from the ACPI method) before "enabling" the
feature (creating a sysfs attribute or registering a new device etc)
when the module is probed and loaded.

Instead of just sending the code as-is in a new patch then I thought
to ask you all as the PDX86 maintainers if there is anything glaring
that you would prefer should be changed or re-designed before we try
to push this in as a patch and add this driver to the kernel?

You can see more background and what features are supported in the
README file here:
https://github.com/joshuagrisham/samsung-galaxybook-extras/blob/main/README.md

A few potentially "controversial" bits that I can highlight already now:

1. various failure messages or "unsupported features" write a warning
that directs users to create an issue in my own Github repository
instead of in Bugzilla -- maybe this is ok at the beginning but assume
it would be better to just remove some of this info from the message
and/or direct users to create a new bug in Bugzilla under the right
component there ?

2. some features where Kernel version are checked for handling some
things different for older versions of the kernel, but all of this I
would take away before submitting a patch

3. usage of the i8042 filter and ACPI hotkey notifications to handle a
few of the hotkey actions within the driver itself instead of just
emitting input events and allow userspace to handle the actions
(namely cycling through keyboard backlight levels, performance modes,
etc)

This last item (executing hotkey actions in kernel space) is not
totally unprecedented either, as I have seen there seems to exist
similar i8042 filters driving hotkey actions in msi-laptop,
toshiba_acpi, and dell-laptop and ACPI notifications from hotkeys
driving actions in several x86 platform drivers as well (dell-laptop,
acer-wmi, asus-laptop, ideapad-laptop, etc; this is an even more
common pattern than using an i8042 filter, it seems).

The problem with just emitting the "right" input events and relying on
the userspace to handle this stuff in the right way is that 1) there
are not really keycodes that exist for exactly the keys we want here
(even though "Keyboard Backlight Cycle" and some kind of "Performance
Mode" hotkeys are very common on laptops today) and 2) functionality
for how to handle these kind of events do not really support these
use-cases either (an example if you read through the discussion here:
https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/issues/41 and
especially several of the comments from Benjamin Berg, that
implementation of "Keyboard Backlight Toggle" is just on vs off and
does not cycle, and there should either be special handling of this or
a new key is created specifically for this purpose... this was from
5-6 years ago and the state of this has not changed since then from
what I can tell). It is because of these same problems that I assume
the existing PDX86 drivers do in fact implement some of this hotkey
action logic in the kernel space, in a similar way that I have tried
to do in this new samsung-galaxybook driver. I am not sure the
appetite for having even more of this pattern exist and/or if there
are any details of the implementation that you all would wish that I
should tweak a bit? I am very open to any kind of feedback on this.

Any other discussion or questions are of course welcome! Otherwise
and/or once things are to a point that is looking good then I can
create and submit a patch for this new driver.

Thank you!

Best regards,
Joshua Grisham

Re: Adding a new platform driver samsung-galaxybook

[Ilpo Järvinen]

On Mon, 18 Nov 2024, Joshua Grisham wrote:

> Hello! I have created a platform driver for Samsung Galaxy Book series
> notebooks which has now gone through several iterations and
> contributions from several other community members. Based on stars and
> community involvement I would guess that the usage of the driver is
> more than at least 100 users (if not more?) across multiple different
> generations of devices and many different distributions, so hopefully
> we have ironed out a lot of issues by now!
> 
> The existing driver samsung-laptop is of course somewhat
> famous/notorious for how it works, but on newer devices (primarily
> Samsung Galaxy Book series devices but does include a few others),
> Samsung is using a new ACPI device called "SCAI" which is what this
> new driver is built on, and the functionality is totally different.
> There are only a few ACPI methods on this device that then actually
> control a lot of different features; the "magic" is in building
> various payloads to steer all of these different functionalities even
> though it is often using the same ACPI method.
> 
> It is my opinion that, as we now have achieved some level of stability
> with this driver, it would be good to try and get it added to mainline
> as having it in mainline will add a lot of benefits (even larger
> number of users who will gain benefit from this, better quality and
> standardization with involvement from maintainers and the larger
> community, etc).
> 
> I have myself tried to adhere to many of the existing patterns that
> exist within other pdx86 drivers and the community has helped to find
> and ensure (and in some cases even directly contributed to that)
> various features are using standard interfaces such as with the
> battery extension, platform profile, etc, in a way that seems to be
> unified with existing platform drivers as well.
> 
> The driver code is currently located here:
> https://github.com/joshuagrisham/samsung-galaxybook-extras/blob/main/samsung-galaxybook.c
> 
> As there are a few variants of what features are supported on
> different devices (even devices with the same ACPI device id) then one
> of the key principles that I have tried to now follow with the driver
> is that each feature tries to check that it works or not (receives an
> error code in the payload from the ACPI method) before "enabling" the
> feature (creating a sysfs attribute or registering a new device etc)
> when the module is probed and loaded.
> 
> Instead of just sending the code as-is in a new patch then I thought
> to ask you all as the PDX86 maintainers if there is anything glaring
> that you would prefer should be changed or re-designed before we try
> to push this in as a patch and add this driver to the kernel?
> 
> You can see more background and what features are supported in the
> README file here:
> https://github.com/joshuagrisham/samsung-galaxybook-extras/blob/main/README.md
> 
> A few potentially "controversial" bits that I can highlight already now:
> 
> 1. various failure messages or "unsupported features" write a warning
> that directs users to create an issue in my own Github repository
> instead of in Bugzilla -- maybe this is ok at the beginning but assume
> it would be better to just remove some of this info from the message
> and/or direct users to create a new bug in Bugzilla under the right
> component there ?

Hi Joshua,

There are some pre-existing 'please report to' prints already.

For the different levels of information needed for production vs 
troubleshooting, I sugges you rely mostly on dynamic debugging though so 
if you need to have a deeper look, just ask the reporter to enable dyndbg 
for the driver so you don't need to have them spamming logs in normal 
situations.

There's nothing wrong with having github as bug reporting place as long 
as somebody keeps an eye on the reports, obviously. You can add a 
MAINTAINERS entry with B:.

I also suggest if you really plan to keep an eye on this driver, add 
yourself as the maintainer too as then tools would automatically add you 
to the relevant patches so you can comment/review. I can still manage the 
merging the patches into pdx86 repo and PRs to Linus so you don't need to 
worry about handling such a boring part of maintainership :-).

> 2. some features where Kernel version are checked for handling some
> things different for older versions of the kernel, but all of this I
> would take away before submitting a patch

Yes. It would be unnecessary dead code.

> 3. usage of the i8042 filter and ACPI hotkey notifications to handle a
> few of the hotkey actions within the driver itself instead of just
> emitting input events and allow userspace to handle the actions
> (namely cycling through keyboard backlight levels, performance modes,
> etc)
>
> This last item (executing hotkey actions in kernel space) is not
> totally unprecedented either, as I have seen there seems to exist
> similar i8042 filters driving hotkey actions in msi-laptop,
> toshiba_acpi, and dell-laptop and ACPI notifications from hotkeys
> driving actions in several x86 platform drivers as well (dell-laptop,
> acer-wmi, asus-laptop, ideapad-laptop, etc; this is an even more
> common pattern than using an i8042 filter, it seems).
>
> The problem with just emitting the "right" input events and relying on
> the userspace to handle this stuff in the right way is that 1) there
> are not really keycodes that exist for exactly the keys we want here
> (even though "Keyboard Backlight Cycle" and some kind of "Performance
> Mode" hotkeys are very common on laptops today) and 2) functionality
> for how to handle these kind of events do not really support these
> use-cases either (an example if you read through the discussion here:
> https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/issues/41 and
> especially several of the comments from Benjamin Berg, that
> implementation of "Keyboard Backlight Toggle" is just on vs off and
> does not cycle, and there should either be special handling of this or
> a new key is created specifically for this purpose... this was from
> 5-6 years ago and the state of this has not changed since then from
> what I can tell). It is because of these same problems that I assume
> the existing PDX86 drivers do in fact implement some of this hotkey
> action logic in the kernel space, in a similar way that I have tried
> to do in this new samsung-galaxybook driver. I am not sure the
> appetite for having even more of this pattern exist and/or if there
> are any details of the implementation that you all would wish that I
> should tweak a bit? I am very open to any kind of feedback on this.

That would be more Hans' domain of expertize so I suggest you ping
him with a private mail as he has decided to pay less attention on
pdx86 mailing list's posts:
  https://lore.kernel.org/platform-driver-x86/02b4c051-ea11-4fe9-876d-070d18cd84bf@redhat.com/

> Any other discussion or questions are of course welcome! Otherwise
> and/or once things are to a point that is looking good then I can
> create and submit a patch for this new driver.

Try to replace as many literals with named defines, it always helps
the code reader. Especially, if you end up adding a comment to explain a 
literal, you really would want to use a named define instead and forgo the 
comment as the code explains itself.

There are some indentation/alignment/style issues, e.g., braces around 
"else" and on continuation lines.

Be silent when no error occurs (=no non-dbg level prints).

Remove any double empty lines.

Don't split print strings to multiple lines to allow easier grep.


Please do realize it's much easier to pinpoint review comments when you 
post actual patch on the list so I hope you won't shy out too far from 
that. We do appreciate contributions and try to help you out to get them 
into shape for merging.


-- 
 i.

Re: Adding a new platform driver samsung-galaxybook

[Armin Wolf]

Am 18.11.24 um 14:51 schrieb Joshua Grisham:

> Hello! I have created a platform driver for Samsung Galaxy Book series
> notebooks which has now gone through several iterations and
> contributions from several other community members. Based on stars and
> community involvement I would guess that the usage of the driver is
> more than at least 100 users (if not more?) across multiple different
> generations of devices and many different distributions, so hopefully
> we have ironed out a lot of issues by now!

Nice work! Improving the hardware support for notebooks under linux is always
welcomed.

> The existing driver samsung-laptop is of course somewhat
> famous/notorious for how it works, but on newer devices (primarily
> Samsung Galaxy Book series devices but does include a few others),
> Samsung is using a new ACPI device called "SCAI" which is what this
> new driver is built on, and the functionality is totally different.
> There are only a few ACPI methods on this device that then actually
> control a lot of different features; the "magic" is in building
> various payloads to steer all of these different functionalities even
> though it is often using the same ACPI method.
>
> It is my opinion that, as we now have achieved some level of stability
> with this driver, it would be good to try and get it added to mainline
> as having it in mainline will add a lot of benefits (even larger
> number of users who will gain benefit from this, better quality and
> standardization with involvement from maintainers and the larger
> community, etc).

I support your initiative to upstream your driver. Having such a piece of software
upstream helps everyone.

> I have myself tried to adhere to many of the existing patterns that
> exist within other pdx86 drivers and the community has helped to find
> and ensure (and in some cases even directly contributed to that)
> various features are using standard interfaces such as with the
> battery extension, platform profile, etc, in a way that seems to be
> unified with existing platform drivers as well.
>
> The driver code is currently located here:
> https://github.com/joshuagrisham/samsung-galaxybook-extras/blob/main/samsung-galaxybook.c
>
> As there are a few variants of what features are supported on
> different devices (even devices with the same ACPI device id) then one
> of the key principles that I have tried to now follow with the driver
> is that each feature tries to check that it works or not (receives an
> error code in the payload from the ACPI method) before "enabling" the
> feature (creating a sysfs attribute or registering a new device etc)
> when the module is probed and loaded.

Sounds like a good strategy to me, being able to automatically detect which features are
available is usually better than having a very long quirk list.

> Instead of just sending the code as-is in a new patch then I thought
> to ask you all as the PDX86 maintainers if there is anything glaring
> that you would prefer should be changed or re-designed before we try
> to push this in as a patch and add this driver to the kernel?

After looking at the driver, i would advise you to drop the acpi_driver stuff and instead
implement the whole driver as a platform_driver. Does the kernel already create a suitable
platform device with the name "SAM04[number]:[number]"?

> You can see more background and what features are supported in the
> README file here:
> https://github.com/joshuagrisham/samsung-galaxybook-extras/blob/main/README.md
>
> A few potentially "controversial" bits that I can highlight already now:
>
> 1. various failure messages or "unsupported features" write a warning
> that directs users to create an issue in my own Github repository
> instead of in Bugzilla -- maybe this is ok at the beginning but assume
> it would be better to just remove some of this info from the message
> and/or direct users to create a new bug in Bugzilla under the right
> component there ?

As a general rule driver should be quiet if everything works, so unsupported features should not
result in a warning message. The other error messages should just contain the message without any
bugzilla/github links since stable kernel users might want to use the bugtrackers of their distro first.

>
> 2. some features where Kernel version are checked for handling some
> things different for older versions of the kernel, but all of this I
> would take away before submitting a patch

Yes, please remove any kernel version checks.

Thanks,
Armin Wolf

> 3. usage of the i8042 filter and ACPI hotkey notifications to handle a
> few of the hotkey actions within the driver itself instead of just
> emitting input events and allow userspace to handle the actions
> (namely cycling through keyboard backlight levels, performance modes,
> etc)
>
> This last item (executing hotkey actions in kernel space) is not
> totally unprecedented either, as I have seen there seems to exist
> similar i8042 filters driving hotkey actions in msi-laptop,
> toshiba_acpi, and dell-laptop and ACPI notifications from hotkeys
> driving actions in several x86 platform drivers as well (dell-laptop,
> acer-wmi, asus-laptop, ideapad-laptop, etc; this is an even more
> common pattern than using an i8042 filter, it seems).
>
> The problem with just emitting the "right" input events and relying on
> the userspace to handle this stuff in the right way is that 1) there
> are not really keycodes that exist for exactly the keys we want here
> (even though "Keyboard Backlight Cycle" and some kind of "Performance
> Mode" hotkeys are very common on laptops today) and 2) functionality
> for how to handle these kind of events do not really support these
> use-cases either (an example if you read through the discussion here:
> https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/issues/41 and
> especially several of the comments from Benjamin Berg, that
> implementation of "Keyboard Backlight Toggle" is just on vs off and
> does not cycle, and there should either be special handling of this or
> a new key is created specifically for this purpose... this was from
> 5-6 years ago and the state of this has not changed since then from
> what I can tell). It is because of these same problems that I assume
> the existing PDX86 drivers do in fact implement some of this hotkey
> action logic in the kernel space, in a similar way that I have tried
> to do in this new samsung-galaxybook driver. I am not sure the
> appetite for having even more of this pattern exist and/or if there
> are any details of the implementation that you all would wish that I
> should tweak a bit? I am very open to any kind of feedback on this.
>
> Any other discussion or questions are of course welcome! Otherwise
> and/or once things are to a point that is looking good then I can
> create and submit a patch for this new driver.
>
> Thank you!
>
> Best regards,
> Joshua Grisham
>

Re: Adding a new platform driver samsung-galaxybook

[Joshua Grisham]

Hi Ilpo and Armin and thank you for the replies; these are exactly the
kind of questions I was hoping for! :)

Den ons 20 nov. 2024 kl 13:00 skrev Armin Wolf <W_Armin@gmx.de>:
> After looking at the driver, i would advise you to drop the acpi_driver stuff and instead
> implement the whole driver as a platform_driver. Does the kernel already create a suitable
> platform device with the name "SAM04[number]:[number]"?

Yes, the kernel creates an ACPI device exactly as you asked.

As a bit of history: much of how this driver is structured came from
inspiration I took from existing platform/x86 drivers and this pattern
of having an acpi_driver followed by the platform_driver seems to be
more common than not from what I can see (except for specific cases
like the the WMI-specific drivers for example). I took a cursory look
and it does seem possible to do as you say, but I would still need to
grab a handle to the ACPI device somehow of course, and the full path
might be different for different models, so from what I can tell it
feels a bit more "clumsy" because I don't see any function which will
allow me to pass an array of acpi_device_id or strings but instead
would need to loop through the array and within the loop see if I get
a match and fetch the device that way (e.g. using something like
acpi_dev_get_first_match_dev()). Is this in fact the preferred
approach now? (and just so happens that many of the existing
platform/x86 drivers do not follow this design currently?) I am of
course happy to take this in either direction, but would just want to
know what the preference is so that I know what to do next on it!

> As a general rule driver should be quiet if everything works, so unsupported features should not
> result in a warning message. The other error messages should just contain the message without any
> bugzilla/github links since stable kernel users might want to use the bugtrackers of their distro first.

(Similar comment/question from both of you on this)
I have now gone over to using dynamic debug in the driver now instead
of a custom debug module parameter, and revamped quite a lot of the
print stuff. Now much of the previous information has been moved to
debug but there are still a few cases which warrant warn or error I
think.

One thing I feel like does actually add some value to print to INFO is
that, because how some of the dynamic features will work can vary for
different models, to me it seems like it would be a good thing to
actually tell the user about these specific things (e.g. only power
profiles x, y, z will be available and will be mapped to vendor
performance modes a, b, c, ... just as an example). Otherwise I can
understand how there would be a fair amount of confusion about what
actually happens when you press the hotkey or try and toggle the modes
using various services (power-profiles-daemon, etc..). To me, I see
these messages as similar to what prints to INFO when for example a
new input device is added, a new battery extension is added, etc. Does
this sound ok or should ALL of these kind of prints be moved to debug
instead?


I will also try to reach out to Hans directly to ask this question
regarding if it would make sense to handle hotkey work actions from
the platform driver or not. Otherwise, I feel like once I can resolve
these other two main questions I asked here ( 1) register acpi_driver
vs looping array to get the dev handle and 2) printing some of these
last dynamic things to info vs debug ), then I will go ahead and tidy
up the rest of the bits and send in a patch here.

Thank you again!

Best regards,
Joshua

>
> Am 18.11.24 um 14:51 schrieb Joshua Grisham:
>
> > Hello! I have created a platform driver for Samsung Galaxy Book series
> > notebooks which has now gone through several iterations and
> > contributions from several other community members. Based on stars and
> > community involvement I would guess that the usage of the driver is
> > more than at least 100 users (if not more?) across multiple different
> > generations of devices and many different distributions, so hopefully
> > we have ironed out a lot of issues by now!
>
> Nice work! Improving the hardware support for notebooks under linux is always
> welcomed.
>
> > The existing driver samsung-laptop is of course somewhat
> > famous/notorious for how it works, but on newer devices (primarily
> > Samsung Galaxy Book series devices but does include a few others),
> > Samsung is using a new ACPI device called "SCAI" which is what this
> > new driver is built on, and the functionality is totally different.
> > There are only a few ACPI methods on this device that then actually
> > control a lot of different features; the "magic" is in building
> > various payloads to steer all of these different functionalities even
> > though it is often using the same ACPI method.
> >
> > It is my opinion that, as we now have achieved some level of stability
> > with this driver, it would be good to try and get it added to mainline
> > as having it in mainline will add a lot of benefits (even larger
> > number of users who will gain benefit from this, better quality and
> > standardization with involvement from maintainers and the larger
> > community, etc).
>
> I support your initiative to upstream your driver. Having such a piece of software
> upstream helps everyone.
>
> > I have myself tried to adhere to many of the existing patterns that
> > exist within other pdx86 drivers and the community has helped to find
> > and ensure (and in some cases even directly contributed to that)
> > various features are using standard interfaces such as with the
> > battery extension, platform profile, etc, in a way that seems to be
> > unified with existing platform drivers as well.
> >
> > The driver code is currently located here:
> > https://github.com/joshuagrisham/samsung-galaxybook-extras/blob/main/samsung-galaxybook.c
> >
> > As there are a few variants of what features are supported on
> > different devices (even devices with the same ACPI device id) then one
> > of the key principles that I have tried to now follow with the driver
> > is that each feature tries to check that it works or not (receives an
> > error code in the payload from the ACPI method) before "enabling" the
> > feature (creating a sysfs attribute or registering a new device etc)
> > when the module is probed and loaded.
>
> Sounds like a good strategy to me, being able to automatically detect which features are
> available is usually better than having a very long quirk list.
>
> > Instead of just sending the code as-is in a new patch then I thought
> > to ask you all as the PDX86 maintainers if there is anything glaring
> > that you would prefer should be changed or re-designed before we try
> > to push this in as a patch and add this driver to the kernel?
>
> After looking at the driver, i would advise you to drop the acpi_driver stuff and instead
> implement the whole driver as a platform_driver. Does the kernel already create a suitable
> platform device with the name "SAM04[number]:[number]"?
>
> > You can see more background and what features are supported in the
> > README file here:
> > https://github.com/joshuagrisham/samsung-galaxybook-extras/blob/main/README.md
> >
> > A few potentially "controversial" bits that I can highlight already now:
> >
> > 1. various failure messages or "unsupported features" write a warning
> > that directs users to create an issue in my own Github repository
> > instead of in Bugzilla -- maybe this is ok at the beginning but assume
> > it would be better to just remove some of this info from the message
> > and/or direct users to create a new bug in Bugzilla under the right
> > component there ?
>
> As a general rule driver should be quiet if everything works, so unsupported features should not
> result in a warning message. The other error messages should just contain the message without any
> bugzilla/github links since stable kernel users might want to use the bugtrackers of their distro first.
>
> >
> > 2. some features where Kernel version are checked for handling some
> > things different for older versions of the kernel, but all of this I
> > would take away before submitting a patch
>
> Yes, please remove any kernel version checks.
>
> Thanks,
> Armin Wolf
>
> > 3. usage of the i8042 filter and ACPI hotkey notifications to handle a
> > few of the hotkey actions within the driver itself instead of just
> > emitting input events and allow userspace to handle the actions
> > (namely cycling through keyboard backlight levels, performance modes,
> > etc)
> >
> > This last item (executing hotkey actions in kernel space) is not
> > totally unprecedented either, as I have seen there seems to exist
> > similar i8042 filters driving hotkey actions in msi-laptop,
> > toshiba_acpi, and dell-laptop and ACPI notifications from hotkeys
> > driving actions in several x86 platform drivers as well (dell-laptop,
> > acer-wmi, asus-laptop, ideapad-laptop, etc; this is an even more
> > common pattern than using an i8042 filter, it seems).
> >
> > The problem with just emitting the "right" input events and relying on
> > the userspace to handle this stuff in the right way is that 1) there
> > are not really keycodes that exist for exactly the keys we want here
> > (even though "Keyboard Backlight Cycle" and some kind of "Performance
> > Mode" hotkeys are very common on laptops today) and 2) functionality
> > for how to handle these kind of events do not really support these
> > use-cases either (an example if you read through the discussion here:
> > https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/issues/41 and
> > especially several of the comments from Benjamin Berg, that
> > implementation of "Keyboard Backlight Toggle" is just on vs off and
> > does not cycle, and there should either be special handling of this or
> > a new key is created specifically for this purpose... this was from
> > 5-6 years ago and the state of this has not changed since then from
> > what I can tell). It is because of these same problems that I assume
> > the existing PDX86 drivers do in fact implement some of this hotkey
> > action logic in the kernel space, in a similar way that I have tried
> > to do in this new samsung-galaxybook driver. I am not sure the
> > appetite for having even more of this pattern exist and/or if there
> > are any details of the implementation that you all would wish that I
> > should tweak a bit? I am very open to any kind of feedback on this.
> >
> > Any other discussion or questions are of course welcome! Otherwise
> > and/or once things are to a point that is looking good then I can
> > create and submit a patch for this new driver.
> >
> > Thank you!
> >
> > Best regards,
> > Joshua Grisham
> >

Re: Adding a new platform driver samsung-galaxybook

[Kurt Borja]

On Fri, Nov 22, 2024 at 06:29:57PM +0100, Joshua Grisham wrote:
> Hi Ilpo and Armin and thank you for the replies; these are exactly the
> kind of questions I was hoping for! :)
> 
> Den ons 20 nov. 2024 kl 13:00 skrev Armin Wolf <W_Armin@gmx.de>:
> > After looking at the driver, i would advise you to drop the acpi_driver stuff and instead
> > implement the whole driver as a platform_driver. Does the kernel already create a suitable
> > platform device with the name "SAM04[number]:[number]"?
> 
> Yes, the kernel creates an ACPI device exactly as you asked.

Hi!

If there is a suitable platform device, your platform driver already has
an acpi_match_table, thus you can get your acpi_device with
ACPI_COMPANION or your handler with ACPI_HANDLER. Check [1] for an
example.

~ Kurt

[1] https://elixir.bootlin.com/linux/v6.12/source/drivers/platform/x86/wmi.c#L1239

> [...]

Re: Adding a new platform driver samsung-galaxybook

[Joshua Grisham]

Den fre 22 nov. 2024 kl 21:25 skrev Kurt Borja <kuurtb@gmail.com>:
> If there is a suitable platform device, your platform driver already has
> an acpi_match_table, thus you can get your acpi_device with
> ACPI_COMPANION or your handler with ACPI_HANDLER. Check [1] for an
> example.

Thank you Kurt! I looked into this and I think it makes more sense to
me now. Also it seems the existing driver ideapad-laptop is also quite
similar to this pattern and potentially a good reference, as well.

One thing more that I have found while looking at it from this
perspective now: in addition to the acpi_driver stuff, the driver is
also creating and registering a totally new platform_device and that
is currently the "interface" I have created (via sysfs) to control
some of the settings (e.g. "start_on_lid_open" etc). After an initial
local draft of the changes, it became apparent to me that even this
extra platform_device is not needed, and everything can work from the
existing ACPI device ID-based platform device (SAM0429:00, etc).

The downside to this is that users with these devices will not have a
fixed name for controlling some of these ACPI settings via sysfs
attributes on the device (depending on which model, they will have a
different platform device name). For example, like this (using
existing platform_device):

cat /sys/devices/platform/SAM0429\:00/start_on_lid_open

Instead of this (creating new platform_device like currently exists in
the code):

cat /sys/devices/platform/samsung-galaxybook/start_on_lid_open

I guess to me having this be based on ACPI Device ID and differing per
device feels "less nice" compared to having a nice "user friendly"
path that feels a bit more obvious. Is it preferred to just use the
existing platform_device based on the ACPI device ID instead of
creating a new "virtual" platform_device with a nicer name for
purposes like settings sysfs attributes be more "user friendly" like
this?

Another alternative is that I could move these kind of sysfs
attributes to driver attributes instead; then if I am guessing
correctly then it would be like this:

cat /sys/bus/platform/drivers/samsung-galaxybook/start_on_lid_open

But then I do not know if having this kind of sysfs attribute (which
actually will query and write values to the device itself using ACPI
methods) feel correct as "driver" attributes ? Maybe it does not
matter and I am just over thinking it :)

Any preference on these two questions? Again, to recap:
1) Yes or no to create new platform_device even though one already
exists with the ACPI device ID as it's name?
2) If using existing platform_device, should these kind of sysfs
attributes be under the dynamic device ID-based platform_device or is
it ok / make sense to move them to driver attributes?

> [...]

Re: Adding a new platform driver samsung-galaxybook

[Armin Wolf]

Am 23.11.24 um 18:58 schrieb Joshua Grisham:

> Den fre 22 nov. 2024 kl 21:25 skrev Kurt Borja <kuurtb@gmail.com>:
>> If there is a suitable platform device, your platform driver already has
>> an acpi_match_table, thus you can get your acpi_device with
>> ACPI_COMPANION or your handler with ACPI_HANDLER. Check [1] for an
>> example.
> Thank you Kurt! I looked into this and I think it makes more sense to
> me now. Also it seems the existing driver ideapad-laptop is also quite
> similar to this pattern and potentially a good reference, as well.
>
> One thing more that I have found while looking at it from this
> perspective now: in addition to the acpi_driver stuff, the driver is
> also creating and registering a totally new platform_device and that
> is currently the "interface" I have created (via sysfs) to control
> some of the settings (e.g. "start_on_lid_open" etc). After an initial
> local draft of the changes, it became apparent to me that even this
> extra platform_device is not needed, and everything can work from the
> existing ACPI device ID-based platform device (SAM0429:00, etc).
>
> The downside to this is that users with these devices will not have a
> fixed name for controlling some of these ACPI settings via sysfs
> attributes on the device (depending on which model, they will have a
> different platform device name). For example, like this (using
> existing platform_device):
>
> cat /sys/devices/platform/SAM0429\:00/start_on_lid_open
>
> Instead of this (creating new platform_device like currently exists in
> the code):
>
> cat /sys/devices/platform/samsung-galaxybook/start_on_lid_open
>
> I guess to me having this be based on ACPI Device ID and differing per
> device feels "less nice" compared to having a nice "user friendly"
> path that feels a bit more obvious. Is it preferred to just use the
> existing platform_device based on the ACPI device ID instead of
> creating a new "virtual" platform_device with a nicer name for
> purposes like settings sysfs attributes be more "user friendly" like
> this?
>
> Another alternative is that I could move these kind of sysfs
> attributes to driver attributes instead; then if I am guessing
> correctly then it would be like this:
>
> cat /sys/bus/platform/drivers/samsung-galaxybook/start_on_lid_open
>
> But then I do not know if having this kind of sysfs attribute (which
> actually will query and write values to the device itself using ACPI
> methods) feel correct as "driver" attributes ? Maybe it does not
> matter and I am just over thinking it :)
>
> Any preference on these two questions? Again, to recap:
> 1) Yes or no to create new platform_device even though one already
> exists with the ACPI device ID as it's name?

I advise against this, because if the driver somehow binds to multiple devices in the future
then creating a second platform device will fail (same name). So No.

> 2) If using existing platform_device, should these kind of sysfs
> attributes be under the dynamic device ID-based platform_device or is
> it ok / make sense to move them to driver attributes?

Driver attributes get created then the driver is registered, so this could lead to
various lifetime problems, so please use device attributes.

I suggest that you update the documentation of the driver to tell people that:

1. They should use udev for device discovery

and/or

2. They can find the attributes under /sys/bus/platform/drivers/<driver name>/<device name>/<attribute>

Thank,
Armin Wolf

>> [...]

Re: Adding a new platform driver samsung-galaxybook

[Joshua Grisham]

Den lör 23 nov. 2024 kl 23:19 skrev Armin Wolf <W_Armin@gmx.de>:
> > 1) Yes or no to create new platform_device even though one already
> > exists with the ACPI device ID as it's name?
>
> I advise against this, because if the driver somehow binds to multiple devices in the future
> then creating a second platform device will fail (same name). So No.
> [...]
> > 2) If using existing platform_device, should these kind of sysfs
> > attributes be under the dynamic device ID-based platform_device or is
> > it ok / make sense to move them to driver attributes?
>
> Driver attributes get created then the driver is registered, so this could lead to
> various lifetime problems, so please use device attributes.
>
> I suggest that you update the documentation of the driver to tell people that:
>
> 1. They should use udev for device discovery
>
> and/or
>
> 2. They can find the attributes under /sys/bus/platform/drivers/<driver name>/<device name>/<attribute>

Thanks again Armin, this is very good advice! I have made these
adjustments now in a local copy but then stumbled onto something else:
what about the hwmon and kbd_backlight LED class device names? Right
now I have also hard-coded these with "samsung-galaxybook" but in
theory if multiple different instances of one of the ACPI device IDs
existed (as you mentioned could be a "possible" scenario in the
future?) then I guess these would break as well?

Having said that, it would be REALLY strange if any of these Samsung
notebooks had multiple instances of one or more of these ACPI devices;
it would be like saying there are two completely separate "platform"
devices on the same notebook, where both can control their own
keyboard backlight, performance mode, etc.. I can't imagine this
should ever actually be the case? Also in this case my driver will
actually only try to create the LED class device if the ACPI method to
enable and get/set its value within the device itself is returning the
right success codes, so it seems even less likely that there would be
two instances of the ACPI device IDs on the same notebook and that
both are responding positively to the ACPI method to enable/get/set
the kbd backlight brightness? The hwmon device, on the other hand,
will always be created if it finds any PNP fan devices that need
special handling (e.g. they will not be reporting their values by
default as acpi4 fans so will be set up by the driver instead).

I tried to look in the kernel for existing platform drivers (and
glanced through several non-platform drivers as well) to find any that
might be following this pattern of dynamic LED class device name or
hwmon device name and could not really find anything except the Intel
int3472 which is using the ACPI device ID as the LED class device
name; otherwise, everything else I could find under platform is using
hard-coded names, including drivers that seem to follow this pattern
we are talking about here (using existing platform devices based on
ACPI device ID name instead of creating new ones, e.g. ideapad-laptop
has hard-coded LED class device names with its
"platform::kbd_backlight" and "platform::fnlock" even though the
actual platform device is just matched from the ACPI device ID VPC2004
??).

For specifically kbd_backlight and hwmon devices, I think it is more
likely that people will be making various scripts / config / etc that
do things like show the fan speeds in various widgets and/or control
the keyboard backlight via script, so it seems to me like it is even
better if these can be fixed names that anyone who uses these devices
will be able to use (e.g. "samsung-galaxybook::kbd_backlight" feels
better than something non-fixed based on ACPI device ID like
"SAM0429_00::kbd_backlight").

This feels a bit like sub-optimizing here, especially since pretty
much all of the other drivers I can see are hard-coding these kind of
names already as well.. is it ok to just leave hwmon and LED class
device names as hard-coded with prefix "samsung-galaxybook" and
if/when it comes along that someone has a problem with multiple
instances, it will fail with an error message in the kernel log and
they can submit a bug? (where we figure out what the right course of
action is exactly for that case)

Thanks again!
Joshua

> [...]

Re: Adding a new platform driver samsung-galaxybook

[Armin Wolf]

Am 25.11.24 um 15:16 schrieb Joshua Grisham:

> Den lör 23 nov. 2024 kl 23:19 skrev Armin Wolf <W_Armin@gmx.de>:
>>> 1) Yes or no to create new platform_device even though one already
>>> exists with the ACPI device ID as it's name?
>> I advise against this, because if the driver somehow binds to multiple devices in the future
>> then creating a second platform device will fail (same name). So No.
>> [...]
>>> 2) If using existing platform_device, should these kind of sysfs
>>> attributes be under the dynamic device ID-based platform_device or is
>>> it ok / make sense to move them to driver attributes?
>> Driver attributes get created then the driver is registered, so this could lead to
>> various lifetime problems, so please use device attributes.
>>
>> I suggest that you update the documentation of the driver to tell people that:
>>
>> 1. They should use udev for device discovery
>>
>> and/or
>>
>> 2. They can find the attributes under /sys/bus/platform/drivers/<driver name>/<device name>/<attribute>
> Thanks again Armin, this is very good advice! I have made these
> adjustments now in a local copy but then stumbled onto something else:
> what about the hwmon and kbd_backlight LED class device names? Right
> now I have also hard-coded these with "samsung-galaxybook" but in
> theory if multiple different instances of one of the ACPI device IDs
> existed (as you mentioned could be a "possible" scenario in the
> future?) then I guess these would break as well?

The hwmon subsystem can handle duplicate chip names, so naming the hwmon chip
"samsung-galaxybook" should be OK.

For the LED class device: you can use led_init_data to set default_label and
devicename (just copy the platform device name for devicename). By setting
devname_mandatory to true the resulting led class device should have a unique
name.

> Having said that, it would be REALLY strange if any of these Samsung
> notebooks had multiple instances of one or more of these ACPI devices;
> it would be like saying there are two completely separate "platform"
> devices on the same notebook, where both can control their own
> keyboard backlight, performance mode, etc.. I can't imagine this
> should ever actually be the case? Also in this case my driver will
> actually only try to create the LED class device if the ACPI method to
> enable and get/set its value within the device itself is returning the
> right success codes, so it seems even less likely that there would be
> two instances of the ACPI device IDs on the same notebook and that
> both are responding positively to the ACPI method to enable/get/set
> the kbd backlight brightness? The hwmon device, on the other hand,
> will always be created if it finds any PNP fan devices that need
> special handling (e.g. they will not be reporting their values by
> default as acpi4 fans so will be set up by the driver instead).

You are right that it is unlikely for multiple compatible ACPI devices to
exist, but being able to gracefully handle this case would still be nice.

> I tried to look in the kernel for existing platform drivers (and
> glanced through several non-platform drivers as well) to find any that
> might be following this pattern of dynamic LED class device name or
> hwmon device name and could not really find anything except the Intel
> int3472 which is using the ACPI device ID as the LED class device
> name; otherwise, everything else I could find under platform is using
> hard-coded names, including drivers that seem to follow this pattern
> we are talking about here (using existing platform devices based on
> ACPI device ID name instead of creating new ones, e.g. ideapad-laptop
> has hard-coded LED class device names with its
> "platform::kbd_backlight" and "platform::fnlock" even though the
> actual platform device is just matched from the ACPI device ID VPC2004
> ??).

Sadly not all drivers try to handle such a situation.

> For specifically kbd_backlight and hwmon devices, I think it is more
> likely that people will be making various scripts / config / etc that
> do things like show the fan speeds in various widgets and/or control
> the keyboard backlight via script, so it seems to me like it is even
> better if these can be fixed names that anyone who uses these devices
> will be able to use (e.g. "samsung-galaxybook::kbd_backlight" feels
> better than something non-fixed based on ACPI device ID like
> "SAM0429_00::kbd_backlight").
>
> This feels a bit like sub-optimizing here, especially since pretty
> much all of the other drivers I can see are hard-coding these kind of
> names already as well.. is it ok to just leave hwmon and LED class
> device names as hard-coded with prefix "samsung-galaxybook" and
> if/when it comes along that someone has a problem with multiple
> instances, it will fail with an error message in the kernel log and
> they can submit a bug? (where we figure out what the right course of
> action is exactly for that case)

I am CCing the LED maintainers so they can give us some advise on how to handle
this the best way.

Thanks,
Armin Wolf

> Thanks again!
> Joshua
>
>> [...]

Re: Adding a new platform driver samsung-galaxybook

[Joshua Grisham]

Den mån 25 nov. 2024 kl 18:20 skrev Armin Wolf <W_Armin@gmx.de>:
>
> For the LED class device: you can use led_init_data to set default_label and
> devicename (just copy the platform device name for devicename). By setting
> devname_mandatory to true the resulting led class device should have a unique
> name.
>

Thank you, this was excellent guidance as usual! I tested a few
variations and found that it is possible to match existing platform
drivers with the format of <name>::kbd_backlight and here is how this
change looks in the branch I am using to make these adjustments in:

https://github.com/joshuagrisham/samsung-galaxybook-extras/commit/20f28efed07fb04b7c5397e709bbd00772bdf4fb

As a summary I am basically setting the following:

init_data.devicename = "samsung-galaxybook";
init_data.default_label = ":kbd_backlight";
init_data.devname_mandatory = true;

This results in the name of the first added LED device to still be
"samsung-galaxybook::kbd_backlight" which also matches the pattern
that exists in basically every other currently existing x86 Platform
driver.

I "mocked" having multiple existing by temporarily removing the
unregister step and then removing and re-probing the module several
times. What happens is that the string is just appended with a rolling
number like this: samsung-galaxybook::kbd_backlight_1,
samsung-galaxybook::kbd_backlight_2, etc.

I then checked in the code of both udev and upower to ensure that
these names would still work with the features in those services, and
in both places they are looking for the existence of the string
"kbd_backlight" anywhere within the name, so it should still work with
this implementation as well.. To be sure that it does in fact work, I
forced the name to be exactly samsung-galaxybook::kbd_backlight_1 and
rebooted, and everything seemed to be working fine / automatically
within GNOME and otherwise--so far, so good, there!

I am quite pleased with this solution as it should give the originally
intended name in all existing known cases and additional names would
be added only in super weird circumstances, yet will still handle
multiple names in a graceful way.

> I am CCing the LED maintainers so they can give us some advise on how to handle
> this the best way.
>

Amazing, thank you! How does it look with the change I mentioned
here--does it seem like this is matching the desired outcome, or is
there anything that should be changed?

Thank you again!

Best,
Joshua

> [...]

Re: Adding a new platform driver samsung-galaxybook

[Lee Jones]

> > For specifically kbd_backlight and hwmon devices, I think it is more
> > likely that people will be making various scripts / config / etc that
> > do things like show the fan speeds in various widgets and/or control
> > the keyboard backlight via script, so it seems to me like it is even
> > better if these can be fixed names that anyone who uses these devices
> > will be able to use (e.g. "samsung-galaxybook::kbd_backlight" feels
> > better than something non-fixed based on ACPI device ID like
> > "SAM0429_00::kbd_backlight").
> > 
> > This feels a bit like sub-optimizing here, especially since pretty
> > much all of the other drivers I can see are hard-coding these kind of
> > names already as well.. is it ok to just leave hwmon and LED class
> > device names as hard-coded with prefix "samsung-galaxybook" and
> > if/when it comes along that someone has a problem with multiple
> > instances, it will fail with an error message in the kernel log and
> > they can submit a bug? (where we figure out what the right course of
> > action is exactly for that case)
> 
> I am CCing the LED maintainers so they can give us some advise on how to handle
> this the best way.

I'm only in receipt of a snippet of the conversation here and lack all
context, however I can speak generally.

It is unlikely that you find yourself in uncharted territory with
respect to device enumeration and future-proofing.  The kernel is
designed in such a way as to support subsequent versions of devices,
usually by versioning or literal enumeration (see PLATFORM_DEVID_AUTO as
an example of this).  Allowing future devices to break and subsequently
relying on users to submit bug reports sounds suboptimal.  If we can
prevent breakage rather than react to it, possibly after developers have
moved on to something else, then we should do that. Matching on known
ACPI implementations and providing support for that sounds sane.

-- 
Lee Jones [李琼斯]

Re: Adding a new platform driver samsung-galaxybook

[Hans de Goede]

Hi Joshua,

On 18-Nov-24 2:51 PM, Joshua Grisham wrote:
> Hello! I have created a platform driver for Samsung Galaxy Book series
> notebooks which has now gone through several iterations and
> contributions from several other community members.

Thank you for your work on this and thank you for submitting your
driver upstream. It is always good to have support for more laptop
models in the mainline kernel.

And my apologies for being slow with responding to this.

<snip>

> 3. usage of the i8042 filter and ACPI hotkey notifications to handle a
> few of the hotkey actions within the driver itself instead of just
> emitting input events and allow userspace to handle the actions
> (namely cycling through keyboard backlight levels, performance modes,
> etc)
> 
> This last item (executing hotkey actions in kernel space) is not
> totally unprecedented either, as I have seen there seems to exist
> similar i8042 filters driving hotkey actions in msi-laptop,
> toshiba_acpi, and dell-laptop and ACPI notifications from hotkeys
> driving actions in several x86 platform drivers as well (dell-laptop,
> acer-wmi, asus-laptop, ideapad-laptop, etc; this is an even more
> common pattern than using an i8042 filter, it seems).
> 
> The problem with just emitting the "right" input events and relying on
> the userspace to handle this stuff in the right way is that 1) there
> are not really keycodes that exist for exactly the keys we want here
> (even though "Keyboard Backlight Cycle" and some kind of "Performance
> Mode" hotkeys are very common on laptops today) and 2) functionality
> for how to handle these kind of events do not really support these
> use-cases either.

I agree that for performance-mode-cycling handling the hotkey in
the kernel is the right thing to do, this is already done by
several other drivers and we even have a helper for this:
platform_profile_cycle() please make sure you use this helper and
otherwise this is fine.

For kbd-backlight-brightness cycling most laptops actually implement
this in the embedded-controller and we only get an event that
the brightness was changed (with if we are lucky also the new
brightness level inside the event).

As you say we have KEY_KBDILLUMTOGGLE/XF86KbdLightOnOff and
KEY_KBDILLUM[UP|DOWN]/XF86KbdBrightness[Up|Down] but those don't
match what we want here and if we add a new keycode for that
userspace support will also need to be added.

So I think it is best to just emulate what the laptops where
the cycling is directly done by the embedded-control do.

That is:

1. Add LED_BRIGHT_HW_CHANGED to the flags of the led_classdev
for the "xxx:kbd_backlight" led class device you expose

2. Filter out kbd-backlight-cycle keypresses and on such
a keypress:

2.1 Determine new brightness level
2.2 Apply new brightness level
2.3 Call:

led_classdev_notify_brightness_hw_changed(&kbd_backlight_led_classdev, new_brightness_level);

This last call emulates what the kernel reports to userspace
on the events we get from embedded controllers after they have
changed the brightness level based on the cycle-key getting
pressed.

With this in place you should get a nice OSD notification like
for volume up/down in at least GNOME when changing the kbd
backlight level.

Note if you rmmod + modprobe your laptop driver you need
to restart upower afterwards for this to work. IIRC gnome-shell
does not need a logout + login, just restarting upower is enough.
But I might be wrong about not needing to restart gnome-shell...

I hope this helps and I'm looking forward to you submitting
your driver upstream.

Regards,

Hans

Re: Adding a new platform driver samsung-galaxybook

[Joshua Grisham]

Hi Hans, thank you so much for taking the time to read through the
questions and get back to me!

Den ons 4 dec. 2024 kl 18:31 skrev Hans de Goede <hdegoede@redhat.com>:
>
> So I think it is best to just emulate what the laptops where
> the cycling is directly done by the embedded-control do.
>
> That is:
>
> 1. Add LED_BRIGHT_HW_CHANGED to the flags of the led_classdev
> for the "xxx:kbd_backlight" led class device you expose
>
> 2. Filter out kbd-backlight-cycle keypresses and on such
> a keypress:
>
> 2.1 Determine new brightness level
> 2.2 Apply new brightness level
> 2.3 Call:
>
> led_classdev_notify_brightness_hw_changed(&kbd_backlight_led_classdev, new_brightness_level);
>

This is actually exactly what I have already implemented with the one
exception: I am executing exactly the same kind of logic you mentioned
(via schedule_work()) but I have NOT filtered out the keypress;
instead, it is just scheduling this logic to run in a workqueue and
then going ahead and passing along the keypress as well, just in case
anyone wanted to trigger any other kind of event from this hotkey.

I have actually submitted a patch to the keyboard hwdb which was
merged in to systemd that maps this particular key to "unknown" with
the idea that someone who has this model would also likely have this
platform driver module loaded, so by default the kernel-space action
to actually change the brightness level would be executed (the
"EC-like" behavior as you mentioned that they could not change), but
the user would also have the option of remapping the key and
triggering additional actions on top of this if they wanted.  Does
that sound appropriate or is it better to just filter out the keypress
entirely once the above actions are scheduled/executed?

Also as an aside, I have had a few users who have mentioned that if
they have compiled and loaded i8042 as a module (which is then marked
as "used by" samsung_galaxybook due to the i8042 filter), if they
execute a modprobe -r then it also removes i8042 and their keyboard
stops working. Is this known/expected behavior and/or is there
anything that can be done in this driver itself to try and help
prevent this from happening? Otherwise I guess a "fix" for this would
be if users compile their kernel with CONFIG_SERIO_I8042=y then they
would not have this problem?

Thank you again!

Best regards,
Joshua

> [...]

Re: Adding a new platform driver samsung-galaxybook

[Hans de Goede]

Hi Joshua,

On 4-Dec-24 9:33 PM, Joshua Grisham wrote:
> Hi Hans, thank you so much for taking the time to read through the
> questions and get back to me!
> 
> Den ons 4 dec. 2024 kl 18:31 skrev Hans de Goede <hdegoede@redhat.com>:
>>
>> So I think it is best to just emulate what the laptops where
>> the cycling is directly done by the embedded-control do.
>>
>> That is:
>>
>> 1. Add LED_BRIGHT_HW_CHANGED to the flags of the led_classdev
>> for the "xxx:kbd_backlight" led class device you expose
>>
>> 2. Filter out kbd-backlight-cycle keypresses and on such
>> a keypress:
>>
>> 2.1 Determine new brightness level
>> 2.2 Apply new brightness level
>> 2.3 Call:
>>
>> led_classdev_notify_brightness_hw_changed(&kbd_backlight_led_classdev, new_brightness_level);
>>
> 
> This is actually exactly what I have already implemented with the one
> exception: I am executing exactly the same kind of logic you mentioned
> (via schedule_work()) but I have NOT filtered out the keypress;
> instead, it is just scheduling this logic to run in a workqueue and
> then going ahead and passing along the keypress as well, just in case
> anyone wanted to trigger any other kind of event from this hotkey.
> 
> I have actually submitted a patch to the keyboard hwdb which was
> merged in to systemd that maps this particular key to "unknown" with
> the idea that someone who has this model would also likely have this
> platform driver module loaded, so by default the kernel-space action
> to actually change the brightness level would be executed (the
> "EC-like" behavior as you mentioned that they could not change), but
> the user would also have the option of remapping the key and
> triggering additional actions on top of this if they wanted.  Does
> that sound appropriate or is it better to just filter out the keypress
> entirely once the above actions are scheduled/executed?

In my experience it is best to pick one approach of

1. Deliver event to userspace and let userspace handle everything
2. Handle everything in kernel and stick with that

We actually have what you are suggesting for display brightness
up/down presses in the drivers/acpi/acpi_video.c driver
which exposes both a /sys/class/backlight device and an evdev
device delivering key-press events and which automatically
increases the brightness of the /sys/class/backlight device on
brightness up/down hotkey presses.

And that combination is a hot mess. GNOME/KDE see the keypress
and then race with the kernel increasing the brightness. Typically
they loose the race reading the new brigthness increasing the
brightness by 2 steps on one keypress. And some older laptops
have only 8 steps, so that is a problem.

I disabled the in kernel handling of the brightness up/down
keypresses in the ACPI video bus driver because of this, but
some users complained about this breaking old X11 setups using
e.g. Window Maker of fvwm. Linus Torvalds ended up "fixing"
this by instead of having the kernel immediately react giving
userspace like 0.25 seconds or something to respond and if
it does not, then handle it in the kernel. Which of course
is racy so sometimes users still hit the 2 steps for one
keypress issue if the laptop is under load.

Note this is meant as an example of what NOT to do.

As for the hwdb mapping of they keypress to unknown I predict
that at some point a well intending user is going to notice
this, map it to KEY_KBDILLUMTOGGLE and submit a PR to systemd
upstream.  Then the systemd upstream maintainers will trust
this user, who actually has such a laptop which they don't to
be doing the right thing and merge it.

And then if GNOME/KDE/xxxx grow support for actually acting
on KEY_KBDILLUMTOGGLE (if they do not do so already) we have
the kernel hotkey and userspace hotkey handling fighting
each other just like the example above.

So based on this I would strongly advice you to filter out
the key event completely at the kernel level.

If someone ever really needs / wants that event then my
suggestion would be to add a module option which *completely*
disables all in kernel handling for the key in kernelspace
and instead delivers the events to userspace.

TL;DR: IMHO mixing in kernel handling with keypress reporting
is a bad idea. Please chose one model and stick with it.

> Also as an aside, I have had a few users who have mentioned that if
> they have compiled and loaded i8042 as a module (which is then marked
> as "used by" samsung_galaxybook due to the i8042 filter), if they
> execute a modprobe -r then it also removes i8042 and their keyboard
> stops working. Is this known/expected behavior and/or is there
> anything that can be done in this driver itself to try and help
> prevent this from happening? Otherwise I guess a "fix" for this would
> be if users compile their kernel with CONFIG_SERIO_I8042=y then they
> would not have this problem?

IMHO, the best way to solve this issue is to tell users to do
"rmmod samsung_galaxybook" instead of modprobe -r. And you can do
the same in any Makefiles / scripts you may have.

Regards,

Hans

Re: Adding a new platform driver samsung-galaxybook

[Armin Wolf]

Am 04.12.24 um 23:00 schrieb Hans de Goede:

> Hi Joshua,
>
> On 4-Dec-24 9:33 PM, Joshua Grisham wrote:
>> Hi Hans, thank you so much for taking the time to read through the
>> questions and get back to me!
>>
>> Den ons 4 dec. 2024 kl 18:31 skrev Hans de Goede <hdegoede@redhat.com>:
>>> So I think it is best to just emulate what the laptops where
>>> the cycling is directly done by the embedded-control do.
>>>
>>> That is:
>>>
>>> 1. Add LED_BRIGHT_HW_CHANGED to the flags of the led_classdev
>>> for the "xxx:kbd_backlight" led class device you expose
>>>
>>> 2. Filter out kbd-backlight-cycle keypresses and on such
>>> a keypress:
>>>
>>> 2.1 Determine new brightness level
>>> 2.2 Apply new brightness level
>>> 2.3 Call:
>>>
>>> led_classdev_notify_brightness_hw_changed(&kbd_backlight_led_classdev, new_brightness_level);
>>>
>> This is actually exactly what I have already implemented with the one
>> exception: I am executing exactly the same kind of logic you mentioned
>> (via schedule_work()) but I have NOT filtered out the keypress;
>> instead, it is just scheduling this logic to run in a workqueue and
>> then going ahead and passing along the keypress as well, just in case
>> anyone wanted to trigger any other kind of event from this hotkey.
>>
>> I have actually submitted a patch to the keyboard hwdb which was
>> merged in to systemd that maps this particular key to "unknown" with
>> the idea that someone who has this model would also likely have this
>> platform driver module loaded, so by default the kernel-space action
>> to actually change the brightness level would be executed (the
>> "EC-like" behavior as you mentioned that they could not change), but
>> the user would also have the option of remapping the key and
>> triggering additional actions on top of this if they wanted.  Does
>> that sound appropriate or is it better to just filter out the keypress
>> entirely once the above actions are scheduled/executed?
> In my experience it is best to pick one approach of
>
> 1. Deliver event to userspace and let userspace handle everything
> 2. Handle everything in kernel and stick with that
>
> We actually have what you are suggesting for display brightness
> up/down presses in the drivers/acpi/acpi_video.c driver
> which exposes both a /sys/class/backlight device and an evdev
> device delivering key-press events and which automatically
> increases the brightness of the /sys/class/backlight device on
> brightness up/down hotkey presses.
>
> And that combination is a hot mess. GNOME/KDE see the keypress
> and then race with the kernel increasing the brightness. Typically
> they loose the race reading the new brigthness increasing the
> brightness by 2 steps on one keypress. And some older laptops
> have only 8 steps, so that is a problem.
>
> I disabled the in kernel handling of the brightness up/down
> keypresses in the ACPI video bus driver because of this, but
> some users complained about this breaking old X11 setups using
> e.g. Window Maker of fvwm. Linus Torvalds ended up "fixing"
> this by instead of having the kernel immediately react giving
> userspace like 0.25 seconds or something to respond and if
> it does not, then handle it in the kernel. Which of course
> is racy so sometimes users still hit the 2 steps for one
> keypress issue if the laptop is under load.
>
> Note this is meant as an example of what NOT to do.
>
> As for the hwdb mapping of they keypress to unknown I predict
> that at some point a well intending user is going to notice
> this, map it to KEY_KBDILLUMTOGGLE and submit a PR to systemd
> upstream.  Then the systemd upstream maintainers will trust
> this user, who actually has such a laptop which they don't to
> be doing the right thing and merge it.
>
> And then if GNOME/KDE/xxxx grow support for actually acting
> on KEY_KBDILLUMTOGGLE (if they do not do so already) we have
> the kernel hotkey and userspace hotkey handling fighting
> each other just like the example above.
>
> So based on this I would strongly advice you to filter out
> the key event completely at the kernel level.
>
> If someone ever really needs / wants that event then my
> suggestion would be to add a module option which *completely*
> disables all in kernel handling for the key in kernelspace
> and instead delivers the events to userspace.
>
> TL;DR: IMHO mixing in kernel handling with keypress reporting
> is a bad idea. Please chose one model and stick with it.

I agree with this viewpoint. Also the user still sees the event when you filter out
the special keypress, since calling led_classdev_notify_brightness_hw_changed() sends a
poll notification to the led class device. So when using poll(), select(), etc. the user
can still react to this event.

>> Also as an aside, I have had a few users who have mentioned that if
>> they have compiled and loaded i8042 as a module (which is then marked
>> as "used by" samsung_galaxybook due to the i8042 filter), if they
>> execute a modprobe -r then it also removes i8042 and their keyboard
>> stops working. Is this known/expected behavior and/or is there
>> anything that can be done in this driver itself to try and help
>> prevent this from happening? Otherwise I guess a "fix" for this would
>> be if users compile their kernel with CONFIG_SERIO_I8042=y then they
>> would not have this problem?
> IMHO, the best way to solve this issue is to tell users to do
> "rmmod samsung_galaxybook" instead of modprobe -r. And you can do
> the same in any Makefiles / scripts you may have.
>
> Regards,
>
> Hans
>
Same problem with any driver which registers a acpi_battery_hook, if you unload the driver you also unload
the ACPI battery driver itself :( The reason for this seems to be that modprobe assumes that the ACPI battery
driver is a pure dependency of the first driver and can be unloaded if no other module depend on it.

I think this problem could be fixable, but i have no experience when it comes to the kernel modules infrastructure.

For now using rmmmod instead of modprobe -r should indeed do the job.

Thanks,
Armin Wolf

Re: Adding a new platform driver samsung-galaxybook

[Joshua Grisham]

Den ons 4 dec. 2024 kl 23:19 skrev Armin Wolf <W_Armin@gmx.de>:
>
> Am 04.12.24 um 23:00 schrieb Hans de Goede:
>
> > Hi Joshua,
> >
> > On 4-Dec-24 9:33 PM, Joshua Grisham wrote:
> >> Hi Hans, thank you so much for taking the time to read through the
> >> questions and get back to me!
> >>
> >> Den ons 4 dec. 2024 kl 18:31 skrev Hans de Goede <hdegoede@redhat.com>:
> >>> So I think it is best to just emulate what the laptops where
> >>> the cycling is directly done by the embedded-control do.
> >>>
> >>> That is:
> >>>
> >>> 1. Add LED_BRIGHT_HW_CHANGED to the flags of the led_classdev
> >>> for the "xxx:kbd_backlight" led class device you expose
> >>>
> >>> 2. Filter out kbd-backlight-cycle keypresses and on such
> >>> a keypress:
> >>>
> >>> 2.1 Determine new brightness level
> >>> 2.2 Apply new brightness level
> >>> 2.3 Call:
> >>>
> >>> led_classdev_notify_brightness_hw_changed(&kbd_backlight_led_classdev, new_brightness_level);
> >>>
> >> This is actually exactly what I have already implemented with the one
> >> exception: I am executing exactly the same kind of logic you mentioned
> >> (via schedule_work()) but I have NOT filtered out the keypress;
> >> instead, it is just scheduling this logic to run in a workqueue and
> >> then going ahead and passing along the keypress as well, just in case
> >> anyone wanted to trigger any other kind of event from this hotkey.
> >>
> >> I have actually submitted a patch to the keyboard hwdb which was
> >> merged in to systemd that maps this particular key to "unknown" with
> >> the idea that someone who has this model would also likely have this
> >> platform driver module loaded, so by default the kernel-space action
> >> to actually change the brightness level would be executed (the
> >> "EC-like" behavior as you mentioned that they could not change), but
> >> the user would also have the option of remapping the key and
> >> triggering additional actions on top of this if they wanted.  Does
> >> that sound appropriate or is it better to just filter out the keypress
> >> entirely once the above actions are scheduled/executed?
> > In my experience it is best to pick one approach of
> >
> > 1. Deliver event to userspace and let userspace handle everything
> > 2. Handle everything in kernel and stick with that
> >
> > We actually have what you are suggesting for display brightness
> > up/down presses in the drivers/acpi/acpi_video.c driver
> > which exposes both a /sys/class/backlight device and an evdev
> > device delivering key-press events and which automatically
> > increases the brightness of the /sys/class/backlight device on
> > brightness up/down hotkey presses.
> >
> > And that combination is a hot mess. GNOME/KDE see the keypress
> > and then race with the kernel increasing the brightness. Typically
> > they loose the race reading the new brigthness increasing the
> > brightness by 2 steps on one keypress. And some older laptops
> > have only 8 steps, so that is a problem.
> >
> > I disabled the in kernel handling of the brightness up/down
> > keypresses in the ACPI video bus driver because of this, but
> > some users complained about this breaking old X11 setups using
> > e.g. Window Maker of fvwm. Linus Torvalds ended up "fixing"
> > this by instead of having the kernel immediately react giving
> > userspace like 0.25 seconds or something to respond and if
> > it does not, then handle it in the kernel. Which of course
> > is racy so sometimes users still hit the 2 steps for one
> > keypress issue if the laptop is under load.
> >
> > Note this is meant as an example of what NOT to do.
> >
> > As for the hwdb mapping of they keypress to unknown I predict
> > that at some point a well intending user is going to notice
> > this, map it to KEY_KBDILLUMTOGGLE and submit a PR to systemd
> > upstream.  Then the systemd upstream maintainers will trust
> > this user, who actually has such a laptop which they don't to
> > be doing the right thing and merge it.
> >
> > And then if GNOME/KDE/xxxx grow support for actually acting
> > on KEY_KBDILLUMTOGGLE (if they do not do so already) we have
> > the kernel hotkey and userspace hotkey handling fighting
> > each other just like the example above.
> >
> > So based on this I would strongly advice you to filter out
> > the key event completely at the kernel level.
> >
> > If someone ever really needs / wants that event then my
> > suggestion would be to add a module option which *completely*
> > disables all in kernel handling for the key in kernelspace
> > and instead delivers the events to userspace.
> >
> > TL;DR: IMHO mixing in kernel handling with keypress reporting
> > is a bad idea. Please chose one model and stick with it.
>
> I agree with this viewpoint. Also the user still sees the event when you filter out
> the special keypress, since calling led_classdev_notify_brightness_hw_changed() sends a
> poll notification to the led class device. So when using poll(), select(), etc. the user
> can still react to this event.
>
> >> Also as an aside, I have had a few users who have mentioned that if
> >> they have compiled and loaded i8042 as a module (which is then marked
> >> as "used by" samsung_galaxybook due to the i8042 filter), if they
> >> execute a modprobe -r then it also removes i8042 and their keyboard
> >> stops working. Is this known/expected behavior and/or is there
> >> anything that can be done in this driver itself to try and help
> >> prevent this from happening? Otherwise I guess a "fix" for this would
> >> be if users compile their kernel with CONFIG_SERIO_I8042=y then they
> >> would not have this problem?
> > IMHO, the best way to solve this issue is to tell users to do
> > "rmmod samsung_galaxybook" instead of modprobe -r. And you can do
> > the same in any Makefiles / scripts you may have.
> >
> > Regards,
> >
> > Hans
> >
> Same problem with any driver which registers a acpi_battery_hook, if you unload the driver you also unload
> the ACPI battery driver itself :( The reason for this seems to be that modprobe assumes that the ACPI battery
> driver is a pure dependency of the first driver and can be unloaded if no other module depend on it.
>
> I think this problem could be fixable, but i have no experience when it comes to the kernel modules infrastructure.
>
> For now using rmmmod instead of modprobe -r should indeed do the job.
>
> Thanks,
> Armin Wolf
>

Perfect, thank you both! All of this makes perfect sense to me and I
will make these suggested changes.

I am planning to try and fix up some documentation which I will also
add for this platform driver, plus add relevant KConfig/Makefile/etc
entries, do some additional testing, and will try to get a patch
submitted here hopefully sometime within the next few days.

My plan was to take the latest commit at the time from the 'for-next'
branch of pdx86/platform-drivers-x86.git and then submit it all
together as a patch via a new thread here. Please let me know if there
are any problems with that plan or anything else I might need to
adjust before doing so.

Thank you again!

Best regards,
Joshua

Re: Adding a new platform driver samsung-galaxybook

[Armin Wolf]

Am 05.12.24 um 21:40 schrieb Joshua Grisham:

> Den ons 4 dec. 2024 kl 23:19 skrev Armin Wolf <W_Armin@gmx.de>:
>> Am 04.12.24 um 23:00 schrieb Hans de Goede:
>>
>>> Hi Joshua,
>>>
>>> On 4-Dec-24 9:33 PM, Joshua Grisham wrote:
>>>> Hi Hans, thank you so much for taking the time to read through the
>>>> questions and get back to me!
>>>>
>>>> Den ons 4 dec. 2024 kl 18:31 skrev Hans de Goede <hdegoede@redhat.com>:
>>>>> So I think it is best to just emulate what the laptops where
>>>>> the cycling is directly done by the embedded-control do.
>>>>>
>>>>> That is:
>>>>>
>>>>> 1. Add LED_BRIGHT_HW_CHANGED to the flags of the led_classdev
>>>>> for the "xxx:kbd_backlight" led class device you expose
>>>>>
>>>>> 2. Filter out kbd-backlight-cycle keypresses and on such
>>>>> a keypress:
>>>>>
>>>>> 2.1 Determine new brightness level
>>>>> 2.2 Apply new brightness level
>>>>> 2.3 Call:
>>>>>
>>>>> led_classdev_notify_brightness_hw_changed(&kbd_backlight_led_classdev, new_brightness_level);
>>>>>
>>>> This is actually exactly what I have already implemented with the one
>>>> exception: I am executing exactly the same kind of logic you mentioned
>>>> (via schedule_work()) but I have NOT filtered out the keypress;
>>>> instead, it is just scheduling this logic to run in a workqueue and
>>>> then going ahead and passing along the keypress as well, just in case
>>>> anyone wanted to trigger any other kind of event from this hotkey.
>>>>
>>>> I have actually submitted a patch to the keyboard hwdb which was
>>>> merged in to systemd that maps this particular key to "unknown" with
>>>> the idea that someone who has this model would also likely have this
>>>> platform driver module loaded, so by default the kernel-space action
>>>> to actually change the brightness level would be executed (the
>>>> "EC-like" behavior as you mentioned that they could not change), but
>>>> the user would also have the option of remapping the key and
>>>> triggering additional actions on top of this if they wanted.  Does
>>>> that sound appropriate or is it better to just filter out the keypress
>>>> entirely once the above actions are scheduled/executed?
>>> In my experience it is best to pick one approach of
>>>
>>> 1. Deliver event to userspace and let userspace handle everything
>>> 2. Handle everything in kernel and stick with that
>>>
>>> We actually have what you are suggesting for display brightness
>>> up/down presses in the drivers/acpi/acpi_video.c driver
>>> which exposes both a /sys/class/backlight device and an evdev
>>> device delivering key-press events and which automatically
>>> increases the brightness of the /sys/class/backlight device on
>>> brightness up/down hotkey presses.
>>>
>>> And that combination is a hot mess. GNOME/KDE see the keypress
>>> and then race with the kernel increasing the brightness. Typically
>>> they loose the race reading the new brigthness increasing the
>>> brightness by 2 steps on one keypress. And some older laptops
>>> have only 8 steps, so that is a problem.
>>>
>>> I disabled the in kernel handling of the brightness up/down
>>> keypresses in the ACPI video bus driver because of this, but
>>> some users complained about this breaking old X11 setups using
>>> e.g. Window Maker of fvwm. Linus Torvalds ended up "fixing"
>>> this by instead of having the kernel immediately react giving
>>> userspace like 0.25 seconds or something to respond and if
>>> it does not, then handle it in the kernel. Which of course
>>> is racy so sometimes users still hit the 2 steps for one
>>> keypress issue if the laptop is under load.
>>>
>>> Note this is meant as an example of what NOT to do.
>>>
>>> As for the hwdb mapping of they keypress to unknown I predict
>>> that at some point a well intending user is going to notice
>>> this, map it to KEY_KBDILLUMTOGGLE and submit a PR to systemd
>>> upstream.  Then the systemd upstream maintainers will trust
>>> this user, who actually has such a laptop which they don't to
>>> be doing the right thing and merge it.
>>>
>>> And then if GNOME/KDE/xxxx grow support for actually acting
>>> on KEY_KBDILLUMTOGGLE (if they do not do so already) we have
>>> the kernel hotkey and userspace hotkey handling fighting
>>> each other just like the example above.
>>>
>>> So based on this I would strongly advice you to filter out
>>> the key event completely at the kernel level.
>>>
>>> If someone ever really needs / wants that event then my
>>> suggestion would be to add a module option which *completely*
>>> disables all in kernel handling for the key in kernelspace
>>> and instead delivers the events to userspace.
>>>
>>> TL;DR: IMHO mixing in kernel handling with keypress reporting
>>> is a bad idea. Please chose one model and stick with it.
>> I agree with this viewpoint. Also the user still sees the event when you filter out
>> the special keypress, since calling led_classdev_notify_brightness_hw_changed() sends a
>> poll notification to the led class device. So when using poll(), select(), etc. the user
>> can still react to this event.
>>
>>>> Also as an aside, I have had a few users who have mentioned that if
>>>> they have compiled and loaded i8042 as a module (which is then marked
>>>> as "used by" samsung_galaxybook due to the i8042 filter), if they
>>>> execute a modprobe -r then it also removes i8042 and their keyboard
>>>> stops working. Is this known/expected behavior and/or is there
>>>> anything that can be done in this driver itself to try and help
>>>> prevent this from happening? Otherwise I guess a "fix" for this would
>>>> be if users compile their kernel with CONFIG_SERIO_I8042=y then they
>>>> would not have this problem?
>>> IMHO, the best way to solve this issue is to tell users to do
>>> "rmmod samsung_galaxybook" instead of modprobe -r. And you can do
>>> the same in any Makefiles / scripts you may have.
>>>
>>> Regards,
>>>
>>> Hans
>>>
>> Same problem with any driver which registers a acpi_battery_hook, if you unload the driver you also unload
>> the ACPI battery driver itself :( The reason for this seems to be that modprobe assumes that the ACPI battery
>> driver is a pure dependency of the first driver and can be unloaded if no other module depend on it.
>>
>> I think this problem could be fixable, but i have no experience when it comes to the kernel modules infrastructure.
>>
>> For now using rmmmod instead of modprobe -r should indeed do the job.
>>
>> Thanks,
>> Armin Wolf
>>
> Perfect, thank you both! All of this makes perfect sense to me and I
> will make these suggested changes.
>
> I am planning to try and fix up some documentation which I will also
> add for this platform driver, plus add relevant KConfig/Makefile/etc
> entries, do some additional testing, and will try to get a patch
> submitted here hopefully sometime within the next few days.
>
> My plan was to take the latest commit at the time from the 'for-next'
> branch of pdx86/platform-drivers-x86.git and then submit it all
> together as a patch via a new thread here. Please let me know if there
> are any problems with that plan or anything else I might need to
> adjust before doing so.
>
> Thank you again!
>
> Best regards,
> Joshua

Sounds good to me, looking forward to review this patch :)

Thanks,
Armin Wolf