MIME-Version: 1.0
In-Reply-To: <20211205191009.32454-1-nishadkamdar@gmail.com>
References: <20211205191009.32454-1-nishadkamdar@gmail.com>
From:   Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.9.1
Date:   Mon, 6 Dec 2021 12:44:14 -0800
Message-ID: <CAE-0n515qDr95A5PgbhpYer+UT053VzbaDKrxe6zjVgAQwpcEw@mail.gmail.com>
Subject: Re: [PATCH] mmc: core: Add support for the eMMC RTC feature in mmc_ops
To:     Adrian Hunter <adrian.hunter@intel.com>,
        Avri Altman <avri.altman@wdc.com>,
        Bean Huo <beanhuo@micron.com>,
        Huijin Park <huijin.park@samsung.com>,
        Jens Axboe <axboe@kernel.dk>,
        Nishad Kamdar <nishadkamdar@gmail.com>,
        Shawn Lin <shawn.lin@rock-chips.com>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        Wolfram Sang <wsa+renesas@sang-engineering.com>,
        Yue Hu <huyue2@yulong.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Quoting Nishad Kamdar (2021-12-05 11:10:08)
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index d63d1c735335..490372341b3b 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -1063,3 +1063,62 @@ int mmc_sanitize(struct mmc_card *card, unsigned int timeout_ms)
>         return err;
>  }
>  EXPORT_SYMBOL_GPL(mmc_sanitize);
> +
> +int mmc_set_time(struct mmc_card *card, struct mmc_host *host,
> +                u8 rtc_info_type, u64 seconds)
> +{
> +       struct mmc_request mrq = {};
> +       struct mmc_command cmd = {};
> +       struct mmc_data data = {};
> +       struct scatterlist sg;
> +       int err = 0;
> +       u8 *data_buf;
> +
> +       data_buf = kzalloc(512, GFP_KERNEL);

Use some #define for 512 because it's used three times in here?

> +       if (!data_buf)
> +               return -ENOMEM;
> +
> +       if (rtc_info_type == 0x01 || rtc_info_type == 0x02 ||
> +           rtc_info_type == 0x03) {
> +               data_buf[0] = 0x01;
> +               data_buf[1] = rtc_info_type;
> +               memcpy(&data_buf[2], &seconds, sizeof(u64));

Use sizeof(seconds) instead?

> +       } else {
> +               pr_err("%s: invalid rtc_info_type %d\n",
> +                      mmc_hostname(host), rtc_info_type);
> +               kfree(data_buf);
> +               return -EINVAL;
> +       }
> +
> +       mrq.cmd = &cmd;
> +       mrq.data = &data;
> +
> +       cmd.opcode = MMC_SET_TIME;
> +       cmd.arg = 0;
> +       cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> +
> +       data.blksz = 512;
> +       data.blocks = 1;
> +       data.flags = MMC_DATA_WRITE;
> +       data.sg = &sg;
> +       data.sg_len = 1;
> +       sg_init_one(&sg, data_buf, 512);
> +
> +       mmc_set_data_timeout(&data, card);
> +
> +       mmc_wait_for_req(host, &mrq);
> +
> +       if (cmd.error) {
> +               err = cmd.error;
> +               goto out;
> +       }
> +
> +       if (data.error) {
> +               err = data.error;
> +               goto out;
> +       }

Why not

	if (cmd.error) {
		err = cmd.error;
	} else if (data.error) {
		err = data.error;
	} else {
		err = 0;
	}

> +out:

And then drop out: and the assignment of err to 0 up above?


> +       kfree(data_buf);
> +       return err;
> +}
> +EXPORT_SYMBOL_GPL(mmc_set_time);