From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5CA77C43219 for ; Tue, 3 May 2022 06:53:10 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 915CA83B1B; Tue, 3 May 2022 08:53:06 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=solid-run.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=solid-run-com.20210112.gappssmtp.com header.i=@solid-run-com.20210112.gappssmtp.com header.b="1x2Z5k/n"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id E76BC83A1E; Tue, 3 May 2022 08:53:03 +0200 (CEST) Received: from mail-wr1-x42e.google.com (mail-wr1-x42e.google.com [IPv6:2a00:1450:4864:20::42e]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id B012283C08 for ; Tue, 3 May 2022 08:52:53 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=solid-run.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=josua@solid-run.com Received: by mail-wr1-x42e.google.com with SMTP id e24so22039901wrc.9 for ; Mon, 02 May 2022 23:52:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=solid-run-com.20210112.gappssmtp.com; s=20210112; h=message-id:date:mime-version:user-agent:subject:content-language:to :cc:references:from:in-reply-to:content-transfer-encoding; bh=9n8iMFshYUrxgW+Pa2cQhdMFKTceukgh1KnkBTF4y78=; b=1x2Z5k/n++v8TIiw3+G2LeRtRR2nTsfkwK3kXyi1FkTJgVov3Ea3/hk6IUQuKIZhl/ n3I73Hr42uNtpDp2OsiXFd65Kw763NjVLvQfVGMjtZkY8VBAolruTEghYtnWOJCL13jD fNkbe6Y0cVNrqASaBuiYBtfWWbit6L5+7f6Rc/bHnlerd37hbd/HICoBhsnSVxWnDy/G x4HE7HJtrTkMIB4796TbMNFF1imiqodOVFoSs+yNKoFx3qCJcXipC9frI63DhIorcLjI Dclo3Dhl3XpLqQdh9ZpWTO6+wb2WlRf4vo4/bFjng7dM/2Dx8vPaZcZIIaW8B6PEVQNe 9F4Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=9n8iMFshYUrxgW+Pa2cQhdMFKTceukgh1KnkBTF4y78=; b=BD1n5WQLoWWh4BX6+k9Ene/oTw8x6NfXbXaSJfNpXWVcyEBNHkBvTTrurx/Gag4w2N 4+1lwRyq/prKTFDByErlGlvskTXszOFnHMQ4XKrt1sw7LsdXKiVh03IsUGPK2Ii763Lk dHy8LFqv+QFPmJEvDjnzAMcOB1Vb/vNTEG08+GsTzz0/Aoqk7IwgHimv3NtqPDhqhQFA evPHj4BIVqW9lUzUognDCnE/kYGpDI+fJ84rmz2t21ggVIZb381rRRznuvv+LMmIKLv9 6b570S4p8fao2I03/OSUtoE5s0UHFi2ljwN/uAKrzJ3bwT3Wd1Tbg/COjSvbHyz17wi+ tHEw== X-Gm-Message-State: AOAM532ck6W1fcCxw9AmA0pHPHx0LQ3TpS2CPdNZJeSlesSaezIKKnqV rD6mxSLwmR8zbiOyQYWtil+GSg== X-Google-Smtp-Source: ABdhPJwcfmijMK7yxK/BygJ43B8LMwc267U7HY2lnDmWBSXYfPF8kelEHB3RBqI8spsRJjU8z40D/A== X-Received: by 2002:a05:6000:18cb:b0:207:8c65:3fd4 with SMTP id w11-20020a05600018cb00b002078c653fd4mr11570040wrq.131.1651560773247; Mon, 02 May 2022 23:52:53 -0700 (PDT) Received: from [10.149.0.162] ([194.169.217.220]) by smtp.gmail.com with ESMTPSA id s29-20020adf979d000000b0020c5253d925sm8565921wrb.113.2022.05.02.23.52.51 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 02 May 2022 23:52:52 -0700 (PDT) Message-ID: Date: Tue, 3 May 2022 09:52:00 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 Subject: Re: [PATCH 01/12] cmd: tlv_eeprom: remove use of global variable current_dev Content-Language: en-US To: Stefan Roese , u-boot@lists.denx.de Cc: Simon Glass , Sven Auhagen References: <20220502141838.15912-1-josua@solid-run.com> <20220502141838.15912-2-josua@solid-run.com> <30873f66-995b-2ced-37e0-2c444ae72394@denx.de> From: Josua Mayer In-Reply-To: <30873f66-995b-2ced-37e0-2c444ae72394@denx.de> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean Hi Stefan, Thank you for starting to review this patchset! Am 03.05.22 um 09:09 schrieb Stefan Roese: > Hi Josua, > > a few general comments about this series: > > - A cover letter for this patchset would be very helpful, so that >   reviewers have a summary of the changes. I have sent a cover-letter to the list, but you are not in the To field - perhaps you can find it - as it does give an overview what I am trying to do (subject [PATCH 00/12] split tlv_eeprom command into a separate library) > - Please Cc the original author of this code Baruch Siach in the >   next version as well (if there is next version that is). Made a mental note, will do. > > On 02.05.22 16:18, Josua Mayer wrote: >> Make tlv_eeprom command device selection an explicit parameter of all >> function calls. >> >> Signed-off-by: Josua Mayer > > Reviewed-by: Stefan Roese > > Thanks, > Stefan > >> --- >>   cmd/tlv_eeprom.c     | 50 ++++++++++++++++++++++---------------------- >>   include/tlv_eeprom.h |  3 ++- >>   2 files changed, 27 insertions(+), 26 deletions(-) >> >> diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c >> index bf8d453dc5..f91c11b304 100644 >> --- a/cmd/tlv_eeprom.c >> +++ b/cmd/tlv_eeprom.c >> @@ -29,18 +29,18 @@ DECLARE_GLOBAL_DATA_PTR; >>     /* File scope function prototypes */ >>   static bool is_checksum_valid(u8 *eeprom); >> -static int read_eeprom(u8 *eeprom); >> -static void show_eeprom(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(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); >>   static int set_date(char *buf, const char *string); >>   static int set_bytes(char *buf, const char *string, int >> *converted_accum); >> -static void show_tlv_devices(void); >> +static void show_tlv_devices(int current_dev); >>     /* Set to 1 if we've read EEPROM into memory */ >>   static int has_been_read; >> @@ -48,7 +48,6 @@ static int has_been_read; >>   static u8 eeprom[TLV_INFO_MAX_LEN]; >>     static struct udevice *tlv_devices[MAX_TLV_DEVICES]; >> -static unsigned int current_dev; >>     #define to_header(p) ((struct tlvinfo_header *)p) >>   #define to_entry(p) ((struct tlvinfo_tlv *)p) >> @@ -125,7 +124,7 @@ static bool is_checksum_valid(u8 *eeprom) >>    * >>    *  Read the EEPROM into memory, if it hasn't already been read. >>    */ >> -static int read_eeprom(u8 *eeprom) >> +static int read_eeprom(int devnum, u8 *eeprom) >>   { >>       int ret; >>       struct tlvinfo_header *eeprom_hdr = to_header(eeprom); >> @@ -135,12 +134,11 @@ static int read_eeprom(u8 *eeprom) >>           return 0; >>         /* Read the header */ >> -    ret = read_tlv_eeprom((void *)eeprom_hdr, 0, HDR_SIZE, >> current_dev); >> +    ret = read_tlv_eeprom((void *)eeprom_hdr, 0, HDR_SIZE, devnum); >>       /* If the header was successfully read, read the TLVs */ >>       if (ret == 0 && is_valid_tlvinfo_header(eeprom_hdr)) >>           ret = read_tlv_eeprom((void *)eeprom_tlv, HDR_SIZE, >> -                      be16_to_cpu(eeprom_hdr->totallen), >> -                      current_dev); >> +                      be16_to_cpu(eeprom_hdr->totallen), devnum); >>         // If the contents are invalid, start over with default contents >>       if (!is_valid_tlvinfo_header(eeprom_hdr) || >> @@ -165,7 +163,7 @@ static int read_eeprom(u8 *eeprom) >>    * >>    *  Display the contents of the EEPROM >>    */ >> -static void show_eeprom(u8 *eeprom) >> +static void show_eeprom(int devnum, u8 *eeprom) >>   { >>       int tlv_end; >>       int curr_tlv; >> @@ -180,7 +178,7 @@ static void show_eeprom(u8 *eeprom) >>           return; >>       } >>   -    printf("TLV: %u\n", current_dev); >> +    printf("TLV: %u\n", devnum); >>       printf("TlvInfo Header:\n"); >>       printf("   Id String:    %s\n", eeprom_hdr->signature); >>       printf("   Version:      %d\n", eeprom_hdr->version); >> @@ -389,7 +387,7 @@ static void update_crc(u8 *eeprom) >>    * >>    *  Write the EEPROM data from CPU memory to the hardware. >>    */ >> -static int prog_eeprom(u8 *eeprom) >> +static int prog_eeprom(int devnum, u8 *eeprom) >>   { >>       int ret = 0; >>       struct tlvinfo_header *eeprom_hdr = to_header(eeprom); >> @@ -398,7 +396,7 @@ static int prog_eeprom(u8 *eeprom) >>       update_crc(eeprom); >>         eeprom_len = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); >> -    ret = write_tlv_eeprom(eeprom, eeprom_len); >> +    ret = write_tlv_eeprom(eeprom, eeprom_len, devnum); >>       if (ret) { >>           printf("Programming failed.\n"); >>           return -1; >> @@ -433,11 +431,12 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int >> flag, int argc, char *const argv[]) >>   { >>       char cmd; >>       struct tlvinfo_header *eeprom_hdr = to_header(eeprom); >> +    static unsigned int current_dev; >>         // If no arguments, read the EERPOM and display its contents >>       if (argc == 1) { >> -        read_eeprom(eeprom); >> -        show_eeprom(eeprom); >> +        read_eeprom(current_dev, eeprom); >> +        show_eeprom(current_dev, eeprom); >>           return 0; >>       } >>   @@ -448,7 +447,7 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int >> flag, int argc, char *const argv[]) >>       // Read the EEPROM contents >>       if (cmd == 'r') { >>           has_been_read = 0; >> -        if (!read_eeprom(eeprom)) >> +        if (!read_eeprom(current_dev, eeprom)) >>               printf("EEPROM data loaded from device to memory.\n"); >>           return 0; >>       } >> @@ -463,7 +462,7 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int >> flag, int argc, char *const argv[]) >>       if (argc == 2) { >>           switch (cmd) { >>           case 'w':   /* write */ >> -            prog_eeprom(eeprom); >> +            prog_eeprom(current_dev, eeprom); >>               break; >>           case 'e':   /* erase */ >>               strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); >> @@ -476,7 +475,7 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int >> flag, int argc, char *const argv[]) >>               show_tlv_code_list(); >>               break; >>           case 'd':   /* dev */ >> -            show_tlv_devices(); >> +            show_tlv_devices(current_dev); >>               break; >>           default: >>               cmd_usage(cmdtp); >> @@ -886,7 +885,7 @@ static int set_bytes(char *buf, const char >> *string, int *converted_accum) >>       return 0; >>   } >>   -static void show_tlv_devices(void) >> +static void show_tlv_devices(int current_dev) >>   { >>       unsigned int dev; >>   @@ -956,14 +955,14 @@ int read_tlv_eeprom(void *eeprom, int offset, >> int len, int dev_num) >>   /** >>    * write_tlv_eeprom - write the hwinfo to i2c EEPROM >>    */ >> -int write_tlv_eeprom(void *eeprom, int len) >> +int write_tlv_eeprom(void *eeprom, int len, int dev) >>   { >>       if (!(gd->flags & GD_FLG_RELOC)) >>           return -ENODEV; >> -    if (!tlv_devices[current_dev]) >> +    if (!tlv_devices[dev]) >>           return -ENODEV; >>   -    return i2c_eeprom_write(tlv_devices[current_dev], 0, eeprom, >> len); >> +    return i2c_eeprom_write(tlv_devices[dev], 0, eeprom, len); >>   } >>     int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header >> **hdr, >> @@ -1018,10 +1017,11 @@ int mac_read_from_eeprom(void) >>       int maccount; >>       u8 macbase[6]; >>       struct tlvinfo_header *eeprom_hdr = to_header(eeprom); >> +    int devnum = 0; // TODO: support multiple EEPROMs >>         puts("EEPROM: "); >>   -    if (read_eeprom(eeprom)) { >> +    if (read_eeprom(devnum, eeprom)) { >>           printf("Read failed.\n"); >>           return -1; >>       } >> @@ -1086,7 +1086,7 @@ int mac_read_from_eeprom(void) >>    * >>    *  This function must be called after relocation. >>    */ >> -int populate_serial_number(void) >> +int populate_serial_number(int devnum) >>   { >>       char serialstr[257]; >>       int eeprom_index; >> @@ -1095,7 +1095,7 @@ int populate_serial_number(void) >>       if (env_get("serial#")) >>           return 0; >>   -    if (read_eeprom(eeprom)) { >> +    if (read_eeprom(devnum, eeprom)) { >>           printf("Read failed.\n"); >>           return -1; >>       } >> diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h >> index a2c333e744..fd45e5f6eb 100644 >> --- a/include/tlv_eeprom.h >> +++ b/include/tlv_eeprom.h >> @@ -84,11 +84,12 @@ int read_tlv_eeprom(void *eeprom, int offset, int >> len, int dev); >>    * write_tlv_eeprom - Write the entire EEPROM binary data to the >> hardware >>    * @eeprom: Pointer to buffer to hold the binary data >>    * @len   : Maximum size of buffer >> + * @dev   : EEPROM device to write >>    * >>    * Note: this routine does not validate the EEPROM data. >>    * >>    */ >> -int write_tlv_eeprom(void *eeprom, int len); >> +int write_tlv_eeprom(void *eeprom, int len, int dev); >>     /** >>    * read_tlvinfo_tlv_eeprom - Read the TLV from EEPROM, and validate > > Viele Grüße, > Stefan Roese >