netdev - Re: [PATCH net-next 03/11] net: dsa: mv88e6xxx: Add the mv88e6390 family

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8760nqbytd.fsf@ketchup.i-did-not-set--mail-host-address--so-tickle-me>
Date:   Mon, 14 Nov 2016 13:05:50 +1100
From:   Vivien Didelot <vivien.didelot@...oirfairelinux.com>
To:     Andrew Lunn <andrew@...n.ch>, David Miller <davem@...emloft.net>
Cc:     netdev <netdev@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH net-next 03/11] net: dsa: mv88e6xxx: Add the mv88e6390 family

Hi Andrew,

Andrew Lunn <andrew@...n.ch> writes:

> -- compatible           : Should be one of "marvell,mv88e6085",
> +- compatible	       : Should be one of "marvell,mv88e6085" or
> +			 "marvell,mv88e6390"

Just curious here, mv88e6085 was choosen because it was the smaller
product ID supported. Following that logic, shouldn't mv88e6190 be
choosen here instead of mv88e6390?

> +static const struct mv88e6xxx_ops mv88e6390_ops = {
> +	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
> +	.phy_read = mv88e6xxx_g2_smi_phy_read,
> +	.phy_write = mv88e6xxx_g2_smi_phy_write,
> +	.port_set_link = mv88e6xxx_port_set_link,
> +	.port_set_duplex = mv88e6xxx_port_set_duplex,
> +	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
> +	.port_set_speed = mv88e6390_port_set_speed,
> +};
> +
> +static const struct mv88e6xxx_ops mv88e6390x_ops = {
> +	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
> +	.phy_read = mv88e6xxx_g2_smi_phy_read,
> +	.phy_write = mv88e6xxx_g2_smi_phy_write,
> +	.port_set_link = mv88e6xxx_port_set_link,
> +	.port_set_duplex = mv88e6xxx_port_set_duplex,
> +	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
> +	.port_set_speed = mv88e6390x_port_set_speed,
> +};

Even if it is a bit more verbose, I'd intentionally keep one
mv88e6xxx_ops structure per chip. Using per-family structure is
error-prone and simpler is better here.

Thanks,

        Vivien