[PATCH 0/2] add ad714x captouch sensor input driver

[PATCH 0/2] add ad714x captouch sensor input driver

[Barry Song]

Dmitry/David,
The following two patches add the analog devices captouch sensor ad714x(AD7142/AD7147)
driver.

AD7142 and AD7147 are integrated capacitance-to-digital converters (CDCs) with on-chip 
environmental calibration for use in systems requiring a novel user input method. 
The AD7142 and AD7147 can interface to external capacitance sensors implementing 
functions such as capacitive buttons, scrollwheels, sliders, touchpads and so on.

David,
The patch[1/2] adds a file with platform_data definition for AD714x in include/linux/spi,
which describes the components layout on special target boards and need be filled in 
arch/mach.

Dmitry,
The patch[2/2] adds a file in drivers/input/misc and changes Kconfig&Makefile, fulfilling
the algorithms and state machines for the related buttons/sliders/scrollwheels/touchpads
defined in platform_data. So the driver becomes be independent of special target boards.

The patch[2/2] depends on patch[1/2] for compiling and work.

Thanks
Barry

[PATCH 1/2] add ad714x platform_data definition

[Barry Song]

AD714x is very flexible, it can be used as buttons, scrollwheel,
slider, touchpad at the same time. That depends on the boards.
The platform_data for the device's "struct device" holds these
information.
The data-struct defined in this file descript the hardware feature
of button/scrollwheel/slider/touchpad components on target boards,
which need be filled in the arch/mach-/.

Signed-off-by: Barry Song <21cnbao@gmail.com>
---
 include/linux/spi/ad714x.h |   60 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 60 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/spi/ad714x.h

diff --git a/include/linux/spi/ad714x.h b/include/linux/spi/ad714x.h
new file mode 100644
index 0000000..f6439a9
--- /dev/null
+++ b/include/linux/spi/ad714x.h
@@ -0,0 +1,60 @@
+/*
+ * include/linux/ad714x.h
+ *
+ * AD714x is very flexible, it can be used as buttons, scrollwheel,
+ * slider, touchpad at the same time. That depends on the boards.
+ * The platform_data for the device's "struct device" holds this
+ * information.
+ */
+
+
+#ifndef AD714X_HW_H
+#define AD714X_HW_H
+
+#define STAGE_NUM              12
+#define STAGE_CFGREG_NUM       8
+#define SYS_CFGREG_NUM         8
+
+/* board information which need be initialized in arch/mach... */
+struct ad714x_slider_plat {
+	int start_stage;
+	int end_stage;
+	int max_coord;
+};
+
+struct ad714x_wheel_plat {
+	int start_stage;
+	int end_stage;
+	int max_coord;
+};
+
+struct ad714x_touchpad_plat {
+	int x_start_stage;
+	int x_end_stage;
+	int x_max_coord;
+
+	int y_start_stage;
+	int y_end_stage;
+	int y_max_coord;
+};
+
+struct ad714x_button_plat {
+	int keycode;
+	unsigned short l_mask;
+	unsigned short h_mask;
+};
+
+struct ad714x_platform_data {
+	int slider_num;
+	int wheel_num;
+	int touchpad_num;
+	int button_num;
+	struct ad714x_slider_plat *slider;
+	struct ad714x_wheel_plat *wheel;
+	struct ad714x_touchpad_plat *touchpad;
+	struct ad714x_button_plat *button;
+	unsigned short stage_cfg_reg[STAGE_NUM][STAGE_CFGREG_NUM];
+	unsigned short sys_cfg_reg[SYS_CFGREG_NUM];
+};
+
+#endif
-- 
1.5.6.3

[PATCH 2/2] add ad714x input driver for button/scrollwhell/slider/touchpad

[Barry Song]

ad7142/ad7147 are Programmable Controllers for Capacitance Touch Sensors.
The chips don't restrict the specific usage, and it can be used as button/
slider/scrollwheel/touchpad etc. depending on the hardware connection.
One special target board can include one or several these components.

The driver is independent of special boards. It gets the components
layout information from the platform_data, registers related devices,
fullfills the algorithms and state machines for these components and
report related input events to up level.

Signed-off-by: Barry Song <21cnbao@gmail.com>
---
 drivers/input/misc/ad714x.c | 1607 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 1607 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/misc/ad714x.c

