Re: [PATCH 4/5] mmc: omap_hsmmc: cleanup the bitmap definitions of Interrupt Register

[Venkatraman S <svenkatr@ti.com>]

On Wed, Nov 7, 2012 at 6:54 PM, Balaji T K <balajitk@ti.com> wrote:
> On Tuesday 06 November 2012 10:22 PM, Venkatraman S wrote:
>>
>> Define the most frequently used bitmasks of the Interrupt Enable /
>> Interrupt Status register with consistent naming ( with _EN suffix).
>>
>> Use meaningful concatenation of bitfields for INT_EN_MASK, which shows
>> which interrupts are enabled by default.
>> No functional changes.
>>
>> Signed-off-by: Venkatraman S <svenkatr@ti.com>
>
>
> Hi Venkat,
> Not sure if you had chance to look into my comments on Version 2 of this
> patch
>
>
>> Acked-by: Felipe Balbi <balbi@ti.com>
>> ---
>>   drivers/mmc/host/omap_hsmmc.c | 54
>> +++++++++++++++++++++++++------------------
>>   1 file changed, 31 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
>> index e91e85a..d16ef0f 100644
>> --- a/drivers/mmc/host/omap_hsmmc.c
>> +++ b/drivers/mmc/host/omap_hsmmc.c
>> @@ -80,29 +80,17 @@
>>   #define CLKD_SHIFT            6
>>   #define DTO_MASK              0x000F0000
>>   #define DTO_SHIFT             16
>> -#define INT_EN_MASK            0x307F0033
>> -#define BWR_ENABLE             (1 << 4)
>> -#define BRR_ENABLE             (1 << 5)
>> -#define DTO_ENABLE             (1 << 20)
>>   #define INIT_STREAM           (1 << 1)
>>   #define DP_SELECT             (1 << 21)
>>   #define DDIR                  (1 << 4)
>> -#define DMA_EN                 0x1
>> +#define DMAE                   0x1
>
>
> This change is not needed or may not be part of this patch.
>
Actually its about consistency in naming convention. As part of this
patch, the IE, ISE and STAT family of registers all use the _EN suffix
for the bitfields, so this one had to have something different.

>
>>   #define MSBS                  (1 << 5)
>>   #define BCE                   (1 << 1)
>>   #define FOUR_BIT              (1 << 1)
>>   #define HSPE                  (1 << 2)
>>   #define DDR                   (1 << 19)
>>   #define DW8                   (1 << 5)
>> -#define CC                     0x1
>> -#define TC                     0x02
>>   #define OD                    0x1
>> -#define ERR                    (1 << 15)
>> -#define CMD_TIMEOUT            (1 << 16)
>> -#define DATA_TIMEOUT           (1 << 20)
>> -#define CMD_CRC                        (1 << 17)
>> -#define DATA_CRC               (1 << 21)
>> -#define CARD_ERR               (1 << 28)
>>   #define STAT_CLEAR            0xFFFFFFFF
>>   #define INIT_STREAM_CMD               0x00000000
>>   #define DUAL_VOLT_OCR_BIT     7
>> @@ -111,6 +99,26 @@
>>   #define SOFTRESET             (1 << 1)
>>   #define RESETDONE             (1 << 0)
>>
>> +/* Interrupt masks for IE and ISE register */
>> +#define CC_EN                  (1 << 0)
>> +#define TC_EN                  (1 << 1)
>
>
> You might want to retain CC, TC ... which has been defined and already
> used for in many places for MMCHS_STAT instead of CC_EN. CC_EN is not
> mentioned in TRM however CC is defined as bit field in TRM Register spec.
> So It would be better to reuse the previously defined CC, TC (and other bits
> fields) for MMCHS_IE, MMCHS_ISE inorder to reduce the number of #define's.
>
>
Probably yes - One way would be to use TRM fields as is, but sometimes
it leads to conflicts
like the DMAE/DMA_EN case.

Do you think it would be better to switch to _IE suffix instead of _EN ?

>> +#define BWR_EN                 (1 << 4)
>> +#define BRR_EN                 (1 << 5)
>> +#define ERR_EN                 (1 << 15)
>
>
> ERR_EN is not applicable for Interrupt masks for IE and ISE register
>
Ok. The define would still be needed, but probably I shouldn't include
it in INT_EN_MASK then ?

>
>> +#define CTO_EN                 (1 << 16)
>> +#define CCRC_EN                        (1 << 17)
>> +#define CEB_EN                 (1 << 18)
>> +#define CIE_EN                 (1 << 19)
>> +#define DTO_EN                 (1 << 20)
>> +#define DCRC_EN                        (1 << 21)
>> +#define DEB_EN                 (1 << 22)
>> +#define CERR_EN                        (1 << 28)
>> +#define BADA_EN                        (1 << 29)
>> +
>> +#define INT_EN_MASK            (BADA_EN | CERR_EN | DEB_EN | DCRC_EN |\
>> +               DTO_EN | CIE_EN | CEB_EN | CCRC_EN | CTO_EN | ERR_EN |\
>> +               BRR_EN | BWR_EN | TC_EN | CC_EN)
>> +
>>   #define MMC_AUTOSUSPEND_DELAY 100
>>   #define MMC_TIMEOUT_MS                20
>>   #define OMAP_MMC_MIN_CLOCK    400000
>> @@ -458,13 +466,13 @@ static void omap_hsmmc_enable_irq(struct
>> omap_hsmmc_host *host,
>>         unsigned int irq_mask;
>>
>>         if (host->use_dma)
>> -               irq_mask = INT_EN_MASK & ~(BRR_ENABLE | BWR_ENABLE);
>> +               irq_mask = INT_EN_MASK & ~(BRR_EN | BWR_EN);
>>         else
>>                 irq_mask = INT_EN_MASK;
>>
>>         /* Disable timeout for erases */
>>         if (cmd->opcode == MMC_ERASE)
>> -               irq_mask &= ~DTO_ENABLE;
>> +               irq_mask &= ~DTO_EN;
>>
>>         OMAP_HSMMC_WRITE(host->base, STAT, STAT_CLEAR);
>>         OMAP_HSMMC_WRITE(host->base, ISE, irq_mask);
>> @@ -702,8 +710,8 @@ static void send_init_stream(struct omap_hsmmc_host
>> *host)
>>         OMAP_HSMMC_WRITE(host->base, CMD, INIT_STREAM_CMD);
>>
>>         timeout = jiffies + msecs_to_jiffies(MMC_TIMEOUT_MS);
>> -       while ((reg != CC) && time_before(jiffies, timeout))
>> -               reg = OMAP_HSMMC_READ(host->base, STAT) & CC;
>> +       while ((reg != CC_EN) && time_before(jiffies, timeout))
>> +               reg = OMAP_HSMMC_READ(host->base, STAT) & CC_EN;
>
>
> By reusing already defined bit fields documented in TRM,
> this change and other changes below will not be needed thereby keeping
> the patch smaller with less changes and yet removing the magic bit mask
> numbers
>
I don't think the size of the patch is a constraint or a limitation. The overall
intent it to move towards a consistent naming that leads to lesser mistakes
when making patches (using the DMAE bit in the wrong register, for example).

>
>>
>>         OMAP_HSMMC_WRITE(host->base, CON,
>>                 OMAP_HSMMC_READ(host->base, CON) & ~INIT_STREAM);
>> @@ -794,7 +802,7 @@ omap_hsmmc_start_command(struct omap_hsmmc_host *host,
>> struct mmc_command *cmd,
>>         }
>>
>>         if (host->use_dma)
>> -               cmdreg |= DMA_EN;
>> +               cmdreg |= DMAE;
>
>
> DMA_EN for DMA enable is much more readable than DMAE.
>
I can't challenge or accept this "readability" claim, but as I said above,
consistency wins. _EN is for IE/ISE register, so this one has to change.
If I apply your 'TRM'  rule, then this should be 'DE' then, which is a lot less
readable.

>
>>         host->req_in_progress = 1;
>>
>> @@ -1014,11 +1022,11 @@ static void omap_hsmmc_do_irq(struct
>> omap_hsmmc_host *host, int status)
>>         data = host->data;
>>         dev_vdbg(mmc_dev(host->mmc), "IRQ Status is %x\n", status);
>>
>> -       if (status & ERR) {
>> +       if (status & ERR_EN) {
>
>
> ERR can be retained.
> This change will not be needed.
>
The individual changes are needed if it's agreed that consistent
naming is better,
and if the _EN suffix makes sense.

>
>>                 omap_hsmmc_dbg_report_irq(host, status);
>> -               if (status & (CMD_TIMEOUT | DATA_TIMEOUT))
>> +               if (status & (CTO_EN | DTO_EN))
>
>
> If you really want to use uniform naming you can rename CMD_TIMEOUT,
> DATA_TIMEOUT to CTO, DTO ....
>
>
>>                         hsmmc_command_incomplete(host, -ETIMEDOUT);
>> -               else if (status & (CMD_CRC | DATA_CRC))
>> +               else if (status & (CCRC_EN | DCRC_EN))
>>                         hsmmc_command_incomplete(host, -EILSEQ);
>>
>>                 if (host->cmd)
>> @@ -1029,9 +1037,9 @@ static void omap_hsmmc_do_irq(struct omap_hsmmc_host
>> *host, int status)
>>                 }
>>         }
>>
>> -       if (end_cmd || ((status & CC) && host->cmd))
>> +       if (end_cmd || ((status & CC_EN) && host->cmd))
>
>
> CC can be retained.
> This change will not be needed.
>
>
>>                 omap_hsmmc_cmd_done(host, host->cmd);
>> -       if ((end_trans || (status & TC)) && host->mrq)
>> +       if ((end_trans || (status & TC_EN)) && host->mrq)
>
>
> TC can be retained.
> This change will not be needed.
>
>>                 omap_hsmmc_xfer_done(host, data);
>>   }
>>
>>