| Message ID | 20220321115059.21803-9-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: improve API to make clear {h|s}w_reset is for cards | expand |
On Mon, 21 Mar 2022 at 12:51, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > To make it unambiguous that bus_ops->hw_reset() is for cards and not for > controllers, we a) add 'card' to the function name and b) make the > function argument mmc_card instead of mmc_host. All users are converted, > too. Again b) is sufficient in my opinion. All bus_ops are for cards, while host_ops are for hosts. Also, there may be some corner cases where b) can't be done, like the ->remove() bus_ops for example. In that case, we either have to make more re-structuring of the code of simply live with that there may be some special cases. [...] Kind regards Uffe
> > To make it unambiguous that bus_ops->hw_reset() is for cards and not for > > controllers, we a) add 'card' to the function name and b) make the > > function argument mmc_card instead of mmc_host. All users are converted, > > too. > > Again b) is sufficient in my opinion. All bus_ops are for cards, while > host_ops are for hosts. Okay, this argument I buy right away. > Also, there may be some corner cases where b) can't be done, like the > ->remove() bus_ops for example. In that case, we either have to make > more re-structuring of the code of simply live with that there may be > some special cases. With the above argument, I could even imaging to simply drop this patch? That keeps 'host' consistently as the default argument? All given that 'bus_ops' are for cards anyway.
On Wed, 6 Apr 2022 at 10:02, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > > > > To make it unambiguous that bus_ops->hw_reset() is for cards and not for > > > controllers, we a) add 'card' to the function name and b) make the > > > function argument mmc_card instead of mmc_host. All users are converted, > > > too. > > > > Again b) is sufficient in my opinion. All bus_ops are for cards, while > > host_ops are for hosts. > > Okay, this argument I buy right away. > > > Also, there may be some corner cases where b) can't be done, like the > > ->remove() bus_ops for example. In that case, we either have to make > > more re-structuring of the code of simply live with that there may be > > some special cases. > > With the above argument, I could even imaging to simply drop this patch? > That keeps 'host' consistently as the default argument? All given that > 'bus_ops' are for cards anyway. I have no strong opinion around this. Perhaps one simply needs to make a patch to convert them (most of them) to take a "card" as an in-parameter to really see if that improves the understanding of code. Kind regards Uffe
> Perhaps one simply needs to make a patch to convert them (most of > them) to take a "card" as an in-parameter to really see if that > improves the understanding of code. Might be worth it, but looks like a seperate patch to me, focused on bus_ops, not on {h|s}w_reset. I might try it next.
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 3498b341acaf..1d874b064202 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -969,7 +969,7 @@ void mmc_set_bus_width(struct mmc_host *host, unsigned int width) } /* - * Set initial state after a power cycle or a hw_reset. + * Set initial state after a power cycle or a card_hw_reset. */ void mmc_set_initial_state(struct mmc_host *host) { @@ -2009,7 +2009,7 @@ int mmc_card_hw_reset(struct mmc_card *card) struct mmc_host *host = card->host; int ret; - ret = host->bus_ops->hw_reset(host); + ret = host->bus_ops->card_hw_reset(card); if (ret < 0) pr_warn("%s: tried to HW reset card, got error %d\n", mmc_hostname(host), ret); diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h index f5f3f623ea49..c54270a6d457 100644 --- a/drivers/mmc/core/core.h +++ b/drivers/mmc/core/core.h @@ -27,7 +27,7 @@ struct mmc_bus_ops { int (*runtime_resume)(struct mmc_host *); int (*alive)(struct mmc_host *); int (*shutdown)(struct mmc_host *); - int (*hw_reset)(struct mmc_host *); + int (*card_hw_reset)(struct mmc_card *); int (*sw_reset)(struct mmc_host *); bool (*cache_enabled)(struct mmc_host *); int (*flush_cache)(struct mmc_host *); diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index e7ea45386c22..1857f398298b 100644 --- a/drivers/mmc/core/mmc.c +++ b/drivers/mmc/core/mmc.c @@ -2215,9 +2215,9 @@ static int mmc_can_reset(struct mmc_card *card) return 1; } -static int _mmc_hw_reset(struct mmc_host *host) +static int _mmc_card_hw_reset(struct mmc_card *card) { - struct mmc_card *card = host->card; + struct mmc_host *host = card->host; /* * In the case of recovery, we can't expect flushing the cache to work @@ -2249,7 +2249,7 @@ static const struct mmc_bus_ops mmc_ops = { .runtime_resume = mmc_runtime_resume, .alive = mmc_alive, .shutdown = mmc_shutdown, - .hw_reset = _mmc_hw_reset, + .card_hw_reset = _mmc_card_hw_reset, .cache_enabled = _mmc_cache_enabled, .flush_cache = _mmc_flush_cache, }; diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c index 68df6b2f49cc..c4a08ee8cdcc 100644 --- a/drivers/mmc/core/sd.c +++ b/drivers/mmc/core/sd.c @@ -1784,10 +1784,10 @@ static int mmc_sd_runtime_resume(struct mmc_host *host) return 0; } -static int mmc_sd_hw_reset(struct mmc_host *host) +static int mmc_sd_card_hw_reset(struct mmc_card *card) { - mmc_power_cycle(host, host->card->ocr); - return mmc_sd_init_card(host, host->card->ocr, host->card); + mmc_power_cycle(card->host, card->ocr); + return mmc_sd_init_card(card->host, card->ocr, card); } static const struct mmc_bus_ops mmc_sd_ops = { @@ -1799,7 +1799,7 @@ static const struct mmc_bus_ops mmc_sd_ops = { .resume = mmc_sd_resume, .alive = mmc_sd_alive, .shutdown = mmc_sd_suspend, - .hw_reset = mmc_sd_hw_reset, + .card_hw_reset = mmc_sd_card_hw_reset, .cache_enabled = sd_cache_enabled, .flush_cache = sd_flush_cache, }; diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c index 25799accf8a0..30242faf703e 100644 --- a/drivers/mmc/core/sdio.c +++ b/drivers/mmc/core/sdio.c @@ -1128,9 +1128,9 @@ static int mmc_sdio_runtime_resume(struct mmc_host *host) * Returns 0 if the HW reset was executed synchronously, returns 1 if the HW * reset was asynchronously scheduled, else a negative error code. */ -static int mmc_sdio_hw_reset(struct mmc_host *host) +static int mmc_sdio_card_hw_reset(struct mmc_card *card) { - struct mmc_card *card = host->card; + struct mmc_host *host = card->host; /* * In case the card is shared among multiple func drivers, reset the @@ -1175,7 +1175,7 @@ static const struct mmc_bus_ops mmc_sdio_ops = { .runtime_suspend = mmc_sdio_runtime_suspend, .runtime_resume = mmc_sdio_runtime_resume, .alive = mmc_sdio_alive, - .hw_reset = mmc_sdio_hw_reset, + .card_hw_reset = mmc_sdio_card_hw_reset, .sw_reset = mmc_sdio_sw_reset, };
To make it unambiguous that bus_ops->hw_reset() is for cards and not for controllers, we a) add 'card' to the function name and b) make the function argument mmc_card instead of mmc_host. All users are converted, too. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/mmc/core/core.c | 4 ++-- drivers/mmc/core/core.h | 2 +- drivers/mmc/core/mmc.c | 6 +++--- drivers/mmc/core/sd.c | 8 ++++---- drivers/mmc/core/sdio.c | 6 +++--- 5 files changed, 13 insertions(+), 13 deletions(-)