[U-Boot] [U-boot] [PATCHv2 1/2] NET: QE: UEC: Make uec_miiphy_read() and uec_miiphy_write() use the devname arg.

[richardretanubun]

Hi Ben,

Thanks for the feedback. I've made the changes you suggested.
Here is the patch re-pasted (please let me know if this is not the 
proper way to submit patch v2).
--------------------------------------------

Added a new function uec_miiphy_find_dev_by_name to allow 
uec_miiphy_read and uec_miiphy_write to use the passed devname and not 
hardcoded to devlist[0]

Signed-off-by: Richard Retanubun <RichardRetanubun_at_ruggedcom.com>

diff --git a/drivers/qe/uec.c b/drivers/qe/uec.c
index 344c649..9b0b3bc 100644
--- a/drivers/qe/uec.c
+++ b/drivers/qe/uec.c
@@ -639,16 +639,51 @@ static void phy_change(struct eth_device *dev)
     && !defined(BITBANGMII)
 
 /*
+ * Find a device index from the devlist by name
+ *
+ * Returns:
+ *  The index where the device is located, -1 on error
+ */
+static int uec_miiphy_find_dev_by_name(char *devname)
+{
+    int i;
+
+
+    for (i = 0; i < MAXCONTROLLERS; i++) {
+        if (strncmp(devname, devlist[i]->name, strlen(devname)) == 0) {
+            break;
+        }
+    }
+
+    /* If device cannot be found, returns -1 */
+    if (i == MAXCONTROLLERS) {
+        debug ("%s: device %s not found in devlist\n", __FUNCTION__, 
devname);
+        i = -1;
+    }
+
+    return i;
+}
+
+/*
  * Read a MII PHY register.
  *
  * Returns:
  *  0 on success
  */
 static int uec_miiphy_read(char *devname, unsigned char addr,
-                unsigned char reg, unsigned short *value)
+                            unsigned char reg, unsigned short *value)
 {
-    *value = uec_read_phy_reg(devlist[0], addr, reg);
+    int devindex = 0;
 
+
+    if (devname == NULL || value == NULL) {
+        debug("%s: NULL pointer given\n", __FUNCTION__);
+    } else {
+        devindex = uec_miiphy_find_dev_by_name(devname);
+        if (devindex >= 0) {
+            *value = uec_read_phy_reg(devlist[devindex], addr, reg);
+        }
+    }
     return 0;
 }
 
@@ -659,13 +694,21 @@ static int uec_miiphy_read(char *devname, unsigned 
char addr,
  *  0 on success
  */
 static int uec_miiphy_write(char *devname, unsigned char addr,
-                 unsigned char reg, unsigned short value)
+                            unsigned char reg, unsigned short value)
 {
-    uec_write_phy_reg(devlist[0], addr, reg, value);
+    int devindex = 0;
+
 
+    if (devname == NULL) {
+        debug("%s: NULL pointer given\n", __FUNCTION__);
+    } else {
+        devindex = uec_miiphy_find_dev_by_name(devname);
+        if (devindex >= 0) {
+            uec_write_phy_reg(devlist[devindex], addr, reg, value);
+        }
+    }
     return 0;
 }
-
 #endif
 
 static int uec_set_mac_address(uec_private_t *uec, u8 *mac_addr)





Ben Warren wrote:
> richardretanubun wrote:
>> Added a new function uec_miiphy_find_dev_by_name to allow 
>> uec_miiphy_read and uec_miiphy_write to use the passed devname and 
>> not hardcoded to devlist[0]
>>
>> Signed-off-by: Richard Retanubun <RichardRetanubun_at_ruggedcom.com>
>>
>> diff --git a/drivers/qe/uec.c b/drivers/qe/uec.c
>> index 344c649..d14566e 100644
>> --- a/drivers/qe/uec.c
>> +++ b/drivers/qe/uec.c
>> @@ -639,6 +639,32 @@ static void phy_change(struct eth_device *dev)
>>      && !defined(BITBANGMII)
>>  
>>  /*
>> + * Find a device index from the devlist by name
>> + *
>> + * Returns:
>> + *  The index where the device is located, else 0
>>   
> But index 0 is a valid device.  Return -1 on failure and check for it.
>> + */
>> +static int uec_miiphy_find_dev_by_name(char *devname)
>> +{
>> +    int i = 0;
>>   
> No need to initialize this variable.
>> +
>> +
>> +    for (i = 0; i < MAXCONTROLLERS; i++) {
>> +        if (strncmp(devname, devlist[i]->name, strlen(devname)) == 0) {
>> +            break;
>> +        }
>> +    }
>> +
>> +    // If device cannot be found, default to 0
>>   
> No C++ - style comments please.
>> +    if (i == MAXCONTROLLERS) {
>> +        debug ("%s: device %s not found in devlist\n", __FUNCTION__, 
>> devname);
>> +        i = 0;
>>   
> As mentioned above, don't set to a valid index on failure.  -1 is 
> standard for this type of thing.
>> +    }
>> +
>> +    return (i);
>>   
> No brackets needed around return value.
>> +}
>> +
>> +/*
>>   * Read a MII PHY register.
>>   *
>>   * Returns:
>> @@ -647,8 +673,15 @@ static void phy_change(struct eth_device *dev)
>>  static int uec_miiphy_read(char *devname, unsigned char addr,
>>                  unsigned char reg, unsigned short *value)
>>  {
>> -    *value = uec_read_phy_reg(devlist[0], addr, reg);
>> +    int i = 0;
>>   
> This isn't a good variable name except for iterators.  Please use 
> something more meaningful.
>> +
>>  
>> +    if (devname == NULL || value == NULL) {
>> +        debug("%s: NULL pointer given\n", __FUNCTION__);
>> +    } else {
>> +        i = uec_miiphy_find_dev_by_name(devname);
>>   
> Bail if i<0
>> +        *value = uec_read_phy_reg(devlist[i], addr, reg);
>> +    }
>>      return 0;
>>  }
>>  
>> @@ -661,11 +694,17 @@ static int uec_miiphy_read(char *devname, 
>> unsigned char addr,
>>  static int uec_miiphy_write(char *devname, unsigned char addr,
>>                   unsigned char reg, unsigned short value)
>>  {
>> -    uec_write_phy_reg(devlist[0], addr, reg, value);
>> +    int i = 0;
>> +
>>  
>> +    if (devname == NULL) {
>> +        debug("%s: NULL pointer given\n", __FUNCTION__);
>> +    } else {
>> +        i = uec_miiphy_find_dev_by_name(devname);
>> +        uec_write_phy_reg(devlist[i], addr, reg, value);
>> +    }
>>   
> Same comments as with other function.
>>      return 0;
>>  }
>> -
>>  #endif
>>  
>>  static int uec_set_mac_address(uec_private_t *uec, u8 *mac_addr)
>>
>>   
> regards,
> Ben