From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Thu, 26 Nov 2015 15:35:26 +0100 Subject: [U-Boot] [PATCH] pmic: Fix pfuze100 bit definitions In-Reply-To: <201511261408.03973.marex@denx.de> References: <1448493344-6336-1-git-send-email-marex@denx.de> <5656F950.9090709@samsung.com> <201511261408.03973.marex@denx.de> Message-ID: <565718AE.9070209@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Marek, On 11/26/2015 02:08 PM, Marek Vasut wrote: > On Thursday, November 26, 2015 at 01:21:36 PM, Przemyslaw Marczak wrote: >> Hello Marek, > > Hi, > >> On 11/26/2015 12:15 AM, Marek Vasut wrote: >>> The following patch changed the PFUZE100 swbst register bit definitions >>> and broke PMIC configuration on multiple boards, at least on the novena >>> and gw_ventana. This patch fixes it. >> >> Ok we missed this in the review. But as I can see it broken only the two >> boards, you mentioned. >> >>> commit 8fa46350a4c7dca7710362f6c871098557b934ad >>> Author: Peng Fan >>> Date: Fri Aug 7 16:43:45 2015 +0800 >>> >>> power: regulator: add pfuze100 support >>> >>> Signed-off-by: Marek Vasut >>> Cc: Fabio Estevam >>> Cc: Peng Fan >>> Cc: Przemyslaw Marczak >>> Cc: Tim Harvey >>> Cc: Vagrant Cascadian >>> --- >>> >>> include/power/pfuze100_pmic.h | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/include/power/pfuze100_pmic.h >>> b/include/power/pfuze100_pmic.h index 41cb710..cc019a9 100644 >>> --- a/include/power/pfuze100_pmic.h >>> +++ b/include/power/pfuze100_pmic.h >>> @@ -215,10 +215,10 @@ enum { >>> >>> #define SWBST_VOL_MASK 0x3 >>> #define SWBST_MODE_MASK 0xC >>> #define SWBST_MODE_SHIFT 0x2 >>> >>> -#define SWBST_MODE_OFF 0 >>> -#define SWBST_MODE_PFM 1 >>> -#define SWBST_MODE_AUTO 2 >>> -#define SWBST_MODE_APS 3 >>> +#define SWBST_MODE_OFF (0 << 2) >>> +#define SWBST_MODE_PFM (1 << 2) >>> +#define SWBST_MODE_AUTO (2 << 2) >>> +#define SWBST_MODE_APS (3 << 2) >>> >>> /* >>> >>> * Regulator Mode Control >> >> The intentions are good, but this patch fixes one thing and breaks the >> another one, I would prefer avoid this. >> >> 'git grep -n SWBST_MODE' >> >> As I can see, you can fix the issue for multiple boards by update only >> two lines in those two boards, which you mentioned. >> >> So why you moving back those definitions, since they are now used in >> more places? >> >> The line suggested by Peng is good enough to call it 'fix' for your boards: >> >> (SWBST_MODE_AUTO << SWBST_MODE_SHIFT) > > OK, so instead of fixing the patch which introduced a bug, we're supposed to > be fixing the fallout from that. I cannot say I'm very happy with this sort > of handling of a bug and with the testing this particular change received. > You are right, the mentioned patch breaks your boards, and we missed this in the review as I mentioned - sorry for that. But for now, there is also other code based on those definitions, so you can not just revert only this particular change and ignore the rest - because it breaks the new code? Should we all work in this way? As a custodian I'm not able to test everything, especially when I don't have the hardware for it. Moreover I trust people who are working for this project and I can imagine that they test the code. > Besides, seeing how this patch already needed another patch to make it complete > and how it now needs more patches to fix the boards which it broke, I am really > disappointed. I can't understand what is the problem. You send new patch with two simple lines - it fixes your issue and doesn't break the existing PMIC driver. I think, this is what we need here. > > Best regards, > Marek Vasut > > Best regards, -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com