* Re: [PATCH] add support for ALPS v7 protocol device
From: Elaine Chen @ 2014-01-14 0:59 UTC (permalink / raw)
To: Dmitry Torokhov, cernekee, david turvene
Cc: linux-input, ndevos, jclift, Qiting Chen
In-Reply-To: <1389333218-31139-1-git-send-email-qiting.chen@cn.alps.com>
As far as I know, the ALPS v7 protocol device is used on following laptops:
Lenovo S430/S435/S530, Lenovo Z410/Z510, HP Odie, HP Revolve 810 G1
2014/1/10 Qiting Chen <elaineee66@gmail.com>:
> Here is the patch of supporting ALPS v7 protocol device.
>
> v7 device is a clickpad device.
> Device info:
> Device ID = 0x73, 0x03, 0x0a
> Firmware ID = 0x88, 0xb*, 0x**
>
> Support function of v7 device:
> - Cursor
> - Tap, double tap, tap drag, 2finger tap
> - Pan, pinch
> - Button click: v7 device has one physical button under the touchpad. A Button Area covers the bottom of touchpad. Clicking on Button Area produces button acitivities.
> Click touchpad with all fingers outside right Button Area --> left click
> Click touchpad with at lease 1 finger inside right Button Area --> right click
> - Resting finger function: Cursor and gestures won't be influenced with one finger placed still on Button Area.
>
> The resting finger process is in alps.c as it seems the latest xserver-xorg-input-synaptics(1.7.1) doesn't support that part.
> We tried registering our device as a clickpad by:
> set_bit(INPUT_PROP_BUTTONPAD, dev1->propbit)
> But only button click can be correctly recognized. Yet a finger at Button Area won't be ignored while doing cursroing.
>
>
> Signed-off-by: Qiting Chen <qiting.chen@cn.alps.com>
> ---
> drivers/input/mouse/alps.c | 478 ++++++++++++++++++++++++++++++++++++++++++---
> drivers/input/mouse/alps.h | 127 ++++++++++--
> 2 files changed, 554 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index fb15c64..3e8e8f7 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -32,6 +32,11 @@
> #define ALPS_REG_BASE_RUSHMORE 0xc2c0
> #define ALPS_REG_BASE_PINNACLE 0x0000
>
> +#define LEFT_BUTTON_BIT 0x01
> +#define RIGHT_BUTTON_BIT 0x02
> +
> +#define V7_LARGE_MOVEMENT 130
> +
> static const struct alps_nibble_commands alps_v3_nibble_commands[] = {
> { PSMOUSE_CMD_SETPOLL, 0x00 }, /* 0 */
> { PSMOUSE_CMD_RESET_DIS, 0x00 }, /* 1 */
> @@ -99,6 +104,7 @@ static const struct alps_nibble_commands alps_v6_nibble_commands[] = {
> #define ALPS_FOUR_BUTTONS 0x40 /* 4 direction button present */
> #define ALPS_PS2_INTERLEAVED 0x80 /* 3-byte PS/2 packet interleaved with
> 6-byte ALPS packet */
> +#define ALPS_BTNLESS 0x100 /* ALPS ClickPad flag */
>
> static const struct alps_model_info alps_model_data[] = {
> { { 0x32, 0x02, 0x14 }, 0x00, ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_PASS | ALPS_DUALPOINT }, /* Toshiba Salellite Pro M10 */
> @@ -140,6 +146,20 @@ static void alps_set_abs_params_mt(struct alps_data *priv,
> * isn't valid per PS/2 spec.
> */
>
> +static unsigned int alps_pt_distance(struct alps_abs_data *pt0,
> + struct alps_abs_data *pt1)
> +{
> + int vect_x, vect_y;
> +
> + if (!pt0 || !pt1)
> + return 0;
> +
> + vect_x = pt0->x - pt1->x;
> + vect_y = pt0->y - pt1->y;
> +
> + return int_sqrt(vect_x * vect_x + vect_y * vect_y);
> +}
> +
> /* Packet formats are described in Documentation/input/alps.txt */
>
> static bool alps_is_valid_first_byte(struct alps_data *priv,
> @@ -320,8 +340,8 @@ static void alps_process_bitmap_dolphin(struct alps_data *priv,
> end_bit = y_msb - 1;
> box_middle_y = (priv->y_max * (start_bit + end_bit)) /
> (2 * (priv->y_bits - 1));
> - *x1 = fields->x;
> - *y1 = fields->y;
> + *x1 = fields->pt.x;
> + *y1 = fields->pt.y;
> *x2 = 2 * box_middle_x - *x1;
> *y2 = 2 * box_middle_y - *y1;
> }
> @@ -461,6 +481,38 @@ static void alps_report_semi_mt_data(struct input_dev *dev, int num_fingers,
> alps_set_slot(dev, 1, num_fingers == 2, x2, y2);
> }
>
> +static void alps_report_coord_and_btn(struct psmouse *psmouse,
> + struct alps_fields *f)
> +{
> + struct input_dev *dev;
> +
> + if (!psmouse || !f)
> + return;
> +
> + dev = psmouse->dev;
> +
> + if (f->fingers) {
> + input_report_key(dev, BTN_TOUCH, 1);
> + alps_report_semi_mt_data(dev, f->fingers,
> + f->pt_img[0].x, f->pt_img[0].y,
> + f->pt_img[1].x, f->pt_img[1].y);
> + input_mt_report_finger_count(dev, f->fingers);
> +
> + input_report_abs(dev, ABS_X, f->pt_img[0].x);
> + input_report_abs(dev, ABS_Y, f->pt_img[0].y);
> + input_report_abs(dev, ABS_PRESSURE, f->pt_img[0].z);
> + } else {
> + input_report_key(dev, BTN_TOUCH, 0);
> + input_mt_report_finger_count(dev, 0);
> + input_report_abs(dev, ABS_PRESSURE, 0);
> + }
> +
> + input_report_key(dev, BTN_LEFT, f->btn.left);
> + input_report_key(dev, BTN_RIGHT, f->btn.right);
> +
> + input_sync(dev);
> +}
> +
> static void alps_process_trackstick_packet_v3(struct psmouse *psmouse)
> {
> struct alps_data *priv = psmouse->private;
> @@ -523,13 +575,13 @@ static void alps_process_trackstick_packet_v3(struct psmouse *psmouse)
>
> static void alps_decode_buttons_v3(struct alps_fields *f, unsigned char *p)
> {
> - f->left = !!(p[3] & 0x01);
> - f->right = !!(p[3] & 0x02);
> - f->middle = !!(p[3] & 0x04);
> + f->btn.left = !!(p[3] & 0x01);
> + f->btn.right = !!(p[3] & 0x02);
> + f->btn.middle = !!(p[3] & 0x04);
>
> - f->ts_left = !!(p[3] & 0x10);
> - f->ts_right = !!(p[3] & 0x20);
> - f->ts_middle = !!(p[3] & 0x40);
> + f->btn.ts_left = !!(p[3] & 0x10);
> + f->btn.ts_right = !!(p[3] & 0x20);
> + f->btn.ts_middle = !!(p[3] & 0x40);
> }
>
> static void alps_decode_pinnacle(struct alps_fields *f, unsigned char *p,
> @@ -546,10 +598,10 @@ static void alps_decode_pinnacle(struct alps_fields *f, unsigned char *p,
> ((p[2] & 0x7f) << 1) |
> (p[4] & 0x01);
>
> - f->x = ((p[1] & 0x7f) << 4) | ((p[4] & 0x30) >> 2) |
> + f->pt.x = ((p[1] & 0x7f) << 4) | ((p[4] & 0x30) >> 2) |
> ((p[0] & 0x30) >> 4);
> - f->y = ((p[2] & 0x7f) << 4) | (p[4] & 0x0f);
> - f->z = p[5] & 0x7f;
> + f->pt.y = ((p[2] & 0x7f) << 4) | (p[4] & 0x0f);
> + f->pt.z = p[5] & 0x7f;
>
> alps_decode_buttons_v3(f, p);
> }
> @@ -573,9 +625,9 @@ static void alps_decode_dolphin(struct alps_fields *f, unsigned char *p,
> f->is_mp = !!(p[0] & 0x20);
>
> if (!f->is_mp) {
> - f->x = ((p[1] & 0x7f) | ((p[4] & 0x0f) << 7));
> - f->y = ((p[2] & 0x7f) | ((p[4] & 0xf0) << 3));
> - f->z = (p[0] & 4) ? 0 : p[5] & 0x7f;
> + f->pt.x = ((p[1] & 0x7f) | ((p[4] & 0x0f) << 7));
> + f->pt.y = ((p[2] & 0x7f) | ((p[4] & 0xf0) << 3));
> + f->pt.z = (p[0] & 4) ? 0 : p[5] & 0x7f;
> alps_decode_buttons_v3(f, p);
> } else {
> f->fingers = ((p[0] & 0x6) >> 1 |
> @@ -687,7 +739,7 @@ static void alps_process_touchpad_packet_v3_v5(struct psmouse *psmouse)
> * with x, y, and z all zero, so these seem to be flukes.
> * Ignore them.
> */
> - if (f.x && f.y && !f.z)
> + if (f.pt.x && f.pt.y && !f.pt.z)
> return;
>
> /*
> @@ -695,12 +747,12 @@ static void alps_process_touchpad_packet_v3_v5(struct psmouse *psmouse)
> * to rely on ST data.
> */
> if (!fingers) {
> - x1 = f.x;
> - y1 = f.y;
> - fingers = f.z > 0 ? 1 : 0;
> + x1 = f.pt.x;
> + y1 = f.pt.y;
> + fingers = f.pt.z > 0 ? 1 : 0;
> }
>
> - if (f.z >= 64)
> + if (f.pt.z >= 64)
> input_report_key(dev, BTN_TOUCH, 1);
> else
> input_report_key(dev, BTN_TOUCH, 0);
> @@ -709,22 +761,22 @@ static void alps_process_touchpad_packet_v3_v5(struct psmouse *psmouse)
>
> input_mt_report_finger_count(dev, fingers);
>
> - input_report_key(dev, BTN_LEFT, f.left);
> - input_report_key(dev, BTN_RIGHT, f.right);
> - input_report_key(dev, BTN_MIDDLE, f.middle);
> + input_report_key(dev, BTN_LEFT, f.btn.left);
> + input_report_key(dev, BTN_RIGHT, f.btn.right);
> + input_report_key(dev, BTN_MIDDLE, f.btn.middle);
>
> - if (f.z > 0) {
> - input_report_abs(dev, ABS_X, f.x);
> - input_report_abs(dev, ABS_Y, f.y);
> + if (f.pt.z > 0) {
> + input_report_abs(dev, ABS_X, f.pt.x);
> + input_report_abs(dev, ABS_Y, f.pt.y);
> }
> - input_report_abs(dev, ABS_PRESSURE, f.z);
> + input_report_abs(dev, ABS_PRESSURE, f.pt.z);
>
> input_sync(dev);
>
> if (!(priv->quirks & ALPS_QUIRK_TRACKSTICK_BUTTONS)) {
> - input_report_key(dev2, BTN_LEFT, f.ts_left);
> - input_report_key(dev2, BTN_RIGHT, f.ts_right);
> - input_report_key(dev2, BTN_MIDDLE, f.ts_middle);
> + input_report_key(dev2, BTN_LEFT, f.btn.ts_left);
> + input_report_key(dev2, BTN_RIGHT, f.btn.ts_right);
> + input_report_key(dev2, BTN_MIDDLE, f.btn.ts_middle);
> input_sync(dev2);
> }
> }
> @@ -916,6 +968,294 @@ static void alps_process_packet_v4(struct psmouse *psmouse)
> input_sync(dev);
> }
>
> +static bool alps_is_valid_package_v7(struct psmouse *psmouse)
> +{
> + if ((psmouse->pktcnt == 3) && ((psmouse->packet[2] & 0x40) != 0x40))
> + return false;
> + if ((psmouse->pktcnt == 4) && ((psmouse->packet[3] & 0x48) != 0x48))
> + return false;
> + if ((psmouse->pktcnt == 6) && ((psmouse->packet[5] & 0x40) != 0x0))
> + return false;
> + return true;
> +}
> +
> +static int alps_drop_unsupported_packet_v7(struct psmouse *psmouse)
> +{
> + struct alps_data *priv = psmouse->private;
> + int drop = 1;
> +
> + if (priv->r.v7.pkt_id == V7_PACKET_ID_NEW ||
> + priv->r.v7.pkt_id == V7_PACKET_ID_TWO ||
> + priv->r.v7.pkt_id == V7_PACKET_ID_MULTI ||
> + priv->r.v7.pkt_id == V7_PACKET_ID_IDLE)
> + drop = 0;
> +
> + return drop;
> +}
> +
> +static unsigned char alps_get_packet_id_v7(char *byte)
> +{
> + unsigned char packet_id;
> +
> + if (byte[4] & 0x40)
> + packet_id = V7_PACKET_ID_TWO;
> + else if (byte[4] & 0x01)
> + packet_id = V7_PACKET_ID_MULTI;
> + else if ((byte[0] & 0x10) && !(byte[4] & 0x43))
> + packet_id = V7_PACKET_ID_NEW;
> + else
> + packet_id = V7_PACKET_ID_IDLE;
> +
> + return packet_id;
> +}
> +
> +static void alps_get_finger_coordinate_v7(struct alps_abs_data *pt,
> + unsigned char *pkt,
> + unsigned char pkt_id)
> +{
> + if ((pkt_id == V7_PACKET_ID_TWO) ||
> + (pkt_id == V7_PACKET_ID_MULTI) ||
> + (pkt_id == V7_PACKET_ID_NEW)) {
> + pt[0].x = ((pkt[2] & 0x80) << 4);
> + pt[0].x |= ((pkt[2] & 0x3F) << 5);
> + pt[0].x |= ((pkt[3] & 0x30) >> 1);
> + pt[0].x |= (pkt[3] & 0x07);
> + pt[0].y = (pkt[1] << 3) | (pkt[0] & 0x07);
> +
> + pt[1].x = ((pkt[3] & 0x80) << 4);
> + pt[1].x |= ((pkt[4] & 0x80) << 3);
> + pt[1].x |= ((pkt[4] & 0x3F) << 4);
> + pt[1].y = ((pkt[5] & 0x80) << 3);
> + pt[1].y |= ((pkt[5] & 0x3F) << 4);
> +
> + if (pkt_id == V7_PACKET_ID_TWO) {
> + pt[1].x &= ~0x000F;
> + pt[1].y |= 0x000F;
> + } else if (pkt_id == V7_PACKET_ID_MULTI) {
> + pt[1].x &= ~0x003F;
> + pt[1].y &= ~0x0020;
> + pt[1].y |= ((pkt[4] & 0x02) << 4);
> + pt[1].y |= 0x001F;
> + } else if (pkt_id == V7_PACKET_ID_NEW) {
> + pt[1].x &= ~0x003F;
> + pt[1].x |= (pkt[0] & 0x20);
> + pt[1].y |= 0x000F;
> + }
> +
> + pt[0].y = 0x7FF - pt[0].y;
> + pt[1].y = 0x7FF - pt[1].y;
> +
> + pt[0].z = (pt[0].x && pt[0].y) ? 62 : 0;
> + pt[1].z = (pt[1].x && pt[1].y) ? 62 : 0;
> + }
> +}
> +
> +static void alps_decode_packet_v7(struct alps_fields *f,
> + unsigned char *p,
> + struct psmouse *psmouse)
> +{
> + struct alps_data *priv = psmouse->private;
> +
> + priv->r.v7.pkt_id = alps_get_packet_id_v7(p);
> +
> + alps_get_finger_coordinate_v7(f->pt_img, p, priv->r.v7.pkt_id);
> +
> + priv->r.v7.rest_left = 0;
> + priv->r.v7.rest_right = 0;
> + priv->r.v7.additional_fingers = 0;
> + priv->phy_btn = 0;
> +
> + if (priv->r.v7.pkt_id == V7_PACKET_ID_TWO ||
> + priv->r.v7.pkt_id == V7_PACKET_ID_MULTI) {
> + priv->r.v7.rest_left = (p[0] & 0x10) >> 4;
> + priv->r.v7.rest_right = (p[0] & 0x20) >> 5;
> + }
> +
> + if (priv->r.v7.pkt_id == V7_PACKET_ID_MULTI)
> + priv->r.v7.additional_fingers = p[5] & 0x03;
> +
> + priv->phy_btn = (p[0] & 0x80) >> 7;
> +}
> +
> +static void alps_set_each_pt_attr_v7(struct psmouse *psmouse,
> + struct alps_abs_data *pt,
> + struct alps_bl_pt_attr *pt_attr)
> +{
> + struct alps_data *priv = psmouse->private;
> + unsigned int dist;
> +
> + if (!pt_attr->is_init_pt_got && pt->z != 0) {
> + pt_attr->is_init_pt_got = 1;
> + pt_attr->is_counted = 0;
> + memcpy(&pt_attr->init_pt, pt, sizeof(pt_attr->init_pt));
> + }
> +
> + if (pt->z != 0) {
> + if (pt->y < priv->resting_zone_y_min) {
> + /* A finger is recognized as a non-resting finger
> + if it's position is outside the resting finger zone.*/
> + pt_attr->zone = ZONE_NORMAL;
> + pt_attr->is_counted = 1;
> + } else {
> + /* A finger is recognized as a resting finger if it's
> + position is inside the resting finger zone and there's
> + no large movement from it's touch down position.*/
> + pt_attr->zone = ZONE_RESTING;
> +
> + if (pt->x > priv->x_max / 2)
> + pt_attr->zone |= ZONE_RIGHT_BTN;
> + else
> + pt_attr->zone |= ZONE_LEFT_BTN;
> +
> + /* A resting finger will turn to be a non-resting
> + finger if it has made large movement from it's touch
> + down position. A non-resting finger will never turn
> + to a resting finger before it leaves the touchpad
> + surface.*/
> + if (pt_attr->is_init_pt_got) {
> + dist = alps_pt_distance(pt, &pt_attr->init_pt);
> +
> + if (dist > V7_LARGE_MOVEMENT)
> + pt_attr->is_counted = 1;
> + }
> + }
> + }
> +}
> +
> +static void alps_set_pt_attr_v7(struct psmouse *psmouse,
> + struct alps_fields *f)
> +{
> + struct alps_data *priv = psmouse->private;
> + int i;
> +
> + switch (priv->r.v7.pkt_id) {
> + case V7_PACKET_ID_TWO:
> + case V7_PACKET_ID_MULTI:
> + for (i = 0; i < V7_IMG_PT_NUM; i++)
> + alps_set_each_pt_attr_v7(psmouse,
> + &f->pt_img[i],
> + &priv->pt_attr[i]);
> + break;
> + default:
> + /*All finger attributes are cleared when packet ID is
> + 'IDLE', 'New'or other unknown IDs. An 'IDLE' packet
> + indicates that there's no finger and no button activity.
> + A 'NEW' packet indicates the finger position in packet
> + is not continues from previous packet. Such as the
> + condition there's finger placed or lifted. In these cases,
> + finger attributes will be reset.*/
> + memset(priv->pt_attr, 0, sizeof(priv->pt_attr[0]) * 2);
> + break;
> + }
> +}
> +
> +static void alps_cal_output_finger_num_v7(struct psmouse *psmouse,
> + struct alps_fields *f)
> +{
> + struct alps_data *priv = psmouse->private;
> + unsigned int fn = 0;
> + int i;
> +
> + switch (priv->r.v7.pkt_id) {
> + case V7_PACKET_ID_IDLE:
> + case V7_PACKET_ID_NEW:
> + /*No finger is reported when packet ID is 'IDLE' or 'New'.
> + An 'IDLE' packet indicates that there's no finger on touchpad.
> + A 'NEW' packet indicates there's finger placed or lifted.
> + Finger position of 'New' packet is not continues from the
> + previous packet.*/
> + fn = 0;
> + break;
> + case V7_PACKET_ID_TWO:
> + if (f->pt_img[0].z == 0) {
> + /*The first finger slot is zero when a non-resting
> + finger lifted and remaining only one resting finger
> + on touchpad. Hardware report the remaining resting
> + finger in second slot. This resting is ignored*/
> + fn = 0;
> + } else if (f->pt_img[1].z == 0) {
> + /* The second finger slot is zero if there's
> + only one finger*/
> + fn = 1;
> + } else {
> + /*All non-resting fingers will be counted to report*/
> + fn = 0;
> + for (i = 0; i < V7_IMG_PT_NUM; i++) {
> + if (priv->pt_attr[i].is_counted)
> + fn++;
> + }
> +
> + /*In the case that both fingers are
> + resting fingers, report the first one*/
> + if (!priv->pt_attr[0].is_counted &&
> + !priv->pt_attr[1].is_counted) {
> + fn = 1;
> + }
> + }
> + break;
> + case V7_PACKET_ID_MULTI:
> + /*A packet ID 'MULTI' indicats that at least 3 non-resting
> + finger exist.*/
> + fn = 3 + priv->r.v7.additional_fingers;
> + break;
> + }
> +
> + f->fingers = fn;
> +}
> +
> +static void alps_assign_buttons_v7(struct psmouse *psmouse,
> + struct alps_fields *f)
> +{
> + struct alps_data *priv = psmouse->private;
> +
> + if (priv->phy_btn) {
> + if (!priv->prev_phy_btn) {
> + /* Report a right click as long as there's finger on
> + right button zone. Othrewise, report a left click.*/
> + if (priv->r.v7.rest_right ||
> + priv->pt_attr[0].zone & ZONE_RIGHT_BTN ||
> + priv->pt_attr[1].zone & ZONE_RIGHT_BTN) {
> + f->btn.right = 1;
> + priv->pressed_btn_bits |= RIGHT_BUTTON_BIT;
> + } else {
> + f->btn.left = 1;
> + priv->pressed_btn_bits |= LEFT_BUTTON_BIT;
> + }
> + } else {
> + if (priv->pressed_btn_bits & RIGHT_BUTTON_BIT)
> + f->btn.right = 1;
> + if (priv->pressed_btn_bits & LEFT_BUTTON_BIT)
> + f->btn.left = 1;
> + }
> + } else {
> + priv->pressed_btn_bits = 0;
> + f->btn.right = 0;
> + f->btn.left = 0;
> + }
> +
> + priv->prev_phy_btn = priv->phy_btn;
> +}
> +
> +static void alps_process_packet_v7(struct psmouse *psmouse)
> +{
> + struct alps_data *priv = psmouse->private;
> + struct alps_fields f = {0};
> + unsigned char *packet = psmouse->packet;
> +
> + priv->decode_fields(&f, packet, psmouse);
> +
> + if (alps_drop_unsupported_packet_v7(psmouse))
> + return;
> +
> + alps_set_pt_attr_v7(psmouse, &f);
> +
> + alps_cal_output_finger_num_v7(psmouse, &f);
> +
> + alps_assign_buttons_v7(psmouse, &f);
> +
> + alps_report_coord_and_btn(psmouse, &f);
> +}
> +
> static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
> unsigned char packet[],
> bool report_buttons)
> @@ -1080,6 +1420,14 @@ static psmouse_ret_t alps_process_byte(struct psmouse *psmouse)
> return PSMOUSE_BAD_DATA;
> }
>
> + if ((priv->proto_version == ALPS_PROTO_V7 &&
> + !alps_is_valid_package_v7(psmouse))) {
> + psmouse_dbg(psmouse, "refusing packet[%i] = %x\n",
> + psmouse->pktcnt - 1,
> + psmouse->packet[psmouse->pktcnt - 1]);
> + return PSMOUSE_BAD_DATA;
> + }
> +
> if (psmouse->pktcnt == psmouse->pktsize) {
> priv->process_packet(psmouse);
> return PSMOUSE_FULL_PACKET;
> @@ -1192,6 +1540,22 @@ static int alps_rpt_cmd(struct psmouse *psmouse, int init_command,
> return 0;
> }
>
> +static int alps_check_valid_firmware_id(unsigned char id[])
> +{
> + int valid = 1;
> +
> + if (id[0] == 0x73)
> + valid = 1;
> + else if (id[0] == 0x88) {
> + if ((id[1] == 0x07) ||
> + (id[1] == 0x08) ||
> + ((id[1] & 0xf0) == 0xB0))
> + valid = 1;
> + }
> +
> + return valid;
> +}
> +
> static int alps_enter_command_mode(struct psmouse *psmouse)
> {
> unsigned char param[4];
> @@ -1201,8 +1565,7 @@ static int alps_enter_command_mode(struct psmouse *psmouse)
> return -1;
> }
>
> - if ((param[0] != 0x88 || (param[1] != 0x07 && param[1] != 0x08)) &&
> - param[0] != 0x73) {
> + if (!alps_check_valid_firmware_id(param)) {
> psmouse_dbg(psmouse,
> "unknown response while entering command mode\n");
> return -1;
> @@ -1704,6 +2067,32 @@ error:
> return ret;
> }
>
> +static int alps_hw_init_v7(struct psmouse *psmouse)
> +{
> + struct ps2dev *ps2dev = &psmouse->ps2dev;
> + int reg_val, ret = -1;
> +
> + if (alps_enter_command_mode(psmouse) ||
> + alps_command_mode_read_reg(psmouse, 0xc2d9) == -1)
> + goto error;
> +
> + if (alps_command_mode_write_reg(psmouse, 0xc2c9, 0x64))
> + goto error;
> +
> + reg_val = alps_command_mode_read_reg(psmouse, 0xc2c4);
> + if (reg_val == -1)
> + goto error;
> + if (__alps_command_mode_write_reg(psmouse, reg_val | 0x02))
> + goto error;
> +
> + alps_exit_command_mode(psmouse);
> + return ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
> +
> +error:
> + alps_exit_command_mode(psmouse);
> + return ret;
> +}
> +
> /* Must be in command mode when calling this function */
> static int alps_absolute_mode_v4(struct psmouse *psmouse)
> {
> @@ -1875,6 +2264,7 @@ static void alps_set_defaults(struct alps_data *priv)
> priv->set_abs_params = alps_set_abs_params_st;
> priv->x_max = 1023;
> priv->y_max = 767;
> + priv->slot_number = 1;
> break;
> case ALPS_PROTO_V3:
> priv->hw_init = alps_hw_init_v3;
> @@ -1883,6 +2273,7 @@ static void alps_set_defaults(struct alps_data *priv)
> priv->decode_fields = alps_decode_pinnacle;
> priv->nibble_commands = alps_v3_nibble_commands;
> priv->addr_command = PSMOUSE_CMD_RESET_WRAP;
> + priv->slot_number = 2;
> break;
> case ALPS_PROTO_V4:
> priv->hw_init = alps_hw_init_v4;
> @@ -1890,6 +2281,7 @@ static void alps_set_defaults(struct alps_data *priv)
> priv->set_abs_params = alps_set_abs_params_mt;
> priv->nibble_commands = alps_v4_nibble_commands;
> priv->addr_command = PSMOUSE_CMD_DISABLE;
> + priv->slot_number = 2;
> break;
> case ALPS_PROTO_V5:
> priv->hw_init = alps_hw_init_dolphin_v1;
> @@ -1905,6 +2297,7 @@ static void alps_set_defaults(struct alps_data *priv)
> priv->y_max = 660;
> priv->x_bits = 23;
> priv->y_bits = 12;
> + priv->slot_number = 2;
> break;
> case ALPS_PROTO_V6:
> priv->hw_init = alps_hw_init_v6;
> @@ -1913,6 +2306,22 @@ static void alps_set_defaults(struct alps_data *priv)
> priv->nibble_commands = alps_v6_nibble_commands;
> priv->x_max = 2047;
> priv->y_max = 1535;
> + priv->slot_number = 2;
> + break;
> + case ALPS_PROTO_V7:
> + priv->hw_init = alps_hw_init_v7;
> + priv->process_packet = alps_process_packet_v7;
> + priv->decode_fields = alps_decode_packet_v7;
> + priv->set_abs_params = alps_set_abs_params_mt;
> + priv->nibble_commands = alps_v3_nibble_commands;
> + priv->addr_command = PSMOUSE_CMD_RESET_WRAP;
> + priv->x_max = 0xfff;
> + priv->y_max = 0x7ff;
> + priv->resting_zone_y_min = 0x654;
> + priv->byte0 = 0x48;
> + priv->mask0 = 0x48;
> + priv->flags = 0;
> + priv->slot_number = 2;
> break;
> }
> }
> @@ -1982,6 +2391,11 @@ static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
> return -EIO;
> else
> return 0;
> + } else if (ec[0] == 0x88 && (ec[1] & 0xf0) == 0xB0) {
> + priv->proto_version = ALPS_PROTO_V7;
> + alps_set_defaults(priv);
> +
> + return 0;
> } else if (ec[0] == 0x88 && ec[1] == 0x08) {
> priv->proto_version = ALPS_PROTO_V3;
> alps_set_defaults(priv);
> @@ -2045,7 +2459,7 @@ static void alps_set_abs_params_mt(struct alps_data *priv,
> struct input_dev *dev1)
> {
> set_bit(INPUT_PROP_SEMI_MT, dev1->propbit);
> - input_mt_init_slots(dev1, 2, 0);
> + input_mt_init_slots(dev1, priv->slot_number, 0);
> input_set_abs_params(dev1, ABS_MT_POSITION_X, 0, priv->x_max, 0, 0);
> input_set_abs_params(dev1, ABS_MT_POSITION_Y, 0, priv->y_max, 0, 0);
>
> diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
> index 03f88b6..5d2f9ea 100644
> --- a/drivers/input/mouse/alps.h
> +++ b/drivers/input/mouse/alps.h
> @@ -18,11 +18,36 @@
> #define ALPS_PROTO_V4 4
> #define ALPS_PROTO_V5 5
> #define ALPS_PROTO_V6 6
> +#define ALPS_PROTO_V7 7
> +
> +#define MAX_IMG_PT_NUM 5
> +#define V7_IMG_PT_NUM 2
> +
> +#define ZONE_NORMAL 0x01
> +#define ZONE_RESTING 0x02
> +#define ZONE_LEFT_BTN 0x04
> +#define ZONE_RIGHT_BTN 0x08
>
> #define DOLPHIN_COUNT_PER_ELECTRODE 64
> #define DOLPHIN_PROFILE_XOFFSET 8 /* x-electrode offset */
> #define DOLPHIN_PROFILE_YOFFSET 1 /* y-electrode offset */
>
> +/*
> + * enum V7_PACKET_ID - defines the packet type for V7
> + * V7_PACKET_ID_IDLE: There's no finger and no button activity.
> + * V7_PACKET_ID_TWO: There's one or two non-resting fingers on touchpad
> + * or there's button activities.
> + * V7_PACKET_ID_MULTI: There are at least three non-resting fingers.
> + * V7_PACKET_ID_NEW: The finger position in slot is not continues from
> + * previous packet.
> +*/
> +enum V7_PACKET_ID {
> + V7_PACKET_ID_IDLE,
> + V7_PACKET_ID_TWO,
> + V7_PACKET_ID_MULTI,
> + V7_PACKET_ID_NEW,
> +};
> +
> /**
> * struct alps_model_info - touchpad ID table
> * @signature: E7 response string to match.
> @@ -66,15 +91,7 @@ struct alps_nibble_commands {
> };
>
> /**
> - * struct alps_fields - decoded version of the report packet
> - * @x_map: Bitmap of active X positions for MT.
> - * @y_map: Bitmap of active Y positions for MT.
> - * @fingers: Number of fingers for MT.
> - * @x: X position for ST.
> - * @y: Y position for ST.
> - * @z: Z position for ST.
> - * @first_mp: Packet is the first of a multi-packet report.
> - * @is_mp: Packet is part of a multi-packet report.
> + * struct alps_btn - decoded version of the button status
> * @left: Left touchpad button is active.
> * @right: Right touchpad button is active.
> * @middle: Middle touchpad button is active.
> @@ -82,16 +99,7 @@ struct alps_nibble_commands {
> * @ts_right: Right trackstick button is active.
> * @ts_middle: Middle trackstick button is active.
> */
> -struct alps_fields {
> - unsigned int x_map;
> - unsigned int y_map;
> - unsigned int fingers;
> - unsigned int x;
> - unsigned int y;
> - unsigned int z;
> - unsigned int first_mp:1;
> - unsigned int is_mp:1;
> -
> +struct alps_btn {
> unsigned int left:1;
> unsigned int right:1;
> unsigned int middle:1;
> @@ -102,6 +110,69 @@ struct alps_fields {
> };
>
> /**
> + * struct alps_btn - decoded version of the X Y Z postion for ST.
> + * @x: X position for ST.
> + * @y: Y position for ST.
> + * @z: Z position for ST.
> + */
> +struct alps_abs_data {
> + unsigned int x;
> + unsigned int y;
> + unsigned int z;
> +};
> +
> +/**
> + * struct alps_fields - decoded version of the report packet
> + * @fingers: Number of fingers for MT.
> + * @pt: X Y Z postion for ST.
> + * @pt: X Y Z postion for image MT.
> + * @x_map: Bitmap of active X positions for MT.
> + * @y_map: Bitmap of active Y positions for MT.
> + * @first_mp: Packet is the first of a multi-packet report.
> + * @is_mp: Packet is part of a multi-packet report.
> + * @btn: Button activity status
> + */
> +struct alps_fields {
> + unsigned int fingers;
> + struct alps_abs_data pt;
> + struct alps_abs_data pt_img[MAX_IMG_PT_NUM];
> + unsigned int x_map;
> + unsigned int y_map;
> + unsigned int first_mp:1;
> + unsigned int is_mp:1;
> + struct alps_btn btn;
> +};
> +
> +/**
> + * struct v7_raw - data decoded from raw packet for V7.
> + * @pkt_id: An id that specifies the type of packet.
> + * @additional_fingers: Number of additional finger that is neighter included
> + * in pt slot nor reflected in rest_left and rest_right flag of data packet.
> + * @rest_left: There are fingers on left resting zone.
> + * @rest_right: There are fingers on right resting zone.
> + */
> +struct v7_raw {
> + unsigned char pkt_id;
> + unsigned int additional_fingers;
> + unsigned char rest_left;
> + unsigned char rest_right;
> +};
> +
> +/**
> + * struct alps_bl_pt_attr - generic attributes of touch points for buttonless device
> + * @zone: The part of touchpad that the touch point locates
> + * @is_counted: The touch point is not a resting finger.
> + * @is_init_pt_got: The touch down point is got.
> + * @init_pt: The X Y Z position of the touch down point.
> + */
> +struct alps_bl_pt_attr {
> + unsigned char zone;
> + unsigned char is_counted;
> + unsigned char is_init_pt_got;
> + struct alps_abs_data init_pt;
> +};
> +
> +/**
> * struct alps_data - private data structure for the ALPS driver
> * @dev2: "Relative" device used to report trackstick or mouse activity.
> * @phys: Physical path for the relative device.
> @@ -116,8 +187,10 @@ struct alps_fields {
> * @flags: Additional device capabilities (passthrough port, trackstick, etc.).
> * @x_max: Largest possible X position value.
> * @y_max: Largest possible Y position value.
> + * @resting_zone_y_min: Smallest Y postion value of the bottom resting zone.
> * @x_bits: Number of X bits in the MT bitmap.
> * @y_bits: Number of Y bits in the MT bitmap.
> + * @img_fingers: Number of image fingers.
> * @hw_init: Protocol-specific hardware init function.
> * @process_packet: Protocol-specific function to process a report packet.
> * @decode_fields: Protocol-specific function to read packet bitfields.
> @@ -132,6 +205,11 @@ struct alps_fields {
> * @fingers: Number of fingers from last MT report.
> * @quirks: Bitmap of ALPS_QUIRK_*.
> * @timer: Timer for flushing out the final report packet in the stream.
> + * @v7: Data decoded from raw packet for V7
> + * @phy_btn: Physical button is active.
> + * @prev_phy_btn: Physical button of previous packet is active.
> + * @pressed_btn_bits: Pressed positon of button zone
> + * @pt_attr: Generic attributes of touch points for buttonless device.
> */
> struct alps_data {
> struct input_dev *dev2;
> @@ -145,8 +223,10 @@ struct alps_data {
> unsigned char flags;
> int x_max;
> int y_max;
> + int resting_zone_y_min;
> int x_bits;
> int y_bits;
> + unsigned char slot_number;
>
> int (*hw_init)(struct psmouse *psmouse);
> void (*process_packet)(struct psmouse *psmouse);
> @@ -161,6 +241,15 @@ struct alps_data {
> int fingers;
> u8 quirks;
> struct timer_list timer;
> +
> + /* these are used for buttonless touchpad*/
> + union {
> + struct v7_raw v7;
> + } r;
> + unsigned char phy_btn;
> + unsigned char prev_phy_btn;
> + unsigned char pressed_btn_bits;
> + struct alps_bl_pt_attr pt_attr[MAX_IMG_PT_NUM];
> };
>
> #define ALPS_QUIRK_TRACKSTICK_BUTTONS 1 /* trakcstick buttons in trackstick packet */
> --
> 1.8.3.2
>
^ permalink raw reply
* [PATCH] input: Add commonly used event types
From: Alexander Shiyan @ 2014-01-13 17:13 UTC (permalink / raw)
To: linux-input; +Cc: Dmitry Torokhov, devicetree, linux-kernel, Alexander Shiyan
Patch adds commonly used event types (EV_KEY and EV_SW) to
devicetree bindings header for input subsystem.
Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
include/dt-bindings/input/input.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/dt-bindings/input/input.h b/include/dt-bindings/input/input.h
index 042e7b3..3a141d92 100644
--- a/include/dt-bindings/input/input.h
+++ b/include/dt-bindings/input/input.h
@@ -9,6 +9,9 @@
#ifndef _DT_BINDINGS_INPUT_INPUT_H
#define _DT_BINDINGS_INPUT_INPUT_H
+#define EV_KEY 0x01
+#define EV_SW 0x05
+
#define KEY_RESERVED 0
#define KEY_ESC 1
#define KEY_1 2
--
1.8.3.2
^ permalink raw reply related
* Re: [PATCH 2/2] Input: edt-ft5x06: Add DT support
From: Mark Rutland @ 2014-01-13 13:44 UTC (permalink / raw)
To: Lothar Waßmann
Cc: linux-input@vger.kernel.org, Rob Herring, Pawel Moll,
Ian Campbell, Kumar Gala, Rob Landley, Dmitry Torokhov,
grant.likely@linaro.org, Thierry Reding, Jonathan Cameron,
Shawn Guo, Silvio F, Guennadi Liakhovetski, Jingoo Han,
Fugang Duan, Sachin Kamat, devicetree@vger.kernel.org,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <1389608224-9975-2-git-send-email-LW@KARO-electronics.de>
On Mon, Jan 13, 2014 at 10:17:04AM +0000, Lothar Waßmann wrote:
> This patch allows the edt-ft5x06 multitouch panel driver to be
> configured via DT.
>
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
> .../bindings/input/touchscreen/edt-ft5x06.txt | 31 ++++
> drivers/input/touchscreen/edt-ft5x06.c | 145 +++++++++++++++----
> 2 files changed, 145 insertions(+), 31 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> new file mode 100644
> index 0000000..629dbdd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> @@ -0,0 +1,31 @@
> +* EDT FT5x06 Multiple Touch Controller
> +
> +Required properties:
> +- compatible: must be "edt,ft5x06"
> +- reg: i2c slave address
> +- interrupt-parent: the phandle for the interrupt controller
> +- interrupts: touch controller interrupt
> +
> +Optional properties:
> +- reset-gpios: the gpio pin to be used for resetting the controller
> +- wakeup-gpios: the gpio pin to be used for waking up the controller
> +
> + The following properties provide default values for the
> + corresponding parameters configurable via sysfs
> + (see Documentation/input/edt-ft5x06.txt)
The sysfs interface shouldn't matter for the binding, it's a Linux
detail. There's no reason for it to be mentioned in the binding.
> +- threshold: allows setting the "click"-threshold in the range from 20 to 80.
> +- gain: sensitivity (0..31) (lower value -> higher sensitivity)
> +- offset: edge compensation (0..31)
> +- report_rate: report rate (3..14)
s/_/-/ on property names please. Also, it may make sense to prefix these
as they're rather generic sounding names.
Could you elaborate on these a litle please? What units are each of
these in? Why does it make sense to have them in the dt?
> +
> +Example:
> +
> + edt_ft5x06@38 {
> + compatible = "edt,ft5x06";
> + reg = <0x38>;
> + interrupt-parent = <&gpio2>;
> + interrupts = <5 0>;
> + wakeup-gpios = <&gpio1 9 0>;
> + reset-gpios = <&gpio2 6 1>;
> + wakeup-gpios = <&gpio4 9 0>;
> + };
One of those wakeup-gpios properties should go.
[...]
> +#define EDT_GET_PROP(name, reg) { \
> + const u32 *prop = of_get_property(np, #name, NULL); \
> + if (prop) \
> + edt_ft5x06_register_write(tsdata, reg, be32_to_cpu(*prop)); \
> +}
Use of_property_read_u32, which does endianness conversion and property
length checking (which you're not doing). Sparse _will_ complain here
because you've not maintained the __be32 annotation.
> +static void edt_ft5x06_ts_get_dt_defaults(struct device_node *np,
> + struct edt_ft5x06_ts_data *tsdata)
> +{
> + EDT_GET_PROP(threshold, WORK_REGISTER_THRESHOLD);
> + EDT_GET_PROP(gain, WORK_REGISTER_GAIN);
> + EDT_GET_PROP(offset, WORK_REGISTER_OFFSET);
> + EDT_GET_PROP(report_rate, WORK_REGISTER_REPORT_RATE);
These should probably have sanity checks given you've described valid
ranges in the documentation.
[...]
> +static int edt_ft5x06_i2c_ts_probe_dt(struct device *dev,
> + struct edt_ft5x06_ts_data *tsdata)
> +{
> + int ret;
> + struct device_node *np = dev->of_node;
> + enum of_gpio_flags gpio_flags;
> +
> + if (!np)
> + return -ENODEV;
> +
> + /*
> + * irq_pin is not needed for DT setup.
> + * irq is associated via 'interrupts' property in DT
> + */
> + tsdata->irq_pin = -EINVAL;
> +
> + ret = of_get_named_gpio_flags(np, "reset-gpios", 0, &gpio_flags);
> + tsdata->reset_pin = ret;
> +
> + ret = of_get_named_gpio_flags(np, "wake-gpios", 0, &gpio_flags);
> + tsdata->wake_pin = ret;
Shouldn't that be "wakeup-gpios"?
Do you not need the value of flags? I'm not that familiar with the GPIO
subsystem, and it feels odd to rtequest the flags and throw them away.
Thanks,
Mark.
^ permalink raw reply
* [PATCH 2/2] Input: edt-ft5x06: Add DT support
From: Lothar Waßmann @ 2014-01-13 10:17 UTC (permalink / raw)
To: linux-input, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
Kumar Gala, Rob Landley, Dmitry Torokhov, Grant Likely,
Thierry Reding, Jonathan Cameron, Shawn Guo, Silvio F,
Guennadi Liakhovetski, Jingoo Han, Fugang Duan, Sachin Kamat,
devicetree, linux-doc, linux-kernel
Cc: Lothar Waßmann
In-Reply-To: <1389608224-9975-1-git-send-email-LW@KARO-electronics.de>
This patch allows the edt-ft5x06 multitouch panel driver to be
configured via DT.
Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
.../bindings/input/touchscreen/edt-ft5x06.txt | 31 ++++
drivers/input/touchscreen/edt-ft5x06.c | 145 +++++++++++++++----
2 files changed, 145 insertions(+), 31 deletions(-)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
new file mode 100644
index 0000000..629dbdd
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
@@ -0,0 +1,31 @@
+* EDT FT5x06 Multiple Touch Controller
+
+Required properties:
+- compatible: must be "edt,ft5x06"
+- reg: i2c slave address
+- interrupt-parent: the phandle for the interrupt controller
+- interrupts: touch controller interrupt
+
+Optional properties:
+- reset-gpios: the gpio pin to be used for resetting the controller
+- wakeup-gpios: the gpio pin to be used for waking up the controller
+
+ The following properties provide default values for the
+ corresponding parameters configurable via sysfs
+ (see Documentation/input/edt-ft5x06.txt)
+- threshold: allows setting the "click"-threshold in the range from 20 to 80.
+- gain: sensitivity (0..31) (lower value -> higher sensitivity)
+- offset: edge compensation (0..31)
+- report_rate: report rate (3..14)
+
+Example:
+
+ edt_ft5x06@38 {
+ compatible = "edt,ft5x06";
+ reg = <0x38>;
+ interrupt-parent = <&gpio2>;
+ interrupts = <5 0>;
+ wakeup-gpios = <&gpio1 9 0>;
+ reset-gpios = <&gpio2 6 1>;
+ wakeup-gpios = <&gpio4 9 0>;
+ };
diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index af0d68b..02dce42 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -33,6 +33,7 @@
#include <linux/debugfs.h>
#include <linux/slab.h>
#include <linux/gpio.h>
+#include <linux/of_gpio.h>
#include <linux/input/mt.h>
#include <linux/input/edt-ft5x06.h>
@@ -65,6 +66,10 @@ struct edt_ft5x06_ts_data {
u16 num_x;
u16 num_y;
+ int reset_pin;
+ int irq_pin;
+ int wake_pin;
+
#if defined(CONFIG_DEBUG_FS)
struct dentry *debug_dir;
u8 *raw_buffer;
@@ -617,24 +622,37 @@ edt_ft5x06_ts_teardown_debugfs(struct edt_ft5x06_ts_data *tsdata)
static int edt_ft5x06_ts_reset(struct i2c_client *client,
- int reset_pin)
+ struct edt_ft5x06_ts_data *tsdata)
{
int error;
- if (gpio_is_valid(reset_pin)) {
+ if (gpio_is_valid(tsdata->wake_pin)) {
+ error = gpio_request_one(tsdata->wake_pin, GPIOF_OUT_INIT_LOW,
+ "edt-ft5x06 wake");
+ if (error) {
+ dev_err(&client->dev,
+ "Failed to request GPIO %d as wake pin, error %d\n",
+ tsdata->wake_pin, error);
+ return error;
+ }
+
+ mdelay(5);
+ gpio_set_value(tsdata->wake_pin, 1);
+ }
+ if (gpio_is_valid(tsdata->reset_pin)) {
/* this pulls reset down, enabling the low active reset */
- error = gpio_request_one(reset_pin, GPIOF_OUT_INIT_LOW,
+ error = gpio_request_one(tsdata->reset_pin, GPIOF_OUT_INIT_LOW,
"edt-ft5x06 reset");
if (error) {
dev_err(&client->dev,
"Failed to request GPIO %d as reset pin, error %d\n",
- reset_pin, error);
+ tsdata->reset_pin, error);
return error;
}
- mdelay(50);
- gpio_set_value(reset_pin, 1);
- mdelay(100);
+ mdelay(5);
+ gpio_set_value(tsdata->reset_pin, 1);
+ mdelay(300);
}
return 0;
@@ -674,6 +692,21 @@ static int edt_ft5x06_ts_identify(struct i2c_client *client,
pdata->name <= edt_ft5x06_attr_##name.limit_high) \
edt_ft5x06_register_write(tsdata, reg, pdata->name)
+#define EDT_GET_PROP(name, reg) { \
+ const u32 *prop = of_get_property(np, #name, NULL); \
+ if (prop) \
+ edt_ft5x06_register_write(tsdata, reg, be32_to_cpu(*prop)); \
+}
+
+static void edt_ft5x06_ts_get_dt_defaults(struct device_node *np,
+ struct edt_ft5x06_ts_data *tsdata)
+{
+ EDT_GET_PROP(threshold, WORK_REGISTER_THRESHOLD);
+ EDT_GET_PROP(gain, WORK_REGISTER_GAIN);
+ EDT_GET_PROP(offset, WORK_REGISTER_OFFSET);
+ EDT_GET_PROP(report_rate, WORK_REGISTER_REPORT_RATE);
+}
+
static void
edt_ft5x06_ts_get_defaults(struct edt_ft5x06_ts_data *tsdata,
const struct edt_ft5x06_platform_data *pdata)
@@ -701,6 +734,39 @@ edt_ft5x06_ts_get_parameters(struct edt_ft5x06_ts_data *tsdata)
tsdata->num_y = edt_ft5x06_register_read(tsdata, WORK_REGISTER_NUM_Y);
}
+#ifdef CONFIG_OF
+static int edt_ft5x06_i2c_ts_probe_dt(struct device *dev,
+ struct edt_ft5x06_ts_data *tsdata)
+{
+ int ret;
+ struct device_node *np = dev->of_node;
+ enum of_gpio_flags gpio_flags;
+
+ if (!np)
+ return -ENODEV;
+
+ /*
+ * irq_pin is not needed for DT setup.
+ * irq is associated via 'interrupts' property in DT
+ */
+ tsdata->irq_pin = -EINVAL;
+
+ ret = of_get_named_gpio_flags(np, "reset-gpios", 0, &gpio_flags);
+ tsdata->reset_pin = ret;
+
+ ret = of_get_named_gpio_flags(np, "wake-gpios", 0, &gpio_flags);
+ tsdata->wake_pin = ret;
+
+ return 0;
+}
+#else
+static inline int edt_ft5x06_i2c_ts_probe_dt(struct device *dev,
+ struct edt_ft5x06_i2c_ts_data *tsdata)
+{
+ return -ENODEV;
+}
+#endif
+
static int edt_ft5x06_ts_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -713,30 +779,43 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
dev_dbg(&client->dev, "probing for EDT FT5x06 I2C\n");
+ tsdata = devm_kzalloc(&client->dev, sizeof(*tsdata), GFP_KERNEL);
+ if (!tsdata) {
+ dev_err(&client->dev, "failed to allocate driver data.\n");
+ return -ENOMEM;
+ }
+
if (!pdata) {
- dev_err(&client->dev, "no platform data?\n");
- return -EINVAL;
+ error = edt_ft5x06_i2c_ts_probe_dt(&client->dev, tsdata);
+ if (error) {
+ dev_err(&client->dev,
+ "DT probe failed and no platform data present\n");
+ return error;
+ }
+ } else {
+ tsdata->reset_pin = pdata->reset_pin;
+ tsdata->irq_pin = pdata->irq_pin;
+ tsdata->wake_pin = -EINVAL;
}
- error = edt_ft5x06_ts_reset(client, pdata->reset_pin);
+ error = edt_ft5x06_ts_reset(client, tsdata);
if (error)
return error;
- if (gpio_is_valid(pdata->irq_pin)) {
- error = gpio_request_one(pdata->irq_pin,
+ if (gpio_is_valid(tsdata->irq_pin)) {
+ error = gpio_request_one(tsdata->irq_pin,
GPIOF_IN, "edt-ft5x06 irq");
if (error) {
dev_err(&client->dev,
"Failed to request GPIO %d, error %d\n",
- pdata->irq_pin, error);
+ tsdata->irq_pin, error);
return error;
}
}
- tsdata = kzalloc(sizeof(*tsdata), GFP_KERNEL);
input = input_allocate_device();
- if (!tsdata || !input) {
- dev_err(&client->dev, "failed to allocate driver data.\n");
+ if (!input) {
+ dev_err(&client->dev, "failed to allocate input device.\n");
error = -ENOMEM;
goto err_free_mem;
}
@@ -752,7 +831,11 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
goto err_free_mem;
}
- edt_ft5x06_ts_get_defaults(tsdata, pdata);
+ if (!pdata)
+ edt_ft5x06_ts_get_dt_defaults(client->dev.of_node, tsdata);
+ else
+ edt_ft5x06_ts_get_defaults(tsdata, pdata);
+
edt_ft5x06_ts_get_parameters(tsdata);
dev_dbg(&client->dev,
@@ -802,8 +885,8 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
device_init_wakeup(&client->dev, 1);
dev_dbg(&client->dev,
- "EDT FT5x06 initialized: IRQ pin %d, Reset pin %d.\n",
- pdata->irq_pin, pdata->reset_pin);
+ "EDT FT5x06 initialized: IRQ %d, WAKE pin %d, Reset pin %d.\n",
+ client->irq, tsdata->wake_pin, tsdata->reset_pin);
return 0;
@@ -813,18 +896,18 @@ err_free_irq:
free_irq(client->irq, tsdata);
err_free_mem:
input_free_device(input);
- kfree(tsdata);
-
- if (gpio_is_valid(pdata->irq_pin))
- gpio_free(pdata->irq_pin);
+ if (gpio_is_valid(tsdata->irq_pin))
+ gpio_free(tsdata->irq_pin);
+ if (gpio_is_valid(tsdata->reset_pin))
+ gpio_free(tsdata->reset_pin);
+ if (gpio_is_valid(tsdata->wake_pin))
+ gpio_free(tsdata->wake_pin);
return error;
}
static int edt_ft5x06_ts_remove(struct i2c_client *client)
{
- const struct edt_ft5x06_platform_data *pdata =
- dev_get_platdata(&client->dev);
struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
edt_ft5x06_ts_teardown_debugfs(tsdata);
@@ -833,12 +916,12 @@ static int edt_ft5x06_ts_remove(struct i2c_client *client)
free_irq(client->irq, tsdata);
input_unregister_device(tsdata->input);
- if (gpio_is_valid(pdata->irq_pin))
- gpio_free(pdata->irq_pin);
- if (gpio_is_valid(pdata->reset_pin))
- gpio_free(pdata->reset_pin);
-
- kfree(tsdata);
+ if (gpio_is_valid(tsdata->irq_pin))
+ gpio_free(tsdata->irq_pin);
+ if (gpio_is_valid(tsdata->reset_pin))
+ gpio_free(tsdata->reset_pin);
+ if (gpio_is_valid(tsdata->wake_pin))
+ gpio_free(tsdata->wake_pin);
return 0;
}
--
1.7.2.5
^ permalink raw reply related
* [PATCH 1/2] DT: Add vendor prefix for Emerging Display Technologies
From: Lothar Waßmann @ 2014-01-13 10:17 UTC (permalink / raw)
To: linux-input-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala, Rob Landley,
Dmitry Torokhov, Grant Likely, Thierry Reding, Jonathan Cameron,
Shawn Guo, Silvio F, Guennadi Liakhovetski, Jingoo Han,
Fugang Duan, Sachin Kamat, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Lothar Waßmann
Signed-off-by: Lothar Waßmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
---
.../devicetree/bindings/vendor-prefixes.txt | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index b458760..49774c3 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -29,6 +29,7 @@ dallas Maxim Integrated Products (formerly Dallas Semiconductor)
davicom DAVICOM Semiconductor, Inc.
denx Denx Software Engineering
dmo Data Modul AG
+edt Emerging Display Technologies
emmicro EM Microelectronic
epson Seiko Epson Corp.
est ESTeem Wireless Modems
--
1.7.2.5
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH 1/4] Input: uinput: add full absinfo support
From: Dmitry Torokhov @ 2014-01-12 19:40 UTC (permalink / raw)
To: Peter Hutterer
Cc: David Herrmann, linux-input, Jiri Kosina, Benjamin Tissoires,
Antonio Ospite, linux-kernel, input-tools
In-Reply-To: <20131218222732.GA6315@yabbi.redhat.com>
On Thu, Dec 19, 2013 at 08:27:32AM +1000, Peter Hutterer wrote:
> On Tue, Dec 17, 2013 at 04:48:51PM +0100, David Herrmann wrote:
> > +
> > + user_dev2->version = UINPUT_VERSION;
> > + memcpy(user_dev2->name, user_dev->name, UINPUT_MAX_NAME_SIZE);
> > + memcpy(&user_dev2->id, &user_dev->id, sizeof(struct input_id));
>
> you copy the id bits one-by-one into the input_dev but you memcpy it here.
> is this intentional?
That should simply be:
user_dev2->id = user_dev->id;
and in othe rplace as well I think.
>
> > + user_dev2->ff_effects_max = user_dev->ff_effects_max;
> > +
> > + for (i = 0; i < ABS_CNT; ++i) {
> > + user_dev2->abs[i].value = 0;
> > + user_dev2->abs[i].maximum = user_dev->absmax[i];
> > + user_dev2->abs[i].minimum = user_dev->absmin[i];
> > + user_dev2->abs[i].fuzz = user_dev->absfuzz[i];
> > + user_dev2->abs[i].flat = user_dev->absflat[i];
> > + user_dev2->abs[i].resolution = 0;
> > + }
> > +
> > + retval = uinput_setup_device(udev, user_dev2, ABS_CNT);
> >
> > - exit:
> > kfree(user_dev);
> > - return retval;
> > + kfree(user_dev2);
> > +
> > + return retval ? retval : count;
> > +}
> > +
> > +static int uinput_setup_device2(struct uinput_device *udev,
> > + const char __user *buffer, size_t count)
> > +{
> > + struct uinput_user_dev2 *user_dev2;
> > + int retval;
> > + size_t off, abscnt, max;
> > +
> > + /* The first revision of "uinput_user_dev2" is bigger than
> > + * "uinput_user_dev" and growing. Disallow any smaller payloads. */
> > + if (count <= sizeof(struct uinput_user_dev))
> > + return -EINVAL;
> > +
> > + /* rough check to avoid huge kernel space allocations */
> > + max = ABS_CNT * sizeof(*user_dev2->abs) + sizeof(*user_dev2);
> > + if (count > max)
> > + return -EINVAL;
> > +
> > + user_dev2 = memdup_user(buffer, count);
> > + if (IS_ERR(user_dev2))
> > + return PTR_ERR(user_dev2);
> > +
> > + if (user_dev2->version > UINPUT_VERSION) {
> > + retval = -EINVAL;
> > + } else {
> > + off = offsetof(struct uinput_user_dev2, abs);
> > + abscnt = (count - off) / sizeof(*user_dev2->abs);
> > + retval = uinput_setup_device(udev, user_dev2, abscnt);
> > + }
> > +
>
> I really wish uinput would be a bit easier to debug than just returning
> -EINVAL when it's not happy. having said that, the only errno that would
> remotely make sense is -ERANGE for count > max and even that is a bit meh.
Maybe we should add a few dev_dbg() and rely on having dynamic debug to
activate logging on demand?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/4] Input: uinput: add full absinfo support
From: Dmitry Torokhov @ 2014-01-12 19:38 UTC (permalink / raw)
To: David Herrmann
Cc: linux-input, Jiri Kosina, Benjamin Tissoires, Peter Hutterer,
Antonio Ospite, linux-kernel, input-tools
In-Reply-To: <1387295334-1744-2-git-send-email-dh.herrmann@gmail.com>
On Tue, Dec 17, 2013 at 04:48:51PM +0100, David Herrmann wrote:
> +
> +struct uinput_user_dev2 {
> + __u8 version;
It does not make sense to have version u8 since we going to have padding
(1 byte I believe) padding between name and id.
> + char name[UINPUT_MAX_NAME_SIZE];
> + struct input_id id;
> + __u32 ff_effects_max;
> + struct input_absinfo abs[ABS_CNT];
> +};
> +
> #endif /* _UAPI__UINPUT_H_ */
> --
> 1.8.5.1
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/1] Drivers: input: serio:hyperv-keyoard: Handle 0xE1 prefix
From: Dmitry Torokhov @ 2014-01-12 19:14 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: olaf, gregkh, jasowang, linux-kernel, vojtech, linux-input, apw,
devel
In-Reply-To: <1389498110-4790-1-git-send-email-kys@microsoft.com>
On Sat, Jan 11, 2014 at 07:41:50PM -0800, K. Y. Srinivasan wrote:
> Handle the 0xE1 prefix.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Applied, thank you.
> ---
> drivers/input/serio/hyperv-keyboard.c | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c
> index 3a83c3c..6132619 100644
> --- a/drivers/input/serio/hyperv-keyboard.c
> +++ b/drivers/input/serio/hyperv-keyboard.c
> @@ -160,7 +160,9 @@ static void hv_kbd_on_receive(struct hv_device *hv_dev,
> if (info & IS_E0)
> serio_interrupt(kbd_dev->hv_serio,
> XTKBD_EMUL0, 0);
> -
> + if (info & IS_E1)
> + serio_interrupt(kbd_dev->hv_serio,
> + XTKBD_EMUL1, 0);
> scan_code = __le16_to_cpu(ks_msg->make_code);
> if (info & IS_BREAK)
> scan_code |= XTKBD_RELEASE;
> --
> 1.7.4.1
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH TRIVIAL] Input: logips2pp - Spelling s/reciver/receiver/
From: Dmitry Torokhov @ 2014-01-12 19:09 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Jiri Kosina, linux-input, Geert Uytterhoeven
In-Reply-To: <1389531708-6095-1-git-send-email-geert@linux-m68k.org>
On Sun, Jan 12, 2014 at 02:01:48PM +0100, Geert Uytterhoeven wrote:
> From: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
Applied, thank you.
> ---
> drivers/input/mouse/logips2pp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/mouse/logips2pp.c b/drivers/input/mouse/logips2pp.c
> index 84de2fc6acc1..136e222e2a16 100644
> --- a/drivers/input/mouse/logips2pp.c
> +++ b/drivers/input/mouse/logips2pp.c
> @@ -220,7 +220,7 @@ static const struct ps2pp_info *get_model_info(unsigned char model)
> { 61, PS2PP_KIND_MX, /* MX700 */
> PS2PP_WHEEL | PS2PP_SIDE_BTN | PS2PP_TASK_BTN |
> PS2PP_EXTRA_BTN | PS2PP_NAV_BTN },
> - { 66, PS2PP_KIND_MX, /* MX3100 reciver */
> + { 66, PS2PP_KIND_MX, /* MX3100 receiver */
> PS2PP_WHEEL | PS2PP_SIDE_BTN | PS2PP_TASK_BTN |
> PS2PP_EXTRA_BTN | PS2PP_NAV_BTN | PS2PP_HWHEEL },
> { 72, PS2PP_KIND_TRACKMAN, 0 }, /* T-CH11: TrackMan Marble */
> --
> 1.7.9.5
>
--
Dmitry
^ permalink raw reply
* [PATCH TRIVIAL] Input: logips2pp - Spelling s/reciver/receiver/
From: Geert Uytterhoeven @ 2014-01-12 13:01 UTC (permalink / raw)
To: Dmitry Torokhov, Jiri Kosina; +Cc: linux-input, Geert Uytterhoeven
From: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
Signed-off-by: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
---
drivers/input/mouse/logips2pp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/mouse/logips2pp.c b/drivers/input/mouse/logips2pp.c
index 84de2fc6acc1..136e222e2a16 100644
--- a/drivers/input/mouse/logips2pp.c
+++ b/drivers/input/mouse/logips2pp.c
@@ -220,7 +220,7 @@ static const struct ps2pp_info *get_model_info(unsigned char model)
{ 61, PS2PP_KIND_MX, /* MX700 */
PS2PP_WHEEL | PS2PP_SIDE_BTN | PS2PP_TASK_BTN |
PS2PP_EXTRA_BTN | PS2PP_NAV_BTN },
- { 66, PS2PP_KIND_MX, /* MX3100 reciver */
+ { 66, PS2PP_KIND_MX, /* MX3100 receiver */
PS2PP_WHEEL | PS2PP_SIDE_BTN | PS2PP_TASK_BTN |
PS2PP_EXTRA_BTN | PS2PP_NAV_BTN | PS2PP_HWHEEL },
{ 72, PS2PP_KIND_TRACKMAN, 0 }, /* T-CH11: TrackMan Marble */
--
1.7.9.5
^ permalink raw reply related
* [PATCH 1/1] Drivers: input: serio:hyperv-keyoard: Handle 0xE1 prefix
From: K. Y. Srinivasan @ 2014-01-12 3:41 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, dmitry.torokhov, linux-input,
vojtech, olaf, apw, jasowang
Handle the 0xE1 prefix.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/input/serio/hyperv-keyboard.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c
index 3a83c3c..6132619 100644
--- a/drivers/input/serio/hyperv-keyboard.c
+++ b/drivers/input/serio/hyperv-keyboard.c
@@ -160,7 +160,9 @@ static void hv_kbd_on_receive(struct hv_device *hv_dev,
if (info & IS_E0)
serio_interrupt(kbd_dev->hv_serio,
XTKBD_EMUL0, 0);
-
+ if (info & IS_E1)
+ serio_interrupt(kbd_dev->hv_serio,
+ XTKBD_EMUL1, 0);
scan_code = __le16_to_cpu(ks_msg->make_code);
if (info & IS_BREAK)
scan_code |= XTKBD_RELEASE;
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH v2 1/3] HID: sony: Add force-feedback support for the Dualshock 4
From: simon @ 2014-01-12 0:25 UTC (permalink / raw)
To: Frank Praznik; +Cc: linux-input, Jiri Kosina
> Adds the Dualshock 4 to the HID device list and enables force-feedback.
Adds a Dualshock 4 specific worker function since the Dualshock 4 needs
a
> different report than the Sixaxis.
> The right motor in the Dualshock 4 is variable so the full rumble value
is now passed to the worker function and clamped there if necessary.
Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
I was able to build this and the LED patch against the 3.13rc7 kernel
having first applied this sequence of patches:
https://patchwork.kernel.org/patch/3203761/
I can confirm that FF/LEDs appears to work OK with USB connected
Dualshock4, Dualshock3(SixAxis) and 3rd part wired PS3 controller.
I wasn't able to get Dualshock4 working over BT, but I think that the
problem is with my system - I am yet to figure out getting the HIDP
connection to work properly.
I can use the depricated 'HIDD --connect xxx' to have LEDs are listed in
'/sys/class/leds', but they don't change when instructed. They stay solid
white.
Cheers,
Simon
Tested-by: Simon Wood <simon@mungewell.org>
^ permalink raw reply
* Re: [PATCH] ARM: Kirkwood: Add DT description of QNAP 419
From: Andrew Lunn @ 2014-01-11 21:04 UTC (permalink / raw)
To: Ian Campbell, devicetree, linux-input
Cc: Andrew Lunn, Jason Cooper, Sebastian Hesselbarth, Gregory Clement,
linux ARM, tbm
In-Reply-To: <1389174517.12612.87.camel@kazak.uk.xensource.com>
On Wed, Jan 08, 2014 at 09:48:37AM +0000, Ian Campbell wrote:
> On Mon, 2014-01-06 at 23:49 +0100, Andrew Lunn wrote:
Hi Ian
I added in the devicetree list and the input list and added my
thoughts below.
> The other issue I spotted is
> that /dev/input/by-path/platform-gpio-keys-event has
> become /dev/input/by-path/platform-gpio_keys.3-event. Is it considered
> valid for a by-path name to change? In particular the 3 here is
> apparently the node depth in the DTB, which doesn't make much logical
> sense as a "path" in this context I don't think (I expect it to be some
> sort of path through the hardware buses, perhaps my expectation is
> wrong?).
The ts41x-setup.c board file creates the gpio keys platform device
using the following structure:
static struct platform_device qnap_ts41x_button_device = {
.name = "gpio-keys",
.id = -1,
.num_resources = 0,
.dev = {
.platform_data = &qnap_ts41x_button_data,
}
};
The id of -1 causes platform_device_add() to set the device name to
plain "gpio-keys".
When using DT, the device name is created by the function
of_device_make_bus_id(). It has the following comment:
* This routine will first try using either the dcr-reg or the reg property
* value to derive a unique name. As a last resort it will use the node
* name followed by a unique number.
Since the gpio_keys node does not have a reg properties, it gets a
unique number appended to it. We end up with the device name
"gpio_keys.3"
So as it stands, it does not appear i can make the DT system use the
same device name as a board system.
But i'm also a little bit concerned by the "unique number" and this
ending up in /dev/input/by-path/platform-gpio_keys.3-event. Is this
path supposed to be stable? This unique number is not stable. An
unwitting change to the DT could cause its value to change. Do we need
to make it stable?
Andrew
^ permalink raw reply
* [PATCH v2 3/3] HID: sony: Rename worker function
From: Frank Praznik @ 2014-01-11 20:13 UTC (permalink / raw)
To: linux-input; +Cc: Jiri Kosina
Rename sony_state_worker to sixaxis_state_worker since the function is now
sixaxis specific.
Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
Apply against jikos/hid.git/for-3.14/sony
drivers/hid/hid-sony.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 79e0d58..1dfed23 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -632,7 +632,7 @@ error_leds:
return ret;
}
-static void sony_state_worker(struct work_struct *work)
+static void sixaxis_state_worker(struct work_struct *work)
{
struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
unsigned char buf[] = {
@@ -771,7 +771,7 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
ret = sixaxis_set_operational_usb(hdev);
- INIT_WORK(&sc->state_worker, sony_state_worker);
+ INIT_WORK(&sc->state_worker, sixaxis_state_worker);
}
else if (sc->quirks & SIXAXIS_CONTROLLER_BT)
ret = sixaxis_set_operational_bt(hdev);
--
1.8.3.2
^ permalink raw reply related
* [PATCH v2 2/3] HID: sony: Add LED controls for the Dualshock 4
From: Frank Praznik @ 2014-01-11 20:13 UTC (permalink / raw)
To: linux-input; +Cc: Jiri Kosina
Add LED lightbar controls for the Dualshock 4.
The Dualshock 4 light bar has 3 separate RGB LEDs that can range in
brightness from 0 to 255 so a full byte is now needed to store each LED's
state
Changed the module to support an arbitrary number of LEDs instead of being
hardcoded to 4.
Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
Apply against jikos/hid.git/for-3.14/sony
drivers/hid/hid-sony.c | 77 ++++++++++++++++++++++++++++++++------------------
1 file changed, 50 insertions(+), 27 deletions(-)
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 8020d10..79e0d58 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -40,7 +40,9 @@
#define PS3REMOTE BIT(4)
#define DUALSHOCK4_CONTROLLER BIT(5)
-#define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER_USB | BUZZ_CONTROLLER)
+#define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER_USB | BUZZ_CONTROLLER | DUALSHOCK4_CONTROLLER)
+
+#define MAX_LEDS 4
static const u8 sixaxis_rdesc_fixup[] = {
0x95, 0x13, 0x09, 0x01, 0x81, 0x02, 0x95, 0x0C,
@@ -227,7 +229,7 @@ static const unsigned int buzz_keymap[] = {
struct sony_sc {
struct hid_device *hdev;
- struct led_classdev *leds[4];
+ struct led_classdev *leds[MAX_LEDS];
unsigned long quirks;
struct work_struct state_worker;
@@ -236,7 +238,8 @@ struct sony_sc {
__u8 right;
#endif
- __u8 led_state;
+ __u8 led_state[MAX_LEDS];
+ __u8 led_count;
};
static __u8 *ps3remote_fixup(struct hid_device *hdev, __u8 *rdesc,
@@ -447,7 +450,7 @@ static int sixaxis_set_operational_bt(struct hid_device *hdev)
return hdev->hid_output_raw_report(hdev, buf, sizeof(buf), HID_FEATURE_REPORT);
}
-static void buzz_set_leds(struct hid_device *hdev, int leds)
+static void buzz_set_leds(struct hid_device *hdev, const __u8 *leds)
{
struct list_head *report_list =
&hdev->report_enum[HID_OUTPUT_REPORT].report_list;
@@ -456,23 +459,28 @@ static void buzz_set_leds(struct hid_device *hdev, int leds)
__s32 *value = report->field[0]->value;
value[0] = 0x00;
- value[1] = (leds & 1) ? 0xff : 0x00;
- value[2] = (leds & 2) ? 0xff : 0x00;
- value[3] = (leds & 4) ? 0xff : 0x00;
- value[4] = (leds & 8) ? 0xff : 0x00;
+ value[1] = leds[0] ? 0xff : 0x00;
+ value[2] = leds[1] ? 0xff : 0x00;
+ value[3] = leds[2] ? 0xff : 0x00;
+ value[4] = leds[3] ? 0xff : 0x00;
value[5] = 0x00;
value[6] = 0x00;
hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
}
-static void sony_set_leds(struct hid_device *hdev, __u8 leds)
+static void sony_set_leds(struct hid_device *hdev, const __u8 *leds, int count)
{
struct sony_sc *drv_data = hid_get_drvdata(hdev);
+ int n;
- if (drv_data->quirks & BUZZ_CONTROLLER) {
+ BUG_ON(count > MAX_LEDS);
+
+ if (drv_data->quirks & BUZZ_CONTROLLER && count == 4) {
buzz_set_leds(hdev, leds);
- } else if (drv_data->quirks & SIXAXIS_CONTROLLER_USB) {
- drv_data->led_state = leds;
+ } else if ((drv_data->quirks & SIXAXIS_CONTROLLER_USB) ||
+ (drv_data->quirks & DUALSHOCK4_CONTROLLER)) {
+ for (n = 0; n < count; n++)
+ drv_data->led_state[n] = leds[n];
schedule_work(&drv_data->state_worker);
}
}
@@ -492,15 +500,11 @@ static void sony_led_set_brightness(struct led_classdev *led,
return;
}
- for (n = 0; n < 4; n++) {
+ for (n = 0; n < drv_data->led_count; n++) {
if (led == drv_data->leds[n]) {
- int on = !!(drv_data->led_state & (1 << n));
- if (value == LED_OFF && on) {
- drv_data->led_state &= ~(1 << n);
- sony_set_leds(hdev, drv_data->led_state);
- } else if (value != LED_OFF && !on) {
- drv_data->led_state |= (1 << n);
- sony_set_leds(hdev, drv_data->led_state);
+ if (value != drv_data->led_state[n]) {
+ drv_data->led_state[n] = value;
+ sony_set_leds(hdev, drv_data->led_state, drv_data->led_count);
}
break;
}
@@ -522,9 +526,9 @@ static enum led_brightness sony_led_get_brightness(struct led_classdev *led)
return LED_OFF;
}
- for (n = 0; n < 4; n++) {
+ for (n = 0; n < drv_data->led_count; n++) {
if (led == drv_data->leds[n]) {
- on = !!(drv_data->led_state & (1 << n));
+ on = !!(drv_data->led_state[n]);
break;
}
}
@@ -541,7 +545,7 @@ static void sony_leds_remove(struct hid_device *hdev)
drv_data = hid_get_drvdata(hdev);
BUG_ON(!(drv_data->quirks & SONY_LED_SUPPORT));
- for (n = 0; n < 4; n++) {
+ for (n = 0; n < drv_data->led_count; n++) {
led = drv_data->leds[n];
drv_data->leds[n] = NULL;
if (!led)
@@ -549,17 +553,21 @@ static void sony_leds_remove(struct hid_device *hdev)
led_classdev_unregister(led);
kfree(led);
}
+
+ drv_data->led_count = 0;
}
static int sony_leds_init(struct hid_device *hdev)
{
struct sony_sc *drv_data;
int n, ret = 0;
+ int max_brightness;
struct led_classdev *led;
size_t name_sz;
char *name;
size_t name_len;
const char *name_fmt;
+ static const __u8 initial_values[MAX_LEDS] = { 0x00, 0x00, 0x00, 0x00 };
drv_data = hid_get_drvdata(hdev);
BUG_ON(!(drv_data->quirks & SONY_LED_SUPPORT));
@@ -575,14 +583,22 @@ static int sony_leds_init(struct hid_device *hdev)
name_fmt = "%s::sony%d";
}
+ if (drv_data->quirks & DUALSHOCK4_CONTROLLER) {
+ drv_data->led_count = 3;
+ max_brightness = 255;
+ } else {
+ drv_data->led_count = 4;
+ max_brightness = 1;
+ }
+
/* Clear LEDs as we have no way of reading their initial state. This is
* only relevant if the driver is loaded after somebody actively set the
* LEDs to on */
- sony_set_leds(hdev, 0x00);
+ sony_set_leds(hdev, initial_values, drv_data->led_count);
name_sz = strlen(dev_name(&hdev->dev)) + name_len + 1;
- for (n = 0; n < 4; n++) {
+ for (n = 0; n < drv_data->led_count; n++) {
led = kzalloc(sizeof(struct led_classdev) + name_sz, GFP_KERNEL);
if (!led) {
hid_err(hdev, "Couldn't allocate memory for LED %d\n", n);
@@ -594,7 +610,7 @@ static int sony_leds_init(struct hid_device *hdev)
snprintf(name, name_sz, name_fmt, dev_name(&hdev->dev), n + 1);
led->name = name;
led->brightness = 0;
- led->max_brightness = 1;
+ led->max_brightness = max_brightness;
led->brightness_get = sony_led_get_brightness;
led->brightness_set = sony_led_set_brightness;
@@ -635,7 +651,10 @@ static void sony_state_worker(struct work_struct *work)
buf[5] = sc->left;
#endif
- buf[10] |= (sc->led_state & 0xf) << 1;
+ buf[10] |= sc->led_state[0] << 1;
+ buf[10] |= sc->led_state[1] << 2;
+ buf[10] |= sc->led_state[2] << 3;
+ buf[10] |= sc->led_state[3] << 4;
sc->hdev->hid_output_raw_report(sc->hdev, buf, sizeof(buf),
HID_OUTPUT_REPORT);
@@ -660,6 +679,10 @@ static void dualshock4_state_worker(struct work_struct *work)
buf[5] = sc->left;
#endif
+ buf[6] = sc->led_state[0];
+ buf[7] = sc->led_state[1];
+ buf[8] = sc->led_state[2];
+
sc->hdev->hid_output_raw_report(sc->hdev, buf, sizeof(buf),
HID_OUTPUT_REPORT);
}
--
1.8.3.2
^ permalink raw reply related
* [PATCH v2 1/3] HID: sony: Add force-feedback support for the Dualshock 4
From: Frank Praznik @ 2014-01-11 20:12 UTC (permalink / raw)
To: linux-input; +Cc: Jiri Kosina
Adds the Dualshock 4 to the HID device list and enables force-feedback.
Adds a Dualshock 4 specific worker function since the Dualshock 4 needs a
different report than the Sixaxis.
The right motor in the Dualshock 4 is variable so the full rumble value
is now passed to the worker function and clamped there if necessary.
Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
Apply against jikos/hid.git/for-3.14/sony
drivers/hid/hid-core.c | 2 ++
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-sony.c | 39 ++++++++++++++++++++++++++++++++++++---
3 files changed, 39 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 957d35b..70cc468 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1831,6 +1831,8 @@ static const struct hid_device_id hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER) },
{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_NAVIGATION_CONTROLLER) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER) },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER) },
{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_VAIO_VGX_MOUSE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_VAIO_VGP_MOUSE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_STEELSERIES, USB_DEVICE_ID_STEELSERIES_SRWS1) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 60336f06..ce24459 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -766,6 +766,7 @@
#define USB_DEVICE_ID_SONY_VAIO_VGP_MOUSE 0x0374
#define USB_DEVICE_ID_SONY_PS3_BDREMOTE 0x0306
#define USB_DEVICE_ID_SONY_PS3_CONTROLLER 0x0268
+#define USB_DEVICE_ID_SONY_PS4_CONTROLLER 0x05c4
#define USB_DEVICE_ID_SONY_NAVIGATION_CONTROLLER 0x042f
#define USB_DEVICE_ID_SONY_BUZZ_CONTROLLER 0x0002
#define USB_DEVICE_ID_SONY_WIRELESS_BUZZ_CONTROLLER 0x1000
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index f57ab5e..8020d10 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -38,6 +38,7 @@
#define SIXAXIS_CONTROLLER_BT BIT(2)
#define BUZZ_CONTROLLER BIT(3)
#define PS3REMOTE BIT(4)
+#define DUALSHOCK4_CONTROLLER BIT(5)
#define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER_USB | BUZZ_CONTROLLER)
@@ -630,7 +631,7 @@ static void sony_state_worker(struct work_struct *work)
};
#ifdef CONFIG_SONY_FF
- buf[3] = sc->right;
+ buf[3] = sc->right ? 1 : 0;
buf[5] = sc->left;
#endif
@@ -640,6 +641,29 @@ static void sony_state_worker(struct work_struct *work)
HID_OUTPUT_REPORT);
}
+static void dualshock4_state_worker(struct work_struct *work)
+{
+ struct sony_sc *sc = container_of(work, struct sony_sc, state_worker);
+ unsigned char buf[] = {
+ 0x05,
+ 0x03, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00,
+ };
+
+#ifdef CONFIG_SONY_FF
+ buf[4] = sc->right;
+ buf[5] = sc->left;
+#endif
+
+ sc->hdev->hid_output_raw_report(sc->hdev, buf, sizeof(buf),
+ HID_OUTPUT_REPORT);
+}
+
#ifdef CONFIG_SONY_FF
static int sony_play_effect(struct input_dev *dev, void *data,
struct ff_effect *effect)
@@ -651,7 +675,7 @@ static int sony_play_effect(struct input_dev *dev, void *data,
return 0;
sc->left = effect->u.rumble.strong_magnitude / 256;
- sc->right = effect->u.rumble.weak_magnitude ? 1 : 0;
+ sc->right = effect->u.rumble.weak_magnitude / 256;
schedule_work(&sc->state_worker);
return 0;
@@ -728,8 +752,12 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
}
else if (sc->quirks & SIXAXIS_CONTROLLER_BT)
ret = sixaxis_set_operational_bt(hdev);
- else
+ else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
ret = 0;
+ INIT_WORK(&sc->state_worker, dualshock4_state_worker);
+ } else {
+ ret = 0;
+ }
if (ret < 0)
goto err_stop;
@@ -787,6 +815,11 @@ static const struct hid_device_id sony_devices[] = {
/* Logitech Harmony Adapter for PS3 */
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_HARMONY_PS3),
.driver_data = PS3REMOTE },
+ /* Sony Dualshock 4 controllers for PS4 */
+ { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER),
+ .driver_data = DUALSHOCK4_CONTROLLER },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS4_CONTROLLER),
+ .driver_data = DUALSHOCK4_CONTROLLER },
{ }
};
MODULE_DEVICE_TABLE(hid, sony_devices);
--
1.8.3.2
^ permalink raw reply related
* Re: [PATCH 4/4] Input: synaptics-rmi4 - switch to using i2c_transfer()
From: Christopher Heiny @ 2014-01-10 23:29 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Linux Input, Linux Kernel
In-Reply-To: <1389339867-8399-4-git-send-email-dmitry.torokhov@gmail.com>
On 01/09/2014 11:44 PM, Dmitry Torokhov wrote:
> Instead of using 2 separate transactions when reading from the device let's
> use i2c_transfer. Because we now have single point of failure I had to
> change how we collect statistics. I elected to drop control data from the
> stats and only track number of bytes read/written for the device data.
>
> Also, since we are not prepared to deal with short reads and writes change
> read_block_data and write_block_data to indicate error if we detect short
> transfers.
We tried this change once before a couple of years ago, but the
conversion was unsuccessful on some older platforms. I've tested it on
some more current platforms, though, and it works there. The old
platforms are running 2.6.xx series kernels, and don't look likely ever
to be updated, So....
Acked-by: Christopher Heiny <cheiny@synaptics.com>
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/rmi4/rmi_i2c.c | 71 ++++++++++++++++++++++++--------------------
> 1 file changed, 39 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index c176218..51f5bc8 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -57,22 +57,17 @@ struct rmi_i2c_xport {
> */
> static int rmi_set_page(struct rmi_i2c_xport *rmi_i2c, u8 page)
> {
> - struct rmi_transport_dev *xport = &rmi_i2c->xport;
> struct i2c_client *client = rmi_i2c->client;
> u8 txbuf[2] = {RMI_PAGE_SELECT_REGISTER, page};
> int retval;
>
> - dev_dbg(&client->dev, "writes 3 bytes: %02x %02x\n",
> - txbuf[0], txbuf[1]);
> - xport->stats.tx_count++;
> - xport->stats.tx_bytes += sizeof(txbuf);
> retval = i2c_master_send(client, txbuf, sizeof(txbuf));
> if (retval != sizeof(txbuf)) {
> - xport->stats.tx_errs++;
> dev_err(&client->dev,
> "%s: set page failed: %d.", __func__, retval);
> return (retval < 0) ? retval : -EIO;
> }
> +
> rmi_i2c->page = page;
> return 0;
> }
> @@ -107,22 +102,27 @@ static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
>
> if (RMI_I2C_PAGE(addr) != rmi_i2c->page) {
> retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr));
> - if (retval < 0)
> + if (retval)
> goto exit;
> }
>
> + retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size);
> + if (retval == tx_size)
> + retval = 0;
> + else if (retval >= 0)
> + retval = -EIO;
> +
> +exit:
> dev_dbg(&client->dev,
> - "writes %zd bytes at %#06x: %*ph\n", len, addr, (int)len, buf);
> + "write %zd bytes at %#06x: %d (%*ph)\n",
> + len, addr, retval, (int)len, buf);
>
> xport->stats.tx_count++;
> - xport->stats.tx_bytes += tx_size;
> - retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size);
> - if (retval < 0)
> + if (retval)
> xport->stats.tx_errs++;
> else
> - retval--; /* don't count the address byte */
> + xport->stats.tx_bytes += len;
>
> -exit:
> mutex_unlock(&rmi_i2c->page_mutex);
> return retval;
> }
> @@ -133,40 +133,47 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
> struct rmi_i2c_xport *rmi_i2c =
> container_of(xport, struct rmi_i2c_xport, xport);
> struct i2c_client *client = rmi_i2c->client;
> - u8 txbuf[1] = {addr & 0xff};
> + u8 addr_offset = addr & 0xff;
> int retval;
> + struct i2c_msg msgs[] = {
> + {
> + .addr = client->addr,
> + .len = sizeof(addr_offset),
> + .buf = &addr_offset,
> + },
> + {
> + .addr = client->addr,
> + .flags = I2C_M_RD,
> + .len = len,
> + .buf = buf,
> + },
> + };
>
> mutex_lock(&rmi_i2c->page_mutex);
>
> if (RMI_I2C_PAGE(addr) != rmi_i2c->page) {
> retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr));
> - if (retval < 0)
> + if (retval)
> goto exit;
> }
>
> - dev_dbg(&client->dev, "writes 1 bytes: %02x\n", txbuf[0]);
> + retval = i2c_transfer(client->adapter, msgs, sizeof(msgs));
> + if (retval == sizeof(msgs))
> + retval = 0; /* success */
> + else if (retval >= 0)
> + retval = -EIO;
>
> - xport->stats.tx_count++;
> - xport->stats.tx_bytes += sizeof(txbuf);
> - retval = i2c_master_send(client, txbuf, sizeof(txbuf));
> - if (retval != sizeof(txbuf)) {
> - xport->stats.tx_errs++;
> - retval = (retval < 0) ? retval : -EIO;
> - goto exit;
> - }
> +exit:
> + dev_dbg(&client->dev,
> + "read %zd bytes at %#06x: %d (%*ph)\n",
> + len, addr, retval, (int)len, buf);
>
> xport->stats.rx_count++;
> - xport->stats.rx_bytes += len;
> -
> - retval = i2c_master_recv(client, buf, len);
> - if (retval < 0)
> + if (retval)
> xport->stats.rx_errs++;
> else
> - dev_dbg(&client->dev,
> - "read %zd bytes at %#06x: %*ph\n",
> - len, addr, (int)len, buf);
> + xport->stats.rx_bytes += len;
>
> -exit:
> mutex_unlock(&rmi_i2c->page_mutex);
> return retval;
> }
>
--
Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated
^ permalink raw reply
* Re: [PATCH 3/4] Input: synaptics-rmi4 - fix I2C functionality check
From: Christopher Heiny @ 2014-01-10 23:25 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Linux Input, Linux Kernel
In-Reply-To: <1389339867-8399-3-git-send-email-dmitry.torokhov@gmail.com>
On 01/09/2014 11:44 PM, Dmitry Torokhov wrote:
> When adapter does not support required functionality (I2C_FUNC_I2C) we were
> returning 0 to the upper layers, making them believe that device bound
> successfully.
Acked-by: Christopher Heiny <cheiny@synaptics.com>
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/rmi4/rmi_i2c.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index cdc8527..c176218 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -193,11 +193,10 @@ static int rmi_i2c_probe(struct i2c_client *client,
> pdata->sensor_name ? pdata->sensor_name : "-no name-",
> client->addr, pdata->attn_gpio);
>
> - retval = i2c_check_functionality(client->adapter, I2C_FUNC_I2C);
> - if (!retval) {
> - dev_err(&client->dev, "i2c_check_functionality error %d.\n",
> - retval);
> - return retval;
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> + dev_err(&client->dev,
> + "adapter does not support required functionality.\n");
> + return -ENODEV;
> }
>
> if (pdata->gpio_config) {
>
--
Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated
^ permalink raw reply
* Re: [PATCH 2/4] Input: synaptics-rmi4 - rework transport device allocation
From: Christopher Heiny @ 2014-01-10 23:25 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Linux Input, Linux Kernel
In-Reply-To: <1389339867-8399-2-git-send-email-dmitry.torokhov@gmail.com>
On 01/09/2014 11:44 PM, Dmitry Torokhov wrote:
> Instead of allocating common and private part of transport device
> separately make private wrap common part and get rid of private data
> pointer in the transport device.
>
> Also rename rmi_i2c_data -> rmi_i2c_xport and data -> rmi_i2c.
Acked-by: Christopher Heiny <cheiny@synaptics.com>
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/rmi4/rmi_bus.h | 3 --
> drivers/input/rmi4/rmi_i2c.c | 112 +++++++++++++++++++++----------------------
> 2 files changed, 56 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
> index ccf26dc..decb479 100644
> --- a/drivers/input/rmi4/rmi_bus.h
> +++ b/drivers/input/rmi4/rmi_bus.h
> @@ -165,7 +165,6 @@ struct rmi_transport_stats {
> * default irq_thread implementation.
> * @hard_irq: if not NULL, the sensor driver will use this for the hard IRQ
> * handling
> - * @data: Private data pointer
> * @proto_name: name of the transport protocol (SPI, i2c, etc)
> * @ops: pointer to transport operations implementation
> * @stats: transport statistics
> @@ -181,8 +180,6 @@ struct rmi_transport_dev {
> irqreturn_t (*irq_thread)(int irq, void *p);
> irqreturn_t (*hard_irq)(int irq, void *p);
>
> - void *data;
> -
> const char *proto_name;
> const struct rmi_transport_ops *ops;
> struct rmi_transport_stats stats;
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index 40badf3..cdc8527 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -17,22 +17,25 @@
> #define BUFFER_SIZE_INCREMENT 32
>
> /**
> - * struct rmi_i2c_data - stores information for i2c communication
> + * struct rmi_i2c_xport - stores information for i2c communication
> + *
> + * @xport: The transport interface structure
> *
> * @page_mutex: Locks current page to avoid changing pages in unexpected ways.
> * @page: Keeps track of the current virtual page
> - * @xport: Pointer to the transport interface
> *
> * @tx_buf: Buffer used for transmitting data to the sensor over i2c.
> * @tx_buf_size: Size of the buffer
> */
> -struct rmi_i2c_data {
> +struct rmi_i2c_xport {
> + struct rmi_transport_dev xport;
> + struct i2c_client *client;
> +
> struct mutex page_mutex;
> int page;
> - struct rmi_transport_dev *xport;
>
> u8 *tx_buf;
> - int tx_buf_size;
> + size_t tx_buf_size;
> };
>
> #define RMI_PAGE_SELECT_REGISTER 0xff
> @@ -52,10 +55,10 @@ struct rmi_i2c_data {
> *
> * Returns zero on success, non-zero on failure.
> */
> -static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
> +static int rmi_set_page(struct rmi_i2c_xport *rmi_i2c, u8 page)
> {
> - struct i2c_client *client = to_i2c_client(xport->dev);
> - struct rmi_i2c_data *data = xport->data;
> + struct rmi_transport_dev *xport = &rmi_i2c->xport;
> + struct i2c_client *client = rmi_i2c->client;
> u8 txbuf[2] = {RMI_PAGE_SELECT_REGISTER, page};
> int retval;
>
> @@ -70,37 +73,40 @@ static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
> "%s: set page failed: %d.", __func__, retval);
> return (retval < 0) ? retval : -EIO;
> }
> - data->page = page;
> + rmi_i2c->page = page;
> return 0;
> }
>
> static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
> const void *buf, size_t len)
> {
> - struct i2c_client *client = to_i2c_client(xport->dev);
> - struct rmi_i2c_data *data = xport->data;
> + struct rmi_i2c_xport *rmi_i2c =
> + container_of(xport, struct rmi_i2c_xport, xport);
> + struct i2c_client *client = rmi_i2c->client;
> size_t tx_size = len + 1;
> int retval;
>
> - mutex_lock(&data->page_mutex);
> -
> - if (!data->tx_buf || data->tx_buf_size < tx_size) {
> - if (data->tx_buf)
> - devm_kfree(&client->dev, data->tx_buf);
> - data->tx_buf_size = tx_size + BUFFER_SIZE_INCREMENT;
> - data->tx_buf = devm_kzalloc(&client->dev, data->tx_buf_size,
> - GFP_KERNEL);
> - if (!data->tx_buf) {
> - data->tx_buf_size = 0;
> + mutex_lock(&rmi_i2c->page_mutex);
> +
> + if (!rmi_i2c->tx_buf || rmi_i2c->tx_buf_size < tx_size) {
> + if (rmi_i2c->tx_buf)
> + devm_kfree(&client->dev, rmi_i2c->tx_buf);
> + rmi_i2c->tx_buf_size = tx_size + BUFFER_SIZE_INCREMENT;
> + rmi_i2c->tx_buf = devm_kzalloc(&client->dev,
> + rmi_i2c->tx_buf_size,
> + GFP_KERNEL);
> + if (!rmi_i2c->tx_buf) {
> + rmi_i2c->tx_buf_size = 0;
> retval = -ENOMEM;
> goto exit;
> }
> }
> - data->tx_buf[0] = addr & 0xff;
> - memcpy(data->tx_buf + 1, buf, len);
>
> - if (RMI_I2C_PAGE(addr) != data->page) {
> - retval = rmi_set_page(xport, RMI_I2C_PAGE(addr));
> + rmi_i2c->tx_buf[0] = addr & 0xff;
> + memcpy(rmi_i2c->tx_buf + 1, buf, len);
> +
> + if (RMI_I2C_PAGE(addr) != rmi_i2c->page) {
> + retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr));
> if (retval < 0)
> goto exit;
> }
> @@ -110,29 +116,30 @@ static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
>
> xport->stats.tx_count++;
> xport->stats.tx_bytes += tx_size;
> - retval = i2c_master_send(client, data->tx_buf, tx_size);
> + retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size);
> if (retval < 0)
> xport->stats.tx_errs++;
> else
> retval--; /* don't count the address byte */
>
> exit:
> - mutex_unlock(&data->page_mutex);
> + mutex_unlock(&rmi_i2c->page_mutex);
> return retval;
> }
>
> static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
> void *buf, size_t len)
> {
> - struct i2c_client *client = to_i2c_client(xport->dev);
> - struct rmi_i2c_data *data = xport->data;
> + struct rmi_i2c_xport *rmi_i2c =
> + container_of(xport, struct rmi_i2c_xport, xport);
> + struct i2c_client *client = rmi_i2c->client;
> u8 txbuf[1] = {addr & 0xff};
> int retval;
>
> - mutex_lock(&data->page_mutex);
> + mutex_lock(&rmi_i2c->page_mutex);
>
> - if (RMI_I2C_PAGE(addr) != data->page) {
> - retval = rmi_set_page(xport, RMI_I2C_PAGE(addr));
> + if (RMI_I2C_PAGE(addr) != rmi_i2c->page) {
> + retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr));
> if (retval < 0)
> goto exit;
> }
> @@ -160,7 +167,7 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
> len, addr, (int)len, buf);
>
> exit:
> - mutex_unlock(&data->page_mutex);
> + mutex_unlock(&rmi_i2c->page_mutex);
> return retval;
> }
>
> @@ -174,14 +181,14 @@ static int rmi_i2c_probe(struct i2c_client *client,
> {
> const struct rmi_device_platform_data *pdata =
> dev_get_platdata(&client->dev);
> - struct rmi_transport_dev *xport;
> - struct rmi_i2c_data *data;
> + struct rmi_i2c_xport *rmi_i2c;
> int retval;
>
> if (!pdata) {
> dev_err(&client->dev, "no platform data\n");
> return -EINVAL;
> }
> +
> dev_dbg(&client->dev, "Probing %s at %#02x (GPIO %d).\n",
> pdata->sensor_name ? pdata->sensor_name : "-no name-",
> client->addr, pdata->attn_gpio);
> @@ -202,44 +209,36 @@ static int rmi_i2c_probe(struct i2c_client *client,
> }
> }
>
> - xport = devm_kzalloc(&client->dev, sizeof(struct rmi_transport_dev),
> + rmi_i2c = devm_kzalloc(&client->dev, sizeof(struct rmi_i2c_xport),
> GFP_KERNEL);
> -
> - if (!xport)
> + if (!rmi_i2c)
> return -ENOMEM;
>
> - data = devm_kzalloc(&client->dev, sizeof(struct rmi_i2c_data),
> - GFP_KERNEL);
> - if (!data)
> - return -ENOMEM;
> -
> - data->xport = xport;
> -
> - xport->data = data;
> - xport->dev = &client->dev;
> + rmi_i2c->client = client;
> + mutex_init(&rmi_i2c->page_mutex);
>
> - xport->proto_name = "i2c";
> - xport->ops = &rmi_i2c_ops;
> -
> - mutex_init(&data->page_mutex);
> + rmi_i2c->xport.dev = &client->dev;
> + rmi_i2c->xport.proto_name = "i2c";
> + rmi_i2c->xport.ops = &rmi_i2c_ops;
>
> /*
> * Setting the page to zero will (a) make sure the PSR is in a
> * known state, and (b) make sure we can talk to the device.
> */
> - retval = rmi_set_page(xport, 0);
> + retval = rmi_set_page(rmi_i2c, 0);
> if (retval) {
> dev_err(&client->dev, "Failed to set page select to 0.\n");
> return retval;
> }
>
> - retval = rmi_register_transport_device(xport);
> + retval = rmi_register_transport_device(&rmi_i2c->xport);
> if (retval) {
> dev_err(&client->dev, "Failed to register transport driver at 0x%.2X.\n",
> client->addr);
> goto err_gpio;
> }
> - i2c_set_clientdata(client, xport);
> +
> + i2c_set_clientdata(client, rmi_i2c);
>
> dev_info(&client->dev, "registered rmi i2c driver at %#04x.\n",
> client->addr);
> @@ -248,16 +247,17 @@ static int rmi_i2c_probe(struct i2c_client *client,
> err_gpio:
> if (pdata->gpio_config)
> pdata->gpio_config(pdata->gpio_data, false);
> +
> return retval;
> }
>
> static int rmi_i2c_remove(struct i2c_client *client)
> {
> - struct rmi_transport_dev *xport = i2c_get_clientdata(client);
> const struct rmi_device_platform_data *pdata =
> dev_get_platdata(&client->dev);
> + struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
>
> - rmi_unregister_transport_device(xport);
> + rmi_unregister_transport_device(&rmi_i2c->xport);
>
> if (pdata->gpio_config)
> pdata->gpio_config(pdata->gpio_data, false);
>
--
Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated
^ permalink raw reply
* Re: [PATCH 1/4] Input: synaptics-rmi4 - split of transport ops into a separate structure
From: Christopher Heiny @ 2014-01-10 23:25 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Linux Input, Linux Kernel
In-Reply-To: <1389339867-8399-1-git-send-email-dmitry.torokhov@gmail.com>
On 01/09/2014 11:44 PM, Dmitry Torokhov wrote:
> Split off transport operations from rmi_transport_dev into a separate
> structure that will be shared between all devices using the same transport
> and use const pointer to access it.
>
> Change signature on transport methods so that length is using the proper
> tyep - size_t.
>
> Also rename rmi_transport_info to rmi_transport_stats and move protocol
> name (which is the only immutable piece of data there) into the transport
> device itself.
Acked-by: Christopher Heiny <cheiny@synaptics.com>
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/rmi4/rmi_bus.h | 64 ++++++++++++++++++++++++-----------------
> drivers/input/rmi4/rmi_driver.c | 8 +++---
> drivers/input/rmi4/rmi_i2c.c | 49 ++++++++++++++++---------------
> 3 files changed, 67 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
> index 3e8b57a..ccf26dc 100644
> --- a/drivers/input/rmi4/rmi_bus.h
> +++ b/drivers/input/rmi4/rmi_bus.h
> @@ -135,26 +135,25 @@ struct rmi_driver {
> #define to_rmi_driver(d) \
> container_of(d, struct rmi_driver, driver);
>
> -/** struct rmi_transport_info - diagnostic information about the RMI transport
> +/**
> + * struct rmi_transport_stats - diagnostic information about the RMI transport
> * device, used in the xport_info debugfs file.
> *
> * @proto String indicating the protocol being used.
> * @tx_count Number of transmit operations.
> - * @tx_bytes Number of bytes transmitted.
> * @tx_errs Number of errors encountered during transmit operations.
> + * @tx_bytes Number of bytes transmitted.
> * @rx_count Number of receive operations.
> - * @rx_bytes Number of bytes received.
> * @rx_errs Number of errors encountered during receive operations.
> - * @att_count Number of times ATTN assertions have been handled.
> + * @rx_bytes Number of bytes received.
> */
> -struct rmi_transport_info {
> - const char *proto;
> - long tx_count;
> - long tx_bytes;
> - long tx_errs;
> - long rx_count;
> - long rx_bytes;
> - long rx_errs;
> +struct rmi_transport_stats {
> + unsigned long tx_count;
> + unsigned long tx_errs;
> + size_t tx_bytes;
> + unsigned long rx_count;
> + unsigned long rx_errs;
> + size_t rx_bytes;
> };
>
> /**
> @@ -162,13 +161,14 @@ struct rmi_transport_info {
> *
> * @dev: Pointer to the communication device, e.g. i2c or spi
> * @rmi_dev: Pointer to the RMI device
> - * @write_block: Writing a block of data to the specified address
> - * @read_block: Read a block of data from the specified address.
> * @irq_thread: if not NULL, the sensor driver will use this instead of the
> * default irq_thread implementation.
> * @hard_irq: if not NULL, the sensor driver will use this for the hard IRQ
> * handling
> * @data: Private data pointer
> + * @proto_name: name of the transport protocol (SPI, i2c, etc)
> + * @ops: pointer to transport operations implementation
> + * @stats: transport statistics
> *
> * The RMI transport device implements the glue between different communication
> * buses such as I2C and SPI.
> @@ -178,20 +178,30 @@ struct rmi_transport_dev {
> struct device *dev;
> struct rmi_device *rmi_dev;
>
> - int (*write_block)(struct rmi_transport_dev *xport, u16 addr,
> - const void *buf, const int len);
> - int (*read_block)(struct rmi_transport_dev *xport, u16 addr,
> - void *buf, const int len);
> -
> - int (*enable_device) (struct rmi_transport_dev *xport);
> - void (*disable_device) (struct rmi_transport_dev *xport);
> -
> irqreturn_t (*irq_thread)(int irq, void *p);
> irqreturn_t (*hard_irq)(int irq, void *p);
>
> void *data;
>
> - struct rmi_transport_info info;
> + const char *proto_name;
> + const struct rmi_transport_ops *ops;
> + struct rmi_transport_stats stats;
> +};
> +
> +/**
> + * struct rmi_transport_ops - defines transport protocol operations.
> + *
> + * @write_block: Writing a block of data to the specified address
> + * @read_block: Read a block of data from the specified address.
> + */
> +struct rmi_transport_ops {
> + int (*write_block)(struct rmi_transport_dev *xport, u16 addr,
> + const void *buf, size_t len);
> + int (*read_block)(struct rmi_transport_dev *xport, u16 addr,
> + void *buf, size_t len);
> +
> + int (*enable_device) (struct rmi_transport_dev *xport);
> + void (*disable_device) (struct rmi_transport_dev *xport);
> };
>
> /**
> @@ -232,7 +242,7 @@ bool rmi_is_physical_device(struct device *dev);
> */
> static inline int rmi_read(struct rmi_device *d, u16 addr, void *buf)
> {
> - return d->xport->read_block(d->xport, addr, buf, 1);
> + return d->xport->ops->read_block(d->xport, addr, buf, 1);
> }
>
> /**
> @@ -248,7 +258,7 @@ static inline int rmi_read(struct rmi_device *d, u16 addr, void *buf)
> static inline int rmi_read_block(struct rmi_device *d, u16 addr, void *buf,
> const int len)
> {
> - return d->xport->read_block(d->xport, addr, buf, len);
> + return d->xport->ops->read_block(d->xport, addr, buf, len);
> }
>
> /**
> @@ -262,7 +272,7 @@ static inline int rmi_read_block(struct rmi_device *d, u16 addr, void *buf,
> */
> static inline int rmi_write(struct rmi_device *d, u16 addr, const u8 data)
> {
> - return d->xport->write_block(d->xport, addr, &data, 1);
> + return d->xport->ops->write_block(d->xport, addr, &data, 1);
> }
>
> /**
> @@ -278,7 +288,7 @@ static inline int rmi_write(struct rmi_device *d, u16 addr, const u8 data)
> static inline int rmi_write_block(struct rmi_device *d, u16 addr,
> const void *buf, const int len)
> {
> - return d->xport->write_block(d->xport, addr, buf, len);
> + return d->xport->ops->write_block(d->xport, addr, buf, len);
> }
>
> int rmi_register_transport_device(struct rmi_transport_dev *xport);
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index eb790ff..3483e5b 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -126,8 +126,8 @@ static void disable_sensor(struct rmi_device *rmi_dev)
> if (!data->irq)
> disable_polling(rmi_dev);
>
> - if (rmi_dev->xport->disable_device)
> - rmi_dev->xport->disable_device(rmi_dev->xport);
> + if (rmi_dev->xport->ops->disable_device)
> + rmi_dev->xport->ops->disable_device(rmi_dev->xport);
>
> if (data->irq) {
> disable_irq(data->irq);
> @@ -146,8 +146,8 @@ static int enable_sensor(struct rmi_device *rmi_dev)
> if (data->enabled)
> return 0;
>
> - if (rmi_dev->xport->enable_device) {
> - retval = rmi_dev->xport->enable_device(rmi_dev->xport);
> + if (rmi_dev->xport->ops->enable_device) {
> + retval = rmi_dev->xport->ops->enable_device(rmi_dev->xport);
> if (retval)
> return retval;
> }
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index 12aea8c..40badf3 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -61,11 +61,11 @@ static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
>
> dev_dbg(&client->dev, "writes 3 bytes: %02x %02x\n",
> txbuf[0], txbuf[1]);
> - xport->info.tx_count++;
> - xport->info.tx_bytes += sizeof(txbuf);
> + xport->stats.tx_count++;
> + xport->stats.tx_bytes += sizeof(txbuf);
> retval = i2c_master_send(client, txbuf, sizeof(txbuf));
> if (retval != sizeof(txbuf)) {
> - xport->info.tx_errs++;
> + xport->stats.tx_errs++;
> dev_err(&client->dev,
> "%s: set page failed: %d.", __func__, retval);
> return (retval < 0) ? retval : -EIO;
> @@ -75,12 +75,12 @@ static int rmi_set_page(struct rmi_transport_dev *xport, u8 page)
> }
>
> static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
> - const void *buf, const int len)
> + const void *buf, size_t len)
> {
> struct i2c_client *client = to_i2c_client(xport->dev);
> struct rmi_i2c_data *data = xport->data;
> + size_t tx_size = len + 1;
> int retval;
> - int tx_size = len + 1;
>
> mutex_lock(&data->page_mutex);
>
> @@ -106,13 +106,13 @@ static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
> }
>
> dev_dbg(&client->dev,
> - "writes %d bytes at %#06x: %*ph\n", len, addr, len, buf);
> + "writes %zd bytes at %#06x: %*ph\n", len, addr, (int)len, buf);
>
> - xport->info.tx_count++;
> - xport->info.tx_bytes += tx_size;
> + xport->stats.tx_count++;
> + xport->stats.tx_bytes += tx_size;
> retval = i2c_master_send(client, data->tx_buf, tx_size);
> if (retval < 0)
> - xport->info.tx_errs++;
> + xport->stats.tx_errs++;
> else
> retval--; /* don't count the address byte */
>
> @@ -121,9 +121,8 @@ exit:
> return retval;
> }
>
> -
> static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
> - void *buf, const int len)
> + void *buf, size_t len)
> {
> struct i2c_client *client = to_i2c_client(xport->dev);
> struct rmi_i2c_data *data = xport->data;
> @@ -140,31 +139,36 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
>
> dev_dbg(&client->dev, "writes 1 bytes: %02x\n", txbuf[0]);
>
> - xport->info.tx_count++;
> - xport->info.tx_bytes += sizeof(txbuf);
> + xport->stats.tx_count++;
> + xport->stats.tx_bytes += sizeof(txbuf);
> retval = i2c_master_send(client, txbuf, sizeof(txbuf));
> if (retval != sizeof(txbuf)) {
> - xport->info.tx_errs++;
> + xport->stats.tx_errs++;
> retval = (retval < 0) ? retval : -EIO;
> goto exit;
> }
>
> - retval = i2c_master_recv(client, buf, len);
> + xport->stats.rx_count++;
> + xport->stats.rx_bytes += len;
>
> - xport->info.rx_count++;
> - xport->info.rx_bytes += len;
> + retval = i2c_master_recv(client, buf, len);
> if (retval < 0)
> - xport->info.rx_errs++;
> + xport->stats.rx_errs++;
> else
> dev_dbg(&client->dev,
> - "read %d bytes at %#06x: %*ph\n",
> - len, addr, len, buf);
> + "read %zd bytes at %#06x: %*ph\n",
> + len, addr, (int)len, buf);
>
> exit:
> mutex_unlock(&data->page_mutex);
> return retval;
> }
>
> +static const struct rmi_transport_ops rmi_i2c_ops = {
> + .write_block = rmi_i2c_write_block,
> + .read_block = rmi_i2c_read_block,
> +};
> +
> static int rmi_i2c_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -214,9 +218,8 @@ static int rmi_i2c_probe(struct i2c_client *client,
> xport->data = data;
> xport->dev = &client->dev;
>
> - xport->write_block = rmi_i2c_write_block;
> - xport->read_block = rmi_i2c_read_block;
> - xport->info.proto = "i2c";
> + xport->proto_name = "i2c";
> + xport->ops = &rmi_i2c_ops;
>
> mutex_init(&data->page_mutex);
>
>
--
Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated
^ permalink raw reply
* [PATCH] input: synaptics-rmi4 - Count IRQs before creating functions; save F01 container.
From: Christopher Heiny @ 2014-01-10 21:53 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Christopher Heiny, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires
Because creating a function can trigger events that result in the IRQ related
storage being accessed, we need to count the IRQs and allocate their storage
before the functions are created, rather than counting them as the functions
are created and allocating them afterwards. Since we know the number of IRQs
already, we can allocate the mask at function creation time, rather than in
a post-creation loop. Also, the F01 function_container is needed elsewhere,
so we need to save it here.
In order to keep the IRQ count logic sane in bootloader mode, we move the
check for bootloader mode from F01 initialization to the IRQ counting routine.
Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/input/rmi4/rmi_driver.c | 129 +++++++++++++++++++++++++++++-----------
drivers/input/rmi4/rmi_f01.c | 11 +---
2 files changed, 96 insertions(+), 44 deletions(-)
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index f5970f4..f2acd3a 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2011-2013 Synaptics Incorporated
+ * Copyright (c) 2011-2014 Synaptics Incorporated
* Copyright (c) 2011 Unixphere
*
* This driver provides the core support for a single RMI4-based device.
@@ -553,10 +553,19 @@ static int create_function(struct rmi_device *rmi_dev,
rmi_driver_copy_pdt_to_fd(pdt, &fn->fd, page_start);
+ error = rmi_driver_irq_get_mask(rmi_dev, fn);
+ if (error < 0) {
+ dev_err(dev, "%s: Failed to create irq_mask for F%02X.\n",
+ __func__, pdt->function_number);
+ return error;
+ }
+
error = rmi_register_function(fn);
if (error)
goto err_free_mem;
+ if (pdt->function_number == 0x01)
+ data->f01_container = fn;
list_add_tail(&fn->node, &data->function_list);
return 0;
@@ -566,10 +575,33 @@ err_free_mem:
return error;
}
-
#define RMI_SCAN_CONTINUE 0
#define RMI_SCAN_DONE 1
+/* Indicates that flash programming is enabled (bootloader mode). */
+#define RMI_F01_STATUS_BOOTLOADER(status) (!!((status) & 0x40))
+
+/*
+ * Given the PDT entry for F01, read the device status register to determine
+ * if we're stuck in bootloader mode or not.
+ *
+ */
+static int check_bootloader_mode(struct rmi_device *rmi_dev, struct pdt_entry *pdt,
+ u16 page_start)
+{
+ u8 device_status;
+ int retval = 0;
+
+ retval = rmi_read(rmi_dev, pdt->data_base_addr + page_start,
+ &device_status);
+ if (retval < 0) {
+ dev_err(&rmi_dev->dev, "Failed to read device status.\n");
+ return retval;
+ }
+
+ return RMI_F01_STATUS_BOOTLOADER(device_status);
+}
+
static int rmi_initial_reset(struct rmi_device *rmi_dev,
void *clbk_ctx, struct pdt_entry *pdt_entry, int page)
{
@@ -596,6 +628,23 @@ static int rmi_initial_reset(struct rmi_device *rmi_dev,
return (!page) ? RMI_SCAN_CONTINUE : -ENODEV;
}
+static int rmi_count_irqs(struct rmi_device *rmi_dev,
+ void * clbk_ctx, struct pdt_entry *pdt_entry, int page)
+{
+ struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
+
+ data->irq_count += pdt_entry->interrupt_source_count;
+ if (pdt_entry->function_number == 0x01) {
+ data->f01_bootloader_mode = check_bootloader_mode(rmi_dev,
+ pdt_entry, page);
+ if (data->f01_bootloader_mode)
+ dev_warn(&rmi_dev->dev,
+ "WARNING: RMI4 device is in bootloader mode!\n");
+ }
+
+ return RMI_SCAN_CONTINUE;
+}
+
static int rmi_create_functions_clbk(struct rmi_device *rmi_dev,
void *clbk_ctx, struct pdt_entry *entry, int page)
{
@@ -608,7 +657,6 @@ static int rmi_create_functions_clbk(struct rmi_device *rmi_dev,
static int rmi_create_functions(struct rmi_device *rmi_dev)
{
struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
- struct device *dev = &rmi_dev->dev;
int irq_count = 0;
int retval;
@@ -763,7 +811,6 @@ static int rmi_driver_probe(struct device *dev)
{
struct rmi_driver *rmi_driver;
struct rmi_driver_data *data = NULL;
- struct rmi_function *fn;
struct rmi_device_platform_data *pdata;
int retval = 0;
struct rmi_device *rmi_dev;
@@ -818,28 +865,6 @@ static int rmi_driver_probe(struct device *dev)
if (retval)
dev_warn(dev, "RMI initial reset failed! Continuing in spite of this.\n");
- retval = rmi_create_functions(rmi_dev);
- if (retval) {
- dev_err(dev, "PDT scan for %s failed with code %d.\n",
- pdata->sensor_name, retval);
- goto err_free_data;
- }
-
- if (!data->f01_container) {
- dev_err(dev, "missing F01 container!\n");
- retval = -EINVAL;
- goto err_free_data;
- }
-
- list_for_each_entry(fn, &data->function_list, node) {
- retval = rmi_driver_irq_get_mask(rmi_dev, fn);
- if (retval < 0) {
- dev_err(dev, "%s: Failed to create irq_mask.\n",
- __func__);
- goto err_free_data;
- }
- }
-
retval = rmi_read(rmi_dev, PDT_PROPERTIES_LOCATION, &data->pdt_props);
if (retval < 0) {
/*
@@ -850,6 +875,21 @@ static int rmi_driver_probe(struct device *dev)
PDT_PROPERTIES_LOCATION);
}
+ /*
+ * We need to count the IRQs and allocate their storage before scanning
+ * the PDT and creating the function entries, because adding a new
+ * function can trigger events that result in the IRQ related storage
+ * being accessed.
+ */
+ dev_dbg(dev, "Counting IRQs.\n");
+ retval = rmi_scan_pdt(rmi_dev, NULL, rmi_count_irqs);
+ if (retval) {
+ dev_err(dev, "IRQ counting for %s failed with code %d.\n",
+ pdata->sensor_name, retval);
+ goto err_free_data;
+ }
+ data->num_of_irq_regs = (data->irq_count + 7) / 8;
+
mutex_init(&data->irq_mutex);
data->irq_status = devm_kzalloc(dev,
BITS_TO_LONGS(data->irq_count)*sizeof(unsigned long),
@@ -869,6 +909,28 @@ static int rmi_driver_probe(struct device *dev)
goto err_free_data;
}
+ data->irq_mask_store = devm_kzalloc(dev,
+ BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long),
+ GFP_KERNEL);
+ if (!data->irq_mask_store) {
+ dev_err(dev, "Failed to allocate irq_mask_store.\n");
+ retval = -ENOMEM;
+ goto err_free_data;
+ }
+
+ retval = rmi_create_functions(rmi_dev);
+ if (retval) {
+ dev_err(dev, "Function creation failed with code %d.\n",
+ retval);
+ goto err_free_data;
+ }
+
+ if (!data->f01_container) {
+ dev_err(dev, "missing F01 container!\n");
+ retval = -EINVAL;
+ goto err_free_data;
+ }
+
retval = rmi_read_block(rmi_dev,
data->f01_container->fd.control_base_addr+1,
data->current_irq_mask, data->num_of_irq_regs);
@@ -878,14 +940,6 @@ static int rmi_driver_probe(struct device *dev)
goto err_free_data;
}
- data->irq_mask_store = devm_kzalloc(dev,
- BITS_TO_LONGS(data->irq_count)*sizeof(unsigned long),
- GFP_KERNEL);
- if (!data->irq_mask_store) {
- dev_err(dev, "Failed to allocate mask store.\n");
- retval = -ENOMEM;
- goto err_free_data;
- }
if (IS_ENABLED(CONFIG_PM)) {
data->pm_data = pdata->pm_data;
data->pre_suspend = pdata->pre_suspend;
@@ -951,6 +1005,13 @@ static int rmi_driver_probe(struct device *dev)
return 0;
err_free_data:
+ rmi_free_function_list(rmi_dev);
+ if (gpio_is_valid(pdata->attn_gpio))
+ gpio_free(pdata->attn_gpio);
+ devm_kfree(&rmi_dev->dev, data->irq_status);
+ devm_kfree(&rmi_dev->dev, data->current_irq_mask);
+ devm_kfree(&rmi_dev->dev, data->irq_mask_store);
+ devm_kfree(&rmi_dev->dev, data);
return retval;
}
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 628b082..c7f2360 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2011-2012 Synaptics Incorporated
+ * Copyright (c) 2011-2014 Synaptics Incorporated
* Copyright (c) 2011 Unixphere
*
* This program is free software; you can redistribute it and/or modify it
@@ -59,8 +59,6 @@ struct f01_basic_properties {
/* Most recent device status event */
#define RMI_F01_STATUS_CODE(status) ((status) & 0x0f)
-/* Indicates that flash programming is enabled (bootloader mode). */
-#define RMI_F01_STATUS_BOOTLOADER(status) (!!((status) & 0x40))
/* The device has lost its configuration for some reason. */
#define RMI_F01_STATUS_UNCONFIGURED(status) (!!((status) & 0x80))
@@ -358,13 +356,6 @@ static int rmi_f01_initialize(struct rmi_function *fn)
goto error_exit;
}
- driver_data->f01_bootloader_mode =
- RMI_F01_STATUS_BOOTLOADER(data->device_status);
- if (driver_data->f01_bootloader_mode)
- dev_warn(&rmi_dev->dev,
- "WARNING: RMI4 device is in bootloader mode!\n");
-
-
if (RMI_F01_STATUS_UNCONFIGURED(data->device_status)) {
dev_err(&fn->dev,
"Device was reset during configuration process, status: %#02x!\n",
^ permalink raw reply related
* Re: [PATCH] input synaptics-rmi4: PDT scan cleanup
From: Christopher Heiny @ 2014-01-10 21:50 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires
In-Reply-To: <1754181.lppeTWGTml@dtor-d630.eng.vmware.com>
On 01/10/2014 12:31 PM, Dmitry Torokhov wrote:
> On Friday, January 10, 2014 12:23:15 PM Christopher Heiny wrote:
>> >On 01/07/2014 12:33 PM, Christopher Heiny wrote:
>>> > >Eliminates copy-paste code that handled scans of the Page Descriptor
>>> > >Table,
>>> > >replacing it with a single PDT scan routine that invokes a callback
>>> > >function. The scan routine is not static so that it can be used by the
>>> > >firmware update code (under development, not yet submitted).
>>> > >
>>> > >Updated the copyright dates while we were at it.
>> >
>> >Hi Dmitry,
>> >
>> >Could you apply this or provide some feedback on it? We've got a
>> >pending patch that depends on it, and that pending work will bring the
>> >driver back to a working (if not necessarily beautiful) state. I don't
>> >want to submit it if this change isn't satisfactory, though.
>
> Speaking of the devil. I was just thinking about it and I wanted to ask
> you to send me an example of how it is used as I can;t make my mind about
> it.
>
> In general it is OK to submit a few patches at a time even if they are
> depend on each other - it gives me better context (as long as there
> aren't 80 of those so that I down in them ;) ).
>
> Thanks.
No problem!
Currently the rmi_driver.c iterates over the PDT twice during probe():
1) find F01 and reset the ASIC;
2) create and initialize the function devices & count the number of
interrupt sources.
followed by
3) a loop over the function devices allocating each one's interrupt
related bitmasks.
Unfortunately, depending on how the function drivers are loaded, a
function driver might access the device's and the function device's
bitmasks before step (3) is complete. This causes all kinds of
unintended consequences, none of which are amusing.
We considered fixing this by splitting the function device creation and
initialization, like so:
1) first to find F01 and reset the ASIC;
2) count the number of interrupt sources and create the function devices
followed by
3) a loop over the function devices to allocate their interrupt related
bitmasks and initialize the function devices
However, this led to function device setup being in two different
places, which obscured the code and could lead to bugs if someone added
new code in the wrong place (initialization instead of creation, or vice
versa).
The PDT scan loops have a lot of redundant boilerplate and were quite
large compared to the core code that was being iterated, obscuring just
what the loop was supposed to be doing. We found it clearer to
implement a single PDT scan routine, and put the logic for the different
step in callbacks. Plus it eliminated the maintenance headaches of
three (or more, see reflash, below) copy-paste PDT scan loops.
The pending patch (I'll send that right after this note) changes
rmi_driver.c the order to scan the PDT 3 times:
1) first to find F01 and reset the ASIC;
2) count the number of interrupt sources
3) create and initialize the function devices
Similar logic applies in the reflash code, which has rescan the PDT
after forcing the touch sensor firmware into bootloader mode (the
bootloader might have a different register map, unfortunately). Once we
had the scan+callback routine in rmi_driver.c, it only made sense for
the reflash library to use that as well.
Thanks,
Chris
^ permalink raw reply
* Re: Re: [PATCH] Introduce Naming Convention in Input Subsystem
From: Dmitry Torokhov @ 2014-01-10 21:46 UTC (permalink / raw)
To: Aniroop Mathur
Cc: Aniroop Mathur, linux-input@vger.kernel.org, cpgs .,
Anurag Aggarwal, Naveen Kumar, VIKAS KALA, Poorva Srivastava
In-Reply-To: <CADYu30_qxKV7SmUdUc1Y-sN+KDYbWyz6Yv_H6BsSA-koK9aWYQ@mail.gmail.com>
On Sat, Jan 11, 2014 at 02:55:33AM +0530, Aniroop Mathur wrote:
> Hello Mr. Torokhov,
> Greetings!
>
> First of all, So sorry, unfortunately i used HTML text again.
> and Many thanks for all replies.
>
> Sending email again in plain text.
>
>
> On Sat, Jan 11, 2014 at 12:41 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Hi Aniroop,
> >
> > On Fri, Jan 10, 2014 at 04:49:43PM +0000, Aniroop Mathur wrote:
> >> Hello Mr. Torokhov,
> >> Greetings!
> >>
> >> On Thu, Jan 09, 2014 at 10:27:56AM +0530, Aniroop Mathur wrote:
> >> > This patch allows user(driver) to set sysfs node name of input
> >> > devices. To set sysfs node name, user(driver) just needs to set
> >> > node_name_unique variable. If node_name_unique is not set, default
> >> > name is given(as before). So, this patch is completely
> >> > backward-compatible.
> >> >
> >> > Sysfs Input node name format is: input_
> >> > Sysfs Event node name format is: event_
> >> >
> >> > This "name" is given by user and automatically, prefix(input and
> >> > event) is added by input core.
> >> >
> >> > This name must be unique among all input devices and driver(user) has
> >> > the responsibility to ensure it. If same name is used again for other
> >> > input device, registration of that input device will fail because two
> >> > input devices cannot have same name.
> >> >
> >> > Advantages of this patch are:
> >> >
> >> > 1. Reduces Booting Time of HAL/Upper-Layer because now HAL or
> >> > Upper-Layer do not need to search input/event number corresponding to
> >> > each input device in /dev/input/... This searching in /dev/input/ was
> >> > taking too much time. (Especially in mobile devices, where there are
> >> > many input devices (many sensors, touchscreen, etc), it reduces a lot
> >> > of booting time)
> >>
> >> I am sorry, how much time does it take to scan a directory of what, 20
> >> devices? If it such a factor have udev create nodes that are easier for
> >> you to locate, similarly how we already create nodes by-id and by-path.
> >> For example you can encode major:minor in device name.
> >>
> >> Re: (Aniroop Mathur)
> >
> > First of all, it would be great if you could use MUA that can properly
> > quote and wrap long lines...
> >
> >> Its correct that we can set name of a device node using udev. Yes,
> >> this will change the name of device node(/dev/...) but not sysfs
> >> node.(/sys/class/input/...) So now, the problem area will shift from
> >> dev path to sysfs path, because now we dont know which sysfs node to
> >> refer for a particular input device and hence HAL/Upper-Layer will
> >> need to search in /sys/class/input/... instead of /dev/... directory.
> >
> > [dtor@dtor-d630 ~]$ mkdir my-sysfs-view
> > [dtor@dtor-d630 ~]$ ln -s
> > /sys/devices/platform/i8042/serio1/input/input6
> > my-sysfs-view/input_touchpad
> > [dtor@dtor-d630 ~]$ ls my-sysfs-view/input_touchpad/
> > capabilities/ event6/ modalias name power/
> > subsystem/ uniq
> > device/ id/ mouse1/ phys properties
> > uevent
> > [dtor@dtor-d630 ~]$ ls my-sysfs-view/input_touchpad/
> > capabilities device event6 id modalias mouse1 name phys power
> > properties subsystem uevent uniq
> > [dtor@dtor-d630 ~]$ ls my-sysfs-view/input_touchpad/event6/
> > dev device power subsystem uevent
> >
> > Mmmmkay?
> >
>
> Yes, agreed, we can use udev and soft links to achieve this.
> But i think there is something more to take care.
>
> So far, as per discussion, i understood that if an end user wants to use
> node names instead of numbers, he/she has to do the following things:
No, not the end user, system integrator, which is quite different
beast.
> 1. Create rules for all input devices in udev rule file i.e. set atleast
> unique id and unique name.
> (end user need to determine unique id too for every input device)
> 2. Create links for all input device nodes using names.
> (in probe function, after input_register_device)
>
> By following above two steps, the file structure will look like:
> devfs - /dev/input_proximity
> sysfs - my-sysfs-view/input_proximity --> sys/class/input/input2
> sysfs - my-sysfs-view/event_proximity --> sys/class/input/event2
>
> But my concern is why to create trouble for end user to perform
> and spend time for two extra steps, when an easy way is possible
> to achieve the same task ?
>
> With this patch, end user only need to set node_name_unique variable
> and right after that, both for devfs and sysfs,same node name is set.
> End user does not need to do or take care of any other extra work,
> like creating entry in udev rules, creating links, etc
>
> Also, with creating links for all input devices and checking udev rules
> before actually creating a device node, will only increase computation
> and time in kernel code.
So do not create links, use something else to track devices. You are
getting uevents, that is all you need.
>
> My purpose is to avoid extra work load and directly create node names
> within input subsystem. Also backward compatibility is there.
> So i think, it is better than the other alternative way.
> Isn't this more easy ? Is there any side-effect or drawback of this patch ?
Yes, there is huge side effect - it is maintenance nightmare, where one
driver can now cause failure for others. What if I have 2 proximity
sensors? 2 accelerometers? How will generic drivers select names that
will satisfy all boards that might use the chips out there? Are you
proposing to put this data in device tree for example? Board files?
IOW no, this is not right solution and the patch will not be accepted.
>
> >>
> >> Moreover, as i know, udev is mainly for hot-pluggable devices, but my
> >> problem is for platform devices, which are already present on the
> >> board during boot up. (Like in Embedded devices)
> >
> > No, udev also manages those by requesting to replay all events that
> > happened dyuring boot.
> >
> >>
> >> To avoid confusion and make the problem more clear,
> >> I would like to explain the problem and my suggestion by taking an example:
> >>
> >> Suppose in a mobile device, there are 10 embedded input devices as below:
> >> Proximity --- /dev/input0 --- /sys/class/input/input0 --- /sys/class/input/event0
> >> Magnetometer --- /dev/input1 --- /sys/class/input/input1 --- /sys/class/input/event1
> >> Accelerometer --- /dev/input2 --- /sys/class/input/input2 --- /sys/class/input/event2
> >> Touchscreen --- /dev/input3 --- /sys/class/input/input3 --- /sys/class/input/event3
> >> ... 6 more like this
> >> (All these are created during boot up time)
> >>
> >> Kernel has created all these nodes, so that HAL/UpperLayer can read or
> >> write values from it. HAL/Upper-Layer needs to do main tasks like:
> >> 1. Read raw data - does through /dev/input<num>
> >> 2. Enable device - does through sys/class/input<num>/enable
> >> 3. Set delay - does through sys/class/input<num>/delay
> >> and many more...
> >>
> >> Now, Lets suppose we need to do these tasks for Accelerometer.
> >>
> >> If dev node name is set, HAL can directly read value from it (no
> >> search required) But for enabling the accelerometer device or set the
> >> delay of a hardware chip, there is no direct way, HAL can know which
> >> input node to refer for accelerometer because the input number is
> >> created dynamically as per device probe order, so this input number
> >> can be anything (0,1,2,3...) So HAL will need to search every input
> >> node and read its name attribute and keep on searching until a match
> >> is found between the "attribute name" and "name passed as parameter".
> >> Like for accelerometer, this searching needs to be done for all other
> >> input devices. All of this part is done during booting and this takes
> >> a lot for time from booting perspective.
> >>
> >
> > See the above. You can very easily create your own private 'view' of
> > sysfs, no kernel changes needed.
> >
> >> As I measured, if there are ten devices, it is taking 1 second to do
> >> all this searching. (for all devices) So for 20 devices, i guess, it
> >> could take upto 2 seconds.
> >
> > That seems _very_ high, maybe you need to profile your code a bit. To
> > search though 2 directories with less than a hundred files each should
> > not take 1 second.
> >
>
> In this i am including time to open a directory, close a directory, open file of
> that directory, close file of that directory, searching and computation part.
> Including all these, every time for each input device.
> All this sums upto 1 second.
Why are you doing it one at at time? It appears that this happens in
build at boot up for you...
>
> >>
> >> With naming convention, there is no need of search neither for dev
> >> path nor for sysfs path because HAL directly know which node to refer
> >> for which input device and hence this 1 second is reduced to 10ms or
> >> even less, therefore saving 990ms. I believe, this is a very good
> >> time saving. (from device booting perspective)
> >
> > OK, so create your own sysfs view and use it to do direct lookups.
> >
> >>
> >> (Is there any direct way, without scanning all nodes for every input
> >> device ?)
> >>
> >> >
> >> > 2. Improves Readabilty of input and event sysfs node paths because
> >> > names are used instead of numbers.
> >>
> >> I do not see why it is that important. If one wants overview
> >> /proc/bus/input/devices gives nice picture.
> >>
> >> Re: (Aniroop Mathur)
> >> Its correct, we can get an overview from /proc/bus/input/devices.
> >> And therefore using this, we can know input node number for every input device.
> >> But there are many input devices and input numbers are not fixed,
> >> so its quite difficult to memorize input number for all input devices.
> >> Therefore, if a user needs to open some input node from sysfs path,
> >> he needs to check /proc/bus/input/devices before opening because
> >> he does not know the input number. Moreover, this applies for all other
> >> input devices and hence a user need to check this every time.
> >>
> >> It improves readabilty as below
> >>
> >> Before: After patch:
> >> /dev/input0 /dev/input_proximity
> >> /dev/input1 /dev/input_accelerometer
> >> ...many more
> >>
> >> /sys/class/input/input0 /sys/class/input/input_proximity
> >> /sys/class/input/input1 /sys/class/input/input_accelerometer
> >> ...many more
> >>
> >> /sys/class/input/event0 /sys/class/input/event_proximity
> >> /sys/class/input/event1 /sys/class/input/event_accelerometer
> >> ...many more
> >>
> >> So, just by looking, user can directly open or refer any input node.
> >> (no need to refer any other path)
> >
> > User as in end user or your HAL layer?
> >
>
> End user.
Why would end user care? He wants his touchscreen to work, not fiddle
with its settings, And we aleady discussed what system integrator should
do.
>
> >>
> >> >
> >> > 3. Removes Input Devices Dependency. If one input device probe fails,
> >> > other input devices still work. Before this patch, if one input
> >> > device probe fails before input_register_device, then input number of
> >> > other input devices changes and due to this permission settings are
> >> > disturbed and hence HAL or upper layer cannot open the required sysfs
> >> > node because permission denied error comes.
> >>
> >> I have only one suggestion here: fix your userspace so that does not
> >> depend on device initialization ordering.
> >>
> >> Re: (Aniroop Mathur)
> >> We cannot fix userspace because these input/event/dev number are
> >> decided/allocated in kernel as per device initialization ordering
> >> during boot up. (userspace has no role in it) So, userspace is not
> >> aware, which exact input number corresponds to which input device so
> >> it ends up searching/scanning every input node untill a match is
> >> found.
> >>
> >> So, there is input device dependency which needs to be removed.
> >
> > Do not use numbers. We emit uevents describing the devices and there a
> > _lot_ of data there that helps identifying device, such as its path,
> > subsystem, name, etc.
> >
>
> Sorry, I am not able to understand this point with respect to removing input
> device dependency. Please elaborate a bit more.
Look at the data that is passed in uevents that are sent either when new
input device is created, or when you request replay of such evenst,
realize that it is enough to identify the device and stop relying on
inputX names to remain static. That's it.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: Re: [PATCH] Introduce Naming Convention in Input Subsystem
From: Aniroop Mathur @ 2014-01-10 21:25 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Aniroop Mathur, linux-input@vger.kernel.org, cpgs .,
Anurag Aggarwal, Naveen Kumar, VIKAS KALA, Poorva Srivastava
In-Reply-To: <20140110191108.GC10889@core.coreip.homeip.net>
Hello Mr. Torokhov,
Greetings!
First of all, So sorry, unfortunately i used HTML text again.
and Many thanks for all replies.
Sending email again in plain text.
On Sat, Jan 11, 2014 at 12:41 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Aniroop,
>
> On Fri, Jan 10, 2014 at 04:49:43PM +0000, Aniroop Mathur wrote:
>> Hello Mr. Torokhov,
>> Greetings!
>>
>> On Thu, Jan 09, 2014 at 10:27:56AM +0530, Aniroop Mathur wrote:
>> > This patch allows user(driver) to set sysfs node name of input
>> > devices. To set sysfs node name, user(driver) just needs to set
>> > node_name_unique variable. If node_name_unique is not set, default
>> > name is given(as before). So, this patch is completely
>> > backward-compatible.
>> >
>> > Sysfs Input node name format is: input_
>> > Sysfs Event node name format is: event_
>> >
>> > This "name" is given by user and automatically, prefix(input and
>> > event) is added by input core.
>> >
>> > This name must be unique among all input devices and driver(user) has
>> > the responsibility to ensure it. If same name is used again for other
>> > input device, registration of that input device will fail because two
>> > input devices cannot have same name.
>> >
>> > Advantages of this patch are:
>> >
>> > 1. Reduces Booting Time of HAL/Upper-Layer because now HAL or
>> > Upper-Layer do not need to search input/event number corresponding to
>> > each input device in /dev/input/... This searching in /dev/input/ was
>> > taking too much time. (Especially in mobile devices, where there are
>> > many input devices (many sensors, touchscreen, etc), it reduces a lot
>> > of booting time)
>>
>> I am sorry, how much time does it take to scan a directory of what, 20
>> devices? If it such a factor have udev create nodes that are easier for
>> you to locate, similarly how we already create nodes by-id and by-path.
>> For example you can encode major:minor in device name.
>>
>> Re: (Aniroop Mathur)
>
> First of all, it would be great if you could use MUA that can properly
> quote and wrap long lines...
>
>> Its correct that we can set name of a device node using udev. Yes,
>> this will change the name of device node(/dev/...) but not sysfs
>> node.(/sys/class/input/...) So now, the problem area will shift from
>> dev path to sysfs path, because now we dont know which sysfs node to
>> refer for a particular input device and hence HAL/Upper-Layer will
>> need to search in /sys/class/input/... instead of /dev/... directory.
>
> [dtor@dtor-d630 ~]$ mkdir my-sysfs-view
> [dtor@dtor-d630 ~]$ ln -s
> /sys/devices/platform/i8042/serio1/input/input6
> my-sysfs-view/input_touchpad
> [dtor@dtor-d630 ~]$ ls my-sysfs-view/input_touchpad/
> capabilities/ event6/ modalias name power/
> subsystem/ uniq
> device/ id/ mouse1/ phys properties
> uevent
> [dtor@dtor-d630 ~]$ ls my-sysfs-view/input_touchpad/
> capabilities device event6 id modalias mouse1 name phys power
> properties subsystem uevent uniq
> [dtor@dtor-d630 ~]$ ls my-sysfs-view/input_touchpad/event6/
> dev device power subsystem uevent
>
> Mmmmkay?
>
Yes, agreed, we can use udev and soft links to achieve this.
But i think there is something more to take care.
So far, as per discussion, i understood that if an end user wants to use
node names instead of numbers, he/she has to do the following things:
1. Create rules for all input devices in udev rule file i.e. set atleast
unique id and unique name.
(end user need to determine unique id too for every input device)
2. Create links for all input device nodes using names.
(in probe function, after input_register_device)
By following above two steps, the file structure will look like:
devfs - /dev/input_proximity
sysfs - my-sysfs-view/input_proximity --> sys/class/input/input2
sysfs - my-sysfs-view/event_proximity --> sys/class/input/event2
But my concern is why to create trouble for end user to perform
and spend time for two extra steps, when an easy way is possible
to achieve the same task ?
With this patch, end user only need to set node_name_unique variable
and right after that, both for devfs and sysfs,same node name is set.
End user does not need to do or take care of any other extra work,
like creating entry in udev rules, creating links, etc
Also, with creating links for all input devices and checking udev rules
before actually creating a device node, will only increase computation
and time in kernel code.
My purpose is to avoid extra work load and directly create node names
within input subsystem. Also backward compatibility is there.
So i think, it is better than the other alternative way.
Isn't this more easy ? Is there any side-effect or drawback of this patch ?
>>
>> Moreover, as i know, udev is mainly for hot-pluggable devices, but my
>> problem is for platform devices, which are already present on the
>> board during boot up. (Like in Embedded devices)
>
> No, udev also manages those by requesting to replay all events that
> happened dyuring boot.
>
>>
>> To avoid confusion and make the problem more clear,
>> I would like to explain the problem and my suggestion by taking an example:
>>
>> Suppose in a mobile device, there are 10 embedded input devices as below:
>> Proximity --- /dev/input0 --- /sys/class/input/input0 --- /sys/class/input/event0
>> Magnetometer --- /dev/input1 --- /sys/class/input/input1 --- /sys/class/input/event1
>> Accelerometer --- /dev/input2 --- /sys/class/input/input2 --- /sys/class/input/event2
>> Touchscreen --- /dev/input3 --- /sys/class/input/input3 --- /sys/class/input/event3
>> ... 6 more like this
>> (All these are created during boot up time)
>>
>> Kernel has created all these nodes, so that HAL/UpperLayer can read or
>> write values from it. HAL/Upper-Layer needs to do main tasks like:
>> 1. Read raw data - does through /dev/input<num>
>> 2. Enable device - does through sys/class/input<num>/enable
>> 3. Set delay - does through sys/class/input<num>/delay
>> and many more...
>>
>> Now, Lets suppose we need to do these tasks for Accelerometer.
>>
>> If dev node name is set, HAL can directly read value from it (no
>> search required) But for enabling the accelerometer device or set the
>> delay of a hardware chip, there is no direct way, HAL can know which
>> input node to refer for accelerometer because the input number is
>> created dynamically as per device probe order, so this input number
>> can be anything (0,1,2,3...) So HAL will need to search every input
>> node and read its name attribute and keep on searching until a match
>> is found between the "attribute name" and "name passed as parameter".
>> Like for accelerometer, this searching needs to be done for all other
>> input devices. All of this part is done during booting and this takes
>> a lot for time from booting perspective.
>>
>
> See the above. You can very easily create your own private 'view' of
> sysfs, no kernel changes needed.
>
>> As I measured, if there are ten devices, it is taking 1 second to do
>> all this searching. (for all devices) So for 20 devices, i guess, it
>> could take upto 2 seconds.
>
> That seems _very_ high, maybe you need to profile your code a bit. To
> search though 2 directories with less than a hundred files each should
> not take 1 second.
>
In this i am including time to open a directory, close a directory, open file of
that directory, close file of that directory, searching and computation part.
Including all these, every time for each input device.
All this sums upto 1 second.
>>
>> With naming convention, there is no need of search neither for dev
>> path nor for sysfs path because HAL directly know which node to refer
>> for which input device and hence this 1 second is reduced to 10ms or
>> even less, therefore saving 990ms. I believe, this is a very good
>> time saving. (from device booting perspective)
>
> OK, so create your own sysfs view and use it to do direct lookups.
>
>>
>> (Is there any direct way, without scanning all nodes for every input
>> device ?)
>>
>> >
>> > 2. Improves Readabilty of input and event sysfs node paths because
>> > names are used instead of numbers.
>>
>> I do not see why it is that important. If one wants overview
>> /proc/bus/input/devices gives nice picture.
>>
>> Re: (Aniroop Mathur)
>> Its correct, we can get an overview from /proc/bus/input/devices.
>> And therefore using this, we can know input node number for every input device.
>> But there are many input devices and input numbers are not fixed,
>> so its quite difficult to memorize input number for all input devices.
>> Therefore, if a user needs to open some input node from sysfs path,
>> he needs to check /proc/bus/input/devices before opening because
>> he does not know the input number. Moreover, this applies for all other
>> input devices and hence a user need to check this every time.
>>
>> It improves readabilty as below
>>
>> Before: After patch:
>> /dev/input0 /dev/input_proximity
>> /dev/input1 /dev/input_accelerometer
>> ...many more
>>
>> /sys/class/input/input0 /sys/class/input/input_proximity
>> /sys/class/input/input1 /sys/class/input/input_accelerometer
>> ...many more
>>
>> /sys/class/input/event0 /sys/class/input/event_proximity
>> /sys/class/input/event1 /sys/class/input/event_accelerometer
>> ...many more
>>
>> So, just by looking, user can directly open or refer any input node.
>> (no need to refer any other path)
>
> User as in end user or your HAL layer?
>
End user.
>>
>> >
>> > 3. Removes Input Devices Dependency. If one input device probe fails,
>> > other input devices still work. Before this patch, if one input
>> > device probe fails before input_register_device, then input number of
>> > other input devices changes and due to this permission settings are
>> > disturbed and hence HAL or upper layer cannot open the required sysfs
>> > node because permission denied error comes.
>>
>> I have only one suggestion here: fix your userspace so that does not
>> depend on device initialization ordering.
>>
>> Re: (Aniroop Mathur)
>> We cannot fix userspace because these input/event/dev number are
>> decided/allocated in kernel as per device initialization ordering
>> during boot up. (userspace has no role in it) So, userspace is not
>> aware, which exact input number corresponds to which input device so
>> it ends up searching/scanning every input node untill a match is
>> found.
>>
>> So, there is input device dependency which needs to be removed.
>
> Do not use numbers. We emit uevents describing the devices and there a
> _lot_ of data there that helps identifying device, such as its path,
> subsystem, name, etc.
>
Sorry, I am not able to understand this point with respect to removing input
device dependency. Please elaborate a bit more.
>>
>> ----------------------------
>>
>> IOW I am totally unconvinced that this facility is needed.
>>
>> Re: (Aniroop Mathur)
>> I hope my problem and suggestion is more clear and convincing now.
>>
>
> Not in the slightest, I am sorry.
>
> Thanks.
>
> --
> Dmitry
Thanks,
Aniroop Mathur
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4: PDT scan cleanup
From: Dmitry Torokhov @ 2014-01-10 20:31 UTC (permalink / raw)
To: Christopher Heiny
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires
In-Reply-To: <52D056B3.406@synaptics.com>
On Friday, January 10, 2014 12:23:15 PM Christopher Heiny wrote:
> On 01/07/2014 12:33 PM, Christopher Heiny wrote:
> > Eliminates copy-paste code that handled scans of the Page Descriptor
> > Table,
> > replacing it with a single PDT scan routine that invokes a callback
> > function. The scan routine is not static so that it can be used by the
> > firmware update code (under development, not yet submitted).
> >
> > Updated the copyright dates while we were at it.
>
> Hi Dmitry,
>
> Could you apply this or provide some feedback on it? We've got a
> pending patch that depends on it, and that pending work will bring the
> driver back to a working (if not necessarily beautiful) state. I don't
> want to submit it if this change isn't satisfactory, though.
Speaking of the devil. I was just thinking about it and I wanted to ask
you to send me an example of how it is used as I can;t make my mind about
it.
In general it is OK to submit a few patches at a time even if they are
depend on each other - it gives me better context (as long as there
aren't 80 of those so that I down in them ;) ).
Thanks.
--
Dmitry
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox