Messages in this thread | | | Date | Fri, 15 Apr 2022 20:05:23 +0200 | Subject | Re: [PATCH v3 3/8] staging: r8188eu: clean up comments in struct rt_firmware_hdr | From | Michael Straube <> |
| |
On 4/15/22 17:44, Larry Finger wrote: > On 4/15/22 07:10, Michael Straube wrote: >> The comments in struct rt_firmware_hdr are not needed. >> Remove them. >> >> Signed-off-by: Michael Straube <straube.linux@gmail.com> >> --- >> v3: >> - no changes >> >> v2: >> - no changes >> >> drivers/staging/r8188eu/core/rtw_fw.c | 37 ++++++++------------------- >> 1 file changed, 11 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/staging/r8188eu/core/rtw_fw.c >> b/drivers/staging/r8188eu/core/rtw_fw.c >> index 7cd08268f3b9..323e0c634c4e 100644 >> --- a/drivers/staging/r8188eu/core/rtw_fw.c >> +++ b/drivers/staging/r8188eu/core/rtw_fw.c >> @@ -14,37 +14,22 @@ >> (le16_to_cpu(_fwhdr->Signature) & 0xFFF0) == 0x2300 || \ >> (le16_to_cpu(_fwhdr->Signature) & 0xFFF0) == 0x88E0) >> -/* This structure must be careful with byte-ordering */ >> - >> struct rt_firmware_hdr { >> - /* 8-byte alinment required */ >> - /* LONG WORD 0 ---- */ >> - __le16 Signature; /* 92C0: test chip; 92C, >> - * 88C0: test chip; 88C1: MP A-cut; >> - * 92C1: MP A-cut */ >> - u8 Category; /* AP/NIC and USB/PCI */ >> - u8 Function; /* Reserved for different FW function >> - * indcation, for further use when >> - * driver needs to download different >> - * FW for different conditions */ >> - __le16 Version; /* FW Version */ >> - u8 Subversion; /* FW Subversion, default 0x00 */ >> + __le16 Signature; >> + u8 Category; >> + u8 Function; >> + __le16 Version; >> + u8 Subversion; >> u8 Rsvd1; >> - >> - /* LONG WORD 1 ---- */ >> - u8 Month; /* Release time Month field */ >> - u8 Date; /* Release time Date field */ >> - u8 Hour; /* Release time Hour field */ >> - u8 Minute; /* Release time Minute field */ >> - __le16 RamCodeSize; /* The size of RAM code */ >> + u8 Month; >> + u8 Date; >> + u8 Hour; >> + u8 Minute; >> + __le16 RamCodeSize; >> u8 Foundry; >> u8 Rsvd2; >> - >> - /* LONG WORD 2 ---- */ >> - __le32 SvnIdx; /* The SVN entry index */ >> + __le32 SvnIdx; >> __le32 Rsvd3; >> - >> - /* LONG WORD 3 ---- */ >> __le32 Rsvd4; >> __le32 Rsvd5; >> }; > > The comments "LONG WORD" are useless, but the comments describing the > fields are still useful. I do not like this patch. >
Hi Larry,
You are right the in-line comments are useful. I'll send v4 keeping them.
You only mentioned the in-line comments, just to get it right this time:
What about the "8-byte alignment required" comment? You said in another thread that the __le16 references need alignment 4. Should I make the struct __aligned(8) or at least __aligned(4)?
And about "/* This structure must be careful with byte-ordering */" ? I think it's obvious because of the __le16 and __le32 fields and can be removed. Do you agree on that?
regards, Michael
| |