From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752456AbbH1R72 (ORCPT <rfc822;w@1wt.eu>);
	Fri, 28 Aug 2015 13:59:28 -0400
Received: from mail-wi0-f169.google.com ([209.85.212.169]:33542 "EHLO
	mail-wi0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751725AbbH1R70 (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 28 Aug 2015 13:59:26 -0400
Date: Fri, 28 Aug 2015 18:59:22 +0100
From: Peter Griffin <peter.griffin@linaro.org>
To: Paul Bolle <pebolle@tiscali.nl>
Cc: lee.jones@linaro.org, devicetree@vger.kernel.org,
        dmaengine@vger.kernel.org, ludovic.barre@st.com,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        srinivas.kandagatla@gmail.com, maxime.coquelin@st.com,
        patrice.chotard@st.com, vinod.koul@intel.com, dan.j.williams@intel.com
Subject: Re: [PATCH 3/7] dmaengine: st_fdma: Add STMicroelectronics FDMA
 engine driver support
Message-ID: <20150828175922.GA2853@griffinp-ThinkPad-X1-Carbon-2nd>
References: <1436371888-27863-1-git-send-email-peter.griffin@linaro.org>
 <1436371888-27863-4-git-send-email-peter.griffin@linaro.org>
 <1436429872.20619.77.camel@tiscali.nl>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1436429872.20619.77.camel@tiscali.nl>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Paul,

Thanks for reviewing.

On Thu, 09 Jul 2015, Paul Bolle wrote:

> On wo, 2015-07-08 at 17:11 +0100, Peter Griffin wrote:
> > --- a/drivers/dma/Kconfig
> > +++ b/drivers/dma/Kconfig
>  
> > +config ST_FDMA
> > +	bool "ST FDMA dmaengine support"
> > +	depends on ARCH_STI
> > +	select DMA_ENGINE
> > +	select FW_LOADER
> > +	select FW_LOADER_USER_HELPER_FALLBACK
> > +	select LIBELF_32
> > +	select DMA_VIRTUAL_CHANNELS
> > +	help
> > +	  Enable support for ST FDMA controller.
> > +	  It supports 16 independent DMA channels, accepts up to 32 DMA requests
> > +
> > +	  Say Y here if you have such a chipset.
> > +	  If unsure, say N.
> 
> > --- a/drivers/dma/Makefile
> > +++ b/drivers/dma/Makefile
> > 
> > +obj-$(CONFIG_ST_FDMA) += st_fdma.o
> 
> ST_FDMA is a bool symbol, so st_fdma.o can only be built-in.

Yes good spot, that is a mistake it should be tristate. Will fix in v2.

> 
> > --- /dev/null
> > +++ b/drivers/dma/st_fdma.c
> 
> > +#include <linux/module.h>
> 
> Needed?
> 
> > +void *st_fdma_seg_to_mem(struct st_fdma_dev *fdev, u64 da, int len)
> 
> static?

Fixed in v2.

> 
> > +{
> > +	[...]
> > +}
> 
> > +static int
> > +st_fdma_elf_load_segments(struct st_fdma_dev *fdev, const struct 
> > firmware *fw)
> > +{
> > +	[...]
> > +		dst = st_fdma_seg_to_mem(fdev, da, memsz);
> > +		[...]
> > +}
> 
> > +static const struct of_device_id st_fdma_match[] = {
> > +	{ .compatible = "st,fdma_mpe31", .data = &fdma_mpe31 },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, st_fdma_match);
> 
> MODULE_DEVICE_TABLE() is preprocessed away for built-in only code.
> 
> > +static int __exit st_fdma_remove(struct platform_device *pdev)
> > +{
> > +	struct st_fdma_dev *fdev = platform_get_drvdata(pdev);
> > +
> > +	wait_for_completion(&fdev->fw_ack);
> > +
> > +	st_fdma_clk_disable(fdev);
> > +
> > +	return 0;
> > +}
> 
> Since this driver is built-in only this means st_fdma_remove() can never
> be used, right?

Will be capable of being a module in v2.

> 
> > +static struct platform_driver st_fdma_platform_driver = {
> > +	.driver = {
> > +		.name = "st-fdma",
> > +		.of_match_table = st_fdma_match,
> > +		.pm = ST_FDMA_PM,
> > +	},
> > +	.probe = st_fdma_probe,
> > +	.remove = st_fdma_remove,
> > +};
> > +module_platform_driver(st_fdma_platform_driver);
> 
> So can .remove be dropped?
> 
> Since v4.2-rc1 there's builtin_platform_driver() for built-in only code.

Thanks for the pointer, I wasn't aware of that function.

> 
> > +bool st_fdma_filter_fn(struct dma_chan *chan, void *param)
> > +{
> > +	[...]
> > +}
> > +EXPORT_SYMBOL(st_fdma_filter_fn);
> 
> This series adds no users of this export. I suppose they will be added
> in another series. Is that correct?

No the export is not required. Will fix in v2.

> 
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("STMicroelectronics FDMA engine driver");
> > +MODULE_AUTHOR("Ludovic.barre <Ludovic.barre@st.com>");
> 
> These macros will, basically, be preprocessed away for built-in only
> code.

The driver will be capable of being a module in v2.

regards,

Peter.