From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ben Warren <biggerbadderben@gmail.com>
Date: Mon, 18 Aug 2008 14:00:25 -0700
Subject: [U-Boot] [U-boot] [PATCH 1/2] NET: QE: UEC: Make
 uec_miiphy_read() and uec_miiphy_write() use the devname arg.
In-Reply-To: <48A9D199.9030909@ruggedcom.com>
References: <48A9D199.9030909@ruggedcom.com>
Message-ID: <48A9E2E9.1030703@gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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