RE: [PATCH] Ported Xilinx GPIO driver to OpenFirmware.

["Magnus Hjorth" <mh@omnisys.se>]

Hi,

Thanks for the feedback. I'll have a look into refining the patch in a few weeks when I get some
more time. 

I have also been tinkering a little with the SPI driver, and that got me thinking. Wouldn't it be
great if SPI controllers and devices could be specified in the OF device tree and registered on boot
time? Even better if SPI worked as a true bus in EDK, with placeholder IP-cores for each slave
device, so such device entries could be autogenerated.

Cheers,
Magnus

> -----Original Message-----
> From: Stephen Neuendorffer [mailto:stephen.neuendorffer@xilinx.com]
> Sent: den 11 mars 2008 18:36
> To: Magnus Hjorth; git
> Cc: linuxppc-embedded@ozlabs.org; Grant Likely
> Subject: RE: [PATCH] Ported Xilinx GPIO driver to OpenFirmware.
> 
> 
> 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