Re: [PATCH] regulator: s2mps11: Added shutdown function to poweroff Odroid-XU3

[Krzysztof Kozlowski]

2015-06-24 19:32 GMT+09:00 Anand Moon <linux.amoon@xxxxxxxxx>:
> Hi Krzysztof,
>
> On 24 June 2015 at 13:27, Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx> wrote:
>> 2015-06-24 16:16 GMT+09:00 Anand Moon <linux.amoon@xxxxxxxxx>:
>>> Added .shutdown function to s2mps11 to help poweroff the board succefully.
>>
>> s/succefully/successfully/
>>
>>> The device drivers set the register to turn off the PMIC.
>>
>> Driver or drivers?
>>
>>>
>>> Signed-off-by: Anand Moon <linux.amoon@xxxxxxxxx>
>>> ---
>>> Changes fixes the poweroff
>>> root@odroidxu3:~# poweroff
>>>
>>> Broadcast message from root@odroidxu3
>>>         (/dev/ttySAC2) at 13:08 ...
>>>
>>> The system is going down for power off NOW!
>>> root@odroidxu3:~# wait-for-state stop/waiting
>>>  * Stopping rsync daemon rsync                                           [ OK ]
>>>  * Stopping RDP Session manager                                          [ OK ]
>>>  * Stopping NTP server ntpd                                              [ OK ]
>>>  * Asking all remaining processes to terminate...                        [ OK ]
>>>  * All processes ended within 1 seconds...                               [ OK ]
>>> nm-dispatcher.action: Caught signal 15, shutting down...
>>> ModemManager[2134]: <warn>  Could not acquire the 'org.freedesktop.ModemManager1' service name
>>>
>>> ModemManager[2134]: <info>  ModemManager is shut down
>>>
>>>  * Unmounting temporary filesystems...                                   [ OK ]
>>>  * Deactivating swap...                                                  [ OK ]
>>>  * Unmounting local filesystems...                                       [ OK ]
>>>  * Will now halt
>>> [  209.020280] reboot: Power down
>>> [  209.122039] Power down failed, please power off system manually.
>>> ---
>>>  drivers/regulator/s2mps11.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
>>> index ff82811..871f7b8 100644
>>> --- a/drivers/regulator/s2mps11.c
>>> +++ b/drivers/regulator/s2mps11.c
>>> @@ -1060,6 +1060,13 @@ out:
>>>         return ret;
>>>  }
>>>
>>> +static void s2mps11_pmic_shutdown(struct platform_device *pdev)
>>> +{
>>> +       struct sec_pmic_dev *iodev = dev_get_drvdata(pdev->dev.parent);
>>> +
>>> +       regmap_update_bits(iodev->regmap_pmic, S2MPS11_REG_CTRL1, 0xff, 0x00);
>>
>> This looks odd to me and interesting in the same time...
>> 1. Why clearing all of the fields from the register? Don't you want to
>> clear only one of it?
>
> I have followed what the hardkernel source code point at this point.
> I will look into which bit need to clear/set to power off successful.

Following other tree is not enough, even when it solves some one
problem. Drivers could be used on different boards, where hardkernel
patches were not tested. This driver is used on many boards so please
explain exactly what is the cause of problem, what have to be done and
how patch achieves this. Such explanation helps understanding impact
on other boards.

>
>> 2. What exactly you want to do here? What is expected behaviour?
>
> When you power off the board dose not power off cleanly.
> [  209.122039] Power down failed, please power off system manually.
>
> After this changes Its power off the board. Leaving the board with
> solid red led blowing.

You described observable issue, which is nice, but I am asking for
technical details. What exactly do you want to achieve here?
Technically. What is expected behaviour. In technical details, not
only observable. I am asking for these technical details not only
because they are important but also because I do not know them.

>
>> 3. How this relates to PWRHOLD coming from AP to the PMIC?
>
> No Idea right now about this right now. but will update you it I have
> some thing.

The PMIC receives two signals - PWREN and PWRHOLD. It seems strange
that PMIC must override PWRHOLD... maybe there is an issue in
mach-exynos code?

>
>> 4. Why this is needed only for this driver and only for this board (Odroid XU3)?
>
> Yes It could be generic not specific for the Odroid XU3 will correct
> in next patch.

Thanks,
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/