| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 31 Mar 2011 16:38:38 -0600
From: Grant Likely <grant.likely@...retlab.ca>
To: Tomoya MORINAGA <tomoya-linux@....okisemi.com>
Cc: David Brownell <dbrownell@...rs.sourceforge.net>,
spi-devel-general@...ts.sourceforge.net,
linux-kernel@...r.kernel.org, qi.wang@...el.com,
yong.y.wang@...el.com, joel.clark@...el.com, kok.howg.ewe@...el.com
Subject: Re: [PATCH v3] spi_topcliff_pch: support new device ML7213 IOH
On Mon, Feb 28, 2011 at 08:47:57PM +0900, Tomoya MORINAGA wrote:
> Support ML7213 device of OKI SEMICONDUCTOR.
> ML7213 is companion chip of Intel Atom E6xx series for IVI(In-Vehicle Infotainment).
> ML7213 is compatible for Intel EG20T PCH.
>
> Signed-off-by: Tomoya MORINAGA <tomoya-linux@....okisemi.com>
Hi Tomoya,
I'm considerably happier with this patch. I have some more comments
below, but in general this is moving in the right direction. Good
work!
Sorry that I cannot pick it up for .39, I just didn't have the
bandwidth to give it the attention it required before the merge
window. I'll try to give the next version my utmost attention with
the hope that it can be merged early for .40
> ---
> drivers/spi/Kconfig | 5 +-
> drivers/spi/spi_topcliff_pch.c | 481 ++++++++++++++++++++--------------------
> 2 files changed, 250 insertions(+), 236 deletions(-)
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index bb233a9..6f6fb1f 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -351,12 +351,15 @@ config SPI_TEGRA
> SPI driver for NVidia Tegra SoCs
>
> config SPI_TOPCLIFF_PCH
> - tristate "Topcliff PCH SPI Controller"
> + tristate "Intel EG20T PCH/OKI SEMICONDUCTOR ML7213 IOH SPI controller"
> depends on PCI
> help
> SPI driver for the Topcliff PCH (Platform Controller Hub) SPI bus
> used in some x86 embedded processors.
>
> + This driver also supports the ML7213, a companion chip for the
> + Atom E6xx series and compatible with the Intel EG20T PCH.
> +
> config SPI_TXX9
> tristate "Toshiba TXx9 SPI controller"
> depends on GENERIC_GPIO && CPU_TX49XX
> diff --git a/drivers/spi/spi_topcliff_pch.c b/drivers/spi/spi_topcliff_pch.c
> index 79e48d4..93c9bce 100644
> --- a/drivers/spi/spi_topcliff_pch.c
> +++ b/drivers/spi/spi_topcliff_pch.c
> @@ -26,6 +26,7 @@
> #include <linux/spi/spidev.h>
> #include <linux/module.h>
> #include <linux/device.h>
> +#include <linux/platform_device.h>
>
> /* Register offsets */
> #define PCH_SPCR 0x00 /* SPI control register */
> @@ -35,6 +36,7 @@
> #define PCH_SPDRR 0x10 /* SPI read data register */
> #define PCH_SSNXCR 0x18 /* SSN Expand Control Register */
> #define PCH_SRST 0x1C /* SPI reset register */
> +#define PCH_SPI_ADDRESS_SIZE 0x20
>
> #define PCH_SPSR_TFD 0x000007C0
> #define PCH_SPSR_RFD 0x0000F800
> @@ -88,6 +90,16 @@
> #define PCH_CLOCK_HZ 50000000
> #define PCH_MAX_SPBR 1023
>
> +/* Definition for ML7213 by OKI SEMICONDUCTOR */
> +#define PCI_VENDOR_ID_ROHM 0x10DB
> +#define PCI_DEVICE_ID_ML7213_SPI 0x802c
> +
> +/*
> + * Set the number of SPI instance max
> + * Intel EG20T PCH : 1ch
> + * OKI SEMICONDUCTOR ML7213 IOH : 2ch
> +*/
> +#define PCH_SPI_MAX_DEV 2
>
> /**
> * struct pch_spi_data - Holds the SPI channel specific details
> @@ -121,6 +133,8 @@
> * @cur_trans: The current transfer that this SPI driver is
> * handling
> * @board_dat: Reference to the SPI device data structure
> + * @plat_dev: platform_device structure
> + * @ch: SPI channel number
> */
> struct pch_spi_data {
> void __iomem *io_remap_addr;
> @@ -144,6 +158,8 @@ struct pch_spi_data {
> struct spi_message *current_msg;
> struct spi_transfer *cur_trans;
> struct pch_spi_board_data *board_dat;
> + struct platform_device *plat_dev;
> + int ch;
> };
>
> /**
> @@ -153,6 +169,7 @@ struct pch_spi_data {
> * @pci_req_sts: Status of pci_request_regions
> * @suspend_sts: Status of suspend
> * @data: Pointer to SPI channel data structure
> + * @num: The number of SPI device instance
> */
> struct pch_spi_board_data {
> struct pci_dev *pdev;
> @@ -160,11 +177,18 @@ struct pch_spi_board_data {
> u8 pci_req_sts;
> u8 suspend_sts;
> struct pch_spi_data *data;
> + int num;
> +};
> +
> +struct pch_pd_dev_save {
> + int num;
> + struct platform_device *pd_save[PCH_SPI_MAX_DEV];
> };
>
> static struct pci_device_id pch_spi_pcidev_id[] = {
> - {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_GE_SPI)},
> - {0,}
> + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_GE_SPI), 1, },
> + { PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ML7213_SPI), 2, },
> + { }
> };
>
> /**
> @@ -298,7 +322,6 @@ static irqreturn_t pch_spi_handler(int irq, void *dev_id)
> data = board_dat->data;
> io_remap_addr = data->io_remap_addr;
> spsr = io_remap_addr + PCH_SPSR;
> -
> reg_spsr_val = ioread32(spsr);
>
> /* Check if the interrupt is for SPI device */
Unrelated whitespace change
> @@ -309,7 +332,6 @@ static irqreturn_t pch_spi_handler(int irq, void *dev_id)
>
> dev_dbg(&board_dat->pdev->dev, "%s EXIT return value=%d\n",
> __func__, ret);
> -
> return ret;
> }
>
Ditto. Lots more through the rest of the file. Please be careful
about this, it makes the patch hard to review.
> @@ -412,7 +434,6 @@ static int pch_spi_setup(struct spi_device *pspi)
>
> static int pch_spi_transfer(struct spi_device *pspi, struct spi_message *pmsg)
> {
> -
> struct spi_transfer *transfer;
> struct pch_spi_data *data = spi_master_get_devdata(pspi->master);
> int retval;
> @@ -469,7 +490,6 @@ static int pch_spi_transfer(struct spi_device *pspi, struct spi_message *pmsg)
> }
> }
> }
> -
> spin_lock_irqsave(&data->lock, flags);
>
> /* We won't process any messages if we have been asked to terminate */
> @@ -621,7 +641,6 @@ static void pch_spi_set_tx(struct pch_spi_data *data, int *bpw,
> data->transfer_active = true;
> }
>
> -
> static void pch_spi_nomore_transfer(struct pch_spi_data *data,
> struct spi_message *pmsg)
> {
> @@ -742,7 +761,6 @@ static void pch_spi_copy_rx_data(struct pch_spi_data *data, int bpw)
> }
> }
>
> -
> static void pch_spi_process_messages(struct work_struct *pwork)
> {
> struct spi_message *pmsg;
> @@ -884,46 +902,26 @@ static void pch_spi_free_resources(struct pch_spi_board_data *board_dat)
> /* disable interrupts & free IRQ */
> if (board_dat->irq_reg_sts) {
> /* disable interrupts */
> - pch_spi_setclr_reg(board_dat->data->master, PCH_SPCR, 0,
> - PCH_ALL);
> -
> - /* free IRQ */
> - free_irq(board_dat->pdev->irq, board_dat);
> + pch_spi_setclr_reg(board_dat->data->master, PCH_SPCR,
> + 0, PCH_ALL);
And again, it makes a 2 line change into 7
>
> dev_dbg(&board_dat->pdev->dev,
> - "%s free_irq invoked successfully\n", __func__);
> + "%s free_irq invoked successfully\n",
> + __func__);
Ditto
>
> board_dat->irq_reg_sts = false;
> }
> -
> - /* unmap PCI base address */
> - if (board_dat->data->io_remap_addr != 0) {
> - pci_iounmap(board_dat->pdev, board_dat->data->io_remap_addr);
> -
> - board_dat->data->io_remap_addr = 0;
> -
> - dev_dbg(&board_dat->pdev->dev,
> - "%s pci_iounmap invoked successfully\n", __func__);
> - }
> -
> - /* release PCI region */
> - if (board_dat->pci_req_sts) {
> - pci_release_regions(board_dat->pdev);
> - dev_dbg(&board_dat->pdev->dev,
> - "%s pci_release_regions invoked successfully\n",
> - __func__);
> - board_dat->pci_req_sts = false;
> - }
> }
>
> static int pch_spi_get_resources(struct pch_spi_board_data *board_dat)
> {
> - void __iomem *io_remap_addr;
> - int retval;
> + int retval = 0;
> +
> dev_dbg(&board_dat->pdev->dev, "%s ENTRY\n", __func__);
>
> /* create workqueue */
> board_dat->data->wk = create_singlethread_workqueue(KBUILD_MODNAME);
> +
> if (!board_dat->data->wk) {
> dev_err(&board_dat->pdev->dev,
> "%s create_singlet hread_workqueue failed\n", __func__);
> @@ -931,46 +929,13 @@ static int pch_spi_get_resources(struct pch_spi_board_data *board_dat)
> goto err_return;
> }
>
> - dev_dbg(&board_dat->pdev->dev,
> - "%s create_singlethread_workqueue success\n", __func__);
> -
> - retval = pci_request_regions(board_dat->pdev, KBUILD_MODNAME);
> - if (retval != 0) {
> - dev_err(&board_dat->pdev->dev,
> - "%s request_region failed\n", __func__);
> - goto err_return;
> - }
> -
> board_dat->pci_req_sts = true;
>
> - io_remap_addr = pci_iomap(board_dat->pdev, 1, 0);
> - if (io_remap_addr == 0) {
> - dev_err(&board_dat->pdev->dev,
> - "%s pci_iomap failed\n", __func__);
> - retval = -ENOMEM;
> - goto err_return;
> - }
> -
> - /* calculate base address for all channels */
> - board_dat->data->io_remap_addr = io_remap_addr;
> -
> /* reset PCH SPI h/w */
> pch_spi_reset(board_dat->data->master);
> dev_dbg(&board_dat->pdev->dev,
> "%s pch_spi_reset invoked successfully\n", __func__);
>
> - /* register IRQ */
> - retval = request_irq(board_dat->pdev->irq, pch_spi_handler,
> - IRQF_SHARED, KBUILD_MODNAME, board_dat);
> - if (retval != 0) {
> - dev_err(&board_dat->pdev->dev,
> - "%s request_irq failed\n", __func__);
> - goto err_return;
> - }
> -
> - dev_dbg(&board_dat->pdev->dev, "%s request_irq returned=%d\n",
> - __func__, retval);
> -
> board_dat->irq_reg_sts = true;
> dev_dbg(&board_dat->pdev->dev, "%s data->irq_reg_sts=true\n", __func__);
>
> @@ -986,131 +951,96 @@ err_return:
> return retval;
> }
>
> -static int pch_spi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +static int __devinit pch_spi_pd_probe(struct platform_device *plat_dev)
> {
> -
> + int ret;
> struct spi_master *master;
> -
> - struct pch_spi_board_data *board_dat;
> - int retval;
> -
> - dev_dbg(&pdev->dev, "%s ENTRY\n", __func__);
> -
> - /* allocate memory for private data */
> - board_dat = kzalloc(sizeof(struct pch_spi_board_data), GFP_KERNEL);
> - if (board_dat == NULL) {
> - dev_err(&pdev->dev,
> - " %s memory allocation for private data failed\n",
> - __func__);
> - retval = -ENOMEM;
> - goto err_kmalloc;
> - }
> -
> - dev_dbg(&pdev->dev,
> - "%s memory allocation for private data success\n", __func__);
> -
> - /* enable PCI device */
> - retval = pci_enable_device(pdev);
> - if (retval != 0) {
> - dev_err(&pdev->dev, "%s pci_enable_device FAILED\n", __func__);
> -
> - goto err_pci_en_device;
> + struct pch_spi_board_data *board_dat = dev_get_platdata(&plat_dev->dev);
> +
> + master = spi_alloc_master(&board_dat->pdev->dev,
> + sizeof(struct pch_spi_data));
> + if (!master) {
> + dev_err(&plat_dev->dev, "spi_alloc_master[%d] failed.\n",
> + plat_dev->id);
> + return -ENOMEM;
> }
>
> - dev_dbg(&pdev->dev, "%s pci_enable_device returned=%d\n",
> - __func__, retval);
> -
> - board_dat->pdev = pdev;
> + board_dat->data = spi_master_get_devdata(master);
> + board_dat->data->master = master;
You need to be careful here. board_dat is obtained from the platform
data pointer; but it is a /read only/ data structure. Device drivers
are not allowed to modify it (even in this case where another driver
in the same file produced it in the first place). The reason being is
that drivers need to support being unbound/rebound multiple times, and
subsequent bindings may not work if the data has been changed by a
driver.
Basically the rule is platform_data is read-only static information
about the device. Drivers that need to maintain their own state need
to allocated their own private data structure and keep a pointer to it
with platform_set_drvdata()
>
> - /* alllocate memory for SPI master */
> - master = spi_alloc_master(&pdev->dev, sizeof(struct pch_spi_data));
> - if (master == NULL) {
> - retval = -ENOMEM;
> - dev_err(&pdev->dev, "%s Fail.\n", __func__);
> - goto err_spi_alloc_master;
> + /* baseaddress + 0x20(offset) */
> + board_dat->data->io_remap_addr = pci_iomap(board_dat->pdev, 1, 0) +
> + 0x20 * plat_dev->id;
> + if (!board_dat->data->io_remap_addr) {
> + dev_err(&plat_dev->dev, "%s pci_iomap failed\n", __func__);
> + ret = -ENOMEM;
> + goto err_pci_iomap;
> }
>
> - dev_dbg(&pdev->dev,
> - "%s spi_alloc_master returned non NULL\n", __func__);
> + dev_dbg(&plat_dev->dev, "[ch%d] remap_addr=%p \n",
> + plat_dev->id, board_dat->data->io_remap_addr);
>
> /* initialize members of SPI master */
> - master->bus_num = -1;
> + master->bus_num = plat_dev->id;
> master->num_chipselect = PCH_MAX_CS;
> master->setup = pch_spi_setup;
> master->transfer = pch_spi_transfer;
> - dev_dbg(&pdev->dev,
> - "%s transfer member of SPI master initialized\n", __func__);
> -
> - board_dat->data = spi_master_get_devdata(master);
>
> - board_dat->data->master = master;
> + board_dat->data->plat_dev = plat_dev;
> board_dat->data->n_curnt_chip = 255;
> - board_dat->data->board_dat = board_dat;
> board_dat->data->status = STATUS_RUNNING;
> + board_dat->data->board_dat = board_dat;
> + board_dat->data->ch = plat_dev->id;
>
> INIT_LIST_HEAD(&board_dat->data->queue);
> spin_lock_init(&board_dat->data->lock);
> INIT_WORK(&board_dat->data->work, pch_spi_process_messages);
> init_waitqueue_head(&board_dat->data->wait);
>
> - /* allocate resources for PCH SPI */
> - retval = pch_spi_get_resources(board_dat);
> - if (retval) {
> - dev_err(&pdev->dev, "%s fail(retval=%d)\n", __func__, retval);
> + ret = pch_spi_get_resources(board_dat);
> + if (ret) {
> + dev_err(&plat_dev->dev, "%s fail(retval=%d)\n", __func__, ret);
> goto err_spi_get_resources;
> }
>
> - dev_dbg(&pdev->dev, "%s pch_spi_get_resources returned=%d\n",
> - __func__, retval);
> -
> - /* save private data in dev */
> - pci_set_drvdata(pdev, board_dat);
> - dev_dbg(&pdev->dev, "%s invoked pci_set_drvdata\n", __func__);
> + ret = request_irq(board_dat->pdev->irq, pch_spi_handler,
> + IRQF_SHARED, KBUILD_MODNAME, board_dat);
> + if (ret) {
> + dev_err(&plat_dev->dev,
> + "%s request_irq failed\n", __func__);
> + goto err_request_irq;
> + }
>
> - /* set master mode */
> pch_spi_set_master_mode(master);
> - dev_dbg(&pdev->dev,
> - "%s invoked pch_spi_set_master_mode\n", __func__);
>
> - /* Register the controller with the SPI core. */
> - retval = spi_register_master(master);
> - if (retval != 0) {
> - dev_err(&pdev->dev,
> + ret = spi_register_master(master);
> + if (ret != 0) {
> + dev_err(&plat_dev->dev,
> "%s spi_register_master FAILED\n", __func__);
> - goto err_spi_reg_master;
> + goto err_spi_register_master;
> }
>
> - dev_dbg(&pdev->dev, "%s spi_register_master returned=%d\n",
> - __func__, retval);
> -
> -
> return 0;
>
> -err_spi_reg_master:
> - spi_unregister_master(master);
> +err_spi_register_master:
> + free_irq(board_dat->pdev->irq, board_dat);
> +err_request_irq:
> + pch_spi_free_resources(board_dat);
> err_spi_get_resources:
> -err_spi_alloc_master:
> + pci_iounmap(board_dat->pdev, board_dat->data->io_remap_addr);
> +err_pci_iomap:
> spi_master_put(master);
> - pci_disable_device(pdev);
> -err_pci_en_device:
> - kfree(board_dat);
> -err_kmalloc:
> - return retval;
> +
> + return ret;
> }
>
> -static void pch_spi_remove(struct pci_dev *pdev)
> +static int __devexit pch_spi_pd_remove(struct platform_device *plat_dev)
> {
> - struct pch_spi_board_data *board_dat = pci_get_drvdata(pdev);
> + struct pch_spi_board_data *board_dat = dev_get_platdata(&plat_dev->dev);
> int count;
>
> - dev_dbg(&pdev->dev, "%s ENTRY\n", __func__);
> -
> - if (!board_dat) {
> - dev_err(&pdev->dev,
> - "%s pci_get_drvdata returned NULL\n", __func__);
> - return;
> - }
> -
> + dev_dbg(&plat_dev->dev, "%s:[ch%d] irq=%d \n",
> + __func__, plat_dev->id, board_dat->pdev->irq);
> /* check for any pending messages; no action is taken if the queue
> * is still full; but at least we tried. Unload anyway */
> count = 500;
> @@ -1125,116 +1055,217 @@ static void pch_spi_remove(struct pci_dev *pdev)
> }
> spin_unlock(&board_dat->data->lock);
>
> - /* Free resources allocated for PCH SPI */
> pch_spi_free_resources(board_dat);
> -
> + free_irq(board_dat->pdev->irq, board_dat);
> + pci_iounmap(board_dat->pdev, board_dat->data->io_remap_addr);
> spi_unregister_master(board_dat->data->master);
> + spi_master_put(board_dat->data->master);
> + platform_set_drvdata(plat_dev, NULL);
>
> - /* free memory for private data */
> - kfree(board_dat);
> -
> - pci_set_drvdata(pdev, NULL);
> -
> - /* disable PCI device */
> - pci_disable_device(pdev);
> -
> - dev_dbg(&pdev->dev, "%s invoked pci_disable_device\n", __func__);
> + return 0;
> }
>
> -#ifdef CONFIG_PM
> -static int pch_spi_suspend(struct pci_dev *pdev, pm_message_t state)
> +static int pch_spi_pd_suspend(struct platform_device *pd_dev, pm_message_t state)
> {
> u8 count;
> - int retval;
> -
> - struct pch_spi_board_data *board_dat = pci_get_drvdata(pdev);
> + struct pch_spi_board_data *board_dat = dev_get_platdata(&pd_dev->dev);
>
> - dev_dbg(&pdev->dev, "%s ENTRY\n", __func__);
> + dev_dbg(&pd_dev->dev, "%s ENTRY\n", __func__);
>
> if (!board_dat) {
> - dev_err(&pdev->dev,
> + dev_err(&pd_dev->dev,
> "%s pci_get_drvdata returned NULL\n", __func__);
> return -EFAULT;
> }
>
> - retval = 0;
> board_dat->suspend_sts = true;
>
> /* check if the current message is processed:
> Only after thats done the transfer will be suspended */
> count = 255;
> - while ((--count) > 0) {
> + while ((--count) > 0)
> if (!(board_dat->data->bcurrent_msg_processing)) {
> - dev_dbg(&pdev->dev, "%s board_dat->data->bCurrent_"
> - "msg_processing = false\n", __func__);
> break;
> - } else {
> - dev_dbg(&pdev->dev, "%s board_dat->data->bCurrent_msg_"
> - "processing = true\n", __func__);
> - }
> msleep(PCH_SLEEP_TIME);
> }
>
> /* Free IRQ */
> if (board_dat->irq_reg_sts) {
> /* disable all interrupts */
> - pch_spi_setclr_reg(board_dat->data->master, PCH_SPCR, 0,
> - PCH_ALL);
> + pch_spi_setclr_reg(board_dat->data->master, PCH_SPCR,
> + 0, PCH_ALL);
> pch_spi_reset(board_dat->data->master);
>
> free_irq(board_dat->pdev->irq, board_dat);
>
> board_dat->irq_reg_sts = false;
> - dev_dbg(&pdev->dev,
> + dev_dbg(&pd_dev->dev,
> "%s free_irq invoked successfully.\n", __func__);
> }
>
> + return 0;
> +}
> +
> +static int pch_spi_pd_resume(struct platform_device *pd_dev)
> +{
> + struct pch_spi_board_data *board_dat = dev_get_platdata(&pd_dev->dev);
> + int retval;
> +
> + if (!board_dat) {
> + dev_err(&pd_dev->dev,
> + "%s pci_get_drvdata returned NULL\n", __func__);
> + return -EFAULT;
> + }
> +
> + if (!board_dat->irq_reg_sts) {
> + /* register IRQ */
> + retval = request_irq(board_dat->pdev->irq, pch_spi_handler,
> + IRQF_SHARED, KBUILD_MODNAME, board_dat);
> + if (retval < 0) {
> + dev_err(&pd_dev->dev,
> + "%s request_irq failed\n", __func__);
> + return retval;
> + }
> + board_dat->irq_reg_sts = true;
> +
> + /* reset PCH SPI h/w */
> + pch_spi_reset(board_dat->data->master);
> + pch_spi_set_master_mode(board_dat->data->master);
> +
> + /* set suspend status to false */
> + board_dat->suspend_sts = false;
> +
> + }
> + return 0;
> +}
> +
> +static struct platform_driver pch_spi_pd_driver = {
> + .driver = {
> + .name = "pch-spi",
> + .owner = THIS_MODULE,
> + },
> + .probe = pch_spi_pd_probe,
> + .remove = __devexit_p(pch_spi_pd_remove),
> + .suspend = pch_spi_pd_suspend,
> + .resume = pch_spi_pd_resume
> +};
> +
> +static int pch_spi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
__devinit
> +{
> + struct pch_spi_board_data board_dat;
> + struct platform_device *pd_dev = NULL;
> + int retval;
> + int i;
> + struct pch_pd_dev_save *pd_dev_save;
> +
> + pd_dev_save = kzalloc(sizeof(struct pch_pd_dev_save), GFP_KERNEL);
> + if (!pd_dev_save) {
> + return -ENOMEM;
> + }
> +
> + retval = pci_request_regions(pdev, KBUILD_MODNAME);
> + if (retval) {
> + dev_err(&pdev->dev, "%s request_region failed\n", __func__);
> + return retval;
> + }
> +
> + memset(&board_dat, 0, sizeof(board_dat));
> + board_dat.pdev = pdev;
> + board_dat.num = id->driver_data;
> + pd_dev_save->num = id->driver_data;
> +
> + retval = pci_enable_device(pdev);
> + if (retval) {
> + dev_err(&pdev->dev, "%s pci_enable_device failed\n", __func__);
> + goto pci_enable_device;
> + }
> +
> + for (i = 0; i < board_dat.num; i++) {
> + pd_dev = platform_device_alloc("pch-spi", i);
> + if (!pd_dev) {
> + dev_err(&pdev->dev, "platform_device_alloc failed\n");
> + goto err_platform_device;
> + }
> + pd_dev->dev.release = NULL;
Don't clear the release pointer. It's there for a reason. When
the last reference to a platform_device is dropped, the release hook
allows it to be freed correctly.
> + pd_dev_save->pd_save[i] = pd_dev;
You also need:
pd_dev->parent = &pdev->dev;
So that the driver model shows the correct hierarchy.
> +
> + retval = platform_device_add_data(pd_dev, &board_dat,
> + sizeof(board_dat));
> + if (retval) {
> + dev_err(&pdev->dev,
> + "platform_device_add_data failed\n");
> + platform_device_put(pd_dev);
> + goto err_platform_device;
> + }
> +
> + retval = platform_device_add(pd_dev);
> + if (retval) {
> + dev_err(&pdev->dev, "platform_device_add failed\n");
> + platform_device_put(pd_dev);
> + goto err_platform_device;
> + }
> + }
> +
> + pci_set_drvdata(pdev, pd_dev_save);
> +
> + return 0;
> +
> +err_platform_device:
> + pci_disable_device(pdev);
> +pci_enable_device:
> + pci_release_regions(pdev);
> +
> + return retval;
> +}
> +
> +static void pch_spi_remove(struct pci_dev *pdev)
__devexit
> +{
> + int i;
> + struct pch_pd_dev_save *pd_dev_save = pci_get_drvdata(pdev);
> +
> + dev_dbg(&pdev->dev, "%s ENTRY:pdev=%p\n", __func__, pdev);
> +
> + for (i = 0; i < pd_dev_save->num; i++)
> + platform_device_unregister(pd_dev_save->pd_save[i]);
> +
> + pci_disable_device(pdev);
> + pci_release_regions(pdev);
> + kfree(pd_dev_save);
> +}
> +
> +#ifdef CONFIG_PM
> +static int pch_spi_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> + int retval;
> + int i;
> + struct pch_pd_dev_save *pd_dev_save = pci_get_drvdata(pdev);
> +
> + dev_dbg(&pdev->dev, "%s ENTRY\n", __func__);
> +
> + for (i = 0; i < pd_dev_save->num; i++)
> + pch_spi_pd_driver.suspend(pd_dev_save->pd_save[i], state);
The platform_driver should be doing its own suspend/resume. If it is
a child of the PCI device, then the driver model should handle the
ordering correctly.
Changing this should also further reduce this # of lines changed in the patch.
> +
> /* save config space */
> retval = pci_save_state(pdev);
> -
> if (retval == 0) {
> - dev_dbg(&pdev->dev, "%s pci_save_state returned=%d\n",
> - __func__, retval);
> - /* disable PM notifications */
> pci_enable_wake(pdev, PCI_D3hot, 0);
> - dev_dbg(&pdev->dev,
> - "%s pci_enable_wake invoked successfully\n", __func__);
> - /* disable PCI device */
> pci_disable_device(pdev);
> - dev_dbg(&pdev->dev,
> - "%s pci_disable_device invoked successfully\n",
> - __func__);
> - /* move device to D3hot state */
> pci_set_power_state(pdev, PCI_D3hot);
> - dev_dbg(&pdev->dev,
> - "%s pci_set_power_state invoked successfully\n",
> - __func__);
> } else {
> dev_err(&pdev->dev, "%s pci_save_state failed\n", __func__);
> }
>
> - dev_dbg(&pdev->dev, "%s return=%d\n", __func__, retval);
> -
> return retval;
> }
>
> static int pch_spi_resume(struct pci_dev *pdev)
> {
> int retval;
> -
> - struct pch_spi_board_data *board = pci_get_drvdata(pdev);
> + int i;
> + struct pch_pd_dev_save *pd_dev_save = pci_get_drvdata(pdev);
> dev_dbg(&pdev->dev, "%s ENTRY\n", __func__);
>
> - if (!board) {
> - dev_err(&pdev->dev,
> - "%s pci_get_drvdata returned NULL\n", __func__);
> - return -EFAULT;
> - }
> -
> - /* move device to DO power state */
> pci_set_power_state(pdev, PCI_D0);
> -
> - /* restore state */
> pci_restore_state(pdev);
>
> retval = pci_enable_device(pdev);
> @@ -1242,34 +1273,12 @@ static int pch_spi_resume(struct pci_dev *pdev)
> dev_err(&pdev->dev,
> "%s pci_enable_device failed\n", __func__);
> } else {
> - /* disable PM notifications */
> pci_enable_wake(pdev, PCI_D3hot, 0);
>
> - /* register IRQ handler */
> - if (!board->irq_reg_sts) {
> - /* register IRQ */
> - retval = request_irq(board->pdev->irq, pch_spi_handler,
> - IRQF_SHARED, KBUILD_MODNAME,
> - board);
> - if (retval < 0) {
> - dev_err(&pdev->dev,
> - "%s request_irq failed\n", __func__);
> - return retval;
> - }
> - board->irq_reg_sts = true;
> -
> - /* reset PCH SPI h/w */
> - pch_spi_reset(board->data->master);
> - pch_spi_set_master_mode(board->data->master);
> -
> - /* set suspend status to false */
> - board->suspend_sts = false;
> -
> - }
> + for (i = 0; i < pd_dev_save->num; i++)
> + pch_spi_pd_driver.resume(pd_dev_save->pd_save[i]);
> }
>
> - dev_dbg(&pdev->dev, "%s returning=%d\n", __func__, retval);
> -
> return retval;
> }
> #else
> @@ -1289,15 +1298,17 @@ static struct pci_driver pch_spi_pcidev = {
>
> static int __init pch_spi_init(void)
> {
> + platform_driver_register(&pch_spi_pd_driver);
> return pci_register_driver(&pch_spi_pcidev);
This doesn't handle a failure in platform_device_register().
Personally, I'd suggest having the PCI and platform_driver portions in
separate modules so that each module does it's own error handling, but
I won't make a big deal about this.
> }
> module_init(pch_spi_init);
>
> static void __exit pch_spi_exit(void)
> {
> + platform_driver_unregister(&pch_spi_pd_driver);
> pci_unregister_driver(&pch_spi_pcidev);
Typically unregistration should be in reverse order from registration.
> }
> module_exit(pch_spi_exit);
>
> MODULE_LICENSE("GPL");
> -MODULE_DESCRIPTION("Topcliff PCH SPI PCI Driver");
> +MODULE_DESCRIPTION("Intel EG20T PCH/OKI SEMICONDUCTOR ML7213 IOH SPI Driver");
> --
> 1.7.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists