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

[Victor Shih]

Hi, Ulf

On Wed, Jun 21, 2023 at 9:44 PM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>
> On Wed, 21 Jun 2023 at 12:02, Victor Shih <victorshihgli@xxxxxxxxx> wrote:
> >
> > From: Victor Shih <victor.shih@xxxxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> > Signed-off-by: Victor Shih <victor.shih@xxxxxxxxxxxxxxxxxxx>
> > ---
>
> 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