[PATCH] Ported Xilinx GPIO driver to OpenFirmware.

Stephen Neuendorffer stephen.neuendorffer at xilinx.com
Wed Mar 12 04:35:59 EST 2008


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




More information about the Linuxppc-embedded mailing list