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? 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 > [...]
