Messages in this thread | | | Date | Thu, 31 Mar 2022 00:20:05 +0530 | From | Pratyush Yadav <> | Subject | Re: [PATCH v2 3/8] mtd: spi-nor: core: Use auto-detection only once |
| |
On 21/03/22 11:38PM, Michael Walle wrote: > Am 2022-03-21 18:42, schrieb Pratyush Yadav: > > On 21/03/22 12:50PM, Tudor.Ambarus@microchip.com wrote: > > > On 3/21/22 14:14, Pratyush Yadav wrote: > > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > > > > > On 28/02/22 01:17PM, Tudor Ambarus wrote: > > > >> In case spi_nor_match_name() returned NULL, the auto detection was > > > >> issued twice. There's no reason to try to detect the same chip twice, > > > >> do the auto detection only once. > > > >> > > > >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > > > >> --- > > > >> drivers/mtd/spi-nor/core.c | 10 ++++++---- > > > >> 1 file changed, 6 insertions(+), 4 deletions(-) > > > >> > > > >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > > > >> index f87cb7d3daab..b1d6fa65417d 100644 > > > >> --- a/drivers/mtd/spi-nor/core.c > > > >> +++ b/drivers/mtd/spi-nor/core.c > > > >> @@ -2894,13 +2894,15 @@ static const struct flash_info *spi_nor_match_name(struct spi_nor *nor, > > > >> static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor, > > > >> const char *name) > > > >> { > > > >> - const struct flash_info *info = NULL; > > > >> + const struct flash_info *info = NULL, *detected_info = NULL; > > > >> > > > >> if (name) > > > >> info = spi_nor_match_name(nor, name); > > > >> /* Try to auto-detect if chip name wasn't specified or not found */ > > > >> - if (!info) > > > >> - info = spi_nor_read_id(nor); > > > >> + if (!info) { > > > >> + detected_info = spi_nor_read_id(nor); > > > >> + info = detected_info; > > > >> + } > > > >> if (IS_ERR_OR_NULL(info)) > > > >> return ERR_PTR(-ENOENT); > > > >> > > > >> @@ -2908,7 +2910,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor, > > > >> * If caller has specified name of flash model that can normally be > > > >> * detected using JEDEC, let's verify it. > > > >> */ > > > >> - if (name && info->id_len) { > > > >> + if (name && !detected_info && info->id_len) { > > > >> const struct flash_info *jinfo; > > > >> > > > >> jinfo = spi_nor_read_id(nor); > > > > > > > > I think the flow can be a little bit better. How about: > > > > > > > > if (name) > > > > info = spi_nor_match_name(); > > > > > > > > if (!info) { > > > > info = spi_nor_read_id(); > > > > if (IS_ERR_OR_NULL(info)) > > > > return ERR_PTR(-ENOENT); > > > > > > > > return info; > > > > } > > +1 for the flow. But is it correct that we just ignore any former > error and just replace it with ENOENT? Should we return NULL here > and let the caller handle the translation from NULL to ENOENT (and > keeping any other errors) > > > > > > > Here we miss the IS_ERR check in case info is retrieved with > > > spi_nor_match_name(). > > > Do you expect spi_nor_match_name() to ever return an error? As it is > > > now it doesn't. > > > I'm fine either way. In case you want me to follow your suggestion, > > > give me a sign > > > and I'll make a dedicated patch to move the IS_ERR_OR_NULL check. > > > Will add your > > > Suggested-by tag. > > > > I think it should be safe to assume it won't ever return an error since > > all it does is iterate over an array that is always present. I don't see > > that changing in the foreseeable future either. So I think not having > > the IS_ERR check is fine. > > But what does it cost to just add the error check now so it won't > be forgotten in the future? > > if (name) { > info = spi_nor_match_name(); > if (IS_ERR(info)) > return info; > } > if (!info) > return spi_nor_read_id(); > > <flash model check code follows here> > > And then let the caller handle NULL and translate it to ENOENT.
Sounds good to me.
-- Regards, Pratyush Yadav Texas Instruments Inc.
| |