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

[Andrew Lunn <andrew@xxxxxxx>]

On Fri, Oct 05, 2018 at 10:24:53AM +0000, Igor Russkikh wrote:
> From: Dmitry Bezrukov <dmitry.bezrukov@xxxxxxxxxxxx>
> 
> 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.

> @@ -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;
>  	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