Re: [U-Boot] [PATCH 4/4] T210: Add support for T210-based P2571 board

[Tom Warren]

> -----Original Message-----
> From: Stephen Warren [mailto:swar...@wwwdotorg.org]
> Sent: Monday, June 15, 2015 10:59 AM
> To: Tom Warren
> Cc: u-boot@lists.denx.de; Stephen Warren; Tom Warren
> Subject: Re: [U-Boot] [PATCH 4/4] T210: Add support for T210-based P2571
> board
> 
> On 06/03/2015 02:35 PM, Tom Warren wrote:
> > Based on Venice2, may change as P2571 board is fully brought up.
> > Incorporates Stephen Warren's P2571 pinmux table.
> 
> > diff --git a/board/nvidia/p2571/max77620_init.c
> > b/board/nvidia/p2571/max77620_init.c
> 
> > +void tegra_i2c_ll_write_addr(uint addr, uint config) {
> > +   struct i2c_ctlr *reg = (struct i2c_ctlr *)TEGRA_DVC_BASE;
> > +
> > +   writel(addr, &reg->cmd_addr0);
> > +   writel(config, &reg->cnfg);
> > +}
> > +
> > +void tegra_i2c_ll_write_data(uint data, uint config) {
> > +   struct i2c_ctlr *reg = (struct i2c_ctlr *)TEGRA_DVC_BASE;
> > +
> > +   writel(data, &reg->cmd_data1);
> > +   writel(config, &reg->cnfg);
> > +}
> > +
> 
> We really should put that into a lilbrary function, probably along with the
> definition of things like I2C_SEND_2_BYTES (or make some more helper
> functions to hide that too).
Yep, we should make a PMIC driver for this part, but haven't yet. This is 
cut&paste from all the other Tegra boards.
I'll look into moving things around to make it cleaner, but that'll affect all 
boards and would be a separate effort.

> 
> > +void pmic_enable_cpu_vdd(void)
> > +{
> > +   debug("%s entry\n", __func__);
> > +
> > +        //from TegraShell init script: Set GPIO5 to drive CPU_REG_EN
> > +        //                              then 1.0V on ???
> 
> I don't think we should mention internal tool names in upstream source.
Good catch, thanks.  This has been addressed in a V2 patchset I did last week 
when I ran checkpatch.

> 
> > +        debug("%s: Setting GPIO5 to push-pull out, logic high to enable CPU
> regulator\n", __func__);
> > +        tegra_i2c_ll_write_addr(MAX77620_I2C_ADDR, 2);
> > +        tegra_i2c_ll_write_data(0x2040, I2C_SEND_2_BYTES);      //data/reg
> > +        udelay(10*1000);
> 
> Need spaces around "*".
> 
> I'm not sure what "data/reg" means?
0x2040 is 0x20 data, 0x40 register. Just a note to myself during bringup. 
Removed.
> 
> > +
> > +        tegra_i2c_ll_write_addr(MAX77620_I2C_ADDR, 2);
> > +        tegra_i2c_ll_write_data(0x093B, I2C_SEND_2_BYTES);      
> > //B3=1=logic
> high,B2=dontcare,B1=0=output,B0=1=push-pull
> > +        udelay(10 * 1000);
> 
> Can we wrap that put that comment on a separate line, since it's rather long. 
> I
> don't think U-Boot likes // comments.
> 
> Does this patch pass checkpatch?
V2 does, all C99 // comments removed, 80-char+ lines fixed, etc. I'll post it 
when I address your other concerns.

> 
> > diff --git a/board/nvidia/p2571/pinmux-config-p2571.h
> > b/board/nvidia/p2571/pinmux-config-p2571.h
> 
> I think I generated this a long time ago. I'd like to get this into 
> tegra-pinmux-
> scripts, and make sure we're pulling in data from the latest board 
> spreadsheet,
> before we commit this. I'll look into that today.
Good idea. Thanks.

--
Nvpublic

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot