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

[Stephen Warren]

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?

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.

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

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