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

  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(-)
  

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
>
  

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