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