- 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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
On Wed, Feb 19, 2020 at 08:06:08AM +0000, [email protected] 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
On Friday, February 21, 2020 6:22:48 PM EET Jonathan Neusch?fer wrote:
> On Wed, Feb 19, 2020 at 08:06:08AM +0000, [email protected] 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
On Fri, Feb 21, 2020 at 04:50:37PM +0000, [email protected] 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