[U-Boot] [PATCH] pmic: Fix pfuze100 bit definitions

[Marek Vasut]

On Thursday, November 26, 2015 at 05:13:55 PM, Przemyslaw Marczak wrote:
> Hello Marek,

Hi,

> 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 <Peng.Fan at freescale.com>
> >>>>> Date:   Fri Aug 7 16:43:45 2015 +0800
> >>>>> 
> >>>>>        power: regulator: add pfuze100 support
> >>>>> 
> >>>>> Signed-off-by: Marek Vasut <marex at denx.de>
> >>>>> Cc: Fabio Estevam <fabio.estevam at freescale.com>
> >>>>> Cc: Peng Fan <Peng.Fan at freescale.com>
> >>>>> Cc: Przemyslaw Marczak <p.marczak at samsung.com>
> >>>>> Cc: Tim Harvey <tharvey at gateworks.com>
> >>>>> Cc: Vagrant Cascadian <vagrant at aikidev.net>
> >>>>> ---
> >>>>> 
> >>>>>     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...

Yeah

> >> 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.

I already did that, but it seems to be about time to re-state that if you
dig in /include, then you should check thoroughly if your change cannot
have some sort of side-effects.

> Don't cry if got a comments - this is the Open Source project:)

Please, educate me more on this topic :)

> I hope you don't get me wrong.

DTTO.

> Have a nice weekend! - I'm starting it right now:)

You as well, cheers!