[U-Boot] [PATCH v3 03/25] tpm: disociate TPMv1.x specific and generic code

[Miquel Raynal]

Hi Tom,

On Mon, 14 May 2018 15:43:07 -0400, Tom Rini <trini at konsulko.com> wrote:

> On Mon, May 14, 2018 at 08:01:57PM +0200, Miquel Raynal wrote:
> > Hi Simon,
> > 
> > On Wed, 2 May 2018 20:31:48 -0600, Simon Glass <sjg at chromium.org> wrote:
> >   
> > > Hi Miquel,
> > > 
> > > On 2 May 2018 at 02:59, Miquel Raynal <miquel.raynal at bootlin.com> wrote:  
> > > > There are no changes in this commit unless:
> > > > 1/ a new organization of the code as follow.
> > > > 2/ some *very* basic checkpatch.pl corrections that polluated my reports
> > > >    like s/uint<x>_t/u<x>/, blank spaces and non-aligned parameters on
> > > >    parenthesis.
> > > >
> > > > * cmd/ directory:    
> > > >         > move existing code from cmd/tpm.c in cmd/tpm-common.c
> > > >         > move specific code in cmd/tpm-v1.c
> > > >         > create a specific header file with generic definitions for    
> > > >           commands only called cmd/tpm-user-utils.h
> > > >
> > > > * lib/ directory:    
> > > >         > move existing code from lib/tpm.c in lib/tpm-common.c
> > > >         > move specific code in lib/tpm-v1.c
> > > >         > create a specific header file with generic definitions for    
> > > >           the library itself called lib/tpm-utils.h
> > > >
> > > > * include/ directory:    
> > > >         > move existing code from include/tpm.h in include/tpm-common.h
> > > >         > move specific code in include/tpm-v1.h    
> > > >
> > > > Code designated as 'common' is compiled if TPM are used. Code designated
> > > > as 'specific' is compiled only if the right specification has been
> > > > selected.
> > > >
> > > > All files include tpm-common.h.
> > > > Files in cmd/ include tpm-user-utils.h.
> > > > Files in lib/ include tpm-utils.h.
> > > > Depending on the specification, files may include either (not both)
> > > > tpm-v1.h or tpm-v2.h.
> > > >
> > > > Signed-off-by: Miquel Raynal <miquel.raynal at bootlin.com>
> > > > ---
> > > >  cmd/Makefile                       |   3 +-
> > > >  cmd/tpm-common.c                   | 289 +++++++++++++++++++++++++++++++++++
> > > >  cmd/tpm-user-utils.h               |  25 +++
> > > >  cmd/{tpm.c => tpm-v1.c}            | 305 ++-----------------------------------
> > > >  cmd/tpm_test.c                     |   2 +-
> > > >  drivers/tpm/tpm-uclass.c           |   4 +-
> > > >  drivers/tpm/tpm_atmel_twi.c        |   2 +-
> > > >  drivers/tpm/tpm_tis_infineon.c     |   2 +-
> > > >  drivers/tpm/tpm_tis_lpc.c          |   2 +-
> > > >  drivers/tpm/tpm_tis_sandbox.c      |   2 +-
> > > >  drivers/tpm/tpm_tis_st33zp24_i2c.c |   2 +-
> > > >  drivers/tpm/tpm_tis_st33zp24_spi.c |   2 +-
> > > >  include/tpm-common.h               | 214 ++++++++++++++++++++++++++
> > > >  include/{tpm.h => tpm-v1.h}        | 274 ++++++---------------------------
> > > >  lib/Makefile                       |   3 +-
> > > >  lib/tpm-common.c                   | 189 +++++++++++++++++++++++
> > > >  lib/tpm-utils.h                    |  96 ++++++++++++
> > > >  lib/{tpm.c => tpm-v1.c}            | 248 +-----------------------------
> > > >  18 files changed, 886 insertions(+), 778 deletions(-)
> > > >  create mode 100644 cmd/tpm-common.c
> > > >  create mode 100644 cmd/tpm-user-utils.h
> > > >  rename cmd/{tpm.c => tpm-v1.c} (76%)
> > > >  create mode 100644 include/tpm-common.h
> > > >  rename include/{tpm.h => tpm-v1.h} (62%)
> > > >  create mode 100644 lib/tpm-common.c
> > > >  create mode 100644 lib/tpm-utils.h
> > > >  rename lib/{tpm.c => tpm-v1.c} (81%)
> > > >    
> > > 
> > > This is a bit hard to review as there is so much going on.
> > > 
> > > Can you do the patman/chcekpatch clean-up in a separate patch before
> > > this one? Then hopefully most of this becomes just a rename?  
> > 
> > I understand your point and made all the checkpatch.pl changes in
> > different commits previously to this one.
> > 
> > Unfortunately, as I split one file (<dir> being include/cmd/lib):
> > - <dir>/tpm.x
> > in two files:
> > - <dir>/tpm-common.x
> > - <dir>/tpm-v1.x
> > 
> > There will never be just a rename :/
> >   
> > > 
> > > Also do you have to do it all at once in one patch? It seem slike you
> > > could move lib/ around separately from cmd/ for example.
> > > 
> > > At present all I can give is a rubber stamp.  
> > 
> > I think it would add more changes doing so because of the includes
> > being renamed. Plus, I don't think the understanding would be enhanced
> > as the point of this commit is to clearly separate shared code and
> > specific code for TPMv1 only chips. From my point of view doing so in
> > several commits does not clarify the goal, nor it would simplify the
> > review :(
> > 
> > Otherwise, in the next version there will be nothing more than just code
> > moves in this commit.
> > 
> > Hope this new split of the changes will be enough?  
> 
> You're really sure you've only got just file renames, content moving and
> related header additions here?  Thanks!
> 

I tried my best, yes :)

Thanks,
Miquèl