Re: [PATCH/RESEND v4 1/2] spi_topcliff_pch: support new deviceML7213 IOH

[Grant Likely]

On Tue, Jun 07, 2011 at 02:50:10PM +0900, Tomoya MORINAGA wrote:
> ***Modify Grant's comments.
>    - Delete unrelated whitespace
>    - Prevent device driver from accessing platform data
>    - Add __devinit and __devexit
>    - Save pdev->dev to pd_dev->dev.parent
>    - Have own suspend/resume processing in platform_driver.
>    - Care returned value in pch_spi_init
>    - Change unregister order
> 
> 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@xxxxxxxxxxxxxxx>
> ---

Hi Tomoya,

comment below...

> -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 = dev_get_platdata(&plat_dev->dev);
> +	struct pch_spi_data *data;
>  
> -	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;
> +	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);
> +	data = spi_master_get_devdata(master);
> +	data->master = master;
>  
> -	board_dat->pdev = pdev;
> +	platform_set_drvdata(plat_dev, data);
>  
> -	/* 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) */
> +	data->io_remap_addr = pci_iomap(board_dat->pdev, 1, 0) +
> +						   0x20 * plat_dev->id;
> +	if (!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, data->io_remap_addr);
>  
>  	/* initialize members of SPI master */
> -	master->bus_num = -1;
> +	master->bus_num = plat_dev->id;

This shouldn't be here.  The bus id should be dynamically allocated,
and using the plat_dev->id assumes that there are no other spi busses
in the system, which is a bad assumption.

I picked up the patch (it's about time I guess, I've left this out
alone for too long), but I've dropped this hunk.

You can post a followup patch if it broke anything.

g.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/