RE: [PATCH net-next] net: phy: microchip_t1: add cable test support for lan87xx phy

[<Yuiko.Oshino@microchip.com>]

>-----Original Message-----
>From: Andrew Lunn <andrew@lunn.ch>
>Sent: Friday, October 22, 2021 3:57 PM
>To: Yuiko Oshino - C18177 <Yuiko.Oshino@microchip.com>
>Cc: davem@davemloft.net; netdev@vger.kernel.org; Nisar Sayed - I17970
><Nisar.Sayed@microchip.com>; UNGLinuxDriver
><UNGLinuxDriver@microchip.com>
>Subject: Re: [PATCH net-next] net: phy: microchip_t1: add cable test support for
>lan87xx phy
>
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>content is safe
>
>> +static int lan87xx_cable_test_start(struct phy_device *phydev) {
>> +     static const struct access_ereg_val cable_test[] = {
>> +             /* min wait */
>> +             {PHYACC_ATTR_MODE_WRITE, PHYACC_ATTR_BANK_DSP, 93,
>> +              0, 0},
>> +             /* max wait */
>> +             {PHYACC_ATTR_MODE_WRITE, PHYACC_ATTR_BANK_DSP, 94,
>> +              10, 0},
>> +             /* pulse cycle */
>> +             {PHYACC_ATTR_MODE_WRITE, PHYACC_ATTR_BANK_DSP, 95,
>> +              90, 0},
>> +             /* cable diag thresh */
>> +             {PHYACC_ATTR_MODE_WRITE, PHYACC_ATTR_BANK_DSP, 92,
>> +              60, 0},
>> +             /* max gain */
>> +             {PHYACC_ATTR_MODE_WRITE, PHYACC_ATTR_BANK_DSP, 79,
>> +              31, 0},
>> +             /* clock align for each iteration */
>> +             {PHYACC_ATTR_MODE_MODIFY, PHYACC_ATTR_BANK_DSP, 55,
>> +              0, 0x0038},
>> +             /* max cycle wait config */
>> +             {PHYACC_ATTR_MODE_WRITE, PHYACC_ATTR_BANK_DSP, 94,
>> +              70, 0},
>> +             /* start cable diag*/
>> +             {PHYACC_ATTR_MODE_WRITE, PHYACC_ATTR_BANK_DSP, 90,
>> +              1, 0},
>> +     };
>> +     int rc, i;
>> +
>> +     rc = microchip_cable_test_start_common(phydev);
>> +     if (rc < 0)
>> +             return rc;
>> +
>> +     /* start cable diag */
>> +     /* check if part is alive - if not, return diagnostic error */
>> +     rc = access_ereg(phydev, PHYACC_ATTR_MODE_READ,
>PHYACC_ATTR_BANK_SMI,
>> +                      0x00, 0);
>> +     if (rc < 0)
>> +             return rc;
>> +
>> +     if (rc != 0x2100)
>> +             return -ENODEV;
>
>What does this actually mean? Would -EOPNOTSUPP be better?

This register should return the value of 0x2100. So if the return value is different, then I assume there is no device.
>
>> +static int lan87xx_cable_test_report_trans(u32 result) {
>> +     switch (result) {
>> +     case 0:
>> +             return ETHTOOL_A_CABLE_RESULT_CODE_OK;
>> +     case 1:
>> +             return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
>> +     case 2:
>> +             return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
>
>Please add some #defines for 0, 1, 2.

Sure, will do.
>
>       Andrew

Thank you.
Yuiko