Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 17 Apr 2018 11:10:09 +0100
From:   Lee Jones <lee.jones@linaro.org>
To:     Fabrice Gasnier <fabrice.gasnier@st.com>
Cc:     alexandre.torgue@st.com, thierry.reding@gmail.com,
        benjamin.gaignard@linaro.org, robh+dt@kernel.org,
        mark.rutland@arm.com, linux@armlinux.org.uk,
        mcoquelin.stm32@gmail.com, benjamin.gaignard@st.com,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, linux-pwm@vger.kernel.org
Subject: Re: [PATCH v4 2/6] mfd: stm32-timers: add support for dmas
Message-ID: <20180417101009.do42adq24ltgw4lt@dell>
References: <1523895561-4073-1-git-send-email-fabrice.gasnier@st.com>
 <1523895561-4073-3-git-send-email-fabrice.gasnier@st.com>
 <20180417071250.yhgl7c7apn7w53xf@dell>
 <34e30463-6236-a8e4-fd1f-6217612375eb@st.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <34e30463-6236-a8e4-fd1f-6217612375eb@st.com>
User-Agent: NeoMutt/20170609 (1.8.3)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 17 Apr 2018, Fabrice Gasnier wrote:

> 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@st.com>
> >> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@linaro.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 ?

That was what I thought we agreed.

However, I left the final decision to you.  If you do not think this
is a reasonable i.e. the comment alone will not be enough to prevent
people from abusing the API, then leave it as it is.

Bear in mind that I think this introduces a build dependency on the
MFD driver for *each and every* other source file which includes this
header.  If you choose the current solution, you will need to handle
that accordingly.

> +/**
> + * 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
> > 

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