[PATCH 01/19] spi: cadence_qspi: Add support for DDR PHY mode

[Bhumkar, Tejas Arvind]

[AMD Official Use Only - General]

Hi Dan,

> -----Original Message-----
> From: Dan Carpenter <dan.carpenter at linaro.org>
> Sent: Wednesday, March 13, 2024 2:28 PM
> To: Bhumkar, Tejas Arvind <tejas.arvind.bhumkar at amd.com>
> Cc: u-boot at lists.denx.de; jagan at amarulasolutions.com; n-francis at ti.com;
> Simek, Michal <michal.simek at amd.com>; Abbarapu, Venkatesh
> <venkatesh.abbarapu at amd.com>; git (AMD-Xilinx) <git at amd.com>; T
> Karthik Reddy <t.karthik.reddy at xilinx.com>; Ashok Reddy Soma
> <ashok.reddy.soma at amd.com>
> Subject: Re: [PATCH 01/19] spi: cadence_qspi: Add support for DDR PHY mode
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> > diff --git a/drivers/mtd/spi/spi-nor-core.c
> > b/drivers/mtd/spi/spi-nor-core.c index faf02c7778..5895b5de09 100644
> > --- a/drivers/mtd/spi/spi-nor-core.c
> > +++ b/drivers/mtd/spi/spi-nor-core.c
> > @@ -1511,8 +1511,10 @@ static const struct flash_info
> *spi_nor_read_id(struct spi_nor *nor)
> >       info = spi_nor_ids;
> >       for (; info->name; info++) {
> >               if (info->id_len) {
> > -                     if (!memcmp(info->id, id, info->id_len))
> > +                     if ((!memcmp(info->id, id, info->id_len)) &&
> > +                         memcpy(nor->spi->device_id, id,
> > + SPI_NOR_MAX_ID_LEN)) {
>
> Please, don't put a memcpy() into a condition.  It looks like a memcmp() to the
> eye.
>
> >                               return info;
> > +                     }
>
>                 if (!memcmp(info->id, id, info->id_len)) {
>                         memcpy(nor->spi->device_id, id, SPI_NOR_MAX_ID_LEN);
>                         return info;
>                 }
>
> >               }
> >       }
> >
>
> [ snip ]
[Tejas ] : Thanks for the review, I will update this in v2
>
> >  static int cadence_spi_mem_exec_op(struct spi_slave *spi,
> >                                  const struct spi_mem_op *op)  { @@
> > -353,6 +649,9 @@ static int cadence_spi_mem_exec_op(struct spi_slave
> *spi,
> >               break;
> >       }
> >
> > +     if (op->cmd.dtr)
> > +             err = cadence_spi_setup_ddrmode(spi, op);
> > +
> >       return err;
>
>
> I think there should be another if (err) return err after the end of the switch
> statement.
[Tejas ] : I will update this too in v2.
>
> >  }
> >
>
> regards,
> dan carpenter