diff mbox series

[V8,01/23] mmc: core: Cleanup printing of speed mode at card insertion

Message ID 20230621100151.6329-2-victorshihgli@gmail.com (mailing list archive)
State New, archived
Headers show
Series Add support UHS-II for GL9755 | expand

Commit Message

Victor Shih June 21, 2023, 10:01 a.m. UTC 
From: Victor Shih <victor.shih@genesyslogic.com.tw>

The current print of the bus speed mode in mmc_add_card() has grown over
the years and is now difficult to parse. Let's clean up the code and also
take the opportunity to properly announce "DDR" for eMMCs as
"high speed DDR", which is according to the eMMC spec.

Updates in V8:
 - Modify commit message.

Updates in V7:
 - Remove unnecessary parentheses.

Updates in V6:
 - Adjust the position of matching brackets.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Victor Shih <victor.shih@genesyslogic.com.tw>
---
 drivers/mmc/core/bus.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

Comments

Ulf Hansson June 21, 2023, 1:44 p.m. UTC | #1 
On Wed, 21 Jun 2023 at 12:02, Victor Shih <victorshihgli@gmail.com> wrote:
>
> From: Victor Shih <victor.shih@genesyslogic.com.tw>

Please do not claim authorship of patches that haven't been authored
by you. Of course, there is a balance, if you need to make bigger
modifications, then you deserve to claim the authorship, but that
isn't the case here I think.

This applies to a couple of more patches in the series, I will not
comment on them in this regard, but leave that to you to look over at
the next submission.

>
> The current print of the bus speed mode in mmc_add_card() has grown over
> the years and is now difficult to parse. Let's clean up the code and also
> take the opportunity to properly announce "DDR" for eMMCs as
> "high speed DDR", which is according to the eMMC spec.
>
> Updates in V8:
>  - Modify commit message.
>
> Updates in V7:
>  - Remove unnecessary parentheses.
>
> Updates in V6:
>  - Adjust the position of matching brackets.

I appreciate the version history per patch. However, this doesn't
belong in the commit message.

Instead you have to manually edit each formatted patch to add this,
exactly where see below.

>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Victor Shih <victor.shih@genesyslogic.com.tw>
> ---

After this line you can add comments and version history for the
patch. In this way, it will not be a part of the commit message when
applying.

Complete the section by adding three new dashes and a newline - this
keeps the patch format correct.

---

