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

[Fabrice Gasnier]

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.

> 
> [...]
> 
>>>> +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 Regards,
Fabrice

>