From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Lunn <andrew@lunn.ch>
Subject: Re: [PATCH v1 net-next 6/9] lan743x: Add power management support
Date: Thu, 5 Jul 2018 19:37:43 +0200
Message-ID: <20180705173743.GM23469@lunn.ch>
References: <1530808766-13973-1-git-send-email-Bryan.Whitehead@microchip.com>
 <1530808766-13973-7-git-send-email-Bryan.Whitehead@microchip.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: davem@davemloft.net, netdev@vger.kernel.org,
        UNGLinuxDriver@microchip.com
To: Bryan Whitehead <Bryan.Whitehead@microchip.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from vps0.lunn.ch ([185.16.172.187]:53095 "EHLO vps0.lunn.ch"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1753213AbeGERhp (ORCPT <rfc822;netdev@vger.kernel.org>);
        Thu, 5 Jul 2018 13:37:45 -0400
Content-Disposition: inline
In-Reply-To: <1530808766-13973-7-git-send-email-Bryan.Whitehead@microchip.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

> +static void lan743x_ethtool_get_wol(struct net_device *netdev,
> +				    struct ethtool_wolinfo *wol)
> +{
> +	struct lan743x_adapter *adapter = netdev_priv(netdev);
> +	u32 data;
> +
> +	data = lan743x_csr_read(adapter, PMT_CTL);

Hi Bryan

Why do you do this read? You do not use the result.

> +
> +	wol->supported = WAKE_BCAST | WAKE_UCAST | WAKE_MCAST |
> +		WAKE_MAGIC | WAKE_PHY | WAKE_ARP;
> +
> +	wol->wolopts = adapter->wolopts;
> +}
> +#endif /* CONFIG_PM */
> +
> +static int lan743x_pm_wakeframe_crc16(const u8 *buf, int len)
> +{
> +	const u16 crc16poly = 0x8005;
> +	u16 bit, crc, msb;
> +	u8 data;
> +	int i;
> +
> +	crc = 0xFFFF;
> +	for (i = 0; i < len; i++) {
> +		data = *buf++;
> +		for (bit = 0; bit < 8; bit++) {
> +			msb = crc >> 15;
> +			crc <<= 1;
> +
> +			if (msb ^ (u16)(data & 1)) {
> +				crc ^= crc16poly;
> +				crc |= (u16)0x0001U;
> +			}
> +			data >>= 1;
> +		}
> +	}
> +

There are a few different crc algorithms in lib. Can you use one of
them, rather than implementing it yourself?

> +#if CONFIG_PM
> +static int lan743x_pm_suspend(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct net_device *netdev = pci_get_drvdata(pdev);
> +	struct lan743x_adapter *adapter = netdev_priv(netdev);
> +	u16 phydata;
> +	int ret;
> +
> +	if (adapter->wolopts & WAKE_PHY) {
> +		phydata = phy_read(netdev->phydev, 27);
> +		phydata |= 0x0500;
> +		phy_write(netdev->phydev, 27, phydata);
> +	}

Shouldn't the PHY driver do this?

	  Andrew