[PATCH v2 1/4] xilinx_spi: Split into of driver and generic part.
Richard Röjfors
richard.rojfors at mocean-labs.com
Fri Nov 13 02:36:37 EST 2009
Hi,
There are some proposed updates from you which are inherited from the old code
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öjfors
> <richard.rojfors at mocean-labs.com> 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öjfors <richard.rojfors at mocean-labs.com>
>> ---
>> 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 Xilinx 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) += spi_s3c24xx_gpio.o
>> obj-$(CONFIG_SPI_S3C24XX) += spi_s3c24xx.o
>> obj-$(CONFIG_SPI_TXX9) += spi_txx9.o
>> obj-$(CONFIG_SPI_XILINX) += xilinx_spi.o
>> +obj-$(CONFIG_SPI_XILINX_OF) += xilinx_spi_of.o
>> obj-$(CONFIG_SPI_SH_SCI) += spi_sh_sci.o
>> obj-$(CONFIG_SPI_STMP3XXX) += 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 <linux/module.h>
>> #include <linux/init.h>
>> #include <linux/interrupt.h>
>> -#include <linux/platform_device.h>
>> -
>> -#include <linux/of_platform.h>
>> -#include <linux/of_device.h>
>> -#include <linux/of_spi.h>
>>
>> #include <linux/spi/spi.h>
>> #include <linux/spi/spi_bitbang.h>
>> #include <linux/io.h>
>>
>> +#include "xilinx_spi.h"
>> +#include <linux/spi/xilinx_spi.h>
>> +
>> #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 *dev_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 = &r_irq_struct;
>> - struct resource *r_mem = &r_mem_struct;
>> - int rc = 0;
>> - const u32 *prop;
>> - int len;
>> -
>> - /* Get resources(memory, IRQ) associated with the device */
>> - master = spi_alloc_master(&ofdev->dev, sizeof(struct xilinx_spi));
>> + struct xspi_platform_data *pdata = dev->platform_data;
>> + int ret;
>>
>> - if (master == NULL) {
>> - return -ENOMEM;
>> - }
>> -
>> - dev_set_drvdata(&ofdev->dev, master);
>> -
>> - rc = 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 = of_irq_to_resource(ofdev->node, 0, r_irq);
>> - if (rc == NO_IRQ) {
>> - dev_warn(&ofdev->dev, "no IRQ found\n");
>> - goto put_master;
>> - }
>> + master = spi_alloc_master(dev, sizeof(struct xilinx_spi));
>> + if (!master)
>> + return NULL;
>>
>> /* the spi->mode bits understood by this driver: */
>> master->mode_bits = SPI_CPOL | SPI_CPHA;
>> @@ -329,128 +309,67 @@ static int __init xilinx_spi_of_probe(struct of_device *ofdev,
>> xspi->bitbang.master->setup = xilinx_spi_setup;
>> init_completion(&xspi->done);
>>
>> - xspi->irq = r_irq->start;
>> -
>> - if (!request_mem_region(r_mem->start,
>> - r_mem->end - r_mem->start + 1, XILINX_SPI_NAME)) {
>> - rc = -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 = ioremap(r_mem->start, r_mem->end - r_mem->start + 1);
>> + xspi->regs = ioremap(mem->start, resource_size(mem));
>> if (xspi->regs == NULL) {
>> - rc = -ENOMEM;
>> - dev_warn(&ofdev->dev, "ioremap failure\n");
>> - goto release_mem;
>> + dev_warn(dev, "ioremap failure\n");
>> + goto map_failed;
>> }
>> - xspi->irq = r_irq->start;
>>
>> - /* dynamic bus assignment */
>> - master->bus_num = -1;
>> + master->bus_num = bus_num;
>> + master->num_chipselect = pdata->num_chipselect;
>>
>> - /* number of slave select bits is required */
>> - prop = 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 = *prop;
>> + xspi->mem = *mem;
>> + xspi->irq = irq;
>>
>> /* SPI controller initializations */
>> xspi_init_hw(xspi->regs);
>>
>> /* Register for SPI Interrupt */
>> - rc = request_irq(xspi->irq, xilinx_spi_irq, 0, XILINX_SPI_NAME, xspi);
>> - if (rc != 0) {
>> - dev_warn(&ofdev->dev, "irq request failure: %d\n", xspi->irq);
>> + ret = request_irq(xspi->irq, xilinx_spi_irq, 0, XILINX_SPI_NAME, xspi);
>> + if (ret)
>> goto unmap_io;
>> - }
>>
>> - rc = spi_bitbang_start(&xspi->bitbang);
>> - if (rc != 0) {
>> - dev_err(&ofdev->dev, "spi_bitbang_start FAILED\n");
>> + ret = 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=%d\n",
>> - (unsigned int)r_mem->start, (u32)xspi->regs, xspi->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=%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 = platform_get_drvdata(ofdev);
>> xspi = 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[] = {
>> - { .compatible = "xlnx,xps-spi-2.00.a", },
>> - { .compatible = "xlnx,xps-spi-2.00.b", },
>> - {}
>> -};
>> -
>> -MODULE_DEVICE_TABLE(of, xilinx_spi_of_match);
>> -
>> -static struct of_platform_driver xilinx_spi_of_driver = {
>> - .owner = THIS_MODULE,
>> - .name = "xilinx-xps-spi",
>> - .match_table = xilinx_spi_of_match,
>> - .probe = xilinx_spi_of_probe,
>> - .remove = __exit_p(xilinx_spi_of_remove),
>> - .driver = {
>> - .name = "xilinx-xps-spi",
>> - .owner = 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. <source at mvista.com>");
>> 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 <linux/spi/spi.h>
>> +#include <linux/spi/spi_bitbang.h>
>> +
>> +#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 <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +
>> +#include <linux/of_platform.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_spi.h>
>> +
>> +#include <linux/spi/xilinx_spi.h>
>> +#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 = &r_irq_struct;
>> + struct resource *r_mem = &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 = 0;
>> + const u32 *prop;
>> + int len;
>> +
>> + rc = of_address_to_resource(ofdev->node, 0, r_mem);
>> + if (rc) {
>> + dev_warn(&ofdev->dev, "invalid address\n");
>> + return rc;
>> + }
>> +
>> + rc = of_irq_to_resource(ofdev->node, 0, r_irq);
>> + if (rc == NO_IRQ) {
>> + dev_warn(&ofdev->dev, "no IRQ found\n");
>> + return -ENODEV;
>> + }
>> +
>> + if (!ofdev->dev.platform_data) {
>> + ofdev->dev.platform_data =
>> + kzalloc(sizeof(struct xspi_platform_data), GFP_KERNEL);
>> + 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 "leak"
once...
>
>> +
>> + /* number of slave select bits is required */
>> + prop = 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 = *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 config.
I will have to update.
>
>> + master = 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[] = {
>> + { .compatible = "xlnx,xps-spi-2.00.a", },
>> + { .compatible = "xlnx,xps-spi-2.00.b", },
>> + {}
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, xilinx_spi_of_match);
>> +
>> +static struct of_platform_driver xilinx_spi_of_driver = {
>> + .owner = THIS_MODULE,
>> + .name = "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 = xilinx_spi_of_match,
>> + .probe = xilinx_spi_of_probe,
>> + .remove = __exit_p(xilinx_spi_of_remove),
>> + .driver = {
>> + .name = "xilinx-xps-spi",
>> + .owner = THIS_MODULE,
>> + },
>> +};
>
More information about the Linuxppc-dev
mailing list