| Message ID | 1523895561-4073-1-git-send-email-fabrice.gasnier@st.com |
|---|---|
| Headers | show |
| Series | Add support for PWM input capture on STM32 | expand |
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? > }; > + > +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
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 ? +/** + * 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 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > >
On 04/17/2018 12:10 PM, Lee Jones wrote: > 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. Hi Lee, Ok, I'll update this in v5. BTW, I'll fix warning reported by Dan: > smatch warnings: drivers/mfd/stm32-timers.c:165 stm32_timers_dma_burst_read() warn: warn: dma_mapping_error() doesn't return an error code Thanks, Fabrice > 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 >>> > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html