From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752178AbeAWPxW (ORCPT ); Tue, 23 Jan 2018 10:53:22 -0500 Received: from mx08-00178001.pphosted.com ([91.207.212.93]:41138 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751258AbeAWPxT (ORCPT ); Tue, 23 Jan 2018 10:53:19 -0500 Subject: Re: [PATCH 4/8] mfd: stm32-timers: add support for dmas To: Lee Jones CC: , , , , , , , , , , , References: <1516106631-18722-1-git-send-email-fabrice.gasnier@st.com> <1516106631-18722-5-git-send-email-fabrice.gasnier@st.com> <20180123133234.xmep76cgdremnj47@dell> <20180123153035.2rvhspe2m5ej57op@dell> From: Fabrice Gasnier Message-ID: Date: Tue, 23 Jan 2018 16:52:49 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <20180123153035.2rvhspe2m5ej57op@dell> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.75.127.45] X-ClientProxiedBy: SFHDAG7NODE3.st.com (10.75.127.21) To SFHDAG5NODE3.st.com (10.75.127.15) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2018-01-23_04:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 01/23/2018 04:30 PM, Lee Jones wrote: > On Tue, 23 Jan 2018, Fabrice Gasnier wrote: > >> On 01/23/2018 02:32 PM, Lee Jones wrote: >>> On Tue, 16 Jan 2018, Fabrice Gasnier wrote: >>> >>>> STM32 Timers can support up to 7 dma requests: >>>> 4 channels, update, compare and trigger. >>>> Optionally request part, or all dmas from stm32-timers MFD core. >>>> Also, keep reference of device's bus address to allow child drivers to >>>> transfer data from/to device by using dma. >>>> >>>> Signed-off-by: Fabrice Gasnier >>>> --- >>>> drivers/mfd/stm32-timers.c | 37 ++++++++++++++++++++++++++++++++++++- >>>> include/linux/mfd/stm32-timers.h | 14 ++++++++++++++ >>>> 2 files changed, 50 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c >>>> index a6675a4..372b51e 100644 >>>> --- a/drivers/mfd/stm32-timers.c >>>> +++ b/drivers/mfd/stm32-timers.c >>>> @@ -29,6 +29,23 @@ static void stm32_timers_get_arr_size(struct stm32_timers *ddata) >>>> regmap_write(ddata->regmap, TIM_ARR, 0x0); >>>> } >>>> >>>> +static void stm32_timers_dma_probe(struct device *dev, >>>> + struct stm32_timers *ddata) >>>> +{ >>>> + int i; >>>> + char name[4]; >>>> + >>>> + for (i = STM32_TIMERS_DMA_CH1; i <= STM32_TIMERS_DMA_CH4; i++) { >>>> + snprintf(name, ARRAY_SIZE(name), "ch%1d", i + 1); >>>> + ddata->dmas[i] = dma_request_slave_channel(dev, name); >>> >>> And if any of them fail? >> >> Hi Lee, >> >> If some of these fails, reference will be NULL. It is checked in child >> driver (pwm for instance) at runtime. Support is being added as an >> option: pwm capture will simply be unavailable in this case (fail with >> error). > > Can you place a simple one-line comment in here please. Or else > someone will come along and add error handling for you. Hi Lee, Ok, I'll add a comment in v2. > >>>> + } >>>> + ddata->dmas[STM32_TIMERS_DMA_UP] = dma_request_slave_channel(dev, "up"); >>>> + ddata->dmas[STM32_TIMERS_DMA_TRIG] = >>>> + dma_request_slave_channel(dev, "trig"); >>>> + ddata->dmas[STM32_TIMERS_DMA_COM] = >>>> + dma_request_slave_channel(dev, "com"); >>> >>> Can you format these in the same why. This hurts my eyes. >> >> I use enum values and try to match as possible with reference manual for >> "up", "trig" & "com" names. >> Would have some suggestion to beautify this? > > I just mean, can you push the first dma_request_slave_channel() call > on to the next line, so each of the calls are formatted in the same > manner please? Ok, got it now. I'll do this in v2. > >>>> +} >>>> + >>>> static int stm32_timers_probe(struct platform_device *pdev) >>>> { >>>> struct device *dev = &pdev->dev; >>>> @@ -44,6 +61,7 @@ static int stm32_timers_probe(struct platform_device *pdev) >>>> mmio = devm_ioremap_resource(dev, res); >>>> if (IS_ERR(mmio)) >>>> return PTR_ERR(mmio); >>>> + ddata->phys_base = res->start; >>> >>> What do you use this for? >> >> This is used in in child driver (pwm) for capture data transfer by dma. > > Might be worth being clear about that. > > Perhaps pass in 'dma_base' (phys_base + offset) instead? > I guess you've had a look at [PATCH 5/8] pwm: stm32: add capture support. Are you talking about passing phys_base + TIM_DMAR ? If this is the case, I'd prefer to keep phys base only if you don't mind, and handle TIM_DMAR offset in pwm driver. This way, all dma slave config is kept locally at one place. Or do you mean something else ? Maybe I can add a comment here about this ? Something like: /* phys_base to be used by child driver, e.g. DMA burst mode */ Please advise, Thanks, Fabrice