>  drivers/mmc/core/bus.c | 36 ++++++++++++++++++++----------------
>  1 file changed, 20 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> index 2c3074a605fc..cf32cf135781 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -299,6 +299,7 @@ int mmc_add_card(struct mmc_card *card)
>  {
>         int ret;
>         const char *type;
> +       const char *speed_mode = "";
>         const char *uhs_bus_speed_mode = "";
>         static const char *const uhs_speeds[] = {
>                 [UHS_SDR12_BUS_SPEED] = "SDR12 ",
> @@ -337,27 +338,30 @@ int mmc_add_card(struct mmc_card *card)
>                 break;
>         }
>
> +       if (mmc_card_hs(card))
> +               speed_mode = "high speed ";
> +       else if (mmc_card_uhs(card))
> +               speed_mode = "ultra high speed ";
> +       else if (mmc_card_ddr52(card))
> +               speed_mode = "high speed DDR ";
> +       else if (mmc_card_hs200(card))
> +               speed_mode = "HS200 ";
> +       else if (mmc_card_hs400es(card))
> +               speed_mode = "HS400 Enhanced strobe ";
> +       else if (mmc_card_hs400(card))
> +               speed_mode = "HS400 ";
> +
>         if (mmc_card_uhs(card) &&
>                 (card->sd_bus_speed < ARRAY_SIZE(uhs_speeds)))
>                 uhs_bus_speed_mode = uhs_speeds[card->sd_bus_speed];
>
> -       if (mmc_host_is_spi(card->host)) {
> -               pr_info("%s: new %s%s%s card on SPI\n",
> -                       mmc_hostname(card->host),
> -                       mmc_card_hs(card) ? "high speed " : "",
> -                       mmc_card_ddr52(card) ? "DDR " : "",
> -                       type);
> -       } else {
> -               pr_info("%s: new %s%s%s%s%s%s card at address %04x\n",
> -                       mmc_hostname(card->host),
> -                       mmc_card_uhs(card) ? "ultra high speed " :
> -                       (mmc_card_hs(card) ? "high speed " : ""),
> -                       mmc_card_hs400(card) ? "HS400 " :
> -                       (mmc_card_hs200(card) ? "HS200 " : ""),
> -                       mmc_card_hs400es(card) ? "Enhanced strobe " : "",
> -                       mmc_card_ddr52(card) ? "DDR " : "",
> +       if (mmc_host_is_spi(card->host))
> +               pr_info("%s: new %s%s card on SPI\n",
> +                       mmc_hostname(card->host), speed_mode, type);
> +       else
> +               pr_info("%s: new %s%s%s card at address %04x\n",
> +                       mmc_hostname(card->host), speed_mode,
>                         uhs_bus_speed_mode, type, card->rca);
> -       }
>
>         mmc_add_card_debugfs(card);
>         card->dev.of_node = mmc_of_find_child_device(card->host, 0);
> --
> 2.25.1
>
Victor Shih June 29, 2023, 10:24 a.m. UTC | #2 
Hi, Ulf

On Wed, Jun 21, 2023 at 9:44 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 21 Jun 2023 at 12:02, Victor Shih <victorshihgli@gmail.com> wrote:
> >
> > From: Victor Shih <victor.shih@genesyslogic.com.tw>
>
> Please do not claim authorship of patches that haven't been authored
> by you. Of course, there is a balance, if you need to make bigger
> modifications, then you deserve to claim the authorship, but that
> isn't the case here I think.
>
> This applies to a couple of more patches in the series, I will not
> comment on them in this regard, but leave that to you to look over at
> the next submission.
>

I have confirmed this issue in several other patches in this series,
and I will fix it in the next version of the patch.

> >
> > The current print of the bus speed mode in mmc_add_card() has grown over
> > the years and is now difficult to parse. Let's clean up the code and also
> > take the opportunity to properly announce "DDR" for eMMCs as
> > "high speed DDR", which is according to the eMMC spec.
> >
> > Updates in V8:
> >  - Modify commit message.
> >
> > Updates in V7:
> >  - Remove unnecessary parentheses.
> >
> > Updates in V6:
> >  - Adjust the position of matching brackets.
>
> I appreciate the version history per patch. However, this doesn't
> belong in the commit message.
>
> Instead you have to manually edit each formatted patch to add this,
> exactly where see below.
>
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Victor Shih <victor.shih@genesyslogic.com.tw>
> > ---
>
> After this line you can add comments and version history for the
> patch. In this way, it will not be a part of the commit message when
> applying.
>
> Complete the section by adding three new dashes and a newline - this
> keeps the patch format correct.
>
> ---
>

I will implement this in the next version of the patch.

> >  drivers/mmc/core/bus.c | 36 ++++++++++++++++++++----------------
> >  1 file changed, 20 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> > index 2c3074a605fc..cf32cf135781 100644
> > --- a/drivers/mmc/core/bus.c
> > +++ b/drivers/mmc/core/bus.c
> > @@ -299,6 +299,7 @@ int mmc_add_card(struct mmc_card *card)
> >  {
> >         int ret;
> >         const char *type;
> > +       const char *speed_mode = "";
> >         const char *uhs_bus_speed_mode = "";
> >         static const char *const uhs_speeds[] = {
> >                 [UHS_SDR12_BUS_SPEED] = "SDR12 ",
> > @@ -337,27 +338,30 @@ int mmc_add_card(struct mmc_card *card)
> >                 break;
> >         }
> >
> > +       if (mmc_card_hs(card))
> > +               speed_mode = "high speed ";
> > +       else if (mmc_card_uhs(card))
> > +               speed_mode = "ultra high speed ";
> > +       else if (mmc_card_ddr52(card))
> > +               speed_mode = "high speed DDR ";
> > +       else if (mmc_card_hs200(card))
> > +               speed_mode = "HS200 ";
> > +       else if (mmc_card_hs400es(card))
> > +               speed_mode = "HS400 Enhanced strobe ";
> > +       else if (mmc_card_hs400(card))
> > +               speed_mode = "HS400 ";
> > +
> >         if (mmc_card_uhs(card) &&
> >                 (card->sd_bus_speed < ARRAY_SIZE(uhs_speeds)))
> >                 uhs_bus_speed_mode = uhs_speeds[card->sd_bus_speed];
> >
> > -       if (mmc_host_is_spi(card->host)) {
> > -               pr_info("%s: new %s%s%s card on SPI\n",
> > -                       mmc_hostname(card->host),
> > -                       mmc_card_hs(card) ? "high speed " : "",
> > -                       mmc_card_ddr52(card) ? "DDR " : "",
> > -                       type);
> > -       } else {
> > -               pr_info("%s: new %s%s%s%s%s%s card at address %04x\n",
> > -                       mmc_hostname(card->host),
> > -                       mmc_card_uhs(card) ? "ultra high speed " :
> > -                       (mmc_card_hs(card) ? "high speed " : ""),
> > -                       mmc_card_hs400(card) ? "HS400 " :
> > -                       (mmc_card_hs200(card) ? "HS200 " : ""),
> > -                       mmc_card_hs400es(card) ? "Enhanced strobe " : "",
> > -                       mmc_card_ddr52(card) ? "DDR " : "",
> > +       if (mmc_host_is_spi(card->host))
> > +               pr_info("%s: new %s%s card on SPI\n",
> > +                       mmc_hostname(card->host), speed_mode, type);
> > +       else
> > +               pr_info("%s: new %s%s%s card at address %04x\n",
> > +                       mmc_hostname(card->host), speed_mode,
> >                         uhs_bus_speed_mode, type, card->rca);
> > -       }
> >
> >         mmc_add_card_debugfs(card);
> >         card->dev.of_node = mmc_of_find_child_device(card->host, 0);
> > --
> > 2.25.1
> >

Thanks, Victor Shih
diff mbox series

Patch

diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index 2c3074a605fc..cf32cf135781 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -299,6 +299,7 @@  int mmc_add_card(struct mmc_card *card)
 {
 	int ret;
 	const char *type;
+	const char *speed_mode = "";
 	const char *uhs_bus_speed_mode = "";
 	static const char *const uhs_speeds[] = {
 		[UHS_SDR12_BUS_SPEED] = "SDR12 ",
@@ -337,27 +338,30 @@  int mmc_add_card(struct mmc_card *card)
 		break;
 	}
 
+	if (mmc_card_hs(card))
+		speed_mode = "high speed ";
+	else if (mmc_card_uhs(card))
+		speed_mode = "ultra high speed ";
+	else if	(mmc_card_ddr52(card))
+		speed_mode = "high speed DDR ";
+	else if (mmc_card_hs200(card))
+		speed_mode = "HS200 ";
+	else if (mmc_card_hs400es(card))
+		speed_mode = "HS400 Enhanced strobe ";
+	else if (mmc_card_hs400(card))
+		speed_mode = "HS400 ";
+
 	if (mmc_card_uhs(card) &&
 		(card->sd_bus_speed < ARRAY_SIZE(uhs_speeds)))
 		uhs_bus_speed_mode = uhs_speeds[card->sd_bus_speed];
 
-	if (mmc_host_is_spi(card->host)) {
-		pr_info("%s: new %s%s%s card on SPI\n",
-			mmc_hostname(card->host),
-			mmc_card_hs(card) ? "high speed " : "",
-			mmc_card_ddr52(card) ? "DDR " : "",
-			type);
-	} else {
-		pr_info("%s: new %s%s%s%s%s%s card at address %04x\n",
-			mmc_hostname(card->host),
-			mmc_card_uhs(card) ? "ultra high speed " :
-			(mmc_card_hs(card) ? "high speed " : ""),
-			mmc_card_hs400(card) ? "HS400 " :
-			(mmc_card_hs200(card) ? "HS200 " : ""),
-			mmc_card_hs400es(card) ? "Enhanced strobe " : "",
-			mmc_card_ddr52(card) ? "DDR " : "",
+	if (mmc_host_is_spi(card->host))
+		pr_info("%s: new %s%s card on SPI\n",
+			mmc_hostname(card->host), speed_mode, type);
+	else
+		pr_info("%s: new %s%s%s card at address %04x\n",
+			mmc_hostname(card->host), speed_mode,
 			uhs_bus_speed_mode, type, card->rca);
-	}
 
 	mmc_add_card_debugfs(card);
 	card->dev.of_node = mmc_of_find_child_device(card->host, 0);