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:
signature.asc
Description: Digital signature
