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

[Przemyslaw Marczak <p.marczak@samsung.com>]

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

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