Received-SPF: pass (google.com: domain of linux-kernel+bounces-72035-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <65d3ce8f.050a0220.e5faf.c252@mx.google.com>
Date: Mon, 19 Feb 2024 22:56:30 +0100
From: Christian Marangi <ansuelsmth@gmail.com>
To: Michael Walle <mwalle@kernel.org>
Cc: Tudor Ambarus <tudor.ambarus@linaro.org>,
	Pratyush Yadav <pratyush@kernel.org>,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>, linux-kernel@vger.kernel.org,
	linux-mtd@lists.infradead.org, David Bauer <mail@david-bauer.net>
Subject: Re: [PATCH] mtd: spi-nor: Add support for BoHong bh25q128as
References: <20240217122029.3278-1-ansuelsmth@gmail.com>
 <CZ8X218EWPC9.25TO9O5C7VXQ9@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CZ8X218EWPC9.25TO9O5C7VXQ9@kernel.org>

On Mon, Feb 19, 2024 at 09:35:27AM +0100, Michael Walle wrote:
> Hi,
> 
> On Sat Feb 17, 2024 at 1:20 PM CET, Christian Marangi wrote:
> > From: David Bauer <mail@david-bauer.net>
> >
> > Add MTD support for the BoHong bh25q128as SPI NOR chip.
> > The chip has 16MB of total capacity, divided into a total of 256
> > sectors, each 64KB sized. The chip also supports 4KB sectors.
> > Additionally, it supports dual and quad read modes.
> >
> > Datasheet is public and can be found here [1].
> 
> Last time it wasn't clear if this flash will support SFDP or not.
> Could you please try to dump the SFDP again, see [1].
>

Ok will include in v2.

> 
> > Functionality was verified on an Tenbay WR1800K / MTK MT7621 board.
> 
> Also per [1], you'd need to provide your test results.
> 
> > [1] https://www.e-interlink.com.tw/userUpload/files/BH25Q128AS_v1_0.pdf
> 
> Link: right above your SoB please.
> 
> > Signed-off-by: David Bauer <mail@david-bauer.net>
> > [ reworked to new flash_info format ]
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  drivers/mtd/spi-nor/Makefile |  1 +
> >  drivers/mtd/spi-nor/bohong.c | 24 ++++++++++++++++++++++++
> >  drivers/mtd/spi-nor/core.c   |  1 +
> >  drivers/mtd/spi-nor/core.h   |  1 +
> >  4 files changed, 27 insertions(+)
> >  create mode 100644 drivers/mtd/spi-nor/bohong.c
> >
> > diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> > index 5e68468b72fc..c8849cf5124f 100644
> > --- a/drivers/mtd/spi-nor/Makefile
> > +++ b/drivers/mtd/spi-nor/Makefile
> > @@ -2,6 +2,7 @@
> >  
> >  spi-nor-objs			:= core.o sfdp.o swp.o otp.o sysfs.o
> >  spi-nor-objs			+= atmel.o
> > +spi-nor-objs			+= bohong.o
> >  spi-nor-objs			+= eon.o
> >  spi-nor-objs			+= esmt.o
> >  spi-nor-objs			+= everspin.o
> > diff --git a/drivers/mtd/spi-nor/bohong.c b/drivers/mtd/spi-nor/bohong.c
> > new file mode 100644
> > index 000000000000..26988c139262
> > --- /dev/null
> > +++ b/drivers/mtd/spi-nor/bohong.c
> > @@ -0,0 +1,24 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2005, Intec Automation Inc.
> > + * Copyright (C) 2014, Freescale Semiconductor, Inc.
> 
> Please remove, there is nothing from the old code left here.
> 
> > + */
> > +
> > +#include <linux/mtd/spi-nor.h>
> > +
> > +#include "core.h"
> > +
> > +static const struct flash_info bohong_parts[] = {
> > +	{
> > +		.id = SNOR_ID(0x68, 0x40, 0x18),
> > +		.name = "bh25q128as",
> No names anymore, please.
> 

Mhhh why this change? Doesn't this makes the thing problematic to
identify?

> > +		.size = SZ_16M,
> > +		.no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
> > +	},
> > +};
> > +
> > +const struct spi_nor_manufacturer spi_nor_bohong = {
> > +	.name = "bohong",
> 
> This should be dropped, too. Otherwise looks good, if SFDP is not
> supported.
>

Ok, thanks a lot for the review!

> 
> [1] https://docs.kernel.org/driver-api/mtd/spi-nor.html
> 
> > +	.parts = bohong_parts,
> > +	.nparts = ARRAY_SIZE(bohong_parts),
> > +};
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 4129764fad8c..29c28ee683a1 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -2037,6 +2037,7 @@ int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor)
> >  
> >  static const struct spi_nor_manufacturer *manufacturers[] = {
> >  	&spi_nor_atmel,
> > +	&spi_nor_bohong,
> >  	&spi_nor_eon,
> >  	&spi_nor_esmt,
> >  	&spi_nor_everspin,
> > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> > index d36c0e072954..c293568ae827 100644
> > --- a/drivers/mtd/spi-nor/core.h
> > +++ b/drivers/mtd/spi-nor/core.h
> > @@ -601,6 +601,7 @@ struct sfdp {
> >  
> >  /* Manufacturer drivers. */
> >  extern const struct spi_nor_manufacturer spi_nor_atmel;
> > +extern const struct spi_nor_manufacturer spi_nor_bohong;
> >  extern const struct spi_nor_manufacturer spi_nor_eon;
> >  extern const struct spi_nor_manufacturer spi_nor_esmt;
> >  extern const struct spi_nor_manufacturer spi_nor_everspin;
> 


-- 
	Ansuel