Hi Simon,
On Wed, 2 May 2018 20:31:48 -0600, Simon Glass sjg@chromium.org wrote:
Hi Miquel,
On 2 May 2018 at 02:59, Miquel Raynal miquel.raynal@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@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