[U-Boot] [PATCH v2 07/12] exynos5: support tps65090 pmic

[Minkyu Kang]

Dear Simon,

On 23/05/14 02:27, Simon Glass wrote:
> Hi Minkyu,
> 
> On 21 May 2014 15:20, Minkyu Kang <mk7.kang at samsung.com> wrote:
>> On 22/05/14 03:58, Simon Glass wrote:
>>> Hi Minkyu,
>>>
>>> On 21 May 2014 00:05, Minkyu Kang <mk7.kang at samsung.com> wrote:
>>>> On 20/05/14 20:47, Simon Glass wrote:
>>>>> Hi Minkyu,
>>>>>
>>>>> On 15 May 2014 00:51, Minkyu Kang <mk7.kang at samsung.com> wrote:
>>>>>> On 03/04/14 08:24, Simon Glass wrote:
>>>>>>> From: Aaron Durbin <adurbin at chromium.org>
>>>>>>>
>>>>>>> The TSP65090 is a PMIC on some exynos5 boards. The init function is
>>>>>>> called for the TPS65090 pmic. If that device is not a part of the device
>>>>>>> tree (returns -ENODEV) then continue. Otherwise return a failure.
>>>>>>>
>>>>>>> Signed-off-by: Aaron Durbin <adurbin at chromium.org>
>>>>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>>>>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes in v2:
>>>>>>> - Move code to exynos5-dt.c
>>>>>>> - Fix comment style
>>>>>>> - Add #ifdef around tps65090 code
>>>>>>>
>>>>>>>  board/samsung/smdk5250/exynos5-dt.c | 13 +++++++++++++
>>>>>>>  1 file changed, 13 insertions(+)
>>>>>>>
>>>>>>> diff --git a/board/samsung/smdk5250/exynos5-dt.c b/board/samsung/smdk5250/exynos5-dt.c
>>>>>>> index 1a64b9b..2c1cf8a 100644
>>>>>>> --- a/board/samsung/smdk5250/exynos5-dt.c
>>>>>>> +++ b/board/samsung/smdk5250/exynos5-dt.c
>>>>>>> @@ -20,6 +20,7 @@
>>>>>>>  #include <asm/arch/sromc.h>
>>>>>>>  #include <power/pmic.h>
>>>>>>>  #include <power/max77686_pmic.h>
>>>>>>> +#include <power/tps65090_pmic.h>
>>>>>>>  #include <tmu.h>
>>>>>>>
>>>>>>>  DECLARE_GLOBAL_DATA_PTR;
>>>>>>> @@ -164,7 +165,19 @@ int exynos_power_init(void)
>>>>>>>
>>>>>>>  #ifdef CONFIG_POWER_MAX77686
>>>>>>>       ret = max77686_init();
>>>>>>> +     if (ret)
>>>>>>> +             return ret;
>>>>>>>  #endif
>>>>>>> +#ifdef CONFIG_POWER_TPS65090
>>>>>>> +     /*
>>>>>>> +      * The TPS65090 may not be in the device tree. If so, it is not
>>>>>>> +      * an error.
>>>>>>
>>>>>> Then, how we can initialise the tps65090?
>>>>>
>>>>> It is initialised if a suitable node is found in the device tree. If
>>>>> the device tree does not have it, then the hardware is assumed to not
>>>>> have this chip.
>>>>
>>>> then I think, it's an error.
>>>> Why you said, it is not an error?
>>>
>>> I may be misunderstanding your question, but I'll try to answer what I
>>> think you are asking.
>>>
>>> The device tree contains nodes for hardware in the machine that you
>>> want to initialise, and information about each one. Devices can be
>>> enabled or disabled by including or removing this information from the
>>> device tree (or marking a device disabled with a status = "disabled"
>>> property in the node).
>>>
>>> The tps65090 chip is not used in all exynos5-dt boards, but is used in
>>> some. For example smdk5250 does not have it, but snow does. So we use
>>> the device tree to specify the difference, including (on snow) things
>>> like the tps65090, the display bridge chip, etc.
>>>
>>
>> Hm, it doesn't make sense.
>> Then it(#define CONFIG_POWER_TPS65090) should go into each config files.
>> Not in exynos5-dt.h.
>> Please modify it and patch 6/12 also.
> 
> The way it works at present is that there is a single exynos5-dt.h
> file which is used by all exynos5 boards. To the extent possible we
> have avoided putting particular features in things like snow.h and
> smdk5250.h - they just include exynos5-dt.h without changes.
> 
> The idea is that we can have one U-Boot binary that runs on any
> exynos5 device, rather than lots of different options. This makes it
> much easier to test changes sine we only need to build it once. If
> every exynos5 board has different #defines then there are more
> combinations to build and test. This is similar to what the kernel
> does with arch/arm/mach-exynos/mach-exynos5-dt.c.
> 
> Using this approach the only differences between smdk5250, daisy,
> snow, spring, etc. are in the device tree. I'd really like to avoid
> changing this now. It is easy enough for people to add their own
> private variants of these locally if they want to, but for mainline I
> believe it is better to be more generic.

I totally understood what you assert.
But I can't agree with you.
Do you think if we collect all features at exynos5-dt.h then
it can be generic? even if some boards doesn't have the device?
I think no. It just wrong definition.
We should separate exynos5 generic features and board specific features.

+	ret = tps65090_init();
+	if (ret == 0 || ret == -ENODEV)
+		return 0;

At here.. whatever, the error is an error.
If you got NODEV then should return it, because that board does not have device.

Thanks,
Minkyu Kang.