| Message ID | 20200916090121.2350-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: core: document mmc_hw_reset | expand |
On Wed, 16 Sep 2020 at 11:01, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > Add documentation for mmc_hw_reset to make sure the intended use case is > clear. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > While working on this, I get the feeling this function should be renamed > to 'mmc_card_reset' or something similar. 'hw' is still confusing > because it could easily be the host controller, too. I volunteer to > prepare a patch if we can agree on a better name. You have a point. Although we also have mmc_sw_reset(). Another thing that I would like to change is to make both of these functions take a struct mmc_card* as in-parameter, rather than the current struct mmc_host*. Not sure that it would completely solve the confusion, but at least it would be a little more clear. > > drivers/mmc/core/core.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index 8ccae6452b9c..a9ad72a18d1b 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -2063,6 +2063,16 @@ static void mmc_hw_reset_for_init(struct mmc_host *host) > host->ops->hw_reset(host); > } > > +/** > + * mmc_hw_reset - reset the card > + * @host: MMC host to which the card is attached > + * > + * Reset the remote card. This function is only for upper layers, like the Perhaps make it clear that it's a full (or hw) reset, not just a reset (as it could also be a soft reset). Moreover, I think you can skip "remote". > + * block layer or card drivers. You cannot use it in host drivers (struct > + * mmc_card might be gone then). > + * > + * Return: 0 on success, -errno on failure > + */ > int mmc_hw_reset(struct mmc_host *host) > { > int ret; > -- > 2.28.0 > Kind regards Uffe
On Wed, Sep 16, 2020 at 11:47:10AM +0200, Ulf Hansson wrote: > On Wed, 16 Sep 2020 at 11:01, Wolfram Sang > <wsa+renesas@sang-engineering.com> wrote: > > > > Add documentation for mmc_hw_reset to make sure the intended use case is > > clear. > > > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > --- > > > > While working on this, I get the feeling this function should be renamed > > to 'mmc_card_reset' or something similar. 'hw' is still confusing > > because it could easily be the host controller, too. I volunteer to > > prepare a patch if we can agree on a better name. > > You have a point. Although we also have mmc_sw_reset(). Ah, I didn't know that. Though, mmc_card_{hw|sw}_reset() sounds still way better to me. > Another thing that I would like to change is to make both of these > functions take a struct mmc_card* as in-parameter, rather than the > current struct mmc_host*. > > Not sure that it would completely solve the confusion, but at least it > would be a little more clear. Maybe if we do both, it will be really clear? :) > > +/** > > + * mmc_hw_reset - reset the card > > + * @host: MMC host to which the card is attached > > + * > > + * Reset the remote card. This function is only for upper layers, like the > > Perhaps make it clear that it's a full (or hw) reset, not just a reset > (as it could also be a soft reset). Moreover, I think you can skip > "remote". OK, will send V2 in a minute.
On Fri, 18 Sep 2020 at 23:48, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > On Wed, Sep 16, 2020 at 11:47:10AM +0200, Ulf Hansson wrote: > > On Wed, 16 Sep 2020 at 11:01, Wolfram Sang > > <wsa+renesas@sang-engineering.com> wrote: > > > > > > Add documentation for mmc_hw_reset to make sure the intended use case is > > > clear. > > > > > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > > --- > > > > > > While working on this, I get the feeling this function should be renamed > > > to 'mmc_card_reset' or something similar. 'hw' is still confusing > > > because it could easily be the host controller, too. I volunteer to > > > prepare a patch if we can agree on a better name. > > > > You have a point. Although we also have mmc_sw_reset(). > > Ah, I didn't know that. Though, mmc_card_{hw|sw}_reset() sounds still > way better to me. I fully agree. > > > Another thing that I would like to change is to make both of these > > functions take a struct mmc_card* as in-parameter, rather than the > > current struct mmc_host*. > > > > Not sure that it would completely solve the confusion, but at least it > > would be a little more clear. > > Maybe if we do both, it will be really clear? :) Makes perfect sense to me. > > > > +/** > > > + * mmc_hw_reset - reset the card > > > + * @host: MMC host to which the card is attached > > > + * > > > + * Reset the remote card. This function is only for upper layers, like the > > > > Perhaps make it clear that it's a full (or hw) reset, not just a reset > > (as it could also be a soft reset). Moreover, I think you can skip > > "remote". > > OK, will send V2 in a minute. > I have applied that one. Let's deal with potential additional improvements on top. Kind regards Uffe
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 8ccae6452b9c..a9ad72a18d1b 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -2063,6 +2063,16 @@ static void mmc_hw_reset_for_init(struct mmc_host *host) host->ops->hw_reset(host); } +/** + * mmc_hw_reset - reset the card + * @host: MMC host to which the card is attached + * + * Reset the remote card. This function is only for upper layers, like the + * block layer or card drivers. You cannot use it in host drivers (struct + * mmc_card might be gone then). + * + * Return: 0 on success, -errno on failure + */ int mmc_hw_reset(struct mmc_host *host) { int ret;
Add documentation for mmc_hw_reset to make sure the intended use case is clear. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- While working on this, I get the feeling this function should be renamed to 'mmc_card_reset' or something similar. 'hw' is still confusing because it could easily be the host controller, too. I volunteer to prepare a patch if we can agree on a better name. drivers/mmc/core/core.c | 10 ++++++++++ 1 file changed, 10 insertions(+)