Re: [PATCH qemu 1/3] hw/block: Add Macronix MX25R6435F flash

[Samuel Tardieu <sam@rfc1149.net>]

~mohammadodeh <mohammadodeh@git.sr.ht> writes:

>      { INFO("mx25l25655e", 0xc22619,      0,  64 << 10, 512, 0) },
> +    { INFO("mx25r6435",   0xc22817,      0,  64 << 10, 128, ER_4K | ER_32K),
> +      .sfdp_read = m25p80_sfdp_mx25r6435 },
>      { INFO("mx66l51235f", 0xc2201a,      0,  64 << 10, 1024, ER_4K | ER_32K) },

By looking at the surrounding lines, it looks like the part suffix ("f" here) is usually included. Shouldn't it be "mx25r6435f" instead of "mx25r6435"?

>      PP = 0x02,
>      PP4 = 0x12,
>      PP4_4 = 0x3e,
> +    QPP_1_4_4 = 0x38,
>      DPP = 0xa2,
>      QPP = 0x32,
>      QPP_4 = 0x34,

I would have put the `QPP_1_4_4` definition after the `QPP_4` to group the `QPP` instances.

> @@ -759,6 +762,7 @@ static void complete_collecting_data(Flash *s)
>  
>      switch (s->cmd_in_progress) {
>      case DPP:
> +    case QPP_1_4_4:
>      case QPP:
>      case QPP_4:
>      case PP:

Same here.

Also, you haven't added a case to the switch in `get_addr_length()`. Is that because it matches the default case, or is that an oversight?

Best.

  Sam
-- 
Samuel Tardieu