Re: [net-next: PATCH 2/3] net: mvpp2: enable using phylink with ACPI

On Mon, Jun 14, 2021 at 01:46:06AM +0200, Marcin Wojtas wrote:

<Adding ACPI Maintainers>

Hi Andrew,

niedz., 13 cze 2021 o 23:35 Andrew Lunn [off-list ref] napisał(a):

quoted

quoted

True. I picked the port type properties that are interpreted by
phylink. Basically, I think that everything that's described in:
devicetree/bindings/net/ethernet-controller.yaml
is valid for the ACPI as well

So you are saying ACPI is just DT stuff into tables? Then why bother
with ACPI? Just use DT.

Any user is free to use whatever they like, however apparently there
must have been valid reasons, why ARM is choosing ACPI as the
preferred way of describing the hardware over DT. In such
circumstances, we all work to improve adoption and its usability for
existing devices.

Regarding the properties in _DSD package, please refer to
https://www.kernel.org/doc/html/latest/firmware-guide/acpi/DSD-properties-rules.html,
especially to two fragments:
"The _DSD (Device Specific Data) configuration object, introduced in
ACPI 5.1, allows any type of device configuration data to be provided
via the ACPI namespace. In principle, the format of the data may be
arbitrary [...]"
"It often is useful to make _DSD return property sets that follow
Device Tree bindings."
Therefore what I understand is that (within some constraints) simple
reusing existing sets of nodes' properties, should not violate ACPI
spec. In this patchset no new extension/interfaces/method is
introduced.

quoted

Right, O.K. Please document anything which phylink already supports:

hylink.c:               ret = fwnode_property_read_u32(fixed_node, "speed", &speed);
phylink.c:              if (fwnode_property_read_bool(fixed_node, "full-duplex"))
phylink.c:              if (fwnode_property_read_bool(fixed_node, "pause"))
phylink.c:              if (fwnode_property_read_bool(fixed_node, "asym-pause"))
phylink.c:              ret = fwnode_property_read_u32_array(fwnode, "fixed-link",
phylink.c:              ret = fwnode_property_read_u32_array(fwnode, "fixed-link",
phylink.c:      if (dn || fwnode_property_present(fwnode, "fixed-link"))
phylink.c:      if ((fwnode_property_read_string(fwnode, "managed", &managed) == 0 &&

If you are adding new properties, please do that In a separate patch,
which needs an ACPI maintainer to ACK it before it gets merged.

Ok, I can extend the documentation.

My real fear is snowflakes. Each ACPI implementation is unique. That
is going to be a maintenance nightmare, and it will make it very hard
to change the APIs between phylib/phylink and MAC drivers. To avoid
that, we need to push are much as possible into the core, document as
much as possible, and NACK anything does looks like a snowflake.

I actually like what you pointed out above. It makes it possible to
say, ACPI for phylink/phylib needs to follow device tree, 1 to 1.
It also means we should be able to remove a lot of the

if (is_of()) {}
else if (is_acpi() {}
else
	return -EINVAL;

in drivers, and put it into the core.

   Andrew