Re: [PATCH net-next v2 04/10] net: phy: replace is_c45 with phy_accces_mode

[Andrew Lunn]

> @@ -131,9 +131,11 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
>  
>  	is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
>  	if (is_c45 || fwnode_get_phy_id(child, &phy_id))
> -		phy = get_phy_device(bus, addr, is_c45);
> +		phy = get_phy_device(bus, addr,
> +				     is_c45 ? PHY_ACCESS_C45 : PHY_ACCESS_C22);
>  	else
> -		phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> +		phy = phy_device_create(bus, addr, phy_id, PHY_ACCESS_C22,
> +					NULL);

Documentation/devicetree/bindings/net/ethernet-phy.yaml says:

  compatible:
    oneOf:
      - const: ethernet-phy-ieee802.3-c22
        description: PHYs that implement IEEE802.3 clause 22
      - const: ethernet-phy-ieee802.3-c45
        description: PHYs that implement IEEE802.3 clause 45

It would be nice to make this documentation more specific. It now
refers to 'bus transaction', so maybe we want to append that to these
lines?

> -static struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr, bool c45)
> +static struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr,
> +				       enum phy_access_mode mode)

> +/**
> + * enum phy_access_mode - PHY register access mode definitions
> + *
> + * @PHY_ACCESS_C22: use 802.3 c22 MDIO transactions
> + * @PHY_ACCESS_C45: use 802.3 c45 MDIO transactions
> + */
> +enum phy_access_mode {
> +	PHY_ACCESS_C22,
> +	PHY_ACCESS_C45,
> +};

Was the change from bool to enum enough to make the compiler warn when
passed the wrong type? i.e. a true/false? not PHY_ACCESS_C22 and
PHY_ACCESS_C45? Maybe we could set these enum values to 22 and 45?
true/false would then not match, and we get some sort of error, like
-EIO from the switch statement?

>  /**
>   * struct phy_device - An instance of a PHY
>   *
> @@ -539,8 +550,8 @@ struct macsec_ops;
>   * @devlink: Create a link between phy dev and mac dev, if the external phy
>   *           used by current mac interface is managed by another mac interface.
>   * @phy_id: UID for this device found during discovery
> - * @c45_ids: 802.3-c45 Device Identifiers if is_c45.
> - * @is_c45:  Set to true if this PHY uses clause 45 addressing.
> + * @access_mode:  MDIO access mode of the PHY.
> + * @c45_ids: 802.3-c45 Device Identifiers if it's an C45 PHY.
>   * @is_internal: Set to true if this PHY is internal to a MAC.
>   * @is_pseudo_fixed_link: Set to true if this PHY is an Ethernet switch, etc.
>   * @is_gigabit_capable: Set to true if PHY supports 1000Mbps
> @@ -637,8 +648,9 @@ struct phy_device {
>  
>  	u32 phy_id;
>  
> +	enum phy_access_mode access_mode;
> +

This enum might not pad too well between a u32 and struct? If you put
it after the bitfields, or maybe next to the enum phy_state the
compiler might make both a u16 and put them together?

	 Andrew