Re: [PATCH 04/12] cmd: tlv_eeprom: convert functions used by command to api functions
Hi Josua,
On 03.05.22 21:09, Josua Mayer wrote:
\o/
Am 03.05.22 um 13:54 schrieb Stefan Roese:
Hi Josua,
On 03.05.22 09:17, Josua Mayer wrote:
Am 03.05.22 um 09:16 schrieb Stefan Roese:
On 02.05.22 16:18, Josua Mayer wrote:
- prog_eeprom: write_tlvinfo_tlv_eeprom
- update_crc: tlvinfo_update_crc
- is_valid_tlv: is_valid_tlvinfo_entry
- is_checksum_valid: tlvinfo_check_crc
So while creating a new API it makes sense to prepend the function
names identical IMHO to not "pollute" the namespace. Something like
- tlv_is_valid_entry
- tlv_check_crc
...
Just examples, you get the idea.
Yes. The hard part in this particular implementation is that the
naming is not consistent.
The most sense I could make is that prefix tlvinfo indicates all tlv
data, i.e. working with the whole structure, while tlvinfo_tlv
indicates working with one data entry. Further write, read and is_
are currently prefixed in the header, but for previously static
functions in the C file it was put in the middle ...
I found it quite difficult to prepare for splitting off a library in
a way that preserves history, i.e. diffs should still be readable for
spotting mistakes.
Yes, a decent history would be welcome. But still, when going global
here with a new API this should be consistant.
My view more like - patches 1-10 are not really new API, in that I am
only changing what is necessary to allow splitting the code.
I was considering to at the very end do a mass-rename and come up
with better naming, something like
tlv_{set,get}_{blob,string,mac}
tlv_find_entry
tlv_{read,write}_eeprom
But this is pending a refactoring and extension of the tlv parsing
code in board/solidrun/common/tlv_data.*, to figure out what is
required or useful.
So your plan is to this:
a) Get this patchset included
b) Use it in board specific code, e.g. solidrun
c) Do the mass-rename
Is this correct?
This is close. I would say
1) get the split merged
2) rebase board-specific code
2a) figure out what kinds of set/get/add/remove functions are useful
3) redesign api
- there are inconsistencies with the order of function arguments
- read/write functions imo should use the tlv header to determine
size, not a function argument
- at least one of the tlv data types can appear multiple times,
existing code does not support this
4) submit any renames and extensions to the tlv library
5) submit board-specific use of tlv eeprom data
If yes, why is it better to do the renaming at the end?
It is very difficult to work on a patch-set that touches the same code
before and after moving it to a different file, which I found myself
doing a lot while prototyping this.
So if now I went ahead and figured out proper names and purposes for all
functions that I think should be the tlv api, then I have to divide that
into those parts that are renames or refactoring of existing
functionality, and those parts that are strictly new - putting the
former before splitting off the library, and the latter to after.
I am not sure I can manage this level of complexity.
To cut it short, if you plan to rename the API to some consitant naming
in the end, then I am okay with moving forward without a renaming at
this early stage.
Thanks,
Stefan
- Josua Mayer
Thanks,
Stefan
Thanks,
Stefan
Signed-off-by: Josua Mayer
---
cmd/tlv_eeprom.c | 56
+++
include/tlv_eeprom.h | 57
2 files changed, 76 insertions(+), 37 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c
index 00c5b5f840..1b4f2537f6 100644
--- a/cmd/tlv_eeprom.c
+++ b/cmd/tlv_eeprom.c
@@ -28,13 +28,9 @@ DECLARE_GLOBAL_DATA_PTR;
#define MAX_TLV_DEVICES 2
/* File scope function prototypes */
-static bool is_checksum_valid(u8 *eeprom);
static int read_eeprom(int devnum, u8 *eeprom);
static void show_eeprom(int devnum, u8 *eeprom);
static void decode_tlv(struct tlvinfo_tlv *tlv);
-static void update_crc(u8 *eeprom);
-static int prog_eeprom(int devnum, u8 *eeprom);
-static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int
*eeprom_index);
static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code);
static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval);
static int set_mac(char *buf, const char *string);
@@ -58,18 +54,6 @@ static inline bool is_digit(char c)
return (c >= '0' && c <= '9');
}
-/**
- * is_valid_tlv
- *
- * Perform basic sanity checks on a TLV field. The TLV is pointed to
- * by the parameter provided.
- * 1. The type code is not reserved (0x00 or 0xFF)
- */
-static inline bool is_valid_tlv(struct tlvinfo_tlv *tlv)
-{
- return((tlv->type != 0x00) && (tlv->type != 0xFF));
-}
-
/**
* is_hex
*
@@ -83,14 +67,12 @@ static inline u8 is_hex(char p)
}
/**
- * is_checksum_valid
- *
* Validate the checksum in the provided TlvInfo EEPROM data.
First,
* verify that the TlvInfo header is valid, then make sure the last
*
Re: [PATCH 04/12] cmd: tlv_eeprom: convert functions used by command to api functions
\o/
Am 03.05.22 um 13:54 schrieb Stefan Roese:
Hi Josua,
On 03.05.22 09:17, Josua Mayer wrote:
Am 03.05.22 um 09:16 schrieb Stefan Roese:
On 02.05.22 16:18, Josua Mayer wrote:
- prog_eeprom: write_tlvinfo_tlv_eeprom
- update_crc: tlvinfo_update_crc
- is_valid_tlv: is_valid_tlvinfo_entry
- is_checksum_valid: tlvinfo_check_crc
So while creating a new API it makes sense to prepend the function
names identical IMHO to not "pollute" the namespace. Something like
- tlv_is_valid_entry
- tlv_check_crc
...
Just examples, you get the idea.
Yes. The hard part in this particular implementation is that the
naming is not consistent.
The most sense I could make is that prefix tlvinfo indicates all tlv
data, i.e. working with the whole structure, while tlvinfo_tlv
indicates working with one data entry. Further write, read and is_
are currently prefixed in the header, but for previously static
functions in the C file it was put in the middle ...
I found it quite difficult to prepare for splitting off a library in
a way that preserves history, i.e. diffs should still be readable for
spotting mistakes.
Yes, a decent history would be welcome. But still, when going global
here with a new API this should be consistant.
My view more like - patches 1-10 are not really new API, in that I am
only changing what is necessary to allow splitting the code.
I was considering to at the very end do a mass-rename and come up
with better naming, something like
tlv_{set,get}_{blob,string,mac}
tlv_find_entry
tlv_{read,write}_eeprom
But this is pending a refactoring and extension of the tlv parsing
code in board/solidrun/common/tlv_data.*, to figure out what is
required or useful.
So your plan is to this:
a) Get this patchset included
b) Use it in board specific code, e.g. solidrun
c) Do the mass-rename
Is this correct?
This is close. I would say
1) get the split merged
2) rebase board-specific code
2a) figure out what kinds of set/get/add/remove functions are useful
3) redesign api
- there are inconsistencies with the order of function arguments
- read/write functions imo should use the tlv header to determine
size, not a function argument
- at least one of the tlv data types can appear multiple times,
existing code does not support this
4) submit any renames and extensions to the tlv library
5) submit board-specific use of tlv eeprom data
If yes, why is it better to do the renaming at the end?
It is very difficult to work on a patch-set that touches the same code
before and after moving it to a different file, which I found myself
doing a lot while prototyping this.
So if now I went ahead and figured out proper names and purposes for all
functions that I think should be the tlv api, then I have to divide that
into those parts that are renames or refactoring of existing
functionality, and those parts that are strictly new - putting the
former before splitting off the library, and the latter to after.
I am not sure I can manage this level of complexity.
- Josua Mayer
Thanks,
Stefan
Thanks,
Stefan
Signed-off-by: Josua Mayer
---
cmd/tlv_eeprom.c | 56
+++
include/tlv_eeprom.h | 57
2 files changed, 76 insertions(+), 37 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c
index 00c5b5f840..1b4f2537f6 100644
--- a/cmd/tlv_eeprom.c
+++ b/cmd/tlv_eeprom.c
@@ -28,13 +28,9 @@ DECLARE_GLOBAL_DATA_PTR;
#define MAX_TLV_DEVICES 2
/* File scope function prototypes */
-static bool is_checksum_valid(u8 *eeprom);
static int read_eeprom(int devnum, u8 *eeprom);
static void show_eeprom(int devnum, u8 *eeprom);
static void decode_tlv(struct tlvinfo_tlv *tlv);
-static void update_crc(u8 *eeprom);
-static int prog_eeprom(int devnum, u8 *eeprom);
-static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int
*eeprom_index);
static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code);
static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval);
static int set_mac(char *buf, const char *string);
@@ -58,18 +54,6 @@ static inline bool is_digit(char c)
return (c >= '0' && c <= '9');
}
-/**
- * is_valid_tlv
- *
- * Perform basic sanity checks on a TLV field. The TLV is pointed to
- * by the parameter provided.
- * 1. The type code is not reserved (0x00 or 0xFF)
- */
-static inline bool is_valid_tlv(struct tlvinfo_tlv *tlv)
-{
- return((tlv->type != 0x00) && (tlv->type != 0xFF));
-}
-
/**
* is_hex
*
@@ -83,14 +67,12 @@ static inline u8 is_hex(char p)
}
/**
- * is_checksum_valid
- *
* Validate the checksum in the provided TlvInfo EEPROM data.
First,
* verify that the TlvInfo header is valid, then make sure the last
* TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data
* and compare it to the value stored in the EEPROM CRC-32 TLV.
*/
-static bool is_checksum_valid(u8 *eeprom)
+bool tlvinfo_check_crc(u8 *eeprom)
{
str
Re: [PATCH 04/12] cmd: tlv_eeprom: convert functions used by command to api functions
Hi Josua,
On 03.05.22 09:17, Josua Mayer wrote:
Am 03.05.22 um 09:16 schrieb Stefan Roese:
On 02.05.22 16:18, Josua Mayer wrote:
- prog_eeprom: write_tlvinfo_tlv_eeprom
- update_crc: tlvinfo_update_crc
- is_valid_tlv: is_valid_tlvinfo_entry
- is_checksum_valid: tlvinfo_check_crc
So while creating a new API it makes sense to prepend the function
names identical IMHO to not "pollute" the namespace. Something like
- tlv_is_valid_entry
- tlv_check_crc
...
Just examples, you get the idea.
Yes. The hard part in this particular implementation is that the naming
is not consistent.
The most sense I could make is that prefix tlvinfo indicates all tlv
data, i.e. working with the whole structure, while tlvinfo_tlv indicates
working with one data entry. Further write, read and is_ are currently
prefixed in the header, but for previously static functions in the C
file it was put in the middle ...
I found it quite difficult to prepare for splitting off a library in a
way that preserves history, i.e. diffs should still be readable for
spotting mistakes.
Yes, a decent history would be welcome. But still, when going global
here with a new API this should be consistant.
I was considering to at the very end do a mass-rename and come up with
better naming, something like
tlv_{set,get}_{blob,string,mac}
tlv_find_entry
tlv_{read,write}_eeprom
But this is pending a refactoring and extension of the tlv parsing code
in board/solidrun/common/tlv_data.*, to figure out what is required or
useful.
So your plan is to this:
a) Get this patchset included
b) Use it in board specific code, e.g. solidrun
c) Do the mass-rename
Is this correct? If yes, why is it better to do the renaming at the end?
Thanks,
Stefan
Thanks,
Stefan
Signed-off-by: Josua Mayer
---
cmd/tlv_eeprom.c | 56 +++
include/tlv_eeprom.h | 57
2 files changed, 76 insertions(+), 37 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c
index 00c5b5f840..1b4f2537f6 100644
--- a/cmd/tlv_eeprom.c
+++ b/cmd/tlv_eeprom.c
@@ -28,13 +28,9 @@ DECLARE_GLOBAL_DATA_PTR;
#define MAX_TLV_DEVICES 2
/* File scope function prototypes */
-static bool is_checksum_valid(u8 *eeprom);
static int read_eeprom(int devnum, u8 *eeprom);
static void show_eeprom(int devnum, u8 *eeprom);
static void decode_tlv(struct tlvinfo_tlv *tlv);
-static void update_crc(u8 *eeprom);
-static int prog_eeprom(int devnum, u8 *eeprom);
-static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index);
static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code);
static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval);
static int set_mac(char *buf, const char *string);
@@ -58,18 +54,6 @@ static inline bool is_digit(char c)
return (c >= '0' && c <= '9');
}
-/**
- * is_valid_tlv
- *
- * Perform basic sanity checks on a TLV field. The TLV is pointed to
- * by the parameter provided.
- * 1. The type code is not reserved (0x00 or 0xFF)
- */
-static inline bool is_valid_tlv(struct tlvinfo_tlv *tlv)
-{
- return((tlv->type != 0x00) && (tlv->type != 0xFF));
-}
-
/**
* is_hex
*
@@ -83,14 +67,12 @@ static inline u8 is_hex(char p)
}
/**
- * is_checksum_valid
- *
* Validate the checksum in the provided TlvInfo EEPROM data. First,
* verify that the TlvInfo header is valid, then make sure the last
* TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data
* and compare it to the value stored in the EEPROM CRC-32 TLV.
*/
-static bool is_checksum_valid(u8 *eeprom)
+bool tlvinfo_check_crc(u8 *eeprom)
{
struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
struct tlvinfo_tlv *eeprom_crc;
@@ -137,11 +119,11 @@ static int read_eeprom(int devnum, u8 *eeprom)
// If the contents are invalid, start over with default contents
if (!is_valid_tlvinfo_header(eeprom_hdr) ||
- !is_checksum_valid(eeprom)) {
+ !tlvinfo_check_crc(eeprom)) {
strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING);
eeprom_hdr->version = TLV_INFO_VERSION;
eeprom_hdr->totallen = cpu_to_be16(0);
- update_crc(eeprom);
+ tlvinfo_update_crc(eeprom);
}
#ifdef DEBUG
@@ -183,7 +165,7 @@ static void show_eeprom(int devnum, u8 *eeprom)
tlv_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen);
while (curr_tlv < tlv_end) {
eeprom_tlv = to_entry(&eeprom[curr_tlv]);
- if (!is_valid_tlv(eeprom_tlv)) {
+ if (!is_valid_tlvinfo_entry(eeprom_tlv)) {
printf("Invalid TLV field starting at EEPROM offset %d\n",
curr_tlv);
return;
@@ -193,7 +175,7 @@ static void show_eeprom(int devnum, u8 *eeprom)
}
printf("Checksum is %s.\n",
- is_checksum_valid(eeprom) ? "valid" : "invalid");
+ tlvinfo_check_crc(eeprom) ? "valid" : "invalid");
#ifdef DEBUG
printf("EEPROM
Re: [PATCH 04/12] cmd: tlv_eeprom: convert functions used by command to api functions
Am 03.05.22 um 09:16 schrieb Stefan Roese:
On 02.05.22 16:18, Josua Mayer wrote:
- prog_eeprom: write_tlvinfo_tlv_eeprom
- update_crc: tlvinfo_update_crc
- is_valid_tlv: is_valid_tlvinfo_entry
- is_checksum_valid: tlvinfo_check_crc
So while creating a new API it makes sense to prepend the function
names identical IMHO to not "pollute" the namespace. Something like
- tlv_is_valid_entry
- tlv_check_crc
...
Just examples, you get the idea.
Yes. The hard part in this particular implementation is that the naming
is not consistent.
The most sense I could make is that prefix tlvinfo indicates all tlv
data, i.e. working with the whole structure, while tlvinfo_tlv indicates
working with one data entry. Further write, read and is_ are currently
prefixed in the header, but for previously static functions in the C
file it was put in the middle ...
I found it quite difficult to prepare for splitting off a library in a
way that preserves history, i.e. diffs should still be readable for
spotting mistakes.
I was considering to at the very end do a mass-rename and come up with
better naming, something like
tlv_{set,get}_{blob,string,mac}
tlv_find_entry
tlv_{read,write}_eeprom
But this is pending a refactoring and extension of the tlv parsing code
in board/solidrun/common/tlv_data.*, to figure out what is required or
useful.
Thanks,
Stefan
Signed-off-by: Josua Mayer
---
cmd/tlv_eeprom.c | 56 +++
include/tlv_eeprom.h | 57
2 files changed, 76 insertions(+), 37 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c
index 00c5b5f840..1b4f2537f6 100644
--- a/cmd/tlv_eeprom.c
+++ b/cmd/tlv_eeprom.c
@@ -28,13 +28,9 @@ DECLARE_GLOBAL_DATA_PTR;
#define MAX_TLV_DEVICES 2
/* File scope function prototypes */
-static bool is_checksum_valid(u8 *eeprom);
static int read_eeprom(int devnum, u8 *eeprom);
static void show_eeprom(int devnum, u8 *eeprom);
static void decode_tlv(struct tlvinfo_tlv *tlv);
-static void update_crc(u8 *eeprom);
-static int prog_eeprom(int devnum, u8 *eeprom);
-static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index);
static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code);
static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval);
static int set_mac(char *buf, const char *string);
@@ -58,18 +54,6 @@ static inline bool is_digit(char c)
return (c >= '0' && c <= '9');
}
-/**
- * is_valid_tlv
- *
- * Perform basic sanity checks on a TLV field. The TLV is pointed to
- * by the parameter provided.
- * 1. The type code is not reserved (0x00 or 0xFF)
- */
-static inline bool is_valid_tlv(struct tlvinfo_tlv *tlv)
-{
- return((tlv->type != 0x00) && (tlv->type != 0xFF));
-}
-
/**
* is_hex
*
@@ -83,14 +67,12 @@ static inline u8 is_hex(char p)
}
/**
- * is_checksum_valid
- *
* Validate the checksum in the provided TlvInfo EEPROM data. First,
* verify that the TlvInfo header is valid, then make sure the last
* TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data
* and compare it to the value stored in the EEPROM CRC-32 TLV.
*/
-static bool is_checksum_valid(u8 *eeprom)
+bool tlvinfo_check_crc(u8 *eeprom)
{
struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
struct tlvinfo_tlv *eeprom_crc;
@@ -137,11 +119,11 @@ static int read_eeprom(int devnum, u8 *eeprom)
// If the contents are invalid, start over with default contents
if (!is_valid_tlvinfo_header(eeprom_hdr) ||
- !is_checksum_valid(eeprom)) {
+ !tlvinfo_check_crc(eeprom)) {
strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING);
eeprom_hdr->version = TLV_INFO_VERSION;
eeprom_hdr->totallen = cpu_to_be16(0);
- update_crc(eeprom);
+ tlvinfo_update_crc(eeprom);
}
#ifdef DEBUG
@@ -183,7 +165,7 @@ static void show_eeprom(int devnum, u8 *eeprom)
tlv_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen);
while (curr_tlv < tlv_end) {
eeprom_tlv = to_entry(&eeprom[curr_tlv]);
- if (!is_valid_tlv(eeprom_tlv)) {
+ if (!is_valid_tlvinfo_entry(eeprom_tlv)) {
printf("Invalid TLV field starting at EEPROM offset %d\n",
curr_tlv);
return;
@@ -193,7 +175,7 @@ static void show_eeprom(int devnum, u8 *eeprom)
}
printf("Checksum is %s.\n",
- is_checksum_valid(eeprom) ? "valid" : "invalid");
+ tlvinfo_check_crc(eeprom) ? "valid" : "invalid");
#ifdef DEBUG
printf("EEPROM dump: (0x%x bytes)", TLV_INFO_MAX_LEN);
@@ -340,13 +322,13 @@ static void decode_tlv(struct tlvinfo_tlv *tlv)
}
/**
- * update_crc
+ * tlvinfo_update_crc
*
* This function updates the CRC-32 TLV. If there is no CRC-32
TLV, then
* one is added. This function should be called after each update
to the
* EEPROM structure, to make sure the CRC is always corre
Re: [PATCH 04/12] cmd: tlv_eeprom: convert functions used by command to api functions
On 02.05.22 16:18, Josua Mayer wrote:
- prog_eeprom: write_tlvinfo_tlv_eeprom
- update_crc: tlvinfo_update_crc
- is_valid_tlv: is_valid_tlvinfo_entry
- is_checksum_valid: tlvinfo_check_crc
So while creating a new API it makes sense to prepend the function
names identical IMHO to not "pollute" the namespace. Something like
- tlv_is_valid_entry
- tlv_check_crc
...
Just examples, you get the idea.
Thanks,
Stefan
Signed-off-by: Josua Mayer
---
cmd/tlv_eeprom.c | 56 +++
include/tlv_eeprom.h | 57
2 files changed, 76 insertions(+), 37 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c
index 00c5b5f840..1b4f2537f6 100644
--- a/cmd/tlv_eeprom.c
+++ b/cmd/tlv_eeprom.c
@@ -28,13 +28,9 @@ DECLARE_GLOBAL_DATA_PTR;
#define MAX_TLV_DEVICES 2
/* File scope function prototypes */
-static bool is_checksum_valid(u8 *eeprom);
static int read_eeprom(int devnum, u8 *eeprom);
static void show_eeprom(int devnum, u8 *eeprom);
static void decode_tlv(struct tlvinfo_tlv *tlv);
-static void update_crc(u8 *eeprom);
-static int prog_eeprom(int devnum, u8 *eeprom);
-static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index);
static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code);
static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval);
static int set_mac(char *buf, const char *string);
@@ -58,18 +54,6 @@ static inline bool is_digit(char c)
return (c >= '0' && c <= '9');
}
-/**
- * is_valid_tlv
- *
- * Perform basic sanity checks on a TLV field. The TLV is pointed to
- * by the parameter provided.
- * 1. The type code is not reserved (0x00 or 0xFF)
- */
-static inline bool is_valid_tlv(struct tlvinfo_tlv *tlv)
-{
- return((tlv->type != 0x00) && (tlv->type != 0xFF));
-}
-
/**
* is_hex
*
@@ -83,14 +67,12 @@ static inline u8 is_hex(char p)
}
/**
- * is_checksum_valid
- *
* Validate the checksum in the provided TlvInfo EEPROM data. First,
* verify that the TlvInfo header is valid, then make sure the last
* TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data
* and compare it to the value stored in the EEPROM CRC-32 TLV.
*/
-static bool is_checksum_valid(u8 *eeprom)
+bool tlvinfo_check_crc(u8 *eeprom)
{
struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
struct tlvinfo_tlv*eeprom_crc;
@@ -137,11 +119,11 @@ static int read_eeprom(int devnum, u8 *eeprom)
// If the contents are invalid, start over with default contents
if (!is_valid_tlvinfo_header(eeprom_hdr) ||
- !is_checksum_valid(eeprom)) {
+ !tlvinfo_check_crc(eeprom)) {
strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING);
eeprom_hdr->version = TLV_INFO_VERSION;
eeprom_hdr->totallen = cpu_to_be16(0);
- update_crc(eeprom);
+ tlvinfo_update_crc(eeprom);
}
#ifdef DEBUG
@@ -183,7 +165,7 @@ static void show_eeprom(int devnum, u8 *eeprom)
tlv_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen);
while (curr_tlv < tlv_end) {
eeprom_tlv = to_entry(&eeprom[curr_tlv]);
- if (!is_valid_tlv(eeprom_tlv)) {
+ if (!is_valid_tlvinfo_entry(eeprom_tlv)) {
printf("Invalid TLV field starting at EEPROM offset
%d\n",
curr_tlv);
return;
@@ -193,7 +175,7 @@ static void show_eeprom(int devnum, u8 *eeprom)
}
printf("Checksum is %s.\n",
- is_checksum_valid(eeprom) ? "valid" : "invalid");
+ tlvinfo_check_crc(eeprom) ? "valid" : "invalid");
#ifdef DEBUG
printf("EEPROM dump: (0x%x bytes)", TLV_INFO_MAX_LEN);
@@ -340,13 +322,13 @@ static void decode_tlv(struct tlvinfo_tlv *tlv)
}
/**
- * update_crc
+ * tlvinfo_update_crc
*
* This function updates the CRC-32 TLV. If there is no CRC-32 TLV, then
* one is added. This function should be called after each update to the
* EEPROM structure, to make sure the CRC is always correct.
*/
-static void update_crc(u8 *eeprom)
+void tlvinfo_update_crc(u8 *eeprom)
{
struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
struct tlvinfo_tlv*eeprom_crc;
@@ -376,20 +358,20 @@ static void update_crc(u8 *eeprom)
}
/**
- * prog_eeprom
+ * write_tlvinfo_tlv_eeprom
*
- * Write the EEPROM data from CPU memory to the hardware.
+ * Write the TLV data from CPU memory to the hardware.
*/
-static int prog_eeprom(int devnum, u8 *eeprom)
+int write_tlvinfo_tlv_eeprom(void *eeprom, int dev)
{
int ret = 0;
struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
int eeprom_len;
- update_crc(eeprom);
+ tlvinfo_update_crc(eeprom);
eeprom_len = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen);
- ret = write_tlv_eeprom(eeprom, eeprom_l
[PATCH 04/12] cmd: tlv_eeprom: convert functions used by command to api functions
- prog_eeprom: write_tlvinfo_tlv_eeprom
- update_crc: tlvinfo_update_crc
- is_valid_tlv: is_valid_tlvinfo_entry
- is_checksum_valid: tlvinfo_check_crc
Signed-off-by: Josua Mayer
---
cmd/tlv_eeprom.c | 56 +++
include/tlv_eeprom.h | 57
2 files changed, 76 insertions(+), 37 deletions(-)
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c
index 00c5b5f840..1b4f2537f6 100644
--- a/cmd/tlv_eeprom.c
+++ b/cmd/tlv_eeprom.c
@@ -28,13 +28,9 @@ DECLARE_GLOBAL_DATA_PTR;
#define MAX_TLV_DEVICES2
/* File scope function prototypes */
-static bool is_checksum_valid(u8 *eeprom);
static int read_eeprom(int devnum, u8 *eeprom);
static void show_eeprom(int devnum, u8 *eeprom);
static void decode_tlv(struct tlvinfo_tlv *tlv);
-static void update_crc(u8 *eeprom);
-static int prog_eeprom(int devnum, u8 *eeprom);
-static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index);
static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code);
static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval);
static int set_mac(char *buf, const char *string);
@@ -58,18 +54,6 @@ static inline bool is_digit(char c)
return (c >= '0' && c <= '9');
}
-/**
- * is_valid_tlv
- *
- * Perform basic sanity checks on a TLV field. The TLV is pointed to
- * by the parameter provided.
- * 1. The type code is not reserved (0x00 or 0xFF)
- */
-static inline bool is_valid_tlv(struct tlvinfo_tlv *tlv)
-{
- return((tlv->type != 0x00) && (tlv->type != 0xFF));
-}
-
/**
* is_hex
*
@@ -83,14 +67,12 @@ static inline u8 is_hex(char p)
}
/**
- * is_checksum_valid
- *
* Validate the checksum in the provided TlvInfo EEPROM data. First,
* verify that the TlvInfo header is valid, then make sure the last
* TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data
* and compare it to the value stored in the EEPROM CRC-32 TLV.
*/
-static bool is_checksum_valid(u8 *eeprom)
+bool tlvinfo_check_crc(u8 *eeprom)
{
struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
struct tlvinfo_tlv*eeprom_crc;
@@ -137,11 +119,11 @@ static int read_eeprom(int devnum, u8 *eeprom)
// If the contents are invalid, start over with default contents
if (!is_valid_tlvinfo_header(eeprom_hdr) ||
- !is_checksum_valid(eeprom)) {
+ !tlvinfo_check_crc(eeprom)) {
strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING);
eeprom_hdr->version = TLV_INFO_VERSION;
eeprom_hdr->totallen = cpu_to_be16(0);
- update_crc(eeprom);
+ tlvinfo_update_crc(eeprom);
}
#ifdef DEBUG
@@ -183,7 +165,7 @@ static void show_eeprom(int devnum, u8 *eeprom)
tlv_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen);
while (curr_tlv < tlv_end) {
eeprom_tlv = to_entry(&eeprom[curr_tlv]);
- if (!is_valid_tlv(eeprom_tlv)) {
+ if (!is_valid_tlvinfo_entry(eeprom_tlv)) {
printf("Invalid TLV field starting at EEPROM offset
%d\n",
curr_tlv);
return;
@@ -193,7 +175,7 @@ static void show_eeprom(int devnum, u8 *eeprom)
}
printf("Checksum is %s.\n",
- is_checksum_valid(eeprom) ? "valid" : "invalid");
+ tlvinfo_check_crc(eeprom) ? "valid" : "invalid");
#ifdef DEBUG
printf("EEPROM dump: (0x%x bytes)", TLV_INFO_MAX_LEN);
@@ -340,13 +322,13 @@ static void decode_tlv(struct tlvinfo_tlv *tlv)
}
/**
- * update_crc
+ * tlvinfo_update_crc
*
* This function updates the CRC-32 TLV. If there is no CRC-32 TLV, then
* one is added. This function should be called after each update to the
* EEPROM structure, to make sure the CRC is always correct.
*/
-static void update_crc(u8 *eeprom)
+void tlvinfo_update_crc(u8 *eeprom)
{
struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
struct tlvinfo_tlv*eeprom_crc;
@@ -376,20 +358,20 @@ static void update_crc(u8 *eeprom)
}
/**
- * prog_eeprom
+ * write_tlvinfo_tlv_eeprom
*
- * Write the EEPROM data from CPU memory to the hardware.
+ * Write the TLV data from CPU memory to the hardware.
*/
-static int prog_eeprom(int devnum, u8 *eeprom)
+int write_tlvinfo_tlv_eeprom(void *eeprom, int dev)
{
int ret = 0;
struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
int eeprom_len;
- update_crc(eeprom);
+ tlvinfo_update_crc(eeprom);
eeprom_len = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen);
- ret = write_tlv_eeprom(eeprom, eeprom_len, devnum);
+ ret = write_tlv_eeprom(eeprom, eeprom_len, dev);
if (ret) {
printf("Programming failed.\n");
return -1;
@@ -462,13 +444,13 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int
argc, char *const argv[])
if (argc == 2) {
