Hi Andrew,

Andrew Lunn [off-list ref] writes:

On Mon, Jul 31, 2017 at 06:17:18PM -0400, Vivien Didelot wrote:

quoted

The PHY's EEE settings are already accessed by the DSA layer through the
Marvell PHY driver and there is nothing to be done for switch's MACs.

I'm confused, or missing something. Does not patch #1 mean that if the
DSA driver does not have a set_eee function, we always return -ENODEV
in slave.c?

If there is a PHY, phy_init_eee (if eee_enabled is true) and
phy_ethtool_set_eee is called. If there is a .set_eee op, it is
called. If both are absent, -ENODEV is returned.

There might be nothing to configure here, but some of the switches do
support EEE. So we need at least a NOP set_eee. Better still it should
return -ENODEV for those switches which don't actually support EEE,
and 0 for those that do?

As I explain in a commit message, I didn't want to make the EEE ops
mandatory, because it makes it impossible for the DSA layer to
distinguish whether the driver did not update the ethtool_eee structure
because there is nothing to do on the port's MAC side (e.g. mv88e6xxx or
qca8k) or if it returned EEE disabled. To avoid confusion, I prefered to
make the ops optional, making the phy_* calls enough in the first case.

That being said, if you don't share this point of view and prefer to
define an inline dsa_set_eee_noop() function, I don't mind, since this
allows the DSA layer to make the distinction.


Thanks,

        Vivien