On 03/29/2018 04:31 PM, Lee Jones wrote: > On Thu, 29 Mar 2018, Fabrice Gasnier wrote: > >> On 03/29/2018 02:59 PM, Lee Jones wrote: >>> On Wed, 28 Mar 2018, Fabrice Gasnier wrote: >>> >>>> On 03/28/2018 05:22 PM, Lee Jones wrote: >>>>> On Wed, 14 Feb 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@xxxxxx> >>>>>> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxx> >>>>>> --- >>>>>> 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 | 215 ++++++++++++++++++++++++++++++++++++++- >>>>>> include/linux/mfd/stm32-timers.h | 27 +++++ >>>>>> 2 files changed, 238 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/drivers/mfd/stm32-timers.c b/drivers/mfd/stm32-timers.c >>>>>> index a6675a4..2cdad2c 100644 >>>>>> --- a/drivers/mfd/stm32-timers.c >>>>>> +++ b/drivers/mfd/stm32-timers.c >>> >>> [...] >>> >>>>>> + struct dma_chan *dmas[STM32_TIMERS_MAX_DMAS]; >>>>>> + struct stm32_timers ddata; >>>>> >>>>> This looks odd to me. Why can't you expand the current ddata >>>>> structure? Wouldn't it be better to create a stm32_timers_dma >>>>> structure to place all this information in (except *dev, that should >>>>> live in the ddata struct), then place a reference in the existing >>>>> stm32_timers struct? >>>> >>>> Maybe I miss-understand you here, from what we discussed in V1: >>>> https://lkml.org/lkml/2018/1/23/574 >>>>> ... "passing in the physical address of the parent MFD into >>>>> a child device doesn't quite sit right with me" >>>> I introduced this private struct in MFD parent, and completely hide it >>>> from the child. >>>> >>>> So, do you suggest to add struct definition here ? But make it part of >>>> struct stm32_timers *ddata? >>>> >>>> And only put declaration in include/linux/mfd/stm32-timers.h: >>>> + struct stm32_timers_dma; >>>> >>>> struct stm32_timers { >>>> struct clk *clk; >>>> struct regmap *regmap; >>>> u32 max_arr; >>>> + struct stm32_timers_dma; >>>> }; >>> >>> Yes, that's the basic idea. >>> >>>> I can probably spare the *dev then... use dev->parent in child driver. >>> >>> What would you use dev->parent for? >> >> Hi Lee, >> >> This is to follow your sugestion to use *dev instead of *ddata when >> calling stm32_timers_dma_burst_read(), the idea is to use it on child side: >> stm32_timers_dma_burst_read(dev->parent,...) from pwm driver. >> Then there is no need to keep *dev inside ddata struct. > > I'm wondering if it would be neater to us the child's *dev, then do > the ->parent deference in the parent MFD (with a comment to say what > you're doing of course). > There's already dev.parent dereference in child drivers, for same purpose: dev_get_drvdata(pdev->dev.parent). So, I guess same can be done here ? Thanks for you review, I'll update all this and send a v3. Best regards, Fabrice >>> [...] >>> >>>>>> +static int stm32_timers_remove(struct platform_device *pdev) >>>>>> +{ >>>>>> + struct stm32_timers *ddata = platform_get_drvdata(pdev); >>>>>> + struct stm32_timers_priv *priv = to_stm32_timers_priv(ddata); >>>>>> + >>>>>> + of_platform_depopulate(&pdev->dev); >>>>> >>>>> Why can't you continue using devm_*? >>>> >>>> I can use devm_of_platform_depopulate() here if you prefer, and keep >>>> devm_of_platform_populate() in probe. >>> >>> The point of devm_* is that you don't have to call depopulate. >>> >>> It happens automatically once this driver is unbound. >> >> Ok, so to clarify, keeping devm_ here may be a bit racy: >> of_platform_depopulate will happen after dma has been released (there is >> no devm_ variant to release dma). >> Only way to prevent race condition here, is to enforce >> of_platform_depopulate() is called before dma release (e.g. in reverse >> order compared to probe). >> >> Do you wish I add a comment about it ? > > Best thing to do then is keep the non-devm variant and provide a > comment as to why is it not possible to use devm_*. > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
