Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()

[Dmitry Osipenko]

30.09.2020 19:15, Thierry Reding пишет:
> On Wed, Sep 30, 2020 at 07:06:27PM +0300, Dmitry Osipenko wrote:
>> 30.09.2020 19:03, Thierry Reding пишет:
>>> On Wed, Sep 30, 2020 at 06:53:06PM +0300, Dmitry Osipenko wrote:
>>>> 30.09.2020 18:23, Thierry Reding пишет:
>>>>> On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote:
>>>>>> From: Dmitry Osipenko <digetx@xxxxxxxxx>
>>>>>>
>>>>>> Multiple Tegra drivers need to retrieve Memory Controller and hence there
>>>>>> is quite some duplication of the retrieval code among the drivers. Let's
>>>>>> add a new common helper for the retrieval of the MC.
>>>>>>
>>>>>> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
>>>>>> Signed-off-by: Nicolin Chen <nicoleotsuka@xxxxxxxxx>
>>>>>> ---
>>>>>>
>>>>>> Changelog
>>>>>> v2->v3:
>>>>>>  * Replaced with Dimtry's devm_tegra_get_memory_controller()
>>>>>> v1->v2:
>>>>>>  * N/A
>>>>>>
>>>>>>  drivers/memory/tegra/mc.c | 39 +++++++++++++++++++++++++++++++++++++++
>>>>>>  include/soc/tegra/mc.h    | 17 +++++++++++++++++
>>>>>>  2 files changed, 56 insertions(+)
>>>>>
>>>>> Let's not add this helper, please. If a device needs a reference to the
>>>>> memory controller, it should have a phandle to the memory controller in
>>>>> device tree so that it can be looked up explicitly.
>>>>>
>>>>> Adding this helper is officially sanctioning that it's okay not to have
>>>>> that reference and that's a bad idea.
>>>>
>>>> And please explain why it's a bad idea, I don't see anything bad here at
>>>> all.
>>>
>>> Well, you said yourself in a recent comment that we should avoid global
>>> variables. devm_tegra_get_memory_controller() is nothing but a glorified
>>> global variable.
>>
>> This is not a variable, but a common helper function which will remove
>> the duplicated code and will help to avoid common mistakes like a missed
>> put_device().
> 
> Yeah, you're right: this is actually much worse than a global variable.
> It's a helper function that needs 50+ lines in order to effectively
> access a global variable.
> 
> You could write this much simpler by doing something like this:
> 
> 	static struct tegra_mc *global_mc;
> 
> 	int tegra_mc_probe(...)
> 	{
> 		...
> 
> 		global_mc = mc;
> 
> 		...
> 	}
> 
> 	struct tegra_mc *tegra_get_memory_controller(void)
> 	{
> 		return global_mc;
> 	}
> 
> The result is *exactly* the same, except that this is actually more
> honest. Nicolin's patch *pretends* that it isn't using a global variable
> by wrapping a lot of complicated code around it.
> 
> But that doesn't change the fact that this accesses a singleton object
> without actually being able to tie it to the device in the first place.

I don't think that the MC driver will stay built-in forever, although
its modularization is complicated right now. Hence something shall keep
the reference to the MC device resources while they are in use and this
patch takes care of doing that.

Secondly, the Nicolin's patch doesn't pretend on anything, but rather
brings the already existing duplicated code to a single common place.