From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <stephen.neuendorffer@xilinx.com>
Received: from outbound8-dub-R.bigfish.com (outbound-dub.frontbridge.com
	[213.199.154.16])
	(using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))
	(Client CN "*.bigfish.com", Issuer "*.bigfish.com" (not verified))
	by ozlabs.org (Postfix) with ESMTPS id 11A4EDE321
	for <linuxppc-embedded@ozlabs.org>;
	Fri, 14 Mar 2008 04:18:25 +1100 (EST)
MIME-Version: 1.0
Content-Type: text/plain;
	charset="us-ascii"
Subject: RE: [PATCH] Ported Xilinx GPIO driver to OpenFirmware.
Date: Thu, 13 Mar 2008 10:18:00 -0700
In-Reply-To: <001601c884e4$ef90bff0$ceb23fd0$@se>
References: <000001c88392$c38a8c40$4a9fa4c0$@se>
	<20080311173602.8E61C728093@mail209-sin.bigfish.com>
	<001601c884e4$ef90bff0$ceb23fd0$@se>
From: "Stephen Neuendorffer" <stephen.neuendorffer@xilinx.com>
To: "Magnus Hjorth" <mh@omnisys.se>,
	"git" <git@xilinx.com>
Message-Id: <20080313171804.22E89E88074@mail153-dub.bigfish.com>
Cc: linuxppc-embedded@ozlabs.org
List-Id: Linux on Embedded PowerPC Developers Mail List
	<linuxppc-embedded.ozlabs.org>
List-Unsubscribe: <https://ozlabs.org/mailman/listinfo/linuxppc-embedded>,
	<mailto:linuxppc-embedded-request@ozlabs.org?subject=unsubscribe>
List-Archive: <http://ozlabs.org/pipermail/linuxppc-embedded>
List-Post: <mailto:linuxppc-embedded@ozlabs.org>
List-Help: <mailto:linuxppc-embedded-request@ozlabs.org?subject=help>
List-Subscribe: <https://ozlabs.org/mailman/listinfo/linuxppc-embedded>,
	<mailto:linuxppc-embedded-request@ozlabs.org?subject=subscribe>


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@omnisys.se]
> Sent: Thursday, March 13, 2008 1:34 AM
> To: Stephen Neuendorffer; git
> Cc: linuxppc-embedded@ozlabs.org; 'Grant Likely'
> Subject: RE: [PATCH] Ported Xilinx GPIO driver to OpenFirmware.
>=20
> Hi,
>=20
> Thanks for the feedback. I'll have a look into refining the patch in a
few weeks when I get some
> more time.
>=20
> 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.
>=20
> Cheers,
> Magnus
>=20
> > -----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 =3D 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 =3D container_of(inode->i_cdev, struct hwicap_drvdata,
> > cdev);
> > 	...
> > 	file->private_data =3D 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[] =3D {
> > > +	{.compatible =3D "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