Re: [PATCH net-next 05/19] net: usb: aqc111: Introduce PHY access

On Fri, Oct 05, 2018 at 10:24:53AM +0000, Igor Russkikh wrote:

From: Dmitry Bezrukov <redacted>

Implement PHY power up/down sequences.
AQC111, depending on FW used, may has PHY being controlled either
directly (dpa = 1) or via vendor command interface (dpa = 0).

Hi Igor

dpa is not a very descriptive name.

Once we figure out if phylib is going to be used, or even phylink, i
suggest you rename this to something like aqc111_data->use_phylib.

quoted hunk

@@ -172,6 +211,8 @@ static void aqc111_unbind(struct usbnet *dev, struct usb_interface *intf)
 	u8 reg8;
 	u16 reg16;
 
+	struct aqc111_data *aqc111_data = (struct aqc111_data *)dev->data[0];

Having to do this cast all the time is quiet ugly. It seems like some
other usb_net drivers use netdev_priv().

+
 	/* Force bz */

g/>  	reg16 = SFR_PHYPWR_RSTCTL_BZ;

quoted hunk

 	aqc111_write_cmd_nopm(dev, AQ_ACCESS_MAC, SFR_PHYPWR_RSTCTL,

@@ -179,12 +220,52 @@ static void aqc111_unbind(struct usbnet *dev, struct usb_interface *intf)
 	reg16 = 0;
 	aqc111_write_cmd_nopm(dev, AQ_ACCESS_MAC, SFR_PHYPWR_RSTCTL,
 			      2, 2, &reg16);
+
+	/* Power down ethernet PHY */
+	if (aqc111_data->dpa) {
+		reg8 = 0x00;
+		aqc111_write_cmd_nopm(dev, AQ_PHY_POWER, 0,
+				      0, 1, &reg8);
+	} else {
+		aqc111_data->phy_ops.low_power = 1;
+		aqc111_data->phy_ops.phy_power = 0;
+		aqc111_write_cmd_nopm(dev, AQ_PHY_OPS, 0, 0,
+				      4, &aqc111_data->phy_ops);
+	}
+
+	kfree(aqc111_data);
 }
 

+struct aqc111_phy_options {
+	union {
+		struct {
+			u8 adv_100M:	1;
+			u8 adv_1G:	1;
+			u8 adv_2G5:	1;
+			u8 adv_5G:	1;
+			u8 rsvd1:	4;
+		};
+		u8 advertising;
+	};
+	union {
+		struct {
+			u8 eee_100M:	1;
+			u8 eee_1G:	1;
+			u8 eee_2G5:	1;
+			u8 eee_5G:	1;
+			u8 rsvd2:	4;
+		};
+		u8 eee;
+	};
+	union {
+		struct {
+			u8 pause:	1;
+			u8 asym_pause:	1;
+			u8 low_power:	1;
+			u8 phy_power:	1;
+			u8 wol:		1;
+			u8 downshift:	1;
+			u8 rsvd4:	2;
+		};
+		u8 phy_ctrl1;
+	};
+	union {
+		struct {
+		u8 dsh_ret_cnt:	4;
+		u8 magic_packet:1;
+		u8 rsvd5:	3;

The indentation looks wrong here.

+		};
+		u8 phy_ctrl2;
+	};
+};
+
+struct aqc111_data {
+	struct {
+		u8 major;
+		u8 minor;
+		u8 rev;
+	} fw_ver;
+	u8 dpa; /*direct PHY access*/
+	struct aqc111_phy_options phy_ops;
+} __packed;

Why pack this? Do you send it to the firmware?

    Andrew