Re: [PATCH 2/2] INPUT/HID: add touch support for SiS touch driver

[Benjamin Tissoires]

On Mon, Feb 16, 2015 at 10:10 AM, Oliver Neukum <oneukum@xxxxxxx> wrote:
> On Thu, 2015-02-12 at 15:24 +0800, æåè (tammy_tseng) wrote:
>
>> Sorry. Re-send the code diff again.
>> Here is the hid touch driver for sis touch controller.
>> Thanks.
>
> This driver does very strange things. It looks like you are
> simulating a disconnect() to the usbhid driver currently driving
> the device. This is unacceptable. Please add the device to the
> generic blacklist and implement a clean open/close.
>

Indeed, this driver does very strange things.
At least you should explain in the commit messages why you need to
have a custom chardev in the hid tree and why using a plain HID
communication is not enough while discussing with the hardware.

Also, I am not especially happy with your way of quirking
hid-multitouch in such a specific way without external control (i.e.
not having put this in a MT_CLS).

Please split the 2 features (if I understand correctly enabling of the
multitouch mode and firmware update) in 2 separate patches.

How about not having a FW update capability in the kernel and just use
a plain hidraw user space application which would call the firmware
update?

Last, all HID drivers go through Jiri's tree, so you definitively
should add him in CC of your submissions.

Cheers,
Benjamin

