From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753996AbdGUJa5 (ORCPT ); Fri, 21 Jul 2017 05:30:57 -0400 Received: from mx07-00178001.pphosted.com ([62.209.51.94]:62310 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753547AbdGUJaz (ORCPT ); Fri, 21 Jul 2017 05:30:55 -0400 From: Pierre Yves MORDRET To: Vinod Koul CC: Rob Herring , Mark Rutland , Maxime Coquelin , Alexandre TORGUE , Russell King , Dan Williams , "M'boumba Cedric Madianga" , Fabrice GASNIER , Herbert Xu , Fabien DESSENNE , Amelie DELAUNAY , "dmaengine@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v2 2/4] dmaengine: Add STM32 MDMA driver Thread-Topic: [PATCH v2 2/4] dmaengine: Add STM32 MDMA driver Thread-Index: AQHS9lMC2tZHuLg31UegQW4kewwAdaJd31mAgAAaSIA= Date: Fri, 21 Jul 2017 09:30:00 +0000 Message-ID: References: <1499343941-6375-1-git-send-email-pierre-yves.mordret@st.com> <1499343941-6375-3-git-send-email-pierre-yves.mordret@st.com> <20170721075547.GO3053@localhost> In-Reply-To: <20170721075547.GO3053@localhost> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: user-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 x-ms-exchange-messagesentrepresentingtype: 1 x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.75.127.45] Content-Type: text/plain; charset="utf-8" Content-ID: <8D258A67913A3F44B7F973107788184C@st.com> MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-07-21_04:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id v6L9V3Bf030773 On 07/21/2017 09:55 AM, Vinod Koul wrote: > On Thu, Jul 06, 2017 at 02:25:39PM +0200, Pierre-Yves MORDRET wrote: > >> +config STM32_MDMA >> + bool "STMicroelectronics STM32 master dma support" >> + depends on ARCH_STM32 || COMPILE_TEST > ^^^ > why multiple spaces typo I guess > >> +static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32 tlen) >> +{ >> + enum dma_slave_buswidth max_width = DMA_SLAVE_BUSWIDTH_8_BYTES; >> + >> + while (((buf_len % max_width) || (tlen < max_width)) && >> + (max_width > DMA_SLAVE_BUSWIDTH_1_BYTE)) >> + max_width = max_width >> 1; > > ok, this is a bit hard to read... This code snippet has already been reworked and optimized. Would you mind to provide me a example with your expectation ? Thanks > >> +static int stm32_mdma_set_xfer_param(struct stm32_mdma_chan *chan, >> + enum dma_transfer_direction direction, >> + u32 *mdma_ccr, u32 *mdma_ctcr, >> + u32 *mdma_ctbr, u32 buf_len) >> +{ >> + struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan); >> + struct stm32_mdma_chan_config *chan_config = &chan->chan_config; >> + enum dma_slave_buswidth src_addr_width, dst_addr_width; >> + phys_addr_t src_addr, dst_addr; >> + int src_bus_width, dst_bus_width; >> + u32 src_maxburst, dst_maxburst, src_best_burst, dst_best_burst; >> + u32 ccr, ctcr, ctbr, tlen; >> + >> + src_addr_width = chan->dma_config.src_addr_width; >> + dst_addr_width = chan->dma_config.dst_addr_width; >> + src_maxburst = chan->dma_config.src_maxburst; >> + dst_maxburst = chan->dma_config.dst_maxburst; >> + src_addr = chan->dma_config.src_addr; >> + dst_addr = chan->dma_config.dst_addr; > > this doesn't seem right to me, only the periphral address would come from > slave_config, the memory address is passed as an arg to transfer.. > > ... > Correct. But these locals are managed in the case statement below. if direction is Mem2Dev only dst_addr(Peripheral) is considered. In the other way around with Dev2Mem direction only src_addr(Peripheral) is considered. However to disambiguate I can move src_addr & dst_addr affectation in the corresponding case statement if you'd like. >> +static int stm32_mdma_setup_xfer(struct stm32_mdma_chan *chan, >> + struct stm32_mdma_desc *desc, >> + struct scatterlist *sgl, u32 sg_len, >> + enum dma_transfer_direction direction) >> +{ >> + struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan); >> + struct dma_slave_config *dma_config = &chan->dma_config; >> + struct scatterlist *sg; >> + dma_addr_t src_addr, dst_addr; >> + u32 ccr, ctcr, ctbr; >> + int i, ret = 0; >> + >> + for_each_sg(sgl, sg, sg_len, i) { >> + if (sg_dma_len(sg) > STM32_MDMA_MAX_BLOCK_LEN) { >> + dev_err(chan2dev(chan), "Invalid block len\n"); >> + return -EINVAL; >> + } >> + >> + ret = stm32_mdma_set_xfer_param(chan, direction, &ccr, &ctcr, >> + &ctbr, sg_dma_len(sg)); >> + if (ret < 0) >> + return ret; >> + >> + if (direction == DMA_MEM_TO_DEV) { >> + src_addr = sg_dma_address(sg); >> + dst_addr = dma_config->dst_addr; > > and this seems correct, but then why are we doing it in > stm32_mdma_set_xfer_param() > See comment above. >> +static struct dma_async_tx_descriptor *stm32_mdma_prep_slave_sg( >> + struct dma_chan *c, struct scatterlist *sgl, >> + u32 sg_len, enum dma_transfer_direction direction, >> + unsigned long flags, void *context) > > right justfied these please, it makes a terrible read > Given the amount of parameters difficult to right align. Agree with this formatting ? static struct dma_async_tx_descriptor *stm32_mdma_prep_slave_sg(struct dma_chan *c, struct scatterlist *sgl, u32 sg_len, enum dma_transfer_direction direction, unsigned long flags, void *context) >> +{ >> + struct stm32_mdma_chan *chan = to_stm32_mdma_chan(c); >> + struct stm32_mdma_desc *desc; >> + int ret; >> + >> + desc = stm32_mdma_alloc_desc(chan, sg_len); >> + if (!desc) >> + return NULL; >> + >> + ret = stm32_mdma_setup_xfer(chan, desc, sgl, sg_len, direction); >> + if (ret < 0) >> + goto xfer_setup_err; >> + >> + desc->cyclic = false; >> + >> + return vchan_tx_prep(&chan->vchan, &desc->vdesc, flags); >> + >> +xfer_setup_err: >> + dma_pool_free(chan->desc_pool, &desc->hwdesc, desc->hwdesc_phys); >> + kfree(desc); >> + return NULL; >> +} >> + >> +static struct dma_async_tx_descriptor *stm32_mdma_prep_dma_cyclic( >> + struct dma_chan *c, dma_addr_t buf_addr, size_t buf_len, >> + size_t period_len, enum dma_transfer_direction direction, >> + unsigned long flags) > > here too and few other places ok. See comment above. > >> +static int stm32_mdma_probe(struct platform_device *pdev) >> +{ >> + struct stm32_mdma_chan *chan; >> + struct stm32_mdma_device *dmadev; >> + struct dma_device *dd; >> + struct device_node *of_node; >> + struct resource *res; >> + u32 nr_channels, nr_requests; >> + int i, count, ret; >> + >> + of_node = pdev->dev.of_node; >> + if (!of_node) >> + return -ENODEV; >> + >> + ret = of_property_read_u32(of_node, "dma-channels", &nr_channels); >> + if (ret) >> + nr_channels = STM32_MDMA_MAX_CHANNELS; >> + >> + ret = of_property_read_u32(of_node, "dma-requests", &nr_requests); >> + if (ret) >> + nr_requests = STM32_MDMA_MAX_REQUESTS; > > wouldn't it make sense to print error about these properties not being > present and continuing w/ defaults..? Those are optional parameters as stated by bindings. I can print out a warning or info if you'd like but not error. > > and can we have device_property_xxx instead of of_ variants? > of course ! >> +static int __init stm32_mdma_init(void) >> +{ >> + return platform_driver_probe(&stm32_mdma_driver, stm32_mdma_probe); >> +} >> + >> +subsys_initcall(stm32_mdma_init); > > Where are the MODULE_xx tags, license is mandatory. You should add author > too. > > Correct. I will change the Header at the same time. Thanks.