On 9 March 2018 at 14:51, Paul Cercueil <paul@xxxxxxxxxxxxxxx> wrote:
> Hi,
>
>
> Le 2018-03-09 16:12, Ezequiel Garcia a écrit :
>>
>> From: Alex Smith <alex.smith@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
>> [Ezequiel: rebase and introduce register accessors]
>> Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx>
>> + * 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@xxxxxxxx> 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
