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

[Andreas Färber]

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