From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48186) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SALAL-0003QL-92 for qemu-devel@nongnu.org; Wed, 21 Mar 2012 09:01:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SALAA-0005Cs-NH for qemu-devel@nongnu.org; Wed, 21 Mar 2012 09:01:24 -0400 Received: from mailout3.w1.samsung.com ([210.118.77.13]:12181) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SALAA-0005CU-E0 for qemu-devel@nongnu.org; Wed, 21 Mar 2012 09:01:14 -0400 MIME-version: 1.0 Content-transfer-encoding: 7BIT Content-type: text/plain; charset=UTF-8; format=flowed Received: from euspt1 ([210.118.77.13]) by mailout3.w1.samsung.com (Sun Java(tm) System Messaging Server 6.3-8.04 (built Jul 29 2009; 32bit)) with ESMTP id <0M1800CQXK5RKS80@mailout3.w1.samsung.com> for qemu-devel@nongnu.org; Wed, 21 Mar 2012 13:01:03 +0000 (GMT) Received: from [106.109.8.162] by spt1.w1.samsung.com (iPlanet Messaging Server 5.2 Patch 2 (built Jul 14 2004)) with ESMTPA id <0M1800KWJK5WGR@spt1.w1.samsung.com> for qemu-devel@nongnu.org; Wed, 21 Mar 2012 13:01:09 +0000 (GMT) Date: Wed, 21 Mar 2012 17:01:08 +0400 From: Igor Mitsyanko In-reply-to: Message-id: <4F69D114.9070008@samsung.com> References: <1331796924-30669-1-git-send-email-i.mitsyanko@samsung.com> <1331796924-30669-3-git-send-email-i.mitsyanko@samsung.com> Subject: Re: [Qemu-devel] [PATCH V2 2/3] exynos4210: add exynos4210 GPIO implementation Reply-To: i.mitsyanko@samsung.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: qemu-devel@nongnu.org, afaerber@suse.de, d.solodkiy@samsung.com On 03/20/2012 05:27 PM, Peter Maydell wrote: > On 15 March 2012 07:35, Igor Mitsyanko wrote: >> Now that we have GPIO emulation for exynos4210 SoC we can use it to >> properly hook up IRQ line to lan9215 controller on SMDK board. > >> +#elif EXYNOS4210_GPIO_DEBUG == 1 >> +#define DPRINT_L1(fmt, args...) \ >> + do {fprintf(stderr, "QEMU GPIO: "fmt, ## args); } while (0) > > Space after the closing quote in these macros (this patch and > others), please. > >> + >> + DPRINT_L1("Input pin GPIO%s_PIN%u %s by external device\n", >> + g->ports[group_num].name, pin, (level ? "set" : "cleared")); >> + /* Set new value on corresponding gpio pin */ >> + (level) ? (g->ports[group_num].dat |= (1<< pin)) : >> + (g->ports[group_num].dat&= ~(1<< pin)); > > Don't use ?: like this please, just write it out as an if statement. > OK >> +static void exynos4_gpio_write(void *opaque, target_phys_addr_t off, >> + uint64_t value, unsigned size) >> +{ > > I find these read and write functions quite hard to read. I'm > wondering if it would work better to split them up into smaller > memory regions which get registered at the right addresses, rather > than effectively doing the decode into different subsections based > on address and g->part here? > Initially I implemented these functions splitted like you suggest, but that left me with three basically identical long switch statements, so I decided to combine them into one to achieve better code reuse. If you think code reuse is not an issue here, I'll gladly return it back to splitted implementation. >> + Exynos4GpioState *g = (Exynos4GpioState *)opaque; >> + unsigned int port_end, extint_con_start, extint_con_end; >> + unsigned int extint_flt_start, extint_flt_end; >> + unsigned int extint_mask_start, extint_mask_end; >> + unsigned int extint_pend_start, extint_pend_end; >> + unsigned int etcp_start_addr, etcp_start_idx, extint_pri_end; >> + unsigned idx; >> + >> + DPRINT_L2("GPIO%u write off 0x%x = %u(0x%x)\n", g->part, >> + (uint32_t)off, (uint32_t)value, (uint32_t)value); >> + >> + switch (g->part) { >> + case GPIO_PART2X: >> + port_end = GPIO2_XPORT_END; >> + extint_con_start = GPIO2_WKPINT_CON_START; >> + extint_con_end = GPIO2_WKPINT_CON_END; >> + extint_flt_start = GPIO2_WKPINT_FLT_START; >> + extint_flt_end = GPIO2_WKPINT_FLT_END; >> + extint_mask_start = GPIO2_WKPINT_MASK_START; >> + extint_mask_end = GPIO2_WKPINT_MASK_END; >> + extint_pend_start = GPIO2_WKPINT_PEND_START; >> + extint_pend_end = GPIO2_WKPINT_PEND_END; >> + etcp_start_addr = etcp_start_idx = extint_pri_end = 0; >> + break; >> + case GPIO_PART1: default: >> + port_end = GPIO_NORM_PORT_END; >> + extint_con_start = GPIO_EXTINT_CON_START; >> + extint_con_end = GPIO1_EXTINT_CON_END; >> + extint_flt_start = GPIO_EXTINT_FLT_START; >> + extint_flt_end = GPIO1_EXTINT_FLT_END; >> + extint_mask_start = GPIO_EXTINT_MASK_START; >> + extint_mask_end = GPIO1_EXTINT_MASK_END; >> + extint_pend_start = GPIO_EXTINT_PEND_START; >> + extint_pend_end = GPIO1_EXTINT_PEND_END; >> + etcp_start_addr = GPIO1_ETCPORT_START; >> + etcp_start_idx = GPIO1_NORM_PORT_NUM; >> + extint_pri_end = GPIO1_EXTINT_FIXPRI_END; >> + break; >> + case GPIO_PART2: >> + port_end = GPIO_NORM_PORT_END; >> + extint_con_start = GPIO_EXTINT_CON_START; >> + extint_con_end = GPIO2_EXTINT_CON_END; >> + extint_flt_start = GPIO_EXTINT_FLT_START; >> + extint_flt_end = GPIO2_EXTINT_FLT_END; >> + extint_mask_start = GPIO_EXTINT_MASK_START; >> + extint_mask_end = GPIO2_EXTINT_MASK_END; >> + extint_pend_start = GPIO_EXTINT_PEND_START; >> + extint_pend_end = GPIO2_EXTINT_PEND_END; >> + etcp_start_addr = GPIO2_ETCPORT_START; >> + etcp_start_idx = GPIO2_NORM_PORT_NUM; >> + extint_pri_end = GPIO2_EXTINT_FIXPRI_END; >> + break; >> + case GPIO_PART3: >> + if (off< GPIO3_NORM_PORT_END) { >> + idx = DIV_BY_PORTGR_SIZE(off); >> + exynos4_gpio_portgr_write(g, idx, MOD_PORTGR_SIZE(off), value); >> + } else { >> + DPRINT_ERROR("GPIO3 bad write off 0x%x = %u(0x%x)\n", >> + (uint32_t)off, (uint32_t)value, (uint32_t)value); >> + } >> + return; >> + } >> + >> + if (off< port_end) { >> + idx = DIV_BY_PORTGR_SIZE(off); >> + exynos4_gpio_portgr_write(g, idx, MOD_PORTGR_SIZE(off), value); >> + } else if (off>= extint_mask_start&& off< extint_mask_end) { >> + idx = (off - extint_mask_start)>> 2; >> + DPRINT_L1("GPIO%u EXTINT%u_MASK register write = %u(0x%x)\n", g->part, >> + g->port_ints[idx].int_line_num, (uint32_t)value, (uint32_t)value); >> + g->port_ints[idx].mask = value; >> + } else if (off>= extint_pend_start&& off< extint_pend_end) { >> + idx = (off - extint_pend_start)>> 2; >> + DPRINT_L1("GPIO%u EXTINT%u_PEND register write = %u(0x%x)\n", g->part, >> + g->port_ints[idx].int_line_num, (uint32_t)value, (uint32_t)value); >> + if (g->part == GPIO_PART2X) { >> + unsigned i, irq_n; >> + Exynos4Gpio2XState *g2 = EXYNOS4210_GPIO2X(g); >> + for (i = 0; i< GPIO_MAX_PIN_IN_PORT; i++) { >> + if ((g->port_ints[idx].pend& (1<< i))&& (value& (1<< i))) { >> + g->port_ints[idx].pend&= ~(1<< i); >> + irq_n = idx * GPIO_MAX_PIN_IN_PORT + i; >> + if (irq_n>= GPIO2_X_PORT_IRQ_NUM) { >> + irq_n = GPIO2_X_PORT_IRQ_NUM - 1; >> + } >> + qemu_irq_lower(g2->ext_irq[irq_n]); >> + } >> + } >> + } else { >> + g->port_ints[idx].pend&= ~value; >> + exynos4_gpioirq_update(g); >> + } >> + } else if (off>= extint_con_start&& off< extint_con_end) { >> + idx = (off - extint_con_start)>> 2; >> + DPRINT_L1("GPIO%u EXTINT%u_CON register write = %u(0x%x)\n", g->part, >> + g->port_ints[idx].int_line_num, (uint32_t)value, (uint32_t)value); >> + g->port_ints[idx].con = value; >> + } else if (off>= extint_flt_start&& off< extint_flt_end) { >> + unsigned i = ((off - extint_flt_start)>> 2)& 1; >> + idx = (off - extint_flt_start)>> 3; >> + DPRINT_L1("GPIO%u EXTINT%u_FLTCON%u reg write = %u(0x%x)\n", g->part, >> + g->port_ints[idx].int_line_num, i, (uint32_t)value, (uint32_t)value); >> + g->port_ints[idx].fltcon[i] = value; >> + } else if (g->part == GPIO_PART2X) { >> + DPRINT_ERROR("GPIO2 group X bad write off 0x%x = %u(0x%x)\n", >> + (uint32_t)off, (uint32_t)value, (uint32_t)value); >> + return; >> + } else if (off == GPIO_EXTINT_SERVICE) { >> + DPRINT_L1("GPIO%u EXT_INT_SERVICE register write = %u(0x%x)\n", >> + g->part, (uint32_t)value, (uint32_t)value); >> + EXYNOS4210_GPIO_1_2(g)->extint_serv = value; >> + } else if (off == GPIO_EXTINT_SERVICE_PEND) { >> + DPRINT_L1("GPIO%u EXT_INT_SERVICE_PEND register write = %u(0x%x)\n", >> + g->part, (uint32_t)value, (uint32_t)value); >> + EXYNOS4210_GPIO_1_2(g)->extint_serv_pend = value; >> + } else if (off == GPIO_EXTINT_GRPFIXPRI) { >> + DPRINT_L1("GPIO%u EXT_INT_GRPFIXPRI register write = %u(0x%x)\n", >> + g->part, (uint32_t)value, (uint32_t)value); >> + EXYNOS4210_GPIO_1_2(g)->extint_grpfixpri = value; >> + } else if (off>= GPIO_EXTINT_FIXPRI_START&& off< extint_pri_end) { >> + idx = (off - GPIO_EXTINT_FIXPRI_START)>> 2; >> + DPRINT_L1("GPIO%u EXTINT%u_FIXPRI register write = %u(0x%x)\n", g->part, >> + g->port_ints[idx].int_line_num, (uint32_t)value, (uint32_t)value); >> + g->port_ints[idx].fixpri = value; >> + } else if (off>= etcp_start_addr&& off< GPIO_ETCPORT_END) { >> + idx = etcp_start_idx + DIV_BY_PORTGR_SIZE(off - etcp_start_addr); >> + exynos4_etc_portgroup_write(&g->ports[idx], >> + MOD_PORTGR_SIZE(off - etcp_start_addr), value); >> + } else { >> + DPRINT_ERROR("GPIO%u bad write off 0x%x = %u(0x%x)\n", g->part, >> + (uint32_t)off, (uint32_t)value, (uint32_t)value); >> + } >> +} >> +static void exynos4_gpio1_init(Object *obj) >> +{ >> + Exynos4GpioState *g = EXYNOS4210_GPIO(obj); >> + >> + g->part = GPIO_PART1; >> + g->ports = gpio1_ports; >> + g->port_ints = gpio1_ports_interrupts; >> + g->num_of_ports = sizeof(gpio1_ports) / sizeof(Exynos4PortGroup); >> + g->num_of_portints = >> + sizeof(gpio1_ports_interrupts) / sizeof(Exynos4PortIntState); > > You can use the ARRAY_SIZE macro here. > > -- PMM > > -- Mitsyanko Igor ASWG, Moscow R&D center, Samsung Electronics email: i.mitsyanko@samsung.com