Re: [PATCH 0/4] mfd: RK8xx tidyup

From: Soeren Moch <smoch@web.de>
To: Lee Jones <lee.jones@linaro.org>, Robin Murphy <robin.murphy@arm.com>
Cc: linux-kernel@vger.kernel.org, heiko@sntech.de,
	linux.amoon@gmail.com, linux-rockchip@lists.infradead.org,
	Daniel Schultz <d.schultz@phytec.de>
Subject: Re: [PATCH 0/4] mfd: RK8xx tidyup
Date: Tue, 17 Dec 2019 00:30:08 +0100	[thread overview]
Message-ID: <3c78d334-9110-e88c-84d0-2f41c28a6317@web.de> (raw)
In-Reply-To: <20191216111219.GB2369@dell>

On 16.12.19 12:12, Lee Jones wrote:
> On Tue, 10 Dec 2019, Robin Murphy wrote:
>
>> Hi all,
>>
>> In trying to debug suspend issues on my RK3328 box, I was looking at
>> how the RK8xx driver handles the RK805 sleep pin, and frankly the whole
>> driver seemed untidy enough to warrant some cleanup and minor fixes
>> before going any further. I've based the series on top of Soeren's
>> "mfd: rk808: Always use poweroff when requested" patch[1].
>>
>> Note that I've only had time to build-test these patches so far, but I
>> wanted to share them early for the sake of discussion in response to
>> the other thread[2].
>>
>> Robin.
>>
>> [1] https://patchwork.kernel.org/patch/11279249/
>> [2] https://patchwork.kernel.org/cover/11276945/
>>
>> Robin Murphy (4):
>>   mfd: rk808: Set global instance unconditionally
>>   mfd: rk808: Always register syscore ops
>>   mfd: rk808: Reduce shutdown duplication
>>   mfd: rk808: Convert RK805 to syscore/PM ops
>>
>>  drivers/mfd/rk808.c       | 122 ++++++++++++++++----------------------
>>  include/linux/mfd/rk808.h |   2 -
>>  2 files changed, 50 insertions(+), 74 deletions(-)
> Not sure what's happening with these (competing?) patch-sets.  I'm not
> planning on getting involved until you guys have arrived at and agreed
> upon a single solution.
>
My understanding is like this:

The patch [1] fixes power-off to use the method requested in the
devicetree. This patch series on top of that cleans up the rk808
power-off code, which perfectly makes sense for me. I think these 2
patch series are not controversial as such.

And then we have the series [2], which is a mix of
- some clean-up
- converting the existing code to use syscon_shutdown for actual power-off

Robin, Heiko, and myself agree that the shutdown method introduced in
[2] is fundamentally wrong. All syscon_shutdown methods of all
registered syscons have to run before the actual shutdown, what is
triggered in pm_power_off. This is how it works now, and what is the
right way to do it. Patch series [2] breaks this promise since it does
the actual shutdown in the middle of the chain of syscon_shutdown handlers.

In the discussion about series [2] Anand repeatedly talks about restart
problems. But this rk808 mfd driver is not involved in restart, also
patch series [2] 
does not change that. So I cannot see what should be the point here.

There was an earlier attempt [3] to enhance this rk808 mfd driver for
restart. I'm not sure, however, what happened to this. For me it could
make sense to reimplement such restart functionality on top of this
"mfd: RK8xx tidyup" series.

Regards,
Soeren

[3] https://lore.kernel.org/patchwork/patch/934323/