Re: [PATCH 4/4] asix: Add a new driver for the AX88172A

[Christian Riesch <christian.riesch@omicron.at>]

Hi Grant,

On Mon, Jul 9, 2012 at 12:22 PM, Christian Riesch
<christian.riesch@omicron.at> wrote:
> Grant,
>
> On Fri, Jul 6, 2012 at 11:20 PM, Grant Grundler <grundler@chromium.org> wrote:
>> On Fri, Jul 6, 2012 at 4:33 AM, Christian Riesch
>> <christian.riesch@omicron.at> wrote:
>>> The Asix AX88172A is a USB 2.0 Ethernet interface that supports both an
>>> internal PHY as well as an external PHY (connected via MII).
>>>
>>> This patch adds a driver for the AX88172A and provides support for
>>> both modes and supports phylib.
>>
>> Christian,
>> In general this looks fine to me...but I wouldn't know about "bus
>> identifier life times" (Ben Hutchings comment).
>>
>> My nit pick is the declaration and of use_embdphy. An alternative
>> coding _suggestion_ below.  I'm not substantially altering the
>> functionality.
>>
>> thanks,
>> grant
>
> [...]
>
>>> +
>>> +struct ax88172a_private {
>>> +       int use_embdphy;
>>
>> Can you move the "int" to the end of the struct?
>> It's cleaner to have fields "natively align". ie pointers should start
>> at 8 byte alignments when compiled for 64-bit.
>>
>>> +       struct mii_bus *mdio;
>>> +       struct phy_device *phydev;
>>> +       char phy_name[20];
>>> +       u16 phy_addr;
>>> +       u16 oldmode;
>>> +};
>>> +
>
> [...]
>
>>> +       /* are we using the internal or the external phy? */
>>> +       ret = asix_read_cmd(dev, AX_CMD_SW_PHY_STATUS, 0, 0, 1, buf);
>>> +       if (ret < 0) {
>>> +               dbg("Failed to read software interface selection register: %d",
>>> +                   ret);
>>> +               goto free;
>>> +       }
>>> +       dbg("AX_CMD_SW_PHY_STATUS = 0x%02x\n", buf[0]);
>>> +       switch ((buf[0] & 0x0c) >> 2) {
>>> +       case 0:
>>> +               dbg("use internal phy\n");
>>> +               priv->use_embdphy = 1;
>>> +               break;
>>> +       case 1:
>>> +               dbg("use external phy\n");
>>> +               priv->use_embdphy = 0;
>>> +               break;
>>> +       default:
>>> +               dbg("Interface mode not supported by driver\n");
>>> +               goto free;
>>> +       }
>>
>> This switch statement inverts the existing logic. Much simpler code would be:
>>     /* buf[0] & 0xc describes phy interface mode */
>>     if (buf[0] &  8) {
>>          dbg("Interface mode not supported by driver\n");
>>          goto free;
>>     }
>>     priv->use_extphy = (buf[0] & 4) >> 2;
>>
>
> Thank your for your comments! I'll change that in the next version!
> Regards, Christian

After rethinking it I decided to keep the switch structure, but use
defines for the different modes and the bitmask. I think this will be
easier to understand. I will submit a new version of the patchset
later today, please have a look at it.
Thanks!
Christian