Re: [PATCH v2] HID: rmi: Make hid-rmi a transport driver for synaptics-rmi4

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Jun 03 2016 or thereabouts, Andrew Duggan wrote:
> The Synaptics RMI4 driver provides support for RMI4 devices. Instead of
> duplicating the RMI4 processing code, make hid-rmi a transport driver
> and register it with the Synaptics RMI4 core.
> 
> Signed-off-by: Andrew Duggan <aduggan@xxxxxxxxxxxxx>
> ---
> Here is the updated version of the hid-rmi patch. This patch along with
> the series I just posted for rmi_core, provides all of the functionality
> currently present in hid-rmi. This patch does depend on the changes in
> the rmi_core patch series to build since it is setting fields in 
> struct rmi_2d_sensor_platform_data which are added in that series.
> I can separate setting those fields into a separate patch if that helps
> with integrating between the input and HID trees.
> 
> In addition to the core changes this version also adds a fifo so that
> attention reports are stored when the attn worker is processing a report.
> 
> I also changes the Kconfig options since the previous patch. The previous
> version just added a dependency on RMI4_CORE. But, if you take a previous
> config with HID_RMI set and don't know to set RMI4_CORE, F11, and F30
> users will end up with a touchpad which doesn't work.

Don't you need F12 too (for precision touchpads IIRC?).

