linux-kernel - Re: [PATCH v4 2/6] mfd: stm32-timers: add support for dmas

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <34e30463-6236-a8e4-fd1f-6217612375eb@st.com>
Date:   Tue, 17 Apr 2018 09:37:59 +0200
From:   Fabrice Gasnier <fabrice.gasnier@...com>
To:     Lee Jones <lee.jones@...aro.org>
CC:     <alexandre.torgue@...com>, <thierry.reding@...il.com>,
        <benjamin.gaignard@...aro.org>, <robh+dt@...nel.org>,
        <mark.rutland@....com>, <linux@...linux.org.uk>,
        <mcoquelin.stm32@...il.com>, <benjamin.gaignard@...com>,
        <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <linux-pwm@...r.kernel.org>
Subject: Re: [PATCH v4 2/6] mfd: stm32-timers: add support for dmas

On 04/17/2018 09:12 AM, Lee Jones wrote:
> On Mon, 16 Apr 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 add routine to implement burst reads using DMA from timer registers.
>> This is exported. So, it can be used by child drivers, PWM capture
>> for instance (but not limited to).
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@...com>
>> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@...aro.org>
>> ---
>> Changes in v4:
>> - Lee's comments: Add kerneldoc header, better format comments.
>> Changes in v3:
>> - Basically Lee's comments:
>> - rather create a struct stm32_timers_dma, and place a reference to it
>>   in existing ddata (instead of adding priv struct).
>> - rather use a struct device in exported routine prototype, and use
>>   standard helpers instead of ddata. Get rid of to_stm32_timers_priv().
>> - simplify error handling in probe (remove a goto)
>> - comment on devm_of_platform_*populate() usage.
>>
>> Changes in v2:
>> - Abstract DMA handling from child driver: move it to MFD core
>> - Add comments on optional dma support
>> ---
>>  drivers/mfd/stm32-timers.c       | 227 ++++++++++++++++++++++++++++++++++++++-
>>  include/linux/mfd/stm32-timers.h |  32 ++++++
>>  2 files changed, 257 insertions(+), 2 deletions(-)
> 
> [...]
> 
>> diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h
>> index 2aadab6..a04d7a1 100644
>> --- a/include/linux/mfd/stm32-timers.h
>> +++ b/include/linux/mfd/stm32-timers.h
>> @@ -8,6 +8,8 @@
>>  #define _LINUX_STM32_GPTIMER_H_
>>  
>>  #include <linux/clk.h>
>> +#include <linux/dmaengine.h>
>> +#include <linux/dma-mapping.h>
>>  #include <linux/regmap.h>
> 
> [...]
> 
>> +struct stm32_timers_dma;
>> +
>>  struct stm32_timers {
>>  	struct clk *clk;
>>  	struct regmap *regmap;
>>  	u32 max_arr;
>> +	struct stm32_timers_dma *dma; /* Only to be used by the parent */
> 
> I'm confused.  I thought the point of putting this comment in was so
> that you could place the definition of 'stm32_timers_dma' and remove
> the forward declaration?

Hi Lee,

Sorry, if I miss-understood the point then. So, do you wish I both:
- move the full struct definition in above header ?
- and keep this comment ?

+/**
+ * struct stm32_timers_dma - STM32 timer DMA handling.
+ * @completion:		end of DMA transfer completion
+ * @phys_base:		control registers physical base address
+ * @lock:		protect DMA access
+ * @chan:		DMA channel in use
+ * @chans:		DMA channels available for this timer instance
+ */
+struct stm32_timers_dma {
+	struct completion completion;
+	phys_addr_t phys_base;
+	struct mutex lock;
+	struct dma_chan *chan;
+	struct dma_chan *chans[STM32_TIMERS_MAX_DMAS];
+};

This will basically expose the struct to child drivers. But I'm ok if
you think this is acceptable.

I can send a V5 if you wish...

Please advise,
Best regards,
Fabrice

> 
>>  };
>> +
>> +int stm32_timers_dma_burst_read(struct device *dev, u32 *buf,
>> +				enum stm32_timers_dmas id, u32 reg,
>> +				unsigned int num_reg, unsigned int bursts,
>> +				unsigned long tmo_ms);
>>  #endif
>