From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756930AbbJVIbU (ORCPT ); Thu, 22 Oct 2015 04:31:20 -0400 Received: from mail-bn1bon0115.outbound.protection.outlook.com ([157.56.111.115]:33718 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1756214AbbJVIab convert rfc822-to-8bit (ORCPT ); Thu, 22 Oct 2015 04:30:31 -0400 X-Greylist: delayed 2018 seconds by postgrey-1.27 at vger.kernel.org; Thu, 22 Oct 2015 04:30:30 EDT From: Yao Yuan To: Vinod Koul CC: "shawn.guo@linaro.org" , "dan.j.williams@intel.com" , "dmaengine@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "devicetree@vger.kernel.org" Subject: RE: [PATCH 1/2] dma: Add Freescale qDMA engine driver support Thread-Topic: [PATCH 1/2] dma: Add Freescale qDMA engine driver support Thread-Index: AQHQ7Fc3DfoyI7v9i06sSiqzRlZ6Wp5dHfgAgBo/yIA= Date: Thu, 22 Oct 2015 07:56:47 +0000 Message-ID: References: <1441950833-27684-1-git-send-email-yao.yuan@freescale.com> <20151005143713.GC13501@vkoul-mobl.iind.intel.com> In-Reply-To: <20151005143713.GC13501@vkoul-mobl.iind.intel.com> Accept-Language: en-US, zh-CN Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=yao.yuan@freescale.com; x-originating-ip: [199.59.230.102] x-microsoft-exchange-diagnostics: 1;BLUPR03MB136;5:Xiu4Uz8ZUqGNeSIw+2AQUf1gPvvPogEvbidkZU7WyfcrHRTciZwARtuN6I9qK1m+3DENtiDqRSBib9Pk1WT/Uf/FyFvM5DAlUt+VF3sz4MU1e3RcOO/islx3dZFfa8uFPOY+TnmRq0gnUXt7tDVpCQ==;24:iL1J+kq1x9w2hiiYWGAzKb1lTNPU00dKw48z6JaL/X09DYeL9pS6+e0s4r2sKTKHcRuZMq5EDxMUVla09Tq6VQ8WH1qUeCf3U3dnS73WptM=;20:cFI2rO9zubBg268pHp8+0Aim9qC3CvNYZ/+oU7Qd/6PGa9rn8OO3uNWjqidjSUYfxSvPsaoYkDLcokDM4bU8bg== x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BLUPR03MB136; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(101931422205132); x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(601004)(2401047)(8121501046)(520078)(5005006)(3002001)(102215026);SRVR:BLUPR03MB136;BCL:0;PCL:0;RULEID:;SRVR:BLUPR03MB136; x-forefront-prvs: 0737B96801 x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(6009001)(377454003)(199003)(189002)(13464003)(24454002)(76176999)(97736004)(87936001)(81156007)(19580405001)(66066001)(10400500002)(64706001)(11100500001)(86362001)(99286002)(85806002)(101416001)(92566002)(5007970100001)(110136002)(189998001)(76576001)(2950100001)(54356999)(102836002)(5001960100002)(19580395003)(46102003)(50986999)(105586002)(5004730100002)(106356001)(74316001)(5003600100002)(5008740100001)(106116001)(2900100001)(33656002)(40100003)(5002640100001)(122556002);DIR:OUT;SFP:1102;SCL:1;SRVR:BLUPR03MB136;H:BLUPR03MB134.namprd03.prod.outlook.com;FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; spamdiagnosticoutput: 1:23 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-OriginatorOrg: freescale.com X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Oct 2015 07:56:48.0269 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 710a03f5-10f6-4d38-9ff4-a80b81da590d X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLUPR03MB136 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Vinod, Thanks for your review, please see my comments inline. Best Regards, Yuan Yao > -----Original Message----- > From: Vinod Koul [mailto:vinod.koul@intel.com] > Sent: Monday, October 05, 2015 10:37 PM > To: Yuan Yao-B46683 > Cc: shawn.guo@linaro.org; dan.j.williams@intel.com; > dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; devicetree@vger.kernel.org > Subject: Re: [PATCH 1/2] dma: Add Freescale qDMA engine driver support > > On Fri, Sep 11, 2015 at 01:53:52PM +0800, Yuan Yao wrote: > > > +Examples: > > + > > + qdma: qdma@8390000 { > > + compatible = "fsl,ls-qdma"; > > + reg = <0x0 0x8380000 0x0 0x20000>; > > + interrupts = , > > + ; > > + interrupt-names = "qdma-tx", "qdma-err"; > > + big-endian; > > + channels = <1>; > > + }; > > Binding should be a separate patch [Yuan Yao] Ok, Thanks. > > > +FREESCALE qDMA DRIVER > > +M: Yuan Yao > > +L: linux-arm-kernel@lists.infradead.org > > not dmaengine ML ? [Yuan Yao] Ok, Thanks. > > > > +config FSL_QDMA > > + tristate "Freescale qDMA engine support" > > + select DMA_ENGINE > > + select DMA_VIRTUAL_CHANNELS > > No depends on arch, can it work on x86? [Yuan Yao] Ok, Thanks. > > > +static int fsl_qdma_alloc_chan_resources(struct dma_chan *chan) { > > + struct fsl_qdma_chan *fsl_chan = to_fsl_qdma_chan(chan); > > + > > + fsl_chan->desc = NULL; > > + return 0; > > +} > > why do you need this it seems to do nothing [Yuan Yao] I will remove it. > > > +static struct fsl_qdma_desc *fsl_qdma_alloc_desc(struct fsl_qdma_chan > > +*fsl_chan) { > > + struct fsl_qdma_desc *fsl_desc; > > + > > + fsl_desc = kzalloc(sizeof(*fsl_desc), GFP_NOWAIT); > > + > > empty line here is not required > > > + if (!fsl_desc) > > + return NULL; > > + > > + fsl_desc->qchan = fsl_chan; > > + > > + return fsl_desc; > > why not return fsl_desc->qchan ; > [Yuan Yao] I still need some data in fsl_desc. So I have to return fsl_desc here. > > > + dma_cap_set(DMA_PRIVATE, fsl_qdma->dma_dev.cap_mask); > > + dma_cap_set(DMA_SLAVE, fsl_qdma->dma_dev.cap_mask); > > + dma_cap_set(DMA_MEMCPY, fsl_qdma->dma_dev.cap_mask); > > + > > + fsl_qdma->dma_dev.dev = &pdev->dev; > > + fsl_qdma->dma_dev.device_alloc_chan_resources > > + = fsl_qdma_alloc_chan_resources; > > + fsl_qdma->dma_dev.device_free_chan_resources > > + = fsl_qdma_free_chan_resources; > > + fsl_qdma->dma_dev.device_tx_status = fsl_qdma_tx_status; > > + fsl_qdma->dma_dev.device_prep_dma_memcpy = > fsl_qdma_prep_memcpy; > > + fsl_qdma->dma_dev.device_issue_pending = > fsl_qdma_issue_pending; > > You claim DMA_SLAVE but no prep_ for that? > [Yuan Yao] It's a mistake. I will remove it.\ > > + > > +static int __init fsl_qdma_init(void) { > > + return platform_driver_register(&fsl_qdma_driver); > > +} > > +subsys_initcall(fsl_qdma_init); > why subsys_init? > [Yuan Yao] For a preventive, some driver base on DMA, QDMA have to initialize earlier than them. Even now there is no kernel driver base on QDMA, But I still think the subsys_init is better. > -- > ~Vinod From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yao Yuan Subject: RE: [PATCH 1/2] dma: Add Freescale qDMA engine driver support Date: Thu, 22 Oct 2015 07:56:47 +0000 Message-ID: References: <1441950833-27684-1-git-send-email-yao.yuan@freescale.com> <20151005143713.GC13501@vkoul-mobl.iind.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20151005143713.GC13501@vkoul-mobl.iind.intel.com> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Vinod Koul Cc: "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "dmaengine@vger.kernel.org" , "dan.j.williams@intel.com" , "shawn.guo@linaro.org" , "linux-arm-kernel@lists.infradead.org" List-Id: devicetree@vger.kernel.org Hi Vinod, Thanks for your review, please see my comments inline. Best Regards, Yuan Yao > -----Original Message----- > From: Vinod Koul [mailto:vinod.koul@intel.com] > Sent: Monday, October 05, 2015 10:37 PM > To: Yuan Yao-B46683 > Cc: shawn.guo@linaro.org; dan.j.williams@intel.com; > dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; devicetree@vger.kernel.org > Subject: Re: [PATCH 1/2] dma: Add Freescale qDMA engine driver support > > On Fri, Sep 11, 2015 at 01:53:52PM +0800, Yuan Yao wrote: > > > +Examples: > > + > > + qdma: qdma@8390000 { > > + compatible = "fsl,ls-qdma"; > > + reg = <0x0 0x8380000 0x0 0x20000>; > > + interrupts = , > > + ; > > + interrupt-names = "qdma-tx", "qdma-err"; > > + big-endian; > > + channels = <1>; > > + }; > > Binding should be a separate patch [Yuan Yao] Ok, Thanks. > > > +FREESCALE qDMA DRIVER > > +M: Yuan Yao > > +L: linux-arm-kernel@lists.infradead.org > > not dmaengine ML ? [Yuan Yao] Ok, Thanks. > > > > +config FSL_QDMA > > + tristate "Freescale qDMA engine support" > > + select DMA_ENGINE > > + select DMA_VIRTUAL_CHANNELS > > No depends on arch, can it work on x86? [Yuan Yao] Ok, Thanks. > > > +static int fsl_qdma_alloc_chan_resources(struct dma_chan *chan) { > > + struct fsl_qdma_chan *fsl_chan = to_fsl_qdma_chan(chan); > > + > > + fsl_chan->desc = NULL; > > + return 0; > > +} > > why do you need this it seems to do nothing [Yuan Yao] I will remove it. > > > +static struct fsl_qdma_desc *fsl_qdma_alloc_desc(struct fsl_qdma_chan > > +*fsl_chan) { > > + struct fsl_qdma_desc *fsl_desc; > > + > > + fsl_desc = kzalloc(sizeof(*fsl_desc), GFP_NOWAIT); > > + > > empty line here is not required > > > + if (!fsl_desc) > > + return NULL; > > + > > + fsl_desc->qchan = fsl_chan; > > + > > + return fsl_desc; > > why not return fsl_desc->qchan ; > [Yuan Yao] I still need some data in fsl_desc. So I have to return fsl_desc here. > > > + dma_cap_set(DMA_PRIVATE, fsl_qdma->dma_dev.cap_mask); > > + dma_cap_set(DMA_SLAVE, fsl_qdma->dma_dev.cap_mask); > > + dma_cap_set(DMA_MEMCPY, fsl_qdma->dma_dev.cap_mask); > > + > > + fsl_qdma->dma_dev.dev = &pdev->dev; > > + fsl_qdma->dma_dev.device_alloc_chan_resources > > + = fsl_qdma_alloc_chan_resources; > > + fsl_qdma->dma_dev.device_free_chan_resources > > + = fsl_qdma_free_chan_resources; > > + fsl_qdma->dma_dev.device_tx_status = fsl_qdma_tx_status; > > + fsl_qdma->dma_dev.device_prep_dma_memcpy = > fsl_qdma_prep_memcpy; > > + fsl_qdma->dma_dev.device_issue_pending = > fsl_qdma_issue_pending; > > You claim DMA_SLAVE but no prep_ for that? > [Yuan Yao] It's a mistake. I will remove it.\ > > + > > +static int __init fsl_qdma_init(void) { > > + return platform_driver_register(&fsl_qdma_driver); > > +} > > +subsys_initcall(fsl_qdma_init); > why subsys_init? > [Yuan Yao] For a preventive, some driver base on DMA, QDMA have to initialize earlier than them. Even now there is no kernel driver base on QDMA, But I still think the subsys_init is better. > -- > ~Vinod From mboxrd@z Thu Jan 1 00:00:00 1970 From: yao.yuan@freescale.com (Yao Yuan) Date: Thu, 22 Oct 2015 07:56:47 +0000 Subject: [PATCH 1/2] dma: Add Freescale qDMA engine driver support In-Reply-To: <20151005143713.GC13501@vkoul-mobl.iind.intel.com> References: <1441950833-27684-1-git-send-email-yao.yuan@freescale.com> <20151005143713.GC13501@vkoul-mobl.iind.intel.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Vinod, Thanks for your review, please see my comments inline. Best Regards, Yuan Yao > -----Original Message----- > From: Vinod Koul [mailto:vinod.koul at intel.com] > Sent: Monday, October 05, 2015 10:37 PM > To: Yuan Yao-B46683 > Cc: shawn.guo at linaro.org; dan.j.williams at intel.com; > dmaengine at vger.kernel.org; linux-kernel at vger.kernel.org; linux-arm- > kernel at lists.infradead.org; devicetree at vger.kernel.org > Subject: Re: [PATCH 1/2] dma: Add Freescale qDMA engine driver support > > On Fri, Sep 11, 2015 at 01:53:52PM +0800, Yuan Yao wrote: > > > +Examples: > > + > > + qdma: qdma at 8390000 { > > + compatible = "fsl,ls-qdma"; > > + reg = <0x0 0x8380000 0x0 0x20000>; > > + interrupts = , > > + ; > > + interrupt-names = "qdma-tx", "qdma-err"; > > + big-endian; > > + channels = <1>; > > + }; > > Binding should be a separate patch [Yuan Yao] Ok, Thanks. > > > +FREESCALE qDMA DRIVER > > +M: Yuan Yao > > +L: linux-arm-kernel at lists.infradead.org > > not dmaengine ML ? [Yuan Yao] Ok, Thanks. > > > > +config FSL_QDMA > > + tristate "Freescale qDMA engine support" > > + select DMA_ENGINE > > + select DMA_VIRTUAL_CHANNELS > > No depends on arch, can it work on x86? [Yuan Yao] Ok, Thanks. > > > +static int fsl_qdma_alloc_chan_resources(struct dma_chan *chan) { > > + struct fsl_qdma_chan *fsl_chan = to_fsl_qdma_chan(chan); > > + > > + fsl_chan->desc = NULL; > > + return 0; > > +} > > why do you need this it seems to do nothing [Yuan Yao] I will remove it. > > > +static struct fsl_qdma_desc *fsl_qdma_alloc_desc(struct fsl_qdma_chan > > +*fsl_chan) { > > + struct fsl_qdma_desc *fsl_desc; > > + > > + fsl_desc = kzalloc(sizeof(*fsl_desc), GFP_NOWAIT); > > + > > empty line here is not required > > > + if (!fsl_desc) > > + return NULL; > > + > > + fsl_desc->qchan = fsl_chan; > > + > > + return fsl_desc; > > why not return fsl_desc->qchan ; > [Yuan Yao] I still need some data in fsl_desc. So I have to return fsl_desc here. > > > + dma_cap_set(DMA_PRIVATE, fsl_qdma->dma_dev.cap_mask); > > + dma_cap_set(DMA_SLAVE, fsl_qdma->dma_dev.cap_mask); > > + dma_cap_set(DMA_MEMCPY, fsl_qdma->dma_dev.cap_mask); > > + > > + fsl_qdma->dma_dev.dev = &pdev->dev; > > + fsl_qdma->dma_dev.device_alloc_chan_resources > > + = fsl_qdma_alloc_chan_resources; > > + fsl_qdma->dma_dev.device_free_chan_resources > > + = fsl_qdma_free_chan_resources; > > + fsl_qdma->dma_dev.device_tx_status = fsl_qdma_tx_status; > > + fsl_qdma->dma_dev.device_prep_dma_memcpy = > fsl_qdma_prep_memcpy; > > + fsl_qdma->dma_dev.device_issue_pending = > fsl_qdma_issue_pending; > > You claim DMA_SLAVE but no prep_ for that? > [Yuan Yao] It's a mistake. I will remove it.\ > > + > > +static int __init fsl_qdma_init(void) { > > + return platform_driver_register(&fsl_qdma_driver); > > +} > > +subsys_initcall(fsl_qdma_init); > why subsys_init? > [Yuan Yao] For a preventive, some driver base on DMA, QDMA have to initialize earlier than them. Even now there is no kernel driver base on QDMA, But I still think the subsys_init is better. > -- > ~Vinod