Re: [PATCH v1 5/6] Platform support for On-chip MSP ethernet devices.

[Sergei Shtylyov <sshtylyov@mvista.com>]

Hello.

On 25-01-2011 13:28, Anoop P.A wrote:

> From: Anoop P A<anoop.pa@gmail.com>

> Previous version of patch was line wrapped and had some style issues. correcting it.

    Such remarks should folow the --- tear line.

> Some of MSP family SoC's come with legacy 100Mbps mspeth while some comes with
> newer Gigabit TSMAC.Following patch adds platform support for both types of MAC's.
> If TSMAC is not selected assume platform having legacy mspeth.This patch will add
> gpio_macros as well which is required for resetting phy

> Signed-off-by: Anoop P A<anoop.pa@gmail.com>
[...]

> diff --git a/arch/mips/include/asm/pmc-sierra/msp71xx/msp_gpio_macros.h b/arch/mips/include/asm/pmc-sierra/msp71xx/msp_gpio_macros.h
> new file mode 100644
> index 0000000..d83d78f
> --- /dev/null
> +++ b/arch/mips/include/asm/pmc-sierra/msp71xx/msp_gpio_macros.h
> @@ -0,0 +1,343 @@
> +/*
> + *
> + * Macros for external SMP-safe access to the PMC MSP71xx reference
> + * board GPIO pins
> + *
> + * Copyright 2010 PMC-Sierra, Inc.
> + *
> + *  This program is free software; you can redistribute  it and/or modify it
> + *  under  the terms of  the GNU General  Public License as published by the
> + *  Free Software Foundation;  either version 2 of the  License, or (at your
> + *  option) any later version.
> + *
> + *  THIS  SOFTWARE  IS PROVIDED   ``AS  IS'' AND   ANY  EXPRESS OR IMPLIED
> + *  WARRANTIES,   INCLUDING, BUT NOT  LIMITED  TO, THE IMPLIED WARRANTIES OF
> + *  MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.  IN
> + *  NO  EVENT  SHALL   THE AUTHOR  BE    LIABLE FOR ANY   DIRECT, INDIRECT,
> + *  INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> + *  NOT LIMITED   TO, PROCUREMENT OF  SUBSTITUTE GOODS  OR SERVICES; LOSS OF
> + *  USE, DATA,  OR PROFITS; OR  BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
> + *  ANY THEORY OF LIABILITY, WHETHER IN  CONTRACT, STRICT LIABILITY, OR TORT
> + *  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + *  THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + *
> + *  You should have received a copy of the  GNU General Public License along
> + *  with this program; if not, write  to the Free Software Foundation, Inc.,
> + *  675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#ifndef __MSP_GPIO_MACROS_H__
> +#define __MSP_GPIO_MACROS_H__
> +
> +#include <msp_regops.h>
> +#include <msp_regs.h>
> +
> +#ifdef CONFIG_PMC_MSP7120_GW
> +#define MSP_NUM_GPIOS		(20)

    Useless parens.

> +#else
> +#define MSP_NUM_GPIOS		(28)

    Here too.

[...]

> +/* -- Bit masks -- */
> +
> +/* This gives you the 'register relative offlet gpio' number */

    Offlet?

> +#define OFFSET_GPIO_NUMBER(gpio)	(gpio - MSP_GPIO_OFFSET[gpio])
> +
> +/* These take the 'register relative offset gpio' number */
> +#define BASIC_DATA_REG_MASK(ogpio)	\
> +	(1 << ogpio)


> +#define BASIC_MODE_REG_VALUE(mode, ogpio)	\
> +	(mode << BASIC_MODE_REG_SHIFT(ogpio))
> +#define BASIC_MODE_REG_MASK(ogpio)		\
> +	BASIC_MODE_REG_VALUE(0xf, ogpio)
> +#define BASIC_MODE_REG_SHIFT(ogpio)	\
> +	(ogpio * 4)

    Why break the lines if there's enough space for the definition on the 
first line?

