Re: [PATCH] spi: tegra: add spi driver for SLINK controller

[Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2696 bytes --]

On Thu, Oct 18, 2012 at 04:17:13PM +0530, Laxman Dewangan wrote:

> +		udelay(1);
> +		wmb();
> +	}
> +	tspi->dma_control_reg = val;
> +	val |= SLINK_DMA_EN;
> +	tegra_slink_writel(tspi, val, SLINK_DMA_CTL);
> +	return 0;
> +}
> +static int tegra_slink_init_dma_param(struct tegra_slink_data *tspi,
> +			bool dma_to_memory)
> +{

Blank line between functions.

> +	tegra_slink_clk_disable(tspi);
> +	pm_runtime_put_sync(tspi->dev);
> +	return 0;

Throughout the code you're using pm_runtime_put_sync() - why does this
need to be _sync()?  Can't we just use a regular put()?  If we can't we
should document why.

> +static int tegra_slink_prepare_transfer(struct spi_master *master)
> +{
> +	struct tegra_slink_data *tspi = spi_master_get_devdata(master);
> +
> +	pm_runtime_get_sync(tspi->dev);
> +	return 0;

Should really check the error code of the pm_runtime call here.

> +static struct tegra_spi_platform_data *tegra_slink_parese_dt(
> +		struct platform_device *pdev)
> +{

There doens't seem to be any binding documentation; binding
documentation is mandatory.

> +	prop = of_get_property(pdev->dev.of_node, "spi_max_frequency", NULL);
> +	if (prop)
> +		pdata->spi_max_frequency = be32_to_cpup(prop);
> +	else
> +		pdata->spi_max_frequency = 25000000; /* 25MHz */

Why is the default not being picked in the general non-OF case?

> +const struct tegra_slink_chip_data tegra30_spi_cdata = {
> +	.cs_hold_time = true,
> +};
> +
> +#ifdef CONFIG_OF
> +const struct tegra_slink_chip_data tegra20_spi_cdata = {
> +	.cs_hold_time = false,
> +};
> +
> +static struct of_device_id tegra_slink_of_match[] __devinitconst = {
> +	{ .compatible = "nvidia,tegra20-slink", .data = &tegra20_spi_cdata, },
> +	{ .compatible = "nvidia,tegra30-slink", .data = &tegra30_spi_cdata, },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, tegra_slink_of_match);
> +#endif

The ifdefs here look misplaced?

> +	spi_irq = platform_get_irq(pdev, 0);
> +	if (unlikely(spi_irq < 0)) {

Putting optimisation hints like unlikely() here is pointless.

> +	ret = devm_request_threaded_irq(&pdev->dev, tspi->irq,
> +			tegra_slink_isr, tegra_slink_isr_thread, IRQF_ONESHOT,
> +			dev_name(&pdev->dev), tspi);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to register ISR for IRQ %d\n",
> +					tspi->irq);
> +		goto exit_free_master;
> +	}

Any use of devm_ IRQ functions is suspicous... why is this safe against
an interrupt firing after the SPI device has started to deallocate?

> +#ifdef CONFIG_PM_SLEEP
> +static int tegra_slink_suspend(struct device *dev)
> +{
> +	struct spi_master *master = dev_get_drvdata(dev);
> +
> +	spi_master_suspend(master);
> +	return 0;

Should use the return code of spi_master_suspend().

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]