Re: [PATCH v3] platform/x86: add support for devices with Silead touchscreens

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 22-01-17 17:49, Andy Shevchenko wrote:
> On Sun, Jan 22, 2017 at 5:25 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> On ACPI based tablets, the ACPI touchscreen node only contains info on
>> the gpio and the irq, and is missing any info on the axis. This info is
>> expected to be built into the tablet model specific version of the driver
>> shipped with the os-image for the device.
>>
>> Add support for getting the missing info from a table built into the
>> driver, using dmi data to identify which entry of the table to use and
>> add info for the CUBE iwork8 Air and Jumper EZpad mini3 tablets on which
>> this code was tested / developed.
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=187531
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> [dmitry.torokhov@gmail.com: Move to platform/x86]
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> Few nitpicks below. After addressing them
> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thank you.

> (in case Dmitry would like to push this via input subsystem)

I've addressed all your remarks, I will send a v4 shortly.

Regards,

Hans


>
>> ---
>> Changes in v2:
>> -Put the dmi code in a separate silead_dmi.c file
>> -Use device_add_properties to add the info
>> Changes in v3 (Dmitry Torokhov):
>> -Move the silead_dmi code to drivers/platform/x86
>> ---
>>  MAINTAINERS                       |   8 +++
>>  drivers/platform/x86/Kconfig      |  11 +++
>>  drivers/platform/x86/Makefile     |   1 +
>>  drivers/platform/x86/silead_dmi.c | 136 ++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 156 insertions(+)
>>  create mode 100644 drivers/platform/x86/silead_dmi.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index bdc4843..5591ac3 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11255,6 +11255,14 @@ F:     drivers/media/usb/siano/
>>  F:     drivers/media/usb/siano/
>>  F:     drivers/media/mmc/siano/
>>
>> +SILEAD TOUCHSCREEN DRIVER
>> +M:     Hans de Goede <hdegoede@redhat.com>
>> +L:     linux-input@vger.kernel.org
>> +L:     platform-driver-x86@vger.kernel.org
>> +S:     Maintained
>> +F:     drivers/input/touchscreen/silead.c
>> +F:     drivers/platform/x86/silead_dmi.c
>> +
>>  SIMPLEFB FB DRIVER
>>  M:     Hans de Goede <hdegoede@redhat.com>
>>  L:     linux-fbdev@vger.kernel.org
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index 59aa8e3..60be7bc 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -1076,4 +1076,15 @@ config MLX_CPLD_PLATFORM
>>           This driver handles hot-plug events for the power suppliers, power
>>           cables and fans on the wide range Mellanox IB and Ethernet systems.
>>
>> +config SILEAD_DMI
>> +       bool "Tablets with Silead touchscreens"
>> +       depends on ACPI && DMI && I2C && INPUT
>> +       ---help---
>> +         Certain ACPI based tablets with Silead touchscreens do not have
>> +         enough data in ACPI tables for the touchscreen driver to handle
>> +         the touchscreen properly, as OEMs expected the data to be baked
>> +         into the tablet model specific version of the driver shipped
>> +         with the os-image for the device. This option supplies the missing
>
> os -> OS
>
>> +         information. Enable this for x86 tablets with Silead touchscreens.
>> +
>>  endif # X86_PLATFORM_DEVICES
>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>> index d4111f0..840b07b 100644
>> --- a/drivers/platform/x86/Makefile
>> +++ b/drivers/platform/x86/Makefile
>> @@ -76,3 +76,4 @@ obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \
>>  obj-$(CONFIG_INTEL_PMC_CORE)    += intel_pmc_core.o
>>  obj-$(CONFIG_MLX_PLATFORM)     += mlx-platform.o
>>  obj-$(CONFIG_MLX_CPLD_PLATFORM)        += mlxcpld-hotplug.o
>> +obj-$(CONFIG_SILEAD_DMI)       += silead_dmi.o
>> diff --git a/drivers/platform/x86/silead_dmi.c b/drivers/platform/x86/silead_dmi.c
>> new file mode 100644
>> index 0000000..11b0a46
>> --- /dev/null
>> +++ b/drivers/platform/x86/silead_dmi.c
>> @@ -0,0 +1,136 @@
>> +/*
>> + * Silead touchscreen driver DMI based configuration code
>> + *
>> + * Copyright (c) 2017 Red Hat Inc.
>> + *
>> + * 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.
>> + *
>> + * Red Hat authors:
>> + * Hans de Goede <hdegoede@redhat.com>
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/device.h>
>> +#include <linux/dmi.h>
>> +#include <linux/i2c.h>
>> +#include <linux/notifier.h>
>> +#include <linux/property.h>
>> +#include <linux/string.h>
>> +
>> +struct silead_ts_dmi_data {
>> +       const char *acpi_name;
>> +       struct property_entry *properties;
>> +};
>> +
>> +static struct property_entry cube_iwork8_air_props[] = {
>> +       PROPERTY_ENTRY_U32("touchscreen-size-x", 1660),
>> +       PROPERTY_ENTRY_U32("touchscreen-size-y", 900),
>> +       PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"),
>> +       PROPERTY_ENTRY_STRING("firmware-name", "gsl3670-cube-iwork8-air.fw"),
>> +       PROPERTY_ENTRY_U32("silead,max-fingers", 10),
>> +       { }
>> +};
>> +
>> +static const struct silead_ts_dmi_data cube_iwork8_air_data = {
>> +       .acpi_name      = "MSSL1680:00",
>> +       .properties     = cube_iwork8_air_props,
>
> I hope indentation is correct and uses tabs.
>
>> +};
>> +
>> +static struct property_entry jumper_ezpad_mini3_props[] = {
>> +       PROPERTY_ENTRY_U32("touchscreen-size-x", 1700),
>> +       PROPERTY_ENTRY_U32("touchscreen-size-y", 1150),
>> +       PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"),
>> +       PROPERTY_ENTRY_STRING("firmware-name", "gsl3676-jumper-ezpad-mini3.fw"),
>> +       PROPERTY_ENTRY_U32("silead,max-fingers", 10),
>> +       { }
>> +};
>> +
>> +static const struct silead_ts_dmi_data jumper_ezpad_mini3_data = {
>> +       .acpi_name      = "MSSL1680:00",
>> +       .properties     = jumper_ezpad_mini3_props,
>
> Ditto.
>
>> +};
>> +
>> +static const struct dmi_system_id silead_ts_dmi_table[] = {
>> +       {
>> +               .ident = "CUBE iwork8 Air",
>> +               .driver_data = (void *)&cube_iwork8_air_data,
>> +               .matches = {
>> +                       DMI_MATCH(DMI_SYS_VENDOR, "cube"),
>> +                       DMI_MATCH(DMI_PRODUCT_NAME, "i1-TF"),
>> +                       DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
>> +               },
>> +       },
>> +       {
>> +               .ident = "Jumper EZpad mini3",
>> +               .driver_data = (void *)&jumper_ezpad_mini3_data,
>> +               .matches = {
>> +                       DMI_MATCH(DMI_SYS_VENDOR, "Insyde"),
>> +                       /* jumperx.T87.KFBNEEA02 with the version-nr dropped */
>> +                       DMI_MATCH(DMI_BIOS_VERSION, "jumperx.T87.KFBNEEA"),
>> +               },
>> +       },
>> +       { },
>> +};
>> +
>> +static void silead_ts_dmi_add_props(struct device *dev)
>> +{
>> +       struct i2c_client *client = to_i2c_client(dev);
>> +       const struct dmi_system_id *dmi_id;
>> +       const struct silead_ts_dmi_data *ts_data;
>> +       int error;
>> +
>> +       dmi_id = dmi_first_match(silead_ts_dmi_table);
>> +       if (dmi_id) {
>
> if (!dmi_id)
>  return;
> ?
>
>> +               ts_data = dmi_id->driver_data;
>> +               if (ACPI_COMPANION(dev) &&
>
> has_acpi_companion()
>
>> +                   !strncmp(ts_data->acpi_name, client->name, I2C_NAME_SIZE)) {
>> +                       error = device_add_properties(dev, ts_data->properties);
>> +                       if (error)
>> +                               dev_err(dev, "failed to add properties: %d\n",
>> +                                       error);
>
> And perhaps use ret or err and put this to a single line.
>
>> +               }
>> +       }
>> +}
>> +
>> +static int silead_ts_dmi_notifier_call(struct notifier_block *nb,
>> +                                      unsigned long action, void *data)
>> +{
>> +       struct device *dev = data;
>> +
>> +       switch (action) {
>> +       case BUS_NOTIFY_ADD_DEVICE:
>> +               silead_ts_dmi_add_props(dev);
>> +               break;
>> +
>> +       default:
>> +               break;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static struct notifier_block silead_ts_dmi_notifier = {
>> +       .notifier_call = silead_ts_dmi_notifier_call,
>> +};
>> +
>> +static int __init silead_ts_dmi_init(void)
>> +{
>> +       int error;
>> +
>> +       error = bus_register_notifier(&i2c_bus_type, &silead_ts_dmi_notifier);
>> +       if (error)
>> +               pr_err("%s: failed to register i2c bus notifier: %d\n",
>> +                       __func__, error);
>> +
>> +       return error;
>
> If above variable would be renamed this would follow.
>
>> +}
>> +
>> +/*
>> + * We are registering out notifier after i2c core is initialized and i2c bus
>> + * itself is ready (which happens at postcore initcall level), but before
>> + * ACPI starts enumerating devices (at subsys initcall level).
>> + */
>> +arch_initcall(silead_ts_dmi_init);
>> --
>> 2.9.3
>>
>
>
>