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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
