From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:36160) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S887W-0004XN-Ri for qemu-devel@nongnu.org; Thu, 15 Mar 2012 06:41:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S887Q-0007oI-39 for qemu-devel@nongnu.org; Thu, 15 Mar 2012 06:41:22 -0400 Received: from mailout3.w1.samsung.com ([210.118.77.13]:55407) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S887P-0007nZ-QL for qemu-devel@nongnu.org; Thu, 15 Mar 2012 06:41:16 -0400 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 <0M0X002EI9OLLW70@mailout3.w1.samsung.com> for qemu-devel@nongnu.org; Thu, 15 Mar 2012 10:41:09 +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 <0M0X000XR9OKX3@spt1.w1.samsung.com> for qemu-devel@nongnu.org; Thu, 15 Mar 2012 10:41:09 +0000 (GMT) Date: Thu, 15 Mar 2012 14:41:10 +0400 From: Igor Mitsyanko In-reply-to: <4F61BAD5.9030602@suse.de> Message-id: <4F61C746.40108@samsung.com> MIME-version: 1.0 Content-type: text/plain; charset=ISO-8859-15; format=flowed Content-transfer-encoding: QUOTED-PRINTABLE References: <1331796924-30669-1-git-send-email-i.mitsyanko@samsung.com> <1331796924-30669-4-git-send-email-i.mitsyanko@samsung.com> <4F61BAD5.9030602@suse.de> Subject: Re: [Qemu-devel] [PATCH V2 3/3] hw: add Atmel maxtouch touchscreen implementation Reply-To: i.mitsyanko@samsung.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-15?Q?Andreas_F=E4rber?= Cc: peter.maydell@linaro.org, e.voevodin@samsung.com, qemu-devel@nongnu.org, kyungmin.park@samsung.com, d.solodkiy@samsung.com, m.kozlov@samsung.com On 03/15/2012 01:48 PM, Andreas F=E4rber wrote: > Am 15.03.2012 08:35, schrieb Igor Mitsyanko: >> And use it for exynos4210 NURI board emulation >> >> Signed-off-by: Igor Mitsyanko > > Thanks for moving this to libhw. Looks okay, not knowing I2C or the > device and assuming you've tested it; some nitpicks below. > >> diff --git a/hw/exynos4_boards.c b/hw/exynos4_boards.c >> index b438361..f851026 100644 >> --- a/hw/exynos4_boards.c >> +++ b/hw/exynos4_boards.c > >> @@ -134,9 +136,12 @@ static void nuri_init(ram_addr_t ram_size, >> const char *kernel_filename, const char *kernel_cmdline, >> const char *initrd_filename, const char *cpu_model) >> { >> - exynos4_boards_init_common(kernel_filename, kernel_cmdline, >> - initrd_filename, EXYNOS4_BOARD_NURI); >> - >> + Exynos4210State *cpu =3D exynos4_boards_init_common(kernel_fi= lename, >> + kernel_cmdline, initrd_filename, EXYNOS4_BOARD_NURI); > > I'd advise against using "cpu" for what looks like board or SoC sta= te to > avoid confusion. "cpu" is going to be used for CPUState instances, = i.e. > one day: ARMCPU *cpu =3D cpu_arm_init("cortex-a9"); > > I'd suggest s (generically for state) here. Right, and also we already use s in SMDK board init function. >> + DeviceState *dev =3D >> + i2c_create_slave(cpu->i2c_if[3], "maxtouch.var0", MAXTOUC= H_TS_I2C_ADDR); >> + qdev_connect_gpio_out(dev, 0, qdev_get_gpio_in(cpu->gpio2x, >> + EXYNOS4210_GPIO2X_LINE(GPX0, 4))); >> arm_load_kernel(first_cpu,&exynos4_board_binfo); >> } >> >> diff --git a/hw/maxtouch.c b/hw/maxtouch.c >> new file mode 100644 >> index 0000000..0c37d30 >> --- /dev/null >> +++ b/hw/maxtouch.c >> @@ -0,0 +1,1079 @@ >> +/* >> + * Atmel maXTouch touchscreen emulation >> + * >> + * Copyright (c) 2012 Samsung Electronics Co., Ltd. All rights r= eserved. > > It's been remarked that "All rights reserved." conflicts with the G= PL's > copyleft. Please drop that sentence if you can. Ok >> + * Igor Mitsyanko >> + * >> + * This program is free software; you can redistribute it and/or= modify it >> + * under the terms of the GNU General Public License as publishe= d by the >> + * Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be usefu= l, but WITHOUT >> + * ANY WARRANTY; without even the implied warranty of MERCHANTAB= ILITY or >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public = License >> + * for more details. >> + * >> + * You should have received a copy of the GNU General Public Lic= ense along >> + * with this program; if not, see. >> + * >> + */ > >> +/* Object types */ >> +enum { >> + MXT_GEN_MESSAGE_T5 =3D 0, MXT_GEN_COMMAND_T6, >> + MXT_GEN_POWER_T7, MXT_GEN_ACQUIRE_T8, >> + MXT_TOUCH_MULTI_T9, MXT_TOUCH_KEYARRAY_T15, >> + MXT_SPT_COMMSCONFIG_T18, MXT_SPT_GPIOPWM_T19, >> + MXT_PROCI_GRIPFACE_T20, MXT_PROCG_NOISE_T22, >> + MXT_TOUCH_PROXIMITY_T23, MXT_PROCI_ONETOUCH_T24, >> + MXT_SPT_SELFTEST_T25, MXT_PROCI_TWOTOUCH_T27, >> + MXT_SPT_CTECONFIG_T28, MXT_DEBUG_DIAGNOSTIC_T37, >> + MXT_NUM_OF_OBJECT_TYPES >> +}; > > Personally I find it easier to read a mostly monotonically increasi= ng > enum (or array below) when it's not in two columns. Was this copied= from > some Linux header as is so that we wouldn't want to reformat it to = allow > for easy updates, or was this a design choice of yours? It was my attempt to reduce amount of time needed to scroll this file= =20 down, I don't mind reformatting it to one column. >> +#define TYPE_MAXTOUCH "maxtouch" >> +#define MXT(obj) \ >> + OBJECT_CHECK(MXTState, (obj), TYPE_MAXTOUCH) >> +#define MXT_CLASS(klass) \ >> + OBJECT_CLASS_CHECK(MXTClass, (klass), TYPE_MAXTOUCH) >> +#define MXT_GET_CLASS(obj) \ >> + OBJECT_GET_CLASS(MXTClass, (obj), TYPE_MAXTOUCH) > >> +static const uint8_t mxt_variant0_info[MXT_INFO_SIZE] =3D { >> + [MXT_FAMILY_ID] =3D 0x80, >> + [MXT_VARIANT_ID] =3D 0x0, >> + [MXT_VERSION] =3D 0x1, >> + [MXT_BUILD] =3D 0x1, >> + [MXT_MATRIX_X_SIZE] =3D 16, >> + [MXT_MATRIX_Y_SIZE] =3D 14, >> + [MXT_OBJ_NUM] =3D sizeof(mxt_variant0_objlist) / sizeof(ObjCo= nfig), > > We have an ARRAY_SIZE() macro for this. OK >> +}; > >> +static const uint8_t mxt_variant1_info[MXT_INFO_SIZE] =3D { >> + [MXT_FAMILY_ID] =3D 0x80, >> + [MXT_VARIANT_ID] =3D 0x1, >> + [MXT_VERSION] =3D 0x1, >> + [MXT_BUILD] =3D 0x1, >> + [MXT_MATRIX_X_SIZE] =3D 16, >> + [MXT_MATRIX_Y_SIZE] =3D 14, >> + [MXT_OBJ_NUM] =3D sizeof(mxt_variant1_objlist) / sizeof(ObjCo= nfig), > > Dito. > >> +}; >> + >> +static const MXTVariantInfo mxt_variants_info_array[] =3D { >> + { >> + .name =3D TYPE_MXT_VARIANT0, >> + .mxt_variant_info =3D mxt_variant0_info, >> + .mxt_variant_obj_list =3D mxt_variant0_objlist >> + }, >> + { >> + .name =3D TYPE_MXT_VARIANT1, >> + .mxt_variant_info =3D mxt_variant1_info, >> + .mxt_variant_obj_list =3D mxt_variant1_objlist >> + } >> +}; >> + >> +#define MXT_NUM_OF_VARIANTS \ >> + (sizeof(mxt_variants_info_array) / sizeof(MXTVariantInfo)) > > Dito. > >> +#if MXT_DEBUG >> +#define DPRINT(fmt, args...) \ >> + do {fprintf(stderr, "QEMU MXT: "fmt, ## args); } while (0) >> +#define DPRINT_SMPL(fmt, args...) \ >> + do {fprintf(stderr, fmt, ## args); } while (0) >> +#define ERRPRINT(fmt, args...) \ >> + do {fprintf(stderr, "QEMU MXT ERROR: "fmt, ## args); } while = (0) > > Please insert a space after do {. Also somewhere below. > >> +static TypeInfo maxtouch_type_info =3D { > > Can be static const. > Ok >> + .name =3D TYPE_MAXTOUCH, >> + .parent =3D TYPE_I2C_SLAVE, >> + .instance_size =3D sizeof(MXTState), >> + .instance_init =3D mxt_init, >> + .instance_finalize =3D mxt_finalize, >> + .abstract =3D true, >> + .class_size =3D sizeof(MXTClass), >> +}; >> + >> +static void mxt_types_init(void) > > Please name it mxt_register_types by convention. > Sure Thanks for your review! --=20 Mitsyanko Igor ASWG, Moscow R&D center, Samsung Electronics email: i.mitsyanko@samsung.com