Re: [PATCH 2/2] spi: Add SPI master controller for OCTEON SOCs.

[Grant Likely <grant.likely@secretlab.ca>]

On Fri, 11 May 2012 14:34:46 -0700, David Daney <ddaney.cavm@gmail.com> wrote:
> From: David Daney <david.daney@cavium.com>
> 
> Add the driver, link it into the kbuild system and provide device tree
> binding documentation.
> 
> Signed-off-by: David Daney <david.daney@cavium.com>

Some comments below, but you can add my a-b:

Acked-by: Grant Likely <grant.likely@secretlab.ca>

> +#include <asm/octeon/octeon.h>
> +#include <asm/octeon/cvmx-mpi-defs.h>
> +
> +#define DRV_VERSION "2.0" /* Version 1 was the out-of-tree driver */

As already discussed, drop this line.

> +#define DRV_DESCRIPTION "Cavium, Inc. OCTEON SPI bus driver"

Used exactly once.  Drop this line and move string to the
MODULE_DESCRIPTION().

> +static int __devinit octeon_spi_probe(struct platform_device *pdev)
> +{
> +
> +	struct resource *res_mem;
> +	struct spi_master *master;
> +	struct octeon_spi *p;
> +	int err = -ENOENT;
> +
> +	master = spi_alloc_master(&pdev->dev, sizeof(struct octeon_spi));
> +	if (!master)
> +		return -ENOMEM;
> +	p = spi_master_get_devdata(master);
> +	platform_set_drvdata(pdev, p);
> +	p->my_master = master;
> +
> +	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	if (res_mem == NULL) {
> +		dev_err(&pdev->dev, "found no memory resource\n");
> +		err = -ENXIO;
> +		goto fail;
> +	}
> +	if (!devm_request_mem_region(&pdev->dev, res_mem->start,
> +				     resource_size(res_mem), res_mem->name)) {
> +		dev_err(&pdev->dev, "request_mem_region failed\n");
> +		goto fail;
> +	}
> +	p->register_base = (u64)devm_ioremap(&pdev->dev, res_mem->start,
> +					     resource_size(res_mem));

Nasty cast.  p->register_base needs to be an __iomem pointer
variable.  The fact taht cvmx_read_csr accepts a uint64_t instead of
an __iomem pointer looks really wrong.  Why is it written that way?

> +
> +	/* Dynamic bus numbering */
> +	master->bus_num = -1;
> +	master->num_chipselect = 4;
> +	master->mode_bits = SPI_CPHA |
> +			    SPI_CPOL |
> +			    SPI_CS_HIGH |
> +			    SPI_LSB_FIRST |
> +			    SPI_3WIRE;
> +
> +	master->setup = octeon_spi_setup;
> +	master->cleanup = octeon_spi_cleanup;
> +	master->prepare_transfer_hardware = octeon_spi_nop_transfer_hardware;
> +	master->transfer_one_message = octeon_spi_transfer_one_message;
> +	master->unprepare_transfer_hardware = octeon_spi_nop_transfer_hardware;
> +
> +	master->dev.of_node = pdev->dev.of_node;
> +	err = spi_register_master(master);
> +	if (err) {
> +		dev_err(&pdev->dev, "register master failed: %d\n", err);
> +		goto fail;
> +	}
> +
> +	dev_info(&pdev->dev, "Version " DRV_VERSION "\n");
> +
> +	return 0;
> +fail:
> +	spi_master_put(master);
> +	return err;
> +}
> +
> +static int __devexit octeon_spi_remove(struct platform_device *pdev)
> +{
> +	struct octeon_spi *p = platform_get_drvdata(pdev);
> +	struct spi_master *master = p->my_master;
> +
> +	spi_unregister_master(master);
> +
> +	/* Clear the CSENA* and put everything in a known state. */
> +	cvmx_write_csr(p->register_base + OCTEON_SPI_CFG, 0);
> +	spi_master_put(master);
> +	return 0;
> +}
> +
> +static struct of_device_id octeon_spi_match[] = {
> +	{
> +		.compatible = "cavium,octeon-3010-spi",
> +	},
> +	{},

Nitpick:
	{ .compatible = "cavium,octeon-3010-spi", },
	{},

No need for the extra lines when it is so short.

> +};
> +MODULE_DEVICE_TABLE(of, octeon_spi_match);
> +
> +static struct platform_driver octeon_spi_driver = {
> +	.driver = {
> +		.name		= "spi-octeon",
> +		.owner		= THIS_MODULE,
> +		.of_match_table = octeon_spi_match,
> +	},
> +	.probe		= octeon_spi_probe,
> +	.remove		= __exit_p(octeon_spi_remove),

__devexit_p

> +};
> +
> +module_platform_driver(octeon_spi_driver);
> +
> +MODULE_DESCRIPTION(DRV_DESCRIPTION);
> +MODULE_VERSION(DRV_VERSION);
> +MODULE_AUTHOR("David Daney");
> +MODULE_LICENSE("GPL");
> -- 
> 1.7.2.3
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.