[PATCH] mtd: spi-nor: Simplify loop in spi_nor_read_id()

[PATCH] mtd: spi-nor: Simplify loop in spi_nor_read_id()

[Jonathan Neuschäfer]

- Don't use `tmp` for two purposes (return value, loop counter)
- Name the loop counter `i`, as is convention
- Return the pointer variable that the if conditions leading up to the
  return statement already operate on, rather than a different
  expression that evaluates to the same pointer

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 drivers/mtd/spi-nor/spi-nor.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 4fc632ec18fe..c491572d5267 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2711,7 +2711,7 @@ static const struct flash_info spi_nor_ids[] = {

 static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
 {
-	int			tmp;
+	int			tmp, i;
 	u8			*id = nor->bouncebuf;
 	const struct flash_info	*info;

@@ -2732,11 +2732,11 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
 		return ERR_PTR(tmp);
 	}

-	for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
-		info = &spi_nor_ids[tmp];
+	for (i = 0; i < ARRAY_SIZE(spi_nor_ids) - 1; i++) {
+		info = &spi_nor_ids[i];
 		if (info->id_len) {
 			if (!memcmp(info->id, id, info->id_len))
-				return &spi_nor_ids[tmp];
+				return info;
 		}
 	}
 	dev_err(nor->dev, "unrecognized JEDEC id bytes: %*ph\n",
--
2.20.1

Re: [PATCH] mtd: spi-nor: Simplify loop in spi_nor_read_id()

[Tudor.Ambarus]

Hi, Jonathan,

On Tuesday, February 18, 2020 5:10:34 PM EET Jonathan Neuschäfer wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> - Don't use `tmp` for two purposes (return value, loop counter)
> - Name the loop counter `i`, as is convention
> - Return the pointer variable that the if conditions leading up to the
>   return statement already operate on, rather than a different
>   expression that evaluates to the same pointer
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 4fc632ec18fe..c491572d5267 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2711,7 +2711,7 @@ static const struct flash_info spi_nor_ids[] = {
> 
>  static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
>  {
> -       int                     tmp;
> +       int                     tmp, i;

while cleaning this function, would you rename tmp with ret?

>         u8                      *id = nor->bouncebuf;

and please drop this tab between u8 and *id

>         const struct flash_info *info;

Also, IMO, the definition of variables should be done with the focus of 
avoiding stack padding. With this in mind, I would first define the pointers 
and then the ints and smaller types. But there are others than prefer defining 
the variables in a tree/reverse-tree way, depending of the length of the line. 
There's no agreement on this, either way if fine, do as you prefer.

> 
> @@ -2732,11 +2732,11 @@ static const struct flash_info
> *spi_nor_read_id(struct spi_nor *nor) return ERR_PTR(tmp);
>         }
> 
> -       for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
> -               info = &spi_nor_ids[tmp];
> +       for (i = 0; i < ARRAY_SIZE(spi_nor_ids) - 1; i++) {
> +               info = &spi_nor_ids[i];
>                 if (info->id_len) {
>                         if (!memcmp(info->id, id, info->id_len))
> -                               return &spi_nor_ids[tmp];
> +                               return info;

Looks good,

Cheers,
ta

Re: [PATCH] mtd: spi-nor: Simplify loop in spi_nor_read_id()

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 1159 bytes --]

On Wed, Feb 19, 2020 at 08:06:08AM +0000, Tudor.Ambarus@microchip.com wrote:
[...]
> >  static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
> >  {
> > -       int                     tmp;
> > +       int                     tmp, i;
> 
> while cleaning this function, would you rename tmp with ret?

Good idea, I'll do that in v2.

> >         u8                      *id = nor->bouncebuf;
> 
> and please drop this tab between u8 and *id

The same with the other variables in this functions? They have tabs
between the type and the pointer star / name as well.

> 
> >         const struct flash_info *info;
> 
> Also, IMO, the definition of variables should be done with the focus of 
> avoiding stack padding. With this in mind, I would first define the pointers 
> and then the ints and smaller types. But there are others than prefer defining 
> the variables in a tree/reverse-tree way, depending of the length of the line. 
> There's no agreement on this, either way if fine, do as you prefer.

I have no preference here. I think I'll keep it as is for now.


Thanks for the review,
Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] mtd: spi-nor: Simplify loop in spi_nor_read_id()

[Tudor.Ambarus]

On Friday, February 21, 2020 6:22:48 PM EET Jonathan Neuschäfer wrote:
> On Wed, Feb 19, 2020 at 08:06:08AM +0000, Tudor.Ambarus@microchip.com wrote:
> [...]
> 
> > >  static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
> > >  {
> > > 
> > > -       int                     tmp;
> > > +       int                     tmp, i;
> > 
> > while cleaning this function, would you rename tmp with ret?
> 
> Good idea, I'll do that in v2.
> 
> > >         u8                      *id = nor->bouncebuf;
> > 
> > and please drop this tab between u8 and *id
> 
> The same with the other variables in this functions? They have tabs
> between the type and the pointer star / name as well.

yes, please.

> 
> > >         const struct flash_info *info;

how about getting rid of this local variable? Use in the function something 
like:

                if (spi_nor_ids[i].id_len &&
                    !memcmp(spi_nor_ids[i].id, id, spi_nor_ids[i].id_len)
                    return &spi_nor_ids[i];

> > 
> > Also, IMO, the definition of variables should be done with the focus of
> > avoiding stack padding. With this in mind, I would first define the
> > pointers and then the ints and smaller types. But there are others than
> > prefer defining the variables in a tree/reverse-tree way, depending of
> > the length of the line. There's no agreement on this, either way if fine,
> > do as you prefer.
> I have no preference here. I think I'll keep it as is for now.
> 

Ok. Cheers,
ta

Re: [PATCH] mtd: spi-nor: Simplify loop in spi_nor_read_id()

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 439 bytes --]

On Fri, Feb 21, 2020 at 04:50:37PM +0000, Tudor.Ambarus@microchip.com wrote:
> > > >         const struct flash_info *info;
> 
> how about getting rid of this local variable? Use in the function something 
> like:
> 
>                 if (spi_nor_ids[i].id_len &&
>                     !memcmp(spi_nor_ids[i].id, id, spi_nor_ids[i].id_len)
>                     return &spi_nor_ids[i];

Looks alright. I'll do it.


Jonathan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]