Re: [PATCH v4 2/5] drivers: mtd: nand: Add qpic_common API file

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hi,

> >> +/**
> >> + * qcom_offset_to_nandc_reg() - Get the actual offset
> >> + * @regs: pointer to nandc_reg structure
> >> + * @offset: register offset
> >> + *
> >> + * This function will reurn the actual offset for qpic controller register
> >> + */
> >> +__le32 *qcom_offset_to_nandc_reg(struct nandc_regs *regs, int offset)
> >> +{
> >> +	switch (offset) {
> >> +	case NAND_FLASH_CMD:
> >> +		return &regs->cmd;
> >> +	case NAND_ADDR0:
> >> +		return &regs->addr0;
> >> +	case NAND_ADDR1:
> >> +		return &regs->addr1;
> >> +	case NAND_FLASH_CHIP_SELECT:
> >> +		return &regs->chip_sel;
> >> +	case NAND_EXEC_CMD:
> >> +		return &regs->exec;
> >> +	case NAND_FLASH_STATUS:
> >> +		return &regs->clrflashstatus;
> >> +	case NAND_DEV0_CFG0:
> >> +		return &regs->cfg0;
> >> +	case NAND_DEV0_CFG1:
> >> +		return &regs->cfg1;
> >> +	case NAND_DEV0_ECC_CFG:
> >> +		return &regs->ecc_bch_cfg;
> >> +	case NAND_READ_STATUS:
> >> +		return &regs->clrreadstatus;
> >> +	case NAND_DEV_CMD1:
> >> +		return &regs->cmd1;
> >> +	case NAND_DEV_CMD1_RESTORE:
> >> +		return &regs->orig_cmd1;
> >> +	case NAND_DEV_CMD_VLD:
> >> +		return &regs->vld;
> >> +	case NAND_DEV_CMD_VLD_RESTORE:
> >> +		return &regs->orig_vld;
> >> +	case NAND_EBI2_ECC_BUF_CFG:
> >> +		return &regs->ecc_buf_cfg;
> >> +	case NAND_READ_LOCATION_0:
> >> +		return &regs->read_location0;
> >> +	case NAND_READ_LOCATION_1:
> >> +		return &regs->read_location1;
> >> +	case NAND_READ_LOCATION_2:
> >> +		return &regs->read_location2;
> >> +	case NAND_READ_LOCATION_3:
> >> +		return &regs->read_location3;
> >> +	case NAND_READ_LOCATION_LAST_CW_0:
> >> +		return &regs->read_location_last0;
> >> +	case NAND_READ_LOCATION_LAST_CW_1:
> >> +		return &regs->read_location_last1;
> >> +	case NAND_READ_LOCATION_LAST_CW_2:
> >> +		return &regs->read_location_last2;
> >> +	case NAND_READ_LOCATION_LAST_CW_3:
> >> +		return &regs->read_location_last3;  
> > 
> > Why do you need this indirection?  
> 
> This indirection I believe is needed by the write_reg_dma function,
> wherein a bunch of registers are modified based on a starting register.
> Can I change this in a separate cleanup series as a follow up to this?

I think it would be cleaner to make the changes I requested first and
then make a copy. I understand it is more work on your side, so if you
really prefer you can (1) make the copy and then (2) clean it all. But
please do it all in this series.

> >> diff --git a/include/linux/mtd/nand-qpic-common.h b/include/linux/mtd/nand-qpic-common.h
> >> new file mode 100644
> >> index 000000000000..aced15866627
> >> --- /dev/null
> >> +++ b/include/linux/mtd/nand-qpic-common.h
> >> @@ -0,0 +1,486 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * QCOM QPIC common APIs header file
> >> + *
> >> + * Copyright (c) 2023 Qualcomm Inc.
> >> + * Authors:     Md sadre Alam           <quic_mdalam@xxxxxxxxxxx>
> >> + *		Sricharan R             <quic_srichara@xxxxxxxxxxx>
> >> + *		Varadarajan Narayanan   <quic_varada@xxxxxxxxxxx>
> >> + *
> >> + */
> >> +#ifndef __MTD_NAND_QPIC_COMMON_H__
> >> +#define __MTD_NAND_QPIC_COMMON_H__
> >> +
> >> +#include <linux/bitops.h>
> >> +#include <linux/clk.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/dmaengine.h>
> >> +#include <linux/dma-mapping.h>
> >> +#include <linux/dma/qcom_adm.h>
> >> +#include <linux/dma/qcom_bam_dma.h>
> >> +#include <linux/module.h>
> >> +#include <linux/mtd/partitions.h>
> >> +#include <linux/mtd/rawnand.h>  
> > 
> > You really need this?  
> Yes , since some generic structure used here.

Which ones? If this is a common file, you probably should not.

Thanks,
Miquèl