[U-Boot] [PATCH 1/2] ARM: tegra: Add e2220-1170 board

[Simon Glass]

Hi Stephen,

On 30 July 2015 at 12:13, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 07/29/2015 05:02 PM, Simon Glass wrote:
>>
>> Hi Stephen,
>>
>> On 29 July 2015 at 13:48, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>>
>>>
>>
>> Commit message?
>
>
> I guess I can add one, but there really much useful additional information I
> can give.
>
> I've also been "guilty" of pushing for commit messages for cases like this,
> but I wonder if we shouldn't just consider the commit subject as part of the
> overall description and hence allow empty commit descriptions when they
> won't add anything?

Then why apply the patch? There needs to be some basic motivation. I
think in this case it should mention what the board is at least.

>
> Anyway, since I'm likely to spin the patch for e2220-1170.h at least, I can
> add something.
>
>>> diff --git a/arch/arm/mach-tegra/tegra210/Kconfig
>>> b/arch/arm/mach-tegra/tegra210/Kconfig
>
>
>>> +config TARGET_E2220_1170
>>> +       bool "NVIDIA Tegra210 E2220-1170 base board"
>>> +       help
>>> +         E2220-1170 ERS
>>
>>
>> Er, description?
>
>
> The text after "bool" is the description. There's no help text sure, but
> there's not really any useful information I can give beyond the board model.
> It's obvious that this option selects support for a particular board since
> (a) it's in a board selection menu/choice stanza and (b) the message after
> "bool" says so. The board ID is all anyone should need in order to enable
> support for the board they have. I'm prohibited by NVIDIA from listing any
> alternate names the board has. I don't think it's useful to list details of
> the HW attached to the board; most boards have roughly the same devices, and
> I don't think Kconfig should be used as any kind of sales pitch/advertizing.

You could mention that it is a Tegra210 development board with MMC,
LCD, 2 USB 3 ports, etc. etc. I think that is useful and it only takes
a minute.

>
> Do you have an example of a useful help text could put here?
>
>>> diff --git a/board/nvidia/e2220-1170/e2220-1170.c
>>> b/board/nvidia/e2220-1170/e2220-1170.c
>
>
>>> +void pin_mux_mmc(void)
>>> +{
>>> +       struct udevice *dev;
>>> +       uchar val;
>>> +       int ret;
>>> +
>>> +       /* Turn on MAX77620 LDO2 to 3.3V for SD card power */
>>> +       ret = i2c_get_chip_for_busnum(0, 0x3c, 1, &dev);
>>> +       if (ret) {
>>> +               printf("%s: Cannot find MAX77620 I2C chip\n", __func__);
>>> +               return;
>>> +       }
>>> +       val = 0xf2;
>>> +       ret = dm_i2c_write(dev, 0x27, &val, 1);
>>> +       if (ret)
>>> +               printf("i2c_write 0 0x3c 0x27 failed: %d\n", ret);
>>
>>
>> Shouldn't this be in a PMIC driver?
>
>
> I think the overall amount of code contained in U-Boot ends up being a lot
> smaller this way; it simply and directly performs some necessary I2C writes,
> and V2 contains better comments describing the operation. This avoids all
> the overhead of creating a PMIC driver and instantiating it etc. This code
> is also consistent in style with many other Tegra boards. I'm rather
> inclined to leave this as-is unless there's some particular advantage you
> can state to creating a PMIC driver (I had thought of that original, but
> shied away from the large overhead/infrastructure involved with that.)

If this is the only write that happens to the PMIC then that is fine.
But when you add other things, won't we need to adjust more things?

>
>>> diff --git a/board/nvidia/e2220-1170/pinmux-config-e2220-1170.h
>>> b/board/nvidia/e2220-1170/pinmux-config-e2220-1170.h
>
>
>> Is this U-Boot code or is it auto-generated? If the latter, can you
>> please add instructions here?
>
>
> Can I add that as a followon patch that fixes this for all boards; there are
> many pre-existing cases where the same comment would apply.

You should get these patches in however you wish and follow-up
clean-ups seem fine to me.

Regards,
Simon