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

Hi Ben,

Ben Warren wrote:

Hi Richard,

richardretanubun wrote:

...

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).

---

I started to fix this up so that it would apply, but it's been badly mangled by your mailer. Would you mind please re-sending? If you're using Thunderbird or Evolution, select the "preformat" text type when you paste the patch. If using something else, please just attach the patch file.

Sorry for replying so late (and twice, I forgot to cc the mailing list). Here is the patch in preformat form and also attached, just in case.

regards, Ben

Thanks for all your help

Richard

-------------------------------------

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)

The current uec_miiphy_read and uec_miiphy_write hardcode access devlist[0] This patch makes these function use the devname argument that is passed in to allow access to the phy registers of other devices in devlist[].

Signed-of-by: Richard Retanubun RichardRetanubun@RugggedCom.com --- Hi Ben,

I'm hoping the 7th try will do it. I forgot to add the commit message before.

drivers/qe/uec.c | 46 +++++++++++++++++++++++++++++++++++++++++++--- 1 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/drivers/qe/uec.c b/drivers/qe/uec.c index 344c649..e3fbbb0 100644 --- a/drivers/qe/uec.c +++ b/drivers/qe/uec.c @@ -639,6 +639,31 @@ 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: @@ -647,8 +672,16 @@ 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 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; }

@@ -661,11 +694,18 @@ 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 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)