From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752530AbbGOBDt (ORCPT ); Tue, 14 Jul 2015 21:03:49 -0400 Received: from mailout3.w1.samsung.com ([210.118.77.13]:48587 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751241AbbGOBDr (ORCPT ); Tue, 14 Jul 2015 21:03:47 -0400 X-AuditID: cbfec7f5-f794b6d000001495-cf-55a5b1704a09 Message-id: <55A5B16F.9000003@samsung.com> Date: Wed, 15 Jul 2015 10:03:43 +0900 From: Krzysztof Kozlowski User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-version: 1.0 To: Anand Moon , Sangbeom Kim , Marek Szyprowski , Liam Girdwood , Mark Brown Cc: linux-kernel@vger.kernel.org, linux-samsung-soc@vger.kernel.org Subject: Re: [PATCHv2] regulator: s2mps11: Added shutdown function to poweroff References: <1436888477-3841-1-git-send-email-linux.amoon@gmail.com> In-reply-to: <1436888477-3841-1-git-send-email-linux.amoon@gmail.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrBLMWRmVeSWpSXmKPExsVy+t/xq7oFG5eGGjzvV7SY+vAJm8XrF4YW 3650MFlc3jWHzWLG+X1MFus23mK3WHvkLrvFxRVfmBw4PHbOusvusWlVJ5tH35ZVjB6fN8kF sERx2aSk5mSWpRbp2yVwZVw7+46x4IJcxeZPU9kbGI+LdTFyckgImEj87DjEBmGLSVy4tx7I 5uIQEljKKNFzYxc7SEJI4CmjxIG/RV2MHBy8AloS86ZVgIRZBFQlztz7xQRiswkYS2xevgRs jqhAhMTbyyfB4rwCghI/Jt9jAZkpInCQUeL7rGnMIAlmAWeJ9km7GUFsYQE/ie9bN7FA7HKR +HjrG9heTgFXiffrJjKB7GUW0JO4f1ELolVeYvOat8wTGAVmIVkxC6FqFpKqBYzMqxhFU0uT C4qT0nON9IoTc4tL89L1kvNzNzFCwvvrDsalx6wOMQpwMCrx8N5YsjRUiDWxrLgy9xCjBAez kgjvpB6gEG9KYmVValF+fFFpTmrxIUZpDhYlcd6Zu96HCAmkJ5akZqemFqQWwWSZODilGhh3 vLKZ9+7GPa01GZ7/DxgmbH0+1eDx+dkdyltDjx3s3Wh6RyVblan3PtuLgwU7fs03mbrFIqLh 3VxLDR7d0oQld9SeyWd+NG5/zaW2zmfqv68CdmWHPZcHis9TCWU1nfWkfPKc61uecd6yK4tK bv674CpncEPOPNYvfvUZqUpJMnMPCR4rrDuoxFKckWioxVxUnAgArwHIRmsCAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 15.07.2015 00:41, Anand Moon wrote: > Added .shutdown function to s2mps11 to help poweroff the board successfully. Which board does not poweroff? The PMIC is used on multiple boards, did you observed this on all of them? > > s2mps11-pmic: S2MPS11_REG_CTRL1 reg value 16:00000000000000000000000000010000 > > The device driver clears the register to turn off the PMIC. This is not sufficient explanation for commit message. I already raised concerns that this does not look to me as a proper way of doing poweroff. Unfortunately you did not resolved these concerns. The main questions are unanswered: Why you have to do this and why "standard" way does not work? How can you properly fix some problem if you don't know the cause of problem? It is blind shooting which may hurt other boards. > > Signed-off-by: Anand Moon > > --- > Console log for improper shutdown. > root@odroidxu3:~# poweroff > ... > * 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. > > Console log for proper shutdown. > root@odroidxu3:~# poweroff > ... > * Unmounting temporary filesystems... [ OK ] > * Deactivating swap... [ OK ] > * Unmounting local filesystems... [ OK ] > * Will now halt > [ 476.283071] reboo > --- > drivers/regulator/s2mps11.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c > index 326ffb5..823180e 100644 > --- a/drivers/regulator/s2mps11.c > +++ b/drivers/regulator/s2mps11.c > @@ -1060,6 +1060,31 @@ out: > return ret; > } > > +static void s2mps11_pmic_shutdown(struct platform_device *pdev) > +{ > + struct sec_pmic_dev *iodev = dev_get_drvdata(pdev->dev.parent); > + unsigned int reg_val, ret; > + > + ret = regmap_read(iodev->regmap_pmic, S2MPS11_REG_CTRL1, ®_val); > + if (ret < 0) { regmap_read() returns an int which you assign to an unsigned int which then you compare against <0? This does not look good. > + dev_crit(&pdev->dev, "could not read S2MPS11_REG_CTRL1 value\n"); > + } else { > + /* > + * s2mps11-pmic: S2MPS11_REG_CTRL1 reg value > + * is 00000000000000000000000000010000 > + * clear the S2MPS11_REG_CTRL1 0x10 value to shutdown. > + */ > + if (reg_val & BIT(4)) { > + ret = regmap_update_bits(iodev->regmap_pmic, > + S2MPS11_REG_CTRL1, > + BIT(4), BIT(0)); I don't understand. You want to update BIT(4) but the value is BIT(0)? This will clear BIT(4) but is totally unreadable. > + if (ret) > + dev_crit(&pdev->dev, > + "could not write S2MPS11_REG_CTRL1 value\n"); > + } > + } The code is not readable, to many unnecessary indentations. > +} > + > static const struct platform_device_id s2mps11_pmic_id[] = { > { "s2mps11-pmic", S2MPS11X}, > { "s2mps13-pmic", S2MPS13X}, > @@ -1074,6 +1099,7 @@ static struct platform_driver s2mps11_pmic_driver = { > .name = "s2mps11-pmic", > }, > .probe = s2mps11_pmic_probe, > + .shutdown = s2mps11_pmic_shutdown, The purpose of shutdown function is not to shutdown the system but to prepare the system for shutdown. The patch is just wrong and you did not answered the major question - WHY you have to do this? Don't fix the problem blindly (or because some hardkernel tree for some of the boards use such patch). Best regards, Krzysztof > .id_table = s2mps11_pmic_id, > }; > >