[PATCH] Ported Xilinx GPIO driver to OpenFirmware.

[Stephen Neuendorffer]

Thanks Magnus!

Generally speaking this looks reasonable.  Some comments:

>  struct xgpio_instance {
>  	struct list_head link;
>  	unsigned long base_phys;	/* GPIO base address - physical
*/
>  	unsigned long remap_size;
> -	u32 device_id;
> +	u32 device_id;		/* Dev ID for platform devices, 0 for OF
devices */
> +	void *of_id;		/* of_dev pointer for OF devices, NULL
for plat devices */

Why have separate ids?  I don't think the of_dev needs to be kept around
here.  This driver seems seems awkwardly written to have a local list of
all the devices, rather than simply attaching the xgpio_instance as the
private data of the file.

For instance, in drivers/char/xilinx_hwicap.c:

static ssize_t
hwicap_read(struct file *file, char __user *buf, size_t count, loff_t
*ppos)
{
	struct hwicap_drvdata *drvdata = file->private_data;

and the drvdata is set in open:

static int hwicap_open(struct inode *inode, struct file *file)
{
	struct hwicap_drvdata *drvdata;
	int status;

	drvdata = container_of(inode->i_cdev, struct hwicap_drvdata,
cdev);
	...
	file->private_data = drvdata;

Which would work if xgpio_instance directly contains the struct
miscdevice.
I think this is a much cleaner pattern (although it took me a while to
figure out the magic that makes it work... )

> +static struct of_device_id xgpio_of_match[] = {
> +	{.compatible = "xlnx,xps-gpio-1.00.a"},

This should also probably contain the corresponding strings for the
following as well:
      opb_gpio_v1_00_a
      opb_gpio_v2_00_a
      opb_gpio_v3_01_a
      opb_gpio_v3_01_b
	plb_gpio_v1_00_b

This would seem to be a relatively easy driver to clean up (by pulling
it all into one file and converting the other code to the kernel style)
and submit to mainline, if you're interested?

Steve