Message-ID: <000d01cb4d8b$7f6d1800$66f8800a@maildom.okisemi.com>
Date:	Mon, 6 Sep 2010 15:19:40 +0900
From:	"Masayuki Ohtake" <masa-korg@....okisemi.com>
To:	"Grant Likely" <grant.likely@...retlab.ca>
Cc:	<meego-dev@...go.com>, "LKML" <linux-kernel@...r.kernel.org>,
	"David Brownell" <dbrownell@...rs.sourceforge.net>,
	<qi.wang@...el.com>, <yong.y.wang@...el.com>,
	<andrew.chih.howe.khor@...el.com>, <arjan@...ux.intel.com>,
	<gregkh@...e.de>, "Tomoya MORINAGA" <morinaga526@....okisemi.com>
Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_SPI driver to 2.6.35

Hi Grant,

Thank you for your comments.
We will modify for your comments.

> The email client you're currently using is posting the
> patches in HTML format, and is mangling them.  git send-email can help
> you here.
In our network environment, we cannot use send-email command.
We  have used Thunderbird with according to Documentation/email-clients.txt
It may be the reason we are use Thunderbird with pre-format mode.
Can't we use Thunderbird with pre-format mode ?

Thanks, Ohtake(OKISemi)

----- Original Message ----- 
From: "Grant Likely" <grant.likely@...retlab.ca>
To: "Masayuki Ohtak" <masa-korg@....okisemi.com>
Cc: <meego-dev@...go.com>; "LKML" <linux-kernel@...r.kernel.org>; "David Brownell" <dbrownell@...rs.sourceforge.net>;
<qi.wang@...el.com>; <yong.y.wang@...el.com>; <andrew.chih.howe.khor@...el.com>; <arjan@...ux.intel.com>;
<gregkh@...e.de>; "Tomoya MORINAGA" <morinaga526@....okisemi.com>
Sent: Friday, September 03, 2010 11:46 PM
Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_SPI driver to 2.6.35


