Re: [PATCH] Intel IXP4xx network drivers v.2 - Ethernet and HSS

[Lennert Buytenhek]

On Tue, May 08, 2007 at 03:19:22AM +0200, Krzysztof Halasa wrote:

> diff --git a/arch/arm/mach-ixp4xx/ixdp425-setup.c b/arch/arm/mach-ixp4xx/ixdp425-setup.c
> index ec4f079..f20d39d 100644
> --- a/arch/arm/mach-ixp4xx/ixdp425-setup.c
> +++ b/arch/arm/mach-ixp4xx/ixdp425-setup.c
> @@ -101,10 +101,35 @@ static struct platform_device ixdp425_uart = {
>  	.resource		= ixdp425_uart_resources
>  };
>  
> +/* Built-in 10/100 Ethernet MAC interfaces */
> +static struct mac_plat_info ixdp425_plat_mac[] = {
> +	{
> +		.phy		= 0,
> +		.rxq		= 3,
> +	}, {
> +		.phy		= 1,
> +		.rxq		= 4,
> +	}
> +};

As with Christian's driver (I'm feeling like a bit of a broken record
here :-), putting knowledge of which queue to use (which is firmware-
specific) in the _board_ support file is almost certainly wrong.

I would just put the port number in there, and let the ethernet
driver map the port number to the hardware queue number.  After all,
the ethernet driver knows which queues the firmware uses, while the
board support code doesn't.


> +#ifndef __ARMEB__
> +#warning Little endian mode not supported
> +#endif

Yay. :-)  /me hides


> +static inline void set_regbits(u32 bits, u32 __iomem *reg)
> +{
> +	__raw_writel(__raw_readl(reg) | bits, reg);
> +}
> +static inline void clr_regbits(u32 bits, u32 __iomem *reg)
> +{
> +	__raw_writel(__raw_readl(reg) & ~bits, reg);
> +}

I generally discourage the use of such wrappers, as it often makes
people forget that the set and clear operations are not atomic, and
it ignores the fact that some of the other bits in the register you
are modifying might have side-effects.

Didn't review the rest -- not sure whether I'm looking at the latest
version.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/