<div dir="ltr">Hi Joel,<div><br></div><div>Sorry for the delay with the reply, my work computer stopped working, needed to attend that.</div><div><br></div><div>I split the patch into three different self-consistent (hopefully) patches, but somewhat different from what you've suggested.</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 22, 2016 at 6:59 PM, Joel Stanley <span dir="ltr"><<a href="mailto:joel@jms.id.au" target="_blank">joel@jms.id.au</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello Maxim,<br>
<span class=""><br>
On Tue, Sep 20, 2016 at 2:56 AM, <<a href="mailto:maxims@google.com">maxims@google.com</a>> wrote:<br>
> From: Maxim Sloyko <<a href="mailto:maxims@google.com">maxims@google.com</a>><br>
><br>
> The driver is limited: only single master mode is supported and only<br>
> byte-by-byte reads and writes are supported, no DMA or Pool Buffers.<br>
<br>
</span>Has this code been ported from the kernel driver? If so, mention it in<br>
your commit message and/or a comment in the code. A comment about what<br>
needed to be modified in order to make it integrate in the u-boot tree<br>
would be helpful. Also mention where you copied the code from; which<br>
tree or commit.<br></blockquote><div><br></div><div>I would not call that "copied". The APIs are very different at the moment, especially since in u-boot device tree is not fully utilized, so yes, I used kernel driver as a reference, but this is essentially a rewrite.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Be sure to incorporate any changes made to the upstream Linux driver<br>
as it goes through review.<br>
<span class=""><br>
> Also, pin function configuration is performed by the I2C driver, because<br>
> there is no pinctrl driver at the moment.<br>
<br>
</span>It looks like you've got several patches combined into one. To make it<br>
easier to review, try to have a separate patch for each feature you're<br>
adding.<br>
<span class=""><br>
> ---<br>
> arch/arm/dts/Makefile | 2 +<br>
> arch/arm/dts/aspeed-g5-evb.dts | 28 ++<br>
> arch/arm/dts/aspeed-g5.dtsi | 392 ++++++++++++++++++++++++++++<br>
<br>
</span>This would be one patch.<br>
<br>
I assume they came from the kernel tree? If so, mention this,<br>
including which tree/commit you got them from.<br>
<span class=""><br>
> arch/arm/include/asm/arch-<wbr>aspeed/ast_scu.h | 6 +<br>
> arch/arm/include/asm/arch-<wbr>aspeed/regs-scu.h | 74 +++---<br>
<br>
</span>This would be another.<br>
<span class=""><br>
> arch/arm/mach-aspeed/ast-scu.c | 31 ++-<br>
> board/aspeed/ast-g5/ast-g5.c | 2 +-<br>
<br>
</span>I believe these integrate the i2c driver with the arch and the board.<br>
They should be separate commits.<br>
<br>
> configs/ast_g5_defconfig | 4 +-<br>
<br>
Updating the defconfig should be it's own patch.<br></blockquote><div><br></div><div>This change was dropped.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> drivers/i2c/Kconfig | 6 +<br>
> drivers/i2c/Makefile | 1 +<br>
> drivers/i2c/ast_i2c.c | 306 ++++++++++++++++++++++<br>
> drivers/i2c/ast_i2c.h | 155 +++++++++++<br>
<br>
</span>Adding the i2c driver has nothing to do with the other functions, so<br>
that could be it's own patch.<br>
<br>
Cheers,<br>
<br>
Joel<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div><b>M</b>axim <b>S</b>loyko</div></div>
</div></div>