Re: [PATCH v5 25/28] mtd: rawnand: hynix: Implement ->choose_data_interface() for H27UCG8T2ATR-BC

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Rickard Andersson <rickaran@axis.com>,
	Richard Weinberger <richard@nod.at>,
	linux-mtd@lists.infradead.org,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Tudor Ambarus <Tudor.Ambarus@microchip.com>
Subject: Re: [PATCH v5 25/28] mtd: rawnand: hynix: Implement ->choose_data_interface() for H27UCG8T2ATR-BC
Date: Wed, 27 May 2020 09:50:08 +0200	[thread overview]
Message-ID: <20200527095008.69d46afe@xps13> (raw)
In-Reply-To: <20200527000606.2c26f977@collabora.com>

Boris Brezillon <boris.brezillon@collabora.com> wrote on Wed, 27 May
2020 00:06:06 +0200:

> On Tue, 26 May 2020 21:17:22 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > This chip supports ONFI SDR timing mode 4, implement the new hook to
> > advertize it.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/mtd/nand/raw/nand_hynix.c | 16 ++++++++++++++++
> >  drivers/mtd/nand/raw/nand_ids.c   |  2 +-
> >  2 files changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/nand_hynix.c b/drivers/mtd/nand/raw/nand_hynix.c
> > index 7d1be53f27f3..d61a55121962 100644
> > --- a/drivers/mtd/nand/raw/nand_hynix.c
> > +++ b/drivers/mtd/nand/raw/nand_hynix.c
> > @@ -673,6 +673,19 @@ static void hynix_nand_cleanup(struct nand_chip *chip)
> >  	nand_set_manufacturer_data(chip, NULL);
> >  }
> >  
> > +static int
> > +h27ucg8t2atrbc_choose_data_interface(struct nand_chip *chip,
> > +				     struct nand_data_interface *iface)
> > +{
> > +	int ret;
> > +
> > +	ret = onfi_fill_data_interface(chip, iface, NAND_SDR_IFACE, 4);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return nand_choose_best_sdr_timings(chip, iface, NULL);
> > +}
> > +
> >  static int hynix_nand_init(struct nand_chip *chip)
> >  {
> >  	struct hynix_nand *hynix;
> > @@ -689,6 +702,9 @@ static int hynix_nand_init(struct nand_chip *chip)
> >  
> >  	nand_set_manufacturer_data(chip, hynix);
> >  
> > +	if (!strcmp("H27UCG8T2ATR-BC", chip->parameters.model))
> > +		chip->ops.choose_data_interface = h27ucg8t2atrbc_choose_data_interface;  
> 
> Same here. You either have to compare against "H27UCG8T2ATR-BC 64G 3.3V
> 8-bit" or use strncmp. This being said, I wonder if comparing the ID is
> not simpler.

I prefer to stick to strncmp so that the name of the part is clearly
identified for now. Maybe if we have too much exceptions coming in
we'll have to switch to ID bytes to "share" conditions between similar
parts.

> 
> > +
> >  	ret = hynix_nand_rr_init(chip);
> >  	if (ret)
> >  		hynix_nand_cleanup(chip);
> > diff --git a/drivers/mtd/nand/raw/nand_ids.c b/drivers/mtd/nand/raw/nand_ids.c
> > index c729a8bc895d..3b890d55703d 100644
> > --- a/drivers/mtd/nand/raw/nand_ids.c
> > +++ b/drivers/mtd/nand/raw/nand_ids.c
> > @@ -50,7 +50,7 @@ struct nand_flash_dev nand_flash_ids[] = {
> >  	{"H27UCG8T2ATR-BC 64G 3.3V 8-bit",
> >  		{ .id = {0xad, 0xde, 0x94, 0xda, 0x74, 0xc4} },
> >  		  SZ_8K, SZ_8K, SZ_2M, NAND_NEED_SCRAMBLING, 6, 640,
> > -		  NAND_ECC_INFO(40, SZ_1K), 4 },
> > +		  NAND_ECC_INFO(40, SZ_1K) },
> >    
> 
> Ultimately we should have those vendor specific entries defined in each
> vendor driver rather than declared here (not asking that for this
> series though).
> 
> >  	LEGACY_ID_NAND("NAND 4MiB 5V 8-bit",   0x6B, 4, SZ_8K, SP_OPTIONS),
> >  	LEGACY_ID_NAND("NAND 4MiB 3,3V 8-bit", 0xE3, 4, SZ_8K, SP_OPTIONS),  
> 

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