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

[Minkyu Kang]

Dear Simon Glass,

On 27/05/14 09:01, Simon Glass wrote:
> Hi Minkyu,
> 
> On 25 May 2014 21:15, Minkyu Kang <mk7.kang at samsung.com> wrote:
>> 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.
> 
> Yes my intent is that we have an exynos5 build which (given the right
> device tree) can support any board. That has been the intent of the
> device tree effort for a while, and similar to the kernel approach.
> 
> What is the objection of having the driver compiled in for a board
> that doesn't have the device? Is it the extra 1KB of code space for
> smdk5250 etc.?

Yes, it's a tiny part of binary.
But if we allow such things.. then it can be grew more and more. 

> 
> Anyway, it doesn't necessarily have to be exynos5-dt.h. If you really
> don't like that, we could create a new generic exynos5 board and I can
> target that instead. But I really want to avoid having to build
> multiple U-Boots for each board if possible.

I want to define CONFIG_POWER_TPS65090 at snow.h and other each board's config file that have a device.
But, I know what you want.
So I will merge this patchset.
But at future, I'll not allow any exception.
It was not right way I think.
I just want to keep clean our codes.
That's all.

Thanks,
Minkyu Kang.