Re: [PATCH v6 3/3] spi: airoha: Add spi-nand flash controller driver

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4776 bytes --]

> On Fri, May 3, 2024 at 3:15 PM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> >
> > > On Wed, May 01, 2024 at 04:06:43PM +0200, Lorenzo Bianconi wrote:
> > > > Introduce support for spi-nand driver of the Airoha NAND Flash Interface
> > > > found on Airoha ARM SoCs.
> > >
> > > This doesn't apply against current code, please check and resend.
> >
> > Hi Mark,
> >
> > patch v6 3/3 has just a couple of cosmetic changes requested by Andy with
> > respect to v5 3/3.
> >
> > @Andy: do you think we can drop these changes or do you prefer to add them? (in
> > the latter case I can post an incremental patch).
> 
> I am not sure what this is about, do you mean the changes asked by me
> made this driver not applicable?
> 

These are the only changes between patch v5 3/3 (applied by Mark) and patch v6 3/3:

--- /tmp/spi-airoha-snfi.c	2024-05-03 15:13:35.020903623 +0200
+++ drivers/spi/spi-airoha-snfi.c	2024-05-03 15:13:41.836924124 +0200
@@ -638,9 +638,9 @@
 					u64 offs, size_t len, void *buf)
 {
 	struct spi_device *spi = desc->mem->spi;
+	struct airoha_snand_ctrl *as_ctrl = spi_controller_get_devdata(spi->controller);
 	struct airoha_snand_dev *as_dev = spi_get_ctldata(spi);
 	struct spi_mem_op *op = &desc->info.op_tmpl;
-	struct airoha_snand_ctrl *as_ctrl;
 	u32 val, rd_mode;
 	int err;
 
@@ -661,7 +661,6 @@
 		break;
 	}
 
-	as_ctrl = spi_controller_get_devdata(spi->controller);
 	err = airoha_snand_set_mode(as_ctrl, SPI_MODE_DMA);
 	if (err < 0)
 		return err;
@@ -768,13 +767,12 @@
 					 u64 offs, size_t len, const void *buf)
 {
 	struct spi_device *spi = desc->mem->spi;
+	struct airoha_snand_ctrl *as_ctrl = spi_controller_get_devdata(spi->controller);
 	struct airoha_snand_dev *as_dev = spi_get_ctldata(spi);
 	struct spi_mem_op *op = &desc->info.op_tmpl;
-	struct airoha_snand_ctrl *as_ctrl;
 	u32 wr_mode, val;
 	int err;
 
-	as_ctrl = spi_controller_get_devdata(spi->controller);
 	err = airoha_snand_set_mode(as_ctrl, SPI_MODE_MANUAL);
 	if (err < 0)
 		return err;
@@ -885,12 +883,12 @@
 static int airoha_snand_exec_op(struct spi_mem *mem,
 				const struct spi_mem_op *op)
 {
-	struct airoha_snand_dev *as_dev = spi_get_ctldata(mem->spi);
+	struct spi_device *spi = mem->spi;
+	struct airoha_snand_ctrl *as_ctrl = spi_controller_get_devdata(spi->controller);
+	struct airoha_snand_dev *as_dev = spi_get_ctldata(spi);
 	u8 data[8], cmd, opcode = op->cmd.opcode;
-	struct airoha_snand_ctrl *as_ctrl;
 	int i, err;
 
-	as_ctrl = spi_controller_get_devdata(mem->spi->controller);
 	if (opcode == SPI_NAND_OP_PROGRAM_EXECUTE &&
 	    op->addr.val == as_dev->cur_page_num) {
 		as_dev->data_need_update = true;
@@ -965,11 +963,9 @@
 
 static int airoha_snand_setup(struct spi_device *spi)
 {
-	struct airoha_snand_ctrl *as_ctrl;
+	struct airoha_snand_ctrl *as_ctrl = spi_controller_get_devdata(spi->controller);
 	struct airoha_snand_dev *as_dev;
 
-	as_ctrl = spi_controller_get_devdata(spi->controller);
-
 	as_dev = devm_kzalloc(as_ctrl->dev, sizeof(*as_dev), GFP_KERNEL);
 	if (!as_dev)
 		return -ENOMEM;
@@ -994,10 +990,9 @@
 
 static void airoha_snand_cleanup(struct spi_device *spi)
 {
+	struct airoha_snand_ctrl *as_ctrl = spi_controller_get_devdata(spi->controller);
 	struct airoha_snand_dev *as_dev = spi_get_ctldata(spi);
-	struct airoha_snand_ctrl *as_ctrl;
 
-	as_ctrl = spi_controller_get_devdata(spi->controller);
 	dma_unmap_single(as_ctrl->dev, as_dev->dma_addr,
 			 as_dev->buf_len, DMA_BIDIRECTIONAL);
 	spi_set_ctldata(spi, NULL);
@@ -1049,12 +1044,6 @@
 	.max_register	= REG_SPI_NFI_SNF_NFI_CNFG,
 };
 
-static const struct of_device_id airoha_snand_ids[] = {
-	{ .compatible	= "airoha,en7581-snand" },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, airoha_snand_ids);
-
 static int airoha_snand_probe(struct platform_device *pdev)
 {
 	struct airoha_snand_ctrl *as_ctrl;
@@ -1114,6 +1103,12 @@
 	return devm_spi_register_controller(dev, ctrl);
 }
 
+static const struct of_device_id airoha_snand_ids[] = {
+	{ .compatible	= "airoha,en7581-snand" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, airoha_snand_ids);
+
 static struct platform_driver airoha_snand_driver = {
 	.driver = {
 		.name = "airoha-spi",

As you can see the only differences are your comments about patch v5 3/3:
- initialize struct airoha_snand_ctrl *as_ctrl initialization as we did
  for struct airoha_snand_dev *as_dev pointer.
- move struct of_device_id airoha_snand_ids below airoha_snand_probe routine.

My question is if we can drop those changes (they are just cosmetics) or do
you prefer to add them as incremental patch.

Regards,
Lorenzo

> 
> -- 
> With Best Regards,
> Andy Shevchenko

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]