On 3/15/21 8:53 AM, Joe Perches wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> 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;
Isn't else redundant? What are the benefits of keeping the else?
>
> 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;
returning one is wrong indeed, would you submit a patch for the conversion
of the functions to bool?
>
> 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);
This should be fine too.
Cheers,
ta
