On 07/29/2015 05:02 PM, Simon Glass wrote:
Hi Stephen,
On 29 July 2015 at 13:48, Stephen Warren <swar...@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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
