[PATCH] Ported Xilinx GPIO driver to OpenFirmware.

[Stephen Neuendorffer]

Thanks Magnus...

Re: SPI I've been pondering some ideas along this line, but I haven't
done anything concrete. There are other cases where it makes sense to
have some information about the board-level design of the system (e.g.
ethernet PHYs.).

Steve

> -----Original Message-----
> From: Magnus Hjorth [mailto:mh at omnisys.se]
> Sent: Thursday, March 13, 2008 1:34 AM
> To: Stephen Neuendorffer; git
> Cc: linuxppc-embedded at ozlabs.org; 'Grant Likely'
> Subject: RE: [PATCH] Ported Xilinx GPIO driver to OpenFirmware.
> 
> 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 at xilinx.com]
> > Sent: den 11 mars 2008 18:36
> > To: Magnus Hjorth; git
> > Cc: linuxppc-embedded at 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