Am 23.11.24 um 18:58 schrieb Joshua Grisham:
Den fre 22 nov. 2024 kl 21:25 skrev Kurt Borja <kuurtb@xxxxxxxxx>:
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
[...]
