On 20/02/2024 11:26, Marek Vasut wrote:
> 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.+To: Ryder Lee, Weijie Gao, Chunfeng Yun +Cc: [email protected] Do you have any input as ARM MEDIATEK maintainers? -- Paul Barker
OpenPGP_0x27F4B3459F002257.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
