On 2017-07-10 19:40, Sricharan R wrote:

Hi,

On 7/4/2017 12:19 PM, Archit Taneja wrote:

quoted

On 06/29/2017 12:45 PM, Abhishek Sahu wrote:

quoted

The BAM has multiple flags to control the transfer. This patch
adds flags parameter in register and data transfer functions and
modifies all these function call with appropriate flags.

Signed-off-by: Abhishek Sahu <redacted>
---
  drivers/mtd/nand/qcom_nandc.c | 114
++++++++++++++++++++++++------------------
  1 file changed, 65 insertions(+), 49 deletions(-)

diff --git a/drivers/mtd/nand/qcom_nandc.c

b/drivers/mtd/nand/qcom_nandc.c
index 7042a65..65c9059 100644

--- a/drivers/mtd/nand/qcom_nandc.c
+++ b/drivers/mtd/nand/qcom_nandc.c

@@ -170,6 +170,14 @@
  #define    ECC_BCH_4BIT    BIT(2)
  #define    ECC_BCH_8BIT    BIT(3)
  +/* Flags used for BAM DMA desc preparation*/
+/* Don't set the EOT in current tx sgl */
+#define NAND_BAM_NO_EOT            (0x0001)
+/* Set the NWD flag in current sgl */
+#define NAND_BAM_NWD            (0x0002)
+/* Finish writing in the current sgl and start writing in another 

sgl */
+#define NAND_BAM_NEXT_SGL        (0x0004)
+
  #define QPIC_PER_CW_MAX_CMD_ELEMENTS    (32)
  #define QPIC_PER_CW_MAX_CMD_SGL        (32)
  #define QPIC_PER_CW_MAX_DATA_SGL    (8)

  I will remove the braces and use the bit macros.

quoted

quoted

@@ -712,7 +720,7 @@ static int prep_dma_desc(struct 

qcom_nand_controller
*nandc, bool read,
   * @num_regs:        number of registers to read
   */
  static int read_reg_dma(struct qcom_nand_controller *nandc, int 
first,
-            int num_regs)
+            int num_regs, unsigned int flags)
  {
      bool flow_control = false;
      void *vaddr;

@@ -736,7 +744,7 @@ static int read_reg_dma(struct 

qcom_nand_controller
*nandc, int first,
   * @num_regs:        number of registers to write
   */
  static int write_reg_dma(struct qcom_nand_controller *nandc, int 
first,
-             int num_regs)
+             int num_regs, unsigned int flags)

Adding flags to read_reg_dma and write_reg_dma is making things a bit
messy. I can't
think of a better way to share the code either, though.

One thing we could consider doing is something like below. I don't 
know if
it would
make things more legible.

union nand_dma_props {
    bool adm_flow_control;
    unsigned int bam_flags;
};

config_cw_read()
{
    union nand_dma_props dma_props;
    ...
    ...

    if (is_bam)
        dma_props.bam_flags = NAND_BAM_NWD;
    else
        dma_props.adm_flow_control = false;

    write_reg_dma(nandc, NAND_EXEC_CMD, 1, &dma_props);
    ...
    ...
}

  The flags for each write_reg_dma and read_reg_dma will be different.
  Normally, for all the API's which uses flags
  (like dmaengine_prep_slave_sg), we are passing the flags directly.
  this union won't help us making this code more readable.

 Right, with this , i think we can have two different indirections for
functions like,
 prep_dma_desc_command and prep_dma_desc. That will help to reduce the
bam_dma_enabled
 checks.

  Since common code changes are intermixed with bam_dma_enabled check
  so taking function pointer won't help much in making code more 
readable.

  anyway, I will analyze the final code for v2 and will check the
  possibility of using function pointers.

Regards,
 Sricharan

-- 
Abhishek Sahu
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html