On 15 February 2018 at 22:05, Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
> put the function in sdmmc-common.c since we will reuse it in hw/sd/core.c
>
As a general note, your commit messages are generally a bit
shorter than I'd prefer. This is to some extent a personal
style question, but if you're writing lots of patches where
the only commit message is the subject-line summary then I
think that it's worth being a bit more descriptive.
Also, the "body" part of the commit message should really
be able to stand alone as a description, rather than relying
on the subject line to make sense.
> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org>
> ---
> hw/sd/sdmmc-internal.h | 3 +++
> hw/sd/sd.c | 13 +++++----
> hw/sd/sdmmc-common.c | 72
> ++++++++++++++++++++++++++++++++++++++++++++++++++
> hw/sd/Makefile.objs | 2 +-
> hw/sd/trace-events | 8 +++---
> 5 files changed, 88 insertions(+), 10 deletions(-)
> create mode 100644 hw/sd/sdmmc-common.c
>
> diff --git a/hw/sd/sdmmc-internal.h b/hw/sd/sdmmc-internal.h
> index 0e96cb0081..02b730089b 100644
> --- a/hw/sd/sdmmc-internal.h
> +++ b/hw/sd/sdmmc-internal.h
> @@ -12,4 +12,7 @@
>
> #define SDMMC_CMD_MAX 64
>
> +const char *sd_cmd_name(uint8_t cmd);
> +const char *sd_acmd_name(uint8_t cmd);
Can we have doc comments for new function prototypes in header files,
please?
> diff --git a/hw/sd/sdmmc-common.c b/hw/sd/sdmmc-common.c
> new file mode 100644
> index 0000000000..1d0198b1ad
> --- /dev/null
> +++ b/hw/sd/sdmmc-common.c
> @@ -0,0 +1,72 @@
> +/*
> + * SD/MMC cards common helpers
> + *
> + * Copyright (c) 2018 Philippe Mathieu-Daudé <f4...@amsat.org>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + * SPDX-License-Identifier: GPL-2.0-or-later
Ooh, a SPDX identifier. The project doesn't actually use these
at the moment, but it doesn't hurt to have one here I guess.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "sdmmc-internal.h"
> +
> +const char *sd_cmd_name(uint8_t cmd)
> +{
> + static const char *cmd_abbrev[SDMMC_CMD_MAX] = {
> + [0] = "GO_IDLE_STATE",
> + [2] = "ALL_SEND_CID", [3] = "SEND_RELATIVE_ADDR",
> + [4] = "SET_DSR", [5] = "IO_SEND_OP_COND",
> + [6] = "SWITCH_FUNC", [7] = "SELECT/DESELECT_CARD",
> + [8] = "SEND_IF_COND", [9] = "SEND_CSD",
> + [10] = "SEND_CID", [11] = "VOLTAGE_SWITCH",
> + [12] = "STOP_TRANSMISSION", [13] = "SEND_STATUS",
> + [15] = "GO_INACTIVE_STATE",
> + [16] = "SET_BLOCKLEN", [17] = "READ_SINGLE_BLOCK",
> + [18] = "READ_MULTIPLE_BLOCK", [19] = "SEND_TUNING_BLOCK",
> + [20] = "SPEED_CLASS_CONTROL", [21] = "DPS_spec",
> + [23] = "SET_BLOCK_COUNT",
> + [24] = "WRITE_BLOCK", [25] = "WRITE_MULTIPLE_BLOCK",
> + [26] = "MANUF_RSVD", [27] = "PROGRAM_CSD",
> + [28] = "SET_WRITE_PROT", [29] = "CLR_WRITE_PROT",
> + [30] = "SEND_WRITE_PROT",
> + [32] = "ERASE_WR_BLK_START", [33] = "ERASE_WR_BLK_END",
> + [34] = "SW_FUNC_RSVD", [35] = "SW_FUNC_RSVD",
> + [36] = "SW_FUNC_RSVD", [37] = "SW_FUNC_RSVD",
> + [38] = "ERASE",
> + [40] = "DPS_spec",
> + [42] = "LOCK_UNLOCK", [43] = "Q_MANAGEMENT",
> + [44] = "Q_TASK_INFO_A", [45] = "Q_TASK_INFO_B",
> + [46] = "Q_RD_TASK", [47] = "Q_WR_TASK",
> + [48] = "READ_EXTR_SINGLE", [49] = "WRITE_EXTR_SINGLE",
> + [50] = "SW_FUNC_RSVD", /* FIXME */
What's this FIXME for? We should either fix whatever it is, or if
that's not practical the comment needs to describe the problem
so that a future reader of the code knows what it means and might
be able to fix it.
> + [52] = "IO_RW_DIRECT", [53] = "IO_RW_EXTENDED",
> + [54] = "SDIO_RSVD", [55] = "APP_CMD",
> + [56] = "GEN_CMD", [57] = "SW_FUNC_RSVD",
> + [58] = "READ_EXTR_MULTI", [59] = "WRITE_EXTR_MULTI",
> + [60] = "MANUF_RSVD", [61] = "MANUF_RSVD",
> + [62] = "MANUF_RSVD", [63] = "MANUF_RSVD",
> + };
> + return cmd_abbrev[cmd] ? cmd_abbrev[cmd] : "UNKNOWN_CMD";
> +}
> +
> +const char *sd_acmd_name(uint8_t cmd)
> +{
> + static const char *acmd_abbrev[SDMMC_CMD_MAX] = {
> + [6] = "SET_BUS_WIDTH",
> + [13] = "SD_STATUS",
> + [14] = "DPS_spec", [15] = "DPS_spec",
> + [16] = "DPS_spec",
> + [18] = "SECU_spec",
> + [22] = "SEND_NUM_WR_BLOCKS", [23] = "SET_WR_BLK_ERASE_COUNT",
> + [41] = "SD_SEND_OP_COND",
> + [42] = "SET_CLR_CARD_DETECT",
> + [51] = "SEND_SCR",
> + [52] = "SECU_spec", [53] = "SECU_spec",
> + [54] = "SECU_spec",
> + [56] = "SECU_spec", [57] = "SECU_spec",
> + [58] = "SECU_spec", [59] = "SECU_spec",
> + };
> +
> + return acmd_abbrev[cmd] ? acmd_abbrev[cmd] : "UNKNOWN_ACMD";
> +}
thanks
-- PMM
