From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Lunn <andrew@lunn.ch>
Subject: Re: [PATCH v2 net-next 7/8] net: dsa: microchip: Prepare PHY for
 proper advertisement
Date: Wed, 6 Dec 2017 19:56:03 +0100
Message-ID: <20171206185603.GB28774@lunn.ch>
References: <1512524798-16210-1-git-send-email-Tristram.Ha@microchip.com>
 <1512524798-16210-8-git-send-email-Tristram.Ha@microchip.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Florian Fainelli <f.fainelli@gmail.com>,
        Pavel Machek <pavel@ucw.cz>,
        Ruediger Schmitt <ruediger.schmitt@philips.com>,
        Arkadi Sharshevsky <arkadis@mellanox.com>,
        UNGLinuxDriver@microchip.com, netdev@vger.kernel.org
To: Tristram.Ha@microchip.com
Return-path: <netdev-owner@vger.kernel.org>
Received: from vps0.lunn.ch ([185.16.172.187]:49830 "EHLO vps0.lunn.ch"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1751950AbdLFS4H (ORCPT <rfc822;netdev@vger.kernel.org>);
        Wed, 6 Dec 2017 13:56:07 -0500
Content-Disposition: inline
In-Reply-To: <1512524798-16210-8-git-send-email-Tristram.Ha@microchip.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

Hi Tristram

> +static void ksz9477_phy_setup(struct ksz_device *dev, int port,
> +			      struct phy_device *phy)
> +{
> +	if (port < dev->phy_port_cnt) {
> +		/* SUPPORTED_Asym_Pause and SUPPORTED_Pause can be removed to
> +		 * disable flow control when rate limiting is used.
> +		 */
> +		phy->advertising = phy->supported;
> +	}

This looks a bit odd. phy_probe() does the same:

       /* Start out supporting everything. Eventually,
         * a controller will attach, and may modify one
         * or both of these values
         */
        phydev->supported = phydrv->features;
        of_set_phy_supported(phydev);
        phydev->advertising = phydev->supported;

You don't modify anything here, so i don't see why it is needed.

> +void ksz_adjust_link(struct dsa_switch *ds, int port,
> +		     struct phy_device *phydev)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct ksz_port *p = &dev->ports[port];
> +
> +	if (phydev->link) {
> +		p->speed = phydev->speed;
> +		p->duplex = phydev->duplex;
> +		p->flow_ctrl = phydev->pause;
> +		p->link_up = 1;
> +		dev->live_ports |= (1 << port) & dev->on_ports;
> +	} else if (p->link_up) {
> +		p->link_up = 0;
> +		p->link_down = 1;
> +		dev->live_ports &= ~(1 << port);
> +	}

The link_down looks odd. If you are setting link_up to 1, should
link_down also be set to 0? Can it be both up and down at the same
time?

	Andrew