diff --git a/drivers/input/misc/ad714x.c b/drivers/input/misc/ad714x.c
new file mode 100644
index 0000000..a9d4057
--- /dev/null
+++ b/drivers/input/misc/ad714x.c
@@ -0,0 +1,1607 @@
+/*
+ * File:         drivers/input/misc/ad714x.c
+ * Based on:
+ * Author:       Barry Song <Barry.Song@analog.com>
+ *
+ * Created:
+ * Description:  AD714X CapTouch Programmable Controller driver
+ *
+ * Modified:
+ *               Copyright 2009 Analog Devices Inc.
+ *
+ * Bugs:         Enter bugs at http://blackfin.uclinux.org/
+ *
+ * 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/device.h>
+#include <linux/init.h>
+#include <linux/spi/spi.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/spi/ad714x.h>
+
+#define AD714x_SPI_ADDR        0x1C
+#define AD714x_SPI_ADDR_SHFT   11
+#define AD714x_SPI_READ        1
+#define AD714x_SPI_READ_SHFT   10
+
+#define AD714X_PWR_CTRL           0x0
+#define AD714x_STG_CAL_EN_REG     0x1
+#define AD714X_AMB_COMP_CTRL0_REG 0x2
+#define AD714x_PARTID_REG         0x17
+#define AD7147_PARTID             0x1470
+#define AD7142_PARTID             0xE620
+#define AD714x_STAGECFG_REG       0x80
+#define AD714x_SYSCFG_REG         0x0
+
+#define STG_LOW_INT_EN_REG     0x5
+#define STG_HIGH_INT_EN_REG    0x6
+#define STG_COM_INT_EN_REG     0x7
+#define STG_LOW_INT_STA_REG    0x8
+#define STG_HIGH_INT_STA_REG   0x9
+#define STG_COM_INT_STA_REG    0xA
+
+#define CDC_RESULT_S0          0xB
+#define CDC_RESULT_S1          0xC
+#define CDC_RESULT_S2          0xD
+#define CDC_RESULT_S3          0xE
+#define CDC_RESULT_S4          0xF
+#define CDC_RESULT_S5          0x10
+#define CDC_RESULT_S6          0x11
+#define CDC_RESULT_S7          0x12
+#define CDC_RESULT_S8          0x13
+#define CDC_RESULT_S9          0x14
+#define CDC_RESULT_S10         0x15
+#define CDC_RESULT_S11         0x16
+
+#define STAGE0_AMBIENT		0xF1
+#define STAGE1_AMBIENT		0x115
+#define STAGE2_AMBIENT		0x139
+#define STAGE3_AMBIENT		0x15D
+#define STAGE4_AMBIENT		0x181
+#define STAGE5_AMBIENT		0x1A5
+#define STAGE6_AMBIENT		0x1C9
+#define STAGE7_AMBIENT		0x1ED
+#define STAGE8_AMBIENT		0x211
+#define STAGE9_AMBIENT		0x234
+#define STAGE10_AMBIENT		0x259
+#define STAGE11_AMBIENT		0x27D
+
+#define PER_STAGE_REG_NUM      36
+#define STAGE_NUM              12
+#define STAGE_CFGREG_NUM       8
+#define SYS_CFGREG_NUM         8
+
+#if defined(CONFIG_INPUT_AD714X_SPI)
+#define bus_device		struct spi_device
+#elif defined(CONFIG_INPUT_AD714X_I2C)
+#define bus_device		struct i2c_client
+#else
+#error Communication method needs to be selected (I2C or SPI)
+#endif
+
+/*
+ * driver information which will be used to maintain the software flow
+ */
+typedef enum {IDLE, JITTER, ACTIVE, SPACE} ad714x_device_state;
+
+struct ad714x_slider_drv {
+	int highest_stage;
+	int abs_pos;
+	int flt_pos;
+	ad714x_device_state state;
+	struct input_dev *input;
+};
+
+struct ad714x_wheel_drv {
+	int abs_pos;
+	int flt_pos;
+	int pre_mean_value;
+	int pre_highest_stage;
+	int pre_mean_value_no_offset;
+	int mean_value;
+	int mean_value_no_offset;
+	int pos_offset;
+	int pos_ratio;
+	int highest_stage;
+	ad714x_device_state state;
+	struct input_dev *input;
+};
+
+struct ad714x_touchpad_drv {
+	int x_highest_stage;
+	int x_flt_pos;
+	int x_abs_pos;
+	int y_highest_stage;
+	int y_flt_pos;
+	int y_abs_pos;
+	int left_ep;
+	int left_ep_val;
+	int right_ep;
+	int right_ep_val;
+	int top_ep;
+	int top_ep_val;
+	int bottom_ep;
+	int bottom_ep_val;
+	ad714x_device_state state;
+	struct input_dev *input;
+};
+
+struct ad714x_button_drv {
+	ad714x_device_state state;
+	/* Unlike slider/wheel/touchpad, all buttons point to
+	 * same input_dev instance */
+	struct input_dev *input;
+};
+
+struct ad714x_driver_data {
+	struct ad714x_slider_drv *slider;
+	struct ad714x_wheel_drv *wheel;
+	struct ad714x_touchpad_drv *touchpad;
+	struct ad714x_button_drv *button;
+};
+
+/* information to integrate all things which will be private data
+ * of spi/i2c device */
+struct ad714x_chip {
+	unsigned short h_state;
+	unsigned short l_state;
+	unsigned short c_state;
+	unsigned short adc_reg[STAGE_NUM];
+	unsigned short amb_reg[STAGE_NUM];
+	unsigned short sensor_val[STAGE_NUM];
+
+	struct ad714x_platform_data *hw;
+	struct ad714x_driver_data *sw;
+
+	bus_device *bus;
+	int (*read) (bus_device *, unsigned short, unsigned short *);
+	int (*write) (bus_device *, unsigned short, unsigned short);
+
+	struct mutex mutex;
+};
+
+static void stage_use_com_int(struct ad714x_chip *ad714x, int start_stage,
+		int end_stage)
+{
+	unsigned short data;
+	unsigned short mask;
+
+	mask = ((1 << (end_stage + 1)) - 1) - (1 << start_stage);
+
+	ad714x->read(ad714x->bus, STG_COM_INT_EN_REG, &data);
+	data |= 1 << start_stage;
+	ad714x->write(ad714x->bus, STG_COM_INT_EN_REG, data);
+
+	ad714x->read(ad714x->bus, STG_HIGH_INT_EN_REG, &data);
+	data &= ~mask;
+	ad714x->write(ad714x->bus, STG_HIGH_INT_EN_REG, data);
+}
+
+static void stage_use_thr_int(struct ad714x_chip *ad714x, int start_stage,
+		int end_stage)
+{
+	unsigned short data;
+	unsigned short mask;
+
+	mask = ((1 << (end_stage + 1)) - 1) - (1 << start_stage);
+
+	ad714x->read(ad714x->bus, STG_COM_INT_EN_REG, &data);
+	data &= ~(1 << start_stage);
+	ad714x->write(ad714x->bus, STG_COM_INT_EN_REG, data);
+
+	ad714x->read(ad714x->bus, STG_HIGH_INT_EN_REG, &data);
+	data |= mask;
+	ad714x->write(ad714x->bus, STG_HIGH_INT_EN_REG, data);
+}
+
+static int stage_cal_highest_stage(struct ad714x_chip *ad714x, int start_stage,
+		int end_stage)
+{
+	int max_res = 0;
+	int max_idx = 0;
+	int i;
+
+	for (i = start_stage; i <= end_stage; i++) {
+		if (ad714x->sensor_val[i] > max_res) {
+			max_res = ad714x->sensor_val[i];
+			max_idx = i;
+		}
+	}
+
+	return max_idx;
+}
+
+static int stage_cal_abs_pos(struct ad714x_chip *ad714x, int start_stage,
+		int end_stage, int highest_stage, int max_coord)
+{
+	int a_param, b_param;
+
+	if (highest_stage == start_stage) {
+		a_param = ad714x->sensor_val[start_stage + 1];
+		b_param = ad714x->sensor_val[start_stage] +
+			ad714x->sensor_val[start_stage + 1];
+	} else if (highest_stage == end_stage) {
+		a_param = ad714x->sensor_val[end_stage] *
+			(end_stage - start_stage) +
+			ad714x->sensor_val[end_stage - 1] *
+			(end_stage - start_stage - 1);
+		b_param = ad714x->sensor_val[end_stage] +
+			ad714x->sensor_val[end_stage - 1];
+	} else {
+		a_param = ad714x->sensor_val[highest_stage] *
+			(highest_stage - start_stage) +
+			ad714x->sensor_val[highest_stage - 1] *
+			(highest_stage - start_stage - 1) +
+			ad714x->sensor_val[highest_stage + 1] *
+			(highest_stage - start_stage + 1);
+		b_param = ad714x->sensor_val[highest_stage] +
+			ad714x->sensor_val[highest_stage - 1] +
+			ad714x->sensor_val[highest_stage + 1];
+	}
+
+	return (max_coord / (end_stage - start_stage)) * a_param / b_param;
+}
+
+
+/* One button can connect to multi postive and negative of CDCs
+ * Multi-buttons can connect to same postive/negative of one CDC
+ */
+static void button_state_machine(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_button_plat *hw = &ad714x->hw->button[idx];
+	struct ad714x_button_drv *sw = &ad714x->sw->button[idx];
+
+	switch (sw->state) {
+	case IDLE:
+		if (((ad714x->h_state & hw->h_mask) == hw->h_mask) &&
+			((ad714x->l_state & hw->l_mask) == hw->l_mask)) {
+			pr_debug("button %d touched\n", idx);
+			input_report_key(sw->input, hw->keycode, 1);
+			input_sync(sw->input);
+			sw->state = ACTIVE;
+		}
+		break;
+	case ACTIVE:
+		if (((ad714x->h_state & hw->h_mask) != hw->h_mask) ||
+			((ad714x->l_state & hw->l_mask) != hw->l_mask)) {
+			pr_debug("button %d released\n", idx);
+			input_report_key(sw->input, hw->keycode, 0);
+			input_sync(sw->input);
+			sw->state = IDLE;
+		}
+		break;
+	default:
+		break;
+	}
+}
+
+/* The response of a sensor is defined by the absolute number of codes
+ * between the current CDC value and the ambient value.
+ */
+void slider_cal_sensor_val(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_slider_plat *hw = &ad714x->hw->slider[idx];
+	int i;
+
+	for (i = hw->start_stage; i <= hw->end_stage; i++) {
+		ad714x->read(ad714x->bus, CDC_RESULT_S0 + i,
+			&ad714x->adc_reg[i]);
+		ad714x->read(ad714x->bus,
+				STAGE0_AMBIENT + i * PER_STAGE_REG_NUM,
+				&ad714x->amb_reg[i]);
+
+		ad714x->sensor_val[i] = abs(ad714x->adc_reg[i] -
+				ad714x->amb_reg[i]);
+	}
+}
+
+void slider_cal_highest_stage(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_slider_plat *hw = &ad714x->hw->slider[idx];
+	struct ad714x_slider_drv *sw = &ad714x->sw->slider[idx];
+
+	sw->highest_stage = stage_cal_highest_stage(ad714x, hw->start_stage,
+			hw->end_stage);
+
+	pr_debug("slider %d highest_stage:%d\n", idx, sw->highest_stage);
+}
+
+/* The formulae are very straight forward. It uses the sensor with the
+ * highest response and the 2 adjacent ones.
+ * When Sensor 0 has the highest response, only sensor 0 and sensor 1
+ * are used in the calculations. Similarly when the last sensor has the
+ * highest response, only the last sensor and the second last sensors
+ * are used in the calculations.
+ *
+ * For i= idx_of_peak_Sensor-1 to i= idx_of_peak_Sensor+1
+ *         v += Sensor response(i)*i
+ *         w += Sensor response(i)
+ * POS=(Number_of_Positions_Wanted/(Number_of_Sensors_Used-1)) *(v/w)
+ */
+void slider_cal_abs_pos(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_slider_plat *hw = &ad714x->hw->slider[idx];
+	struct ad714x_slider_drv *sw = &ad714x->sw->slider[idx];
+
+	sw->abs_pos = stage_cal_abs_pos(ad714x, hw->start_stage, hw->end_stage,
+		sw->highest_stage, hw->max_coord);
+
+	pr_debug("slider %d absolute position:%d\n", idx, sw->abs_pos);
+}
+
+/*
+ * To minimise the Impact of the noise on the algorithm, ADI developed a
+ * routine that filters the CDC results after they have been read by the
+ * host processor.
+ * The filter used is an Infinite Input Response(IIR) filter implemented
+ * in firmware and attenuates the noise on the CDC results after they've
+ * been read by the host processor.
+ * Filtered_CDC_result = (Filtered_CDC_result * (10 - Coefficient) +
+ * 				Latest_CDC_result * Coefficient)/10
+ */
+void slider_cal_flt_pos(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_slider_drv *sw = &ad714x->sw->slider[idx];
+
+	sw->flt_pos = (sw->flt_pos * (10 - 4) +
+			sw->abs_pos * 4)/10;
+
+	pr_debug("slider %d filter position:%d\n", idx, sw->flt_pos);
+}
+
+static void slider_use_com_int(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_slider_plat *hw = &ad714x->hw->slider[idx];
+	stage_use_com_int(ad714x, hw->start_stage, hw->end_stage);
+}
+
+static void slider_use_thr_int(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_slider_plat *hw = &ad714x->hw->slider[idx];
+	stage_use_thr_int(ad714x, hw->start_stage, hw->end_stage);
+}
+
+static void slider_state_machine(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_slider_plat *hw = &ad714x->hw->slider[idx];
+	struct ad714x_slider_drv *sw = &ad714x->sw->slider[idx];
+	unsigned short h_state, c_state;
+	unsigned short mask;
+
+	mask = ((1 << (hw->end_stage + 1)) - 1) - ((1 << hw->start_stage) - 1);
+
+	h_state = ad714x->h_state & mask;
+	c_state = ad714x->c_state & mask;
+
+	switch (sw->state) {
+	case IDLE:
+		if (h_state) {
+			sw->state = JITTER;
+			/* In End of Conversion interrupt mode, the AD714x
+			 * continuously generates hardware interrupts.
+			 */
+			slider_use_com_int(ad714x, idx);
+			pr_debug("slider %d touched\n", idx);
+		}
+		break;
+	case JITTER:
+		if (c_state == mask) {
+			slider_cal_sensor_val(ad714x, idx);
+			slider_cal_highest_stage(ad714x, idx);
+			slider_cal_abs_pos(ad714x, idx);
+			sw->flt_pos = sw->abs_pos;
+			sw->state = ACTIVE;
+		}
+		break;
+	case ACTIVE:
+		if (c_state == mask) {
+			if (h_state) {
+				slider_cal_sensor_val(ad714x, idx);
+				slider_cal_highest_stage(ad714x, idx);
+				slider_cal_abs_pos(ad714x, idx);
+				slider_cal_flt_pos(ad714x, idx);
+
+				input_report_abs(sw->input, ABS_X, sw->flt_pos);
+				input_report_abs(sw->input, ABS_PRESSURE, 1);
+			} else {
+				/* When the user lifts off the sensor, configure
+				 * the AD714X back to threshold interrupt mode.
+				 */
+				slider_use_thr_int(ad714x, idx);
+				sw->state = IDLE;
+				input_report_abs(sw->input, ABS_PRESSURE, 0);
+				pr_debug("slider %d released\n", idx);
+			}
+			input_sync(sw->input);
+		}
+		break;
+	default:
+		break;
+	}
+}
+
+/* When the scroll wheel is activated, we compute the absolute position based
+ * on the sensor values. To calculate the position, we first determine the
+ * sensor that has the greatest response among the 8 sensors that constitutes
+ * the scrollwheel. Then we determined the 2 sensors on either sides of the
+ * sensor with the highest response and we apply weights to these sensors.
+ */
+void wheel_cal_highest_stage(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_wheel_plat *hw = &ad714x->hw->wheel[idx];
+	struct ad714x_wheel_drv *sw = &ad714x->sw->wheel[idx];
+
+	sw->pre_highest_stage = sw->highest_stage;
+	sw->highest_stage = stage_cal_highest_stage(ad714x, hw->start_stage,
+			hw->end_stage);
+
+	pr_debug("wheel %d highest_stage:%d\n", idx, sw->highest_stage);
+}
+
+void wheel_cal_sensor_val(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_wheel_plat *hw = &ad714x->hw->wheel[idx];
+	int i;
+
+	for (i = hw->start_stage; i <= hw->end_stage; i++) {
+		ad714x->read(ad714x->bus, CDC_RESULT_S0 + i,
+			&ad714x->adc_reg[i]);
+		ad714x->read(ad714x->bus,
+				STAGE0_AMBIENT + i * PER_STAGE_REG_NUM,
+				&ad714x->amb_reg[i]);
+		if (ad714x->adc_reg[i] > ad714x->amb_reg[i])
+			ad714x->sensor_val[i] = ad714x->adc_reg[i] -
+				ad714x->amb_reg[i];
+		else
+			ad714x->sensor_val[i] = 0;
+	}
+}
+
+/* When the scroll wheel is activated, we compute the absolute position based
+ * on the sensor values. To calculate the position, we first determine the
+ * sensor that has the greatest response among the 8 sensors that constitutes
+ * the scrollwheel. Then we determined the 2 sensors on either sides of the
+ * sensor with the highest response and we apply weights to these sensors. The
+ * result of this computation gives us the mean value which defined by the
+ * following formula:
+ * For i= second_before_highest_stage to i= second_after_highest_stage
+ *         v += Sensor response(i)*WEIGHT*(i+3)
+ *         w += Sensor response(i)
+ * Mean_Value=v/w
+ * pos_on_scrollwheel = (Mean_Value - position_offset) / position_ratio
+ *
+ */
+
+#define WEIGHT_FACTOR 30
+/* This constant prevents the "PositionOffset" from reaching a big value */
+#define OFFSET_POSITION_CLAMP	120
+void wheel_cal_abs_pos(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_wheel_plat *hw = &ad714x->hw->wheel[idx];
+	struct ad714x_wheel_drv *sw = &ad714x->sw->wheel[idx];
+	int stage_num = hw->end_stage - hw->start_stage + 1;
+	int second_before, first_before, highest, first_after, second_after;
+	int a_param, b_param;
+
+	/* Calculate Mean value */
+
+	second_before = (sw->highest_stage + stage_num - 2) % stage_num;
+	first_before = (sw->highest_stage + stage_num - 1) % stage_num;
+	highest = sw->highest_stage;
+	first_after = (sw->highest_stage + stage_num + 1) % stage_num;
+	second_after = (sw->highest_stage + stage_num + 2) % stage_num;
+
+	if (((sw->highest_stage - hw->start_stage) > 1) &&
+			((hw->end_stage - sw->highest_stage) > 1)) {
+		a_param = ad714x->sensor_val[second_before] *
+			(second_before - hw->start_stage + 3) +
+			ad714x->sensor_val[first_before] *
+			(second_before - hw->start_stage + 3) +
+			ad714x->sensor_val[highest] *
+			(second_before - hw->start_stage + 3) +
+			ad714x->sensor_val[first_after] *
+			(first_after - hw->start_stage + 3) +
+			ad714x->sensor_val[second_after] *
+			(second_after - hw->start_stage + 3);
+	} else {
+		a_param = ad714x->sensor_val[second_before] *
+			(second_before - hw->start_stage + 1) +
+			ad714x->sensor_val[first_before] *
+			(second_before - hw->start_stage + 2) +
+			ad714x->sensor_val[highest] *
+			(second_before - hw->start_stage + 3) +
+			ad714x->sensor_val[first_after] *
+			(first_after - hw->start_stage + 4) +
+			ad714x->sensor_val[second_after] *
+			(second_after - hw->start_stage + 5);
+	}
+	a_param *= WEIGHT_FACTOR;
+
+	b_param = ad714x->sensor_val[second_before] +
+		ad714x->sensor_val[first_before] +
+		ad714x->sensor_val[highest] +
+		ad714x->sensor_val[first_after] +
+		ad714x->sensor_val[second_after];
+
+	sw->pre_mean_value = sw->mean_value;
+	sw->mean_value = a_param / b_param;
+
+	/* Calculate the offset */
+
+	if ((sw->pre_highest_stage == hw->end_stage) &&
+			(sw->highest_stage == hw->start_stage))
+		sw->pos_offset = sw->mean_value;
+	else if ((sw->pre_highest_stage == hw->start_stage) &&
+			(sw->highest_stage == hw->end_stage))
+		sw->pos_offset = sw->pre_mean_value;
+	if (sw->pos_offset > OFFSET_POSITION_CLAMP)
+		sw->pos_offset = OFFSET_POSITION_CLAMP;
+
+	/* Calculate the mean value without the offset */
+
+	sw->pre_mean_value_no_offset = sw->mean_value_no_offset;
+	sw->mean_value_no_offset = sw->mean_value - sw->pos_offset;
+	if (sw->mean_value_no_offset < 0)
+		sw->mean_value_no_offset = 0;
+
+	/* Calculate ratio to scale down to NUMBER_OF_WANTED_POSITIONS */
+
+	if ((sw->pre_highest_stage == hw->end_stage) &&
+			(sw->highest_stage == hw->start_stage))
+		sw->pos_ratio = (sw->pre_mean_value_no_offset * 100) /
+			hw->max_coord;
+	else if ((sw->pre_highest_stage == hw->start_stage) &&
+			(sw->highest_stage == hw->end_stage))
+		sw->pos_ratio = (sw->mean_value_no_offset * 100) /
+			hw->max_coord;
+	sw->abs_pos = (sw->mean_value_no_offset * 100) / sw->pos_ratio;
+	if (sw->abs_pos > hw->max_coord)
+		sw->abs_pos = hw->max_coord;
+}
+
+void wheel_cal_flt_pos(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_wheel_plat *hw = &ad714x->hw->wheel[idx];
+	struct ad714x_wheel_drv *sw = &ad714x->sw->wheel[idx];
+	if (((sw->pre_highest_stage == hw->end_stage) &&
+				(sw->highest_stage == hw->start_stage)) ||
+			((sw->pre_highest_stage == hw->start_stage) &&
+			 (sw->highest_stage == hw->end_stage)))
+		sw->flt_pos = sw->abs_pos;
+	else
+		sw->flt_pos = ((sw->flt_pos * 30) + (sw->abs_pos * 71)) / 100;
+
+	if (sw->flt_pos > hw->max_coord)
+		sw->flt_pos = hw->max_coord;
+}
+
+static void wheel_use_com_int(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_wheel_plat *hw = &ad714x->hw->wheel[idx];
+	stage_use_com_int(ad714x, hw->start_stage, hw->end_stage);
+}
+
+static void wheel_use_thr_int(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_wheel_plat *hw = &ad714x->hw->wheel[idx];
+	stage_use_thr_int(ad714x, hw->start_stage, hw->end_stage);
+}
+
+static void wheel_state_machine(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_wheel_plat *hw = &ad714x->hw->wheel[idx];
+	struct ad714x_wheel_drv *sw = &ad714x->sw->wheel[idx];
+	unsigned short h_state, c_state;
+	unsigned short mask;
+
+	mask = ((1 << (hw->end_stage + 1)) - 1) - ((1 << hw->start_stage) - 1);
+
+	h_state = ad714x->h_state & mask;
+	c_state = ad714x->c_state & mask;
+
+	switch (sw->state) {
+	case IDLE:
+		if (h_state) {
+			sw->state = JITTER;
+			/* In End of Conversion interrupt mode, the AD714x
+			 * continuously generates hardware interrupts.
+			 */
+			wheel_use_com_int(ad714x, idx);
+			pr_debug("wheel %d touched\n", idx);
+		}
+		break;
+	case JITTER:
+		if (c_state == mask)	{
+			wheel_cal_sensor_val(ad714x, idx);
+			wheel_cal_highest_stage(ad714x, idx);
+			wheel_cal_abs_pos(ad714x, idx);
+			sw->flt_pos = sw->abs_pos;
+			sw->state = ACTIVE;
+		}
+		break;
+	case ACTIVE:
+		if (c_state == mask) {
+			if (h_state) {
+				wheel_cal_sensor_val(ad714x, idx);
+				wheel_cal_highest_stage(ad714x, idx);
+				wheel_cal_abs_pos(ad714x, idx);
+
+				input_report_abs(sw->input, ABS_WHEEL,
+					sw->abs_pos);
+				input_report_abs(sw->input, ABS_PRESSURE, 1);
+			} else {
+				/* When the user lifts off the sensor, configure
+				 * the AD714X back to threshold interrupt mode.
+				 */
+				wheel_use_thr_int(ad714x, idx);
+				sw->state = IDLE;
+				input_report_abs(sw->input, ABS_PRESSURE, 0);
+
+				pr_debug("wheel %d released\n", idx);
+			}
+			input_sync(sw->input);
+		}
+		break;
+	default:
+		break;
+	}
+}
+
+void touchpad_cal_sensor_val(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_touchpad_plat *hw = &ad714x->hw->touchpad[idx];
+	int i;
+
+	for (i = hw->x_start_stage; i <= hw->x_end_stage; i++) {
+		ad714x->read(ad714x->bus, CDC_RESULT_S0 + i,
+				&ad714x->adc_reg[i]);
+		ad714x->read(ad714x->bus,
+				STAGE0_AMBIENT + i * PER_STAGE_REG_NUM,
+				&ad714x->amb_reg[i]);
+		if (ad714x->adc_reg[i] > ad714x->amb_reg[i])
+			ad714x->sensor_val[i] = ad714x->adc_reg[i] -
+				ad714x->amb_reg[i];
+		else
+			ad714x->sensor_val[i] = 0;
+	}
+}
+
+static void touchpad_cal_highest_stage(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_touchpad_plat *hw = &ad714x->hw->touchpad[idx];
+	struct ad714x_touchpad_drv *sw = &ad714x->sw->touchpad[idx];
+
+	sw->x_highest_stage = stage_cal_highest_stage(ad714x, hw->x_start_stage,
+			hw->x_end_stage);
+	sw->y_highest_stage = stage_cal_highest_stage(ad714x, hw->y_start_stage,
+			hw->y_end_stage);
+
+	pr_debug("touchpad %d x_highest_stage:%d, y_highest_stage:%d\n",
+			idx, sw->x_highest_stage, sw->y_highest_stage);
+}
+
+/* If 2 fingers are touching the sensor then 2 peaks can be observed in the
+ * distribution.
+ * The arithmetic doesn't support to get absolute coordinates for multi-touch
+ * yet.
+ */
+static int touchpad_check_second_peak(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_touchpad_plat *hw = &ad714x->hw->touchpad[idx];
+	struct ad714x_touchpad_drv *sw = &ad714x->sw->touchpad[idx];
+	int i;
+
+	for (i = hw->x_start_stage; i < sw->x_highest_stage; i++) {
+		if ((ad714x->sensor_val[i] - ad714x->sensor_val[i + 1])
+			> (ad714x->sensor_val[i + 1] / 10))
+			return 1;
+	}
+
+	for (i = sw->x_highest_stage; i < hw->x_end_stage; i++) {
+		if ((ad714x->sensor_val[i + 1] - ad714x->sensor_val[i])
+			> (ad714x->sensor_val[i] / 10))
+			return 1;
+	}
+
+	for (i = hw->y_start_stage; i < sw->y_highest_stage; i++) {
+		if ((ad714x->sensor_val[i] - ad714x->sensor_val[i + 1])
+			> (ad714x->sensor_val[i + 1] / 10))
+			return 1;
+	}
+
+	for (i = sw->y_highest_stage; i < hw->y_end_stage; i++) {
+		if ((ad714x->sensor_val[i + 1] - ad714x->sensor_val[i])
+			> (ad714x->sensor_val[i] / 10))
+			return 1;
+	}
+
+	return 0;
+}
+
+/* If only one finger is used to activate the touch pad then only 1 peak will be
+ * registered in the distribution. This peak and the 2 adjacent sensors will be
+ * used in the calculation of the absolute position. This will prevent hand
+ * shadows to affect the absolute position calculation.
+ */
+static void touchpad_cal_abs_pos(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_touchpad_plat *hw = &ad714x->hw->touchpad[idx];
+	struct ad714x_touchpad_drv *sw = &ad714x->sw->touchpad[idx];
+
+	sw->x_abs_pos = stage_cal_abs_pos(ad714x, hw->x_start_stage,
+			hw->x_end_stage, sw->x_highest_stage, hw->x_max_coord);
+	sw->y_abs_pos = stage_cal_abs_pos(ad714x, hw->y_start_stage,
+			hw->y_end_stage, sw->y_highest_stage, hw->y_max_coord);
+
+	pr_debug("touchpad %d absolute position:(%d, %d)\n", idx,
+			sw->x_abs_pos, sw->y_abs_pos);
+}
+
+static void touchpad_cal_flt_pos(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_touchpad_drv *sw = &ad714x->sw->touchpad[idx];
+
+	sw->x_flt_pos = (sw->x_flt_pos * (10 - 4) +
+			sw->x_abs_pos * 4)/10;
+	sw->y_flt_pos = (sw->y_flt_pos * (10 - 4) +
+			sw->y_abs_pos * 4)/10;
+
+	pr_debug("touchpad %d filter position:(%d, %d)\n",
+			idx, sw->x_flt_pos, sw->y_flt_pos);
+}
+
+/* To prevent distortion from showing in the absolute position, it is
+ * necessary to detect the end points. When endpoints are detected, the
+ * driver stops updating the status variables with absolute positions.
+ * End points are detected on the 4 edges of the touchpad sensor. The
+ * method to detect them is the same for all 4.
+ * To detect the end points, the firmware computes the difference in
+ * percent between the sensor on the edge and the adjacent one. The
+ * difference is calculated in percent in order to make the end point
+ * detection independent of the pressure.
+ */
+
+#define LEFT_END_POINT_DETECTION_LEVEL			550
+#define RIGHT_END_POINT_DETECTION_LEVEL			750
+#define LEFT_RIGHT_END_POINT_DEAVTIVALION_LEVEL		850
+#define TOP_END_POINT_DETECTION_LEVEL			550
+#define BOTTOM_END_POINT_DETECTION_LEVEL		950
+#define TOP_BOTTOM_END_POINT_DEAVTIVALION_LEVEL		700
+static int touchpad_check_endpoint(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_touchpad_plat *hw = &ad714x->hw->touchpad[idx];
+	struct ad714x_touchpad_drv *sw  = &ad714x->sw->touchpad[idx];
+	int percent_sensor_diff;
+
+	/* left endpoint detect */
+	percent_sensor_diff = (ad714x->sensor_val[hw->x_start_stage] -
+			ad714x->sensor_val[hw->x_start_stage + 1]) * 100 /
+			ad714x->sensor_val[hw->x_start_stage + 1];
+	if (!sw->left_ep) {
+		if (percent_sensor_diff >= LEFT_END_POINT_DETECTION_LEVEL)  {
+			sw->left_ep = 1;
+			sw->left_ep_val =
+				ad714x->sensor_val[hw->x_start_stage + 1];
+		}
+	} else {
+		if ((percent_sensor_diff < LEFT_END_POINT_DETECTION_LEVEL) &&
+		(ad714x->sensor_val[hw->x_start_stage + 1] >
+		LEFT_RIGHT_END_POINT_DEAVTIVALION_LEVEL + sw->left_ep_val))
+			sw->left_ep = 0;
+	}
+
+	/* right endpoint detect */
+	percent_sensor_diff = (ad714x->sensor_val[hw->x_end_stage] -
+			ad714x->sensor_val[hw->x_end_stage - 1]) * 100 /
+			ad714x->sensor_val[hw->x_end_stage - 1];
+	if (!sw->right_ep) {
+		if (percent_sensor_diff >= RIGHT_END_POINT_DETECTION_LEVEL)  {
+			sw->right_ep = 1;
+			sw->right_ep_val =
+				ad714x->sensor_val[hw->x_end_stage - 1];
+		}
+	} else {
+		if ((percent_sensor_diff < RIGHT_END_POINT_DETECTION_LEVEL) &&
+		(ad714x->sensor_val[hw->x_end_stage - 1] >
+		LEFT_RIGHT_END_POINT_DEAVTIVALION_LEVEL + sw->right_ep_val))
+			sw->right_ep = 0;
+	}
+
+	/* top endpoint detect */
+	percent_sensor_diff = (ad714x->sensor_val[hw->y_start_stage] -
+			ad714x->sensor_val[hw->y_start_stage + 1]) * 100 /
+			ad714x->sensor_val[hw->y_start_stage + 1];
+	if (!sw->top_ep) {
+		if (percent_sensor_diff >= TOP_END_POINT_DETECTION_LEVEL)  {
+			sw->top_ep = 1;
+			sw->top_ep_val =
+				ad714x->sensor_val[hw->y_start_stage + 1];
+		}
+	} else {
+		if ((percent_sensor_diff < TOP_END_POINT_DETECTION_LEVEL) &&
+		(ad714x->sensor_val[hw->y_start_stage + 1] >
+		TOP_BOTTOM_END_POINT_DEAVTIVALION_LEVEL + sw->top_ep_val))
+			sw->top_ep = 0;
+	}
+
+	/* bottom endpoint detect */
+	percent_sensor_diff = (ad714x->sensor_val[hw->y_end_stage] -
+		ad714x->sensor_val[hw->y_end_stage - 1]) * 100 /
+		ad714x->sensor_val[hw->y_end_stage - 1];
+	if (!sw->bottom_ep) {
+		if (percent_sensor_diff >= BOTTOM_END_POINT_DETECTION_LEVEL)  {
+			sw->bottom_ep = 1;
+			sw->bottom_ep_val =
+				ad714x->sensor_val[hw->y_end_stage - 1];
+		}
+	} else {
+		if ((percent_sensor_diff < BOTTOM_END_POINT_DETECTION_LEVEL) &&
+		(ad714x->sensor_val[hw->y_end_stage - 1] >
+		 TOP_BOTTOM_END_POINT_DEAVTIVALION_LEVEL + sw->bottom_ep_val))
+			sw->bottom_ep = 0;
+	}
+
+	return sw->left_ep || sw->right_ep || sw->top_ep || sw->bottom_ep;
+}
+
+static void touchpad_use_com_int(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_touchpad_plat *hw = &ad714x->hw->touchpad[idx];
+	stage_use_com_int(ad714x, hw->x_start_stage, hw->x_end_stage);
+}
+
+static void touchpad_use_thr_int(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_touchpad_plat *hw = &ad714x->hw->touchpad[idx];
+	stage_use_thr_int(ad714x, hw->x_start_stage, hw->x_end_stage);
+	stage_use_thr_int(ad714x, hw->y_start_stage, hw->y_end_stage);
+}
+
+static void touchpad_state_machine(struct ad714x_chip *ad714x, int idx)
+{
+	struct ad714x_touchpad_plat *hw = &ad714x->hw->touchpad[idx];
+	struct ad714x_touchpad_drv *sw = &ad714x->sw->touchpad[idx];
+	unsigned short h_state, c_state;
+	unsigned short mask;
+
+	mask = (((1 << (hw->x_end_stage + 1)) - 1) -
+		((1 << hw->x_start_stage) - 1)) +
+		(((1 << (hw->y_end_stage + 1)) - 1) -
+		((1 << hw->y_start_stage) - 1));
+
+	h_state = ad714x->h_state & mask;
+	c_state = ad714x->c_state & mask;
+
+	switch (sw->state) {
+	case IDLE:
+		if (h_state) {
+			sw->state = JITTER;
+			/* In End of Conversion interrupt mode, the AD714x
+			 * continuously generates hardware interrupts.
+			 */
+			touchpad_use_com_int(ad714x, idx);
+			pr_debug("touchpad %d touched\n", idx);
+		}
+		break;
+	case JITTER:
+		if (c_state == mask) {
+			touchpad_cal_sensor_val(ad714x, idx);
+			touchpad_cal_highest_stage(ad714x, idx);
+			if ((!touchpad_check_second_peak(ad714x, idx)) &&
+				(!touchpad_check_endpoint(ad714x, idx))) {
+				pr_debug("touchpad%d, 2 fingers or endpoint\n",
+						idx);
+				touchpad_cal_abs_pos(ad714x, idx);
+				sw->x_flt_pos = sw->x_abs_pos;
+				sw->y_flt_pos = sw->y_abs_pos;
+				sw->state = ACTIVE;
+			}
+		}
+		break;
+	case ACTIVE:
+		if (c_state == mask) {
+			if (h_state) {
+				touchpad_cal_sensor_val(ad714x, idx);
+				touchpad_cal_highest_stage(ad714x, idx);
+				if ((!touchpad_check_second_peak(ad714x, idx))
+				  && (!touchpad_check_endpoint(ad714x, idx))) {
+					touchpad_cal_abs_pos(ad714x, idx);
+					touchpad_cal_flt_pos(ad714x, idx);
+					input_report_abs(sw->input, ABS_X,
+						sw->x_flt_pos);
+					input_report_abs(sw->input, ABS_Y,
+						sw->y_flt_pos);
+					input_report_abs(sw->input,
+						ABS_PRESSURE, 1);
+				}
+			} else {
+				/* When the user lifts off the sensor, configure
+				 * the AD714X back to threshold interrupt mode.
+				 */
+				touchpad_use_thr_int(ad714x, idx);
+				sw->state = IDLE;
+				input_report_abs(sw->input, ABS_PRESSURE, 0);
+				pr_debug("touchpad %d released\n", idx);
+			}
+			input_sync(sw->input);
+		}
+		break;
+	default:
+		break;
+	}
+}
+
+static int ad714x_hw_detect(struct ad714x_chip *ad714x)
+{
+	unsigned short data;
+
+	ad714x->read(ad714x->bus, AD714x_PARTID_REG, &data);
+	switch (data & 0xFFF0) {
+	case AD7147_PARTID:
+		dev_info(&ad714x->bus->dev, "Found AD7147 captouch, rev:%d\n",
+				data & 0xF);
+		return 0;
+	case AD7142_PARTID:
+		dev_info(&ad714x->bus->dev, "Found AD7142 captouch, rev:%d\n",
+				data & 0xF);
+		return 0;
+	default:
+		dev_err(&ad714x->bus->dev, "Fail to detect AD714x captouch,\
+				read ID is %04x\n", data);
+		return -ENODEV;
+	}
+}
+
+static int ad714x_hw_init(struct ad714x_chip *ad714x)
+{
+	int i, j;
+	unsigned short reg_base;
+	unsigned short data;
+
+	/* configuration CDC and interrupts*/
+
+	for (i = 0; i < STAGE_NUM; i++) {
+		reg_base = AD714x_STAGECFG_REG + i * STAGE_CFGREG_NUM;
+		for (j = 0; j < STAGE_CFGREG_NUM; j++)
+			ad714x->write(ad714x->bus, reg_base + j,
+				ad714x->hw->stage_cfg_reg[i][j]);
+	}
+
+	for (i = 0; i < SYS_CFGREG_NUM; i++)
+		ad714x->write(ad714x->bus, AD714x_SYSCFG_REG + i,
+			ad714x->hw->sys_cfg_reg[i]);
+
+	ad714x->write(ad714x->bus, AD714x_STG_CAL_EN_REG, 0xFFF);
+
+	/* clear all interrupts */
+	ad714x->read(ad714x->bus, STG_LOW_INT_STA_REG, &data);
+	ad714x->read(ad714x->bus, STG_HIGH_INT_STA_REG, &data);
+	ad714x->read(ad714x->bus, STG_COM_INT_STA_REG, &data);
+
+	return 0;
+}
+
+static irqreturn_t ad714x_interrupt_thread(int irq, void *data)
+{
+	struct ad714x_chip *ad714x = data;
+	int i;
+
+	mutex_lock(&ad714x->mutex);
+
+	ad714x->read(ad714x->bus, STG_LOW_INT_STA_REG, &ad714x->l_state);
+	ad714x->read(ad714x->bus, STG_HIGH_INT_STA_REG, &ad714x->h_state);
+	ad714x->read(ad714x->bus, STG_COM_INT_STA_REG, &ad714x->c_state);
+
+	pr_debug("%s l_state:%04x h_state:%04x c_stage:%04x\n", __func__,
+			ad714x->l_state, ad714x->h_state, ad714x->c_state);
+
+	for (i = 0; i < ad714x->hw->button_num; i++)
+		button_state_machine(ad714x, i);
+	for (i = 0; i < ad714x->hw->slider_num; i++)
+		slider_state_machine(ad714x, i);
+	for (i = 0; i < ad714x->hw->wheel_num; i++)
+		wheel_state_machine(ad714x, i);
+	for (i = 0; i < ad714x->hw->touchpad_num; i++)
+		touchpad_state_machine(ad714x, i);
+
+	mutex_unlock(&ad714x->mutex);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t ad714x_interrupt(int irq, void *data)
+{
+	return IRQ_WAKE_THREAD;
+}
+
+#define MAX_DEVICE_NUM 8
+static int __devinit ad714x_probe(struct ad714x_chip *ad714x)
+{
+	int ret = 0;
+	struct input_dev *input[MAX_DEVICE_NUM];
+
+	struct ad714x_driver_data *drv_data = NULL;
+
+	struct ad714x_button_plat *bt_plat   = ad714x->hw->button;
+	struct ad714x_slider_plat *sd_plat   = ad714x->hw->slider;
+	struct ad714x_wheel_plat *wl_plat    = ad714x->hw->wheel;
+	struct ad714x_touchpad_plat *tp_plat = ad714x->hw->touchpad;
+
+	struct ad714x_button_drv *bt_drv   = NULL;
+	struct ad714x_slider_drv *sd_drv   = NULL;
+	struct ad714x_wheel_drv *wl_drv    = NULL;
+	struct ad714x_touchpad_drv *tp_drv = NULL;
+
+	int alloc_idx = 0, reg_idx = 0;
+	int i;
+
+	ret = ad714x_hw_detect(ad714x);
+	if (ret)
+		goto det_err;
+
+	/*
+	 * Allocate and register AD714x input device
+	 */
+
+	drv_data = kzalloc(sizeof(struct ad714x_driver_data), GFP_KERNEL);
+	if (!drv_data) {
+		dev_err(&ad714x->bus->dev,
+			"Can't allocate memory for ad714x driver info\n");
+		ret = -ENOMEM;
+		goto fail_alloc_reg;
+	}
+	ad714x->sw = drv_data;
+
+	/* a slider uses one input_dev instance */
+	if (ad714x->hw->slider_num > 0) {
+		sd_drv = kzalloc(sizeof(struct ad714x_slider_drv) *
+				ad714x->hw->slider_num, GFP_KERNEL);
+		if (!sd_drv) {
+			dev_err(&ad714x->bus->dev,
+				"Can't allocate memory for slider info\n");
+			ret = -ENOMEM;
+			goto fail_alloc_reg;
+		}
+
+		for (i = 0; i < ad714x->hw->slider_num; i++) {
+			input[alloc_idx] = input_allocate_device();
+			if (!input[alloc_idx]) {
+				dev_err(&ad714x->bus->dev,
+				"Can't allocate input device %d\n", alloc_idx);
+				ret = -ENOMEM;
+				goto fail_alloc_reg;
+			}
+			alloc_idx++;
+
+			__set_bit(EV_ABS, input[alloc_idx-1]->evbit);
+			__set_bit(ABS_X, input[alloc_idx-1]->absbit);
+			__set_bit(ABS_PRESSURE, input[alloc_idx-1]->absbit);
+			input_set_abs_params(input[alloc_idx-1], ABS_X, 0,
+					sd_plat->max_coord, 0, 0);
+			input_set_abs_params(input[alloc_idx-1], ABS_PRESSURE,
+				0, 1, 0, 0);
+
+			input[alloc_idx-1]->id.bustype = BUS_I2C;
+
+			ret = input_register_device(input[reg_idx]);
+			if (ret) {
+				dev_err(&ad714x->bus->dev,
+				"Failed to register AD714x input device!\n");
+				goto fail_alloc_reg;
+			}
+			reg_idx++;
+
+			sd_drv[i].input = input[alloc_idx-1];
+			ad714x->sw->slider = sd_drv;
+		}
+	}
+
+	/* a wheel uses one input_dev instance */
+	if (ad714x->hw->wheel_num > 0) {
+		wl_drv = kzalloc(sizeof(struct ad714x_wheel_drv) *
+				ad714x->hw->wheel_num, GFP_KERNEL);
+		if (!wl_drv) {
+			dev_err(&ad714x->bus->dev,
+				"Can't allocate memory for wheel info\n");
+			ret = -ENOMEM;
+			goto fail_alloc_reg;
+		}
+
+		for (i = 0; i < ad714x->hw->wheel_num; i++) {
+			input[alloc_idx] = input_allocate_device();
+			if (!input[alloc_idx]) {
+				dev_err(&ad714x->bus->dev,
+				"Can't allocate input device %d\n", alloc_idx);
+				ret = -ENOMEM;
+				goto fail_alloc_reg;
+			}
+			alloc_idx++;
+
+			__set_bit(EV_ABS, input[alloc_idx-1]->evbit);
+			__set_bit(ABS_WHEEL, input[alloc_idx-1]->absbit);
+			__set_bit(ABS_PRESSURE, input[alloc_idx-1]->absbit);
+			input_set_abs_params(input[alloc_idx-1], ABS_WHEEL, 0,
+					wl_plat->max_coord, 0, 0);
+			input_set_abs_params(input[alloc_idx-1], ABS_PRESSURE,
+				0, 1, 0, 0);
+
+			input[alloc_idx-1]->id.bustype = BUS_I2C;
+
+			ret = input_register_device(input[reg_idx]);
+			if (ret) {
+				dev_err(&ad714x->bus->dev,
+				"Failed to register AD714x input device!\n");
+				goto fail_alloc_reg;
+			}
+			reg_idx++;
+
+			wl_drv[i].input = input[alloc_idx-1];
+			ad714x->sw->wheel = wl_drv;
+		}
+	}
+
+	/* a touchpad uses one input_dev instance */
+	if (ad714x->hw->touchpad_num > 0) {
+		tp_drv = kzalloc(sizeof(struct ad714x_touchpad_drv) *
+				ad714x->hw->touchpad_num, GFP_KERNEL);
+		if (!tp_drv) {
+			dev_err(&ad714x->bus->dev,
+				"Can't allocate memory for touchpad info\n");
+			ret = -ENOMEM;
+			goto fail_alloc_reg;
+		}
+
+		for (i = 0; i < ad714x->hw->touchpad_num; i++) {
+			input[alloc_idx] = input_allocate_device();
+			if (!input[alloc_idx]) {
+				dev_err(&ad714x->bus->dev,
+					"Can't allocate input device %d\n",
+					alloc_idx);
+				ret = -ENOMEM;
+				goto fail_alloc_reg;
+			}
+			alloc_idx++;
+
+			__set_bit(EV_ABS, input[alloc_idx-1]->evbit);
+			__set_bit(ABS_X, input[alloc_idx-1]->absbit);
+			__set_bit(ABS_Y, input[alloc_idx-1]->absbit);
+			__set_bit(ABS_PRESSURE, input[alloc_idx-1]->absbit);
+			input_set_abs_params(input[alloc_idx-1], ABS_X, 0,
+					tp_plat->x_max_coord, 0, 0);
+			input_set_abs_params(input[alloc_idx-1], ABS_Y, 0,
+					tp_plat->y_max_coord, 0, 0);
+			input_set_abs_params(input[alloc_idx-1], ABS_PRESSURE,
+				0, 1, 0, 0);
+
+			input[alloc_idx-1]->id.bustype = BUS_I2C;
+
+			ret = input_register_device(input[reg_idx]);
+			if (ret) {
+				dev_err(&ad714x->bus->dev,
+				"Failed to register AD714x input device!\n");
+				goto fail_alloc_reg;
+			}
+			reg_idx++;
+
+			tp_drv[i].input = input[alloc_idx-1];
+			ad714x->sw->touchpad = tp_drv;
+		}
+	}
+
+	/* all buttons use one input node */
+	if (ad714x->hw->button_num > 0) {
+		bt_drv = kzalloc(sizeof(struct ad714x_button_drv) *
+				ad714x->hw->button_num, GFP_KERNEL);
+		if (!bt_drv) {
+			dev_err(&ad714x->bus->dev,
+				"Can't allocate memory for button info\n");
+			ret = -ENOMEM;
+			goto fail_alloc_reg;
+		}
+
+		input[alloc_idx] = input_allocate_device();
+		if (!input[alloc_idx]) {
+			dev_err(&ad714x->bus->dev,
+					"Can't allocate input device %d\n",
+					alloc_idx);
+			ret = -ENOMEM;
+			goto fail_alloc_reg;
+		}
+		alloc_idx++;
+
+		__set_bit(EV_KEY, input[alloc_idx-1]->evbit);
+		for (i = 0; i < ad714x->hw->button_num; i++) {
+			__set_bit(bt_plat[i].keycode,
+				input[alloc_idx-1]->keybit);
+		}
+
+		input[alloc_idx-1]->id.bustype = BUS_I2C;
+
+		ret = input_register_device(input[reg_idx]);
+		if (ret) {
+			dev_err(&ad714x->bus->dev,
+				"Failed to register AD714x input device!\n");
+			goto fail_alloc_reg;
+		}
+		reg_idx++;
+
+		for (i = 0; i < ad714x->hw->button_num; i++)
+			bt_drv[i].input = input[alloc_idx-1];
+		ad714x->sw->button = bt_drv;
+	}
+
+	/* initilize and request sw/hw resources */
+
+	ad714x_hw_init(ad714x);
+	mutex_init(&ad714x->mutex);
+
+	if (ad714x->bus->irq > 0) {
+		ret = request_threaded_irq(ad714x->bus->irq, ad714x_interrupt,
+				ad714x_interrupt_thread, IRQF_TRIGGER_FALLING,
+				"ad714x_captouch", ad714x);
+		if (ret) {
+			dev_err(&ad714x->bus->dev, "Can't allocate irq %d\n",
+					ad714x->bus->irq);
+			goto fail_irq;
+		}
+	} else
+		dev_warn(&ad714x->bus->dev, "IRQ not configured!\n");
+
+	return 0;
+
+fail_irq:
+fail_alloc_reg:
+	for (i = 0; i < reg_idx; i++)
+		input_unregister_device(input[i]);
+	for (i = 0; i < alloc_idx; i++)
+		input_free_device(input[i]);
+	kfree(bt_drv); /* kfree(NULL) is safe check is not required */
+	kfree(sd_drv);
+	kfree(wl_drv);
+	kfree(tp_drv);
+	kfree(drv_data);
+det_err:
+	return ret;
+}
+
+static int __devexit ad714x_remove(struct ad714x_chip *ad714x)
+{
+	int i;
+
+	struct ad714x_driver_data *drv_data = ad714x->sw;
+	struct ad714x_button_drv *bt_drv   = ad714x->sw->button;
+	struct ad714x_slider_drv *sd_drv   = ad714x->sw->slider;
+	struct ad714x_wheel_drv *wl_drv    = ad714x->sw->wheel;
+	struct ad714x_touchpad_drv *tp_drv = ad714x->sw->touchpad;
+
+	/* free irq hardware resource */
+
+	if (ad714x->bus->irq > 0)
+		free_irq(ad714x->bus->irq, ad714x);
+
+	/* unregister and free all input devices */
+
+	for (i = 0; i < ad714x->hw->slider_num; i++) {
+		input_unregister_device(ad714x->sw->slider[i].input);
+		input_free_device(ad714x->sw->slider[i].input);
+	}
+
+	for (i = 0; i < ad714x->hw->wheel_num; i++) {
+		input_unregister_device(ad714x->sw->wheel[i].input);
+		input_free_device(ad714x->sw->wheel[i].input);
+	}
+
+	for (i = 0; i < ad714x->hw->touchpad_num; i++) {
+		input_unregister_device(ad714x->sw->touchpad[i].input);
+		input_free_device(ad714x->sw->touchpad[i].input);
+	}
+
+	input_unregister_device(ad714x->sw->button[0].input);
+	input_free_device(ad714x->sw->button[0].input);
+
+	/* free all memories for software flow */
+	kfree(bt_drv); /* kfree(NULL) is safe check is not required */
+	kfree(sd_drv);
+	kfree(wl_drv);
+	kfree(tp_drv);
+	kfree(drv_data);
+	kfree(ad714x);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int ad714x_suspend(bus_device *bus, pm_message_t message)
+{
+	struct ad714x_chip *ad714x = dev_get_drvdata(&bus->dev);
+	unsigned short data;
+
+	pr_debug("%s enter\n", __func__);
+
+	mutex_lock(&ad714x->mutex);
+
+	data = ad714x->hw->sys_cfg_reg[AD714X_PWR_CTRL] | 0x3;
+	ad714x->write(bus, AD714X_PWR_CTRL, data);
+
+	mutex_unlock(&ad714x->mutex);
+
+	return 0;
+}
+
+static int ad714x_resume(bus_device *bus)
+{
+	struct ad714x_chip *ad714x = dev_get_drvdata(&bus->dev);
+	unsigned short data;
+
+	pr_debug("%s enter\n", __func__);
+
+	mutex_lock(&ad714x->mutex);
+
+	/* resume to non-shutdown mode */
+
+	ad714x->write(bus, AD714X_PWR_CTRL,
+			ad714x->hw->sys_cfg_reg[AD714X_PWR_CTRL]);
+
+	/* make sure the interrupt output line is not low level after resume,
+	 * otherwise we will get no chance to enter falling-edge irq again
+	 */
+
+	ad714x->read(bus, STG_LOW_INT_STA_REG, &data);
+	ad714x->read(bus, STG_HIGH_INT_STA_REG, &data);
+	ad714x->read(bus, STG_COM_INT_STA_REG, &data);
+
+	mutex_unlock(&ad714x->mutex);
+
+	return 0;
+}
+#else
+#define ad714x_suspend NULL
+#define ad714x_resume  NULL
+#endif
+
+#if defined(CONFIG_INPUT_AD714X_SPI)
+int ad714x_spi_read(struct spi_device *spi, unsigned short reg,
+		unsigned short *data)
+{
+	int ret;
+	unsigned short tx[2];
+	unsigned short rx[2];
+	struct spi_transfer t = {
+		.tx_buf = tx,
+		.rx_buf = rx,
+		.len = 4,
+	};
+	struct spi_message m;
+
+	tx[0] = (AD714x_SPI_ADDR << AD714x_SPI_ADDR_SHFT) |
+		(AD714x_SPI_READ << AD714x_SPI_READ_SHFT) | reg;
+
+	spi_message_init(&m);
+	spi_message_add_tail(&t, &m);
+	ret = spi_sync(spi, &m);
+
+	if (ret < 0) {
+		dev_err(&spi->dev, "SPI read error\n");
+		return ret;
+	}
+
+	*data = rx[1];
+	return ret;
+}
+
+int ad714x_spi_write(struct spi_device *spi, unsigned short reg,
+		unsigned short data)
+{
+	int ret = 0;
+	unsigned short tx[2];
+	struct spi_transfer t = {
+		.tx_buf = tx,
+		.len = 4,
+	};
+	struct spi_message m;
+
+	tx[0] = (AD714x_SPI_ADDR << AD714x_SPI_ADDR_SHFT) | reg;
+	tx[1] = data;
+
+	spi_message_init(&m);
+	spi_message_add_tail(&t, &m);
+
+	ret = spi_sync(spi, &m);
+	if (ret < 0)
+		dev_err(&spi->dev, "SPI write error\n");
+
+	return ret;
+}
+
+static int __devinit ad714x_spi_probe(struct spi_device *spi)
+{
+	int ret = 0;
+	struct ad714x_chip *chip;
+
+	if (spi->dev.platform_data == NULL) {
+		dev_err(&spi->dev, "platform data for ad714x doesn't exist\n");
+		return -ENODEV;
+	}
+
+	chip = kzalloc(sizeof(struct ad714x_chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->bus = spi;
+	chip->read = ad714x_spi_read;
+	chip->write = ad714x_spi_write;
+	chip->hw = spi->dev.platform_data;
+	spi_set_drvdata(spi, chip);
+
+	/* common probe not related with spi/i2c */
+	ret = ad714x_probe(chip);
+	if (ret)
+		kfree(chip);
+
+	return ret;
+}
+
+static int __devexit ad714x_spi_remove(struct spi_device *spi)
+{
+	struct ad714x_chip *chip = spi_get_drvdata(spi);
+	ad714x_remove(chip);
+
+	return 0;
+}
+
+static struct spi_driver ad714x_spi_driver = {
+	.driver = {
+		.name	= "ad714x_captouch",
+		.bus	= &spi_bus_type,
+		.owner	= THIS_MODULE,
+	},
+	.probe		= ad714x_spi_probe,
+	.remove		= __devexit_p(ad714x_spi_remove),
+	.suspend	= ad714x_suspend,
+	.resume		= ad714x_resume,
+};
+
+static int __init ad714x_init(void)
+{
+	int ret;
+
+	ret = spi_register_driver(&ad714x_spi_driver);
+	if (ret != 0) {
+		printk(KERN_ERR "Failed to register ad714x SPI driver: %d\n",
+				ret);
+	}
+
+	return ret;
+}
+
+static void __exit ad714x_exit(void)
+{
+	spi_unregister_driver(&ad714x_spi_driver);
+}
+#else
+static int ad714x_i2c_write(struct i2c_client *client, unsigned short reg,
+		unsigned short data)
+{
+	int ret = 0;
+	u8 tx[4];
+
+	/* Do raw I2C, not smbus compatible */
+	tx[0] = (reg & 0xFF00) >> 8;
+	tx[1] = (reg & 0x00FF);
+	tx[2] = (data & 0xFF00) >> 8;
+	tx[3] = data & 0x00FF;
+
+	ret = i2c_master_send(client, tx, 4);
+	if (ret < 0)
+		dev_err(&client->dev, "I2C write error\n");
+
+	return ret;
+}
+
+static int ad714x_i2c_read(struct i2c_client *client, unsigned short reg,
+		unsigned short *data)
+{
+	int ret = 0;
+	u8 tx[2];
+	u8 rx[2];
+
+	/* Do raw I2C, not smbus compatible */
+	tx[0] = (reg & 0xFF00) >> 8;
+	tx[1] = (reg & 0x00FF);
+
+	ret = i2c_master_send(client, tx, 2);
+	if (ret < 0) {
+		dev_err(&client->dev, "I2C read error\n");
+		return ret;
+	}
+
+	ret = i2c_master_recv(client, rx, 2);
+	if (ret < 0) {
+		dev_err(&client->dev, "I2C read error\n");
+		return ret;
+	}
+
+	*data = rx[0];
+	*data = (*data << 8) | rx[1];
+
+	return ret;
+}
+
+static int __devinit ad714x_i2c_probe(struct i2c_client *client,
+		const struct i2c_device_id *id)
+{
+	int ret = 0;
+	struct ad714x_chip *chip;
+
+	if (client->dev.platform_data == NULL) {
+		dev_err(&client->dev,
+			"platform data for ad714x doesn't exist\n");
+		return -ENODEV;
+	}
+
+	chip = kzalloc(sizeof(struct ad714x_chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->bus = client;
+	chip->read = ad714x_i2c_read;
+	chip->write = ad714x_i2c_write;
+	chip->hw = client->dev.platform_data;
+	i2c_set_clientdata(client, chip);
+
+	/* common probe not related with spi/i2c */
+	ret = ad714x_probe(chip);
+	if (ret)
+		kfree(chip);
+
+	return ret;
+}
+
+static int __devexit ad714x_i2c_remove(struct i2c_client *client)
+{
+	struct ad714x_chip *chip = i2c_get_clientdata(client);
+	ad714x_remove(chip);
+
+	return 0;
+}
+
+static const struct i2c_device_id ad714x_id[] = {
+	{ "ad714x_captouch", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ad714x_id);
+
+static struct i2c_driver ad714x_i2c_driver = {
+	.driver = {
+		.name = "ad714x_captouch",
+	},
+	.probe = ad714x_i2c_probe,
+	.remove = __devexit_p(ad714x_i2c_remove),
+	.suspend	= ad714x_suspend,
+	.resume		= ad714x_resume,
+	.id_table = ad714x_id,
+};
+
+static int __init ad714x_init(void)
+{
+	return i2c_add_driver(&ad714x_i2c_driver);
+}
+
+static void __exit ad714x_exit(void)
+{
+	i2c_del_driver(&ad714x_i2c_driver);
+}
+#endif
+
+module_init(ad714x_init);
+module_exit(ad714x_exit);
+
+MODULE_DESCRIPTION("ad714x captouch driver");
+MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>");
+MODULE_LICENSE("GPL");
-- 
1.5.6.3

Re: [PATCH 2/2] add ad714x input driver for button/scrollwhell/slider/touchpad

[Mike Frysinger]

On Mon, Aug 31, 2009 at 23:55, Barry Song wrote:
> ad7142/ad7147 are Programmable Controllers for Capacitance Touch Sensors.
> The chips don't restrict the specific usage, and it can be used as button/
> slider/scrollwheel/touchpad etc. depending on the hardware connection.
> One special target board can include one or several these components.
>
> The driver is independent of special boards. It gets the components
> layout information from the platform_data, registers related devices,
> fullfills the algorithms and state machines for these components and
> report related input events to up level.

seems you didnt address all the comments i posted after your first commit ...

please add this line before the #include's:
#define pr_fmt(fmt) "ad714x: " fmt
this will fix all of your dev_*() output so that it is properly prefixed

> +#define AD714x_SPI_ADDR        0x1C
> +#define AD714x_SPI_ADDR_SHFT   11
> +#define AD714x_SPI_READ        1
> +#define AD714x_SPI_READ_SHFT   10
> +
> +#define AD714X_PWR_CTRL           0x0
> +#define AD714x_STG_CAL_EN_REG     0x1
> +#define AD714X_AMB_COMP_CTRL0_REG 0x2
> +#define AD714x_PARTID_REG         0x17
> +#define AD7147_PARTID             0x1470
> +#define AD7142_PARTID             0xE620
> +#define AD714x_STAGECFG_REG       0x80
> +#define AD714x_SYSCFG_REG         0x0

your AD714X and AD714x needs to be consistent

> +struct ad714x_button_drv {
> +       ad714x_device_state state;
> +       /* Unlike slider/wheel/touchpad, all buttons point to
> +        * same input_dev instance */

either put the comment on one line and do:
/* fooo */
or use proper comment style where the last */ is on a line by itself
/*
 * foo
 */
this applies to many comments in this file

> +struct ad714x_chip {
> +       unsigned short h_state;
> +       unsigned short l_state;
> +       unsigned short c_state;
> +       unsigned short adc_reg[STAGE_NUM];
> +       unsigned short amb_reg[STAGE_NUM];
> +       unsigned short sensor_val[STAGE_NUM];
> +
> +       struct ad714x_platform_data *hw;
> +       struct ad714x_driver_data *sw;
> +
> +       bus_device *bus;
> +       int (*read) (bus_device *, unsigned short, unsigned short *);
> +       int (*write) (bus_device *, unsigned short, unsigned short);

using function pointers is pure overhead in your current
implementation.  create a ad714x_read macro that expands to either the
spi or the i2c version depending on which is enabled.

> +static void stage_use_com_int(struct ad714x_chip *ad714x, int start_stage,
> +               int end_stage)
> +{

all funcs in here really should have ad714x_ symbol prefixes.  if i
saw a crash and it just said "stage_xxx", it isnt immediately obvious
where this symbol is coming from.

> +static int stage_cal_highest_stage(struct ad714x_chip *ad714x, int start_stage,
> +               int end_stage)
> +{
> +       int max_res = 0;
> +       int max_idx = 0;
> +       int i;
> +
> +       for (i = start_stage; i <= end_stage; i++) {
> +               if (ad714x->sensor_val[i] > max_res) {
> +                       max_res = ad714x->sensor_val[i];
> +                       max_idx = i;
> +               }
> +       }

should the loop limit really be "<=" and not "<" ?  seems to be this
way throughout, so maybe the logic is correct ...

> +static int stage_cal_abs_pos(struct ad714x_chip *ad714x, int start_stage,
> +               int end_stage, int highest_stage, int max_coord)
> +{
> +       int a_param, b_param;
> +
> +       if (highest_stage == start_stage) {
> +               a_param = ad714x->sensor_val[start_stage + 1];
> +               b_param = ad714x->sensor_val[start_stage] +
> +                       ad714x->sensor_val[start_stage + 1];
> +       } else if (highest_stage == end_stage) {
> +               a_param = ad714x->sensor_val[end_stage] *
> +                       (end_stage - start_stage) +
> +                       ad714x->sensor_val[end_stage - 1] *
> +                       (end_stage - start_stage - 1);
> +               b_param = ad714x->sensor_val[end_stage] +
> +                       ad714x->sensor_val[end_stage - 1];
> +       } else {
> +               a_param = ad714x->sensor_val[highest_stage] *
> +                       (highest_stage - start_stage) +
> +                       ad714x->sensor_val[highest_stage - 1] *
> +                       (highest_stage - start_stage - 1) +
> +                       ad714x->sensor_val[highest_stage + 1] *
> +                       (highest_stage - start_stage + 1);
> +               b_param = ad714x->sensor_val[highest_stage] +
> +                       ad714x->sensor_val[highest_stage - 1] +
> +                       ad714x->sensor_val[highest_stage + 1];
> +       }
> +
> +       return (max_coord / (end_stage - start_stage)) * a_param / b_param;
> +}

do the local vars really need "_stage" suffix ?  if you trimmed that,
i imagine it'd make the code a bit easier to digest.

> +/* One button can connect to multi postive and negative of CDCs
> + * Multi-buttons can connect to same postive/negative of one CDC

please run a spell checker on your comments ...
postive -> positive

> +static int ad714x_hw_detect(struct ad714x_chip *ad714x)

should be __devinit

> +       default:
> +               dev_err(&ad714x->bus->dev, "Fail to detect AD714x captouch,\
> +                               read ID is %04x\n", data);

line continuation in strings produces awful output

> +static int ad714x_hw_init(struct ad714x_chip *ad714x)
> ...
> +       return 0;
> ...
> +       ad714x_hw_init(ad714x);

considering you always return 0 and you never check the return, this
should be a void function

and ad714x_hw_init should be __devinit

> +       drv_data = kzalloc(sizeof(struct ad714x_driver_data), GFP_KERNEL);

sizeof(*drv_data) ... applies to every alloc in this driver

> + input[alloc_idx-1]->id.bustype = BUS_I2C;

you set bustype to I2C in common code even though this will be
executed for both SPI and I2C interfaces

> +       if (ad714x->bus->irq > 0) {
> +               ret = request_threaded_irq(ad714x->bus->irq, ad714x_interrupt,
> +                               ad714x_interrupt_thread, IRQF_TRIGGER_FALLING,
> +                               "ad714x_captouch", ad714x);
> +               if (ret) {
> +                       dev_err(&ad714x->bus->dev, "Can't allocate irq %d\n",
> +                                       ad714x->bus->irq);
> +                       goto fail_irq;
> +               }
> +       } else
> +               dev_warn(&ad714x->bus->dev, "IRQ not configured!\n");
> +
> +       return 0;

since the driver requires an IRQ in order to work, this should error
out rather than warn and return success

also, wouldnt it make more sense to request the IRQ first and then
fail rather than doing all the memory/input allocation only to find
out the IRQ is already taken ?

> +int ad714x_spi_read(struct spi_device *spi, unsigned short reg,
> +               unsigned short *data)

you're missing "static" here and in a few other functions

> +{
> +       int ret;
> +       unsigned short tx[2];
> +       unsigned short rx[2];
> +       struct spi_transfer t = {
> +               .tx_buf = tx,
> +               .rx_buf = rx,
> +               .len = 4,
> +       };
> +       struct spi_message m;
> +
> +       tx[0] = (AD714x_SPI_ADDR << AD714x_SPI_ADDR_SHFT) |
> +               (AD714x_SPI_READ << AD714x_SPI_READ_SHFT) | reg;
> +
> +       spi_message_init(&m);
> +       spi_message_add_tail(&t, &m);
> +       ret = spi_sync(spi, &m);

cant you use spi_write_then_read() ?  dont let the u8* prototype scare
you, it should work with writing 16bits and then reading 16bits.

> +int ad714x_spi_write(struct spi_device *spi, unsigned short reg,
> +               unsigned short data)
> +{
> +       int ret = 0;
> +       unsigned short tx[2];
> +       struct spi_transfer t = {
> +               .tx_buf = tx,
> +               .len = 4,
> +       };
> +       struct spi_message m;
> +
> +       tx[0] = (AD714x_SPI_ADDR << AD714x_SPI_ADDR_SHFT) | reg;
> +       tx[1] = data;
> +
> +       spi_message_init(&m);
> +       spi_message_add_tail(&t, &m);
> +
> +       ret = spi_sync(spi, &m);
> +       if (ret < 0)
> +               dev_err(&spi->dev, "SPI write error\n");
> +
> +       return ret;
> +}

seems like spi_write() would be better

> +static int __devexit ad714x_i2c_remove(struct i2c_client *client)
> +{
> +       struct ad714x_chip *chip = i2c_get_clientdata(client);
> +       ad714x_remove(chip);
> +
> +       return 0;
> +}

return ad714x_remove(chip);

of course, if you simply created a macro "ad714x_get_chipdata" and had
it evaluate to either the spi or i2c version depending on the
interface enabled, this function could be in common code rather than
duplicating it for the two interfaces

same goes for the module init/exit functions

> +static struct spi_driver ad714x_spi_driver = {
> +       .driver = {
> +               .name   = "ad714x_captouch",
> +               .bus    = &spi_bus_type,
> +               .owner  = THIS_MODULE,
> +       },

i'm pretty sure you dont need to set .bus yourself

> +       .probe          = ad714x_spi_probe,
> +       .remove         = __devexit_p(ad714x_spi_remove),
> +       .suspend        = ad714x_suspend,
> +       .resume         = ad714x_resume,

there is a new power management opts struct in 2.6.31 that should be used

> +static int __init ad714x_init(void)
> +{
> +       int ret;
> +
> +       ret = spi_register_driver(&ad714x_spi_driver);
> +       if (ret != 0) {
> +               printk(KERN_ERR "Failed to register ad714x SPI driver: %d\n",
> +                               ret);
> +       }
> +
> +       return ret;

pr_err(), no need for the braces, and no need to output the ret value.
 this is the module init function which means that error will be
passed back up to userspace and it can handle displaying it.  in that
case, it might as well read:
{
    return spi_register_driver(&ad714x_spi_driver);
}

> +       u8 tx[4];
> +
> +       /* Do raw I2C, not smbus compatible */
> +       tx[0] = (reg & 0xFF00) >> 8;
> +       tx[1] = (reg & 0x00FF);
> +       tx[2] = (data & 0xFF00) >> 8;
> +       tx[3] = data & 0x00FF;

the masks are pretty useless (and i wouldnt be surprised if gcc doesnt
optimize all of them away) since you're already loading into a u8
tx[4] = {
    reg >> 8,
    reg,
    data >> 8,
    data,
};

> +       u8 tx[2];
> +
> +       /* Do raw I2C, not smbus compatible */
> +       tx[0] = (reg & 0xFF00) >> 8;
> +       tx[1] = (reg & 0x00FF);

same here

> +       *data = rx[0];
> +       *data = (*data << 8) | rx[1];

erm, this is funky.  why cant you do it with one assignment ?

> +MODULE_DESCRIPTION("ad714x captouch driver");

please use a description like you did in the Kconfig
-mike
_______________________________________________
Uclinux-dist-devel mailing list
Uclinux-dist-devel@blackfin.uclinux.org
https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel

Re: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input driver for button/scrollwhell/slider/touchpad

[David Brownell]

On Tuesday 01 September 2009, Mike Frysinger wrote:
> > +               ret = request_threaded_irq(ad714x->bus->irq, ad714x_interrupt,
> > +                               ad714x_interrupt_thread, IRQF_TRIGGER_FALLING,
> > +                               "ad714x_captouch", ad714x);

IRQF_SHARED is worth including; and TRIGGER_FALLING is not good
to hard-wire, maybe it gets inverted or hooked up to an IRQ
controller which only supports level triggers.  Best to have
the trigger mode come from the platform data.

Of course, if it's a level triggered IRQ then it clearly can't
be IRQF_SHARED since someone will need to mask it off at the IRQ
controller before exiting the hardirq handler, and unmask it
before exiting the threaded handler.


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day 
trial. Simplify your report design, integration and deployment - and focus on 
what you do best, core application coding. Discover what's new with 
Crystal Reports now.  http://p.sf.net/sfu/bobj-july

RE: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input driver forbutton/scrollwhell/slider/touchpad

[Song, Barry]

 

>-----Original Message-----
>From: uclinux-dist-devel-bounces@blackfin.uclinux.org 
>[mailto:uclinux-dist-devel-bounces@blackfin.uclinux.org] On 
>Behalf Of Mike Frysinger
>Sent: Wednesday, September 02, 2009 3:31 AM
>To: Barry Song
>Cc: dbrownell@users.sourceforge.net; dtor@mail.ru; 
>dmitry.torokhov@gmail.com; 
>spi-devel-general@lists.sourceforge.net; 
>linux-input@vger.kernel.org; uclinux-dist-devel@blackfin.uclinux.org
>Subject: Re: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input 
>driver forbutton/scrollwhell/slider/touchpad
>
>On Mon, Aug 31, 2009 at 23:55, Barry Song wrote:
>> ad7142/ad7147 are Programmable Controllers for Capacitance 
>Touch Sensors.
>> The chips don't restrict the specific usage, and it can be 
>used as button/
>> slider/scrollwheel/touchpad etc. depending on the hardware 
>connection.
>> One special target board can include one or several these components.
>>
>> The driver is independent of special boards. It gets the components
>> layout information from the platform_data, registers related devices,
>> fullfills the algorithms and state machines for these components and
>> report related input events to up level.
>
>seems you didnt address all the comments i posted after your 
>first commit ...
>
>please add this line before the #include's:
>#define pr_fmt(fmt) "ad714x: " fmt
>this will fix all of your dev_*() output so that it is 
>properly prefixed

Fixed.

>
>> +#define AD714x_SPI_ADDR        0x1C
>> +#define AD714x_SPI_ADDR_SHFT   11
>> +#define AD714x_SPI_READ        1
>> +#define AD714x_SPI_READ_SHFT   10
>> +
>> +#define AD714X_PWR_CTRL           0x0
>> +#define AD714x_STG_CAL_EN_REG     0x1
>> +#define AD714X_AMB_COMP_CTRL0_REG 0x2
>> +#define AD714x_PARTID_REG         0x17
>> +#define AD7147_PARTID             0x1470
>> +#define AD7142_PARTID             0xE620
>> +#define AD714x_STAGECFG_REG       0x80
>> +#define AD714x_SYSCFG_REG         0x0
>
>your AD714X and AD714x needs to be consistent
Fixed.
>
>> +struct ad714x_button_drv {
>> +       ad714x_device_state state;
>> +       /* Unlike slider/wheel/touchpad, all buttons point to
>> +        * same input_dev instance */
>
>either put the comment on one line and do:
>/* fooo */
>or use proper comment style where the last */ is on a line by itself
>/*
> * foo
> */
>this applies to many comments in this file

Fixed.

>
>> +struct ad714x_chip {
>> +       unsigned short h_state;
>> +       unsigned short l_state;
>> +       unsigned short c_state;
>> +       unsigned short adc_reg[STAGE_NUM];
>> +       unsigned short amb_reg[STAGE_NUM];
>> +       unsigned short sensor_val[STAGE_NUM];
>> +
>> +       struct ad714x_platform_data *hw;
>> +       struct ad714x_driver_data *sw;
>> +
>> +       bus_device *bus;
>> +       int (*read) (bus_device *, unsigned short, unsigned short *);
>> +       int (*write) (bus_device *, unsigned short, unsigned short);
>
>using function pointers is pure overhead in your current
>implementation.  create a ad714x_read macro that expands to either the
>spi or the i2c version depending on which is enabled.
Using macro can be a choice. But I don't think it can save much overhead here. 
Function pointers encapsulates the object better.
>
>> +static void stage_use_com_int(struct ad714x_chip *ad714x, 
>int start_stage,
>> +               int end_stage)
>> +{
>
>all funcs in here really should have ad714x_ symbol prefixes.  if i
>saw a crash and it just said "stage_xxx", it isnt immediately obvious
>where this symbol is coming from.

Fixed.

>
>> +static int stage_cal_highest_stage(struct ad714x_chip 
>*ad714x, int start_stage,
>> +               int end_stage)
>> +{
>> +       int max_res = 0;
>> +       int max_idx = 0;
>> +       int i;
>> +
>> +       for (i = start_stage; i <= end_stage; i++) {
>> +               if (ad714x->sensor_val[i] > max_res) {
>> +                       max_res = ad714x->sensor_val[i];
>> +                       max_idx = i;
>> +               }
>> +       }
>
>should the loop limit really be "<=" and not "<" ?  seems to be this
>way throughout, so maybe the logic is correct ...

It's <=.

>
>> +static int stage_cal_abs_pos(struct ad714x_chip *ad714x, 
>int start_stage,
>> +               int end_stage, int highest_stage, int max_coord)
>> +{
>> +       int a_param, b_param;
>> +
>> +       if (highest_stage == start_stage) {
>> +               a_param = ad714x->sensor_val[start_stage + 1];
>> +               b_param = ad714x->sensor_val[start_stage] +
>> +                       ad714x->sensor_val[start_stage + 1];
>> +       } else if (highest_stage == end_stage) {
>> +               a_param = ad714x->sensor_val[end_stage] *
>> +                       (end_stage - start_stage) +
>> +                       ad714x->sensor_val[end_stage - 1] *
>> +                       (end_stage - start_stage - 1);
>> +               b_param = ad714x->sensor_val[end_stage] +
>> +                       ad714x->sensor_val[end_stage - 1];
>> +       } else {
>> +               a_param = ad714x->sensor_val[highest_stage] *
>> +                       (highest_stage - start_stage) +
>> +                       ad714x->sensor_val[highest_stage - 1] *
>> +                       (highest_stage - start_stage - 1) +
>> +                       ad714x->sensor_val[highest_stage + 1] *
>> +                       (highest_stage - start_stage + 1);
>> +               b_param = ad714x->sensor_val[highest_stage] +
>> +                       ad714x->sensor_val[highest_stage - 1] +
>> +                       ad714x->sensor_val[highest_stage + 1];
>> +       }
>> +
>> +       return (max_coord / (end_stage - start_stage)) * 
>a_param / b_param;
>> +}
>
>do the local vars really need "_stage" suffix ?  if you trimmed that,
>i imagine it'd make the code a bit easier to digest.
Yes. It need the _stage suffix. Otherwise, nobody know what's started, what's ended.
>
>> +/* One button can connect to multi postive and negative of CDCs
>> + * Multi-buttons can connect to same postive/negative of one CDC
>
>please run a spell checker on your comments ...
>postive -> positive
>

Fixed

>> +static int ad714x_hw_detect(struct ad714x_chip *ad714x)
>
>should be __devinit
>
>> +       default:
>> +               dev_err(&ad714x->bus->dev, "Fail to detect 
>AD714x captouch,\
>> +                               read ID is %04x\n", data);
>
>line continuation in strings produces awful output
>

Fixed

>> +static int ad714x_hw_init(struct ad714x_chip *ad714x)
>> ...
>> +       return 0;
>> ...
>> +       ad714x_hw_init(ad714x);
>
>considering you always return 0 and you never check the return, this
>should be a void function
>
>and ad714x_hw_init should be __devinit

Fixed.

>
>> +       drv_data = kzalloc(sizeof(struct 
>ad714x_driver_data), GFP_KERNEL);
>
>sizeof(*drv_data) ... applies to every alloc in this driver
>
>> + input[alloc_idx-1]->id.bustype = BUS_I2C;
>
>you set bustype to I2C in common code even though this will be
>executed for both SPI and I2C interfaces

There is a BUS_I2C, but no BUS_SPI. So people may use other serial type to imply. 
Do you think we should add a macro in input.h? I am really suprised why the macro hasn't been added until now. 

>
>> +       if (ad714x->bus->irq > 0) {
>> +               ret = request_threaded_irq(ad714x->bus->irq, 
>ad714x_interrupt,
>> +                               ad714x_interrupt_thread, 
>IRQF_TRIGGER_FALLING,
>> +                               "ad714x_captouch", ad714x);
>> +               if (ret) {
>> +                       dev_err(&ad714x->bus->dev, "Can't 
>allocate irq %d\n",
>> +                                       ad714x->bus->irq);
>> +                       goto fail_irq;
>> +               }
>> +       } else
>> +               dev_warn(&ad714x->bus->dev, "IRQ not configured!\n");
>> +
>> +       return 0;
>
>since the driver requires an IRQ in order to work, this should error
>out rather than warn and return success
>
>also, wouldnt it make more sense to request the IRQ first and then
>fail rather than doing all the memory/input allocation only to find
>out the IRQ is already taken ?
>

Ok

>> +int ad714x_spi_read(struct spi_device *spi, unsigned short reg,
>> +               unsigned short *data)
>
>you're missing "static" here and in a few other functions

Fixed

>
>> +{
>> +       int ret;
>> +       unsigned short tx[2];
>> +       unsigned short rx[2];
>> +       struct spi_transfer t = {
>> +               .tx_buf = tx,
>> +               .rx_buf = rx,
>> +               .len = 4,
>> +       };
>> +       struct spi_message m;
>> +
>> +       tx[0] = (AD714x_SPI_ADDR << AD714x_SPI_ADDR_SHFT) |
>> +               (AD714x_SPI_READ << AD714x_SPI_READ_SHFT) | reg;
>> +
>> +       spi_message_init(&m);
>> +       spi_message_add_tail(&t, &m);
>> +       ret = spi_sync(spi, &m);
>
>cant you use spi_write_then_read() ?  dont let the u8* prototype scare
>you, it should work with writing 16bits and then reading 16bits.

I have never been scared by any u8* or something else. I only prefer to use raw spi API, which can show the bottom level timing and SPI bus feature better. 
In fact, I prefer to use raw i2c API too. 

>
>> +int ad714x_spi_write(struct spi_device *spi, unsigned short reg,
>> +               unsigned short data)
>> +{
>> +       int ret = 0;
>> +       unsigned short tx[2];
>> +       struct spi_transfer t = {
>> +               .tx_buf = tx,
>> +               .len = 4,
>> +       };
>> +       struct spi_message m;
>> +
>> +       tx[0] = (AD714x_SPI_ADDR << AD714x_SPI_ADDR_SHFT) | reg;
>> +       tx[1] = data;
>> +
>> +       spi_message_init(&m);
>> +       spi_message_add_tail(&t, &m);
>> +
>> +       ret = spi_sync(spi, &m);
>> +       if (ret < 0)
>> +               dev_err(&spi->dev, "SPI write error\n");
>> +
>> +       return ret;
>> +}
>
>seems like spi_write() would be better
>
>> +static int __devexit ad714x_i2c_remove(struct i2c_client *client)
>> +{
>> +       struct ad714x_chip *chip = i2c_get_clientdata(client);
>> +       ad714x_remove(chip);
>> +
>> +       return 0;
>> +}
>
>return ad714x_remove(chip);

Fixed.

>
>of course, if you simply created a macro "ad714x_get_chipdata" and had
>it evaluate to either the spi or i2c version depending on the
>interface enabled, this function could be in common code rather than
>duplicating it for the two interfaces
>

In fact, dev_set_drvdata and dev_get_drvdata can be used in spi/i2c probe and remove. Then the probe/remove can be unified.
But at the beginning, I prefer to keep this little redundancy, which maybe make the code more readable. 
Maybe I will use a unified probe/remove for SPI/I2C too, Let me think...

>same goes for the module init/exit functions
>
>> +static struct spi_driver ad714x_spi_driver = {
>> +       .driver = {
>> +               .name   = "ad714x_captouch",
>> +               .bus    = &spi_bus_type,
>> +               .owner  = THIS_MODULE,
>> +       },
>
>i'm pretty sure you dont need to set .bus yourself

Fixed.

>
>> +       .probe          = ad714x_spi_probe,
>> +       .remove         = __devexit_p(ad714x_spi_remove),
>> +       .suspend        = ad714x_suspend,
>> +       .resume         = ad714x_resume,
>
>there is a new power management opts struct in 2.6.31 that 
>should be used

Ok

>
>> +static int __init ad714x_init(void)
>> +{
>> +       int ret;
>> +
>> +       ret = spi_register_driver(&ad714x_spi_driver);
>> +       if (ret != 0) {
>> +               printk(KERN_ERR "Failed to register ad714x 
>SPI driver: %d\n",
>> +                               ret);
>> +       }
>> +
>> +       return ret;
>

Ok

>pr_err(), no need for the braces, and no need to output the ret value.
> this is the module init function which means that error will be
>passed back up to userspace and it can handle displaying it.  in that
>case, it might as well read:
>{
>    return spi_register_driver(&ad714x_spi_driver);
>}

Fixed

>
>> +       u8 tx[4];
>> +
>> +       /* Do raw I2C, not smbus compatible */
>> +       tx[0] = (reg & 0xFF00) >> 8;
>> +       tx[1] = (reg & 0x00FF);
>> +       tx[2] = (data & 0xFF00) >> 8;
>> +       tx[3] = data & 0x00FF;
>
>the masks are pretty useless (and i wouldnt be surprised if gcc doesnt
>optimize all of them away) since you're already loading into a u8
>tx[4] = {
>    reg >> 8,
>    reg,
>    data >> 8,
>    data,
>};
>
>> +       u8 tx[2];
>> +
>> +       /* Do raw I2C, not smbus compatible */
>> +       tx[0] = (reg & 0xFF00) >> 8;
>> +       tx[1] = (reg & 0x00FF);
>
>same here
>
>> +       *data = rx[0];
>> +       *data = (*data << 8) | rx[1];
>
>erm, this is funky.  why cant you do it with one assignment ?
>
>> +MODULE_DESCRIPTION("ad714x captouch driver");
>
>please use a description like you did in the Kconfig

Fixed

>-mike
>_______________________________________________
>Uclinux-dist-devel mailing list
>Uclinux-dist-devel@blackfin.uclinux.org
>https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input driver forbutton/scrollwhell/slider/touchpad

[Mike Frysinger]

On Tue, Sep 1, 2009 at 22:46, Song, Barry wrote:
>>From: Mike Frysinger
>>> +struct ad714x_chip {
>>> +       unsigned short h_state;
>>> +       unsigned short l_state;
>>> +       unsigned short c_state;
>>> +       unsigned short adc_reg[STAGE_NUM];
>>> +       unsigned short amb_reg[STAGE_NUM];
>>> +       unsigned short sensor_val[STAGE_NUM];
>>> +
>>> +       struct ad714x_platform_data *hw;
>>> +       struct ad714x_driver_data *sw;
>>> +
>>> +       bus_device *bus;
>>> +       int (*read) (bus_device *, unsigned short, unsigned short *);
>>> +       int (*write) (bus_device *, unsigned short, unsigned short);
>>
>>using function pointers is pure overhead in your current
>>implementation.  create a ad714x_read macro that expands to either the
>>spi or the i2c version depending on which is enabled.
>
> Using macro can be a choice. But I don't think it can save much overhead here.
> Function pointers encapsulates the object better.

did you look at the generated code ?  function pointers requires
pointer loads and indirect calls.  telling gcc that the function is
completely static allows for direct calls (and thus less register
pressure) and if gcc gets clever enough, avoid call overhead.

>>> +static int stage_cal_abs_pos(struct ad714x_chip *ad714x,
>>>>int start_stage, int end_stage, int highest_stage, int max_coord)
>>> +{
>>> +       int a_param, b_param;
>>> +
>>> +       if (highest_stage == start_stage) {
>>> +               a_param = ad714x->sensor_val[start_stage + 1];
>>> +               b_param = ad714x->sensor_val[start_stage] +
>>> +                       ad714x->sensor_val[start_stage + 1];
>>> +       } else if (highest_stage == end_stage) {
>>> +               a_param = ad714x->sensor_val[end_stage] *
>>> +                       (end_stage - start_stage) +
>>> +                       ad714x->sensor_val[end_stage - 1] *
>>> +                       (end_stage - start_stage - 1);
>>> +               b_param = ad714x->sensor_val[end_stage] +
>>> +                       ad714x->sensor_val[end_stage - 1];
>>> +       } else {
>>> +               a_param = ad714x->sensor_val[highest_stage] *
>>> +                       (highest_stage - start_stage) +
>>> +                       ad714x->sensor_val[highest_stage - 1] *
>>> +                       (highest_stage - start_stage - 1) +
>>> +                       ad714x->sensor_val[highest_stage + 1] *
>>> +                       (highest_stage - start_stage + 1);
>>> +               b_param = ad714x->sensor_val[highest_stage] +
>>> +                       ad714x->sensor_val[highest_stage - 1] +
>>> +                       ad714x->sensor_val[highest_stage + 1];
>>> +       }
>>> +
>>> +       return (max_coord / (end_stage - start_stage)) *
>>a_param / b_param;
>>> +}
>>
>>do the local vars really need "_stage" suffix ?  if you trimmed that,
>>i imagine it'd make the code a bit easier to digest.
>
> Yes. It need the _stage suffix. Otherwise, nobody know what's started, what's ended.

that doesnt make sense.  if it's labeled "start" and "end" in the
local function, it's pretty obvious.

>>> + input[alloc_idx-1]->id.bustype = BUS_I2C;
>>
>>you set bustype to I2C in common code even though this will be
>>executed for both SPI and I2C interfaces
>
> There is a BUS_I2C, but no BUS_SPI. So people may use other serial type to imply.
> Do you think we should add a macro in input.h? I am really suprised why the macro hasn't been added until now.

yes, a new BUS_SPI should be added

>>> +{
>>> +       int ret;
>>> +       unsigned short tx[2];
>>> +       unsigned short rx[2];
>>> +       struct spi_transfer t = {
>>> +               .tx_buf = tx,
>>> +               .rx_buf = rx,
>>> +               .len = 4,
>>> +       };
>>> +       struct spi_message m;
>>> +
>>> +       tx[0] = (AD714x_SPI_ADDR << AD714x_SPI_ADDR_SHFT) |
>>> +               (AD714x_SPI_READ << AD714x_SPI_READ_SHFT) | reg;
>>> +
>>> +       spi_message_init(&m);
>>> +       spi_message_add_tail(&t, &m);
>>> +       ret = spi_sync(spi, &m);
>>
>>cant you use spi_write_then_read() ?  dont let the u8* prototype scare
>>you, it should work with writing 16bits and then reading 16bits.
>
> I have never been scared by any u8* or something else. I only prefer to use raw spi API, which can show the bottom level timing and SPI bus feature better.
> In fact, I prefer to use raw i2c API too.

that doesnt make sense.  the entire purpose of writing these helper
write/read functions is because people often do just that -- write a
few then read a few.  your concerns are purely superficial.
-mike
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input driver forbutton/scrollwhell/slider/touchpad

[Song, Barry]

No. I can't agree other coding style issues you said. I will keep my original codes for these issues except adding a BUS_SPI. 

>-----Original Message-----
>From: Mike Frysinger [mailto:vapier.adi@gmail.com] 
>Sent: Wednesday, September 02, 2009 11:09 AM
>To: Song, Barry
>Cc: Barry Song; dbrownell@users.sourceforge.net; dtor@mail.ru; 
>dmitry.torokhov@gmail.com; 
>spi-devel-general@lists.sourceforge.net; 
>linux-input@vger.kernel.org; uclinux-dist-devel@blackfin.uclinux.org
>Subject: Re: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input 
>driver forbutton/scrollwhell/slider/touchpad
>
>On Tue, Sep 1, 2009 at 22:46, Song, Barry wrote:
>>>From: Mike Frysinger
>>>> +struct ad714x_chip {
>>>> +       unsigned short h_state;
>>>> +       unsigned short l_state;
>>>> +       unsigned short c_state;
>>>> +       unsigned short adc_reg[STAGE_NUM];
>>>> +       unsigned short amb_reg[STAGE_NUM];
>>>> +       unsigned short sensor_val[STAGE_NUM];
>>>> +
>>>> +       struct ad714x_platform_data *hw;
>>>> +       struct ad714x_driver_data *sw;
>>>> +
>>>> +       bus_device *bus;
>>>> +       int (*read) (bus_device *, unsigned short, 
>unsigned short *);
>>>> +       int (*write) (bus_device *, unsigned short, 
>unsigned short);
>>>
>>>using function pointers is pure overhead in your current
>>>implementation.  create a ad714x_read macro that expands to 
>either the
>>>spi or the i2c version depending on which is enabled.
>>
>> Using macro can be a choice. But I don't think it can save 
>much overhead here.
>> Function pointers encapsulates the object better.
>
>did you look at the generated code ?  function pointers requires
>pointer loads and indirect calls.  telling gcc that the function is
>completely static allows for direct calls (and thus less register
>pressure) and if gcc gets clever enough, avoid call overhead.
>
>>>> +static int stage_cal_abs_pos(struct ad714x_chip *ad714x,
>>>>>int start_stage, int end_stage, int highest_stage, int max_coord)
>>>> +{
>>>> +       int a_param, b_param;
>>>> +
>>>> +       if (highest_stage == start_stage) {
>>>> +               a_param = ad714x->sensor_val[start_stage + 1];
>>>> +               b_param = ad714x->sensor_val[start_stage] +
>>>> +                       ad714x->sensor_val[start_stage + 1];
>>>> +       } else if (highest_stage == end_stage) {
>>>> +               a_param = ad714x->sensor_val[end_stage] *
>>>> +                       (end_stage - start_stage) +
>>>> +                       ad714x->sensor_val[end_stage - 1] *
>>>> +                       (end_stage - start_stage - 1);
>>>> +               b_param = ad714x->sensor_val[end_stage] +
>>>> +                       ad714x->sensor_val[end_stage - 1];
>>>> +       } else {
>>>> +               a_param = ad714x->sensor_val[highest_stage] *
>>>> +                       (highest_stage - start_stage) +
>>>> +                       ad714x->sensor_val[highest_stage - 1] *
>>>> +                       (highest_stage - start_stage - 1) +
>>>> +                       ad714x->sensor_val[highest_stage + 1] *
>>>> +                       (highest_stage - start_stage + 1);
>>>> +               b_param = ad714x->sensor_val[highest_stage] +
>>>> +                       ad714x->sensor_val[highest_stage - 1] +
>>>> +                       ad714x->sensor_val[highest_stage + 1];
>>>> +       }
>>>> +
>>>> +       return (max_coord / (end_stage - start_stage)) *
>>>a_param / b_param;
>>>> +}
>>>
>>>do the local vars really need "_stage" suffix ?  if you trimmed that,
>>>i imagine it'd make the code a bit easier to digest.
>>
>> Yes. It need the _stage suffix. Otherwise, nobody know 
>what's started, what's ended.
>
>that doesnt make sense.  if it's labeled "start" and "end" in the
>local function, it's pretty obvious.
>
>>>> + input[alloc_idx-1]->id.bustype = BUS_I2C;
>>>
>>>you set bustype to I2C in common code even though this will be
>>>executed for both SPI and I2C interfaces
>>
>> There is a BUS_I2C, but no BUS_SPI. So people may use other 
>serial type to imply.
>> Do you think we should add a macro in input.h? I am really 
>suprised why the macro hasn't been added until now.
>
>yes, a new BUS_SPI should be added
>
>>>> +{
>>>> +       int ret;
>>>> +       unsigned short tx[2];
>>>> +       unsigned short rx[2];
>>>> +       struct spi_transfer t = {
>>>> +               .tx_buf = tx,
>>>> +               .rx_buf = rx,
>>>> +               .len = 4,
>>>> +       };
>>>> +       struct spi_message m;
>>>> +
>>>> +       tx[0] = (AD714x_SPI_ADDR << AD714x_SPI_ADDR_SHFT) |
>>>> +               (AD714x_SPI_READ << AD714x_SPI_READ_SHFT) | reg;
>>>> +
>>>> +       spi_message_init(&m);
>>>> +       spi_message_add_tail(&t, &m);
>>>> +       ret = spi_sync(spi, &m);
>>>
>>>cant you use spi_write_then_read() ?  dont let the u8* 
>prototype scare
>>>you, it should work with writing 16bits and then reading 16bits.
>>
>> I have never been scared by any u8* or something else. I 
>only prefer to use raw spi API, which can show the bottom 
>level timing and SPI bus feature better.
>> In fact, I prefer to use raw i2c API too.
>
>that doesnt make sense.  the entire purpose of writing these helper
>write/read functions is because people often do just that -- write a
>few then read a few.  your concerns are purely superficial.
>-mike
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input driverforbutton/scrollwhell/slider/touchpad

[Robin Getz]

On Tue 1 Sep 2009 23:17, Song, Barry pondered:
> No. I can't agree other coding style issues you said. I will keep my
> original codes for these issues except adding a BUS_SPI.  

style is different than performance....

Did you think about Mike's comment - I think it was a performance issue he was 
talking about.

On Tue 1 Sep 2009 23:09, Mike Frysinger pondered:
> 
> did you look at the generated code?  function pointers requires
> pointer loads and indirect calls.  telling gcc that the function is
> completely static allows for direct calls (and thus less register
> pressure) and if gcc gets clever enough, avoid call overhead.



> >-----Original Message-----
> >From: Mike Frysinger [mailto:vapier.adi@gmail.com] 
> >Sent: Wednesday, September 02, 2009 11:09 AM
> >To: Song, Barry
> >Cc: Barry Song; dbrownell@users.sourceforge.net; dtor@mail.ru; 
> >dmitry.torokhov@gmail.com; 
> >spi-devel-general@lists.sourceforge.net; 
> >linux-input@vger.kernel.org; uclinux-dist-devel@blackfin.uclinux.org
> >Subject: Re: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input 
> >driver forbutton/scrollwhell/slider/touchpad
> >
> >On Tue, Sep 1, 2009 at 22:46, Song, Barry wrote:
> >>>From: Mike Frysinger
> >>>> +struct ad714x_chip {
> >>>> +       unsigned short h_state;
> >>>> +       unsigned short l_state;
> >>>> +       unsigned short c_state;
> >>>> +       unsigned short adc_reg[STAGE_NUM];
> >>>> +       unsigned short amb_reg[STAGE_NUM];
> >>>> +       unsigned short sensor_val[STAGE_NUM];
> >>>> +
> >>>> +       struct ad714x_platform_data *hw;
> >>>> +       struct ad714x_driver_data *sw;
> >>>> +
> >>>> +       bus_device *bus;
> >>>> +       int (*read) (bus_device *, unsigned short, 
> >unsigned short *);
> >>>> +       int (*write) (bus_device *, unsigned short, 
> >unsigned short);
> >>>
> >>>using function pointers is pure overhead in your current
> >>>implementation.  create a ad714x_read macro that expands to 
> >either the
> >>>spi or the i2c version depending on which is enabled.
> >>
> >> Using macro can be a choice. But I don't think it can save 
> >much overhead here.
> >> Function pointers encapsulates the object better.
> >
> >did you look at the generated code ?  function pointers requires
> >pointer loads and indirect calls.  telling gcc that the function is
> >completely static allows for direct calls (and thus less register
> >pressure) and if gcc gets clever enough, avoid call overhead.
> >
> >>>> +static int stage_cal_abs_pos(struct ad714x_chip *ad714x,
> >>>>>int start_stage, int end_stage, int highest_stage, int max_coord)
> >>>> +{
> >>>> +       int a_param, b_param;
> >>>> +
> >>>> +       if (highest_stage == start_stage) {
> >>>> +               a_param = ad714x->sensor_val[start_stage + 1];
> >>>> +               b_param = ad714x->sensor_val[start_stage] +
> >>>> +                       ad714x->sensor_val[start_stage + 1];
> >>>> +       } else if (highest_stage == end_stage) {
> >>>> +               a_param = ad714x->sensor_val[end_stage] *
> >>>> +                       (end_stage - start_stage) +
> >>>> +                       ad714x->sensor_val[end_stage - 1] *
> >>>> +                       (end_stage - start_stage - 1);
> >>>> +               b_param = ad714x->sensor_val[end_stage] +
> >>>> +                       ad714x->sensor_val[end_stage - 1];
> >>>> +       } else {
> >>>> +               a_param = ad714x->sensor_val[highest_stage] *
> >>>> +                       (highest_stage - start_stage) +
> >>>> +                       ad714x->sensor_val[highest_stage - 1] *
> >>>> +                       (highest_stage - start_stage - 1) +
> >>>> +                       ad714x->sensor_val[highest_stage + 1] *
> >>>> +                       (highest_stage - start_stage + 1);
> >>>> +               b_param = ad714x->sensor_val[highest_stage] +
> >>>> +                       ad714x->sensor_val[highest_stage - 1] +
> >>>> +                       ad714x->sensor_val[highest_stage + 1];
> >>>> +       }
> >>>> +
> >>>> +       return (max_coord / (end_stage - start_stage)) *
> >>>a_param / b_param;
> >>>> +}
> >>>
> >>>do the local vars really need "_stage" suffix ?  if you trimmed that,
> >>>i imagine it'd make the code a bit easier to digest.
> >>
> >> Yes. It need the _stage suffix. Otherwise, nobody know 
> >what's started, what's ended.
> >
> >that doesnt make sense.  if it's labeled "start" and "end" in the
> >local function, it's pretty obvious.
> >
> >>>> + input[alloc_idx-1]->id.bustype = BUS_I2C;
> >>>
> >>>you set bustype to I2C in common code even though this will be
> >>>executed for both SPI and I2C interfaces
> >>
> >> There is a BUS_I2C, but no BUS_SPI. So people may use other 
> >serial type to imply.
> >> Do you think we should add a macro in input.h? I am really 
> >suprised why the macro hasn't been added until now.
> >
> >yes, a new BUS_SPI should be added
> >
> >>>> +{
> >>>> +       int ret;
> >>>> +       unsigned short tx[2];
> >>>> +       unsigned short rx[2];
> >>>> +       struct spi_transfer t = {
> >>>> +               .tx_buf = tx,
> >>>> +               .rx_buf = rx,
> >>>> +               .len = 4,
> >>>> +       };
> >>>> +       struct spi_message m;
> >>>> +
> >>>> +       tx[0] = (AD714x_SPI_ADDR << AD714x_SPI_ADDR_SHFT) |
> >>>> +               (AD714x_SPI_READ << AD714x_SPI_READ_SHFT) | reg;
> >>>> +
> >>>> +       spi_message_init(&m);
> >>>> +       spi_message_add_tail(&t, &m);
> >>>> +       ret = spi_sync(spi, &m);
> >>>
> >>>cant you use spi_write_then_read() ?  dont let the u8* 
> >prototype scare
> >>>you, it should work with writing 16bits and then reading 16bits.
> >>
> >> I have never been scared by any u8* or something else. I 
> >only prefer to use raw spi API, which can show the bottom 
> >level timing and SPI bus feature better.
> >> In fact, I prefer to use raw i2c API too.
> >
> >that doesnt make sense.  the entire purpose of writing these helper
> >write/read functions is because people often do just that -- write a
> >few then read a few.  your concerns are purely superficial.
> >-mike
> >
> _______________________________________________
> Uclinux-dist-devel mailing list
> Uclinux-dist-devel@blackfin.uclinux.org
> https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input driverforbutton/scrollwhell/slider/touchpad

[Mike Frysinger]

On Wed, Sep 2, 2009 at 05:50, Robin Getz wrote:
> On Tue 1 Sep 2009 23:17, Song, Barry pondered:
>> No. I can't agree other coding style issues you said. I will keep my
>> original codes for these issues except adding a BUS_SPI.
>
> style is different than performance....
>
> Did you think about Mike's comment - I think it was a performance issue he was
> talking about.

if we do go the proper approach and handle spi/i2c dynamically as
suggested, then this is a moot point as we'll want to keep the
function pointers
-mike

Re: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input driver forbutton/scrollwhell/slider/touchpad

[David Brownell]

On Tuesday 01 September 2009, Song, Barry wrote:
> >> +       spi_message_add_tail(&t, &m);
> >> +       ret = spi_sync(spi, &m);
> >
> >cant you use spi_write_then_read() ?  dont let the u8* prototype scare
> >you, it should work with writing 16bits and then reading 16bits.
> 
> I have never been scared by any u8* or something else. I only prefer
> to use raw spi API, which can show the bottom level timing and SPI 
> bus feature better.   

In this case spi_write_then_read() is required, since
your tx and rx buffers are on the stack and hence not
DMA-safe.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day 
trial. Simplify your report design, integration and deployment - and focus on 
what you do best, core application coding. Discover what's new with 
Crystal Reports now.  http://p.sf.net/sfu/bobj-july

Re: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input driver forbutton/scrollwhell/slider/touchpad

[Barry Song]

David,
I am really surprised the kmalloc memory can keep DMA-safe. Check the
code, no matter spi_write_then_read and spi_init, they only get memory
by a simple
kmalloc(SPI_BUFSIZ, GFP_KERNEL);
I can't find map_single or dma coherent operations at all. So did I
lose something?
Thanks
Barry

On 9/2/09, David Brownell <david-b@pacbell.net> wrote:
> On Tuesday 01 September 2009, Song, Barry wrote:
>> >> +       spi_message_add_tail(&t, &m);
>> >> +       ret = spi_sync(spi, &m);
>> >
>> >cant you use spi_write_then_read() ?  dont let the u8* prototype scare
>> >you, it should work with writing 16bits and then reading 16bits.
>>
>> I have never been scared by any u8* or something else. I only prefer
>> to use raw spi API, which can show the bottom level timing and SPI
>> bus feature better.
>
> In this case spi_write_then_read() is required, since
> your tx and rx buffers are on the stack and hence not
> DMA-safe.
>

RE: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input driver forbutton/scrollwhell/slider/touchpad

[Song, Barry]

 

>-----Original Message-----
>From: Barry Song [mailto:21cnbao@gmail.com] 
>Sent: Wednesday, September 02, 2009 3:52 PM
>To: David Brownell
>Cc: Song, Barry; Mike Frysinger; 
>dbrownell@users.sourceforge.net; dtor@mail.ru; 
>dmitry.torokhov@gmail.com; 
>spi-devel-general@lists.sourceforge.net; 
>linux-input@vger.kernel.org; uclinux-dist-devel@blackfin.uclinux.org
>Subject: Re: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input 
>driver forbutton/scrollwhell/slider/touchpad
>
>David,
>I am really surprised the kmalloc memory can keep DMA-safe. Check the
>code, no matter spi_write_then_read and spi_init, they only get memory
>by a simple
>kmalloc(SPI_BUFSIZ, GFP_KERNEL);
>I can't find map_single or dma coherent operations at all. So did I
>lose something?
>Thanks
>Barry

Get it. SPI driver need to make dma_map_single or other dma coherent
operations by itself. The kmalloc memory you call only makes sure the
DMA-safe but not DMA-consistent.

>
>On 9/2/09, David Brownell <david-b@pacbell.net> wrote:
>> On Tuesday 01 September 2009, Song, Barry wrote:
>>> >> +       spi_message_add_tail(&t, &m);
>>> >> +       ret = spi_sync(spi, &m);
>>> >
>>> >cant you use spi_write_then_read() ?  dont let the u8* 
>prototype scare
>>> >you, it should work with writing 16bits and then reading 16bits.
>>>
>>> I have never been scared by any u8* or something else. I only prefer
>>> to use raw spi API, which can show the bottom level timing and SPI
>>> bus feature better.
>>
>> In this case spi_write_then_read() is required, since
>> your tx and rx buffers are on the stack and hence not
>> DMA-safe.
>>
>

Re: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input driver forbutton/scrollwhell/slider/touchpad

[David Brownell]

On Wednesday 02 September 2009, Barry Song wrote:
> David,
> I am really surprised the kmalloc memory can keep DMA-safe.

Then you need to internalize more of the docs on the DMA
calls ... :)

Notably see in Documentation/DMA-mapping.txt the section up top
where it describes what memory can be used for DMA, and very
explicitly states stack data is not appropriate.  


> Check the 
> code, no matter spi_write_then_read and spi_init, they only get memory
> by a simple
> kmalloc(SPI_BUFSIZ, GFP_KERNEL);
> I can't find map_single or dma coherent operations at all. So did I
> lose something?

When the SPI master controller driver uses DMA and the caller
doesn't set up mappings, that controller driver does the DMA
mapping operations.  That's what those code paths rely on.

You're writing code sitting a layer above that driver for the
SPI master controller.  Your code won't know whether DMA will
be used or not, so your code must either (a) use DMA-safe buffers
in the first place, or else (b) use spi_write_then_read() which
provides a small bounce buffer.  A non-option is (c) write your
code so it breaks on controllers that use DMA.

- Dave


> Thanks
> Barry
> 
> On 9/2/09, David Brownell <david-b@pacbell.net> wrote:
> > On Tuesday 01 September 2009, Song, Barry wrote:
> >> >> +       spi_message_add_tail(&t, &m);
> >> >> +       ret = spi_sync(spi, &m);
> >> >
> >> >cant you use spi_write_then_read() ?  dont let the u8* prototype scare
> >> >you, it should work with writing 16bits and then reading 16bits.
> >>
> >> I have never been scared by any u8* or something else. I only prefer
> >> to use raw spi API, which can show the bottom level timing and SPI
> >> bus feature better.
> >
> > In this case spi_write_then_read() is required, since
> > your tx and rx buffers are on the stack and hence not
> > DMA-safe.
> >
> 
> 

Re: [PATCH 2/2] add ad714x input driver for button/scrollwhell/slider/touchpad

[David Brownell]

On Monday 31 August 2009, Barry Song wrote:
> +
> +#define AD714x_SPI_ADDR        0x1C
> +#define AD714x_SPI_ADDR_SHFT   11
> +#define AD714x_SPI_READ        1
> +#define AD714x_SPI_READ_SHFT   10

Confusing; it is not an address but a fixed bit pattern
flagging command words.  Be less opaque; maybe

  #define AD714x_CMD_PREFIX       0xe000   /* bits 15:11 */
  #define AD714x_READ             BIT(10)


> +#if defined(CONFIG_INPUT_AD714X_SPI)
> +#define bus_device		struct spi_device
> +#elif defined(CONFIG_INPUT_AD714X_I2C)
> +#define bus_device		struct i2c_client
> +#else
> +#error Communication method needs to be selected (I2C or SPI)
> +#endif

Best to allow both interfaces in the same kernel, for build
tests and kernels that can run on more than one board.

The way that works in some contexts is to record

	struct device *dev;

(or somesuch) in the driver state struct and use it pretty much
everywhere (including dev_* messaging!) except bus-related code
which uses to_i2c_client() or to_spi_device() to get the right
pointer view.

And the (missing) Kconfig would depend on "SPI || I2C"; and this
driver would register (and unregister) either or both driver
facets depending on which were possible.  (Someone might even
provide stubs for the I2C=n or SPI=n cases, to let such drivers
avoid #ifdeffery.  Someday.)


> +	pr_debug("slider %d absolute position:%d\n", idx, sw->abs_pos);

You should use the dev_*() functions not pr_*().


> +#define RIGHT_END_POINT_DETECTION_LEVEL			750
> +#define LEFT_RIGHT_END_POINT_DEAVTIVALION_LEVEL		850
> +#define TOP_END_POINT_DETECTION_LEVEL			550
> +#define BOTTOM_END_POINT_DETECTION_LEVEL		950
> +#define TOP_BOTTOM_END_POINT_DEAVTIVALION_LEVEL		700
> +static int touchpad_check_endpoint(struct ad714x_chip *ad714x, int idx)
> +{

Here and elsewhere:  whitespace between CPP directives and
the Real Code (tm), please...


> +#define MAX_DEVICE_NUM 8
> +static int __devinit ad714x_probe(struct ad714x_chip *ad714x)
> +{
> +	int ret = 0;
> +	struct input_dev *input[MAX_DEVICE_NUM];
> +
> +	struct ad714x_driver_data *drv_data = NULL;
> +
> +	struct ad714x_button_plat *bt_plat   = ad714x->hw->button;
> +	struct ad714x_slider_plat *sd_plat   = ad714x->hw->slider;
> +	struct ad714x_wheel_plat *wl_plat    = ad714x->hw->wheel;
> +	struct ad714x_touchpad_plat *tp_plat = ad714x->hw->touchpad;
> +
> +	struct ad714x_button_drv *bt_drv   = NULL;
> +	struct ad714x_slider_drv *sd_drv   = NULL;
> +	struct ad714x_wheel_drv *wl_drv    = NULL;
> +	struct ad714x_touchpad_drv *tp_drv = NULL;
> +
> +	int alloc_idx = 0, reg_idx = 0;
> +	int i;
> +
> +	ret = ad714x_hw_detect(ad714x);
> +	if (ret)
> +		goto det_err;
> +
> +	/*
> +	 * Allocate and register AD714x input device
> +	 */
> +
> +	drv_data = kzalloc(sizeof(struct ad714x_driver_data), GFP_KERNEL);

drv_data is not an input device.


> +	if (!drv_data) {
> +		dev_err(&ad714x->bus->dev,
> +			"Can't allocate memory for ad714x driver info\n");
> +		ret = -ENOMEM;
> +		goto fail_alloc_reg;
> +	}
> +	ad714x->sw = drv_data;
> +
> +	/* a slider uses one input_dev instance */
> +	if (ad714x->hw->slider_num > 0) {
> +		sd_drv = kzalloc(sizeof(struct ad714x_slider_drv) *
> +				ad714x->hw->slider_num, GFP_KERNEL);
> +		if (!sd_drv) {
> +			dev_err(&ad714x->bus->dev,
> +				"Can't allocate memory for slider info\n");
> +			ret = -ENOMEM;
> +			goto fail_alloc_reg;
> +		}
> +
> +		for (i = 0; i < ad714x->hw->slider_num; i++) {
> +			input[alloc_idx] = input_allocate_device();
> +			if (!input[alloc_idx]) {
> +				dev_err(&ad714x->bus->dev,
> +				"Can't allocate input device %d\n", alloc_idx);
> +				ret = -ENOMEM;
> +				goto fail_alloc_reg;
> +			}
> +			alloc_idx++;
> +
> +			__set_bit(EV_ABS, input[alloc_idx-1]->evbit);
> +			__set_bit(ABS_X, input[alloc_idx-1]->absbit);
> +			__set_bit(ABS_PRESSURE, input[alloc_idx-1]->absbit);
> +			input_set_abs_params(input[alloc_idx-1], ABS_X, 0,
> +					sd_plat->max_coord, 0, 0);
> +			input_set_abs_params(input[alloc_idx-1], ABS_PRESSURE,
> +				0, 1, 0, 0);
> +
> +			input[alloc_idx-1]->id.bustype = BUS_I2C;

This could be SPI instead... and there are some fields you
forgot to set.  Like the ones putting this into the device tree
in the right spot, and some identifiers...

Take that bus type code as an input param to this function.


> +
> +			ret = input_register_device(input[reg_idx]);
> +			if (ret) {
> +				dev_err(&ad714x->bus->dev,
> +				"Failed to register AD714x input device!\n");
> +				goto fail_alloc_reg;
> +			}
> +			reg_idx++;
> +
> +			sd_drv[i].input = input[alloc_idx-1];
> +			ad714x->sw->slider = sd_drv;
> +		}
> +	}
> +
> +	/* a wheel uses one input_dev instance */
> +	if (ad714x->hw->wheel_num > 0) {
> +		wl_drv = kzalloc(sizeof(struct ad714x_wheel_drv) *
> +				ad714x->hw->wheel_num, GFP_KERNEL);
> +		if (!wl_drv) {
> +			dev_err(&ad714x->bus->dev,
> +				"Can't allocate memory for wheel info\n");
> +			ret = -ENOMEM;
> +			goto fail_alloc_reg;
> +		}
> +
> +		for (i = 0; i < ad714x->hw->wheel_num; i++) {
> +			input[alloc_idx] = input_allocate_device();
> +			if (!input[alloc_idx]) {
> +				dev_err(&ad714x->bus->dev,
> +				"Can't allocate input device %d\n", alloc_idx);
> +				ret = -ENOMEM;
> +				goto fail_alloc_reg;
> +			}
> +			alloc_idx++;
> +
> +			__set_bit(EV_ABS, input[alloc_idx-1]->evbit);
> +			__set_bit(ABS_WHEEL, input[alloc_idx-1]->absbit);
> +			__set_bit(ABS_PRESSURE, input[alloc_idx-1]->absbit);
> +			input_set_abs_params(input[alloc_idx-1], ABS_WHEEL, 0,
> +					wl_plat->max_coord, 0, 0);
> +			input_set_abs_params(input[alloc_idx-1], ABS_PRESSURE,
> +				0, 1, 0, 0);
> +
> +			input[alloc_idx-1]->id.bustype = BUS_I2C;

Same as above:  it's not necessarily I2C, and there are important
fields left uninitialized.  Probably worth abstracting that setup
into some kind of helper.


> +
> +			ret = input_register_device(input[reg_idx]);
> +			if (ret) {
> +				dev_err(&ad714x->bus->dev,
> +				"Failed to register AD714x input device!\n");
> +				goto fail_alloc_reg;
> +			}
> +			reg_idx++;
> +
> +			wl_drv[i].input = input[alloc_idx-1];
> +			ad714x->sw->wheel = wl_drv;
> +		}
> +	}
> +
> +	/* a touchpad uses one input_dev instance */
> +	if (ad714x->hw->touchpad_num > 0) {
> +		tp_drv = kzalloc(sizeof(struct ad714x_touchpad_drv) *
> +				ad714x->hw->touchpad_num, GFP_KERNEL);
> +		if (!tp_drv) {
> +			dev_err(&ad714x->bus->dev,
> +				"Can't allocate memory for touchpad info\n");
> +			ret = -ENOMEM;
> +			goto fail_alloc_reg;
> +		}
> +
> +		for (i = 0; i < ad714x->hw->touchpad_num; i++) {
> +			input[alloc_idx] = input_allocate_device();
> +			if (!input[alloc_idx]) {
> +				dev_err(&ad714x->bus->dev,
> +					"Can't allocate input device %d\n",
> +					alloc_idx);
> +				ret = -ENOMEM;
> +				goto fail_alloc_reg;
> +			}
> +			alloc_idx++;
> +
> +			__set_bit(EV_ABS, input[alloc_idx-1]->evbit);
> +			__set_bit(ABS_X, input[alloc_idx-1]->absbit);
> +			__set_bit(ABS_Y, input[alloc_idx-1]->absbit);
> +			__set_bit(ABS_PRESSURE, input[alloc_idx-1]->absbit);
> +			input_set_abs_params(input[alloc_idx-1], ABS_X, 0,
> +					tp_plat->x_max_coord, 0, 0);
> +			input_set_abs_params(input[alloc_idx-1], ABS_Y, 0,
> +					tp_plat->y_max_coord, 0, 0);
> +			input_set_abs_params(input[alloc_idx-1], ABS_PRESSURE,
> +				0, 1, 0, 0);
> +
> +			input[alloc_idx-1]->id.bustype = BUS_I2C;

Again with the I2C-only and incomplete setup...


> +
> +			ret = input_register_device(input[reg_idx]);
> +			if (ret) {
> +				dev_err(&ad714x->bus->dev,
> +				"Failed to register AD714x input device!\n");
> +				goto fail_alloc_reg;
> +			}
> +			reg_idx++;
> +
> +			tp_drv[i].input = input[alloc_idx-1];
> +			ad714x->sw->touchpad = tp_drv;
> +		}
> +	}
> +
> +	/* all buttons use one input node */
> +	if (ad714x->hw->button_num > 0) {
> +		bt_drv = kzalloc(sizeof(struct ad714x_button_drv) *
> +				ad714x->hw->button_num, GFP_KERNEL);
> +		if (!bt_drv) {
> +			dev_err(&ad714x->bus->dev,
> +				"Can't allocate memory for button info\n");
> +			ret = -ENOMEM;
> +			goto fail_alloc_reg;
> +		}
> +
> +		input[alloc_idx] = input_allocate_device();
> +		if (!input[alloc_idx]) {
> +			dev_err(&ad714x->bus->dev,
> +					"Can't allocate input device %d\n",
> +					alloc_idx);
> +			ret = -ENOMEM;
> +			goto fail_alloc_reg;
> +		}
> +		alloc_idx++;
> +
> +		__set_bit(EV_KEY, input[alloc_idx-1]->evbit);
> +		for (i = 0; i < ad714x->hw->button_num; i++) {
> +			__set_bit(bt_plat[i].keycode,
> +				input[alloc_idx-1]->keybit);
> +		}
> +
> +		input[alloc_idx-1]->id.bustype = BUS_I2C;

... and again ...


> +
> +		ret = input_register_device(input[reg_idx]);
> +		if (ret) {
> +			dev_err(&ad714x->bus->dev,
> +				"Failed to register AD714x input device!\n");
> +			goto fail_alloc_reg;
> +		}
> +		reg_idx++;
> +
> +		for (i = 0; i < ad714x->hw->button_num; i++)
> +			bt_drv[i].input = input[alloc_idx-1];
> +		ad714x->sw->button = bt_drv;
> +	}
> +
> +	/* initilize and request sw/hw resources */
> +
> +	ad714x_hw_init(ad714x);
> +	mutex_init(&ad714x->mutex);
> +
> +	if (ad714x->bus->irq > 0) {
> +		ret = request_threaded_irq(ad714x->bus->irq, ad714x_interrupt,
> +				ad714x_interrupt_thread, IRQF_TRIGGER_FALLING,
> +				"ad714x_captouch", ad714x);
> +		if (ret) {
> +			dev_err(&ad714x->bus->dev, "Can't allocate irq %d\n",
> +					ad714x->bus->irq);
> +			goto fail_irq;
> +		}
> +	} else
> +		dev_warn(&ad714x->bus->dev, "IRQ not configured!\n");
> +
> +	return 0;
> +
> +fail_irq:
> +fail_alloc_reg:
> +	for (i = 0; i < reg_idx; i++)
> +		input_unregister_device(input[i]);
> +	for (i = 0; i < alloc_idx; i++)
> +		input_free_device(input[i]);
> +	kfree(bt_drv); /* kfree(NULL) is safe check is not required */
> +	kfree(sd_drv);
> +	kfree(wl_drv);
> +	kfree(tp_drv);
> +	kfree(drv_data);
> +det_err:
> +	return ret;
> +}
> +
> +
> +static const struct i2c_device_id ad714x_id[] = {
> +	{ "ad714x_captouch", 0 },

Plain old "ad714x" please.  Though it might be
better to see "ad7147" and "ad7142" separately;
this doesn't work for the ad7143 and ad7148 too,
does it?  Do you know if it will work for as-yet
undefined (I think) ad714[014569] chips?


> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ad714x_id);

RE: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input driver forbutton/scrollwhell/slider/touchpad

[Song, Barry]

 

>-----Original Message-----
>From: uclinux-dist-devel-bounces@blackfin.uclinux.org 
>[mailto:uclinux-dist-devel-bounces@blackfin.uclinux.org] On 
>Behalf Of David Brownell
>Sent: Wednesday, September 02, 2009 9:52 AM
>To: Barry Song
>Cc: dbrownell@users.sourceforge.net; dtor@mail.ru; 
>dmitry.torokhov@gmail.com; 
>spi-devel-general@lists.sourceforge.net; 
>linux-input@vger.kernel.org; uclinux-dist-devel@blackfin.uclinux.org
>Subject: Re: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input 
>driver forbutton/scrollwhell/slider/touchpad
>
>On Monday 31 August 2009, Barry Song wrote:
>> +
>> +#define AD714x_SPI_ADDR        0x1C
>> +#define AD714x_SPI_ADDR_SHFT   11
>> +#define AD714x_SPI_READ        1
>> +#define AD714x_SPI_READ_SHFT   10
>
>Confusing; it is not an address but a fixed bit pattern
>flagging command words.  Be less opaque; maybe
>
>  #define AD714x_CMD_PREFIX       0xe000   /* bits 15:11 */
>  #define AD714x_READ             BIT(10)
>
>
>> +#if defined(CONFIG_INPUT_AD714X_SPI)
>> +#define bus_device		struct spi_device
>> +#elif defined(CONFIG_INPUT_AD714X_I2C)
>> +#define bus_device		struct i2c_client
>> +#else
>> +#error Communication method needs to be selected (I2C or SPI)
>> +#endif
>
>Best to allow both interfaces in the same kernel, for build
>tests and kernels that can run on more than one board.
>
>The way that works in some contexts is to record
>
>	struct device *dev;
>
>(or somesuch) in the driver state struct and use it pretty much
>everywhere (including dev_* messaging!) except bus-related code
>which uses to_i2c_client() or to_spi_device() to get the right
>pointer view.
>
>And the (missing) Kconfig would depend on "SPI || I2C"; and this
>driver would register (and unregister) either or both driver
>facets depending on which were possible.  (Someone might even
>provide stubs for the I2C=n or SPI=n cases, to let such drivers
>avoid #ifdeffery.  Someday.)

Great Idea. I will enable the two buses at the same time. 
But in fact, I2C=n or SPI=n is not necessary because matched spi/i2c
name should trigger the probe called multi-times if there are
multi-devices. But those devices have different hardware resources.

>
>
>> +	pr_debug("slider %d absolute position:%d\n", idx, sw->abs_pos);
>
>You should use the dev_*() functions not pr_*().
>
>
>> +#define RIGHT_END_POINT_DETECTION_LEVEL			750
>> +#define LEFT_RIGHT_END_POINT_DEAVTIVALION_LEVEL		850
>> +#define TOP_END_POINT_DETECTION_LEVEL			550
>> +#define BOTTOM_END_POINT_DETECTION_LEVEL		950
>> +#define TOP_BOTTOM_END_POINT_DEAVTIVALION_LEVEL		700
>> +static int touchpad_check_endpoint(struct ad714x_chip 
>*ad714x, int idx)
>> +{
>
>Here and elsewhere:  whitespace between CPP directives and
>the Real Code (tm), please...
>
>
>> +#define MAX_DEVICE_NUM 8
>> +static int __devinit ad714x_probe(struct ad714x_chip *ad714x)
>> +{
>> +	int ret = 0;
>> +	struct input_dev *input[MAX_DEVICE_NUM];
>> +
>> +	struct ad714x_driver_data *drv_data = NULL;
>> +
>> +	struct ad714x_button_plat *bt_plat   = ad714x->hw->button;
>> +	struct ad714x_slider_plat *sd_plat   = ad714x->hw->slider;
>> +	struct ad714x_wheel_plat *wl_plat    = ad714x->hw->wheel;
>> +	struct ad714x_touchpad_plat *tp_plat = ad714x->hw->touchpad;
>> +
>> +	struct ad714x_button_drv *bt_drv   = NULL;
>> +	struct ad714x_slider_drv *sd_drv   = NULL;
>> +	struct ad714x_wheel_drv *wl_drv    = NULL;
>> +	struct ad714x_touchpad_drv *tp_drv = NULL;
>> +
>> +	int alloc_idx = 0, reg_idx = 0;
>> +	int i;
>> +
>> +	ret = ad714x_hw_detect(ad714x);
>> +	if (ret)
>> +		goto det_err;
>> +
>> +	/*
>> +	 * Allocate and register AD714x input device
>> +	 */
>> +
>> +	drv_data = kzalloc(sizeof(struct ad714x_driver_data), 
>GFP_KERNEL);
>
>drv_data is not an input device.
>
>
>> +	if (!drv_data) {
>> +		dev_err(&ad714x->bus->dev,
>> +			"Can't allocate memory for ad714x 
>driver info\n");
>> +		ret = -ENOMEM;
>> +		goto fail_alloc_reg;
>> +	}
>> +	ad714x->sw = drv_data;
>> +
>> +	/* a slider uses one input_dev instance */
>> +	if (ad714x->hw->slider_num > 0) {
>> +		sd_drv = kzalloc(sizeof(struct ad714x_slider_drv) *
>> +				ad714x->hw->slider_num, GFP_KERNEL);
>> +		if (!sd_drv) {
>> +			dev_err(&ad714x->bus->dev,
>> +				"Can't allocate memory for 
>slider info\n");
>> +			ret = -ENOMEM;
>> +			goto fail_alloc_reg;
>> +		}
>> +
>> +		for (i = 0; i < ad714x->hw->slider_num; i++) {
>> +			input[alloc_idx] = input_allocate_device();
>> +			if (!input[alloc_idx]) {
>> +				dev_err(&ad714x->bus->dev,
>> +				"Can't allocate input device 
>%d\n", alloc_idx);
>> +				ret = -ENOMEM;
>> +				goto fail_alloc_reg;
>> +			}
>> +			alloc_idx++;
>> +
>> +			__set_bit(EV_ABS, input[alloc_idx-1]->evbit);
>> +			__set_bit(ABS_X, input[alloc_idx-1]->absbit);
>> +			__set_bit(ABS_PRESSURE, 
>input[alloc_idx-1]->absbit);
>> +			
>input_set_abs_params(input[alloc_idx-1], ABS_X, 0,
>> +					sd_plat->max_coord, 0, 0);
>> +			
>input_set_abs_params(input[alloc_idx-1], ABS_PRESSURE,
>> +				0, 1, 0, 0);
>> +
>> +			input[alloc_idx-1]->id.bustype = BUS_I2C;
>
>This could be SPI instead... and there are some fields you
>forgot to set.  Like the ones putting this into the device tree
>in the right spot, and some identifiers...
>
>Take that bus type code as an input param to this function.
>
>
>> +
>> +			ret = input_register_device(input[reg_idx]);
>> +			if (ret) {
>> +				dev_err(&ad714x->bus->dev,
>> +				"Failed to register AD714x 
>input device!\n");
>> +				goto fail_alloc_reg;
>> +			}
>> +			reg_idx++;
>> +
>> +			sd_drv[i].input = input[alloc_idx-1];
>> +			ad714x->sw->slider = sd_drv;
>> +		}
>> +	}
>> +
>> +	/* a wheel uses one input_dev instance */
>> +	if (ad714x->hw->wheel_num > 0) {
>> +		wl_drv = kzalloc(sizeof(struct ad714x_wheel_drv) *
>> +				ad714x->hw->wheel_num, GFP_KERNEL);
>> +		if (!wl_drv) {
>> +			dev_err(&ad714x->bus->dev,
>> +				"Can't allocate memory for 
>wheel info\n");
>> +			ret = -ENOMEM;
>> +			goto fail_alloc_reg;
>> +		}
>> +
>> +		for (i = 0; i < ad714x->hw->wheel_num; i++) {
>> +			input[alloc_idx] = input_allocate_device();
>> +			if (!input[alloc_idx]) {
>> +				dev_err(&ad714x->bus->dev,
>> +				"Can't allocate input device 
>%d\n", alloc_idx);
>> +				ret = -ENOMEM;
>> +				goto fail_alloc_reg;
>> +			}
>> +			alloc_idx++;
>> +
>> +			__set_bit(EV_ABS, input[alloc_idx-1]->evbit);
>> +			__set_bit(ABS_WHEEL, 
>input[alloc_idx-1]->absbit);
>> +			__set_bit(ABS_PRESSURE, 
>input[alloc_idx-1]->absbit);
>> +			
>input_set_abs_params(input[alloc_idx-1], ABS_WHEEL, 0,
>> +					wl_plat->max_coord, 0, 0);
>> +			
>input_set_abs_params(input[alloc_idx-1], ABS_PRESSURE,
>> +				0, 1, 0, 0);
>> +
>> +			input[alloc_idx-1]->id.bustype = BUS_I2C;
>
>Same as above:  it's not necessarily I2C, and there are important
>fields left uninitialized.  Probably worth abstracting that setup
>into some kind of helper.
>
>
>> +
>> +			ret = input_register_device(input[reg_idx]);
>> +			if (ret) {
>> +				dev_err(&ad714x->bus->dev,
>> +				"Failed to register AD714x 
>input device!\n");
>> +				goto fail_alloc_reg;
>> +			}
>> +			reg_idx++;
>> +
>> +			wl_drv[i].input = input[alloc_idx-1];
>> +			ad714x->sw->wheel = wl_drv;
>> +		}
>> +	}
>> +
>> +	/* a touchpad uses one input_dev instance */
>> +	if (ad714x->hw->touchpad_num > 0) {
>> +		tp_drv = kzalloc(sizeof(struct ad714x_touchpad_drv) *
>> +				ad714x->hw->touchpad_num, GFP_KERNEL);
>> +		if (!tp_drv) {
>> +			dev_err(&ad714x->bus->dev,
>> +				"Can't allocate memory for 
>touchpad info\n");
>> +			ret = -ENOMEM;
>> +			goto fail_alloc_reg;
>> +		}
>> +
>> +		for (i = 0; i < ad714x->hw->touchpad_num; i++) {
>> +			input[alloc_idx] = input_allocate_device();
>> +			if (!input[alloc_idx]) {
>> +				dev_err(&ad714x->bus->dev,
>> +					"Can't allocate input 
>device %d\n",
>> +					alloc_idx);
>> +				ret = -ENOMEM;
>> +				goto fail_alloc_reg;
>> +			}
>> +			alloc_idx++;
>> +
>> +			__set_bit(EV_ABS, input[alloc_idx-1]->evbit);
>> +			__set_bit(ABS_X, input[alloc_idx-1]->absbit);
>> +			__set_bit(ABS_Y, input[alloc_idx-1]->absbit);
>> +			__set_bit(ABS_PRESSURE, 
>input[alloc_idx-1]->absbit);
>> +			
>input_set_abs_params(input[alloc_idx-1], ABS_X, 0,
>> +					tp_plat->x_max_coord, 0, 0);
>> +			
>input_set_abs_params(input[alloc_idx-1], ABS_Y, 0,
>> +					tp_plat->y_max_coord, 0, 0);
>> +			
>input_set_abs_params(input[alloc_idx-1], ABS_PRESSURE,
>> +				0, 1, 0, 0);
>> +
>> +			input[alloc_idx-1]->id.bustype = BUS_I2C;
>
>Again with the I2C-only and incomplete setup...
>
>
>> +
>> +			ret = input_register_device(input[reg_idx]);
>> +			if (ret) {
>> +				dev_err(&ad714x->bus->dev,
>> +				"Failed to register AD714x 
>input device!\n");
>> +				goto fail_alloc_reg;
>> +			}
>> +			reg_idx++;
>> +
>> +			tp_drv[i].input = input[alloc_idx-1];
>> +			ad714x->sw->touchpad = tp_drv;
>> +		}
>> +	}
>> +
>> +	/* all buttons use one input node */
>> +	if (ad714x->hw->button_num > 0) {
>> +		bt_drv = kzalloc(sizeof(struct ad714x_button_drv) *
>> +				ad714x->hw->button_num, GFP_KERNEL);
>> +		if (!bt_drv) {
>> +			dev_err(&ad714x->bus->dev,
>> +				"Can't allocate memory for 
>button info\n");
>> +			ret = -ENOMEM;
>> +			goto fail_alloc_reg;
>> +		}
>> +
>> +		input[alloc_idx] = input_allocate_device();
>> +		if (!input[alloc_idx]) {
>> +			dev_err(&ad714x->bus->dev,
>> +					"Can't allocate input 
>device %d\n",
>> +					alloc_idx);
>> +			ret = -ENOMEM;
>> +			goto fail_alloc_reg;
>> +		}
>> +		alloc_idx++;
>> +
>> +		__set_bit(EV_KEY, input[alloc_idx-1]->evbit);
>> +		for (i = 0; i < ad714x->hw->button_num; i++) {
>> +			__set_bit(bt_plat[i].keycode,
>> +				input[alloc_idx-1]->keybit);
>> +		}
>> +
>> +		input[alloc_idx-1]->id.bustype = BUS_I2C;
>
>... and again ...
>
>
>> +
>> +		ret = input_register_device(input[reg_idx]);
>> +		if (ret) {
>> +			dev_err(&ad714x->bus->dev,
>> +				"Failed to register AD714x 
>input device!\n");
>> +			goto fail_alloc_reg;
>> +		}
>> +		reg_idx++;
>> +
>> +		for (i = 0; i < ad714x->hw->button_num; i++)
>> +			bt_drv[i].input = input[alloc_idx-1];
>> +		ad714x->sw->button = bt_drv;
>> +	}
>> +
>> +	/* initilize and request sw/hw resources */
>> +
>> +	ad714x_hw_init(ad714x);
>> +	mutex_init(&ad714x->mutex);
>> +
>> +	if (ad714x->bus->irq > 0) {
>> +		ret = request_threaded_irq(ad714x->bus->irq, 
>ad714x_interrupt,
>> +				ad714x_interrupt_thread, 
>IRQF_TRIGGER_FALLING,
>> +				"ad714x_captouch", ad714x);
>> +		if (ret) {
>> +			dev_err(&ad714x->bus->dev, "Can't 
>allocate irq %d\n",
>> +					ad714x->bus->irq);
>> +			goto fail_irq;
>> +		}
>> +	} else
>> +		dev_warn(&ad714x->bus->dev, "IRQ not configured!\n");
>> +
>> +	return 0;
>> +
>> +fail_irq:
>> +fail_alloc_reg:
>> +	for (i = 0; i < reg_idx; i++)
>> +		input_unregister_device(input[i]);
>> +	for (i = 0; i < alloc_idx; i++)
>> +		input_free_device(input[i]);
>> +	kfree(bt_drv); /* kfree(NULL) is safe check is not required */
>> +	kfree(sd_drv);
>> +	kfree(wl_drv);
>> +	kfree(tp_drv);
>> +	kfree(drv_data);
>> +det_err:
>> +	return ret;
>> +}
>> +
>> +
>> +static const struct i2c_device_id ad714x_id[] = {
>> +	{ "ad714x_captouch", 0 },
>
>Plain old "ad714x" please.  Though it might be
>better to see "ad7147" and "ad7142" separately;
>this doesn't work for the ad7143 and ad7148 too,
>does it?  Do you know if it will work for as-yet
>undefined (I think) ad714[014569] chips?
>
>
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ad714x_id);
>_______________________________________________
>Uclinux-dist-devel mailing list
>Uclinux-dist-devel@blackfin.uclinux.org
>https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel
>

Re: [PATCH 2/2] add ad714x input driver for button/scrollwhell/slider/touchpad

[Dmitry Torokhov]

Hi,

On Tue, Sep 01, 2009 at 11:55:30AM +0800, Barry Song wrote:
> ad7142/ad7147 are Programmable Controllers for Capacitance Touch Sensors.
> The chips don't restrict the specific usage, and it can be used as button/
> slider/scrollwheel/touchpad etc. depending on the hardware connection.
> One special target board can include one or several these components.
> 
> The driver is independent of special boards. It gets the components
> layout information from the platform_data, registers related devices,
> fullfills the algorithms and state machines for these components and
> report related input events to up level.
> 

There were already quite a few comments, I will just add:

> +			if (h_state) {
> +				slider_cal_sensor_val(ad714x, idx);
> +				slider_cal_highest_stage(ad714x, idx);
> +				slider_cal_abs_pos(ad714x, idx);
> +				slider_cal_flt_pos(ad714x, idx);
> +
> +				input_report_abs(sw->input, ABS_X, sw->flt_pos);
> +				input_report_abs(sw->input, ABS_PRESSURE, 1);

The driver does not seem to report true pressure so please don't fake
ABS_PRESSURE. You probably want to return BTN_TOUCH here,

Also, what about vertical sliders? I also wonder if slider 

> +fail_irq:
> +fail_alloc_reg:
> +	for (i = 0; i < reg_idx; i++)
> +		input_unregister_device(input[i]);
> +	for (i = 0; i < alloc_idx; i++)
> +		input_free_device(input[i]);

No callls to input_free_device after input_unregister_device.

-- 
Dmitry

RE: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input driverfor button/scrollwhell/slider/touchpad

[Song, Barry]

 

>-----Original Message-----
>From: uclinux-dist-devel-bounces@blackfin.uclinux.org 
>[mailto:uclinux-dist-devel-bounces@blackfin.uclinux.org] On 
>Behalf Of Dmitry Torokhov
>Sent: Tuesday, September 08, 2009 2:35 PM
>To: Barry Song
>Cc: spi-devel-general@lists.sourceforge.net; 
>dbrownell@users.sourceforge.net; 
>uclinux-dist-devel@blackfin.uclinux.org; linux-input@vger.kernel.org
>Subject: Re: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input 
>driverfor button/scrollwhell/slider/touchpad
>
>Hi,
>
>On Tue, Sep 01, 2009 at 11:55:30AM +0800, Barry Song wrote:
>> ad7142/ad7147 are Programmable Controllers for Capacitance 
>Touch Sensors.
>> The chips don't restrict the specific usage, and it can be 
>used as button/
>> slider/scrollwheel/touchpad etc. depending on the hardware 
>connection.
>> One special target board can include one or several these components.
>> 
>> The driver is independent of special boards. It gets the components
>> layout information from the platform_data, registers related devices,
>> fullfills the algorithms and state machines for these components and
>> report related input events to up level.
>> 
>
>There were already quite a few comments, I will just add:
>
>> +			if (h_state) {
>> +				slider_cal_sensor_val(ad714x, idx);
>> +				slider_cal_highest_stage(ad714x, idx);
>> +				slider_cal_abs_pos(ad714x, idx);
>> +				slider_cal_flt_pos(ad714x, idx);
>> +
>> +				input_report_abs(sw->input, 
>ABS_X, sw->flt_pos);
>> +				input_report_abs(sw->input, 
>ABS_PRESSURE, 1);
>
>The driver does not seem to report true pressure so please don't fake
>ABS_PRESSURE. You probably want to return BTN_TOUCH here,
Fixed
>
>Also, what about vertical sliders? I also wonder if slider 
Sliders are only single-dimensional. Is it necessary to report vertical
slider as ABS_Y and horizontal slider as ABS_X? 
>
>> +fail_irq:
>> +fail_alloc_reg:
>> +	for (i = 0; i < reg_idx; i++)
>> +		input_unregister_device(input[i]);
>> +	for (i = 0; i < alloc_idx; i++)
>> +		input_free_device(input[i]);
>
>No callls to input_free_device after input_unregister_device.
Fixed.
>
>-- 
>Dmitry
>_______________________________________________
>Uclinux-dist-devel mailing list
>Uclinux-dist-devel@blackfin.uclinux.org
>https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel
>

Re: [Uclinux-dist-devel] [PATCH 1/2] add ad714x platform_data definition

[Mike Frysinger]

On Mon, Aug 31, 2009 at 23:55, Barry Song<21cnbao@gmail.com> wrote:
> --- /dev/null
> +++ b/include/linux/spi/ad714x.h
> @@ -0,0 +1,60 @@
> +/*
> + * include/linux/ad714x.h
> + *
> + * AD714x is very flexible, it can be used as buttons, scrollwheel,
> + * slider, touchpad at the same time. That depends on the boards.
> + * The platform_data for the device's "struct device" holds this
> + * information.
> + */

missing lines:
 * Copyright 2009 Analog Devices Inc.
 *
 * Licensed under the GPL-2 or later.

> +#ifndef AD714X_HW_H
> +#define AD714X_HW_H

header files should not be using an unprefixed name space (even if
it's an uncommon define name), and the name really should match the
header.  i.e. __LINUX_SPI_AD714X_H__
-mike

RE: [Uclinux-dist-devel] [PATCH 1/2] add ad714x platform_datadefinition

[Song, Barry]

 

>-----Original Message-----
>From: uclinux-dist-devel-bounces@blackfin.uclinux.org 
>[mailto:uclinux-dist-devel-bounces@blackfin.uclinux.org] On 
>Behalf Of Mike Frysinger
>Sent: Wednesday, September 02, 2009 2:41 AM
>To: Barry Song
>Cc: dbrownell@users.sourceforge.net; dtor@mail.ru; 
>dmitry.torokhov@gmail.com; 
>spi-devel-general@lists.sourceforge.net; 
>linux-input@vger.kernel.org; uclinux-dist-devel@blackfin.uclinux.org
>Subject: Re: [Uclinux-dist-devel] [PATCH 1/2] add ad714x 
>platform_datadefinition
>
>On Mon, Aug 31, 2009 at 23:55, Barry Song<21cnbao@gmail.com> wrote:
>> --- /dev/null
>> +++ b/include/linux/spi/ad714x.h
>> @@ -0,0 +1,60 @@
>> +/*
>> + * include/linux/ad714x.h
>> + *
>> + * AD714x is very flexible, it can be used as buttons, scrollwheel,
>> + * slider, touchpad at the same time. That depends on the boards.
>> + * The platform_data for the device's "struct device" holds this
>> + * information.
>> + */
>
>missing lines:
> * Copyright 2009 Analog Devices Inc.
> *
> * Licensed under the GPL-2 or later.
>
>> +#ifndef AD714X_HW_H
>> +#define AD714X_HW_H
>
>header files should not be using an unprefixed name space (even if
>it's an uncommon define name), and the name really should match the
>header.  i.e. __LINUX_SPI_AD714X_H__
Fixed.
>-mike
>_______________________________________________
>Uclinux-dist-devel mailing list
>Uclinux-dist-devel@blackfin.uclinux.org
>https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel
>

Re: [PATCH 1/2] add ad714x platform_data definition

[David Brownell]

On Monday 31 August 2009, Barry Song wrote:
> --- /dev/null
> +++ b/include/linux/spi/ad714x.h

Doesn't belong under SPI since this is also used by the
I2C version of the chip.  Likely include/linux/input/*h
is best.


> @@ -0,0 +1,60 @@
> +/*
> + * include/linux/ad714x.h

Never include the directories.  And the coding style says
this should have a twitter-sized description of the driver
in that line, like

   * ad714x.h -- platform data for capacitance sensor driver

It's good policy to include a brief GPL statement at the top
of every source file too, including a Copyright (C) you.


> + *
> + * AD714x is very flexible, it can be used as buttons, scrollwheel,
> + * slider, touchpad at the same time. That depends on the boards.
> + * The platform_data for the device's "struct device" holds this
> + * information.
> + */
> +
> +
> +#ifndef AD714X_HW_H
> +#define AD714X_HW_H

As noted:  __LINUX_INPUT_AD714X_H is the style to use here.


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day 
trial. Simplify your report design, integration and deployment - and focus on 
what you do best, core application coding. Discover what's new with 
Crystal Reports now.  http://p.sf.net/sfu/bobj-july

RE: [Uclinux-dist-devel] [PATCH 1/2] add ad714x platform_datadefinition

[Song, Barry]

 

>-----Original Message-----
>From: uclinux-dist-devel-bounces@blackfin.uclinux.org 
>[mailto:uclinux-dist-devel-bounces@blackfin.uclinux.org] On 
>Behalf Of David Brownell
>Sent: Wednesday, September 02, 2009 9:58 AM
>To: Barry Song
>Cc: dbrownell@users.sourceforge.net; dtor@mail.ru; 
>dmitry.torokhov@gmail.com; 
>spi-devel-general@lists.sourceforge.net; 
>linux-input@vger.kernel.org; uclinux-dist-devel@blackfin.uclinux.org
>Subject: Re: [Uclinux-dist-devel] [PATCH 1/2] add ad714x 
>platform_datadefinition
>
>On Monday 31 August 2009, Barry Song wrote:
>> --- /dev/null
>> +++ b/include/linux/spi/ad714x.h
>
>Doesn't belong under SPI since this is also used by the
>I2C version of the chip.  Likely include/linux/input/*h
>is best.

I really wanted to place it to include/linux/input, but the
include/linux/input doesn't exist and current input header files are
placed in the parent dir "include/linux".

>
>
>> @@ -0,0 +1,60 @@
>> +/*
>> + * include/linux/ad714x.h
>
>Never include the directories.  And the coding style says
>this should have a twitter-sized description of the driver
>in that line, like
>
>   * ad714x.h -- platform data for capacitance sensor driver
>
>It's good policy to include a brief GPL statement at the top
>of every source file too, including a Copyright (C) you.
>
>
>> + *
>> + * AD714x is very flexible, it can be used as buttons, scrollwheel,
>> + * slider, touchpad at the same time. That depends on the boards.
>> + * The platform_data for the device's "struct device" holds this
>> + * information.
>> + */
>> +
>> +
>> +#ifndef AD714X_HW_H
>> +#define AD714X_HW_H
>
>As noted:  __LINUX_INPUT_AD714X_H is the style to use here.
>
>_______________________________________________
>Uclinux-dist-devel mailing list
>Uclinux-dist-devel@blackfin.uclinux.org
>https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel
>

Re: [PATCH 1/2] add ad714x platform_datadefinition

[David Brownell]

On Tuesday 01 September 2009, Song, Barry wrote:
> >Doesn't belong under SPI since this is also used by the
> >I2C version of the chip.  Likely include/linux/input/*h
> >is best.
> 
> I really wanted to place it to include/linux/input, but the
> include/linux/input doesn't exist and current input header files are
> placed in the parent dir "include/linux".
> 

$ ls include/linux/input
matrix_keypad.h
$ 

RE: [Uclinux-dist-devel] [PATCH 1/2] add ad714x platform_datadefinition

[Song, Barry]

 

>-----Original Message-----
>From: David Brownell [mailto:david-b@pacbell.net] 
>Sent: Wednesday, September 02, 2009 1:14 PM
>To: Song, Barry
>Cc: Barry Song; dbrownell@users.sourceforge.net; dtor@mail.ru; 
>dmitry.torokhov@gmail.com; 
>spi-devel-general@lists.sourceforge.net; 
>linux-input@vger.kernel.org; uclinux-dist-devel@blackfin.uclinux.org
>Subject: Re: [Uclinux-dist-devel] [PATCH 1/2] add ad714x 
>platform_datadefinition
>
>On Tuesday 01 September 2009, Song, Barry wrote:
>> >Doesn't belong under SPI since this is also used by the
>> >I2C version of the chip.  Likely include/linux/input/*h
>> >is best.
>> 
>> I really wanted to place it to include/linux/input, but the
>> include/linux/input doesn't exist and current input header files are
>> placed in the parent dir "include/linux".
>> 
>
>$ ls include/linux/input
>matrix_keypad.h
Thanks. A sync issue. A recent commit adds this file.

author	Eric Miao <eric.y.miao@gmail.com>
	Mon, 29 Jun 2009 07:20:52 +0000 (00:20 -0700)
committer	Dmitry Torokhov <dmitry.torokhov@gmail.com>
	Mon, 29 Jun 2009 07:30:16 +0000 (00:30 -0700)
commit	bab7614d6d1b1fc96ec6c5a7ca34c8641060e659
tree	f7b493a73b03d39e982a325117a14a04b712d1c0	
parent	cb589529f74d69abc111887b45308f333f950ade

>$ 
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [spi-devel-general] [PATCH 1/2] add ad714x platform_data definition

[Bill Gatliff]

David Brownell wrote:
> It's good policy to include a brief GPL statement at the top
> of every source file too, including a Copyright (C) you.
>   

I think "good policy" is probably understating things a bit.  :)



b.g.

-- 
Bill Gatliff
bgat@billgatliff.com

Re: [Uclinux-dist-devel] [PATCH 0/2] add ad714x captouch sensor input driver

[Mike Frysinger]

On Mon, Aug 31, 2009 at 23:55, Barry Song wrote:
> David,
> The patch[1/2] adds a file with platform_data definition for AD714x in include/linux/spi,
> which describes the components layout on special target boards and need be filled in
> arch/mach.

i dont think this needs to split merged between David's spi and
Dmitry's input tree.  would make more sense to me to merge it as one
patchset via Dmitry's input tree.

> Dmitry,
> The patch[2/2] adds a file in drivers/input/misc and changes Kconfig&Makefile, fulfilling
> the algorithms and state machines for the related buttons/sliders/scrollwheels/touchpads
> defined in platform_data. So the driver becomes be independent of special target boards.

looks like your patch [2/2] is incomplete as it doesnt include any
Kconfig or Makefile changes
-mike

Re: [Uclinux-dist-devel] [PATCH 0/2] add ad714x captouch sensor input driver

[David Brownell]

On Tuesday 01 September 2009, Mike Frysinger wrote:
> i dont think this needs to split merged between David's spi and
> Dmitry's input tree.  would make more sense to me to merge it as one
> patchset via Dmitry's input tree.

Right; one driver should usually be one patch.


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day 
trial. Simplify your report design, integration and deployment - and focus on 
what you do best, core application coding. Discover what's new with 
Crystal Reports now.  http://p.sf.net/sfu/bobj-july

RE: [Uclinux-dist-devel] [PATCH 0/2] add ad714x captouch sensorinput driver

[Song, Barry]

 

>-----Original Message-----
>From: uclinux-dist-devel-bounces@blackfin.uclinux.org 
>[mailto:uclinux-dist-devel-bounces@blackfin.uclinux.org] On 
>Behalf Of Mike Frysinger
>Sent: Wednesday, September 02, 2009 2:46 AM
>To: Barry Song
>Cc: dbrownell@users.sourceforge.net; dtor@mail.ru; 
>dmitry.torokhov@gmail.com; 
>spi-devel-general@lists.sourceforge.net; 
>linux-input@vger.kernel.org; uclinux-dist-devel@blackfin.uclinux.org
>Subject: Re: [Uclinux-dist-devel] [PATCH 0/2] add ad714x 
>captouch sensorinput driver
>
>On Mon, Aug 31, 2009 at 23:55, Barry Song wrote:
>> David,
>> The patch[1/2] adds a file with platform_data definition for 
>AD714x in include/linux/spi,
>> which describes the components layout on special target 
>boards and need be filled in
>> arch/mach.
>
>i dont think this needs to split merged between David's spi and
>Dmitry's input tree.  would make more sense to me to merge it as one
>patchset via Dmitry's input tree.
I split them because I am not sure whether input tree can accept files
added in include/linux/spi which is not maintained by them directly.
I think maybe only spi men can accept? I have no idea.
> 
>> Dmitry,
>> The patch[2/2] adds a file in drivers/input/misc and changes 
>Kconfig&Makefile, fulfilling
>> the algorithms and state machines for the related 
>buttons/sliders/scrollwheels/touchpads
>> defined in platform_data. So the driver becomes be 
>independent of special target boards.
>
>looks like your patch [2/2] is incomplete as it doesnt include any
>Kconfig or Makefile changes
>-mike
>_______________________________________________
>Uclinux-dist-devel mailing list
>Uclinux-dist-devel@blackfin.uclinux.org
>https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel
>