On 8/28/24 6:56 PM, Andy Shevchenko wrote:
On Wed, Aug 28, 2024 at 12:10 PM Maximilian Luz <luzmaximilian@xxxxxxxxx> wrote:
I thought I should provide some context:
Thank you, my reply below.
Am 26/08/2024 um 22:54 schrieb Andy Shevchenko:
Wed, Aug 14, 2024 at 12:27:27PM +0200, Konrad Dybcio kirjoitti:
From: Konrad Dybcio <quic_kdybcio@xxxxxxxxxxx>
[...]
nodes = (const struct software_node **)acpi_device_get_match_data(&pdev->dev);
Hmm... Why this doesn't use simple device_get_match_data()?
- if (!nodes)
- return -ENODEV;
+ if (!nodes) {
+ fdt_root = of_find_node_by_path("/");
+ if (!fdt_root)
+ return -ENODEV;
+
+ match = of_match_node(ssam_platform_hub_of_match, fdt_root);
+ of_node_put(fdt_root);
+ if (!match)
+ return -ENODEV;
+
+ nodes = (const struct software_node **)match->data;
This is quite strange! Where are they being defined?
Essentially, this whole module is a giant workaround because there
doesn't seem to be a way to auto-discover which functions or subdevices
the EC actually supports. So this module builds a registry of software
nodes and matches against a Surface-model-specific ACPI ID (in ACPI
mode). Based on that ID, we retrieve the tree of software nodes that
define the EC subdevices and register them using a (virtual) platform
hub device.
The snippet way above registers the platform hub device for DT,
because there we don't have an equivalent ACPI device that we can
use. The code here retrieves the respective nodes.
Yes, and software nodes for DT are quite strange things! Why can't you
simply fix the DT to begin with?
For the ARM/DT variants we could do that. But we still have to deal with
the x86/ACPI ones here. So for me it makes more sense to have it unified
and just deal with everything in this module.
Also, if we consider that at some point we might get ACPI PEP support (I
know, far fetched right now): With that, ACPI on ARM might be feasible
and then we'd have to manage the same thing in two places...
And lastly, the EC subdevices are quite contained and I don't see them
interacting with any other components in the DT, so it's more of a
stylistic choice where to put them.
+ if (!nodes)
+ return -ENODEV;
+ }
...
+MODULE_ALIAS("platform:surface_aggregator_platform_hub");
Can it be platfrom device ID table instead? But do you really need it?
I think the explanation above already kind of answers this, but the
module is named differently than the driver (so that they reflect the
specific nature of each, registry vs hub device). And the platform hub
device added in the snippet I left above is named after the driver. So
for the registry module to load when the platform hub driver is
requested, it is needed.
So, I believe it warrants a platform device ID table to make it explicit.
Yes, that makes sense. (I was not arguing against that, just wanted to
explain why we need the match at all.)
Best regards,
Max
