From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Tue, 23 Sep 2014 12:14:20 +0200
Subject: [U-Boot] [PATCH V3] ARM: mx6: Add support for Kosagi Novena
In-Reply-To: <542096CB.8020209@mail.bg>
References: <1411307086-12105-1-git-send-email-marex@denx.de>
	<542096CB.8020209@mail.bg>
Message-ID: <201409231214.20846.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Monday, September 22, 2014 at 11:38:19 PM, Nikolay Dimitrov wrote:
> Hi Marek,
> 
> I checked the I2C stuff, didn't saw any major issues. Here are a couple
> of comments for possible improvements:
> 
> On 09/21/2014 04:44 PM, Marek Vasut wrote:
> > +/*
> > + * I2C
> > + */
> > +/* I2C1, RAM */
> > +struct i2c_pads_info i2c_pad_info0 = {
> 
> I can suggest to reword the comment as: "I2C1: DDR3 SO-DIMM RAM" or
> something similar, to avoid confusion with I2C RAM.
> 
> > +/* I2C2 Camera, MIPI */
> > +static struct i2c_pads_info i2c_pad_info1 = {
> 
> I didn't found any MIPI camera on the schematic, so I think the comment
> is misleading. The connected devices on I2C2 are PMIC, HDMI DDC, FPGA,
> and only the PMIC could be useful for booting. So I can suggest to
> reword the comment like: "I2C2: PMIC".
> 
> > +/* I2C3, J15 - RGB connector */
> > +static struct i2c_pads_info i2c_pad_info2 = {
> 
> I didn't found such connect J15 on my schematic (novena_pvt2.pdf).
> There's a connector JP10L, which is for the LCD panel. Again the comment
> could be reworded as: "I2C3: Utility EEPROM, LCD panel"
> 
> > +static void novena_spl_setup_iomux_i2c(void)
> > +{
> > +	setup_i2c(0, CONFIG_SYS_I2C_SPEED, 0x7f, &i2c_pad_info0);
> > +	setup_i2c(1, CONFIG_SYS_I2C_SPEED, 0x7f, &i2c_pad_info1);
> > +	setup_i2c(2, CONFIG_SYS_I2C_SPEED, 0x7f, &i2c_pad_info2);
> > +}
> 
> Is the I2C1 bus used anywhere? I found only usage of I2C2 for PMIC and
> I2C3 for Utility EEPROM reading.

OK, tweaked the comments.

Best regards,
Marek Vasut