| Message ID | 20231127100756.41605-3-jaimeliao.tw@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | Hide flash name if it is not set | expand |
Hi, > (null) will print when flash ID founded in ID table but > the flash name didn't include in it. > > Make info->name optional in the print for showing flash name > and flash size. > > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw> > --- > drivers/mtd/spi-nor/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 1c443fe568cf..73405bed2a5a 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -3518,7 +3518,7 @@ int spi_nor_scan(struct spi_nor *nor, const char > *name, > /* No mtd_info fields should be used up to this point. */ > spi_nor_set_mtd_info(nor); > > - dev_info(dev, "%s (%lld Kbytes)\n", info->name, > + dev_info(dev, "%s (%lld Kbytes)\n", info->name ?: "", Maybe print the jedec id if the name is empty. -michael
Hi Michael, michael@walle.cc wrote on Mon, 27 Nov 2023 13:21:34 +0100: > Hi, > > > (null) will print when flash ID founded in ID table but > > the flash name didn't include in it. > > > > Make info->name optional in the print for showing flash name > > and flash size. > > > > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw> > > --- > > drivers/mtd/spi-nor/core.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > > index 1c443fe568cf..73405bed2a5a 100644 > > --- a/drivers/mtd/spi-nor/core.c > > +++ b/drivers/mtd/spi-nor/core.c > > @@ -3518,7 +3518,7 @@ int spi_nor_scan(struct spi_nor *nor, const char > *name, > > /* No mtd_info fields should be used up to this point. */ > > spi_nor_set_mtd_info(nor); > > > > - dev_info(dev, "%s (%lld Kbytes)\n", info->name, > > + dev_info(dev, "%s (%lld Kbytes)\n", info->name ?: "", > > Maybe print the jedec id if the name is empty. What about always printing the jedec id if names aren't reliable enough? Thanks, Miquèl
Hi, >> > (null) will print when flash ID founded in ID table but >> > the flash name didn't include in it. >> > >> > Make info->name optional in the print for showing flash name >> > and flash size. >> > >> > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw> >> > --- >> > drivers/mtd/spi-nor/core.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >> > index 1c443fe568cf..73405bed2a5a 100644 >> > --- a/drivers/mtd/spi-nor/core.c >> > +++ b/drivers/mtd/spi-nor/core.c >> > @@ -3518,7 +3518,7 @@ int spi_nor_scan(struct spi_nor *nor, const char > *name, >> > /* No mtd_info fields should be used up to this point. */ >> > spi_nor_set_mtd_info(nor); >> > >> > - dev_info(dev, "%s (%lld Kbytes)\n", info->name, >> > + dev_info(dev, "%s (%lld Kbytes)\n", info->name ?: "", >> >> Maybe print the jedec id if the name is empty. > > What about always printing the jedec id if names aren't reliable > enough? I'm fine with that, too. Actually I've considered that myself, but maybe Tudor or Pratyush want to keep the output backwards compatible. -michael
On 11/27/23 14:56, Michael Walle wrote: > Hi, > >>> > (null) will print when flash ID founded in ID table but >>> > the flash name didn't include in it. >>> > >>> > Make info->name optional in the print for showing flash name >>> > and flash size. >>> > >>> > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw> >>> > --- >>> > drivers/mtd/spi-nor/core.c | 2 +- >>> > 1 file changed, 1 insertion(+), 1 deletion(-) >>> > >>> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >>> > index 1c443fe568cf..73405bed2a5a 100644 >>> > --- a/drivers/mtd/spi-nor/core.c >>> > +++ b/drivers/mtd/spi-nor/core.c >>> > @@ -3518,7 +3518,7 @@ int spi_nor_scan(struct spi_nor *nor, const >>> char > *name, >>> > /* No mtd_info fields should be used up to this point. */ >>> > spi_nor_set_mtd_info(nor); >>> > >>> > - dev_info(dev, "%s (%lld Kbytes)\n", info->name, >>> > + dev_info(dev, "%s (%lld Kbytes)\n", info->name ?: "", >>> >>> Maybe print the jedec id if the name is empty. >> >> What about always printing the jedec id if names aren't reliable >> enough? > > I'm fine with that, too. Actually I've considered that myself, but > maybe Tudor or Pratyush want to keep the output backwards compatible. > We won't remove the names for flashes that already have a name defined, won't we? We just deprecate the name field and not use it anymore with new flash additions. No backward compatibility problem. BTW, that print should be lowered to dev_dbg, let's no longer pollute the kernel log. Drivers should be quiet if all goes well.
Hi, >>>> > (null) will print when flash ID founded in ID table but >>>> > the flash name didn't include in it. >>>> > >>>> > Make info->name optional in the print for showing flash name >>>> > and flash size. >>>> > >>>> > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw> >>>> > --- >>>> > drivers/mtd/spi-nor/core.c | 2 +- >>>> > 1 file changed, 1 insertion(+), 1 deletion(-) >>>> > >>>> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >>>> > index 1c443fe568cf..73405bed2a5a 100644 >>>> > --- a/drivers/mtd/spi-nor/core.c >>>> > +++ b/drivers/mtd/spi-nor/core.c >>>> > @@ -3518,7 +3518,7 @@ int spi_nor_scan(struct spi_nor *nor, const >>>> char > *name, >>>> > /* No mtd_info fields should be used up to this point. */ >>>> > spi_nor_set_mtd_info(nor); >>>> > >>>> > - dev_info(dev, "%s (%lld Kbytes)\n", info->name, >>>> > + dev_info(dev, "%s (%lld Kbytes)\n", info->name ?: "", >>>> >>>> Maybe print the jedec id if the name is empty. >>> >>> What about always printing the jedec id if names aren't reliable >>> enough? >> >> I'm fine with that, too. Actually I've considered that myself, but >> maybe Tudor or Pratyush want to keep the output backwards compatible. >> > > We won't remove the names for flashes that already have a name defined, > won't we? No. > We just deprecate the name field and not use it anymore with > new flash additions. No backward compatibility problem. I mean in the kernel console output. Someone out there might parse it. If you switch from dev_info(dev, "%s (%lld Kbytes)\n", info->name, ...) to dev_info(dev, ""%*phN (%lld Kbytes)\n", id_len, id, ...) > BTW, that print should be lowered to dev_dbg, let's no longer pollute > the kernel log. Drivers should be quiet if all goes well. Well, ok, that will also answer my question :) So let's go with the dev_dbg() and id only. -michael
On 11/27/23 15:28, Michael Walle wrote: > Hi, > >>>>> > (null) will print when flash ID founded in ID table but >>>>> > the flash name didn't include in it. >>>>> > >>>>> > Make info->name optional in the print for showing flash name >>>>> > and flash size. >>>>> > >>>>> > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw> >>>>> > --- >>>>> > drivers/mtd/spi-nor/core.c | 2 +- >>>>> > 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> > >>>>> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >>>>> > index 1c443fe568cf..73405bed2a5a 100644 >>>>> > --- a/drivers/mtd/spi-nor/core.c >>>>> > +++ b/drivers/mtd/spi-nor/core.c >>>>> > @@ -3518,7 +3518,7 @@ int spi_nor_scan(struct spi_nor *nor, const >>>>> char > *name, >>>>> > /* No mtd_info fields should be used up to this point. */ >>>>> > spi_nor_set_mtd_info(nor); >>>>> > >>>>> > - dev_info(dev, "%s (%lld Kbytes)\n", info->name, >>>>> > + dev_info(dev, "%s (%lld Kbytes)\n", info->name ?: "", >>>>> >>>>> Maybe print the jedec id if the name is empty. >>>> >>>> What about always printing the jedec id if names aren't reliable >>>> enough? >>> >>> I'm fine with that, too. Actually I've considered that myself, but >>> maybe Tudor or Pratyush want to keep the output backwards compatible. >>> >> >> We won't remove the names for flashes that already have a name defined, >> won't we? > > No. > >> We just deprecate the name field and not use it anymore with >> new flash additions. No backward compatibility problem. > > I mean in the kernel console output. Someone out there might parse it. > > If you switch from > dev_info(dev, "%s (%lld Kbytes)\n", info->name, ...) > to > dev_info(dev, ""%*phN (%lld Kbytes)\n", id_len, id, ...) > no problem in changing what we print to kernel log, there's no guarantees associated with it. >> BTW, that print should be lowered to dev_dbg, let's no longer pollute >> the kernel log. Drivers should be quiet if all goes well. > > Well, ok, that will also answer my question :) So let's go > with the dev_dbg() and id only. > I looked again and I saw that together with the name we printed the flash size in Kbytes. But then the next print printed the same flash size in bytes and then in MBytes. And then we printed some mtd data that should be obtained with the mtd ioctls. I didn't like that mess and ended up removing all those prints. We shall now have a faster kernel boot (got rid of the print info), and cleaner kernel log. All that debug data shall be obtained with the mtd ioctls and the SPI NOR sysfs and debugfs entries, so hopefully more people will start using them. Here's the patch that I mentioned: https://lore.kernel.org/linux-mtd/20231127165908.1734951-1-tudor.ambarus@linaro.org/T/#u Cheers, ta
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 1c443fe568cf..73405bed2a5a 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -3518,7 +3518,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, /* No mtd_info fields should be used up to this point. */ spi_nor_set_mtd_info(nor); - dev_info(dev, "%s (%lld Kbytes)\n", info->name, + dev_info(dev, "%s (%lld Kbytes)\n", info->name ?: "", (long long)mtd->size >> 10); dev_dbg(dev,