Am 25.11.24 um 15:16 schrieb Joshua Grisham:
Den lör 23 nov. 2024 kl 23:19 skrev Armin Wolf <W_Armin@xxxxxx>:
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
[...]
