Hello Daniel,
Le Thursday 05 June 2008 21:45:13 Daniel Laird, vous avez écrit :
> The following patch add support for the NXP PNX833x SOC. More
> specifically it adds support for
> the STB222/5 variant. This has I2C support, NAND and onboard ethernet
> support.
You should send the i2c parts to Jean Delvare and Ben Dooks, the I2C
maintainers. Ethernet part should be sent to the netdev mailing-list.
You can send the other pending parts to their respective maintainers, since
your drivers will be either platform drivers, and/or they depend on
CONFIG_NXP_STB225, they can be merged now, and get active when MIPS code is
merged as well.
I have posted some comments below in the body of your email, code looks
overall very good.
> +/***********************************************
> +* INCLUDE FILES *
> +************************************************/
> +
> +#include <asm/mach-pnx833x/pnx833x.h>
> +#include <linux/serial_pnx8xxx.h>
You might want to get rid of such comments with lines of *, and probably
reorder the inclusion to include first linux then asm headers.
> +#if defined(CONFIG_SATA_PNX833X) || defined(CONFIG_SATA_PNX833X_MODULE)
> +static struct resource pnx833x_sata_resources[] = {
> + [0] = {
> + .start = PNX8335_SATA_PORTS_START,
> + .end = PNX8335_SATA_PORTS_END,
> + .flags = IORESOURCE_MEM,
> + },
> + [1] = {
> + .start = PNX8335_PIC_SATA_INT,
> + .end = PNX8335_PIC_SATA_INT,
> + .flags = IORESOURCE_IRQ,
> + },
> +};
> +
> +static struct platform_device pnx833x_sata_device = {
> + .name = "pnx833x-sata",
> + .id = -1,
> + .num_resources = ARRAY_SIZE(pnx833x_sata_resources),
> + .resource = pnx833x_sata_resources,
> +};
> +#endif
What about defining those resources anyway ?
> +
> +#if defined(CONFIG_MTD_NAND_PLATFORM) ||
> defined(CONFIG_MTD_NAND_PLATFORM_MODULE)
Same here and others below too.
> +
> +#define STB225_NAND_BASE 0x18000000 /* I/O location(gets
> remapped)*/ +#define STB225_NAND_CLE_MASK 0x00100000 /* I/O location
> with CLE high */ +#define STB225_NAND_ALE_MASK 0x00010000 /* I/O
> location with ALE high */ +
You might want to put this in an header file instead.
> +void pnx833x_machine_halt(void)
> +{
> + printk(KERN_ALERT "\n\nSystem halted.\n\n");
> +
> + while (1)
> + __asm__ __volatile__ ("wait");
> +}
You might want to use cpu_relax(); instead of the assembly wait instruction.
> +
> +void pnx833x_machine_power_off(void)
> +{
> + printk(KERN_ALERT "\n\nPower off not implemented.");
> + pnx833x_machine_halt();
> +}
And put some less alarming message here, like "*** You can safely turn off the
board".
> +int __init plat_mem_setup(void)
> +{
> + /* fake pci bus to avoid bounce buffers */
> + PCI_DMA_BUS_IS_PHYS = 1;
> +
> + /* set mips clock to 320MHz */
> +#if defined(CONFIG_SOC_PNX8335)
> + PNX8335_WRITEFIELD(0x17, CLOCK_PLL_CPU_CTL, FREQ);
> +#endif
> + gpio_init(); /* so it will be ready in board_setup() */
You can move the GPIO code into its own C file, and use arch_initcall to
initialise it. Prefixing gpio_init with pnx83xx is better to be consistent
with other functions. See below for more comments about the GPIO code.
> +static inline unsigned long ip3902_read_reg(struct net_device *ndev, int
> reg) +{
> + unsigned long value = readl((void * __iomem)(ndev->base_addr +
> reg)); + return value;
Useless cast to void * __iomem in general, did your compiler produced a
warning on this ?
Netdev people will probably ask you to use NAPI unconditionnaly for new
drivers.
> +#ifdef IP3902_NAPI
> +static int ip3902_poll(struct napi_struct *napi, int budget)
> +{
> + struct ip3902_private *ip3902_priv = container_of(napi, struct
> ip3902_private, napi);
> + struct net_device *ndev = ip3902_priv->ndev;
> + int work_done;
> +
> + work_done = ip3902_eth_receive_queue(ndev, ip3902_priv, budget);
> +
> + if (work_done < budget) {
> + ip3902_write_reg(ndev, INT_CLEAR_REG, RX_DONE_INT);
> + ip3902_write_reg(ndev, INT_CLEAR_REG, 0);
> + netif_rx_complete(ndev, napi);
> + ip3902_write_reg(ndev, INT_ENABLE_REG, (TX_UNDERRUN_INT |
> RX_DONE_INT | RX_OVERRUN_INT));
> + }
> +
> + return work_done;
> +}
> +#endif
> +/* BIG FAT WARNING: races danger!
> + No protections exist here. Current users are only early init code,
> + when locking is not needed because no cuncurency yet exists there,
> + and GPIO IRQ dispatcher, which does locking.
> + However, if many uses will ever happen, proper locking will be needed
> + - including locking between different uses
You should consider using the GPIO library, or read what is done for BCM47xx,
AU1000 and TX4938. This should be easy since you comply with most of your
functions to this GPIO API. Providing your board specific gpio functions, the
rest of the API will handle locking and interrupt context for you.
Also, prefix your functions so that they will not collide with the other
implementations naming scheme.
Such functions might be provided by the C file instead, in the archicture
code. Remapping GPIO registers or things like that can be done there as well
at board boot time.
--
Cordialement, Florian Fainelli
------------------------------