> 
> Thanks,
> Andrew
> 
>  drivers/hid/Kconfig   |   3 +
>  drivers/hid/hid-rmi.c | 962 +++++++-------------------------------------------
>  2 files changed, 140 insertions(+), 825 deletions(-)
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 4117225..0520626 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -747,6 +747,9 @@ config HID_SUNPLUS
>  config HID_RMI
>  	tristate "Synaptics RMI4 device support"
>  	depends on HID
> +	select RMI4_CORE
> +	select RMI4_F11
> +	select RMI4_F30
>  	---help---
>  	Support for Synaptics RMI4 touchpads.
>  	Say Y here if you have a Synaptics RMI4 touchpads over i2c-hid or usbhid
> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
> index 9cd2ca3..34ff9a2 100644
> --- a/drivers/hid/hid-rmi.c
> +++ b/drivers/hid/hid-rmi.c
> @@ -19,6 +19,8 @@
>  #include <linux/slab.h>
>  #include <linux/wait.h>
>  #include <linux/sched.h>
> +#include <linux/kfifo.h>
> +#include <linux/rmi.h>
>  #include "hid-ids.h"
>  
>  #define RMI_MOUSE_REPORT_ID		0x01 /* Mouse emulation Report */
> @@ -33,9 +35,6 @@
>  #define RMI_READ_DATA_PENDING		1
>  #define RMI_STARTED			2
>  
> -#define RMI_SLEEP_NORMAL		0x0
> -#define RMI_SLEEP_DEEP_SLEEP		0x1
> -
>  /* device flags */
>  #define RMI_DEVICE			BIT(0)
>  #define RMI_DEVICE_HAS_PHYS_BUTTONS	BIT(1)
> @@ -54,18 +53,10 @@ enum rmi_mode_type {
>  	RMI_MODE_NO_PACKED_ATTN_REPORTS	= 2,
>  };
>  
> -struct rmi_function {
> -	unsigned page;			/* page of the function */
> -	u16 query_base_addr;		/* base address for queries */
> -	u16 command_base_addr;		/* base address for commands */
> -	u16 control_base_addr;		/* base address for controls */
> -	u16 data_base_addr;		/* base address for datas */
> -	unsigned int interrupt_base;	/* cross-function interrupt number
> -					 * (uniq in the device)*/
> -	unsigned int interrupt_count;	/* number of interrupts */
> -	unsigned int report_size;	/* size of a report */
> -	unsigned long irq_mask;		/* mask of the interrupts
> -					 * (to be applied against ATTN IRQ) */
> +/* Structure for storing attn report plus size of valid data in the kfifo */
> +struct rmi_attn_report {
> +	int size;
> +	u8 data[];
>  };
>  
>  /**
> @@ -84,26 +75,22 @@ struct rmi_function {
>   *
>   * @flags: flags for the current device (started, reading, etc...)
>   *
> - * @f11: placeholder of internal RMI function F11 description
> - * @f30: placeholder of internal RMI function F30 description
> - *
> - * @max_fingers: maximum finger count reported by the device
> - * @max_x: maximum x value reported by the device
> - * @max_y: maximum y value reported by the device
> - *
> - * @gpio_led_count: count of GPIOs + LEDs reported by F30
> - * @button_count: actual physical buttons count
> - * @button_mask: button mask used to decode GPIO ATTN reports
> - * @button_state_mask: pull state of the buttons
> - *
> - * @input: pointer to the kernel input device
> - *
>   * @reset_work: worker which will be called in case of a mouse report
> + * @attn_work: worker used for processing attention reports
>   * @hdev: pointer to the struct hid_device
> + *
> + * @device_flags: flags which describe the device
> + *
> + * @attn_report_fifo: Store attn reports for deferred processing by worker
> + * @attn_report_size: size of an input report plus the size int
> + * @attn_report: buffer for storing the attn report while it is being processes
> + * @in_attn_report: buffer for storing input report plus size before adding it
> + * to the fifo.
>   */
>  struct rmi_data {
>  	struct mutex page_mutex;
>  	int page;
> +	struct rmi_transport_dev xport;

Nitpick: this is not documented above :)

>  
>  	wait_queue_head_t wait;
>  
> @@ -115,34 +102,16 @@ struct rmi_data {
>  
>  	unsigned long flags;
>  
> -	struct rmi_function f01;
> -	struct rmi_function f11;
> -	struct rmi_function f30;
> -
> -	unsigned int max_fingers;
> -	unsigned int max_x;
> -	unsigned int max_y;
> -	unsigned int x_size_mm;
> -	unsigned int y_size_mm;
> -	bool read_f11_ctrl_regs;
> -	u8 f11_ctrl_regs[RMI_F11_CTRL_REG_COUNT];
> -
> -	unsigned int gpio_led_count;
> -	unsigned int button_count;
> -	unsigned long button_mask;
> -	unsigned long button_state_mask;
> -
> -	struct input_dev *input;
> -
>  	struct work_struct reset_work;
> +	struct work_struct attn_work;
>  	struct hid_device *hdev;
>  
>  	unsigned long device_flags;
> -	unsigned long firmware_id;
>  
> -	u8 f01_ctrl0;
> -	u8 interrupt_enable_mask;
> -	bool restore_interrupt_mask;
> +	struct kfifo attn_report_fifo;
> +	int attn_report_size;
> +	struct rmi_attn_report *attn_report;
> +	struct rmi_attn_report *in_attn_report;
>  };
>  
>  #define RMI_PAGE(addr) (((addr) >> 8) & 0xff)
> @@ -214,10 +183,11 @@ static int rmi_write_report(struct hid_device *hdev, u8 *report, int len)
>  	return ret;
>  }
>  
> -static int rmi_read_block(struct hid_device *hdev, u16 addr, void *buf,
> -		const int len)
> +static int rmi_hid_read_block(struct rmi_transport_dev *xport, u16 addr,
> +		void *buf, size_t len)
>  {
> -	struct rmi_data *data = hid_get_drvdata(hdev);
> +	struct rmi_data *data = container_of(xport, struct rmi_data, xport);
> +	struct hid_device *hdev = data->hdev;
>  	int ret;
>  	int bytes_read;
>  	int bytes_needed;
> @@ -286,15 +256,11 @@ exit:
>  	return ret;
>  }
>  
> -static inline int rmi_read(struct hid_device *hdev, u16 addr, void *buf)
> -{
> -	return rmi_read_block(hdev, addr, buf, 1);
> -}
> -
> -static int rmi_write_block(struct hid_device *hdev, u16 addr, void *buf,
> -		const int len)
> +static int rmi_hid_write_block(struct rmi_transport_dev *xport, u16 addr,
> +		const void *buf, size_t len)
>  {
> -	struct rmi_data *data = hid_get_drvdata(hdev);
> +	struct rmi_data *data = container_of(xport, struct rmi_data, xport);
> +	struct hid_device *hdev = data->hdev;
>  	int ret;
>  
>  	mutex_lock(&data->page_mutex);
> @@ -326,62 +292,20 @@ exit:
>  	return ret;
>  }
>  
> -static inline int rmi_write(struct hid_device *hdev, u16 addr, void *buf)
> -{
> -	return rmi_write_block(hdev, addr, buf, 1);
> -}
> -
> -static void rmi_f11_process_touch(struct rmi_data *hdata, int slot,
> -		u8 finger_state, u8 *touch_data)
> -{
> -	int x, y, wx, wy;
> -	int wide, major, minor;
> -	int z;
> -
> -	input_mt_slot(hdata->input, slot);
> -	input_mt_report_slot_state(hdata->input, MT_TOOL_FINGER,
> -			finger_state == 0x01);
> -	if (finger_state == 0x01) {
> -		x = (touch_data[0] << 4) | (touch_data[2] & 0x0F);
> -		y = (touch_data[1] << 4) | (touch_data[2] >> 4);
> -		wx = touch_data[3] & 0x0F;
> -		wy = touch_data[3] >> 4;
> -		wide = (wx > wy);
> -		major = max(wx, wy);
> -		minor = min(wx, wy);
> -		z = touch_data[4];
> -
> -		/* y is inverted */
> -		y = hdata->max_y - y;
> -
> -		input_event(hdata->input, EV_ABS, ABS_MT_POSITION_X, x);
> -		input_event(hdata->input, EV_ABS, ABS_MT_POSITION_Y, y);
> -		input_event(hdata->input, EV_ABS, ABS_MT_ORIENTATION, wide);
> -		input_event(hdata->input, EV_ABS, ABS_MT_PRESSURE, z);
> -		input_event(hdata->input, EV_ABS, ABS_MT_TOUCH_MAJOR, major);
> -		input_event(hdata->input, EV_ABS, ABS_MT_TOUCH_MINOR, minor);
> -	}
> -}
> -
>  static int rmi_reset_attn_mode(struct hid_device *hdev)
>  {
>  	struct rmi_data *data = hid_get_drvdata(hdev);
> +	struct rmi_device *rmi_dev = data->xport.rmi_dev;
>  	int ret;
>  
>  	ret = rmi_set_mode(hdev, RMI_MODE_ATTN_REPORTS);
>  	if (ret)
>  		return ret;
>  
> -	if (data->restore_interrupt_mask) {
> -		ret = rmi_write(hdev, data->f01.control_base_addr + 1,
> -				&data->interrupt_enable_mask);
> -		if (ret) {
> -			hid_err(hdev, "can not write F01 control register\n");
> -			return ret;
> -		}
> -	}
> +	if (test_bit(RMI_STARTED, &data->flags))
> +		ret = rmi_dev->driver->reset_handler(rmi_dev);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static void rmi_reset_work(struct work_struct *work)
> @@ -393,102 +317,42 @@ static void rmi_reset_work(struct work_struct *work)
>  	rmi_reset_attn_mode(hdata->hdev);
>  }
>  
> -static inline int rmi_schedule_reset(struct hid_device *hdev)
> +static void rmi_attn_work(struct work_struct *work)
>  {
> -	struct rmi_data *hdata = hid_get_drvdata(hdev);
> -	return schedule_work(&hdata->reset_work);
> -}
> -
> -static int rmi_f11_input_event(struct hid_device *hdev, u8 irq, u8 *data,
> -		int size)
> -{
> -	struct rmi_data *hdata = hid_get_drvdata(hdev);
> -	int offset;
> -	int i;
> -
> -	if (!(irq & hdata->f11.irq_mask) || size <= 0)
> -		return 0;
> -
> -	offset = (hdata->max_fingers >> 2) + 1;
> -	for (i = 0; i < hdata->max_fingers; i++) {
> -		int fs_byte_position = i >> 2;
> -		int fs_bit_position = (i & 0x3) << 1;
> -		int finger_state = (data[fs_byte_position] >> fs_bit_position) &
> -					0x03;
> -		int position = offset + 5 * i;
> -
> -		if (position + 5 > size) {
> -			/* partial report, go on with what we received */
> -			printk_once(KERN_WARNING
> -				"%s %s: Detected incomplete finger report. Finger reports may occasionally get dropped on this platform.\n",
> -				 dev_driver_string(&hdev->dev),
> -				 dev_name(&hdev->dev));
> -			hid_dbg(hdev, "Incomplete finger report\n");
> -			break;
> -		}
> -
> -		rmi_f11_process_touch(hdata, i, finger_state, &data[position]);
> -	}
> -	input_mt_sync_frame(hdata->input);
> -	input_sync(hdata->input);
> -	return hdata->f11.report_size;
> -}
> +	struct rmi_data *hdata = container_of(work, struct rmi_data,
> +						attn_work);
> +	struct rmi_device *rmi_dev = hdata->xport.rmi_dev;
> +	struct rmi_driver_data *drvdata = dev_get_drvdata(&rmi_dev->dev);
> +	int ret;
>  
> -static int rmi_f30_input_event(struct hid_device *hdev, u8 irq, u8 *data,
> -		int size)
> -{
> -	struct rmi_data *hdata = hid_get_drvdata(hdev);
> -	int i;
> -	int button = 0;
> -	bool value;
> +	ret = kfifo_out(&hdata->attn_report_fifo, hdata->attn_report,
> +						hdata->attn_report_size);
> +	if (ret != hdata->attn_report_size)
> +		return;
>  
> -	if (!(irq & hdata->f30.irq_mask))
> -		return 0;
> +	*(drvdata->irq_status) = hdata->attn_report->data[1];
> +	hdata->xport.attn_data = &hdata->attn_report->data[2];
> +	hdata->xport.attn_size = hdata->attn_report->size - 2;
>  
> -	if (size < (int)hdata->f30.report_size) {
> -		hid_warn(hdev, "Click Button pressed, but the click data is missing\n");
> -		return 0;
> -	}
> +	rmi_process_interrupt_requests(rmi_dev);
>  
> -	for (i = 0; i < hdata->gpio_led_count; i++) {
> -		if (test_bit(i, &hdata->button_mask)) {
> -			value = (data[i / 8] >> (i & 0x07)) & BIT(0);
> -			if (test_bit(i, &hdata->button_state_mask))
> -				value = !value;
> -			input_event(hdata->input, EV_KEY, BTN_LEFT + button++,
> -					value);
> -		}
> -	}
> -	return hdata->f30.report_size;
> +	if (!kfifo_is_empty(&hdata->attn_report_fifo))
> +		schedule_work(&hdata->attn_work);
>  }
>  
>  static int rmi_input_event(struct hid_device *hdev, u8 *data, int size)
>  {
>  	struct rmi_data *hdata = hid_get_drvdata(hdev);
> -	unsigned long irq_mask = 0;
> -	unsigned index = 2;
>  
>  	if (!(test_bit(RMI_STARTED, &hdata->flags)))
>  		return 0;
>  
> -	irq_mask |= hdata->f11.irq_mask;
> -	irq_mask |= hdata->f30.irq_mask;
> -
> -	if (data[1] & ~irq_mask)
> -		hid_dbg(hdev, "unknown intr source:%02lx %s:%d\n",
> -			data[1] & ~irq_mask, __FILE__, __LINE__);
> -
> -	if (hdata->f11.interrupt_base < hdata->f30.interrupt_base) {
> -		index += rmi_f11_input_event(hdev, data[1], &data[index],
> -				size - index);
> -		index += rmi_f30_input_event(hdev, data[1], &data[index],
> -				size - index);
> -	} else {
> -		index += rmi_f30_input_event(hdev, data[1], &data[index],
> -				size - index);
> -		index += rmi_f11_input_event(hdev, data[1], &data[index],
> -				size - index);
> -	}
> +	hdata->in_attn_report->size = size;
> +	memcpy(&hdata->in_attn_report->data, data, size);
> +
> +	kfifo_in(&hdata->attn_report_fifo, hdata->in_attn_report,
> +						hdata->attn_report_size);
> +	schedule_work(&hdata->attn_work);
>  
>  	return 1;
>  }
> @@ -562,7 +426,7 @@ static int rmi_event(struct hid_device *hdev, struct hid_field *field,
>  				return 1;
>  		}
>  
> -		rmi_schedule_reset(hdev);
> +		schedule_work(&data->reset_work);
>  		return 1;
>  	}
>  
> @@ -570,637 +434,71 @@ static int rmi_event(struct hid_device *hdev, struct hid_field *field,
>  }
>  
>  #ifdef CONFIG_PM
> -static int rmi_set_sleep_mode(struct hid_device *hdev, int sleep_mode)
> -{
> -	struct rmi_data *data = hid_get_drvdata(hdev);
> -	int ret;
> -	u8 f01_ctrl0;
> -
> -	f01_ctrl0 = (data->f01_ctrl0 & ~0x3) | sleep_mode;
> -
> -	ret = rmi_write(hdev, data->f01.control_base_addr,
> -			&f01_ctrl0);
> -	if (ret) {
> -		hid_err(hdev, "can not write sleep mode\n");
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  static int rmi_suspend(struct hid_device *hdev, pm_message_t message)
>  {
>  	struct rmi_data *data = hid_get_drvdata(hdev);
> -	int ret;
> -	u8 buf[RMI_F11_CTRL_REG_COUNT];
> -
> -	if (!(data->device_flags & RMI_DEVICE))
> -		return 0;
> -
> -	ret = rmi_read_block(hdev, data->f11.control_base_addr, buf,
> -				RMI_F11_CTRL_REG_COUNT);
> -	if (ret)
> -		hid_warn(hdev, "can not read F11 control registers\n");
> -	else
> -		memcpy(data->f11_ctrl_regs, buf, RMI_F11_CTRL_REG_COUNT);
> -
> -
> -	if (!device_may_wakeup(hdev->dev.parent))
> -		return rmi_set_sleep_mode(hdev, RMI_SLEEP_DEEP_SLEEP);
> -
> -	return 0;
> -}
> -
> -static int rmi_post_reset(struct hid_device *hdev)
> -{
> -	struct rmi_data *data = hid_get_drvdata(hdev);
> +	struct rmi_device *rmi_dev = data->xport.rmi_dev;
>  	int ret;
>  
>  	if (!(data->device_flags & RMI_DEVICE))
>  		return 0;
>  
> -	ret = rmi_reset_attn_mode(hdev);
> +	ret = rmi_driver_suspend(rmi_dev);
>  	if (ret) {
> -		hid_err(hdev, "can not set rmi mode\n");
> +		hid_warn(hdev, "Failed to suspend device: %d\n", ret);
>  		return ret;
>  	}
>  
> -	if (data->read_f11_ctrl_regs) {
> -		ret = rmi_write_block(hdev, data->f11.control_base_addr,
> -				data->f11_ctrl_regs, RMI_F11_CTRL_REG_COUNT);
> -		if (ret)
> -			hid_warn(hdev,
> -				"can not write F11 control registers after reset\n");
> -	}
> -
> -	if (!device_may_wakeup(hdev->dev.parent)) {
> -		ret = rmi_set_sleep_mode(hdev, RMI_SLEEP_NORMAL);
> -		if (ret) {
> -			hid_err(hdev, "can not write sleep mode\n");
> -			return ret;
> -		}
> -	}
> -
> -	return ret;
> +	return 0;
>  }
>  
>  static int rmi_post_resume(struct hid_device *hdev)
>  {
>  	struct rmi_data *data = hid_get_drvdata(hdev);
> +	struct rmi_device *rmi_dev = data->xport.rmi_dev;
> +	int ret;
>  
>  	if (!(data->device_flags & RMI_DEVICE))
>  		return 0;
>  
> -	return rmi_reset_attn_mode(hdev);
> -}
> -#endif /* CONFIG_PM */
> -
> -#define RMI4_MAX_PAGE 0xff
> -#define RMI4_PAGE_SIZE 0x0100
> -
> -#define PDT_START_SCAN_LOCATION 0x00e9
> -#define PDT_END_SCAN_LOCATION	0x0005
> -#define RMI4_END_OF_PDT(id) ((id) == 0x00 || (id) == 0xff)
> -
> -struct pdt_entry {
> -	u8 query_base_addr:8;
> -	u8 command_base_addr:8;
> -	u8 control_base_addr:8;
> -	u8 data_base_addr:8;
> -	u8 interrupt_source_count:3;
> -	u8 bits3and4:2;
> -	u8 function_version:2;
> -	u8 bit7:1;
> -	u8 function_number:8;
> -} __attribute__((__packed__));
> -
> -static inline unsigned long rmi_gen_mask(unsigned irq_base, unsigned irq_count)
> -{
> -	return GENMASK(irq_count + irq_base - 1, irq_base);
> -}
> -
> -static void rmi_register_function(struct rmi_data *data,
> -	struct pdt_entry *pdt_entry, int page, unsigned interrupt_count)
> -{
> -	struct rmi_function *f = NULL;
> -	u16 page_base = page << 8;
> -
> -	switch (pdt_entry->function_number) {
> -	case 0x01:
> -		f = &data->f01;
> -		break;
> -	case 0x11:
> -		f = &data->f11;
> -		break;
> -	case 0x30:
> -		f = &data->f30;
> -		break;
> -	}
> -
> -	if (f) {
> -		f->page = page;
> -		f->query_base_addr = page_base | pdt_entry->query_base_addr;
> -		f->command_base_addr = page_base | pdt_entry->command_base_addr;
> -		f->control_base_addr = page_base | pdt_entry->control_base_addr;
> -		f->data_base_addr = page_base | pdt_entry->data_base_addr;
> -		f->interrupt_base = interrupt_count;
> -		f->interrupt_count = pdt_entry->interrupt_source_count;
> -		f->irq_mask = rmi_gen_mask(f->interrupt_base,
> -						f->interrupt_count);
> -		data->interrupt_enable_mask |= f->irq_mask;
> -	}
> -}
> -
> -static int rmi_scan_pdt(struct hid_device *hdev)
> -{
> -	struct rmi_data *data = hid_get_drvdata(hdev);
> -	struct pdt_entry entry;
> -	int page;
> -	bool page_has_function;
> -	int i;
> -	int retval;
> -	int interrupt = 0;
> -	u16 page_start, pdt_start , pdt_end;
> -
> -	hid_info(hdev, "Scanning PDT...\n");
> -
> -	for (page = 0; (page <= RMI4_MAX_PAGE); page++) {
> -		page_start = RMI4_PAGE_SIZE * page;
> -		pdt_start = page_start + PDT_START_SCAN_LOCATION;
> -		pdt_end = page_start + PDT_END_SCAN_LOCATION;
> -
> -		page_has_function = false;
> -		for (i = pdt_start; i >= pdt_end; i -= sizeof(entry)) {
> -			retval = rmi_read_block(hdev, i, &entry, sizeof(entry));
> -			if (retval) {
> -				hid_err(hdev,
> -					"Read of PDT entry at %#06x failed.\n",
> -					i);
> -				goto error_exit;
> -			}
> -
> -			if (RMI4_END_OF_PDT(entry.function_number))
> -				break;
> -
> -			page_has_function = true;
> -
> -			hid_info(hdev, "Found F%02X on page %#04x\n",
> -					entry.function_number, page);
> -
> -			rmi_register_function(data, &entry, page, interrupt);
> -			interrupt += entry.interrupt_source_count;
> -		}
> -
> -		if (!page_has_function)
> -			break;
> -	}
> -
> -	hid_info(hdev, "%s: Done with PDT scan.\n", __func__);
> -	retval = 0;
> -
> -error_exit:
> -	return retval;
> -}
> -
> -#define RMI_DEVICE_F01_BASIC_QUERY_LEN	11
> -
> -static int rmi_populate_f01(struct hid_device *hdev)
> -{
> -	struct rmi_data *data = hid_get_drvdata(hdev);
> -	u8 basic_queries[RMI_DEVICE_F01_BASIC_QUERY_LEN];
> -	u8 info[3];
> -	int ret;
> -	bool has_query42;
> -	bool has_lts;
> -	bool has_sensor_id;
> -	bool has_ds4_queries = false;
> -	bool has_build_id_query = false;
> -	bool has_package_id_query = false;
> -	u16 query_offset = data->f01.query_base_addr;
> -	u16 prod_info_addr;
> -	u8 ds4_query_len;
> -
> -	ret = rmi_read_block(hdev, query_offset, basic_queries,
> -				RMI_DEVICE_F01_BASIC_QUERY_LEN);
> -	if (ret) {
> -		hid_err(hdev, "Can not read basic queries from Function 0x1.\n");
> -		return ret;
> -	}
> -
> -	has_lts = !!(basic_queries[0] & BIT(2));
> -	has_sensor_id = !!(basic_queries[1] & BIT(3));
> -	has_query42 = !!(basic_queries[1] & BIT(7));
> -
> -	query_offset += 11;
> -	prod_info_addr = query_offset + 6;
> -	query_offset += 10;
> -
> -	if (has_lts)
> -		query_offset += 20;
> -
> -	if (has_sensor_id)
> -		query_offset++;
> -
> -	if (has_query42) {
> -		ret = rmi_read(hdev, query_offset, info);
> -		if (ret) {
> -			hid_err(hdev, "Can not read query42.\n");
> -			return ret;
> -		}
> -		has_ds4_queries = !!(info[0] & BIT(0));
> -		query_offset++;
> -	}
> -
> -	if (has_ds4_queries) {
> -		ret = rmi_read(hdev, query_offset, &ds4_query_len);
> -		if (ret) {
> -			hid_err(hdev, "Can not read DS4 Query length.\n");
> -			return ret;
> -		}
> -		query_offset++;
> -
> -		if (ds4_query_len > 0) {
> -			ret = rmi_read(hdev, query_offset, info);
> -			if (ret) {
> -				hid_err(hdev, "Can not read DS4 query.\n");
> -				return ret;
> -			}
> -
> -			has_package_id_query = !!(info[0] & BIT(0));
> -			has_build_id_query = !!(info[0] & BIT(1));
> -		}
> -	}
> -
> -	if (has_package_id_query)
> -		prod_info_addr++;
> -
> -	if (has_build_id_query) {
> -		ret = rmi_read_block(hdev, prod_info_addr, info, 3);
> -		if (ret) {
> -			hid_err(hdev, "Can not read product info.\n");
> -			return ret;
> -		}
> -
> -		data->firmware_id = info[1] << 8 | info[0];
> -		data->firmware_id += info[2] * 65536;
> -	}
> -
> -	ret = rmi_read_block(hdev, data->f01.control_base_addr, info,
> -				2);
> -
> -	if (ret) {
> -		hid_err(hdev, "can not read f01 ctrl registers\n");
> -		return ret;
> -	}
> -
> -	data->f01_ctrl0 = info[0];
> -
> -	if (!info[1]) {
> -		/*
> -		 * Do to a firmware bug in some touchpads the F01 interrupt
> -		 * enable control register will be cleared on reset.
> -		 * This will stop the touchpad from reporting data, so
> -		 * if F01 CTRL1 is 0 then we need to explicitly enable
> -		 * interrupts for the functions we want data for.
> -		 */
> -		data->restore_interrupt_mask = true;
> -
> -		ret = rmi_write(hdev, data->f01.control_base_addr + 1,
> -				&data->interrupt_enable_mask);
> -		if (ret) {
> -			hid_err(hdev, "can not write to control reg 1: %d.\n",
> -				ret);
> -			return ret;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
> -static int rmi_populate_f11(struct hid_device *hdev)
> -{
> -	struct rmi_data *data = hid_get_drvdata(hdev);
> -	u8 buf[20];
> -	int ret;
> -	bool has_query9;
> -	bool has_query10 = false;
> -	bool has_query11;
> -	bool has_query12;
> -	bool has_query27;
> -	bool has_query28;
> -	bool has_query36 = false;
> -	bool has_physical_props;
> -	bool has_gestures;
> -	bool has_rel;
> -	bool has_data40 = false;
> -	bool has_dribble = false;
> -	bool has_palm_detect = false;
> -	unsigned x_size, y_size;
> -	u16 query_offset;
> -
> -	if (!data->f11.query_base_addr) {
> -		hid_err(hdev, "No 2D sensor found, giving up.\n");
> -		return -ENODEV;
> -	}
> -
> -	/* query 0 contains some useful information */
> -	ret = rmi_read(hdev, data->f11.query_base_addr, buf);
> -	if (ret) {
> -		hid_err(hdev, "can not get query 0: %d.\n", ret);
> -		return ret;
> -	}
> -	has_query9 = !!(buf[0] & BIT(3));
> -	has_query11 = !!(buf[0] & BIT(4));
> -	has_query12 = !!(buf[0] & BIT(5));
> -	has_query27 = !!(buf[0] & BIT(6));
> -	has_query28 = !!(buf[0] & BIT(7));
> -
> -	/* query 1 to get the max number of fingers */
> -	ret = rmi_read(hdev, data->f11.query_base_addr + 1, buf);
> -	if (ret) {
> -		hid_err(hdev, "can not get NumberOfFingers: %d.\n", ret);
> -		return ret;
> -	}
> -	data->max_fingers = (buf[0] & 0x07) + 1;
> -	if (data->max_fingers > 5)
> -		data->max_fingers = 10;
> -
> -	data->f11.report_size = data->max_fingers * 5 +
> -				DIV_ROUND_UP(data->max_fingers, 4);
> -
> -	if (!(buf[0] & BIT(4))) {
> -		hid_err(hdev, "No absolute events, giving up.\n");
> -		return -ENODEV;
> -	}
> -
> -	has_rel = !!(buf[0] & BIT(3));
> -	has_gestures = !!(buf[0] & BIT(5));
> -
> -	ret = rmi_read(hdev, data->f11.query_base_addr + 5, buf);
> -	if (ret) {
> -		hid_err(hdev, "can not get absolute data sources: %d.\n", ret);
> -		return ret;
> -	}
> -
> -	has_dribble = !!(buf[0] & BIT(4));
> -
> -	/*
> -	 * At least 4 queries are guaranteed to be present in F11
> -	 * +1 for query 5 which is present since absolute events are
> -	 * reported and +1 for query 12.
> -	 */
> -	query_offset = 6;
> -
> -	if (has_rel)
> -		++query_offset; /* query 6 is present */
> -
> -	if (has_gestures) {
> -		/* query 8 to find out if query 10 exists */
> -		ret = rmi_read(hdev,
> -			data->f11.query_base_addr + query_offset + 1, buf);
> -		if (ret) {
> -			hid_err(hdev, "can not read gesture information: %d.\n",
> -				ret);
> -			return ret;
> -		}
> -		has_palm_detect = !!(buf[0] & BIT(0));
> -		has_query10 = !!(buf[0] & BIT(2));
> -
> -		query_offset += 2; /* query 7 and 8 are present */
> -	}
> -
> -	if (has_query9)
> -		++query_offset;
> -
> -	if (has_query10)
> -		++query_offset;
> -
> -	if (has_query11)
> -		++query_offset;
> -
> -	/* query 12 to know if the physical properties are reported */
> -	if (has_query12) {
> -		ret = rmi_read(hdev, data->f11.query_base_addr
> -				+ query_offset, buf);
> -		if (ret) {
> -			hid_err(hdev, "can not get query 12: %d.\n", ret);
> -			return ret;
> -		}
> -		has_physical_props = !!(buf[0] & BIT(5));
> -
> -		if (has_physical_props) {
> -			query_offset += 1;
> -			ret = rmi_read_block(hdev,
> -					data->f11.query_base_addr
> -						+ query_offset, buf, 4);
> -			if (ret) {
> -				hid_err(hdev, "can not read query 15-18: %d.\n",
> -					ret);
> -				return ret;
> -			}
> -
> -			x_size = buf[0] | (buf[1] << 8);
> -			y_size = buf[2] | (buf[3] << 8);
> -
> -			data->x_size_mm = DIV_ROUND_CLOSEST(x_size, 10);
> -			data->y_size_mm = DIV_ROUND_CLOSEST(y_size, 10);
> -
> -			hid_info(hdev, "%s: size in mm: %d x %d\n",
> -				 __func__, data->x_size_mm, data->y_size_mm);
> -
> -			/*
> -			 * query 15 - 18 contain the size of the sensor
> -			 * and query 19 - 26 contain bezel dimensions
> -			 */
> -			query_offset += 12;
> -		}
> -	}
> -
> -	if (has_query27)
> -		++query_offset;
> -
> -	if (has_query28) {
> -		ret = rmi_read(hdev, data->f11.query_base_addr
> -				+ query_offset, buf);
> -		if (ret) {
> -			hid_err(hdev, "can not get query 28: %d.\n", ret);
> -			return ret;
> -		}
> -
> -		has_query36 = !!(buf[0] & BIT(6));
> -	}
> -
> -	if (has_query36) {
> -		query_offset += 2;
> -		ret = rmi_read(hdev, data->f11.query_base_addr
> -				+ query_offset, buf);
> -		if (ret) {
> -			hid_err(hdev, "can not get query 36: %d.\n", ret);
> -			return ret;
> -		}
> -
> -		has_data40 = !!(buf[0] & BIT(5));
> -	}
> -
> -
> -	if (has_data40)
> -		data->f11.report_size += data->max_fingers * 2;
> -
> -	ret = rmi_read_block(hdev, data->f11.control_base_addr,
> -			data->f11_ctrl_regs, RMI_F11_CTRL_REG_COUNT);
> -	if (ret) {
> -		hid_err(hdev, "can not read ctrl block of size 11: %d.\n", ret);
> -		return ret;
> -	}
> -
> -	/* data->f11_ctrl_regs now contains valid register data */
> -	data->read_f11_ctrl_regs = true;
> -
> -	data->max_x = data->f11_ctrl_regs[6] | (data->f11_ctrl_regs[7] << 8);
> -	data->max_y = data->f11_ctrl_regs[8] | (data->f11_ctrl_regs[9] << 8);
> -
> -	if (has_dribble) {
> -		data->f11_ctrl_regs[0] = data->f11_ctrl_regs[0] & ~BIT(6);
> -		ret = rmi_write(hdev, data->f11.control_base_addr,
> -				data->f11_ctrl_regs);
> -		if (ret) {
> -			hid_err(hdev, "can not write to control reg 0: %d.\n",
> -				ret);
> -			return ret;
> -		}
> -	}
> -
> -	if (has_palm_detect) {
> -		data->f11_ctrl_regs[11] = data->f11_ctrl_regs[11] & ~BIT(0);
> -		ret = rmi_write(hdev, data->f11.control_base_addr + 11,
> -				&data->f11_ctrl_regs[11]);
> -		if (ret) {
> -			hid_err(hdev, "can not write to control reg 11: %d.\n",
> -				ret);
> -			return ret;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
> -static int rmi_populate_f30(struct hid_device *hdev)
> -{
> -	struct rmi_data *data = hid_get_drvdata(hdev);
> -	u8 buf[20];
> -	int ret;
> -	bool has_gpio, has_led;
> -	unsigned bytes_per_ctrl;
> -	u8 ctrl2_addr;
> -	int ctrl2_3_length;
> -	int i;
> -
> -	/* function F30 is for physical buttons */
> -	if (!data->f30.query_base_addr) {
> -		hid_err(hdev, "No GPIO/LEDs found, giving up.\n");
> -		return -ENODEV;
> -	}
> -
> -	ret = rmi_read_block(hdev, data->f30.query_base_addr, buf, 2);
> -	if (ret) {
> -		hid_err(hdev, "can not get F30 query registers: %d.\n", ret);
> +	ret = rmi_reset_attn_mode(hdev);
> +	if (ret)
>  		return ret;
> -	}
> -
> -	has_gpio = !!(buf[0] & BIT(3));
> -	has_led = !!(buf[0] & BIT(2));
> -	data->gpio_led_count = buf[1] & 0x1f;
>  
> -	/* retrieve ctrl 2 & 3 registers */
> -	bytes_per_ctrl = (data->gpio_led_count + 7) / 8;
> -	/* Ctrl0 is present only if both has_gpio and has_led are set*/
> -	ctrl2_addr = (has_gpio && has_led) ? bytes_per_ctrl : 0;
> -	/* Ctrl1 is always be present */
> -	ctrl2_addr += bytes_per_ctrl;
> -	ctrl2_3_length = 2 * bytes_per_ctrl;
> -
> -	data->f30.report_size = bytes_per_ctrl;
> -
> -	ret = rmi_read_block(hdev, data->f30.control_base_addr + ctrl2_addr,
> -				buf, ctrl2_3_length);
> +	ret = rmi_driver_resume(rmi_dev);
>  	if (ret) {
> -		hid_err(hdev, "can not read ctrl 2&3 block of size %d: %d.\n",
> -			ctrl2_3_length, ret);
> +		hid_warn(hdev, "Failed to resume device: %d\n", ret);
>  		return ret;
>  	}
>  
> -	for (i = 0; i < data->gpio_led_count; i++) {
> -		int byte_position = i >> 3;
> -		int bit_position = i & 0x07;
> -		u8 dir_byte = buf[byte_position];
> -		u8 data_byte = buf[byte_position + bytes_per_ctrl];
> -		bool dir = (dir_byte >> bit_position) & BIT(0);
> -		bool dat = (data_byte >> bit_position) & BIT(0);
> -
> -		if (dir == 0) {
> -			/* input mode */
> -			if (dat) {
> -				/* actual buttons have pull up resistor */
> -				data->button_count++;
> -				set_bit(i, &data->button_mask);
> -				set_bit(i, &data->button_state_mask);
> -			}
> -		}
> -
> -	}
> -
>  	return 0;
>  }
> +#endif /* CONFIG_PM */
>  
> -static int rmi_populate(struct hid_device *hdev)
> +static int rmi_hid_reset(struct rmi_transport_dev *xport, u16 reset_addr)
>  {
> -	struct rmi_data *data = hid_get_drvdata(hdev);
> -	int ret;
> +	struct rmi_data *data = container_of(xport, struct rmi_data, xport);
> +	struct hid_device *hdev = data->hdev;
>  
> -	ret = rmi_scan_pdt(hdev);
> -	if (ret) {
> -		hid_err(hdev, "PDT scan failed with code %d.\n", ret);
> -		return ret;
> -	}
> -
> -	ret = rmi_populate_f01(hdev);
> -	if (ret) {
> -		hid_err(hdev, "Error while initializing F01 (%d).\n", ret);
> -		return ret;
> -	}
> -
> -	ret = rmi_populate_f11(hdev);
> -	if (ret) {
> -		hid_err(hdev, "Error while initializing F11 (%d).\n", ret);
> -		return ret;
> -	}
> -
> -	if (!(data->device_flags & RMI_DEVICE_HAS_PHYS_BUTTONS)) {
> -		ret = rmi_populate_f30(hdev);
> -		if (ret)
> -			hid_warn(hdev, "Error while initializing F30 (%d).\n", ret);
> -	}
> -
> -	return 0;
> +	return rmi_reset_attn_mode(hdev);
>  }
>  
>  static int rmi_input_configured(struct hid_device *hdev, struct hid_input *hi)
>  {
>  	struct rmi_data *data = hid_get_drvdata(hdev);
>  	struct input_dev *input = hi->input;
> -	int ret;
> -	int res_x, res_y, i;
> +	int ret = 0;
>  
> -	data->input = input;
> +	if (!(data->device_flags & RMI_DEVICE))
> +		return 0;
> +
> +	data->xport.input = input;

This feels awkward. Why can't you rely on rmi to create the inputs and
just use a plain hidraw driver here?

>  
>  	hid_dbg(hdev, "Opening low level driver\n");
>  	ret = hid_hw_open(hdev);
>  	if (ret)
>  		return ret;
>  
> -	if (!(data->device_flags & RMI_DEVICE))
> -		return 0;
> -
>  	/* Allow incoming hid reports */
>  	hid_device_io_start(hdev);
>  
> @@ -1216,40 +514,10 @@ static int rmi_input_configured(struct hid_device *hdev, struct hid_input *hi)
>  		goto exit;
>  	}
>  
> -	ret = rmi_populate(hdev);
> -	if (ret)
> -		goto exit;
> -
> -	hid_info(hdev, "firmware id: %ld\n", data->firmware_id);
> -
> -	__set_bit(EV_ABS, input->evbit);
> -	input_set_abs_params(input, ABS_MT_POSITION_X, 1, data->max_x, 0, 0);
> -	input_set_abs_params(input, ABS_MT_POSITION_Y, 1, data->max_y, 0, 0);
> -
> -	if (data->x_size_mm && data->y_size_mm) {
> -		res_x = (data->max_x - 1) / data->x_size_mm;
> -		res_y = (data->max_y - 1) / data->y_size_mm;
> -
> -		input_abs_set_res(input, ABS_MT_POSITION_X, res_x);
> -		input_abs_set_res(input, ABS_MT_POSITION_Y, res_y);
> -	}
> -
> -	input_set_abs_params(input, ABS_MT_ORIENTATION, 0, 1, 0, 0);
> -	input_set_abs_params(input, ABS_MT_PRESSURE, 0, 0xff, 0, 0);
> -	input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 0x0f, 0, 0);
> -	input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, 0x0f, 0, 0);
> -
> -	ret = input_mt_init_slots(input, data->max_fingers, INPUT_MT_POINTER);
> -	if (ret < 0)
> +	ret = rmi_register_transport_device(&data->xport);
> +	if (ret < 0) {
> +		dev_err(&hdev->dev, "failed to register transport driver\n");
>  		goto exit;
> -
> -	if (data->button_count) {
> -		__set_bit(EV_KEY, input->evbit);
> -		for (i = 0; i < data->button_count; i++)
> -			__set_bit(BTN_LEFT + i, input->keybit);
> -
> -		if (data->button_count == 1)
> -			__set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
>  	}
>  
>  	set_bit(RMI_STARTED, &data->flags);
> @@ -1298,6 +566,27 @@ static int rmi_check_valid_report_id(struct hid_device *hdev, unsigned type,
>  	return 0;
>  }
>  
> +static struct rmi_2d_sensor_platform_data rmi_hid_2d_sensor_data = {
> +	.sensor_type = rmi_sensor_touchpad,
> +	.axis_align.flip_y = true,
> +	.dribble = RMI_REG_STATE_OFF,

Thinking about the Dell XPS 13 Skylake. I think this one would benefit
from a dribble setting to ON. Do you have plan to add platform quirks?

> +	.palm_detect = RMI_REG_STATE_OFF,
> +};
> +
> +static struct rmi_f30_data rmi_hid_f30_data = {
> +};
> +
> +static struct rmi_device_platform_data rmi_hid_pdata = {
> +	.sensor_pdata = &rmi_hid_2d_sensor_data,
> +	.f30_data = &rmi_hid_f30_data,
> +};
> +
> +static const struct rmi_transport_ops hid_rmi_ops = {
> +	.write_block	= rmi_hid_write_block,
> +	.read_block	= rmi_hid_read_block,
> +	.reset		= rmi_hid_reset,
> +};
> +
>  static int rmi_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  {
>  	struct rmi_data *data = NULL;
> @@ -1312,6 +601,7 @@ static int rmi_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		return -ENOMEM;
>  
>  	INIT_WORK(&data->reset_work, rmi_reset_work);
> +	INIT_WORK(&data->attn_work, rmi_attn_work);
>  	data->hdev = hdev;
>  
>  	hid_set_drvdata(hdev, data);
> @@ -1359,34 +649,52 @@ static int rmi_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  
>  	data->writeReport = devm_kzalloc(&hdev->dev, alloc_size, GFP_KERNEL);
>  	if (!data->writeReport) {
> -		ret = -ENOMEM;
> -		return ret;
> +		hid_err(hdev, "failed to allocate buffer for HID reports\n");
> +		return -ENOMEM;
>  	}
>  
>  	data->readReport = data->writeReport + data->output_report_size;
>  
> +	data->attn_report_size = roundup_pow_of_two(
> +					sizeof(struct rmi_attn_report) +
> +					data->input_report_size);
> +
> +	data->attn_report = devm_kzalloc(&hdev->dev, data->attn_report_size * 2,
> +					 GFP_KERNEL);
> +	if (!data->attn_report)
> +		return -ENOMEM;
> +
> +	data->in_attn_report = (struct rmi_attn_report *)
> +					((u8 *)data->attn_report
> +					+ data->attn_report_size);
> +
> +	ret = kfifo_alloc(&data->attn_report_fifo, data->attn_report_size * 8,
> +			GFP_KERNEL);
> +	if (ret) {
> +		hid_err(hdev, "failed to allocate attn report fifo\n");
> +		return -ENOMEM;
> +	}
> +
>  	init_waitqueue_head(&data->wait);
>  
>  	mutex_init(&data->page_mutex);
>  
> +	if (data->device_flags & RMI_DEVICE_HAS_PHYS_BUTTONS)
> +		rmi_hid_pdata.f30_data->disable = true;
> +
> +	data->xport.dev = hdev->dev.parent;
> +	data->xport.pdata = rmi_hid_pdata;
> +	data->xport.proto_name = "hid";
> +	data->xport.ops = &hid_rmi_ops;
> +
>  start:
>  	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>  	if (ret) {
>  		hid_err(hdev, "hw start failed\n");
> +		kfifo_free(&data->attn_report_fifo);
>  		return ret;
>  	}
>  
> -	if ((data->device_flags & RMI_DEVICE) &&
> -	    !test_bit(RMI_STARTED, &data->flags))
> -		/*
> -		 * The device maybe in the bootloader if rmi_input_configured
> -		 * failed to find F11 in the PDT. Print an error, but don't
> -		 * return an error from rmi_probe so that hidraw will be
> -		 * accessible from userspace. That way a userspace tool
> -		 * can be used to reload working firmware on the touchpad.
> -		 */
> -		hid_err(hdev, "Device failed to be properly configured\n");
> -
>  	return 0;
>  }
>  
> @@ -1395,6 +703,10 @@ static void rmi_remove(struct hid_device *hdev)
>  	struct rmi_data *hdata = hid_get_drvdata(hdev);
>  
>  	clear_bit(RMI_STARTED, &hdata->flags);
> +	cancel_work_sync(&hdata->reset_work);
> +	cancel_work_sync(&hdata->attn_work);
> +	rmi_unregister_transport_device(&hdata->xport);
> +	kfifo_free(&hdata->attn_report_fifo);
>  
>  	hid_hw_stop(hdev);
>  }
> @@ -1419,7 +731,7 @@ static struct hid_driver rmi_driver = {
>  #ifdef CONFIG_PM
>  	.suspend		= rmi_suspend,
>  	.resume			= rmi_post_resume,
> -	.reset_resume		= rmi_post_reset,
> +	.reset_resume		= rmi_post_resume,
>  #endif
>  };
>  
> -- 
> 2.5.0
> 

The rest looks OK.

Cheers,
Benjamin
--
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



[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux