Messages in this thread | | | From | <> | Subject | Re: [PATCH v4 03/11] mtd: spi-nor: core: Use auto-detection only once | Date | Wed, 27 Apr 2022 07:13:45 +0000 |
| |
On 4/27/22 08:20, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 21/04/22 01:41PM, Tudor.Ambarus@microchip.com wrote: >> On 4/21/22 16:16, Pratyush Yadav wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> On 21/04/22 07:18AM, Tudor.Ambarus@microchip.com wrote: >>>> Hi, Pratyush, >>>> >>>> I forgot to remove few checks, would you please remove them when applying? >>>> See below. >>>> >>>> On 4/20/22 13:34, 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> >>>>> Reviewed-by: Michael Walle <michael@walle.cc> >>>>> --- >>>>> drivers/mtd/spi-nor/core.c | 13 ++++++++----- >>>>> 1 file changed, 8 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >>>>> index b9cc8bbf1f62..b55d922d46dd 100644 >>>>> --- a/drivers/mtd/spi-nor/core.c >>>>> +++ b/drivers/mtd/spi-nor/core.c >>>>> @@ -2896,13 +2896,14 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor, >>>>> { >>>>> const struct flash_info *info = NULL; >>>>> >>>>> - if (name) >>>>> + if (name) { >>>>> info = spi_nor_match_name(nor, name); >>>>> + if (IS_ERR(info)) >>>>> + return info; >>>> >>>> As Michael suggested spi_nor_match_name() returns NULL or valid entry, so this >>>> check is not necessary, let's remove them. >>>> >>>>> + } >>>>> /* Try to auto-detect if chip name wasn't specified or not found */ >>>>> if (!info) >>>>> - info = spi_nor_read_id(nor); >>>>> - if (IS_ERR_OR_NULL(info)) >>>>> - return ERR_PTR(-ENOENT); >>>>> + return spi_nor_read_id(nor); >>>>> >>>>> /* >>>>> * If caller has specified name of flash model that can normally be >>>>> @@ -2994,7 +2995,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, >>>>> return -ENOMEM; >>>>> >>>>> info = spi_nor_get_flash_info(nor, name); >>>>> - if (IS_ERR(info)) >>>>> + if (!info) >>>>> + return -ENOENT; >>>> >>>> also according to Michael, this change is not needed as spi_nor_get_flash_info() can't >>>> return NULL. Here we can keep the code as it was. Let me know if you want me to respin. >>> >>> TBH I don't think a NULL check here hurts much since the behaviour might >>> change later, and error paths don't get exercised as often. But I have >> >> I agree, but at the same time we're introducing checks gratuitously. Since >> Michael cared about it, it's fine that we removed it. I don't care too much >> about it. >> >>> made both changes when applying. You can double-check at [0] if you >>> want.> >>> [0] https://github.com/prati0100/linux-0day/commit/67d913746833ee54bf4c661040f3ef13657dffd8 >> >> looks good. >> >> btw: I think this patch >> https://github.com/prati0100/linux-0day/commit/b45bbff85d49529f8daff83c341a292f6c6492ca >> may introduce a regression on some atmel chips. Let me try it please. > > Did you get a chance to try this out? If it works fine, I would like to > apply it.
I tried it on a at25df321a. When calling unlock, everything seems fine. However I haven't tried a lock/unlock cycle as lock is not supported and I couldn't allocate more time. Even if there will be regressions we can handle them afterwards, so let's apply it.
Cheers, ta
| |