Re: [RFC PATCH 04/10] hw/sd: Introduce sd_cmd_handler type

[Philippe Mathieu-Daudé]

On 6/28/21 9:29 AM, Cédric Le Goater wrote:
> On 6/24/21 4:22 PM, Philippe Mathieu-Daudé wrote:
>> Add 2 command handler arrays in SDProto, for CMD and ACMD.
>> Have sd_normal_command() / sd_app_command() use these arrays:
>> if an command handler is registered, call it, otherwise fall
>> back to current code base.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/sd/sd.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index a1cc8ab0be8..ce1eec0374f 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -88,8 +88,12 @@ enum SDCardStates {
>>      sd_disconnect_state,
>>  };
>>  
>> +typedef sd_rsp_type_t (*sd_cmd_handler)(SDState *sd, SDRequest req);
>> +
>>  typedef struct SDProto {
>>      const char *name;
>> +    sd_cmd_handler cmd[SDMMC_CMD_MAX];
>> +    sd_cmd_handler acmd[SDMMC_CMD_MAX];
>>  } SDProto;
> 
> 
> A class would be better but it's no big deal for the moment.

Could be. Easily modifiable later. For now I'd rather focus on
finding the easiest code path keeping maintenance simple enough,
and worry about implementation details later.

> 
>>  
>>  struct SDState {
>> @@ -994,6 +998,10 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
>> SDRequest req)
>>          return sd_illegal;
>>      }
>>  
>> +    if (sd->proto->cmd[req.cmd]) {
>> +        return sd->proto->cmd[req.cmd](sd, req);
>> +    }
>> +
> 
> I expect that some default array will be used to initialize ->cmd ?

Maybe at the end of the conversion. For now it is NULL-initialized.
See patch 3:

+static const SDProto sd_proto_sd = {
+    .name = "SD",
+};

> 
> Thanks,
> 
> C.
> 
> 
>>      switch (req.cmd) {
>>      /* Basic commands (Class 0 and Class 1) */
>>      case 0: /* CMD0:   GO_IDLE_STATE */
>> @@ -1533,6 +1541,11 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
>>      trace_sdcard_app_command(sd->proto->name, sd_acmd_name(req.cmd),
>>                               req.cmd, req.arg, sd_state_name(sd->state));
>>      sd->card_status |= APP_CMD;
>> +
>> +    if (sd->proto->acmd[req.cmd]) {
>> +        return sd->proto->acmd[req.cmd](sd, req);
>> +    }
>> +
>>      switch (req.cmd) {
>>      case 6: /* ACMD6:  SET_BUS_WIDTH */
>>          if (sd->spi) {
>>
> 
>