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

[Romain Izard]

Hi Boris,

2016-01-13 19:14 GMT+01:00 Boris Brezillon <boris.brezillon at free-electrons.com>:
>> 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?
>
>> +     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.

>> +};
>> +
>>  /* 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.

>>  };
>>  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.


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