linux-kernel - Re: [PATCH v3 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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 16 Apr 2018 15:47:53 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Fabrice Gasnier <fabrice.gasnier@...com>
Cc:     thierry.reding@...il.com, alexandre.torgue@...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 v3 2/6] mfd: stm32-timers: add support for dmas

On Mon, 16 Apr 2018, Fabrice Gasnier wrote:

> On 04/16/2018 02:22 PM, Lee Jones wrote:
> > On Fri, 30 Mar 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 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       | 219 ++++++++++++++++++++++++++++++++++++++-
> >>  include/linux/mfd/stm32-timers.h |  32 ++++++
> >>  2 files changed, 249 insertions(+), 2 deletions(-)

[...]

> >> diff --git a/include/linux/mfd/stm32-timers.h b/include/linux/mfd/stm32-timers.h
> >> index 2aadab6..585a4de 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>
> > 
> > [...]
> > 
> >> +enum stm32_timers_dmas {
> >> +	STM32_TIMERS_DMA_CH1,
> >> +	STM32_TIMERS_DMA_CH2,
> >> +	STM32_TIMERS_DMA_CH3,
> >> +	STM32_TIMERS_DMA_CH4,
> >> +	STM32_TIMERS_DMA_UP,
> >> +	STM32_TIMERS_DMA_TRIG,
> >> +	STM32_TIMERS_DMA_COM,
> >> +	STM32_TIMERS_MAX_DMAS,
> >> +};
> >> +
> >> +struct stm32_timers_dma;
> > 
> > Why don't you move the declaration into here?
> 
> To follow previous discussions we had in V1 and V2, this is to avoid
> sharing all the information with child drivers, e.g. passing physical
> address of parent MFD into child devices.
> 
> I should probably add a comment there ? Something like:
> 
> /* STM32 timers MFD parent internal struct to handle DMA transfers */
> struct stm32_timers_dma;
> 
> Do you agree with this ?
> 
> Thanks for reviewing,
> BR,
> Fabrice
> 
> > 
> > Then you don't need to forward declare.

Yes, I remember our previous conversation.

Perhaps you could always put a comment like:

> >>  struct stm32_timers {
> >>  	struct clk *clk;
> >>  	struct regmap *regmap;
> >>  	u32 max_arr;
> >> +	struct stm32_timers_dma *dma; /* Only to be used by the parent */
> >>  };

If this is not acceptable, then the current solution will do.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog