[PATCH] cmd: tlv_eeprom: fix signature for populate_serial_number function

[PATCH] cmd: tlv_eeprom: fix signature for populate_serial_number function

[Josua Mayer]

populate_serial_number is not used internally for the tlv_eeprom
command, but rather provided as a library function for external use..
Remove the devnum that had recently been added by mistake, returning the
function to its original signature.

Instead place a 0-initialised member variable inside the function to
same purpose, along with a node that it only supports reading from the
first EEPROM in the system.

Fixes: dfda0c0 ("cmd: tlv_eeprom: remove use of global variable current_dev")
Signed-off-by: Josua Mayer <josua@solid-run.com>
Cc: Stefan Roese <sr@denx.de>
Cc: Baruch Siach <baruch@tkos.co.il>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 cmd/tlv_eeprom.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c
index 79796394c5c..0ca4d714645 100644
--- a/cmd/tlv_eeprom.c
+++ b/cmd/tlv_eeprom.c
@@ -1100,11 +1100,12 @@ int mac_read_from_eeprom(void)
  *
  *  This function must be called after relocation.
  */
-int populate_serial_number(int devnum)
+int populate_serial_number(void)
 {
 	char serialstr[257];
 	int eeprom_index;
 	struct tlvinfo_tlv *eeprom_tlv;
+	int devnum = 0; // TODO: support multiple EEPROMs
 
 	if (env_get("serial#"))
 		return 0;
-- 
2.35.3

Re: [PATCH] cmd: tlv_eeprom: fix signature for populate_serial_number function

[Heinrich Schuchardt]

On 5/16/23 10:27, Josua Mayer wrote:
> populate_serial_number is not used internally for the tlv_eeprom
> command, but rather provided as a library function for external use..
> Remove the devnum that had recently been added by mistake, returning the
> function to its original signature.
>
> Instead place a 0-initialised member variable inside the function to
> same purpose, along with a node that it only supports reading from the

%s/node/note/

> first EEPROM in the system.
>
> Fixes: dfda0c0 ("cmd: tlv_eeprom: remove use of global variable current_dev")
> Signed-off-by: Josua Mayer <josua@solid-run.com>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Baruch Siach <baruch@tkos.co.il>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>   cmd/tlv_eeprom.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c
> index 79796394c5c..0ca4d714645 100644
> --- a/cmd/tlv_eeprom.c
> +++ b/cmd/tlv_eeprom.c
> @@ -1100,11 +1100,12 @@ int mac_read_from_eeprom(void)
>    *
>    *  This function must be called after relocation.
>    */
> -int populate_serial_number(int devnum)
> +int populate_serial_number(void)

If populate_serial_number() is to be used as a library function, it
should live in lib/ or possibly in drivers/misc/. The definition needs
to be provided in an include file. Otherwise the function should be deleted.

Where will this library function be used?

Shouldn't the EEPROM with the serial number be identified via the
device-tree?

Best regards

Heinrich

>   {
>   	char serialstr[257];
>   	int eeprom_index;
>   	struct tlvinfo_tlv *eeprom_tlv;
> +	int devnum = 0; // TODO: support multiple EEPROMs
>
>   	if (env_get("serial#"))
>   		return 0;

Re: [PATCH] cmd: tlv_eeprom: fix signature for populate_serial_number function

[Josua Mayer]

Hi Heinrich,

Am 16.05.23 um 14:21 schrieb Heinrich Schuchardt:
> On 5/16/23 10:27, Josua Mayer wrote:
>> populate_serial_number is not used internally for the tlv_eeprom
>> command, but rather provided as a library function for external use..
>> Remove the devnum that had recently been added by mistake, returning the
>> function to its original signature.
>>
>> Instead place a 0-initialised member variable inside the function to
>> same purpose, along with a node that it only supports reading from the
>
> %s/node/note/
Good find!
>
>> first EEPROM in the system.
>>
>> Fixes: dfda0c0 ("cmd: tlv_eeprom: remove use of global variable 
>> current_dev")
>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>> Cc: Stefan Roese <sr@denx.de>
>> Cc: Baruch Siach <baruch@tkos.co.il>
>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>   cmd/tlv_eeprom.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c
>> index 79796394c5c..0ca4d714645 100644
>> --- a/cmd/tlv_eeprom.c
>> +++ b/cmd/tlv_eeprom.c
>> @@ -1100,11 +1100,12 @@ int mac_read_from_eeprom(void)
>>    *
>>    *  This function must be called after relocation.
>>    */
>> -int populate_serial_number(int devnum)
>> +int populate_serial_number(void)
>
> If populate_serial_number() is to be used as a library function, it
> should live in lib/ or possibly in drivers/misc/. The definition needs
> to be provided in an include file. Otherwise the function should be 
> deleted.
>
> Where will this library function be used?
I don't know for sure. GitHub search finds its use in board files on 
some old ONIE u-boot forks.

Main purpose of this patch right now is to undo my accidental change and 
restore the default behaviour:
The previous instance of devnum variable inside tlv_eeprom.c would have 
had value 0 by default.


>
> Shouldn't the EEPROM with the serial number be identified via the
> device-tree?
Something like that would be nice.
However we also want to use identical device-tree between Linux and U-Boot.
I am not sure if we have such indication.
>
> Best regards
>
> Heinrich
>
>>   {
>>       char serialstr[257];
>>       int eeprom_index;
>>       struct tlvinfo_tlv *eeprom_tlv;
>> +    int devnum = 0; // TODO: support multiple EEPROMs
>>
>>       if (env_get("serial#"))
>>           return 0;
>

- Josua Mayer

Re: [PATCH] cmd: tlv_eeprom: fix signature for populate_serial_number function

[Heinrich Schuchardt]

On 5/16/23 13:49, Josua Mayer wrote:
> Hi Heinrich,
>
> Am 16.05.23 um 14:21 schrieb Heinrich Schuchardt:
>> On 5/16/23 10:27, Josua Mayer wrote:
>>> populate_serial_number is not used internally for the tlv_eeprom
>>> command, but rather provided as a library function for external use..
>>> Remove the devnum that had recently been added by mistake, returning the
>>> function to its original signature.
>>>
>>> Instead place a 0-initialised member variable inside the function to
>>> same purpose, along with a node that it only supports reading from the
>>
>> %s/node/note/
> Good find!
>>
>>> first EEPROM in the system.
>>>
>>> Fixes: dfda0c0 ("cmd: tlv_eeprom: remove use of global variable
>>> current_dev")
>>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>>> Cc: Stefan Roese <sr@denx.de>
>>> Cc: Baruch Siach <baruch@tkos.co.il>
>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> ---
>>>   cmd/tlv_eeprom.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c
>>> index 79796394c5c..0ca4d714645 100644
>>> --- a/cmd/tlv_eeprom.c
>>> +++ b/cmd/tlv_eeprom.c
>>> @@ -1100,11 +1100,12 @@ int mac_read_from_eeprom(void)
>>>    *
>>>    *  This function must be called after relocation.
>>>    */
>>> -int populate_serial_number(int devnum)
>>> +int populate_serial_number(void)
>>
>> If populate_serial_number() is to be used as a library function, it
>> should live in lib/ or possibly in drivers/misc/. The definition needs
>> to be provided in an include file. Otherwise the function should be
>> deleted.
>>
>> Where will this library function be used?
> I don't know for sure. GitHub search finds its use in board files on
> some old ONIE u-boot forks.
>
> Main purpose of this patch right now is to undo my accidental change and
> restore the default behaviour:
> The previous instance of devnum variable inside tlv_eeprom.c would have
> had value 0 by default.

The right places to call the function might be
board/solidrun/clearfog/clearfog.c and board/kobol/helios4/helios4.c as
these platforms enable CMD_TLV_EEPROM. But I have no such board.

The function has remained unused in upstream U-Boot since 2020. If we do
not plan to use it, it should be removed.

Stefan and Dennis, what are your thoughts?

Best regards

Heinrich

>
>
>>
>> Shouldn't the EEPROM with the serial number be identified via the
>> device-tree?
> Something like that would be nice.
> However we also want to use identical device-tree between Linux and U-Boot.
> I am not sure if we have such indication.
>>
>> Best regards
>>
>> Heinrich
>>
>>>   {
>>>       char serialstr[257];
>>>       int eeprom_index;
>>>       struct tlvinfo_tlv *eeprom_tlv;
>>> +    int devnum = 0; // TODO: support multiple EEPROMs
>>>
>>>       if (env_get("serial#"))
>>>           return 0;
>>
>
> - Josua Mayer
>