Re: [RESEND PATCH v2 4/8] mfd: stm32-timers: add support for dmas

[Fabrice Gasnier]

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_*.
>