> +/* Sets the specified pin to the specified value */
> +static inline void msp_gpio_pin_set(msp_gpio_data_t data, unsigned int gpio)
> +{
> +	if (gpio>= MSP_NUM_GPIOS)
> +		return;
> +
> +	if (gpio < 16) {
> +		if (data == MSP_GPIO_TOGGLE)
> +			toggle_reg32(MSP_GPIO_DATA_REGISTER[gpio],
> +					BASIC_DATA_MASK(gpio));
> +		else if (data == MSP_GPIO_HI)
> +			set_reg32(MSP_GPIO_DATA_REGISTER[gpio],
> +					BASIC_DATA_MASK(gpio));
> +		else
> +			clear_reg32(MSP_GPIO_DATA_REGISTER[gpio],
> +					BASIC_DATA_MASK(gpio));
> +	} else {
> +		if (data == MSP_GPIO_TOGGLE) {
> +			/* Special ugly case:
> +			 *   We have to read the CLR bit.
> +			 *   If set, we write the CLR bit.
> +			 *   If not, we write the SET bit.
> +			 */
> +			u32 tmpdata;

    Empty line wouldn't hurt here.

> +			custom_read_reg32(MSP_GPIO_DATA_REGISTER[gpio],
> +								tmpdata);
> +			if (tmpdata & EXTENDED_CLR(gpio))
> +				tmpdata = EXTENDED_CLR(gpio);
> +			else
> +				tmpdata = EXTENDED_SET(gpio);
> +			custom_write_reg32(MSP_GPIO_DATA_REGISTER[gpio],
> +								tmpdata);
> +		} else {
> +			u32 newdata;

    Here too.

> +			if (data == MSP_GPIO_HI)
> +				newdata = EXTENDED_SET(gpio);
> +			else
> +				newdata = EXTENDED_CLR(gpio);
> +			set_value_reg32(MSP_GPIO_DATA_REGISTER[gpio],
> +						EXTENDED_FULL_MASK, newdata);
> +		}
> +	}
> +}

[...]

> diff --git a/arch/mips/pmc-sierra/msp71xx/msp_eth.c b/arch/mips/pmc-sierra/msp71xx/msp_eth.c
> new file mode 100644
> index 0000000..fa510ca
> --- /dev/null
> +++ b/arch/mips/pmc-sierra/msp71xx/msp_eth.c
> @@ -0,0 +1,188 @@
> +int __init msp_eth_setup(void)
> +{
> +	int i, ret = 0;
> +
> +	/* Configure the GPIO and take the ethernet PHY out of reset */
> +	msp_gpio_pin_mode(MSP_GPIO_OUTPUT, MSP_ETHERNET_GPIO0);
> +	msp_gpio_pin_hi(MSP_ETHERNET_GPIO0);
> +
> +#ifdef CONFIG_MSP_HAS_TSMAC
> +	/* 3 phys on boards with TSMAC */
> +	msp_gpio_pin_mode(MSP_GPIO_OUTPUT, MSP_ETHERNET_GPIO1);
> +	msp_gpio_pin_hi(MSP_ETHERNET_GPIO1);
> +
> +	msp_gpio_pin_mode(MSP_GPIO_OUTPUT, MSP_ETHERNET_GPIO2);
> +	msp_gpio_pin_hi(MSP_ETHERNET_GPIO2);
> +#endif
> +	for (i = 0; i<  ARRAY_SIZE(msp_eth_devs); i++) {
> +		ret = platform_device_register(&msp_eth_devs[i]);
> +		printk(KERN_INFO"device: %d, return value = %d\n", i, ret);
                                 ^
    Space wouldn't hurt here.

> +		if (ret) {
> +			while (--i >= 0)
> +				platform_device_unregister(&msp_eth_devs[i]);

    Why unregister all devices if one registration fails?

> +			break;
> +		}
> +	}
> +
> +	if (ret)
> +		printk(KERN_WARNING "Could not initialize \
> +						MSPETH device structures.\n");

    String literals are not broken up like this, instead use:


		printk(KERN_WARNING "Could not initialize "
					"MSPETH device structures.\n");

WBR, Sergei