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

[Przemyslaw Marczak]

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

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