From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Richard_R=F6jfors?= Subject: Re: [PATCH v2 1/4] xilinx_spi: Split into of driver and generic part. Date: Thu, 12 Nov 2009 16:36:37 +0100 Message-ID: <4AFC2B85.7010606@mocean-labs.com> References: <4AFC1B1E.60304@mocean-labs.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Cc: spi-devel-general@lists.sourceforge.net, Andrew Morton , dbrownell@users.sourceforge.net, John Linn , linuxppc-dev@ozlabs.org To: Grant Likely Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linuxppc-dev-bounces+glppd-linuxppc64-dev=m.gmane.org@lists.ozlabs.org Errors-To: linuxppc-dev-bounces+glppd-linuxppc64-dev=m.gmane.org@lists.ozlabs.org List-Id: linux-spi.vger.kernel.org Hi, There are some proposed updates from you which are inherited from the old c= ode I think it's better that you (xilinx?) take responsibility for that part, I really don't want to maintain others' code which I even can't compile. There was a type error I will fix. Comments below... //Richard Grant Likely wrote: > On Thu, Nov 12, 2009 at 7:26 AM, Richard R=F6jfors > wrote: >> This patch splits the xilinx_spi driver into a generic part and a >> OF driver part. >> >> The reason for this is to later add in a platform driver as well. >> >> Signed-off-by: Richard R=F6jfors >> --- >> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig >> index 4b6f7cb..e60b264 100644 >> --- a/drivers/spi/Kconfig >> +++ b/drivers/spi/Kconfig >> @@ -236,7 +236,7 @@ config SPI_TXX9 >> >> config SPI_XILINX >> tristate "Xilinx SPI controller" >> - depends on (XILINX_VIRTEX || MICROBLAZE) && EXPERIMENTAL >> + depends on EXPERIMENTAL >> select SPI_BITBANG >> help >> This exposes the SPI controller IP from the Xilinx EDK. >> @@ -244,6 +244,12 @@ config SPI_XILINX >> See the "OPB Serial Peripheral Interface (SPI) (v1.00e)" >> Product Specification document (DS464) for hardware details. >> >> +config SPI_XILINX_OF >> + tristate "Xilinx SPI controller OF device" >> + depends on SPI_XILINX && (XILINX_VIRTEX || MICROBLAZE) >> + help >> + This is the OF driver for the SPI controller IP from the Xilin= x EDK. >> + >> # >> # Add new SPI master controllers in alphabetical order above this line >> # >> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile >> index 21a1182..97dee8f 100644 >> --- a/drivers/spi/Makefile >> +++ b/drivers/spi/Makefile >> @@ -31,6 +31,7 @@ obj-$(CONFIG_SPI_S3C24XX_GPIO) +=3D spi= _s3c24xx_gpio.o >> obj-$(CONFIG_SPI_S3C24XX) +=3D spi_s3c24xx.o >> obj-$(CONFIG_SPI_TXX9) +=3D spi_txx9.o >> obj-$(CONFIG_SPI_XILINX) +=3D xilinx_spi.o >> +obj-$(CONFIG_SPI_XILINX_OF) +=3D xilinx_spi_of.o >> obj-$(CONFIG_SPI_SH_SCI) +=3D spi_sh_sci.o >> obj-$(CONFIG_SPI_STMP3XXX) +=3D spi_stmp.o >> # ... add above this line ... >> diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c >> index 46b8c5c..5761a4c 100644 >> --- a/drivers/spi/xilinx_spi.c >> +++ b/drivers/spi/xilinx_spi.c >> @@ -14,16 +14,14 @@ >> #include >> #include >> #include >> -#include >> - >> -#include >> -#include >> -#include >> >> #include >> #include >> #include >> >> +#include "xilinx_spi.h" >> +#include >> + >> #define XILINX_SPI_NAME "xilinx_spi" >> >> /* Register definitions as per "OPB Serial Peripheral Interface (SPI) (= v1.00e) >> @@ -78,7 +76,7 @@ struct xilinx_spi { >> /* bitbang has to be first */ >> struct spi_bitbang bitbang; >> struct completion done; >> - >> + struct resource mem; /* phys mem */ >> void __iomem *regs; /* virt. address of the control registers= */ >> >> u32 irq; >> @@ -283,40 +281,22 @@ static irqreturn_t xilinx_spi_irq(int irq, void *d= ev_id) >> return IRQ_HANDLED; >> } >> >> -static int __init xilinx_spi_of_probe(struct of_device *ofdev, >> - const struct of_device_id *match) >> +struct spi_master *xilinx_spi_init(struct device *dev, struct resource = *mem, >> + u32 irq, s16 bus_num) >> { >> struct spi_master *master; >> struct xilinx_spi *xspi; >> - struct resource r_irq_struct; >> - struct resource r_mem_struct; >> - >> - struct resource *r_irq =3D &r_irq_struct; >> - struct resource *r_mem =3D &r_mem_struct; >> - int rc =3D 0; >> - const u32 *prop; >> - int len; >> - >> - /* Get resources(memory, IRQ) associated with the device */ >> - master =3D spi_alloc_master(&ofdev->dev, sizeof(struct xilinx_sp= i)); >> + struct xspi_platform_data *pdata =3D dev->platform_data; >> + int ret; >> >> - if (master =3D=3D NULL) { >> - return -ENOMEM; >> - } >> - >> - dev_set_drvdata(&ofdev->dev, master); >> - >> - rc =3D of_address_to_resource(ofdev->node, 0, r_mem); >> - if (rc) { >> - dev_warn(&ofdev->dev, "invalid address\n"); >> - goto put_master; >> + if (!pdata) { >> + dev_err(dev, "No platform data attached\n"); >> + return NULL; >> } >> >> - rc =3D of_irq_to_resource(ofdev->node, 0, r_irq); >> - if (rc =3D=3D NO_IRQ) { >> - dev_warn(&ofdev->dev, "no IRQ found\n"); >> - goto put_master; >> - } >> + master =3D spi_alloc_master(dev, sizeof(struct xilinx_spi)); >> + if (!master) >> + return NULL; >> >> /* the spi->mode bits understood by this driver: */ >> master->mode_bits =3D SPI_CPOL | SPI_CPHA; >> @@ -329,128 +309,67 @@ static int __init xilinx_spi_of_probe(struct of_d= evice *ofdev, >> xspi->bitbang.master->setup =3D xilinx_spi_setup; >> init_completion(&xspi->done); >> >> - xspi->irq =3D r_irq->start; >> - >> - if (!request_mem_region(r_mem->start, >> - r_mem->end - r_mem->start + 1, XILINX_SPI_NAME))= { >> - rc =3D -ENXIO; >> - dev_warn(&ofdev->dev, "memory request failure\n"); >> + if (!request_mem_region(mem->start, resource_size(mem), >> + XILINX_SPI_NAME)) >> goto put_master; >> - } >> >> - xspi->regs =3D ioremap(r_mem->start, r_mem->end - r_mem->start += 1); >> + xspi->regs =3D ioremap(mem->start, resource_size(mem)); >> if (xspi->regs =3D=3D NULL) { >> - rc =3D -ENOMEM; >> - dev_warn(&ofdev->dev, "ioremap failure\n"); >> - goto release_mem; >> + dev_warn(dev, "ioremap failure\n"); >> + goto map_failed; >> } >> - xspi->irq =3D r_irq->start; >> >> - /* dynamic bus assignment */ >> - master->bus_num =3D -1; >> + master->bus_num =3D bus_num; >> + master->num_chipselect =3D pdata->num_chipselect; >> >> - /* number of slave select bits is required */ >> - prop =3D of_get_property(ofdev->node, "xlnx,num-ss-bits", &len); >> - if (!prop || len < sizeof(*prop)) { >> - dev_warn(&ofdev->dev, "no 'xlnx,num-ss-bits' property\n"= ); >> - goto unmap_io; >> - } >> - master->num_chipselect =3D *prop; >> + xspi->mem =3D *mem; >> + xspi->irq =3D irq; >> >> /* SPI controller initializations */ >> xspi_init_hw(xspi->regs); >> >> /* Register for SPI Interrupt */ >> - rc =3D request_irq(xspi->irq, xilinx_spi_irq, 0, XILINX_SPI_NAME= , xspi); >> - if (rc !=3D 0) { >> - dev_warn(&ofdev->dev, "irq request failure: %d\n", xspi-= >irq); >> + ret =3D request_irq(xspi->irq, xilinx_spi_irq, 0, XILINX_SPI_NAM= E, xspi); >> + if (ret) >> goto unmap_io; >> - } >> >> - rc =3D spi_bitbang_start(&xspi->bitbang); >> - if (rc !=3D 0) { >> - dev_err(&ofdev->dev, "spi_bitbang_start FAILED\n"); >> + ret =3D spi_bitbang_start(&xspi->bitbang); >> + if (ret) { >> + dev_err(dev, "spi_bitbang_start FAILED\n"); >> goto free_irq; >> } >> >> - dev_info(&ofdev->dev, "at 0x%08X mapped to 0x%08X, irq=3D%d\n", >> - (unsigned int)r_mem->start, (u32)xspi->regs, xsp= i->irq); >> - >> - /* Add any subnodes on the SPI bus */ >> - of_register_spi_devices(master, ofdev->node); >> - >> - return rc; >> + dev_info(dev, "at 0x%08X mapped to 0x%08X, irq=3D%d\n", >> + (u32)mem->start, (u32)xspi->regs, xspi->irq); >> + return master; >> >> free_irq: >> free_irq(xspi->irq, xspi); >> unmap_io: >> iounmap(xspi->regs); >> -release_mem: >> - release_mem_region(r_mem->start, resource_size(r_mem)); >> +map_failed: >> + release_mem_region(mem->start, resource_size(mem)); >> put_master: >> spi_master_put(master); >> - return rc; >> + return NULL; >> } >> +EXPORT_SYMBOL(xilinx_spi_init); >> >> -static int __devexit xilinx_spi_remove(struct of_device *ofdev) >> +void xilinx_spi_deinit(struct spi_master *master) >> { >> struct xilinx_spi *xspi; >> - struct spi_master *master; >> - struct resource r_mem; >> >> - master =3D platform_get_drvdata(ofdev); >> xspi =3D spi_master_get_devdata(master); >> >> spi_bitbang_stop(&xspi->bitbang); >> free_irq(xspi->irq, xspi); >> iounmap(xspi->regs); >> - if (!of_address_to_resource(ofdev->node, 0, &r_mem)) >> - release_mem_region(r_mem.start, resource_size(&r_mem)); >> - dev_set_drvdata(&ofdev->dev, 0); >> - spi_master_put(xspi->bitbang.master); >> - >> - return 0; >> -} >> - >> -/* work with hotplug and coldplug */ >> -MODULE_ALIAS("platform:" XILINX_SPI_NAME); >> - >> -static int __exit xilinx_spi_of_remove(struct of_device *op) >> -{ >> - return xilinx_spi_remove(op); >> -} >> >> -static struct of_device_id xilinx_spi_of_match[] =3D { >> - { .compatible =3D "xlnx,xps-spi-2.00.a", }, >> - { .compatible =3D "xlnx,xps-spi-2.00.b", }, >> - {} >> -}; >> - >> -MODULE_DEVICE_TABLE(of, xilinx_spi_of_match); >> - >> -static struct of_platform_driver xilinx_spi_of_driver =3D { >> - .owner =3D THIS_MODULE, >> - .name =3D "xilinx-xps-spi", >> - .match_table =3D xilinx_spi_of_match, >> - .probe =3D xilinx_spi_of_probe, >> - .remove =3D __exit_p(xilinx_spi_of_remove), >> - .driver =3D { >> - .name =3D "xilinx-xps-spi", >> - .owner =3D THIS_MODULE, >> - }, >> -}; >> - >> -static int __init xilinx_spi_init(void) >> -{ >> - return of_register_platform_driver(&xilinx_spi_of_driver); >> + release_mem_region(xspi->mem.start, resource_size(&xspi->mem)); >> + spi_master_put(xspi->bitbang.master); >> } >> -module_init(xilinx_spi_init); >> +EXPORT_SYMBOL(xilinx_spi_deinit); >> >> -static void __exit xilinx_spi_exit(void) >> -{ >> - of_unregister_platform_driver(&xilinx_spi_of_driver); >> -} >> -module_exit(xilinx_spi_exit); >> MODULE_AUTHOR("MontaVista Software, Inc. "); >> MODULE_DESCRIPTION("Xilinx SPI driver"); >> MODULE_LICENSE("GPL"); >> diff --git a/drivers/spi/xilinx_spi.h b/drivers/spi/xilinx_spi.h >> new file mode 100644 >> index 0000000..d211acc >> --- /dev/null >> +++ b/drivers/spi/xilinx_spi.h >> @@ -0,0 +1,32 @@ >> +/* >> + * Xilinx SPI device driver API and platform data header file >> + * >> + * Copyright (c) 2009 Intel Corporation >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, write to the Free Software >> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. >> + */ >> + >> +#ifndef _XILINX_SPI_H_ >> +#define _XILINX_SPI_H_ >> + >> +#include >> +#include >> + >> +#define XILINX_SPI_NAME "xilinx_spi" >> + >> +struct spi_master *xilinx_spi_init(struct device *dev, struct resource = *mem, >> + u32 irq, s16 bus_num); >> + >> +void xilinx_spi_deinit(struct spi_master *master); >> +#endif >> diff --git a/drivers/spi/xilinx_spi_of.c b/drivers/spi/xilinx_spi_of.c >> new file mode 100644 >> index 0000000..8c3fb54 >> --- /dev/null >> +++ b/drivers/spi/xilinx_spi_of.c >> @@ -0,0 +1,136 @@ >> +/* >> + * Xilinx SPI OF device driver >> + * >> + * Copyright (c) 2009 Intel Corporation >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, write to the Free Software >> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. >> + */ >> + >> +/* Supports: >> + * Xilinx SPI devices as OF devices >> + * >> + * Inspired by xilinx_spi.c, 2002-2007 (c) MontaVista Software, Inc. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> +#include >> + >> +#include >> +#include "xilinx_spi.h" >> + >> + >> +static int __init xilinx_spi_of_probe(struct of_device *ofdev, >> + const struct of_device_id *match) >> +{ >> + struct resource r_irq_struct; >> + struct resource r_mem_struct; >> + struct spi_master *master; >> + >> + struct resource *r_irq =3D &r_irq_struct; >> + struct resource *r_mem =3D &r_mem_struct; > = > This r_{irq,mem}_struct/*r_{irq,mem} construct is really weird (I do > understand this is just copied from the old code). r_*_struct can > just be dropped and reference the structures with &r_irq and &_mem. This is just the old code from montavista, I can't update their code, I don't have the hardware or even a cross compiler for power pc. And even less a proper kernel config for ppc. > = >> + int rc =3D 0; >> + const u32 *prop; >> + int len; >> + >> + rc =3D of_address_to_resource(ofdev->node, 0, r_mem); >> + if (rc) { >> + dev_warn(&ofdev->dev, "invalid address\n"); >> + return rc; >> + } >> + >> + rc =3D of_irq_to_resource(ofdev->node, 0, r_irq); >> + if (rc =3D=3D NO_IRQ) { >> + dev_warn(&ofdev->dev, "no IRQ found\n"); >> + return -ENODEV; >> + } >> + >> + if (!ofdev->dev.platform_data) { >> + ofdev->dev.platform_data =3D >> + kzalloc(sizeof(struct xspi_platform_data), GFP_K= ERNEL); >> + if (!ofdev->dev.platform_data) >> + return -ENOMEM; >> + } > = > Minor memory leak. Anything alloced in the probe path should also be > freed in the remove path. It's not going to spiral out of control or > anything, but it is important to be strict about such things. Drop > the outer if{} block here and kfree platform_data on remove. Yeah I know I though about it, the problem is if a platform_data is already attached, then we brutally free it. That's why I was lazy. It will only "le= ak" once... > = >> + >> + /* number of slave select bits is required */ >> + prop =3D of_get_property(ofdev->node, "xlnx,num-ss-bits", &len); >> + if (!prop || len < sizeof(*prop)) { >> + dev_warn(&ofdev->dev, "no 'xlnx,num-ss-bits' property\n"= ); >> + return -EINVAL; >> + } >> + ofdev->dev.platform_data->num_chipselect =3D *prop; > = > Have you compile tested this? platform_data is a void*, the > dereference will not work. I know you don't have hardware; but > getting the needed cross compiler is easy. Damn this is an error. No I don't have a cross compiler or proper kernel co= nfig. I will have to update. > = >> + master =3D xilinx_spi_init(&ofdev->dev, r_mem, r_irq->start, -1); >> + if (!master) >> + return -ENODEV; >> + >> + dev_set_drvdata(&ofdev->dev, master); >> + >> + /* Add any subnodes on the SPI bus */ >> + of_register_spi_devices(master, ofdev->node); >> + >> + return 0; >> +} >> + >> +static int __devexit xilinx_spi_remove(struct of_device *ofdev) >> +{ >> + xilinx_spi_deinit(dev_get_drvdata(&ofdev->dev)); >> + dev_set_drvdata(&ofdev->dev, 0); >> + return 0; >> +} >> + >> +static int __exit xilinx_spi_of_remove(struct of_device *op) >> +{ >> + return xilinx_spi_remove(op); >> +} >> + >> +static struct of_device_id xilinx_spi_of_match[] =3D { >> + { .compatible =3D "xlnx,xps-spi-2.00.a", }, >> + { .compatible =3D "xlnx,xps-spi-2.00.b", }, >> + {} >> +}; >> + >> +MODULE_DEVICE_TABLE(of, xilinx_spi_of_match); >> + >> +static struct of_platform_driver xilinx_spi_of_driver =3D { >> + .owner =3D THIS_MODULE, >> + .name =3D "xilinx-xps-spi", > = > You can actually drop the above two lines. They aren't needed. Ok, this is old monta vista code. I really don't want to maintain power pc code. > = >> + .match_table =3D xilinx_spi_of_match, >> + .probe =3D xilinx_spi_of_probe, >> + .remove =3D __exit_p(xilinx_spi_of_remove), >> + .driver =3D { >> + .name =3D "xilinx-xps-spi", >> + .owner =3D THIS_MODULE, >> + }, >> +}; > =