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....@freescale.com>
> >>>>> Date: Fri Aug 7 16:43:45 2015 +0800
> >>>>>
> >>>>> power: regulator: add pfuze100 support
> >>>>>
> >>>>> Signed-off-by: Marek Vasut <ma...@denx.de>
> >>>>> Cc: Fabio Estevam <fabio.este...@freescale.com>
> >>>>> Cc: Peng Fan <peng....@freescale.com>
> >>>>> Cc: Przemyslaw Marczak <p.marc...@samsung.com>
> >>>>> Cc: Tim Harvey <thar...@gateworks.com>
> >>>>> Cc: Vagrant Cascadian <vagr...@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!
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
