Re: [PATCH v2 5/5] mtd: spi-nor: swp: Drop 'else' after 'return'

[Joe Perches]

On Mon, 2021-03-08 at 11:58 +0530, Pratyush Yadav wrote:
> On 06/03/21 11:50AM, Tudor Ambarus wrote:
> > else is not generally useful after a break or return.
> > 
> > Signed-off-by: Tudor Ambarus <tudor.amba...@microchip.com>
> 
> Reviewed-by: Pratyush Yadav <p.ya...@ti.com>
> 

I don't think this improves the code.

Generally, checkpatch is a stupid little script.

This code uses a form like:
        if (foo)
                return bar;
        else
                return baz;

which checkpatch recognizes as OK and so checkpatch does not
emit any warning message, but this code just adds comments
before each return which confuses checkpatch.

I think better would be to change the code to use temporaries
and convert the functions to bool.

Something like:
---
 drivers/mtd/spi-nor/core.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 0522304f52fa..e174a2f1d621 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1798,36 +1798,41 @@ static void spi_nor_get_locked_range_sr(struct spi_nor 
*nor, u8 sr, loff_t *ofs,
 }
 
 /*
- * Return 1 if the entire region is locked (if @locked is true) or unlocked (if
- * @locked is false); 0 otherwise
+ * Return true if the entire region is locked
+ * (if @locked is true) or unlocked (if @locked is false); false otherwise
  */
-static int spi_nor_check_lock_status_sr(struct spi_nor *nor, loff_t ofs,
+static bool spi_nor_check_lock_status_sr(struct spi_nor *nor, loff_t ofs,
                                        uint64_t len, u8 sr, bool locked)
 {
        loff_t lock_offs;
        uint64_t lock_len;
+       uint64_t lock_max;
+       uint64_t ofs_max;
 
        if (!len)
-               return 1;
+               return true;
 
        spi_nor_get_locked_range_sr(nor, sr, &lock_offs, &lock_len);
 
+       lock_max = lock_offs + lock_len;
+       ofs_max = ofs + len;
+
        if (locked)
                /* Requested range is a sub-range of locked range */
-               return (ofs + len <= lock_offs + lock_len) && (ofs >= 
lock_offs);
+               return (ofs_max <= lock_max) && (ofs >= lock_offs);
        else
                /* Requested range does not overlap with locked range */
-               return (ofs >= lock_offs + lock_len) || (ofs + len <= 
lock_offs);
+               return (ofs >= lock_max) || (ofs_max <= lock_offs);
 }
 
-static int spi_nor_is_locked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
-                               u8 sr)
+static bool spi_nor_is_locked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
+                                u8 sr)
 {
        return spi_nor_check_lock_status_sr(nor, ofs, len, sr, true);
 }
 
-static int spi_nor_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t 
len,
-                                 u8 sr)
+static bool spi_nor_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t 
len,
+                                  u8 sr)
 {
        return spi_nor_check_lock_status_sr(nor, ofs, len, sr, false);
 }