On Wed, Oct 13, 2021 at 02:44:29PM +0200, Miquel Raynal wrote: > Hi Uwe, > > u.kleine-koenig@xxxxxxxxxxxxxx wrote on Tue, 12 Oct 2021 17:39:38 +0200: > > > When an spi driver's remove function returns a non-zero error code > > Should we s/an spi/a SPI/? My (German) knowledge about the English Grammar claims that independent of how you spell SPI, it must be "an" because when I say it, it's [ɛspi:aɪ] (unless you call it [spaɪ]?) In my eyes "spi" is right, because SPI is the protocol and "spi" is the kernel framework. But I don't feel strong here and you're already the second who suggests something similar. > > nothing happens apart from emitting a generic error message. Make this > > error message more device specific and return zero instead. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> > > --- > > drivers/mtd/devices/mtd_dataflash.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c > > index 9802e265fca8..2691b6b79df8 100644 > > --- a/drivers/mtd/devices/mtd_dataflash.c > > +++ b/drivers/mtd/devices/mtd_dataflash.c > > @@ -919,7 +919,10 @@ static int dataflash_remove(struct spi_device *spi) > > status = mtd_device_unregister(&flash->mtd); > > if (status == 0) > > kfree(flash); > > - return status; > > + else > > + dev_warn(&spi->dev, "Failed to unregister mtd device (%pe)\n", > > + ERR_PTR(status)); > > + return 0; > > As part of a recent NAND cleanup series we ended up adding WARN_ON() [1] > to make it very clear that if this happens, it's not expected at all (it > was Boris' advice). > > I don't think there is only one good solution but perhaps its best to > keep it sync'ed with the other drivers in MTD? Well, if WARN_ON or dev_warn is the right thing is subjective. Your subjective counts more as you're an MTD maintainer. Can rework accordingly for v3. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
