Re: [Qemu-devel] [PATCH V2 3/3] hw: add Atmel maxtouch touchscreen implementation

[Dmitry Solodkiy]

Dear Andreas, 

  "All rights reserved" was added by accident and going to be removed on next
update of all our patches.

Thanks,
     Dmitry Solodkiy,
     Emulator/Kernel PL, Mobile Group,
     Moscow R&D center, Samsung Electronics


-----Original Message-----
From: Andreas Färber [mailto:address@hidden 
Sent: Thursday, March 15, 2012 1:48 PM
To: Igor Mitsyanko
Cc: address@hidden; address@hidden; address@hidden;
address@hidden; address@hidden; address@hidden
Subject: Re: [Qemu-devel] [PATCH V2 3/3] hw: add Atmel maxtouch touchscreen
implementation

Am 15.03.2012 08:35, schrieb Igor Mitsyanko:
> And use it for exynos4210 NURI board emulation
> 
> Signed-off-by: Igor Mitsyanko <address@hidden>

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 = exynos4_boards_init_common(kernel_filename,
> +            kernel_cmdline, initrd_filename, EXYNOS4_BOARD_NURI);

I'd advise against using "cpu" for what looks like board or SoC state to avoid
confusion. "cpu" is going to be used for CPUState instances, i.e.
one day: ARMCPU *cpu = cpu_arm_init("cortex-a9");

I'd suggest s (generically for state) here.

> +    DeviceState *dev =
> +        i2c_create_slave(cpu->i2c_if[3], "maxtouch.var0",
MAXTOUCH_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 reserved.

It's been remarked that "All rights reserved." conflicts with the GPL's
copyleft. Please drop that sentence if you can.

> + *    Igor Mitsyanko  <address@hidden>
> + *
> + *  This program is free software; you can redistribute it and/or 
> + modify it
> + *  under the terms of the GNU General Public License as published 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 useful, 
> + but WITHOUT
> + *  ANY WARRANTY; without even the implied warranty of 
> + MERCHANTABILITY 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 License 
> + along
> + *  with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + */

> +/* Object types */
> +enum {
> +    MXT_GEN_MESSAGE_T5 = 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 increasing 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?

> +#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] = {
> +    [MXT_FAMILY_ID] = 0x80,
> +    [MXT_VARIANT_ID] = 0x0,
> +    [MXT_VERSION] = 0x1,
> +    [MXT_BUILD] = 0x1,
> +    [MXT_MATRIX_X_SIZE] = 16,
> +    [MXT_MATRIX_Y_SIZE] = 14,
> +    [MXT_OBJ_NUM] = sizeof(mxt_variant0_objlist) / sizeof(ObjConfig),

We have an ARRAY_SIZE() macro for this.

> +};

> +static const uint8_t mxt_variant1_info[MXT_INFO_SIZE] = {
> +    [MXT_FAMILY_ID] = 0x80,
> +    [MXT_VARIANT_ID] = 0x1,
> +    [MXT_VERSION] = 0x1,
> +    [MXT_BUILD] = 0x1,
> +    [MXT_MATRIX_X_SIZE] = 16,
> +    [MXT_MATRIX_Y_SIZE] = 14,
> +    [MXT_OBJ_NUM] = sizeof(mxt_variant1_objlist) / sizeof(ObjConfig),

Dito.

> +};
> +
> +static const MXTVariantInfo mxt_variants_info_array[] = {
> +    {
> +        .name = TYPE_MXT_VARIANT0,
> +        .mxt_variant_info = mxt_variant0_info,
> +        .mxt_variant_obj_list = mxt_variant0_objlist
> +    },
> +    {
> +        .name = TYPE_MXT_VARIANT1,
> +        .mxt_variant_info = mxt_variant1_info,
> +        .mxt_variant_obj_list = 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 = {

Can be static const.

> +    .name = TYPE_MAXTOUCH,
> +    .parent = TYPE_I2C_SLAVE,
> +    .instance_size = sizeof(MXTState),
> +    .instance_init = mxt_init,
> +    .instance_finalize = mxt_finalize,
> +    .abstract = true,
> +    .class_size = sizeof(MXTClass),
> +};
> +
> +static void mxt_types_init(void)

Please name it mxt_register_types by convention.

> +{
> +    unsigned i;
> +
> +    type_register_static(&maxtouch_type_info);
> +    for (i = 0; i < MXT_NUM_OF_VARIANTS; i++) {
> +        mxt_register_type(&mxt_variants_info_array[i]);
> +    }
> +}
> +
> +type_init(mxt_types_init)

Andreas

--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg