Re: [PATCH 08/14] mmc: jz4740: Add support for the JZ4780

[Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>]

On 9 March 2018 at 14:51, Paul Cercueil <paul@crapouillou.net> wrote:
> Hi,
>
>
> Le 2018-03-09 16:12, Ezequiel Garcia a écrit :
>>
>> From: Alex Smith <alex.smith@imgtec.com>
>>
>> Add support for the JZ4780 MMC controller to the jz47xx_mmc driver. There
>> are a few minor differences from the 4740 to the 4780 that need to be
>> handled, but otherwise the controllers behave the same. The IREG and IMASK
>> registers are expanded to 32 bits. Additionally, some error conditions are
>> now reported in both STATUS and IREG. Writing IREG before reading STATUS
>> causes the bits in STATUS to be cleared, so STATUS must be read first to
>> ensure we see and report error conditions correctly.
>>
>> Signed-off-by: Alex Smith <alex.smith@imgtec.com>
>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>> [Ezequiel: rebase and introduce register accessors]
>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>> ---
>>  drivers/mmc/host/Kconfig      |   2 +-
>>  drivers/mmc/host/jz4740_mmc.c | 111
>> ++++++++++++++++++++++++++++++++++--------
>>  2 files changed, 93 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>> index 620c2d90a646..7dd5169a2dfb 100644
>> --- a/drivers/mmc/host/Kconfig
>> +++ b/drivers/mmc/host/Kconfig
>> @@ -767,7 +767,7 @@ config MMC_SH_MMCIF
>>
>>  config MMC_JZ4740
>>         tristate "JZ4740 SD/Multimedia Card Interface support"
>> -       depends on MACH_JZ4740
>> +       depends on MACH_JZ4740 || MACH_JZ4780
>>         help
>>           This selects support for the SD/MMC controller on Ingenic JZ4740
>>           SoCs.
>> diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
>> index 7d4dcce76cd8..bb1b9114ef53 100644
>> --- a/drivers/mmc/host/jz4740_mmc.c
>> +++ b/drivers/mmc/host/jz4740_mmc.c
>> @@ -1,5 +1,7 @@
>>  /*
>>   *  Copyright (C) 2009-2010, Lars-Peter Clausen <lars@metafoo.de>
>> + *  Copyright (C) 2013, Imagination Technologies
>> + *
>>   *  JZ4740 SD/MMC controller driver
>>   *
>>   *  This program is free software; you can redistribute  it and/or modify
>> it
>> @@ -52,6 +54,7 @@
>>  #define JZ_REG_MMC_RESP_FIFO   0x34
>>  #define JZ_REG_MMC_RXFIFO      0x38
>>  #define JZ_REG_MMC_TXFIFO      0x3C
>> +#define JZ_REG_MMC_DMAC                0x44
>>
>>  #define JZ_MMC_STRPCL_EXIT_MULTIPLE BIT(7)
>>  #define JZ_MMC_STRPCL_EXIT_TRANSFER BIT(6)
>> @@ -105,11 +108,15 @@
>>  #define JZ_MMC_IRQ_PRG_DONE BIT(1)
>>  #define JZ_MMC_IRQ_DATA_TRAN_DONE BIT(0)
>>
>> +#define JZ_MMC_DMAC_DMA_SEL BIT(1)
>> +#define JZ_MMC_DMAC_DMA_EN BIT(0)
>>
>>  #define JZ_MMC_CLK_RATE 24000000
>>
>>  enum jz4740_mmc_version {
>>         JZ_MMC_JZ4740,
>> +       JZ_MMC_JZ4750,
>> +       JZ_MMC_JZ4780,
>>  };
>>
>>  enum jz4740_mmc_state {
>> @@ -144,7 +151,7 @@ struct jz4740_mmc_host {
>>
>>         uint32_t cmdat;
>>
>> -       uint16_t irq_mask;
>> +       uint32_t irq_mask;
>>
>>         spinlock_t lock;
>>
>> @@ -164,8 +171,46 @@ struct jz4740_mmc_host {
>>   * trigger is when data words in MSC_TXFIFO is < 8.
>>   */
>>  #define JZ4740_MMC_FIFO_HALF_SIZE 8
>> +
>> +       void (*write_irq_mask)(struct jz4740_mmc_host *host, uint32_t
>> val);
>> +       void (*write_irq_reg)(struct jz4740_mmc_host *host, uint32_t val);
>> +       uint32_t (*read_irq_reg)(struct jz4740_mmc_host *host);
>>  };
>
>
> I'm not 100% fan about the callback idea, the original commit
> (https://github.com/gcwnow/linux/commit/62472091) doesn't use them and is
> 30 lines shorter.
>
> I'm not terribly against either, so if nobody else bug on that, feel free
> to ignore my comment.
>

On 9 March 2018 at 19:31, James Hogan <james.hogan@mips.com> wrote:
>
> I was thinking the same as Paul too to be honest. Unless there is a
> measurable benefit to having callbacks, I think its cleaner and more
> readable to stick to conditionals and normal abstractions where
> appropriate.

Well, I believe the benefit of having callbacks is scalability and readability,
but perhaps it's only a matter of taste. Once the helpers are there,
adding a new one is just adding the callbacks.

I've always thought this:

static uint32_t jz4740_mmc_read_irq_reg(struct jz4740_mmc_host *host)
{
        return readw(host->base + JZ_REG_MMC_IREG);
}

static uint32_t jz4780_mmc_read_irq_reg(struct jz4740_mmc_host *host)
{
        return readl(host->base + JZ_REG_MMC_IREG);
}

looks so much better than this:

static uint32_t jz_mmc_read_irq_reg(struct jz4740_mmc_host *host)
{
        if (host->version == JZ_MMC_JZ4780) {
                return readl(host->base + JZ_REG_MMC_IREG);
        } else if ... {
                return readw(host->base + JZ_REG_MMC_IREG);
        }
}

In any case, if you guys are strong about the if-else-ism,
I'm fine changing it.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar