[PATCH v3 4/5] discover/platform-powerpc: add mailbox message structure

[Maxim Polyakov]

08.10.2019 12:30, Jeremy Kerr:
> Hi Maxim,
> 
> Thanks for your patience while reviewing your patches, I've had a bit of
> time to take a look now :)

Hi, Jeremy. Thanks for review

> 
> This series looks good - just one thing to raise with this patch, but
> it's pretty minor:
> 
>> +typedef struct __attribute__((packed)) {
>> +	uint8_t iana[CHASSIS_BOOT_MBOX_IANA_SZ];
>> +	uint8_t data[CHASSIS_BOOT_MBOX_BLOCK0_DATA_SZ];
>> +} mbox_block0_t;
> 
> We generally don't define new typedefs in the petitboot codebase. Can we
> just use the struct names instead?

Yes, I removed all types for my structures.

> 
> And do they need to be separate structs, or shoud we just have the top-
> level 'struct ipmi_mbox_response' ?

@@ -538,15 +537,15 @@ static int get_ipmi_boot_mailbox(struct platform_powerpc *platform,
 	 * on higher numbers.
 	 */

 	for (i = 0; i < UCHAR_MAX; i++) {

-		rc = get_ipmi_boot_mailbox_block(platform, block_buffer, i);
-		if (rc < 3 && i == 0) {
+		int block_size = get_ipmi_boot_mailbox_block(platform, &mailbox, i);

It would be better to split the ipmi message into separate structures, since
outside the get_ipmi_boot_mailbox_block() function we use only the ipmi message
payload - mailbox data (or 'union mbox' in the code), without msg header and so on.

I would not like to change anything in the structures.
So the code is cleaner and more understandable for me. 

>
> [Also, I'm fine with your original patch of keeping these in platform-
> powerpc.c, as there's no current users outside of that file. ipmi.h is
> fine though, no need to change it if you prefer it as-is].

Ok, I moved these structures back to the platform-powerpc.c file. 

> 
> Cheers,
> 
> 
> Jeremy
> 
> 

-- 
Regards,
Maxim Polyakov 
Software Engineer, YADRO.