Re: [PATCH] mmc: Drop unused mmc_send_tuning() cmd_error parameter

[Marek Vasut]

On 2/20/24 11:50, Paul Barker wrote:
On 20/02/2024 08:36, Marek Vasut wrote:
The cmd_error parameter is not used, remove it.
  [snip]
diff --git a/drivers/mmc/mtk-sd.c b/drivers/mmc/mtk-sd.c
index 5a0c61daed5..296aaee7331 100644
--- a/drivers/mmc/mtk-sd.c
+++ b/drivers/mmc/mtk-sd.c
@@ -1131,7 +1131,7 @@ static int hs400_tune_response(struct udevice *dev, u32 
opcode)
                                i << PAD_CMD_TUNE_RX_DLY3_S);
  
  		for (j = 0; j < 3; j++) {
-                       mmc_send_tuning(mmc, opcode, &cmd_err);
+                       cmd_err = mmc_send_tuning(mmc, opcode);
                        if (!cmd_err) {
                                cmd_delay |= (1 << i);
                        } else {
@@ -1181,7 +1181,7 @@ static int msdc_tune_response(struct udevice *dev, u32 
opcode)
                                i << MSDC_PAD_TUNE_CMDRDLY_S);
  
  		for (j = 0; j < 3; j++) {
-                       mmc_send_tuning(mmc, opcode, &cmd_err);
+                       cmd_err = mmc_send_tuning(mmc, opcode);
                        if (!cmd_err) {
                                rise_delay |= (1 << i);
                        } else {
@@ -1203,7 +1203,7 @@ static int msdc_tune_response(struct udevice *dev, u32 
opcode)
                                i << MSDC_PAD_TUNE_CMDRDLY_S);
  
  		for (j = 0; j < 3; j++) {
-                       mmc_send_tuning(mmc, opcode, &cmd_err);
+                       cmd_err = mmc_send_tuning(mmc, opcode);
                        if (!cmd_err) {
                                fall_delay |= (1 << i);
                        } else {
@@ -1238,7 +1238,7 @@ skip_fall:
                clrsetbits_le32(tune_reg, MSDC_PAD_TUNE_CMDRRDLY_M,
                                i << MSDC_PAD_TUNE_CMDRRDLY_S);
  
-		mmc_send_tuning(mmc, opcode, &cmd_err);
+               cmd_err = mmc_send_tuning(mmc, opcode);
                if (!cmd_err)
                        internal_delay |= (1 << i);
        }
@@ -1264,7 +1264,6 @@ static int msdc_tune_data(struct udevice *dev, u32 opcode)
        struct msdc_delay_phase final_rise_delay, final_fall_delay = { 0, };
        u8 final_delay, final_maxlen;
        void __iomem *tune_reg = &host->base->pad_tune;
-       int cmd_err;
        int i, ret;
  
  	if (host->dev_comp->pad_tune0)
@@ -1277,10 +1276,10 @@ static int msdc_tune_data(struct udevice *dev, u32 
opcode)
                clrsetbits_le32(tune_reg, MSDC_PAD_TUNE_DATRRDLY_M,
                                i << MSDC_PAD_TUNE_DATRRDLY_S);
  
-		ret = mmc_send_tuning(mmc, opcode, &cmd_err);
+               ret = mmc_send_tuning(mmc, opcode);
                if (!ret) {
                        rise_delay |= (1 << i);
-               } else if (cmd_err) {
+               } else {
                        /* in this case, retune response is needed */
                        ret = msdc_tune_response(dev, opcode);
                        if (ret)
@@ -1300,10 +1299,10 @@ static int msdc_tune_data(struct udevice *dev, u32 
opcode)
                clrsetbits_le32(tune_reg, MSDC_PAD_TUNE_DATRRDLY_M,
                                i << MSDC_PAD_TUNE_DATRRDLY_S);
  
-		ret = mmc_send_tuning(mmc, opcode, &cmd_err);
+               ret = mmc_send_tuning(mmc, opcode);
                if (!ret) {
                        fall_delay |= (1 << i);
-               } else if (cmd_err) {
+               } else {
                        /* in this case, retune response is needed */
                        ret = msdc_tune_response(dev, opcode);
                        if (ret)

This driver (mtk-sd.c) seems to be the only one that really uses the
`cmd_error` parameter.

Looking at the implementation of mmc_send_tuning() in Linux, this
parameter is used so that a caller can differentiate between a command
error and a data error. I don't know enough details about MMC to
understand the distinction, but I assume there is some reason for this.
So I wonder if the mtk-sd driver will still work if those error paths
are taken for data errors and not just command errors. Has this change
been tested on some board which uses this driver?

Not by me, so far this driver used uninitialized error value and assumed 
it was initialized as far as I can tell, so it is likely already broken.