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

[Miquel Raynal]

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?

> 
> Regards,
> Simon

Thanks,
Miquèl

-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com