Re: [PATCH v1 2/5] mtd: atmel_nand: Support variable RB_EDGE interrupts

Hi Boris,

2016-01-13 19:14 GMT+01:00 Boris Brezillon [off-list ref]:

quoted

diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
index 9d71f9e6a8de..e5d7e7e63f49 100644
--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c

@@ -67,6 +67,10 @@ struct atmel_nand_caps {
      bool pmecc_correct_erase_page;
 };

+struct atmel_nand_nfc_priv {

Can you rename this struct into atmel_nfc_caps to be consistant with the
atmel_nand_caps?

quoted

+     uint32_t rb_edge;

Actually, AFAIU, it's not encoding the type of edges, but the available
native R/B lines (by native I mean the R/B lines directly connected to
the NFC IP).
I suggest renaming this field avail_rb_lines, and making it a bitfield
(one bit per possible R/B line).

It's already a bitfield in the end: it's the mask for the interrupt status
register. It might have been better if it were called rb_edge_mask.

quoted

+};
+
 /* oob layout for large page size
  * bad block info is on bytes 0 and 1
  * the bytes have to be consecutives to avoid

@@ -111,6 +115,7 @@ struct atmel_nfc {
      /* Point to the sram bank which include readed data via NFC */
      void                    *data_in_sram;
      bool                    will_write_sram;
+     uint32_t                rb_edge;

Replace this by
        const struct atmel_nfc_caps *caps;

so that next time you have a per-SoC particularity, you can just add it
to your struct atmel_nfc_caps without adding new fields to atmel_nfc.

I'm reluctant about this, but I will do it. For me, we are exchanging
a single probe-time copy to indirections in each interrupt handler for some
unspecified future user. Let's hope the tradeoff is worth it.

quoted

 };
 static struct atmel_nfc      nand_nfc;

@@ -1675,9 +1680,9 @@ static irqreturn_t hsmc_interrupt(int irq, void *dev_id)
              nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_XFR_DONE);
              ret = IRQ_HANDLED;
      }
-     if (pending & NFC_SR_RB_EDGE) {
+     if (pending & host->nfc->rb_edge) {
              complete(&host->nfc->comp_ready);
-             nfc_writel(host->nfc->hsmc_regs, IDR, NFC_SR_RB_EDGE);
+             nfc_writel(host->nfc->hsmc_regs, IDR, host->nfc->rb_edge);

How about creating a macro that would generate the appropriate bitmask
for you?

Something like

#define NFC_SR_RB_EDGE_MASK(x)  ((x) << 24)

It's already so verbose, it will make everything longer. Once the member name
contains the 'mask' part, all the information will be there.

quoted

diff --git a/drivers/mtd/nand/atmel_nand_nfc.h b/drivers/mtd/nand/atmel_nand_nfc.h
index 4d5d26221a7e..2cd9c57b1e53 100644
--- a/drivers/mtd/nand/atmel_nand_nfc.h
+++ b/drivers/mtd/nand/atmel_nand_nfc.h

@@ -42,7 +42,10 @@
 #define              NFC_SR_UNDEF            (1 << 21)
 #define              NFC_SR_AWB              (1 << 22)
 #define              NFC_SR_ASE              (1 << 23)
-#define              NFC_SR_RB_EDGE          (1 << 24)
+#define              NFC_SR_RB_EDGE0         (1 << 24)
+#define              NFC_SR_RB_EDGE1         (1 << 25)
+#define              NFC_SR_RB_EDGE2         (1 << 26)
+#define              NFC_SR_RB_EDGE3         (1 << 27)

Please replace those 4 definitions by:

#define                 NFC_SR_RB_EDGE(x)       BIT(x + 24)

The whole file could be rewritten in this form, but I see this as a
zero-value proposition. I believe it is best to keep with local
consistency. In fact, I will rather remove RB_EDGE1 & RB_EDGE2, as
those are not mentioned in the datasheets.


Best regards,
--
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