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

[Andrew Lunn]

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