All of lore.kernel.org
 help / color / mirror / Atom feed
From: <Tudor.Ambarus@microchip.com>
To: <j.neuschaefer@gmx.net>
Cc: richard@nod.at, linux-mtd@lists.infradead.org, vigneshr@ti.com,
	linux-kernel@vger.kernel.org, miquel.raynal@bootlin.com
Subject: Re: [PATCH] mtd: spi-nor: Simplify loop in spi_nor_read_id()
Date: Fri, 21 Feb 2020 16:50:37 +0000	[thread overview]
Message-ID: <5932130.I5bQ6OmJFL@localhost.localdomain> (raw)
In-Reply-To: <20200221162248.GG2031@latitude>

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




______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

WARNING: multiple messages have this Message-ID (diff)
From: <Tudor.Ambarus@microchip.com>
To: <j.neuschaefer@gmx.net>
Cc: <linux-mtd@lists.infradead.org>, <miquel.raynal@bootlin.com>,
	<richard@nod.at>, <vigneshr@ti.com>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] mtd: spi-nor: Simplify loop in spi_nor_read_id()
Date: Fri, 21 Feb 2020 16:50:37 +0000	[thread overview]
Message-ID: <5932130.I5bQ6OmJFL@localhost.localdomain> (raw)
In-Reply-To: <20200221162248.GG2031@latitude>

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




  reply	other threads:[~2020-02-21 16:51 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-18 15:10 [PATCH] mtd: spi-nor: Simplify loop in spi_nor_read_id() Jonathan Neuschäfer
2020-02-18 15:10 ` Jonathan Neuschäfer
2020-02-19  8:06 ` Tudor.Ambarus
2020-02-19  8:06   ` Tudor.Ambarus
2020-02-21 16:22   ` Jonathan Neuschäfer
2020-02-21 16:22     ` Jonathan Neuschäfer
2020-02-21 16:50     ` Tudor.Ambarus [this message]
2020-02-21 16:50       ` Tudor.Ambarus
2020-02-22 21:51       ` Jonathan Neuschäfer
2020-02-22 21:51         ` Jonathan Neuschäfer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5932130.I5bQ6OmJFL@localhost.localdomain \
    --to=tudor.ambarus@microchip.com \
    --cc=j.neuschaefer@gmx.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=miquel.raynal@bootlin.com \
    --cc=richard@nod.at \
    --cc=vigneshr@ti.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.