Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750943AbbGBIUx (ORCPT ); Thu, 2 Jul 2015 04:20:53 -0400 Received: from mailgw01.mediatek.com ([218.249.47.110]:37219 "EHLO mailgw01.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1751189AbbGBIUj (ORCPT ); Thu, 2 Jul 2015 04:20:39 -0400 X-Listener-Flag: 11101 Message-ID: <1435825232.7819.19.camel@mhfsdcap03> Subject: Re: [PATCH v2 3/4] spi: mediatek: Add spi bus for Mediatek MT8173 From: leilk liu To: Alexey Klimov CC: Mark Brown , Mark Rutland , , Sascha Hauer , "Linux Kernel Mailing List" , , , "Matthias Brugger" , Eddie Huang , Date: Thu, 2 Jul 2015 16:20:32 +0800 In-Reply-To: References: <1435583070-9600-1-git-send-email-leilk.liu@mediatek.com> <1435583070-9600-4-git-send-email-leilk.liu@mediatek.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit MIME-Version: 1.0 X-MTK: N Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9686 Lines: 299 Hi Alexey, > > +config SPI_MT65XX > > + tristate "MediaTek SPI controller" > > + depends on ARCH_MEDIATEK || COMPILE_TEST > > + help > > + This selects the MediaTek(R) SPI bus driver. > > + If you want to use MediaTek(R) SPI interface, > > + say Y or M here.If you are not sure, say N. > > + SPI drivers for Mediatek mt65XX series ARM SoCs. > > Commit subject and code here and there tells us it's compatible with > mt81xx series. What do you think, does it make any sense to extend > help description here? > The help description will be extended. > > > + > > config SPI_OC_TINY > > tristate "OpenCores tiny SPI" > > depends on GPIOLIB > > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile > > index d8cbf65..ab332ef 100644 > > --- a/drivers/spi/Makefile > > +++ b/drivers/spi/Makefile > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > > +#include > > > +#include > > +#include > > Could you please help me? I can't find any gpio-related things here. > Maybe i miss something. > Maybe is it for future? > The gpio-related include files are not need now, I will delete extra inclde files and sort others. > > > +#include > > +#include > > +static int mtk_spi_transfer_one(struct spi_master *master, > > + struct spi_device *spi, > > + struct spi_transfer *xfer) > > +{ > > + int cmd = 0, ret = 0; > > Maybe initialization of 'cmd' is not needed. > Yes. > > + unsigned int tx_sgl_len = 0, rx_sgl_len = 0; > > + struct scatterlist *tx_sgl = NULL, *rx_sgl = NULL; > > + struct mtk_spi_ddata *mdata = spi_master_get_devdata(master); > > + > > + /* Here is mt8173 HW issue: RX must enable TX, then TX transfer > > + * dummy data; TX don't need to enable RX. so enable TX dma for > > + * RX to workaround. > > + */ > > +static int mtk_spi_probe(struct platform_device *pdev) > > +{ > > + struct spi_master *master; > > + struct mtk_spi_ddata *mdata; > > + const struct of_device_id *of_id; > > + struct resource *res; > > + int ret; > > + > > + master = spi_alloc_master(&pdev->dev, sizeof(struct mtk_spi_ddata)); > > + if (!master) { > > + dev_err(&pdev->dev, "failed to alloc spi master\n"); > > + return -ENOMEM; > > + } > > + > > + pm_runtime_set_active(&pdev->dev); > > + pm_runtime_enable(&pdev->dev); > > + > > + master->auto_runtime_pm = true; > > + master->dev.of_node = pdev->dev.of_node; > > + master->bus_num = pdev->id; > > + master->num_chipselect = 1; > > + master->mode_bits = SPI_CPOL | SPI_CPHA; > > + > > + mdata = spi_master_get_devdata(master); > > + memset(mdata, 0, sizeof(struct mtk_spi_ddata)); > > Could you please check if you really need to fill in mdata by zeroes? > I checked spi_alloc_master() and for me it looks like it calls > kzalloc() for master + mdata. Yes, memset() is not really need. > > > + mdata->master = master; > > + mdata->dev = &pdev->dev; > > + > > + master->set_cs = mtk_spi_set_cs; > > + master->prepare_message = mtk_spi_prepare_message; > > + master->transfer_one = mtk_spi_transfer_one; > > + master->can_dma = mtk_spi_can_dma; > > + > > + of_id = of_match_node(mtk_spi_of_match, pdev->dev.of_node); > > + if (!of_id) { > > + dev_err(&pdev->dev, "failed to probe of_node\n"); > > + ret = -EINVAL; > > + goto err; > > + } > > + > > + mdata->platform_compat = (unsigned long)of_id->data; > > + > > + if (mdata->platform_compat & COMPAT_MT8173) { > > + ret = of_property_read_u32(pdev->dev.of_node, "pad-select", > > + &mdata->pad_sel); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to read pad select: %d\n", > > + ret); > > + goto err; > > + } > > + > > + if (mdata->pad_sel > MT8173_MAX_PAD_SEL) { > > + dev_err(&pdev->dev, "wrong pad-select: %u\n", > > + mdata->pad_sel); > > + ret = -EINVAL; > > + goto err; > > + } > > + } > > + > > + platform_set_drvdata(pdev, master); > > + init_completion(&mdata->done); > > + > > + mdata->clk = devm_clk_get(&pdev->dev, "main"); > > + if (IS_ERR(mdata->clk)) { > > + ret = PTR_ERR(mdata->clk); > > + dev_err(&pdev->dev, "failed to get clock: %d\n", ret); > > + goto err; > > + } > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + if (!res) { > > + ret = -ENODEV; > > + dev_err(&pdev->dev, "failed to determine base address\n"); > > + goto err; > > + } > > + > > + mdata->base = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(mdata->base)) { > > + ret = PTR_ERR(mdata->base); > > + goto err; > > + } > > + > > + ret = platform_get_irq(pdev, 0); > > + if (ret < 0) { > > + dev_err(&pdev->dev, "failed to get irq (%d)\n", ret); > > + goto err; > > + } > > + > > + mdata->irq = ret; > > + > > + if (!pdev->dev.dma_mask) > > + pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; > > + > > + ret = clk_prepare_enable(mdata->clk); > > + if (ret < 0) { > > + dev_err(&pdev->dev, "failed to enable clock (%d)\n", ret); > > + goto err; > > + } > > + > > + ret = devm_request_irq(&pdev->dev, mdata->irq, mtk_spi_interrupt, > > + IRQF_TRIGGER_NONE, dev_name(&pdev->dev), mdata); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to register irq (%d)\n", ret); > > + goto err_disable_clk; > > + } > > + > > + ret = devm_spi_register_master(&pdev->dev, master); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to register master (%d)\n", ret); > > +err_disable_clk: > > + clk_disable_unprepare(mdata->clk); > > +err: > > + spi_master_put(master); > > + } > > + > > + return ret; > > +} > > + > > +static int mtk_spi_remove(struct platform_device *pdev) > > +{ > > + struct spi_master *master = platform_get_drvdata(pdev); > > + struct mtk_spi_ddata *mdata = spi_master_get_devdata(master); > > + > > + pm_runtime_disable(&pdev->dev); > > + > > + mtk_spi_reset(mdata); > > + clk_disable_unprepare(mdata->clk); > > + spi_master_put(master); > > + > > + return 0; > > +} > > + > > +#ifdef CONFIG_PM_SLEEP > > +static int mtk_spi_suspend(struct device *dev) > > +{ > > + int ret = 0; > > Maybe initialization of ret here is not needed. > > > + struct spi_master *master = dev_get_drvdata(dev); > > + struct mtk_spi_ddata *mdata = spi_master_get_devdata(master); > > + > > + ret = spi_master_suspend(master); > > + if (ret) > > + return ret; > > + > > + if (!pm_runtime_suspended(dev)) > > + clk_disable_unprepare(mdata->clk); > > + > > + return ret; > > +} > > + > > +static int mtk_spi_resume(struct device *dev) > > +{ > > + int ret = 0; > > > Maybe initialization of ret here is not needed. You have two return paths: one > doesn't use ret as return value and second re-initializes it. > Yes, it will be fixed on next version. > > + struct spi_master *master = dev_get_drvdata(dev); > > + struct mtk_spi_ddata *mdata = spi_master_get_devdata(master); > > + > > + if (!pm_runtime_suspended(dev)) { > > + ret = clk_prepare_enable(mdata->clk); > > + if (ret < 0) > > + return ret; > > + } > > + > > + return spi_master_resume(master); > > +} > > +#endif /* CONFIG_PM_SLEEP */ > > + > > +#ifdef CONFIG_PM > > +static int mtk_spi_runtime_suspend(struct device *dev) > > +{ > > + struct spi_master *master = dev_get_drvdata(dev); > > + struct mtk_spi_ddata *mdata = spi_master_get_devdata(master); > > + > > + clk_disable_unprepare(mdata->clk); > > + > > + return 0; > > +} > > + > > -- > > 1.8.1.1.dirty > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > Thanks and best regards, > Klimov Alexey -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/