lkml.org 
[lkml]   [2012]   [Oct]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] spi: tegra: add spi driver for SLINK controller
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().
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2012-10-22 15:01    [W:0.086 / U:0.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site