Re: [PATCH 06/19] irqchip: atmel-aic: introduce register data structure

[Milo Kim <milo.kim@ti.com>]

On 01/04/2016 05:53 PM, Boris Brezillon wrote:
> On Mon, 4 Jan 2016 13:28:30 +0900
> Milo Kim <milo.kim@ti.com> wrote:
>
>> Structure, 'aic_reg_offset' describes for device specific register offset.
>> Each offset is used for IRQ chip operation. AIC and AIC5 have different
>> register values, but the structure can be shared.
>>
>> Please note that this is not complete patch, it's a preceding step for
>> making unified AIC driver.
>>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Jason Cooper <jason@lakedaemon.net>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>> Cc: Boris BREZILLON <boris.brezillon@free-electrons.com>
>> Cc: Ludovic Desroches <ludovic.desroches@atmel.com>
>> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Milo Kim <milo.kim@ti.com>
>> ---
>>   drivers/irqchip/irq-atmel-aic-common.c | 91 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 91 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-atmel-aic-common.c b/drivers/irqchip/irq-atmel-aic-common.c
>> index 5effd52..f840165 100644
>> --- a/drivers/irqchip/irq-atmel-aic-common.c
>> +++ b/drivers/irqchip/irq-atmel-aic-common.c
>> @@ -24,6 +24,32 @@
>>
>>   #include "irq-atmel-aic-common.h"
>>
>> +#define AT91_AIC_SMR_BASE		0
>> +#define AT91_AIC_SVR_BASE		0x80
>> +#define AT91_AIC_IVR			0x100
>> +#define AT91_AIC_ISR			0x108
>> +#define AT91_AIC_IECR			0x120
>> +#define AT91_AIC_IDCR			0x124
>> +#define AT91_AIC_ICCR			0x128
>> +#define AT91_AIC_ISCR			0x12c
>> +#define AT91_AIC_EOICR			0x130
>> +#define AT91_AIC_SPU			0x134
>> +#define AT91_AIC_DCR			0x138
>> +#define AT91_INVALID_OFFSET		(-1)
>> +
>> +#define AT91_AIC5_SSR			0x0
>> +#define AT91_AIC5_SMR			0x4
>> +#define AT91_AIC5_SVR			0x8
>> +#define AT91_AIC5_IVR			0x10
>> +#define AT91_AIC5_ISR			0x18
>> +#define AT91_AIC5_EOICR			0x38
>> +#define AT91_AIC5_SPU			0x3c
>> +#define AT91_AIC5_IECR			0x40
>> +#define AT91_AIC5_IDCR			0x44
>> +#define AT91_AIC5_ICCR			0x48
>> +#define AT91_AIC5_ISCR			0x4c
>> +#define AT91_AIC5_DCR			0x6c
>> +
>>   #define AT91_AIC_PRIOR			GENMASK(2, 0)
>>   #define AT91_AIC_IRQ_MIN_PRIORITY	0
>>   #define AT91_AIC_IRQ_MAX_PRIORITY	7
>> @@ -38,6 +64,71 @@ struct aic_chip_data {
>>   	u32 ext_irqs;
>>   };
>>
>> +/**
>> + * struct aic_reg_offset
>> + *
>> + * @eoi:	End of interrupt command register
>> + * @smr:	Source mode register
>> + * @ssr:	Source select register
>> + * @iscr:	Interrupt set command register
>> + * @idcr:	Interrupt disable command register
>> + * @iccr:	Interrupt clear command register
>> + * @iecr:	Interrupt enable command register
>> + * @spu:	Spurious interrupt vector register
>> + * @dcr:	Debug control register
>> + * @svr:	Source vector register
>> + * @ivr:	Interrupt vector register
>> + * @isr:	Interrupt status register
>> + *
>> + * Each value means register offset.
>> + */
>> +struct aic_reg_offset {
>> +	int eoi;
>> +	int smr;
>> +	int ssr;
>> +	int iscr;
>> +	int idcr;
>> +	int iccr;
>> +	int iecr;
>> +	int spu;
>> +	int dcr;
>> +	int svr;
>> +	int ivr;
>> +	int isr;
>> +};
>> +
>> +static const struct aic_reg_offset aic_regs = {
>> +	.eoi	= AT91_AIC_EOICR,
>> +	.smr	= AT91_AIC_SMR_BASE,
>> +	.ssr	= AT91_INVALID_OFFSET,	/* No SSR exists */
>> +	.iscr	= AT91_AIC_ISCR,
>> +	.idcr	= AT91_AIC_IDCR,
>> +	.iccr	= AT91_AIC_ICCR,
>> +	.iecr	= AT91_AIC_IECR,
>> +	.spu	= AT91_AIC_SPU,
>> +	.dcr	= AT91_AIC_DCR,
>> +	.svr	= AT91_AIC_SVR_BASE,
>> +	.ivr	= AT91_AIC_IVR,
>> +	.isr	= AT91_AIC_ISR,
>> +};
>> +
>> +static const struct aic_reg_offset aic5_regs = {
>> +	.eoi	= AT91_AIC5_EOICR,
>> +	.smr	= AT91_AIC5_SMR,
>> +	.ssr	= AT91_AIC5_SSR,
>> +	.iscr	= AT91_AIC5_ISCR,
>> +	.idcr	= AT91_AIC5_IDCR,
>> +	.iccr	= AT91_AIC5_ICCR,
>> +	.iecr	= AT91_AIC5_IECR,
>> +	.spu	= AT91_AIC5_SPU,
>> +	.dcr	= AT91_AIC5_DCR,
>> +	.svr	= AT91_AIC5_SVR,
>> +	.ivr	= AT91_AIC5_IVR,
>> +	.isr	= AT91_AIC5_ISR,
>> +};
>> +
>> +static const struct aic_reg_offset *aic_reg_data;
>> +
>
> Can we avoid adding this global variable: this information can probably
> be added to the aic_chip_data struct.

Chip private data, 'aic_chip_data' is allocated per each chip. If we 
move aic_reg_data into aic_chip_data, then this data will be allocated 
in each IRQ chip data as well. Then, AIC5 will allocate several register 
data. (nchips >= 2 in AIC5)

         aic = kcalloc(nchips, sizeof(*aic), GFP_KERNEL);
         if (!aic) {
                 ret = -ENOMEM;
                 goto err_iounmap;
         }

No need to use multiple register data.

Best regards,
Milo