>         Regards
>                 Oliver
>
>> Tammy
>> -----
>>
>> linux-3.18.5/drivers/hid/Kconfig             |  14 ++
>> linux-3.18.5/drivers/hid/hid-ids.h           |   1 +
>> linux-3.18.5/drivers/hid/hid-multitouch.c    |  16 ++
>> linux-3.18.5/drivers/hid/hid-sis_ctrl.c      | 360 +++++++++++++++++++++++++++
>> linux-3.18.5/drivers/hid/usbhid/hid-quirks.c |   1 +
>> 5 files changed, 392 insertions(+)
>>
>>
>> -----
>> diff --git a/linux-3.18.5/drivers/hid/Kconfig b/linux-3.18.5/drivers/hid/Kconfig
>> index f42df4d..2235cfe 100644
>> --- a/linux-3.18.5/drivers/hid/Kconfig
>> +++ b/linux-3.18.5/drivers/hid/Kconfig
>> @@ -496,6 +496,20 @@ config HID_MULTITOUCH
>>         To compile this driver as a module, choose M here: the
>>         module will be called hid-multitouch.
>>
>> +config HID_SIS_CTRL
>> +      bool "SiS Touch Device Controller"
>> +      depends on HID_MULTITOUCH
>> +      ---help---
>> +      Support for SiS Touch devices update FW.
>> +
>> +config DEBUG_HID_SIS_UPDATE_FW
>> +      bool "SiS Touch device debug message(update firmware)"
>> +      depends on HID_SIS_CTRL
>> +      default n
>> +      ---help---
>> +        Say Y here if you want to enable debug message(update firmware) for SiS Touch
>> +          devices. Must enable config HID_SIS_UPDATE_FW first.
>> +
>> config HID_NTRIG
>>       tristate "N-Trig touch screen"
>>       depends on USB_HID
>> diff --git a/linux-3.18.5/drivers/hid/hid-ids.h b/linux-3.18.5/drivers/hid/hid-ids.h
>> index 0e28190..b9ca441 100644
>> --- a/linux-3.18.5/drivers/hid/hid-ids.h
>> +++ b/linux-3.18.5/drivers/hid/hid-ids.h
>> @@ -809,6 +809,7 @@
>> #define USB_VENDOR_ID_SIS_TOUCH                0x0457
>> #define USB_DEVICE_ID_SIS9200_TOUCH  0x9200
>> #define USB_DEVICE_ID_SIS817_TOUCH    0x0817
>> +#define USB_DEVICE_ID_SISF817_TOUCH  0xF817
>> #define USB_DEVICE_ID_SIS_TS          0x1013
>> #define USB_DEVICE_ID_SIS1030_TOUCH  0x1030
>>
>> diff --git a/linux-3.18.5/drivers/hid/hid-multitouch.c b/linux-3.18.5/drivers/hid/hid-multitouch.c
>> index 51e25b9..11d67bc 100644
>> --- a/linux-3.18.5/drivers/hid/hid-multitouch.c
>> +++ b/linux-3.18.5/drivers/hid/hid-multitouch.c
>> @@ -54,6 +54,10 @@ MODULE_LICENSE("GPL");
>>
>>  #include "hid-ids.h"
>>
>> +#ifdef CONFIG_HID_SIS_CTRL
>> +#include "hid-sis_ctrl.c"
>> +#endif
>> +
>> /* quirks to control the device */
>> #define MT_QUIRK_NOT_SEEN_MEANS_UP      (1 << 0)
>> #define MT_QUIRK_SLOT_IS_CONTACTID   (1 << 1)
>> @@ -951,6 +955,14 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>               }
>>       }
>>
>> +#ifdef CONFIG_HID_SIS_CTRL
>> +      if (hdev->vendor == USB_VENDOR_ID_SIS_TOUCH) {
>> +              ret = sis_setup_chardev(hdev);
>> +              if (ret)
>> +                      dev_err(&hdev->dev, "sis_setup_chardev fail\n");
>> +      }
>> +#endif
>> +
>>       /* This allows the driver to correctly support devices
>>        * that emit events over several HID messages.
>>        */
>> @@ -1043,6 +1055,10 @@ static void mt_remove(struct hid_device *hdev) {
>>       sysfs_remove_group(&hdev->dev.kobj, &mt_attribute_group);
>>       hid_hw_stop(hdev);
>> +#ifdef CONFIG_HID_SIS_CTRL
>> +      if (hdev->vendor == USB_VENDOR_ID_SIS_TOUCH)
>> +              sis_deinit_chardev();
>> +#endif
>> }
>>
>>  /*
>> diff --git a/linux-3.18.5/drivers/hid/hid-sis_ctrl.c b/linux-3.18.5/drivers/hid/hid-sis_ctrl.c
>> new file mode 100644
>> index 0000000..3a7b3df
>> --- /dev/null
>> +++ b/linux-3.18.5/drivers/hid/hid-sis_ctrl.c
>> @@ -0,0 +1,360 @@
>> +/*
>> + *  Character device driver for SIS multitouch panels control
>> + *
>> + *  Copyright (c) 2009 SIS, Ltd.
>> + *
>> + */
>> +
>> +/*
>> + * 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.
>> + */
>> +
>> +#include <linux/hid.h>
>> +#include <linux/module.h>
>> +#include <linux/usb.h>
>> +#include "usbhid/usbhid.h"
>> +#include <linux/init.h>
>> +
>> +/*update FW*/
>> +#include <linux/fs.h>
>> +#include <linux/cdev.h>
>> +/*#include <asm/uaccess.h>*/   /*copy_from_user() & copy_to_user()*/
>> +#include <linux/uaccess.h>
>> +
>> +#include "hid-ids.h"
>> +
>> +#define SIS817_DEVICE_NAME "sis_aegis_hid_touch_device"
>> +#define SISF817_DEVICE_NAME "sis_aegis_hid_bridge_touch_device"
>> +
>> +#define CTRL 0
>> +#define ENDP_01 1
>> +#define ENDP_02 2
>> +#define DIR_IN 0x1
>> +
>> +#define SIS_ERR_ALLOCATE_KERNEL_MEM        -12 /* Allocate memory fail */
>> +#define SIS_ERR_COPY_FROM_USER          -14 /* Copy data from user fail */
>> +#define SIS_ERR_COPY_FROM_KERNEL      -19 /* Copy data from kernel fail */
>
> You _must_ use the symbolic names. You cannot use numbers.
>
>> +
>> +static const int sis_char_devs_count = 1;           /* device count */
>> +static int sis_char_major;
>> +static struct cdev sis_char_cdev;
>> +static struct class *sis_char_class;
>> +
>> +static struct hid_device *hid_dev_backup;        /*backup address*/
>> +static struct urb *backup_urb;
>> +
>> +#ifdef CONFIG_DEBUG_HID_SIS_UPDATE_FW
>> +      #define DBG_FW(fmt, arg...) printk(fmt, ##arg)
>> +      static void sis_dbg_dump_array(u8 *ptr, u32 size)
>> +      {
>> +              u32 i;
>> +
>> +              for (i = 0; i < size; i++) {
>> +                      DBG_FW("%02X ", ptr[i]);
>> +                      if (((i+1)&0xF) == 0)
>> +                              DBG_FW("\n");
>> +              }
>> +              if (size & 0xF)
>> +                      DBG_FW("\n");
>> +      }
>> +#else
>> +      #define DBG_FW(...)
>> +      #define sis_dbg_dump_array(...)
>> +#endif   /*CONFIG_DEBUG_HID_SIS_UPDATE_FW*/
>> +
>> +/*20120306 Yuger ioctl for tool*/
>> +static int sis_cdev_open(struct inode *inode, struct file *filp) {
>> +      struct usbhid_device *usbhid;
>> +
>> +      DBG_FW("%s\n" , __func__);
>> +      /*20110511, Yuger, kill current urb by method of usbhid_stop*/
>> +      if (!hid_dev_backup) {
>
> Why is this needed?
>
>> +              pr_info("(stop)hid_dev_backup is not initialized yet");
>> +              return -1;
>> +      }
>> +
>> +      usbhid = hid_dev_backup->driver_data;
>> +
>> +      pr_info("sys_sis_HID_stop\n");
>> +
>> +      /* printk( KERN_INFO "hid_dev_backup->vendor,
>> +      * hid_dev_backup->product = %x %x\n", hid_dev_backup->vendor,
>> +      * hid_dev_backup->product );*/
>> +
>> +      /*20110602, Yuger, fix bug: not contact usb cause kernel panic*/
>> +      if (!usbhid) {
>> +              pr_info("(stop)usbhid is not initialized yet");
>> +              return -1;
>> +      } else if (!usbhid->urbin) {
>> +              pr_info("(stop)usbhid->urbin is not initialized yet");
>> +              return -1;
>> +      } else if (hid_dev_backup->vendor == USB_VENDOR_ID_SIS_TOUCH) {
>> +              usb_fill_int_urb(backup_urb,
>> +                      usbhid->urbin->dev,
>> +                      usbhid->urbin->pipe,
>> +                      usbhid->urbin->transfer_buffer,
>> +                      usbhid->urbin->transfer_buffer_length,
>> +                      usbhid->urbin->complete,
>> +                      usbhid->urbin->context,
>> +                      usbhid->urbin->interval);
>> +              clear_bit(HID_STARTED, &usbhid->iofl);
>> +              set_bit(HID_DISCONNECTED, &usbhid->iofl);
>> +
>> +              usb_kill_urb(usbhid->urbin);
>> +              usb_free_urb(usbhid->urbin);
>> +              usbhid->urbin = NULL;
>> +              return 0;
>> +      }
>> +
>> +      pr_info("This is not a SiS device");
>> +      return -801;
>> +}
>> +
>> +static int sis_cdev_release(struct inode *inode, struct file *filp) {
>> +      /*20110505, Yuger, rebuild the urb which is at the same urb address,
>> +      * then re-submit it*/
>> +
>> +      int ret;
>> +      struct usbhid_device *usbhid;
>> +      unsigned long flags;
>> +
>> +      DBG_FW("%s: ", __func__);
>> +
>> +      if (!hid_dev_backup) {
>> +              pr_info("(stop)hid_dev_backup is not initialized yet");
>> +              return -1;
>> +      }
>> +
>> +      usbhid = hid_dev_backup->driver_data;
>> +
>> +      pr_info("sys_sis_HID_start");
>> +
>> +      if (!usbhid) {
>> +              pr_info("(start)usbhid is not initialized yet");
>> +              return -1;
>> +      }
>> +
>> +      if (!backup_urb) {
>> +              pr_info("(start)backup_urb is not initialized yet");
>> +              return -1;
>> +      }
>> +
>> +      clear_bit(HID_DISCONNECTED, &usbhid->iofl);
>> +      usbhid->urbin = usb_alloc_urb(0, GFP_KERNEL);
>> +
>> +      if (!backup_urb->interval) {
>> +              pr_info("(start)backup_urb->interval does not exist");
>> +              return -1;
>> +      }
>> +
>> +      usb_fill_int_urb(usbhid->urbin, backup_urb->dev, backup_urb->pipe,
>> +      backup_urb->transfer_buffer, backup_urb->transfer_buffer_length,
>> +      backup_urb->complete, backup_urb->context, backup_urb->interval);
>> +      usbhid->urbin->transfer_dma = usbhid->inbuf_dma;
>> +      usbhid->urbin->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
>> +
>> +      set_bit(HID_STARTED, &usbhid->iofl);
>> +
>> +      /*method at hid_start_in*/
>> +      spin_lock_irqsave(&usbhid->lock, flags);
>> +      ret = usb_submit_urb(usbhid->urbin, GFP_ATOMIC);
>> +      spin_unlock_irqrestore(&usbhid->lock, flags);
>> +      /*yy*/
>> +
>> +      DBG_FW("ret = %d", ret);
>> +
>> +      return ret;
>> +}
>> +
>> +/*SiS 817 only*/
>> +static ssize_t sis_cdev_read(struct file *file, char __user *buf,
>> +                              size_t count, loff_t *ppos) {
>> +      int actual_length = 0;
>> +      u8 *rep_data = NULL;
>> +      u32 size, timeout;
>> +      long rep_ret = 0;
>> +      struct usb_interface *intf =
>> +                      to_usb_interface(hid_dev_backup->dev.parent);
>> +      struct usb_device *dev = interface_to_usbdev(intf);
>> +
>> +      DBG_FW("%s\n", __func__);
>> +
>> +      rep_data = kzalloc(count, GFP_KERNEL);
>> +      if (!rep_data)
>> +              return SIS_ERR_ALLOCATE_KERNEL_MEM;
>> +
>> +      if (copy_from_user(rep_data, (void *)buf, count)) {
>> +              pr_err("copy_to_user fail\n");
>> +              rep_ret = SIS_ERR_COPY_FROM_USER;
>> +              goto end;
>> +      }
>> +      size = be32_to_cpup((u32 *)&rep_data[64]);
>> +      timeout = be32_to_cpup((u32 *)&rep_data[68]);
>> +
>> +      rep_ret = usb_interrupt_msg(dev, backup_urb->pipe,
>> +              rep_data, size, &actual_length, timeout);
>> +
>> +      DBG_FW("%s: rep_data = ", __func__);
>> +      sis_dbg_dump_array(rep_data, 8);
>> +
>> +      if (rep_ret == 0) {
>> +              rep_ret = actual_length;
>> +              if (copy_to_user((void *)buf, rep_data, rep_ret)) {
>> +                      pr_err("copy_to_user fail\n");
>> +                      rep_ret = SIS_ERR_COPY_FROM_KERNEL;
>> +                      goto end;
>> +              }
>> +      }
>> +      DBG_FW("%s: rep_ret = %ld\n", __func__, rep_ret);
>> +end:
>> +      /*free allocated data*/
>> +      kfree(rep_data);
>> +      return rep_ret;
>> +}
>> +
>> +static ssize_t sis_cdev_write(struct file *file, const char __user *buf,
>> +                      size_t count, loff_t *f_pos) {
>> +      int actual_length = 0;
>> +      u8 *rep_data = NULL;
>> +      long rep_ret = 0;
>> +      struct usb_interface *intf =
>> +                      to_usb_interface(hid_dev_backup->dev.parent);
>> +      struct usb_device *dev = interface_to_usbdev(intf);
>> +      struct usbhid_device *usbhid = hid_dev_backup->driver_data;
>> +      u32 size, timeout;
>> +
>> +      rep_data = kzalloc(count, GFP_KERNEL);
>> +      if (!rep_data)
>> +              return SIS_ERR_ALLOCATE_KERNEL_MEM;
>> +
>> +      if (copy_from_user(rep_data, (void *)buf, count)) {
>> +              pr_err("copy_to_user fail\n");
>> +              rep_ret = SIS_ERR_COPY_FROM_USER;
>> +              goto end;
>> +      }
>> +
>> +      size = be32_to_cpup((u32 *)&rep_data[64]);
>> +      timeout = be32_to_cpup((u32 *)&rep_data[68]);
>> +
>> +      rep_ret = usb_interrupt_msg(dev, usbhid->urbout->pipe,
>> +              rep_data, size, &actual_length, timeout);
>> +
>> +      DBG_FW("%s: rep_data = ", __func__);
>> +      sis_dbg_dump_array(rep_data, size);
>> +
>> +      if (rep_ret == 0) {
>> +              rep_ret = actual_length;
>> +              if (copy_to_user((void *)buf, rep_data, rep_ret)) {
>> +                      pr_err("copy_to_user fail\n");
>> +                      rep_ret = SIS_ERR_COPY_FROM_KERNEL;
>> +                      goto end;
>> +              }
>> +      }
>> +      DBG_FW("%s: rep_ret = %ld\n", __func__, rep_ret);
>> +      DBG_FW("End of sys_sis_HID_IO\n");
>> +end:
>> +      /*free allocated data*/
>> +      kfree(rep_data);
>> +      return rep_ret;
>> +}
>> +
>> +
>> +/*for ioctl*/
>> +static const struct file_operations sis_cdev_fops = {
>> +      .owner    = THIS_MODULE,
>> +      .read       = sis_cdev_read,
>> +      .write       = sis_cdev_write,
>> +      .open      = sis_cdev_open,
>> +      .release = sis_cdev_release,
>> +};
>> +
>> +/*for ioctl*/
>> +static int sis_setup_chardev(struct hid_device *hdev) {
>> +      dev_t dev = MKDEV(sis_char_major, 0);
>> +      int ret = 0;
>> +      struct device *class_dev = NULL;
>> +      u8 *name = (hdev->product == USB_DEVICE_ID_SISF817_TOUCH) ?
>> +                      SISF817_DEVICE_NAME : SIS817_DEVICE_NAME;
>> +
>> +      dev_info(&hdev->dev, "sis_setup_chardev.\n");
>> +
>> +      backup_urb = usb_alloc_urb(0, GFP_KERNEL);    /*0721test*/
>> +      if (!backup_urb) {
>> +              dev_err(&hdev->dev, "cannot allocate backup_urb\n");
>> +              return -ENOMEM;
>> +      }
>> +      hid_dev_backup = hdev;
>> +      /*dynamic allocate driver handle*/
>> +      ret = alloc_chrdev_region(&dev, 0, sis_char_devs_count, name);
>> +      if (ret)
>> +              goto err1;
>> +
>> +      sis_char_major = MAJOR(dev);
>> +      cdev_init(&sis_char_cdev, &sis_cdev_fops);
>> +      sis_char_cdev.owner = THIS_MODULE;
>> +      ret = cdev_add(&sis_char_cdev, dev, sis_char_devs_count);
>> +      if (ret)
>> +              goto err2;
>> +
>> +      dev_info(&hdev->dev, "%s driver(major %d) installed.\n",
>> +                                      name, sis_char_major);
>> +
>> +      /*register class*/
>> +      sis_char_class = class_create(THIS_MODULE, name);
>> +      if (IS_ERR(sis_char_class)) {
>> +              ret = PTR_ERR(sis_char_class);
>> +              goto err3;
>> +      }
>> +
>> +      class_dev = device_create(sis_char_class, NULL, dev, NULL, name);
>> +      if (IS_ERR(class_dev)) {
>> +              ret = PTR_ERR(class_dev);
>> +              goto err4;
>> +      }
>> +
>> +      return 0;
>> +err4:
>> +      class_destroy(sis_char_class);
>> +      sis_char_class = NULL;
>> +err3:
>> +      cdev_del(&sis_char_cdev);
>> +      memset(&sis_char_cdev, 0, sizeof(struct cdev));
>> +err2:
>> +      sis_char_major = 0;
>> +      unregister_chrdev_region(dev, sis_char_devs_count);
>> +err1:
>> +      usb_kill_urb(backup_urb);
>> +      usb_free_urb(backup_urb);
>> +      backup_urb = NULL;
>> +      hid_dev_backup = NULL;
>> +      return ret;
>> +}
>> +
>> +/*for ioctl*/
>> +static void sis_deinit_chardev(void)
>> +{
>> +      dev_t dev = MKDEV(sis_char_major, 0);
>> +
>> +      if (hid_dev_backup) {
>> +              dev_info(&hid_dev_backup->dev, "sis_remove\n");
>> +              device_destroy(sis_char_class, dev);
>> +              class_destroy(sis_char_class);
>> +              sis_char_class = NULL;
>> +              cdev_del(&sis_char_cdev);
>> +              memset(&sis_char_cdev, 0, sizeof(struct cdev));
>> +              sis_char_major = 0;
>> +              unregister_chrdev_region(dev, sis_char_devs_count);
>> +              usb_kill_urb(backup_urb);
>> +              usb_free_urb(backup_urb);
>> +              backup_urb = NULL;
>> +              hid_dev_backup = NULL;
>> +      }
>> +}
>> diff --git a/linux-3.18.5/drivers/hid/usbhid/hid-quirks.c b/linux-3.18.5/drivers/hid/usbhid/hid-quirks.c
>> index 4477eb7..b534321 100644
>> --- a/linux-3.18.5/drivers/hid/usbhid/hid-quirks.c
>> +++ b/linux-3.18.5/drivers/hid/usbhid/hid-quirks.c
>> @@ -97,6 +97,7 @@ static const struct hid_blacklist {
>>       { USB_VENDOR_ID_SIGMATEL, USB_DEVICE_ID_SIGMATEL_STMP3780, HID_QUIRK_NOGET },
>>       { USB_VENDOR_ID_SIS_TOUCH, USB_DEVICE_ID_SIS9200_TOUCH, HID_QUIRK_NOGET },
>>       { USB_VENDOR_ID_SIS_TOUCH, USB_DEVICE_ID_SIS817_TOUCH, HID_QUIRK_NOGET },
>> +      { USB_VENDOR_ID_SIS_TOUCH, USB_DEVICE_ID_SISF817_TOUCH,
>> + HID_QUIRK_NOGET },
>>       { USB_VENDOR_ID_SIS_TOUCH, USB_DEVICE_ID_SIS_TS, HID_QUIRK_NO_INIT_REPORTS },
>>       { USB_VENDOR_ID_SIS_TOUCH, USB_DEVICE_ID_SIS1030_TOUCH, HID_QUIRK_NOGET },
>>       { USB_VENDOR_ID_SUN, USB_DEVICE_ID_RARITAN_KVM_DONGLE, HID_QUIRK_NOGET },
>
>
> --
> Oliver Neukum <oneukum@xxxxxxx>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/