> 2010/9/3 Masayuki Ohtak <masa-korg@....okisemi.com>:
> > SPI driver of Topcliff PCH
> >
> > Topcliff PCH is the platform controller hub that is going to be used in
> > Intel's upcoming general embedded platform. All IO peripherals in
> > Topcliff PCH are actually devices sitting on AMBA bus.
> > Topcliff PCH has SPI I/F. Using this I/F, it is able to access system
> > devices connected to SPI.
> >
> > Signed-off-by: Masayuki Ohtake <masa-korg@....okisemi.com>
>
> Hi Ohtak-san
>
> Comments below.  Also, please also fix the method that you're using to
> post patches.  The email client you're currently using is posting the
> patches in HTML format, and is mangling them.  git send-email can help
> you here.
>
> > ---
> >  drivers/spi/Kconfig   |   19 +
> >  drivers/spi/Makefile  |    1 +
> >  drivers/spi/spi_pch.c | 1813
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 1833 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/spi/spi_pch.c
> >
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index 91c2f4f..2c93667 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -53,6 +53,25 @@ if SPI_MASTER
> >
> >  comment "SPI Master Controller Drivers"
> >
> > +config PCH_SPI
> > +# tristate "PCH SPI Controller"
> > + boolean "PCH SPI Controller"
> > + depends on PCI
> > + help
> > +   This driver is for PCH(Platform controller Hub) SPI of Topcliff which
> > +   is an IOH(Input/Output Hub) for x86 embedded processor.
> > +   This driver can access PCH SPI bus device.
> > +
> > +config PCH_SPI_PLATFORM_DEVICE_COUNT
> > + int "PCH SPI Bus count"
> > + range 1 2
> > + depends on PCH_SPI
> > + help
> > +   This driver is for PCH(Platform controller Hub) SPI of Topcliff which
> > +   is an IOH(Input/Output Hub) for x86 embedded processor.
> > +   The number of SPI buses/channels supported by the PCH SPI controller.
> > +   PCH SPI of Topcliff supports only one channel.
> > +
>
> As previously discussed, PCH_SPI_PLATFORM_DEVICE_COUNT needs to be
> removed (see below).
>
> [...]
>
> > +static int pch_spi_probe(struct pci_dev *pdev, const struct pci_device_id
> > *id)
> > +{
> > +
> > + struct spi_master *master[PCH_MAX_DEV];
> > +
> > + struct pch_spi_board_data *board_dat;
> > + int retval, i;
> > + int spi_alloc_num, master_num;
> > +
> > + 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;
> > + }
> > +
> > + dev_dbg(&pdev->dev, "%s pci_enable_device returned=%d\n",
> > + __func__, retval);
> > +
> > + board_dat->pdev = pdev;
> > +
> > + /* alllocate memory for SPI master */
> > + for (i = 0, spi_alloc_num = 0; i < PCH_MAX_DEV; i++, spi_alloc_num++) {
> > + master[i] = spi_alloc_master(&pdev->dev,
> > +      sizeof(struct pch_spi_data));
> > +
> > + if (master[i] == NULL) {
> > + retval = -ENOMEM;
> > + dev_err(&pdev->dev, "%s [ch=%d]Fail.\n", __func__, i);
> > + goto err_spi_alloc_master;
> > + }
> > + }
> > +
> > + dev_dbg(&pdev->dev,
> > + "%s spi_alloc_master returned non NULL\n", __func__);
> > +
> > + /* initialize members of SPI master */
> > + for (i = 0, master_num = 0; i < PCH_MAX_DEV;
> > + i++, master_num++) {
> > + master[i]->bus_num = i;
>
> This is not okay.  The driver cannot assume that it provides the only
> SPI busses in the system.  The bus number should either be set to -1
> so that it gets auto-assigned, or the bus number should be passed in
> by board-specific setup code.
>
> > + master[i]->num_chipselect = PCH_MAX_CS;
> > + master[i]->setup = pch_spi_setup;
> > + dev_dbg(&pdev->dev,
> > + "%s setup member of SPI master initialized\n",
> > + __func__);
> > + master[i]->transfer = pch_spi_transfer;
> > + dev_dbg(&pdev->dev,
> > + "%s transfer member of SPI master initialized\n",
> > + __func__);
> > +
> > + board_dat->data[i] = spi_master_get_devdata(master[i]);
> > +
> > + board_dat->data[i]->master = master[i];
> > + board_dat->data[i]->n_curnt_chip = 255;
> > + board_dat->data[i]->current_chip = NULL;
> > + board_dat->data[i]->transfer_complete = false;
> > + board_dat->data[i]->pkt_tx_buff = NULL;
> > + board_dat->data[i]->pkt_rx_buff = NULL;
> > + board_dat->data[i]->tx_index = 0;
> > + board_dat->data[i]->rx_index = 0;
> > + board_dat->data[i]->transfer_active = false;
> > + board_dat->data[i]->board_dat = board_dat;
> > + board_dat->suspend_sts = false;
> > +
> > + mutex_init(&board_dat->data[i]->mutex);
> > + }
> > +
> > +
> > + /* allocate resources for PCH SPI */
> > + retval = pch_spi_get_resources(board_dat);
> > + if (retval != 0) {
> > + dev_err(&pdev->dev, "%s fail(retval=%d)\n",
> > + __func__, retval);
> > + 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, (void *)board_dat);
> > + dev_dbg(&pdev->dev, "%s invoked pci_set_drvdata\n", __func__);
> > +
> > + /* set master mode */
> > + for (i = 0; i < PCH_MAX_DEV; i++) {
> > + pch_spi_set_master_mode(master[i]);
> > + dev_dbg(&pdev->dev,
> > + "%s invoked pch_spi_set_master_mode\n", __func__);
> > + }
> > +
> > + /* Register the controller with the SPI core. */
> > + for (i = 0, master_num = 0; i < PCH_MAX_DEV; i++, master_num++) {
> > + retval = spi_register_master(master[i]);
> > + if (retval != 0) {
> > + dev_err(&pdev->dev,
> > + "%s spi_register_master FAILED\n", __func__);
> > + goto err_spi_reg_master;
> > + }
> > + }
>
> I *finally* understand what you're trying to do here.  It looks like
> you have a single PCI device that has multiple independent SPI busses.
>  It also appears that this driver attempts to handle all of the
> instances within a single context.
>
> Doing it this way overcomplicates the driver because you need to have
> for() loops all over the driver to deal with the multiple instances.
>
> Instead, the majority of the code should be structured to handle one
> and only one spi bus instance.  Only at the top level pci probe,
> remove and PM code should there be for() loops for setting up or
> tearing down each spi bus instance one at a time.
>
> In fact, it may be better for the pci_probe() hook to simply register
> one or more platform_devices; one for each instance of the SPI bus,
> and then have a separate platform_driver that binds to each instance.
>
> > +static int __init pch_spi_init(void)
> > +{
> > +#ifdef PCH_SPIDEV_REG_OLD
> > + int retval;
> > +
> > + retval = spi_register_board_info
> > + (pch_spi_slaves, ARRAY_SIZE(pch_spi_slaves));
> > + if (retval)
> > + return retval;
> > +#endif
>
> As we discussed previously, this driver must *not* register child spi
> devices with static code like this.  Remove the above lines and the
> pch_spi_slaves array.
>
> > + return pci_register_driver(&pch_spi_pcidev);
> > +}
> > +
> > +
> > +static void __exit pch_spi_exit(void)
> > +{
> > + pci_unregister_driver(&pch_spi_pcidev);
> > +}
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("PCH SPI PCI Driver");
> > +module_init(pch_spi_init);
> > +module_exit(pch_spi_exit);
>
> module_init() and modules_exit() should appear immediately after the
> functions they register as init and exit hooks.
>
> -- 
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
>


--
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