From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Thu, 26 Nov 2015 17:13:55 +0100 Subject: [U-Boot] [PATCH] pmic: Fix pfuze100 bit definitions In-Reply-To: <201511261639.18239.marex@denx.de> References: <1448493344-6336-1-git-send-email-marex@denx.de> <201511261408.03973.marex@denx.de> <565718AE.9070209@samsung.com> <201511261639.18239.marex@denx.de> Message-ID: <56572FC3.7060000@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 04:39 PM, Marek Vasut wrote: > On Thursday, November 26, 2015 at 03:35:26 PM, Przemyslaw Marczak wrote: >> Hello Marek, > > Hi, > >> 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? > > This is even worse then -- the patch adds code which uses the changed macros, > but doesn't fix the existing users. This should not happen again and it'd be > very nice if the author actually checked when digging in /include and changing > some macros there if this might affect someone. > Ok, but as you could check in this example, even recompile all boards with such kind of 'new patch' - will not tell you what is wrong, because it doesn't break the build... >> 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. > > I don't expect you to test anything in this case, other but possibly compile > testing the stuff, don't get me wrong. > >>> 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. > > I did that. And unfortunatelly, it turns out we have really no other option > now, than to fix the boards. Sigh ... > But isn't this also an important part of our job? I don't know why we discuss about this... Found bug? Can fix? Then send a patch and don't blame the people for a bugs - it's natural. Don't cry if got a comments - this is the Open Source project:) I hope you don't get me wrong. Have a nice weekend! - I'm starting it right now:) Best regards, -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com