* Re: [PATCH 44/51] Input: atmel_mxt_ts - Handle reports from T47 Stylus object
From: rydberg @ 2013-07-18 17:23 UTC (permalink / raw)
To: Nick Dyer
Cc: Dmitry Torokhov, Daniel Kurtz, Joonyoung Shim, Alan Bowens,
linux-input, linux-kernel, Peter Meerwald, Benson Leung,
Olof Johansson
In-Reply-To: <1372337366-9286-45-git-send-email-nick.dyer@itdev.co.uk>
On Thu, Jun 27, 2013 at 01:49:19PM +0100, Nick Dyer wrote:
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> Acked-by: Benson Leung <bleung@chromium.org>
> ---
> drivers/input/touchscreen/atmel_mxt_ts.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index ee39683..ceb090a 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -128,6 +128,9 @@ struct t9_range {
> /* Define for MXT_PROCI_TOUCHSUPPRESSION_T42 */
> #define MXT_T42_MSG_TCHSUP (1 << 0)
>
> +/* T47 Stylus */
> +#define MXT_TOUCH_MAJOR_T47_STYLUS 1
> +
Ok - normally, creating dummy values is not recommended, but for the
shared touch/stylys implementation there is obviously no alternative.
> /* T63 Stylus */
> #define MXT_T63_STYLUS_PRESS (1 << 0)
> #define MXT_T63_STYLUS_RELEASE (1 << 1)
> @@ -696,6 +699,7 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
> int area;
> int amplitude;
> u8 vector;
> + int tool;
>
> /* do not report events if input device not yet registered */
> if (!data->enable_reporting)
> @@ -713,6 +717,7 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
> y >>= 2;
>
> area = message[5];
> +
> amplitude = message[6];
> vector = message[7];
>
> @@ -741,8 +746,17 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
> mxt_input_sync(input_dev);
> }
>
> + /* A reported size of zero indicates that the reported touch
> + * is a stylus from a linked Stylus T47 object. */
> + if (area == 0) {
> + area = MXT_TOUCH_MAJOR_T47_STYLUS;
> + tool = MT_TOOL_PEN;
> + } else {
> + tool = MT_TOOL_FINGER;
> + }
> +
> /* Touch active */
> - input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, 1);
> + input_mt_report_slot_state(input_dev, tool, 1);
> input_report_abs(input_dev, ABS_MT_POSITION_X, x);
> input_report_abs(input_dev, ABS_MT_POSITION_Y, y);
> input_report_abs(input_dev, ABS_MT_PRESSURE, amplitude);
> --
> 1.7.10.4
>
Reviewed-by: Henrik Rydberg <rydberg@euromail.se>
Thanks,
Henrik
^ permalink raw reply
* Re: [PATCH 39/51] Input: atmel_mxt_ts - Implement T63 Active Stylus support
From: rydberg @ 2013-07-18 17:21 UTC (permalink / raw)
To: Nick Dyer
Cc: Dmitry Torokhov, Daniel Kurtz, Joonyoung Shim, Alan Bowens,
linux-input, linux-kernel, Peter Meerwald, Benson Leung,
Olof Johansson
In-Reply-To: <1372337366-9286-40-git-send-email-nick.dyer@itdev.co.uk>
On Thu, Jun 27, 2013 at 01:49:14PM +0100, Nick Dyer wrote:
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> Acked-by: Benson Leung <bleung@chromium.org>
> ---
> drivers/input/touchscreen/atmel_mxt_ts.c | 95 +++++++++++++++++++++++++++++-
> 1 file changed, 94 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index b63f227..2e6118a 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -78,6 +78,7 @@
> #define MXT_SPT_DIGITIZER_T43 43
> #define MXT_SPT_MESSAGECOUNT_T44 44
> #define MXT_SPT_CTECONFIG_T46 46
> +#define MXT_PROCI_ACTIVE_STYLUS_T63 63
>
> /* MXT_GEN_MESSAGE_T5 object */
> #define MXT_RPTID_NOMSG 0xff
> @@ -190,6 +191,19 @@ struct t9_range {
> /* Define for MXT_PROCI_TOUCHSUPPRESSION_T42 */
> #define MXT_T42_MSG_TCHSUP (1 << 0)
>
> +/* T63 Stylus */
> +#define MXT_T63_STYLUS_PRESS (1 << 0)
> +#define MXT_T63_STYLUS_RELEASE (1 << 1)
> +#define MXT_T63_STYLUS_MOVE (1 << 2)
> +#define MXT_T63_STYLUS_SUPPRESS (1 << 3)
> +
> +#define MXT_T63_STYLUS_DETECT (1 << 4)
> +#define MXT_T63_STYLUS_TIP (1 << 5)
> +#define MXT_T63_STYLUS_ERASER (1 << 6)
> +#define MXT_T63_STYLUS_BARREL (1 << 7)
> +
> +#define MXT_T63_STYLUS_PRESSURE_MASK 0x3F
> +
> /* Delay times */
> #define MXT_BACKUP_TIME 50 /* msec */
> #define MXT_RESET_TIME 200 /* msec */
> @@ -260,6 +274,7 @@ struct mxt_data {
> bool update_input;
> u8 last_message_count;
> u8 num_touchids;
> + u8 num_stylusids;
>
> /* Cached parameters from object table */
> u16 T5_address;
> @@ -274,6 +289,8 @@ struct mxt_data {
> u8 T42_reportid_max;
> u16 T44_address;
> u8 T48_reportid;
> + u8 T63_reportid_min;
> + u8 T63_reportid_max;
>
> /* for fw update in bootloader */
> struct completion bl_completion;
> @@ -823,6 +840,63 @@ static int mxt_proc_t48_messages(struct mxt_data *data, u8 *msg)
> return 0;
> }
>
> +static void mxt_proc_t63_messages(struct mxt_data *data, u8 *msg)
> +{
> + struct device *dev = &data->client->dev;
> + struct input_dev *input_dev = data->input_dev;
> + u8 id;
> + u16 x, y;
> + u8 pressure;
> +
> + /* do not report events if input device not yet registered */
> + if (!data->enable_reporting)
> + return;
> +
> + /* stylus slots come after touch slots */
> + id = data->num_touchids + (msg[0] - data->T63_reportid_min);
> +
> + if (id < 0 || id > (data->num_touchids + data->num_stylusids)) {
> + dev_err(dev, "invalid stylus id %d, max slot is %d\n",
> + id, data->num_stylusids);
> + return;
> + }
> +
> + x = msg[3] | (msg[4] << 8);
> + y = msg[5] | (msg[6] << 8);
> + pressure = msg[7] & MXT_T63_STYLUS_PRESSURE_MASK;
> +
> + dev_dbg(dev,
> + "[%d] %c%c%c%c x: %d y: %d pressure: %d stylus:%c%c%c%c\n",
> + id,
> + (msg[1] & MXT_T63_STYLUS_SUPPRESS) ? 'S' : '.',
> + (msg[1] & MXT_T63_STYLUS_MOVE) ? 'M' : '.',
> + (msg[1] & MXT_T63_STYLUS_RELEASE) ? 'R' : '.',
> + (msg[1] & MXT_T63_STYLUS_PRESS) ? 'P' : '.',
> + x, y, pressure,
> + (msg[2] & MXT_T63_STYLUS_BARREL) ? 'B' : '.',
> + (msg[2] & MXT_T63_STYLUS_ERASER) ? 'E' : '.',
> + (msg[2] & MXT_T63_STYLUS_TIP) ? 'T' : '.',
> + (msg[2] & MXT_T63_STYLUS_DETECT) ? 'D' : '.');
> +
> + input_mt_slot(input_dev, id);
> +
> + if (msg[2] & MXT_T63_STYLUS_DETECT) {
> + input_mt_report_slot_state(input_dev, MT_TOOL_PEN, 1);
> + input_report_abs(input_dev, ABS_MT_POSITION_X, x);
> + input_report_abs(input_dev, ABS_MT_POSITION_Y, y);
> + input_report_abs(input_dev, ABS_MT_PRESSURE, pressure);
> + } else {
> + input_mt_report_slot_state(input_dev, MT_TOOL_PEN, 0);
> + }
> +
> + input_report_key(input_dev, BTN_STYLUS,
> + (msg[2] & MXT_T63_STYLUS_ERASER));
> + input_report_key(input_dev, BTN_STYLUS2,
> + (msg[2] & MXT_T63_STYLUS_BARREL));
> +
> + mxt_input_sync(input_dev);
> +}
> +
> static int mxt_proc_message(struct mxt_data *data, u8 *message)
> {
> u8 report_id = message[0];
> @@ -838,6 +912,9 @@ static int mxt_proc_message(struct mxt_data *data, u8 *message)
> } else if (report_id == data->T19_reportid) {
> mxt_input_button(data, message);
> data->update_input = true;
> + } else if (report_id >= data->T63_reportid_min
> + && report_id <= data->T63_reportid_max) {
> + mxt_proc_t63_messages(data, message);
> } else if (report_id >= data->T42_reportid_min
> && report_id <= data->T42_reportid_max) {
> mxt_proc_t42_messages(data, message);
> @@ -1483,6 +1560,8 @@ static void mxt_free_object_table(struct mxt_data *data)
> data->T42_reportid_max = 0;
> data->T44_address = 0;
> data->T48_reportid = 0;
> + data->T63_reportid_min = 0;
> + data->T63_reportid_max = 0;
> data->max_reportid = 0;
> }
>
> @@ -1563,6 +1642,12 @@ static int mxt_get_object_table(struct mxt_data *data)
> case MXT_PROCG_NOISESUPPRESSION_T48:
> data->T48_reportid = min_id;
> break;
> + case MXT_PROCI_ACTIVE_STYLUS_T63:
> + data->T63_reportid_min = min_id;
> + data->T63_reportid_max = max_id;
> + data->num_stylusids = object->num_report_ids
> + * mxt_obj_instances(object);
> + break;
> }
>
> end_address = object->start_address
> @@ -2103,7 +2188,7 @@ static int mxt_initialize_t9_input_device(struct mxt_data *data)
> 0, 255, 0, 0);
>
> /* For multi touch */
> - num_mt_slots = data->T9_reportid_max - data->T9_reportid_min + 1;
> + num_mt_slots = data->num_touchids + data->num_stylusids;
> error = input_mt_init_slots(input_dev, num_mt_slots, mt_flags);
> if (error) {
> dev_err(dev, "Error %d initialising slots\n", error);
> @@ -2121,6 +2206,14 @@ static int mxt_initialize_t9_input_device(struct mxt_data *data)
> input_set_abs_params(input_dev, ABS_MT_ORIENTATION,
> 0, 255, 0, 0);
>
> + /* For T63 active stylus */
> + if (data->T63_reportid_min) {
> + input_set_capability(input_dev, EV_KEY, BTN_STYLUS);
> + input_set_capability(input_dev, EV_KEY, BTN_STYLUS2);
> + input_set_abs_params(input_dev, ABS_MT_TOOL_TYPE,
> + 0, MT_TOOL_MAX, 0, 0);
> + }
> +
> input_set_drvdata(input_dev, data);
>
> error = input_register_device(input_dev);
> --
> 1.7.10.4
>
Reviewed-by: Henrik Rydberg <rydberg@euromail.se>
Thanks,
Henrik
^ permalink raw reply
* Re: [PATCH 37/51] Input: atmel_mxt_ts - Implement vector/orientation support
From: rydberg @ 2013-07-18 17:20 UTC (permalink / raw)
To: Nick Dyer
Cc: Dmitry Torokhov, Daniel Kurtz, Joonyoung Shim, Alan Bowens,
linux-input, linux-kernel, Peter Meerwald, Benson Leung,
Olof Johansson
In-Reply-To: <1372337366-9286-38-git-send-email-nick.dyer@itdev.co.uk>
On Thu, Jun 27, 2013 at 01:49:12PM +0100, Nick Dyer wrote:
> The atmel touch messages contain orientation information as a byte in a packed
> format which can be passed straight on to Android if the input device
> configuration is correct, see
> http://source.android.com/tech/input/touch-devices.html#touchorientationcalibration
>
> This requires vector reports to be enabled in maXTouch config (zero DISVECT
> bit in T9 CTRL field)
>
> Android converts the format in frameworks/base/services/input/Input.cpp,
> search for ORIENTATION_CALIBRATION_VECTOR.
How does this compare to the input mt documentation?
>
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> Acked-by: Benson Leung <bleung@chromium.org>
> ---
> drivers/input/touchscreen/atmel_mxt_ts.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 1c5e640..9188cf7 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -716,6 +716,7 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
> int y;
> int area;
> int amplitude;
> + u8 vector;
>
> /* do not report events if input device not yet registered */
> if (!data->enable_reporting)
> @@ -734,9 +735,10 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
>
> area = message[5];
> amplitude = message[6];
> + vector = message[7];
>
> dev_dbg(dev,
> - "[%u] %c%c%c%c%c%c%c%c x: %5u y: %5u area: %3u amp: %3u\n",
> + "[%u] %c%c%c%c%c%c%c%c x: %5u y: %5u area: %3u amp: %3u vector: %02X\n",
> id,
> (status & MXT_T9_DETECT) ? 'D' : '.',
> (status & MXT_T9_PRESS) ? 'P' : '.',
> @@ -746,7 +748,7 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
> (status & MXT_T9_AMP) ? 'A' : '.',
> (status & MXT_T9_SUPPRESS) ? 'S' : '.',
> (status & MXT_T9_UNGRIP) ? 'U' : '.',
> - x, y, area, amplitude);
> + x, y, area, amplitude, vector);
>
> input_mt_slot(input_dev, id);
>
> @@ -766,6 +768,7 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
> input_report_abs(input_dev, ABS_MT_POSITION_Y, y);
> input_report_abs(input_dev, ABS_MT_PRESSURE, amplitude);
> input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, area);
> + input_report_abs(input_dev, ABS_MT_ORIENTATION, vector);
> } else {
> /* Touch no longer active, close out slot */
> input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, 0);
> @@ -2100,6 +2103,8 @@ static int mxt_initialize_t9_input_device(struct mxt_data *data)
> 0, data->max_y, 0, 0);
> input_set_abs_params(input_dev, ABS_MT_PRESSURE,
> 0, 255, 0, 0);
> + input_set_abs_params(input_dev, ABS_MT_ORIENTATION,
> + 0, 255, 0, 0);
>
> input_set_drvdata(input_dev, data);
>
> --
> 1.7.10.4
>
Thanks,
Henrik
^ permalink raw reply
* Re: [PATCH 26/51] Input: atmel_mxt_ts - Move input device init into separate function
From: rydberg @ 2013-07-18 17:20 UTC (permalink / raw)
To: Nick Dyer
Cc: Dmitry Torokhov, Daniel Kurtz, Joonyoung Shim, Alan Bowens,
linux-input, linux-kernel, Peter Meerwald, Benson Leung,
Olof Johansson
In-Reply-To: <1372337366-9286-27-git-send-email-nick.dyer@itdev.co.uk>
On Thu, Jun 27, 2013 at 01:49:01PM +0100, Nick Dyer wrote:
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> ---
> drivers/input/touchscreen/atmel_mxt_ts.c | 129 +++++++++++++++++-------------
> 1 file changed, 75 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 8632133..030ebc5 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -1728,73 +1728,39 @@ static int mxt_handle_pdata(struct mxt_data *data)
> return 0;
> }
>
> -static int mxt_probe(struct i2c_client *client,
> - const struct i2c_device_id *id)
> +static int mxt_initialize_t9_input_device(struct mxt_data *data)
> {
> - struct mxt_data *data;
> + struct device *dev = &data->client->dev;
> + const struct mxt_platform_data *pdata = data->pdata;
Similar code was removed from mxt_initialize(), some consistency would be nice.
> struct input_dev *input_dev;
> int error;
> unsigned int num_mt_slots;
> unsigned int mt_flags = 0;
> int i;
>
> - data = kzalloc(sizeof(struct mxt_data), GFP_KERNEL);
> input_dev = input_allocate_device();
> - if (!data || !input_dev) {
> - dev_err(&client->dev, "Failed to allocate memory\n");
> - error = -ENOMEM;
> - goto err_free_mem;
> + if (!input_dev) {
> + dev_err(dev, "Failed to allocate memory\n");
> + return -ENOMEM;
> }
>
> input_dev->name = "Atmel maXTouch Touchscreen";
> - snprintf(data->phys, sizeof(data->phys), "i2c-%u-%04x/input0",
> - client->adapter->nr, client->addr);
> -
> input_dev->phys = data->phys;
> -
> input_dev->id.bustype = BUS_I2C;
> - input_dev->dev.parent = &client->dev;
> + input_dev->dev.parent = dev;
> input_dev->open = mxt_input_open;
> input_dev->close = mxt_input_close;
>
> - data->client = client;
> - data->input_dev = input_dev;
> - data->irq = client->irq;
> - i2c_set_clientdata(client, data);
> -
> - error = mxt_handle_pdata(data);
> - if (error)
> - goto err_free_mem;
> -
> - init_completion(&data->bl_completion);
> - init_completion(&data->reset_completion);
> - init_completion(&data->crc_completion);
> -
> - error = request_threaded_irq(data->irq, NULL, mxt_interrupt,
> - data->pdata->irqflags | IRQF_ONESHOT,
> - client->name, data);
> - if (error) {
> - dev_err(&client->dev, "Failed to register interrupt\n");
> - goto err_free_pdata;
> - }
> -
> - disable_irq(client->irq);
> -
> - error = mxt_initialize(data);
> - if (error)
> - goto err_free_irq;
> -
> __set_bit(EV_ABS, input_dev->evbit);
> - __set_bit(EV_KEY, input_dev->evbit);
> - __set_bit(BTN_TOUCH, input_dev->keybit);
> + input_set_capability(input_dev, EV_KEY, BTN_TOUCH);
>
> - if (data->pdata->t19_num_keys) {
> + if (pdata->t19_num_keys) {
> __set_bit(INPUT_PROP_BUTTONPAD, input_dev->propbit);
>
> - for (i = 0; i < data->pdata->t19_num_keys; i++)
> - if (data->pdata->t19_keymap[i] != KEY_RESERVED)
> + for (i = 0; i < pdata->t19_num_keys; i++)
> + if (pdata->t19_keymap[i] != KEY_RESERVED)
> input_set_capability(input_dev, EV_KEY,
> - data->pdata->t19_keymap[i]);
> + pdata->t19_keymap[i]);
>
> mt_flags |= INPUT_MT_POINTER;
>
> @@ -1819,8 +1785,11 @@ static int mxt_probe(struct i2c_client *client,
> /* For multi touch */
> num_mt_slots = data->T9_reportid_max - data->T9_reportid_min + 1;
> error = input_mt_init_slots(input_dev, num_mt_slots, mt_flags);
> - if (error)
> - goto err_free_object;
> + if (error) {
> + dev_err(dev, "Error %d initialising slots\n", error);
> + goto err_free_mem;
> + }
> +
> input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR,
> 0, MXT_MAX_AREA, 0, 0);
> input_set_abs_params(input_dev, ABS_MT_POSITION_X,
> @@ -1834,11 +1803,64 @@ static int mxt_probe(struct i2c_client *client,
>
> error = input_register_device(input_dev);
> if (error) {
> - dev_err(&client->dev, "Error %d registering input device\n",
> - error);
> - goto err_free_object;
> + dev_err(dev, "Error %d registering input device\n", error);
> + goto err_free_mem;
> }
>
> + data->input_dev = input_dev;
> +
> + return 0;
> +
> +err_free_mem:
> + input_free_device(input_dev);
> + return error;
> +}
> +
> +static int mxt_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct mxt_data *data;
> + int error;
> +
> + data = kzalloc(sizeof(struct mxt_data), GFP_KERNEL);
> + if (!data) {
> + dev_err(&client->dev, "Failed to allocate memory\n");
> + return -ENOMEM;
> + }
> +
> + snprintf(data->phys, sizeof(data->phys), "i2c-%u-%04x/input0",
> + client->adapter->nr, client->addr);
> +
> + data->client = client;
> + data->irq = client->irq;
> + i2c_set_clientdata(client, data);
> +
> + error = mxt_handle_pdata(data);
> + if (error)
> + goto err_free_mem;
> +
> + init_completion(&data->bl_completion);
> + init_completion(&data->reset_completion);
> + init_completion(&data->crc_completion);
> +
> + error = request_threaded_irq(data->irq, NULL, mxt_interrupt,
> + data->pdata->irqflags | IRQF_ONESHOT,
> + client->name, data);
> + if (error) {
> + dev_err(&client->dev, "Failed to register interrupt\n");
> + goto err_free_pdata;
> + }
> +
> + disable_irq(data->irq);
> +
> + error = mxt_initialize(data);
> + if (error)
> + goto err_free_irq;
> +
> + error = mxt_initialize_t9_input_device(data);
> + if (error)
> + goto err_free_object;
> +
> error = sysfs_create_group(&client->dev.kobj, &mxt_attr_group);
> if (error) {
> dev_err(&client->dev, "Failure %d creating sysfs group\n",
> @@ -1849,8 +1871,8 @@ static int mxt_probe(struct i2c_client *client,
> return 0;
>
> err_unregister_device:
> - input_unregister_device(input_dev);
> - input_dev = NULL;
> + input_unregister_device(data->input_dev);
> + data->input_dev = NULL;
> err_free_object:
> kfree(data->object_table);
> err_free_irq:
> @@ -1859,7 +1881,6 @@ err_free_pdata:
> if (!dev_get_platdata(&data->client->dev))
> kfree(data->pdata);
> err_free_mem:
> - input_free_device(input_dev);
> kfree(data);
> return error;
> }
> --
> 1.7.10.4
>
In what way does this patch simplify things?
Thanks,
Henrik
^ permalink raw reply
* Re: [PATCH 25/51] Input: atmel_mxt_ts - Handle multiple input reports in one message
From: rydberg @ 2013-07-18 17:18 UTC (permalink / raw)
To: Nick Dyer
Cc: Dmitry Torokhov, Daniel Kurtz, Joonyoung Shim, Alan Bowens,
linux-input, linux-kernel, Peter Meerwald, Benson Leung,
Olof Johansson
In-Reply-To: <1372337366-9286-26-git-send-email-nick.dyer@itdev.co.uk>
Hi Nick,
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> Acked-by: Benson Leung <bleung@chromium.org>
> ---
> drivers/input/touchscreen/atmel_mxt_ts.c | 34 +++++++++++++++++++++++-------
> 1 file changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 5a16383..8632133 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -628,6 +628,12 @@ static void mxt_input_button(struct mxt_data *data, struct mxt_message *message)
> }
> }
>
> +static void mxt_input_sync(struct input_dev *input_dev)
> +{
> + input_mt_report_pointer_emulation(input_dev, false);
> + input_sync(input_dev);
> +}
Why not handle the enable_reporting and update_input logic here as
well? The logic is inconsistent with the rest of the patchset.
> +
> static void mxt_input_touchevent(struct mxt_data *data,
> struct mxt_message *message, int id)
> {
> @@ -645,10 +651,12 @@ static void mxt_input_touchevent(struct mxt_data *data,
>
> x = (message->message[1] << 4) | ((message->message[3] >> 4) & 0xf);
> y = (message->message[2] << 4) | ((message->message[3] & 0xf));
> +
> + /* Handle 10/12 bit switching */
> if (data->max_x < 1024)
> - x = x >> 2;
> + x >>= 2;
> if (data->max_y < 1024)
> - y = y >> 2;
> + y >>= 2;
Unrelated changes.
>
> area = message->message[4];
> amplitude = message->message[5];
> @@ -667,14 +675,26 @@ static void mxt_input_touchevent(struct mxt_data *data,
> x, y, area, amplitude);
>
> input_mt_slot(input_dev, id);
> - input_mt_report_slot_state(input_dev, MT_TOOL_FINGER,
> - status & MXT_T9_DETECT);
>
> if (status & MXT_T9_DETECT) {
> + /* Multiple bits may be set if the host is slow to read the
> + * status messages, indicating all the events that have
> + * happened */
> + if (status & MXT_T9_RELEASE) {
> + input_mt_report_slot_state(input_dev,
> + MT_TOOL_FINGER, 0);
> + mxt_input_sync(input_dev);
What are the guarantees that nobody else expects the frame to not be
cut off here? What is the update_input state after this operation?
> + }
> +
> + /* Touch active */
> + input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, 1);
> input_report_abs(input_dev, ABS_MT_POSITION_X, x);
> input_report_abs(input_dev, ABS_MT_POSITION_Y, y);
> input_report_abs(input_dev, ABS_MT_PRESSURE, amplitude);
> input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, area);
> + } else {
> + /* Touch no longer active, close out slot */
> + input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, 0);
> }
> }
>
> @@ -732,10 +752,8 @@ static irqreturn_t mxt_process_messages_until_invalid(struct mxt_data *data)
> }
> } while (reportid != 0xff);
>
> - if (data->enable_reporting && update_input) {
> - input_mt_report_pointer_emulation(data->input_dev, false);
> - input_sync(data->input_dev);
> - }
> + if (data->enable_reporting && update_input)
> + mxt_input_sync(data->input_dev);
>
> return IRQ_HANDLED;
> }
> --
> 1.7.10.4
>
Thanks,
Henrik
^ permalink raw reply
* Re: [PATCH 20/51] Input: atmel_mxt_ts - Set default irqflags when there is no pdata
From: rydberg @ 2013-07-18 17:17 UTC (permalink / raw)
To: Nick Dyer
Cc: Dmitry Torokhov, Daniel Kurtz, Joonyoung Shim, Alan Bowens,
linux-input, linux-kernel, Peter Meerwald, Benson Leung,
Olof Johansson, Yufeng Shen
In-Reply-To: <1372337366-9286-21-git-send-email-nick.dyer@itdev.co.uk>
Hi Nick,
> From: Yufeng Shen <miletus@chromium.org>
>
> This is the preparation for supporting the code path when there is
> platform data provided and still boot the device into a sane state
> with backup NVRAM config.
>
> Make the irqflags default to be IRQF_TRIGGER_FALLING if no platform data is
> provided.
>
> Signed-off-by: Yufeng Shen <miletus@chromium.org>
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> Acked-by: Benson Leung <bleung@chromium.org>
> ---
> drivers/input/touchscreen/atmel_mxt_ts.c | 54 +++++++++++++++++++++---------
> 1 file changed, 39 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 1334e5b..2645d36 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -232,7 +232,7 @@ struct mxt_data {
> struct i2c_client *client;
> struct input_dev *input_dev;
> char phys[64]; /* device physical location */
> - const struct mxt_platform_data *pdata;
> + struct mxt_platform_data *pdata;
> struct mxt_object *object_table;
> struct mxt_info info;
> unsigned int irq;
> @@ -1640,10 +1640,29 @@ static void mxt_input_close(struct input_dev *dev)
> mxt_stop(data);
> }
>
> +static int mxt_handle_pdata(struct mxt_data *data)
> +{
> + data->pdata = dev_get_platdata(&data->client->dev);
> +
> + /* Use provided platform data if present */
> + if (data->pdata)
> + return 0;
> +
> + data->pdata = kzalloc(sizeof(*data->pdata), GFP_KERNEL);
> + if (!data->pdata) {
> + dev_err(&data->client->dev, "Failed to allocate pdata\n");
> + return -ENOMEM;
> + }
Any chance this could be a static instead?
> +
> + /* Set default parameters */
> + data->pdata->irqflags = IRQF_TRIGGER_FALLING;
> +
> + return 0;
> +}
> +
Opencode instead?
> static int mxt_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> - const struct mxt_platform_data *pdata = client->dev.platform_data;
This line keeps reappearing in various versions of this
function. Perhaps it should simply stay as is instead?
> struct mxt_data *data;
> struct input_dev *input_dev;
> int error;
> @@ -1651,9 +1670,6 @@ static int mxt_probe(struct i2c_client *client,
> unsigned int mt_flags = 0;
> int i;
>
> - if (!pdata)
Why not just initialize the default here instead?
> - return -EINVAL;
> -
> data = kzalloc(sizeof(struct mxt_data), GFP_KERNEL);
> input_dev = input_allocate_device();
> if (!data || !input_dev) {
> @@ -1675,19 +1691,23 @@ static int mxt_probe(struct i2c_client *client,
>
> data->client = client;
> data->input_dev = input_dev;
> - data->pdata = pdata;
> data->irq = client->irq;
> + i2c_set_clientdata(client, data);
> +
> + error = mxt_handle_pdata(data);
> + if (error)
> + goto err_free_mem;
then this would go away
>
> init_completion(&data->bl_completion);
> init_completion(&data->reset_completion);
> init_completion(&data->crc_completion);
>
> - error = request_threaded_irq(client->irq, NULL, mxt_interrupt,
> - pdata->irqflags | IRQF_ONESHOT,
> + error = request_threaded_irq(data->irq, NULL, mxt_interrupt,
> + data->pdata->irqflags | IRQF_ONESHOT,
> client->name, data);
and this hunk
> if (error) {
> dev_err(&client->dev, "Failed to register interrupt\n");
> - goto err_free_mem;
> + goto err_free_pdata;
> }
>
> disable_irq(client->irq);
> @@ -1700,13 +1720,13 @@ static int mxt_probe(struct i2c_client *client,
> __set_bit(EV_KEY, input_dev->evbit);
> __set_bit(BTN_TOUCH, input_dev->keybit);
>
> - if (pdata->t19_num_keys) {
> + if (data->pdata->t19_num_keys) {
and this hunk
> __set_bit(INPUT_PROP_BUTTONPAD, input_dev->propbit);
>
> - for (i = 0; i < pdata->t19_num_keys; i++)
> - if (pdata->t19_keymap[i] != KEY_RESERVED)
> + for (i = 0; i < data->pdata->t19_num_keys; i++)
> + if (data->pdata->t19_keymap[i] != KEY_RESERVED)
> input_set_capability(input_dev, EV_KEY,
> - pdata->t19_keymap[i]);
> + data->pdata->t19_keymap[i]);
>
> mt_flags |= INPUT_MT_POINTER;
>
> @@ -1743,7 +1763,6 @@ static int mxt_probe(struct i2c_client *client,
> 0, 255, 0, 0);
>
> input_set_drvdata(input_dev, data);
> - i2c_set_clientdata(client, data);
>
> error = input_register_device(input_dev);
> if (error) {
> @@ -1767,7 +1786,10 @@ err_unregister_device:
> err_free_object:
> kfree(data->object_table);
> err_free_irq:
> - free_irq(client->irq, data);
> + free_irq(data->irq, data);
> +err_free_pdata:
> + if (!dev_get_platdata(&data->client->dev))
> + kfree(data->pdata);
> err_free_mem:
> input_free_device(input_dev);
> kfree(data);
> @@ -1782,6 +1804,8 @@ static int mxt_remove(struct i2c_client *client)
> free_irq(data->irq, data);
> input_unregister_device(data->input_dev);
> kfree(data->object_table);
> + if (!dev_get_platdata(&data->client->dev))
> + kfree(data->pdata);
Shared ownership should perhaps be signalled in a more robust way?
> kfree(data);
>
> return 0;
> --
> 1.7.10.4
>
Thanks,
Henrik
^ permalink raw reply
* Re: [PATCH 08/51] Input: atmel_mxt_ts - Initialise IRQ before probing
From: rydberg @ 2013-07-18 17:13 UTC (permalink / raw)
To: Nick Dyer
Cc: Dmitry Torokhov, Daniel Kurtz, Joonyoung Shim, Alan Bowens,
linux-input, linux-kernel, Peter Meerwald, Benson Leung,
Olof Johansson
In-Reply-To: <1372337366-9286-9-git-send-email-nick.dyer@itdev.co.uk>
Hi Nick,
> This allows the interrupt handler to be used to detect CHG line during config
> download. We need to make sure we do not report events if input device not yet
> registered.
>
> data->enable_reporting is checked in each of the possible message handling
> function paths rather than higher up (such as at mxt_proc_message) because
> some objects may be used for completions (T6).
>
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> Acked-by: Benson Leung <bleung@chromium.org>
> ---
> drivers/input/touchscreen/atmel_mxt_ts.c | 73 +++++++++++++++++++++---------
> 1 file changed, 51 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 55d7667..1e24e54 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -2,6 +2,7 @@
> * Atmel maXTouch Touchscreen driver
> *
> * Copyright (C) 2010 Samsung Electronics Co.Ltd
> + * Copyright (C) 2011-2012 Atmel Corporation
> * Copyright (C) 2012 Google, Inc.
> *
> * Author: Joonyoung Shim <jy0922.shim@samsung.com>
> @@ -261,6 +262,9 @@ struct mxt_data {
>
> /* for fw update in bootloader */
> struct completion bl_completion;
> +
> + /* Enable reporting of input events */
> + bool enable_reporting;
> };
>
> static inline size_t mxt_obj_size(const struct mxt_object *obj)
> @@ -568,6 +572,10 @@ static void mxt_input_button(struct mxt_data *data, struct mxt_message *message)
> bool button;
> int i;
>
> + /* do not report events if input device not yet registered */
> + if (!data->enable_reporting)
> + return;
Regardless of the rationale, this test seems to create complex logic
further down the patchset. Any chance it could be done differently?
> +
> /* Active-low switch */
> for (i = 0; i < pdata->t19_num_keys; i++) {
> if (pdata->t19_keymap[i] == KEY_RESERVED)
> @@ -588,6 +596,10 @@ static void mxt_input_touchevent(struct mxt_data *data,
> int area;
> int pressure;
>
> + /* do not report events if input device not yet registered */
> + if (!data->enable_reporting)
> + return;
> +
> x = (message->message[1] << 4) | ((message->message[3] >> 4) & 0xf);
> y = (message->message[2] << 4) | ((message->message[3] & 0xf));
> if (data->max_x < 1024)
> @@ -667,7 +679,7 @@ static irqreturn_t mxt_process_messages_until_invalid(struct mxt_data *data)
> }
> } while (reportid != 0xff);
>
> - if (update_input) {
> + if (data->enable_reporting && update_input) {
> input_mt_report_pointer_emulation(data->input_dev, false);
> input_sync(data->input_dev);
> }
> @@ -685,6 +697,9 @@ static irqreturn_t mxt_interrupt(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> + if (!data->object_table)
> + return IRQ_NONE;
> +
> return mxt_process_messages_until_invalid(data);
> }
>
> @@ -746,6 +761,19 @@ static int mxt_make_highchg(struct mxt_data *data)
> return 0;
> }
>
> +static int mxt_acquire_irq(struct mxt_data *data)
> +{
> + int error;
> +
> + enable_irq(data->irq);
> +
> + error = mxt_make_highchg(data);
> + if (error)
> + return error;
> +
> + return 0;
> +}
> +
Strange error handling; the name suggests that an error means the irq
has not been enabled, which it clearly does anyways. Also, the error
is not checked everywhere later in the patchset. Opencode instead?
> static int mxt_get_info(struct mxt_data *data)
> {
> struct i2c_client *client = data->client;
> @@ -819,6 +847,7 @@ static void mxt_free_object_table(struct mxt_data *data)
> {
> kfree(data->object_table);
> data->object_table = NULL;
> + data->enable_reporting = false;
> data->T6_reportid = 0;
> data->T9_reportid_min = 0;
> data->T9_reportid_max = 0;
> @@ -849,6 +878,10 @@ static int mxt_initialize(struct mxt_data *data)
> if (error)
> goto err_free_object_table;
>
> + error = mxt_acquire_irq(data);
> + if (error)
> + goto err_free_object_table;
> +
> /* Check register init values */
> error = mxt_check_reg_init(data);
> if (error)
> @@ -886,6 +919,8 @@ static int mxt_initialize(struct mxt_data *data)
> info->matrix_xsize, info->matrix_ysize,
> info->object_num);
>
> + data->enable_reporting = true;
> +
> return 0;
>
> err_free_object_table:
> @@ -1087,11 +1122,7 @@ static ssize_t mxt_update_fw_store(struct device *dev,
> dev_dbg(dev, "The firmware update succeeded\n");
> mxt_free_object_table(data);
>
> - mxt_initialize(data);
> -
> - enable_irq(data->irq);
> -
> - error = mxt_make_highchg(data);
> + error = mxt_initialize(data);
> if (error)
> return error;
> }
> @@ -1188,9 +1219,19 @@ static int mxt_probe(struct i2c_client *client,
>
> mxt_calc_resolution(data);
>
> + error = request_threaded_irq(client->irq, NULL, mxt_interrupt,
> + pdata->irqflags | IRQF_ONESHOT,
> + client->name, data);
> + if (error) {
> + dev_err(&client->dev, "Failed to register interrupt\n");
> + goto err_free_mem;
> + }
> +
> + disable_irq(client->irq);
It is unclear what portion of the code should be executed with irqs
off. A better framing of the sensitive code path would be nice.
> +
> error = mxt_initialize(data);
> if (error)
> - goto err_free_mem;
> + goto err_free_irq;
>
> __set_bit(EV_ABS, input_dev->evbit);
> __set_bit(EV_KEY, input_dev->evbit);
> @@ -1241,21 +1282,9 @@ static int mxt_probe(struct i2c_client *client,
> input_set_drvdata(input_dev, data);
> i2c_set_clientdata(client, data);
>
> - error = request_threaded_irq(client->irq, NULL, mxt_interrupt,
> - pdata->irqflags | IRQF_ONESHOT,
> - client->name, data);
> - if (error) {
> - dev_err(&client->dev, "Failed to register interrupt\n");
> - goto err_free_object;
> - }
> -
> - error = mxt_make_highchg(data);
> - if (error)
> - goto err_free_irq;
> -
> error = input_register_device(input_dev);
> if (error)
> - goto err_free_irq;
> + goto err_free_object;
>
> error = sysfs_create_group(&client->dev.kobj, &mxt_attr_group);
> if (error)
> @@ -1266,10 +1295,10 @@ static int mxt_probe(struct i2c_client *client,
> err_unregister_device:
> input_unregister_device(input_dev);
> input_dev = NULL;
> -err_free_irq:
> - free_irq(client->irq, data);
> err_free_object:
> kfree(data->object_table);
> +err_free_irq:
> + free_irq(client->irq, data);
> err_free_mem:
> input_free_device(input_dev);
> kfree(data);
> --
> 1.7.10.4
>
Thanks,
Henrik
^ permalink raw reply
* Re: [PATCH 02/51] Input: atmel_mxt_ts - Improve T19 GPIO keys handling
From: rydberg @ 2013-07-18 17:11 UTC (permalink / raw)
To: Nick Dyer
Cc: Dmitry Torokhov, Daniel Kurtz, Joonyoung Shim, Alan Bowens,
linux-input, linux-kernel, Peter Meerwald, Benson Leung,
Olof Johansson
In-Reply-To: <1372337366-9286-3-git-send-email-nick.dyer@itdev.co.uk>
On Thu, Jun 27, 2013 at 01:48:37PM +0100, Nick Dyer wrote:
>
> * The mapping of the GPIO numbers into the T19 status byte varies between
> different maXTouch chips. Some have up to 7 GPIOs. Allowing a keycode array
> of up to 8 items is simpler and more generic. So replace #define with
> configurable number of keys which also allows the removal of is_tp.
> * Rename platform data parameters to include "t19" to prevent confusion with
> T15 key array.
> * Probe aborts early on when pdata is NULL, so no need to check.
> * Move "int i" to beginning of function (mixed declarations and code)
> * Use API calls rather than __set_bit()
> * Remove unused dev variable.
>
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> ---
> drivers/input/touchscreen/atmel_mxt_ts.c | 44 ++++++++++++------------------
> drivers/platform/x86/chromeos_laptop.c | 17 ++++++++----
> include/linux/i2c/atmel_mxt_ts.h | 7 ++---
> 3 files changed, 30 insertions(+), 38 deletions(-)
Reviewed-by: Henrik Rydberg <rydberg@euromail.se>
Thanks,
Henrik
^ permalink raw reply
* [PATCH 1/2] Revert "Revert "HID: Fix logitech-dj: missing Unifying device issue""
From: Nestor Lopez Casado @ 2013-07-18 13:21 UTC (permalink / raw)
To: jkosina
Cc: benjamin.tissoires, adlr, joseph.salisbury, linux-input,
linux-kernel, Nestor Lopez Casado
This reverts commit 8af6c08830b1ae114d1a8b548b1f8b056e068887.
This patch re-adds the workaround introduced by 596264082f10dd4
which was reverted by 8af6c08830b1ae114.
The original patch 596264 was needed to overcome a situation where
the hid-core would drop incoming reports while probe() was being
executed.
This issue was solved by c849a6143bec520af which added
hid_device_io_start() and hid_device_io_stop() that enable a specific
hid driver to opt-in for input reports while its probe() is being
executed.
Commit a9dd22b730857347 modified hid-logitech-dj so as to use the
functionality added to hid-core. Having done that, workaround 596264
was no longer necessary and was reverted by 8af6c08.
We now encounter a different problem that ends up 'again' thwarting
the Unifying receiver enumeration. The problem is time and usb controller
dependent. Ocasionally the reports sent to the usb receiver to start
the paired devices enumeration fail with -EPIPE and the receiver never
gets to enumerate the paired devices.
With dcd9006b1b053c7b1c the problem was "hidden" as the call to the usb
driver became asynchronous and none was catching the error from the
failing URB.
As the root cause for this failing SET_REPORT is not understood yet,
-possibly a race on the usb controller drivers or a problem with the
Unifying receiver- reintroducing this workaround solves the problem.
Overall what this workaround does is: If an input report from an
unknown device is received, then a (re)enumeration is performed.
related bug:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1194649
Signed-off-by: Nestor Lopez Casado <nlopezcasad@logitech.com>
---
drivers/hid/hid-logitech-dj.c | 45 +++++++++++++++++++++++++++++++++++++++++
drivers/hid/hid-logitech-dj.h | 1 +
2 files changed, 46 insertions(+)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index db3192b..0d13389 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -192,6 +192,7 @@ static struct hid_ll_driver logi_dj_ll_driver;
static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
size_t count,
unsigned char report_type);
+static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev);
static void logi_dj_recv_destroy_djhid_device(struct dj_receiver_dev *djrcv_dev,
struct dj_report *dj_report)
@@ -232,6 +233,7 @@ static void logi_dj_recv_add_djhid_device(struct dj_receiver_dev *djrcv_dev,
if (dj_report->report_params[DEVICE_PAIRED_PARAM_SPFUNCTION] &
SPFUNCTION_DEVICE_LIST_EMPTY) {
dbg_hid("%s: device list is empty\n", __func__);
+ djrcv_dev->querying_devices = false;
return;
}
@@ -242,6 +244,12 @@ static void logi_dj_recv_add_djhid_device(struct dj_receiver_dev *djrcv_dev,
return;
}
+ if (djrcv_dev->paired_dj_devices[dj_report->device_index]) {
+ /* The device is already known. No need to reallocate it. */
+ dbg_hid("%s: device is already known\n", __func__);
+ return;
+ }
+
dj_hiddev = hid_allocate_device();
if (IS_ERR(dj_hiddev)) {
dev_err(&djrcv_hdev->dev, "%s: hid_allocate_device failed\n",
@@ -305,6 +313,7 @@ static void delayedwork_callback(struct work_struct *work)
struct dj_report dj_report;
unsigned long flags;
int count;
+ int retval;
dbg_hid("%s\n", __func__);
@@ -337,6 +346,25 @@ static void delayedwork_callback(struct work_struct *work)
logi_dj_recv_destroy_djhid_device(djrcv_dev, &dj_report);
break;
default:
+ /* A normal report (i. e. not belonging to a pair/unpair notification)
+ * arriving here, means that the report arrived but we did not have a
+ * paired dj_device associated to the report's device_index, this
+ * means that the original "device paired" notification corresponding
+ * to this dj_device never arrived to this driver. The reason is that
+ * hid-core discards all packets coming from a device while probe() is
+ * executing. */
+ if (!djrcv_dev->paired_dj_devices[dj_report.device_index]) {
+ /* ok, we don't know the device, just re-ask the
+ * receiver for the list of connected devices. */
+ retval = logi_dj_recv_query_paired_devices(djrcv_dev);
+ if (!retval) {
+ /* everything went fine, so just leave */
+ break;
+ }
+ dev_err(&djrcv_dev->hdev->dev,
+ "%s:logi_dj_recv_query_paired_devices "
+ "error:%d\n", __func__, retval);
+ }
dbg_hid("%s: unexpected report type\n", __func__);
}
}
@@ -367,6 +395,12 @@ static void logi_dj_recv_forward_null_report(struct dj_receiver_dev *djrcv_dev,
if (!djdev) {
dbg_hid("djrcv_dev->paired_dj_devices[dj_report->device_index]"
" is NULL, index %d\n", dj_report->device_index);
+ kfifo_in(&djrcv_dev->notif_fifo, dj_report, sizeof(struct dj_report));
+
+ if (schedule_work(&djrcv_dev->work) == 0) {
+ dbg_hid("%s: did not schedule the work item, was already "
+ "queued\n", __func__);
+ }
return;
}
@@ -397,6 +431,12 @@ static void logi_dj_recv_forward_report(struct dj_receiver_dev *djrcv_dev,
if (dj_device == NULL) {
dbg_hid("djrcv_dev->paired_dj_devices[dj_report->device_index]"
" is NULL, index %d\n", dj_report->device_index);
+ kfifo_in(&djrcv_dev->notif_fifo, dj_report, sizeof(struct dj_report));
+
+ if (schedule_work(&djrcv_dev->work) == 0) {
+ dbg_hid("%s: did not schedule the work item, was already "
+ "queued\n", __func__);
+ }
return;
}
@@ -444,6 +484,10 @@ static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev)
struct dj_report *dj_report;
int retval;
+ /* no need to protect djrcv_dev->querying_devices */
+ if (djrcv_dev->querying_devices)
+ return 0;
+
dj_report = kzalloc(sizeof(struct dj_report), GFP_KERNEL);
if (!dj_report)
return -ENOMEM;
@@ -455,6 +499,7 @@ static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev)
return retval;
}
+
static int logi_dj_recv_switch_to_dj_mode(struct dj_receiver_dev *djrcv_dev,
unsigned timeout)
{
diff --git a/drivers/hid/hid-logitech-dj.h b/drivers/hid/hid-logitech-dj.h
index fd28a5e..4a40003 100644
--- a/drivers/hid/hid-logitech-dj.h
+++ b/drivers/hid/hid-logitech-dj.h
@@ -101,6 +101,7 @@ struct dj_receiver_dev {
struct work_struct work;
struct kfifo notif_fifo;
spinlock_t lock;
+ bool querying_devices;
};
struct dj_device {
--
1.7.9.5
^ permalink raw reply related
* [PATCH 2/2] HID: hid-logitech-dj, querying_devices was never set
From: Nestor Lopez Casado @ 2013-07-18 13:21 UTC (permalink / raw)
To: jkosina
Cc: benjamin.tissoires, adlr, joseph.salisbury, linux-input,
linux-kernel, Nestor Lopez Casado
In-Reply-To: <1374153691-25100-1-git-send-email-nlopezcasad@logitech.com>
Set querying_devices flag to true when we start the enumeration
process.
This was missing from the original patch. It never produced
undesirable effects as it is highly improbable to have a second
enumeration triggered while a first one was still in progress.
Signed-off-by: Nestor Lopez Casado <nlopezcasad@logitech.com>
---
drivers/hid/hid-logitech-dj.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 0d13389..d4657a5 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -488,6 +488,8 @@ static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev)
if (djrcv_dev->querying_devices)
return 0;
+ djrcv_dev->querying_devices = true;
+
dj_report = kzalloc(sizeof(struct dj_report), GFP_KERNEL);
if (!dj_report)
return -ENOMEM;
--
1.7.9.5
^ permalink raw reply related
* Re: [RFC 7/8] HID: add transport driver documentation
From: Benjamin Tissoires @ 2013-07-18 8:16 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-input, Jiri Kosina, Henrik Rydberg, Oliver Neukum
In-Reply-To: <CANq1E4QunK-jykBRCft-GRd=d25Y66bchKiefUqFocR0OyW4yQ@mail.gmail.com>
Hi,
On Wed, Jul 17, 2013 at 5:05 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
[snipped]
>>> +
>>> +1) HID Bus
>>> +==========
>>> +
>>> +The HID subsystem is designed as a bus. Any I/O subsystem may provide HID
>>> +devices and register them with the HID bus. HID core then loads generic device
>>> +drivers on top of it. The transport drivers are responsible of raw data
>>> +transport and device setup/management. HID core is responsible of
>>> +report-parsing, report interpretation and the user-space API. Device specifics
>>> +and quirks are also handled by HID core and the HID device drivers.
>>
>> Hmm, the quirks part is not exactly what is currently implemented.
>> Usbhid (Transport Driver) set and use quirks while you say here that
>> they are handled by hid-core.
>>
>> I personally prefer the way it is documented here (quirks handled in
>> hid-core) so HIDP, i2c-hid and uhid will benefit from the dynamic quirks
>> already implemented in usbhid. But that will require another patch :)
>
> I actually meant things like wrong report-descriptors or special
> device drivers. Of course, there are also quirks that apply to the
> transport driver layer. I will rephrase that.
Thanks
>
[snipped]
>> To sum up, I think the differences between the channels are:
>> - Interrupt Channel (intr): asynchronous, device initiated, no
>> acknowledgements (beside the fact that data has been read), read only.
>> - Control Channel (ctrl): synchronous, host initiated, acknowledged
>> (because synchronous), read/write
>> - Output Channel (out): synchronous, host initiated, acknowledged
>> (because synchronous), write only
>
> I intetionally described the channels as "bi-directional". So you
> actually describe the same scenario, but from a different view-point
> (mine obviously is HIDP). So if you consider the OUT channel to be the
> same as writing on INTR, you will get the same scenario. I am not
> entirely sure which is better, but I can change it to your description
> if it helps?
Ok, no all this makes sense to me. FYI the USBHID stack has also 2
separate channels for intr. One Interrupt request and one URB for
writing out Intr. So mixing the two Intr lines into one seems specific
to BT. Anyway, I think you should rephrase it to make it more obvious
that these are not channels, but channel _types_. The Control channel
(type) is a general purposes channel used to send punctual queries to
the device (so it has a head over), whereas the intr channel type is a
dedicated line (half duplex or full duplex) in which each side (host
or device) knows what to do with each packet. Of course, your English
is far better than mine, so feel free to write whatever you want :)
>
> Regarding asynchronous vs. synchronous: I actually don't care how the
> I/O layer implements it. In this document, "asynchronous" means that I
> don't care for any acknowledgements. "synchronous" means, the remote
> device acknowledges the receival. Obviously, if an I/O system always
> acknowledges a SENT, then a transport driver can implement
> asynchronous transports as synchronous transports (which might mean
> having a separate buffer like USBHID does). But HID-core does not
> care.
> Same situation if synchronous is not supported. HID-core assumes it is
> synchronous so the transport layer can simply fake an acknowledgement.
> However, this means that the transport-layer might have to take care
> of retransmissions if an asynchronous transport fails (which L2CAP in
> Bluetooth-Core does tranparently for HIDP).
>
> I will try to be more verbose, but I intentionally posted all the
> callbacks below which should explain that "asynchronous" means it can
> be called in atomic-context and "synchronous" means it is allowed to
> sleep and wait for acknowledgement (speaking of code).
Thanks for the explanation of synchronous vs asynchronous. I was
indeed basing the (a)synchronous thing on the implementation of the
transport layer. So yes, keep your version.
>
> Thanks a lot for the I2C explanation, I think I understand it now and
> it does resemble USBHID and HIDP very much!
>
>> Then, the usbhid transport layer makes the ctrl and out channels
>> asynchronous...
>>
>>> +
>>> +Communication between devices and HID core is mostly done via HID reports. A
>>> +report can be of one of three types:
>>> +
>>> + - INPUT Report: Input reports provide data from device to host. This
>>> + data may include button events, axis events, battery status or more. This
>>> + data is generated by the device and sent to the host without requiring
>>
>> _with_ or without requiring explicit requests.
>> On I2C, the command GET_REPORT is even mandatory (which does not seems
>> to be the case for USB, given the amount of QUIRK_NO_INIT_REPORT we have.)
>
> You're right, they can be sent synchronously via GET_REPORT, too.
>
> But I don't know what you mean that GET_REPORT is mandatory? I thought
> an I2C device sends the data and fires an IRQ? Or is, upon interrupt
> receival, the host required to send a GET_REPORT to receive the
> pending input-event? I sadly have no idea how I2C works, but if it is
> _only_ host initiated, then this makes sense. Just want to go sure.
Indeed, when a device sends the data, it fires an IRQ. Then, the host
reads the INPUT register and fetch the data (no head over).
GET_REPORT is used when some driver wants to retrieve the current
state of the report without waiting for the data to be spontaneously
sent. It is mostly used at the start of the driver (or the
application).
I think Microsoft made this request mandatory because so many HID USB
devices are simply blocked when the host calls a GET_REPORT. This way,
we know for sure that the GET_REPORT command will not block the device
because it may be often used. On top of that, it is the only way to
retrieve the FEATURE reports...
[snipped]
>>> + reports are never sent on the intr channel as this channel is asynchronous.
>>> +
>>> +INPUT and OUTPUT reports can be sent as pure data reports on the intr channel.
>>
>> In my mind, OUTPUT reports are not sent through the intr channel
>> (because write only), but you already get my point I think :)
>
> Yeah, as explained above I merged INTR and OUTPUT into a bi-directional channel.
Ok. Now it makes sense.
>
>>> +For INPUT reports this is the usual operational mode. But for OUTPUT reports,
>>> +this is rarely done as OUTPUT reports are normally quite rare. But devices are
>>> +free to make excessive use of asynchronous OUTPUT reports (for instance, custom
>>> +HID audio speakers make great use of it).
>>
>> Ok, did not know about it.
>>
>>> +
>>> +Raw reports must not be sent on the ctrl channel, though. Instead, the ctrl
>>> +channel provides synchronous GET/SET_REPORT requests.
>>> +
>>> + - GET_REPORT: A GET_REPORT request has a report ID as payload and is sent
>>> + from host to device. The device must answer with a data report for the
>>> + requested report ID on the ctrl channel as a synchronous acknowledgement.
>>
>> Beware that the report ID is not mandatory in case the HID report
>> descriptors declares only 1 report without report ID. But I'm fine with
>> it anyway (it's understandable that way).
>
> Ugh, I am not entirely sure but afaik HIDP doesn't support implicit
> IDs. Could you tell me whether I2C supports it?
>
> A report-descriptor can skip report IDs if there is only a single
> report? Didn't know that, but we definitely need to document it.
Actually, this was a "feature" from the USB specification. But it is
up to the hardware maker to decide if it will implement it that way or
not (because it will influence the INPUT report themselves). I2C only
inherited it (so yes, it does support it). However, I never saw a
device not using the report ID...
>
>>> + Only one GET_REPORT request can be pending for each device. This restriction
>>> + is enforced by HID core as several transport drivers don't allow multiple
>>> + simultaneous GET_REPORT requests.
>>> + Note that data reports which are sent as answer to a GET_REPORT request are
>>> + not handled as generic device events. That is, if a device does not operate
>>> + in continuous data reporting mode, an answer to GET_REPORT does not replace
>>> + the raw data report on the intr channel on state change.
>>> + GET_REPORT is only used by custom HID device drivers to query device state.
>>> + Normally, HID core caches any device state so this request is not necessary
>>> + on devices that follow the HID specs.
>>
>> FYI, HID/I2C spec says: "GET_REPORT is often used by applications on
>> startup to retrieve the ``current state'' of the device rather than
>> waiting for the device to generate the next input/feature report".
>
> Yeah, for startup this makes sense. I will add a short note.
>
>> And as under Linux applications do not talk directly to the hid devices,
>> I fully concurs to your point.
>>
>>> + GET_REPORT requests can be sent for any of the 3 report types and shall
>>> + return the current report state of the device.
>>
>> The HID/I2C spec explicitly says: "the DEVICE shall ignore a GET_REPORT
>> requests with the REPORT TYPE set to Output, as it is not used in this
>> specification." So under i2c, we can send GET_REPORT with OUTPUT, but we
>> will not get anything from the device (this is why it is forbidden by
>> the transport driver).
>
> Heh, didn't know that, either. It's fine if I2C ignores it. However,
> I'd like to avoid forbidding it. There can be devices which use this
> (companies to crazy things..) and I cannot see a reason to forbid it
> in HID core. The transport drivers are free to adhere to their
> specifications, though. I guess that's fine?
perfectly fine :)
>
>>> + - SET_REPORT: A SET_REPORT request has a report ID plus data as payload. It is
>>> + sent from host to device and a device must update it's current report state
>>> + according to the given data. Any of the 3 report types can be used.
>>
>> The HID/I2C spec explicitly says: "the DEVICE might choose to ignore
>> input SET_REPORT requests as meaningless."...
>
> Same as above I think. But thanks for the clarification! I guess I
> will add a note that it is discouraged and devices are not supposed to
> handle it. This should clear all doubts.
Beware that devices should not use SET_REPORT for _input_ reports. But
still, in the end, it is up to the manufacturer of the device to
define it's own protocol. So I think I will need to change I2C to not
prevent SET_REPORT on INPUT reports, or GET_REPORT on OUTPUT reports
because I am sure one company will decide to use it that way to
initialize their device...
>
>>> + A device must answer with a synchronous acknowledgement. However, HID core
>>> + does not require transport drivers to forward this acknowledgement to HID
>>> + core.
>>> + Same as for GET_REPORT, only one SET_REPORT can be pending at a time. This
>>> + restriction is enforced by HID core as some transport drivers do not support
>>> + multiple synchronous SET_REPORT requests.
>>> +
>>> +Other ctrl-channel requests are supported by USB-HID but are not available
>>> +(or deprecated) in most other transport level specifications:
>>> +
>>> + - GET/SET_IDLE: Only used by USB-HID. Do not implement!
>>> + - GET/SET_PROTOCOL: Not used by HID core. Do not implement!
>>
>> The I2C declares also:
>> - RESET: mandatory (reset the device at any time)
>> - SET_POWER: mandatory on the device side (request from host to device
>> to indicate preferred power setting).
>
> Are they hooked up to HID-core? I'd like to avoid any commands which
No they are not. Actually I double checked with the USB spec, and it
is indeed specific to the I2C spec...
> are handled transparently in the transport driver (like
> suspend/resume). However, SET_POWER sounds related to ->power()
> callback. I will look through it again and include it in the next
> revision if it is hooked up.
I would say don't bother with the two of them. I thought they were
more generic but as they are specific to I2C, it does not make sense
to include them in this documentation. And you are right, SET_POWER
should be handled through ->power().
Also I need to remind you that the I2C implementation is not widely
tested because they are so few devices including it. This should
change in the near future (the power consumption is very low compared
to USB, so I expect OEM will start integrating them), but actually, I
still didn't got my hand on a real consumer product with HID/I2C... So
adjustments are expected to come, but the basic features are here, so
that people will not complain of not having a
touchpad/keyboard/touchscreen not working... :)
>
>>> +
>>> +2) HID API
>>> +==========
>>> +
>>> +2.1) Initialization
>>> +-------------------
>>> +
>>> +Transport drivers normally use the following procedure to register a new device
>>> +with HID core:
>>> +
>>> + struct hid_device *hid;
>>> + int ret;
>>> +
>>> + hid = hid_allocate_device();
>>> + if (IS_ERR(hid)) {
>>> + ret = PTR_ERR(hid);
>>> + goto err_<...>;
>>> + }
>>> +
>>> + strlcpy(hid->name, <device-name-src>, 127);
>>> + strlcpy(hid->phys, <device-phys-src>, 63);
>>> + strlcpy(hid->uniq, <device-uniq-src>, 63);
>>> +
>>> + hid->ll_driver = &custom_ll_driver;
>>> + hid->bus = <device-bus>;
>>> + hid->vendor = <device-vendor>;
>>> + hid->product = <device-product>;
>>> + hid->version = <device-version>;
>>> + hid->country = <device-country>;
>>
>> FYI, HID/I2C does not define any device-country field (I guess it will
>> come in a later release...)
>
> Right, I think BT also sets it to 0 in bluez. I will add a note that
> "0" is the default value.
>
>>> + hid->dev.parent = <pointer-to-parent-device>;
>>
>> FYI, I have started implementing a devm API for HID (in the same way the
>> input devm API is implemented), and dev.parent should not be overwritten.
>
> Cool! I will adjust the document once it is merged.
>
>> Anyway the two last comments are not requesting any changes in the document.
>>
>>> + hid->driver_data = <transport-driver-data-field>;
>>> +
>>> + ret = hid_add_device(hid);
>>> + if (ret)
>>> + goto err_<...>;
>>> +
>>> +Once hid_add_device() is entered, HID core might use the callbacks provided in
>>> +"custom_ll_driver". To unregister a device, use:
>> ^^^^
>> Maybe introduce a new paragraph (otherwise, it looks like
>> hid_destroy_device is called from HID core).
>
> Indeed, will do that.
>
>>> +
>>> + hid_destroy_device(hid);
>>> +
>>> +Once hid_destroy_device() returns, HID core will no longer make use of any
>>> +driver callbacks.
>>> +
>>> +2.2) hid_ll_driver operations
>>> +-----------------------------
>>> +
>>> +The available HID callbacks are:
>>> + - int (*start) (struct hid_device *hdev)
>>> + Called from HID device drivers once they want to use the device. Transport
>>> + drivers can choose to setup their device in this callback. However, normally
>>> + devices are already set up before transport drivers register them to HID core
>>> + so this is mostly only used by USB-HID.
>>> +
>>> + - void (*stop) (struct hid_device *hdev)
>>> + Called from HID device drivers once they are done with a device. Transport
>>> + drivers can free any buffers and deinitialize the device. But note that
>>> + ->start() might be called again if another HID device driver is loaded on the
>>> + device.
>>> + Transport drivers are free to ignore it and deinitialize devices after they
>>> + destroyed them via hid_destroy_device().
>>> +
>>> + - int (*open) (struct hid_device *hdev)
>>> + Called from HID device drivers once they are interested in data reports.
>>> + Usually, while user-space didn't open any input API/etc., device drivers are
>>> + not interested in device data and transport drivers can put devices asleep.
>>> + However, once ->open() is called, transport drivers must be ready for I/O.
>>> + ->open() calls are never nested. So in between two ->open() calls there must
>>> + be a call to ->close().
>>
>> This is not true in either USB or I2C. And I guess with every BT or UHID
>> devices. ->open() and ->close() are called upon open/close of the input
>> node (or hidraw, or hiddev). So If two clients are opening the same
>> input node, there will be two calls to ->open() without any call to
>> ->close() in between.
>>
>> I guess you mixed up this part with the ->start() ->stop() :)
>
> I got confused, indeed. input-core only calls ->open() on first-open,
> but there might be multiple input devices. So you're right. I will fix
> this. Thanks for catching it!
>
>>> +
>>> + - void (*close) (struct hid_device *hdev)
>>> + Called from HID device drivers after ->open() was called but they are no
>>> + longer interested in device reports. (Usually if user-space closed any input
>>> + devices of the driver).
>>> + Transport drivers can put devices asleep and terminate any I/O. However,
>>> + ->start() may be called again if the device driver is interested in input
>>> + reports again.
>>> +
>>> + - int (*parse) (struct hid_device *hdev)
>>> + Called once during device setup after ->start() has been called. Transport
>>> + drivers must read the HID report-descriptor from the device and tell HID core
>>> + about it via hid_parse_report().
>>> +
>>> + - int (*power) (struct hid_device *hdev, int level)
>>> + Called by HID core to give PM hints to transport drivers. Usually this is
>>> + analogical to the ->open() and ->close() hints and redundant.
>>> +
>>> + - void (*request) (struct hid_device *hdev, struct hid_report *report,
>>> + int reqtype)
>>> + Send an HID request on the ctrl channel. "report" contains the report that
>>> + should be sent and "reqtype" the request type. Request-type can be
>>> + HID_REQ_SET_REPORT or HID_REQ_GET_REPORT.
>>> + This callback is optional. If not provided, HID core will assemble a raw
>>> + report following the HID specs and send it via the ->raw_request() callback.
>>> + The transport driver is free to implement this asynchronously.
>>> +
>>> + - int (*wait) (struct hid_device *hdev)
>>> + Used by HID core before calling ->request() again. A transport driver can use
>>> + it to wait for any pending requests to complete if only one request is
>>> + allowed at a time.
>>> +
>>> + - int (*raw_request) (struct hid_device *hdev, unsigned char reportnum,
>>> + __u8 *buf, size_t count, unsigned char rtype,
>>> + int reqtype)
>>> + Same as ->request() but provides the report as raw buffer. This request shall
>>> + be synchronous. A transport driver must not use ->wait() to complete such
>>> + requests.
>>
>> 2 questions/remarks here:
>> - is raw_request() meant to replace ->hid_output_raw_report() and
>> ->hid_get_raw_report()?
>
> Both. raw_request() with HID_REQ_GET_REPORT replaces
> hid_get_raw_report() and with HID_REQ_SET_REPORT it replaces
> hid_output_raw_report().
> However, at least HIDP implements hid_output_raw_report() with
> HID_OUTPUT_REPORT as raw output report instead of SET_REPORT. For
> this, "output_report()" below can be used.
thanks. I definitively prefers this. The fact is that USB is very
tolerant and can rely on the USB descriptor to know if it needs to use
SET_REPORT or OUTPUT_REPORT. I tried to implement the same kind of
thing in I2C, but I am not very happy with the final implementation.
So definitively having an explicit way to handle this will allow us to
write specific drivers in case there is some troubles with the
automatic handling.
>
>> - reportnum declared as an unsigned will be problematic regarding the
>> rare devices not having any report ID in their report descriptors.
>
> I thought reportnum 0 is an invalid ID? I will check again and change
> to signed if needed. Thanks!
You are right. reportnum == 0 is treated in the USBHID stack as a
report without a report ID. So keeping the unsigned is good here.
>
>>> +
>>> + - int (*output_report) (struct hid_device *hdev, __u8 *buf, size_t len)
>>> + Send raw output report via intr channel. Used by some HID device drivers
>>> + which require high throughput for outgoing requests on the intr channel. This
>>> + must not cause SET_REPORT calls! This must be implemented as asynchronous
>>> + output report on the intr channel!
>>
>> For me, there is something wrong here. The name infers that we are
>> trying to send an output_report directly (so through the USB URB or
>> through the output i2c register), but you are implementing it through
>> the intr channel... :-S
>
> Again, "write on INTR" is OUTPUT channel for me. So I guess with the
> explanation above we are fine here?
sure, now it's clear.
>
>>> +
>>> + - int (*hidinput_input_event) (struct input_dev *idev, unsigned int type,
>>> + unsigned int code, int value)
>>> + Obsolete callback used by logitech converters. It is called when userspace
>>> + writes input events to the input device (eg., EV_LED). A driver can use this
>>> + callback to convert it into an output report and send it to the device. If
>>> + this callback is not provided, HID core will use ->request() or
>>> + ->raw_request() respectively.
>>
>> I bet there will be a way to make this work with logitech devices too
>> (if we implement a proper ->hid_output_raw_report() in each paired devices).
>
> Yeah, I thought so, but I have no idea what the logitech-dj driver
> does. I guess we can drop this once we have no more users, but I
> wanted to avoid pushing to hard on it.
Well, I know it well because I helped Logitech pushing hid-logitech-dj
upstream. Don't bother with it currently, we will remove this part
from the documentation once all the drivers are converted.
>
>>> +
>>> + - int (*idle) (struct hid_device *hdev, int report, int idle, int reqtype)
>>> + Perform SET/GET_IDLE request. Only used by USB-HID, do not implement!
>>> +
>>> +2.3) Data Path
>>> +--------------
>>> +
>>> +Transport drivers are responsible of reading data from I/O devices. They must
>>> +handle any state-tracking themselves. HID core does not implement protocol
>>
>> I don't get the "state-tracking" here. The reports states should be
>> handled by core, and I do not see the other states (or you meant PM
>> states?).
>
> Ugh, I meant I/O-related state-tracking (or PM). I will rephrase that.
Thanks
>
>>> +handshakes or other management commands which can be required by the given HID
>>> +transport specification.
>>> +
>>> +Every raw data packet read from a device must be fed into HID core via
>>> +hid_input_report(). You must specify the channel-type (intr or ctrl) and report
>>> +type (input/output/feature). Under normal conditions, only input reports are
>>> +provided via this API.
>>> +
>>> +Responses to GET_REPORT requests via ->request() must also be provided via this
>>> +API. Responses to ->raw_request() are synchronous and must be intercepted by the
>>> +transport driver and not passed to hid_input_report().
>>> +Acknowledgements to SET_REPORT requests are not of interest to HID core.
>>> +
>>> +----------------------------------------------------
>>> +Written 2013, David Herrmann <dh.herrmann@gmail.com>
>>> --
>>> 1.8.3.2
>>>
>>
>> done!
>> Many thanks for this David. It was very interesting and detailed. It
>> will make a great documentation.
>
> Thanks a lot for reviewing. I will fix the remaining issues and with
> the "write-on-INTR is OUTPUT" I guess we are on the same page (except
> for minor issues)?
>
Yep, we are on the same page. Once you will have sent the v2, Jiri
will have a look on it and I think we will be good :)
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH 001/001] [Input:] Propagate hardware generated event timestamp to evdev.
From: Alexander Levitskiy @ 2013-07-17 20:31 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, linux-kernel, linux-doc, Rob Landley, Henrik Rydberg,
Arve Hjønnevåg, Stephen Boyd
In-Reply-To: <20130713064430.GA20979@core.coreip.homeip.net>
Dmitry,
I didn't see the MSC_TIMESTAMP before. I should have been more up to
date. I've looked at MSC_TIMESTAMP change and the first impression was
that it is something we can live with. After discussing details with
my colleagues here we came to a conclusion that:
a) we do need absolute time.
b) since we have to use 64-bit timestamp there is no point to discard
extra precision especially in light of event timestamp being of ns
precision.
c) there is no point to propagate the timestamp to userspace since
there is an existing mechanism in place.
I think the concept of propagating hardware timestamp in ABS_MT fits
nicely in the "simultaneous contact" model of multitouch operation.
I'd like to keep this functionality.
Thank you,
- Sasha.
On Fri, Jul 12, 2013 at 11:44 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Sasha,
>
> On Wed, Jul 10, 2013 at 01:38:00PM -0700, Alexander Levitskiy wrote:
>> From: Sasha Levitskiy <sanek@google.com>
>>
>> Input: Propagate hardware event timestamp to evdev.
>>
>> Convey hardware generated timestamp associated with the current event packet.
>> The use of these event codes by hardware drivers is optional.
>> Used to reduce jitter and improve velocity tracking in ABS_MT and other timing
>> sensitive devices.
>>
>> kernel v. 3.4
>>
>> Signed-off-by: Sasha Levitskiy <sanek@google.com>
>> ---
>> diff --git a/Documentation/input/event-codes.txt
>> b/Documentation/input/event-codes.txt
>> index 53305bd..f0f0e07 100644
>> --- a/Documentation/input/event-codes.txt
>> +++ b/Documentation/input/event-codes.txt
>> @@ -91,6 +91,15 @@ sent in the evdev event stream.
>> event and query the device (using EVIOCG* ioctls) to obtain its
>> current state.
>>
>> +* SYN_TIME_SEC, SYN_TIME_NSEC:
>> + - Used to convey hardware timestamp associated with the current
>> + event packet. The use of these event codes by hardware drivers
>> + is optional. If used, the hardware driver should send the timestamp
>> + ahead of any other events associated with this packet. The timestamp
>> + should be adjusted to CLOCK_MONOTONIC base.
>> + This becomes useful for drivers of hardware that handle batching
>> + without involving the main CPU.
>
> We already have MSC_TIMESTAMP (which is in usec), do you really need
> nsec resolution?
>
>> +
>> EV_KEY:
>> ----------
>> EV_KEY events take the form KEY_<name> or BTN_<name>. For example,
>> KEY_A is used
>> diff --git a/Documentation/input/multi-touch-protocol.txt
>> b/Documentation/input/multi-touch-protocol.txt
>> index 543101c..71af317 100644
>> --- a/Documentation/input/multi-touch-protocol.txt
>> +++ b/Documentation/input/multi-touch-protocol.txt
>> @@ -80,6 +80,10 @@ Userspace can detect that a driver can report more
>> total contacts than slots
>> by noting that the largest supported BTN_TOOL_*TAP event is larger than the
>> total number of type B slots reported in the absinfo for the ABS_MT_SLOT axis.
>>
>> +Velocity tracking and temporal precision can be improved if device provides
>> +exact timestamp for touches reported through SYN_TIME_SEC and SYN_TIME_NSEC.
>> +The timestamp should be reported ahead of everything else in the packet.
>> +
>> Protocol Example A
>> ------------------
>>
>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> index 41f79be..48baf6f 100644
>> --- a/drivers/input/evdev.c
>> +++ b/drivers/input/evdev.c
>> @@ -37,6 +37,8 @@ struct evdev {
>> struct mutex mutex;
>> struct device dev;
>> bool exist;
>> + int hw_ts_sec;
>> + int hw_ts_nsec;
>> };
>>
>> struct evdev_client {
>> @@ -109,7 +111,20 @@ static void evdev_event(struct input_handle *handle,
>> struct input_event event;
>> ktime_t time_mono, time_real;
>>
>> - time_mono = ktime_get();
>> + if (type == EV_SYN && code == SYN_TIME_SEC) {
>> + evdev->hw_ts_sec = value;
>> + return;
>
> Why do you need this special handling? Can you simply have MSC_TIMESTAMP
> to be delivered as part of the event packet and use it instead of the
> event timestamp if userspace chooses to do so?
>
> Thanks.
>
> --
> Dmitry
^ permalink raw reply
* Re: [RFC 7/8] HID: add transport driver documentation
From: David Herrmann @ 2013-07-17 15:05 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: linux-input, Jiri Kosina, Henrik Rydberg, Oliver Neukum
In-Reply-To: <51E52152.3020801@gmail.com>
Hi
On Tue, Jul 16, 2013 at 12:32 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> Hi David,
>
> On Mon, Jul 15, 2013 at 7:10 PM, David Herrmann <dh.herrmann@gmail.com>
> wrote:
>> hid-transport.txt describes the transport driver layout which is required
>> by HID-core to work correctly. HID-core supports many different transport
>> drivers and new drivers can be added for any kind of I/O system.
>>
>> The current layout doesn't really differentiate between intr and ctrl
>> channels, which confuses some hid-drivers and may break devices. HIDP,
>> USBHID and I2DHID all implement different semantics for each callback, so
>
> typo: I2CHID
Fixed, thx!
>> our device drivers aren't really transport-driver-agnostic.
>>
>> To solve that, hid-transport.txt describes the layout of transport drivers
>> that is required by HID core. Furthermore, it introduces some new
>> callbacks which replace the current hid_get_raw_report() and
>> hid_output_raw_report() hooks. These will be implemented in follow-up
>> patches.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> ---
>> Documentation/hid/hid-transport.txt | 299 ++++++++++++++++++++++++++++++++++++
>> 1 file changed, 299 insertions(+)
>> create mode 100644 Documentation/hid/hid-transport.txt
>>
>> diff --git a/Documentation/hid/hid-transport.txt b/Documentation/hid/hid-transport.txt
>> new file mode 100644
>> index 0000000..034f11d
>> --- /dev/null
>> +++ b/Documentation/hid/hid-transport.txt
>> @@ -0,0 +1,299 @@
>> + HID I/O Transport Drivers
>> + ===========================
>> +
>> +The HID subsystem is independent of the underlying transport driver. Initially,
>> +only USB was supported, but other specifications adopted the HID design and
>> +provided new transport drivers. The kernel includes at least support for USB,
>> +Bluetooth, i2c and user-space I/O drivers.
>
> IIRC, we should put I2C in capital letters.
Yepp, fixed.
>> +
>> +1) HID Bus
>> +==========
>> +
>> +The HID subsystem is designed as a bus. Any I/O subsystem may provide HID
>> +devices and register them with the HID bus. HID core then loads generic device
>> +drivers on top of it. The transport drivers are responsible of raw data
>> +transport and device setup/management. HID core is responsible of
>> +report-parsing, report interpretation and the user-space API. Device specifics
>> +and quirks are also handled by HID core and the HID device drivers.
>
> Hmm, the quirks part is not exactly what is currently implemented.
> Usbhid (Transport Driver) set and use quirks while you say here that
> they are handled by hid-core.
>
> I personally prefer the way it is documented here (quirks handled in
> hid-core) so HIDP, i2c-hid and uhid will benefit from the dynamic quirks
> already implemented in usbhid. But that will require another patch :)
I actually meant things like wrong report-descriptors or special
device drivers. Of course, there are also quirks that apply to the
transport driver layer. I will rephrase that.
>> +
>> + +-----------+ +-----------+ +-----------+ +-----------+
>> + | Device #1 | | Device #i | | Device #j | | Device #k |
>> + +-----------+ +-----------+ +-----------+ +-----------+
>> + \\ // \\ //
>> + +------------+ +------------+
>> + | I/O Driver | | I/O Driver |
>> + +------------+ +------------+
>> + || ||
>> + +------------------+ +------------------+
>> + | Transport Driver | | Transport Driver |
>> + +------------------+ +------------------+
>> + \___ ___/
>> + \ /
>> + +----------------+
>> + | HID Core |
>> + +----------------+
>> + / | | \
>> + / | | \
>> + ____________/ | | \_________________
>> + / | | \
>> + / | | \
>> + +----------------+ +-----------+ +------------------+ +------------------+
>> + | Generic Driver | | MT Driver | | Custom Driver #1 | | Custom Driver #2 |
>> + +----------------+ +-----------+ +------------------+ +------------------+
>> +
>> +Example Drivers:
>> + I/O: USB, I2C, Bluetooth-l2cap
>> + Transport: USB-HID, I2C-HID, BT-HIDP
>> +
>> +Normally, a transport driver is specific for a given I/O driver. But the HID
>> +design also allows transport drivers to work with different I/O systems at the
>> +same time.
>
> I do not follow you here. Which transport driver you have in mind?
There is none such driver, I just wanted to point out that HID-core
doesn't care. Turns out to be more confusing than helping so I guess I
will just drop it.
>> +
>> +Everything below "HID Core" is simplified in this graph as it is only of
>> +interest to HID device drivers. Transport drivers do not need to know the
>> +specifics.
>> +
>> +1.1) Device Setup
>> +-----------------
>> +
>> +I/O drivers normally provide hotplug detection or device enumeration APIs to the
>> +transport drivers. Transport drivers use this to find any suitable HID device.
>> +They allocate HID device objects and register them with HID core. Transport
>> +drivers are not required to register themselves with HID core. HID core is never
>> +aware of which transport drivers are available and is not interested in it. It
>> +is only interested in devices.
>> +
>> +Transport drivers attach a constant "struct hid_ll_driver" object with each
>> +device. Once a device is registered with HID core, the callbacks provided via
>> +this struct are used by HID core to communicate with the device.
>> +
>> +Transport drivers are responsible of detecting device failures and unplugging.
>> +HID core will operate an device as long as it is registered regardless of any
>
> typo: ^^ "a"
Fixed.
>> +device failures. Once transport drivers detect unplug or failure events, they
>> +must unregister the device from HID core and HID core will stop using the
>> +provided callbacks.
>> +
>> +1.2) Transport Driver Requirements
>> +----------------------------------
>> +
>> +HID core requires transport drivers to follow a given design. A Transport
>> +drivers must provide two bi-directional I/O channels to each HID device. These
>> +channels might be multiplexed on a single physical channel, though.
>> +
>> + - Interrupt Channel (intr): The intr channel is used for asynchronous data
>> + reports. No management commands or data acknowledgements are sent on this
>> + channel. Any unrequested incoming or outgoing data report must be sent on
>> + this channel and is never acknowledged by the remote side. Devices usually
>> + send their input events on this channel. Outgoing events are normally
>> + not send via intr, except if high throughput is required.
>> + - Control Channel (ctrl): The ctrl channel is used for synchronous requests and
>> + device management. Unrequested data input events must not be sent on this
>> + channel and are normally ignored. Instead, devices only send management
>> + events or answers to host requests on this channel.
>> + Outgoing reports are usually sent on the ctrl channel via synchronous
>> + SET_REPORT requests.
>
> This is not entirely true for USB and I2C:
> - for USB, OUTPUT reports can also be sent through a direct write on an URB.
> - for I2C, the notion of intr vs ctrl is slightly different, but it
> remains roughly the same. Let me detail it so that you can make your idea:
> The HID descriptors declares three I2C registers:
> * the "Input" register: this one correspond to the intr you are
> describing. The difference is that this register is only used from the
> communication from the device to the host (no outputs here). The big
> interest in this register is that the communication is device initiated
> thanks to an external line triggered by the device (in I2C, the
> communication is only master (host) initiated).
> * the "Command" register: this one corresponds to the ctrl channel.
> This channel/register is host initiated too (like on the USB side), and
> there is a slight head over due to the HID protocol regarding sending
> output events.
> * the "Output" register: this one corresponds to the URBs described in
> USB. Every communication coming from the host on this channel is
> considered as an OUTPUT report, reducing the HID protocol over head.
>
> The HID/I2C also declares a forth register (the "data") register which
> is used by the HID protocol while using the "Command" register.
>
>
> To sum up, I think the differences between the channels are:
> - Interrupt Channel (intr): asynchronous, device initiated, no
> acknowledgements (beside the fact that data has been read), read only.
> - Control Channel (ctrl): synchronous, host initiated, acknowledged
> (because synchronous), read/write
> - Output Channel (out): synchronous, host initiated, acknowledged
> (because synchronous), write only
I intetionally described the channels as "bi-directional". So you
actually describe the same scenario, but from a different view-point
(mine obviously is HIDP). So if you consider the OUT channel to be the
same as writing on INTR, you will get the same scenario. I am not
entirely sure which is better, but I can change it to your description
if it helps?
Regarding asynchronous vs. synchronous: I actually don't care how the
I/O layer implements it. In this document, "asynchronous" means that I
don't care for any acknowledgements. "synchronous" means, the remote
device acknowledges the receival. Obviously, if an I/O system always
acknowledges a SENT, then a transport driver can implement
asynchronous transports as synchronous transports (which might mean
having a separate buffer like USBHID does). But HID-core does not
care.
Same situation if synchronous is not supported. HID-core assumes it is
synchronous so the transport layer can simply fake an acknowledgement.
However, this means that the transport-layer might have to take care
of retransmissions if an asynchronous transport fails (which L2CAP in
Bluetooth-Core does tranparently for HIDP).
I will try to be more verbose, but I intentionally posted all the
callbacks below which should explain that "asynchronous" means it can
be called in atomic-context and "synchronous" means it is allowed to
sleep and wait for acknowledgement (speaking of code).
Thanks a lot for the I2C explanation, I think I understand it now and
it does resemble USBHID and HIDP very much!
> Then, the usbhid transport layer makes the ctrl and out channels
> asynchronous...
>
>> +
>> +Communication between devices and HID core is mostly done via HID reports. A
>> +report can be of one of three types:
>> +
>> + - INPUT Report: Input reports provide data from device to host. This
>> + data may include button events, axis events, battery status or more. This
>> + data is generated by the device and sent to the host without requiring
>
> _with_ or without requiring explicit requests.
> On I2C, the command GET_REPORT is even mandatory (which does not seems
> to be the case for USB, given the amount of QUIRK_NO_INIT_REPORT we have.)
You're right, they can be sent synchronously via GET_REPORT, too.
But I don't know what you mean that GET_REPORT is mandatory? I thought
an I2C device sends the data and fires an IRQ? Or is, upon interrupt
receival, the host required to send a GET_REPORT to receive the
pending input-event? I sadly have no idea how I2C works, but if it is
_only_ host initiated, then this makes sense. Just want to go sure.
Anyway, even if I2C requires an explicit GET_REPORT, this doesn't
conflict with this description. It's an implementation detail of I2C
and HID-core doesn't care if input events require a GET_REPORT.
>> + explicit requests. Devices can choose to send data continously or only on
>
> My spell checker gives me a typo on: "continuously"
Whoops, right.
>> + change.
>> + - OUTPUT Report: Output reports change device states. They are sent from host
>> + to device and may include LED requests, rumble requests or more. Output
>> + reports are never sent from device to host, except if explicitly requested.
>
> In this case, I think it would be better to say that Output reports are
> never sent from device to host, but the host can _retrieve_ its current
> state.
Yepp, makes sense.
>> + Hosts may choose to send output reports either continously or only on change.
>
> My spell checker gives me a typo on: "continuously"
Fixed.
>> + - FEATURE Report: Feature reports can be anything that doesn't belong into the
>> + other two categories. Some of them are generic and defined by the HID spec,
>> + but most of them are used as custom device extensions. For instance, they may
>> + provide battery status information.
>
> I disagree. FEATURE are specific reports which are read/write, and not
> spontaneously emitted by the device. For instance, this allows the host
> to change the current mode of the device (mouse emulation or multitouch
> in the case I know most), or to set constants in the device (max
> reported touches in my case, but this can be hardware macros for
> keyboards, or something else).
This is actually what I meant. But I agree, it isn't really clear from
my description.
>> + Feature reports are never sent without requests. A host must explicitly
>> + request a device to set or send a feature report. This also means, feature
>
> I would prefer a "host must explicitly request a device to set or
> _retrieve_ a feature report".
Yepp, fixed.
>> + reports are never sent on the intr channel as this channel is asynchronous.
>> +
>> +INPUT and OUTPUT reports can be sent as pure data reports on the intr channel.
>
> In my mind, OUTPUT reports are not sent through the intr channel
> (because write only), but you already get my point I think :)
Yeah, as explained above I merged INTR and OUTPUT into a bi-directional channel.
>> +For INPUT reports this is the usual operational mode. But for OUTPUT reports,
>> +this is rarely done as OUTPUT reports are normally quite rare. But devices are
>> +free to make excessive use of asynchronous OUTPUT reports (for instance, custom
>> +HID audio speakers make great use of it).
>
> Ok, did not know about it.
>
>> +
>> +Raw reports must not be sent on the ctrl channel, though. Instead, the ctrl
>> +channel provides synchronous GET/SET_REPORT requests.
>> +
>> + - GET_REPORT: A GET_REPORT request has a report ID as payload and is sent
>> + from host to device. The device must answer with a data report for the
>> + requested report ID on the ctrl channel as a synchronous acknowledgement.
>
> Beware that the report ID is not mandatory in case the HID report
> descriptors declares only 1 report without report ID. But I'm fine with
> it anyway (it's understandable that way).
Ugh, I am not entirely sure but afaik HIDP doesn't support implicit
IDs. Could you tell me whether I2C supports it?
A report-descriptor can skip report IDs if there is only a single
report? Didn't know that, but we definitely need to document it.
>> + Only one GET_REPORT request can be pending for each device. This restriction
>> + is enforced by HID core as several transport drivers don't allow multiple
>> + simultaneous GET_REPORT requests.
>> + Note that data reports which are sent as answer to a GET_REPORT request are
>> + not handled as generic device events. That is, if a device does not operate
>> + in continuous data reporting mode, an answer to GET_REPORT does not replace
>> + the raw data report on the intr channel on state change.
>> + GET_REPORT is only used by custom HID device drivers to query device state.
>> + Normally, HID core caches any device state so this request is not necessary
>> + on devices that follow the HID specs.
>
> FYI, HID/I2C spec says: "GET_REPORT is often used by applications on
> startup to retrieve the ``current state'' of the device rather than
> waiting for the device to generate the next input/feature report".
Yeah, for startup this makes sense. I will add a short note.
> And as under Linux applications do not talk directly to the hid devices,
> I fully concurs to your point.
>
>> + GET_REPORT requests can be sent for any of the 3 report types and shall
>> + return the current report state of the device.
>
> The HID/I2C spec explicitly says: "the DEVICE shall ignore a GET_REPORT
> requests with the REPORT TYPE set to Output, as it is not used in this
> specification." So under i2c, we can send GET_REPORT with OUTPUT, but we
> will not get anything from the device (this is why it is forbidden by
> the transport driver).
Heh, didn't know that, either. It's fine if I2C ignores it. However,
I'd like to avoid forbidding it. There can be devices which use this
(companies to crazy things..) and I cannot see a reason to forbid it
in HID core. The transport drivers are free to adhere to their
specifications, though. I guess that's fine?
>> + - SET_REPORT: A SET_REPORT request has a report ID plus data as payload. It is
>> + sent from host to device and a device must update it's current report state
>> + according to the given data. Any of the 3 report types can be used.
>
> The HID/I2C spec explicitly says: "the DEVICE might choose to ignore
> input SET_REPORT requests as meaningless."...
Same as above I think. But thanks for the clarification! I guess I
will add a note that it is discouraged and devices are not supposed to
handle it. This should clear all doubts.
>> + A device must answer with a synchronous acknowledgement. However, HID core
>> + does not require transport drivers to forward this acknowledgement to HID
>> + core.
>> + Same as for GET_REPORT, only one SET_REPORT can be pending at a time. This
>> + restriction is enforced by HID core as some transport drivers do not support
>> + multiple synchronous SET_REPORT requests.
>> +
>> +Other ctrl-channel requests are supported by USB-HID but are not available
>> +(or deprecated) in most other transport level specifications:
>> +
>> + - GET/SET_IDLE: Only used by USB-HID. Do not implement!
>> + - GET/SET_PROTOCOL: Not used by HID core. Do not implement!
>
> The I2C declares also:
> - RESET: mandatory (reset the device at any time)
> - SET_POWER: mandatory on the device side (request from host to device
> to indicate preferred power setting).
Are they hooked up to HID-core? I'd like to avoid any commands which
are handled transparently in the transport driver (like
suspend/resume). However, SET_POWER sounds related to ->power()
callback. I will look through it again and include it in the next
revision if it is hooked up.
>> +
>> +2) HID API
>> +==========
>> +
>> +2.1) Initialization
>> +-------------------
>> +
>> +Transport drivers normally use the following procedure to register a new device
>> +with HID core:
>> +
>> + struct hid_device *hid;
>> + int ret;
>> +
>> + hid = hid_allocate_device();
>> + if (IS_ERR(hid)) {
>> + ret = PTR_ERR(hid);
>> + goto err_<...>;
>> + }
>> +
>> + strlcpy(hid->name, <device-name-src>, 127);
>> + strlcpy(hid->phys, <device-phys-src>, 63);
>> + strlcpy(hid->uniq, <device-uniq-src>, 63);
>> +
>> + hid->ll_driver = &custom_ll_driver;
>> + hid->bus = <device-bus>;
>> + hid->vendor = <device-vendor>;
>> + hid->product = <device-product>;
>> + hid->version = <device-version>;
>> + hid->country = <device-country>;
>
> FYI, HID/I2C does not define any device-country field (I guess it will
> come in a later release...)
Right, I think BT also sets it to 0 in bluez. I will add a note that
"0" is the default value.
>> + hid->dev.parent = <pointer-to-parent-device>;
>
> FYI, I have started implementing a devm API for HID (in the same way the
> input devm API is implemented), and dev.parent should not be overwritten.
Cool! I will adjust the document once it is merged.
> Anyway the two last comments are not requesting any changes in the document.
>
>> + hid->driver_data = <transport-driver-data-field>;
>> +
>> + ret = hid_add_device(hid);
>> + if (ret)
>> + goto err_<...>;
>> +
>> +Once hid_add_device() is entered, HID core might use the callbacks provided in
>> +"custom_ll_driver". To unregister a device, use:
> ^^^^
> Maybe introduce a new paragraph (otherwise, it looks like
> hid_destroy_device is called from HID core).
Indeed, will do that.
>> +
>> + hid_destroy_device(hid);
>> +
>> +Once hid_destroy_device() returns, HID core will no longer make use of any
>> +driver callbacks.
>> +
>> +2.2) hid_ll_driver operations
>> +-----------------------------
>> +
>> +The available HID callbacks are:
>> + - int (*start) (struct hid_device *hdev)
>> + Called from HID device drivers once they want to use the device. Transport
>> + drivers can choose to setup their device in this callback. However, normally
>> + devices are already set up before transport drivers register them to HID core
>> + so this is mostly only used by USB-HID.
>> +
>> + - void (*stop) (struct hid_device *hdev)
>> + Called from HID device drivers once they are done with a device. Transport
>> + drivers can free any buffers and deinitialize the device. But note that
>> + ->start() might be called again if another HID device driver is loaded on the
>> + device.
>> + Transport drivers are free to ignore it and deinitialize devices after they
>> + destroyed them via hid_destroy_device().
>> +
>> + - int (*open) (struct hid_device *hdev)
>> + Called from HID device drivers once they are interested in data reports.
>> + Usually, while user-space didn't open any input API/etc., device drivers are
>> + not interested in device data and transport drivers can put devices asleep.
>> + However, once ->open() is called, transport drivers must be ready for I/O.
>> + ->open() calls are never nested. So in between two ->open() calls there must
>> + be a call to ->close().
>
> This is not true in either USB or I2C. And I guess with every BT or UHID
> devices. ->open() and ->close() are called upon open/close of the input
> node (or hidraw, or hiddev). So If two clients are opening the same
> input node, there will be two calls to ->open() without any call to
> ->close() in between.
>
> I guess you mixed up this part with the ->start() ->stop() :)
I got confused, indeed. input-core only calls ->open() on first-open,
but there might be multiple input devices. So you're right. I will fix
this. Thanks for catching it!
>> +
>> + - void (*close) (struct hid_device *hdev)
>> + Called from HID device drivers after ->open() was called but they are no
>> + longer interested in device reports. (Usually if user-space closed any input
>> + devices of the driver).
>> + Transport drivers can put devices asleep and terminate any I/O. However,
>> + ->start() may be called again if the device driver is interested in input
>> + reports again.
>> +
>> + - int (*parse) (struct hid_device *hdev)
>> + Called once during device setup after ->start() has been called. Transport
>> + drivers must read the HID report-descriptor from the device and tell HID core
>> + about it via hid_parse_report().
>> +
>> + - int (*power) (struct hid_device *hdev, int level)
>> + Called by HID core to give PM hints to transport drivers. Usually this is
>> + analogical to the ->open() and ->close() hints and redundant.
>> +
>> + - void (*request) (struct hid_device *hdev, struct hid_report *report,
>> + int reqtype)
>> + Send an HID request on the ctrl channel. "report" contains the report that
>> + should be sent and "reqtype" the request type. Request-type can be
>> + HID_REQ_SET_REPORT or HID_REQ_GET_REPORT.
>> + This callback is optional. If not provided, HID core will assemble a raw
>> + report following the HID specs and send it via the ->raw_request() callback.
>> + The transport driver is free to implement this asynchronously.
>> +
>> + - int (*wait) (struct hid_device *hdev)
>> + Used by HID core before calling ->request() again. A transport driver can use
>> + it to wait for any pending requests to complete if only one request is
>> + allowed at a time.
>> +
>> + - int (*raw_request) (struct hid_device *hdev, unsigned char reportnum,
>> + __u8 *buf, size_t count, unsigned char rtype,
>> + int reqtype)
>> + Same as ->request() but provides the report as raw buffer. This request shall
>> + be synchronous. A transport driver must not use ->wait() to complete such
>> + requests.
>
> 2 questions/remarks here:
> - is raw_request() meant to replace ->hid_output_raw_report() and
> ->hid_get_raw_report()?
Both. raw_request() with HID_REQ_GET_REPORT replaces
hid_get_raw_report() and with HID_REQ_SET_REPORT it replaces
hid_output_raw_report().
However, at least HIDP implements hid_output_raw_report() with
HID_OUTPUT_REPORT as raw output report instead of SET_REPORT. For
this, "output_report()" below can be used.
> - reportnum declared as an unsigned will be problematic regarding the
> rare devices not having any report ID in their report descriptors.
I thought reportnum 0 is an invalid ID? I will check again and change
to signed if needed. Thanks!
>> +
>> + - int (*output_report) (struct hid_device *hdev, __u8 *buf, size_t len)
>> + Send raw output report via intr channel. Used by some HID device drivers
>> + which require high throughput for outgoing requests on the intr channel. This
>> + must not cause SET_REPORT calls! This must be implemented as asynchronous
>> + output report on the intr channel!
>
> For me, there is something wrong here. The name infers that we are
> trying to send an output_report directly (so through the USB URB or
> through the output i2c register), but you are implementing it through
> the intr channel... :-S
Again, "write on INTR" is OUTPUT channel for me. So I guess with the
explanation above we are fine here?
>> +
>> + - int (*hidinput_input_event) (struct input_dev *idev, unsigned int type,
>> + unsigned int code, int value)
>> + Obsolete callback used by logitech converters. It is called when userspace
>> + writes input events to the input device (eg., EV_LED). A driver can use this
>> + callback to convert it into an output report and send it to the device. If
>> + this callback is not provided, HID core will use ->request() or
>> + ->raw_request() respectively.
>
> I bet there will be a way to make this work with logitech devices too
> (if we implement a proper ->hid_output_raw_report() in each paired devices).
Yeah, I thought so, but I have no idea what the logitech-dj driver
does. I guess we can drop this once we have no more users, but I
wanted to avoid pushing to hard on it.
>> +
>> + - int (*idle) (struct hid_device *hdev, int report, int idle, int reqtype)
>> + Perform SET/GET_IDLE request. Only used by USB-HID, do not implement!
>> +
>> +2.3) Data Path
>> +--------------
>> +
>> +Transport drivers are responsible of reading data from I/O devices. They must
>> +handle any state-tracking themselves. HID core does not implement protocol
>
> I don't get the "state-tracking" here. The reports states should be
> handled by core, and I do not see the other states (or you meant PM
> states?).
Ugh, I meant I/O-related state-tracking (or PM). I will rephrase that.
>> +handshakes or other management commands which can be required by the given HID
>> +transport specification.
>> +
>> +Every raw data packet read from a device must be fed into HID core via
>> +hid_input_report(). You must specify the channel-type (intr or ctrl) and report
>> +type (input/output/feature). Under normal conditions, only input reports are
>> +provided via this API.
>> +
>> +Responses to GET_REPORT requests via ->request() must also be provided via this
>> +API. Responses to ->raw_request() are synchronous and must be intercepted by the
>> +transport driver and not passed to hid_input_report().
>> +Acknowledgements to SET_REPORT requests are not of interest to HID core.
>> +
>> +----------------------------------------------------
>> +Written 2013, David Herrmann <dh.herrmann@gmail.com>
>> --
>> 1.8.3.2
>>
>
> done!
> Many thanks for this David. It was very interesting and detailed. It
> will make a great documentation.
Thanks a lot for reviewing. I will fix the remaining issues and with
the "write-on-INTR is OUTPUT" I guess we are on the same page (except
for minor issues)?
Cheers
David
^ permalink raw reply
* [PATCH v5] Input: sysrq - DT binding for key sequence
From: mathieu.poirier @ 2013-07-17 14:34 UTC (permalink / raw)
To: grant.likely, dmitry.torokhov
Cc: devicetree-discuss, john.stultz, linux-input, kernel-team,
mathieu.poirier
From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>
Adding a simple device tree binding for the specification of key sequences.
Definition of the keys found in the sequence are located in
'include/uapi/linux/input.h'.
For the sysrq driver, holding the sequence of keys down for a specific amount of time
will reset the system.
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
changes for v5:
- Corrected error in binding definition.
- Using helper macro to fetch the key sequence.
- Removing white space.
---
.../devicetree/bindings/input/input-reset.txt | 34 ++++++++++++++++++++
drivers/tty/sysrq.c | 37 ++++++++++++++++++++++
2 files changed, 71 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/input-reset.txt
diff --git a/Documentation/devicetree/bindings/input/input-reset.txt b/Documentation/devicetree/bindings/input/input-reset.txt
new file mode 100644
index 0000000..c69390c
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/input-reset.txt
@@ -0,0 +1,34 @@
+Input: sysrq reset sequence
+
+A simple binding to represent a set of keys as described in
+include/uapi/linux/input.h. This is to communicate a
+sequence of keys to the sysrq driver. Upon holding the keys
+for a specified amount of time (if specified) the system is
+sync'ed and reset.
+
+Key sequences are global to the system but all the keys in a
+set must be coming from the same input device.
+
+The /chosen node should contain a 'linux,sysrq-reset-seq' child
+node to define a set of keys.
+
+Required property:
+sysrq-reset-seq: array of keycodes
+
+Optional property:
+timeout-ms: duration keys must be pressed together in milliseconds
+before generating a sysrq
+
+Example:
+
+ chosen {
+ linux,sysrq-reset-seq {
+ keyset = <0x03
+ 0x04
+ 0x0a>;
+ timeout-ms = <3000>;
+ };
+ };
+
+Would represent KEY_2, KEY_3 and KEY_9.
+
diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index b51c154..ed8f00f 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -44,6 +44,7 @@
#include <linux/uaccess.h>
#include <linux/moduleparam.h>
#include <linux/jiffies.h>
+#include <linux/of.h>
#include <asm/ptrace.h>
#include <asm/irq_regs.h>
@@ -671,6 +672,34 @@ static void sysrq_detect_reset_sequence(struct sysrq_state *state,
}
}
+static void sysrq_of_get_keyreset_config(void)
+{
+ u32 key;
+ struct device_node *np;
+ struct property *prop;
+ const __be32 *p;
+
+ np = of_find_node_by_path("/chosen/linux,sysrq-reset-seq");
+ if (!np) {
+ pr_debug("No sysrq node found");
+ return;
+ }
+
+ /* reset in case a __weak definition was present */
+ sysrq_reset_seq_len = 0;
+
+ of_property_for_each_u32(np, "keyset", prop, p, key) {
+ if ((key == KEY_RESERVED || key > KEY_MAX) ||
+ (sysrq_reset_seq_len == SYSRQ_KEY_RESET_MAX))
+ break;
+
+ sysrq_reset_seq[sysrq_reset_seq_len++] = (unsigned short)key;
+ }
+
+ /* get reset timeout if any */
+ of_property_read_u32(np, "timeout-ms", &sysrq_reset_downtime_ms);
+}
+
static void sysrq_reinject_alt_sysrq(struct work_struct *work)
{
struct sysrq_state *sysrq =
@@ -903,6 +932,7 @@ static inline void sysrq_register_handler(void)
int error;
int i;
+ /* first check if a __weak interface was instantiated */
for (i = 0; i < ARRAY_SIZE(sysrq_reset_seq); i++) {
key = platform_sysrq_reset_seq[i];
if (key == KEY_RESERVED || key > KEY_MAX)
@@ -911,6 +941,13 @@ static inline void sysrq_register_handler(void)
sysrq_reset_seq[sysrq_reset_seq_len++] = key;
}
+ /*
+ * DT configuration takes precedence over anything
+ * that would have been defined via the __weak
+ * interface
+ */
+ sysrq_of_get_keyreset_config();
+
error = input_register_handler(&sysrq_handler);
if (error)
pr_err("Failed to register input handler, error %d", error);
--
1.8.1.2
^ permalink raw reply related
* Re: [RFC 3/8] HID: input: generic hidinput_input_event handler
From: David Herrmann @ 2013-07-17 13:58 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: linux-input, Jiri Kosina, Henrik Rydberg, Oliver Neukum
In-Reply-To: <CAN+gG=Hty+R0D38-pJV-3J4oVEGfezh14A2XB0ppkbbXOx5Nuw@mail.gmail.com>
Hi
On Tue, Jul 16, 2013 at 10:04 AM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Mon, Jul 15, 2013 at 7:10 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> The hidinput_input_event() callback converts input events written from
>> userspace into HID reports and sends them to the device. We currently
>> implement this in every HID transport driver, even though most of them do
>> the same.
>>
>> This provides a generic hidinput_input_event() implementation which is
>> mostly copied from usbhid. It uses a delayed worker to allow multiple LED
>> events to be collected into a single output event.
>> We use the custom ->request() transport driver callback to allow drivers
>> to adjust the outgoing report and handle the request asynchronously. If no
>> custom ->request() callback is available, we fall back to the generic raw
>> output report handler (which is synchronous).
>>
>> Drivers can still provide custom hidinput_input_event() handlers (see
>> logitech-dj) if the generic implementation doesn't fit their needs.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> ---
>> drivers/hid/hid-input.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++-
>> include/linux/hid.h | 1 +
>> 2 files changed, 80 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index 7480799..308eee8 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -1137,6 +1137,74 @@ unsigned int hidinput_count_leds(struct hid_device *hid)
>> }
>> EXPORT_SYMBOL_GPL(hidinput_count_leds);
>>
>> +static void hidinput_led_worker(struct work_struct *work)
>> +{
>> + struct hid_device *hid = container_of(work, struct hid_device,
>> + led_work);
>> + struct hid_field *field;
>> + struct hid_report *report;
>> + int len;
>> + __u8 *buf;
>> +
>> + field = hidinput_get_led_field(hid);
>> + if (!field)
>> + return;
>> +
>> + /*
>> + * field->report is accessed unlocked regarding HID core. So there might
>> + * be another incoming SET-LED request from user-space, which changes
>> + * the LED state while we assemble our outgoing buffer. However, this
>> + * doesn't matter as hid_output_report() correctly converts it into a
>> + * boolean value no matter what information is currently set on the LED
>> + * field (even garbage). So the remote device will always get a valid
>> + * request.
>> + * And in case we send a wrong value, a next led worker is spawned
>> + * for every SET-LED request so the following worker will send the
>> + * correct value, guaranteed!
>> + */
>> +
>> + report = field->report;
>> +
>> + /* use custom SET_REPORT request if possible (asynchronous) */
>> + if (hid->ll_driver->request)
>> + return hid->ll_driver->request(hid, report, HID_REQ_SET_REPORT);
>> +
>> + /* fall back to generic raw-output-report */
>> + len = ((report->size - 1) >> 3) + 1 + (report->id > 0);
>> + buf = kmalloc(len, GFP_KERNEL);
>> + if (!buf)
>> + return;
>> +
>> + hid_output_report(report, buf);
>> + /* synchronous output report */
>> + hid->hid_output_raw_report(hid, buf, len, HID_OUTPUT_REPORT);
>> + kfree(buf);
>> +}
>
> Instead of writing a specific fallback in case hid->ll_driver->request
> does not exist, I think it would make sense to implement a generic
> hid_hw_request function in hid-input, so that HIDP and UHID will
> benefit from it. I think it will be better because the implementation
> I made in i2c-hid.c is nearly the exact same calls than the fallback
> here.
Yepp, that sounds about right. However, with the current
hid_output_raw_report() callbacks we cannot implement this as they
work differently on each transport driver. That's why I introduced the
raw_request() and output_report() helpers. These will allow me to do
that.
So Jiri, feel free to drop this patch and I will rebase it on the new
series at the end with the new helpers. Otherwise, I will add a patch
at the end which provides a generic fallback for request().
>> +
>> +static int hidinput_input_event(struct input_dev *dev, unsigned int type,
>> + unsigned int code, int value)
>> +{
>> + struct hid_device *hid = input_get_drvdata(dev);
>> + struct hid_field *field;
>> + int offset;
>> +
>> + if (type == EV_FF)
>> + return input_ff_event(dev, type, code, value);
>> +
>> + if (type != EV_LED)
>> + return -1;
>> +
>> + if ((offset = hidinput_find_field(hid, type, code, &field)) == -1) {
>> + hid_warn(dev, "event field not found\n");
>> + return -1;
>> + }
>> +
>> + hid_set_field(field, offset, value);
>> +
>> + schedule_work(&hid->led_work);
>> + return 0;
>> +}
>> +
>> static int hidinput_open(struct input_dev *dev)
>> {
>> struct hid_device *hid = input_get_drvdata(dev);
>> @@ -1183,7 +1251,10 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
>> }
>>
>> input_set_drvdata(input_dev, hid);
>> - input_dev->event = hid->ll_driver->hidinput_input_event;
>> + if (hid->ll_driver->hidinput_input_event)
>> + input_dev->event = hid->ll_driver->hidinput_input_event;
>> + else if (hid->ll_driver->request || hid->hid_output_raw_report)
>> + input_dev->event = hidinput_input_event;
>
> with a generic hid_hw_request in hid-input, the else is simpler here.
>
>> input_dev->open = hidinput_open;
>> input_dev->close = hidinput_close;
>> input_dev->setkeycode = hidinput_setkeycode;
>> @@ -1278,6 +1349,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>> int i, j, k;
>>
>> INIT_LIST_HEAD(&hid->inputs);
>> + INIT_WORK(&hid->led_work, hidinput_led_worker);
>>
>> if (!force) {
>> for (i = 0; i < hid->maxcollection; i++) {
>> @@ -1379,6 +1451,12 @@ void hidinput_disconnect(struct hid_device *hid)
>> input_unregister_device(hidinput->input);
>> kfree(hidinput);
>> }
>> +
>> + /* led_work is spawned by input_dev callbacks, but doesn't access the
>> + * parent input_dev at all. Once all input devices are removed, we
>> + * know that led_work will never get restarted, so we can cancel it
>> + * synchronously and are safe. */
>> + cancel_work_sync(&hid->led_work);
>
> You missed the multi-lines comment formatting style on this one :)
The ./net/ subsystem uses these comments quite a lot and there was a
discussion between davem and linus with the conclusion that these
comments are ok. But I actually don't care, so I can change to normal
CodingStyle.
Thanks for reviewing!
David
>> }
>> EXPORT_SYMBOL_GPL(hidinput_disconnect);
>>
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index b8058c5..ea4b828 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -456,6 +456,7 @@ struct hid_device { /* device report descriptor */
>> enum hid_type type; /* device type (mouse, kbd, ...) */
>> unsigned country; /* HID country */
>> struct hid_report_enum report_enum[HID_REPORT_TYPES];
>> + struct work_struct led_work; /* delayed LED worker */
>>
>> struct semaphore driver_lock; /* protects the current driver, except during input */
>> struct semaphore driver_input_lock; /* protects the current driver */
>> --
>> 1.8.3.2
>>
>
> Cheers,
> Benjamin
^ permalink raw reply
* Re: [PATCH -next] Input: olpc_apsp - remove redundant dev_err call in olpc_apsp_probe()
From: Daniel Drake @ 2013-07-17 13:00 UTC (permalink / raw)
To: Wei Yongjun
Cc: Dmitry Torokhov, Grant Likely, rob.herring, yongjun_wei,
linux-input, devicetree-discuss@lists.ozlabs.org
In-Reply-To: <CAPgLHd-tOWoxE-qYOkX_HcQRaUsifn0MQH=i+2KDARLKAxz2KA@mail.gmail.com>
On Tue, Jul 16, 2013 at 11:31 PM, Wei Yongjun <weiyj.lk@gmail.com> wrote:
> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
>
> There is a error message within devm_ioremap_resource
> already, so remove the dev_err call to avoid redundant
> error message.
>
> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Acked-by: Daniel Drake <dsd@laptop.org>
^ permalink raw reply
* Re: [Regression][v3.10][v3.11] Revert "HID: Fix logitech-dj: missing Unifying device issue"
From: Nestor Lopez Casado @ 2013-07-17 9:01 UTC (permalink / raw)
To: Andrew de los Reyes
Cc: Joseph Salisbury, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel@vger.kernel.org, Benjamin Tissoires
In-Reply-To: <CAAfQ=89uUcT9RAPSXN1AoJ9tBvAT-z66WuAvRsYUtEW66A7DoA@mail.gmail.com>
Added Benjamin.
On Mon, Jul 15, 2013 at 8:37 PM, Andrew de los Reyes <adlr@chromium.org> wrote:
> I believe this commit was an optimization, so it should be okay to revert
> it.
>
> Nestor, can you confirm?
5962640 is not an optimization, is a workaround for the dropped
packets during enumeration. After a9dd22b73085734 it should not be
necessary as the issue should be resolved.
Nevertheless, it turns out that the workaround not only helps in the
case that some input reports get dropped (which no longer happens),
but also in the case that the underlying usb controllers fail to send
the report that request the enumeration of the Unifying receiver.
We've seen this ocassionally depending on the usb controllers and some
time dependent behaviour.
Concluding I'd say that this patch being otherwise harmless, albeit
ugly, could be put back in. I'd suggest as well investigating why the
usb controllers fails to send the enumeration request with -EPIPE, as
this my be hiding a race inside those usb drivers.
If we re-add 596264082f10dd4. We should fix setting querying_devices
flag to true. This was not done in the original patch. As the impact
was not seen and the patch was to be removed anyway it was never
corrected.
>
>
> On Mon, Jul 15, 2013 at 10:26 AM, Joseph Salisbury
> <joseph.salisbury@canonical.com> wrote:
>>
>> Hi Andrew,
>>
>> A bug was opened against the Ubuntu kernel[0]. It was found that
>> reverting the following commit resolved this bug:
>>
>> commit 8af6c08830b1ae114d1a8b548b1f8b056e068887
>> Author: Andrew de los Reyes <adlr@chromium.org>
>> Date: Mon Feb 18 09:20:23 2013 -0800
>>
>> Revert "HID: Fix logitech-dj: missing Unifying device issue"
>>
>>
>> The regression was introduced as of v3.10-rc1. Commit 8af6c08 reverts
>> commit 5962640, so it seems commit 5962640 may still be needed.
>>
>> I see that you are the author of this patch, so I wanted to run this by
>> you. I was thinking of requesting a revert for v3.11, but I wanted to
>> get your feedback first.
>>
>>
>> Thanks,
>>
>> Joe
>>
>> [0] http://pad.lv/1194649
>
>
Cheers,
Nestor
^ permalink raw reply
* [PATCH -next] Input: olpc_apsp - remove redundant dev_err call in olpc_apsp_probe()
From: Wei Yongjun @ 2013-07-17 5:31 UTC (permalink / raw)
To: dmitry.torokhov, grant.likely, rob.herring, dsd
Cc: yongjun_wei, linux-input, devicetree-discuss
From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
There is a error message within devm_ioremap_resource
already, so remove the dev_err call to avoid redundant
error message.
Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
drivers/input/serio/olpc_apsp.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/input/serio/olpc_apsp.c b/drivers/input/serio/olpc_apsp.c
index 818aa46..c96e3bd 100644
--- a/drivers/input/serio/olpc_apsp.c
+++ b/drivers/input/serio/olpc_apsp.c
@@ -187,10 +187,8 @@ static int olpc_apsp_probe(struct platform_device *pdev)
return -ENOENT;
priv->base = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(priv->base)) {
- dev_err(&pdev->dev, "Failed to map WTM registers\n");
+ if (IS_ERR(priv->base))
return PTR_ERR(priv->base);
- }
priv->irq = platform_get_irq(pdev, 0);
if (priv->irq < 0)
^ permalink raw reply related
* Re: [PATCH v4] Input: sysrq - DT binding for key sequence
From: Rob Herring @ 2013-07-17 2:17 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Grant Likely, Dmitry Torokhov, kernel-team, devicetree-discuss,
John Stultz, linux-input@vger.kernel.org
In-Reply-To: <CANLsYkzgHqnuDq46JEJH713jrnD8rAAG_2wLbKJCAOGLpw+e4g@mail.gmail.com>
On Tue, Jul 16, 2013 at 5:24 PM, Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
>
>
>
> On 15 July 2013 20:43, Rob Herring <robherring2@gmail.com> wrote:
>>
>> On 07/15/2013 10:36 AM, mathieu.poirier@linaro.org wrote:
>> > From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>
>> >
[snip]
>> > + prop = of_find_property(np, "keyset", NULL);
>> > + if (!prop || !prop->value) {
>> > + pr_err("Invalid input keyset");
>> > + goto out;
>> > + }
>> > +
>> > + count = prop->length / sizeof(u32);
>> > + val = prop->value;
>>
>> None of the existing helpers to retrieve property arrays doesn't work
>> here?
>
>
> The problem here is that we never know how long the sequence will be. As
> such the 'of_property_read_uXY_array' functions won't work. Or maybe you're
> referring to another set of helpers... If so, please clarify.
What about of_property_for_each_u32? Or we should make
of_property_read_uXY_array work with arrays that can be less than some
max size.
Rob
^ permalink raw reply
* Re: [PATCH 001/001] [Input:] Propagate hardware generated event timestamp to evdev.
From: Michael Wright @ 2013-07-16 23:22 UTC (permalink / raw)
To: Michael Wright
Cc: Dmitry Torokhov, Alexander Levitskiy, linux-input, linux-kernel,
linux-doc, Rob Landley, Henrik Rydberg, Arve Hjønnevåg,
Stephen Boyd
In-Reply-To: <CALDEARgU_qD88ExdufXkTFjaenVEwLC=q3t-RNwRrm4A-mMVZw@mail.gmail.com>
Apologies, forgot to make sure it was plain text.
On Tue, Jul 16, 2013 at 4:16 PM, Michael Wright <michaelwr@android.com> wrote:
> Hi Dmitry,
>
>
> On Fri, Jul 12, 2013 at 11:44 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> We already have MSC_TIMESTAMP (which is in usec), do you really need
>> nsec resolution?
>
> We don't actually need nsec resolution, usec resolution would be sufficient.
>
>
>> Why do you need this special handling? Can you simply have MSC_TIMESTAMP
>> to be delivered as part of the event packet and use it instead of the
>> event timestamp if userspace chooses to do so?
>
> We need stronger guarantees than MSC_TIMESTAMP gives us. Much of
> our system relies on the fact that timestamps produced by evdev are based on
> the systems monotonic clock, whereas MSC_TIMESTAMP provides no such
> guarantee.
> As for whether it's a MSC or SYN event, I don't think we really have
> preference.
>
> Thanks,
>
> Michael
^ permalink raw reply
* Re: [PATCH v4] Input: sysrq - DT binding for key sequence
From: Mathieu Poirier @ 2013-07-16 22:24 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree-discuss, Dmitry Torokhov, John Stultz,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Grant Likely,
kernel-team-z5hGa2qSFaRBDgjK7y7TUQ
In-Reply-To: <51E4B35D.8020000-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 6122 bytes --]
On 15 July 2013 20:43, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 07/15/2013 10:36 AM, mathieu.poirier-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org wrote:
> > From: "Mathieu J. Poirier" <mathieu.poirier-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >
> > Adding a simple device tree binding for the specification of key
> sequences.
> > Definition of the keys found in the sequence are located in
> > 'include/uapi/linux/input.h'.
> >
> > For the sysrq driver, holding the sequence of keys down for a specific
> amount of time
> > will reset the system.
> >
> > Signed-off-by: Mathieu Poirier <mathieu.poirier-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > ---
> > changes for v4:
> > - Moved seach of reset sequence nodes to a single call.
> > ---
> > .../devicetree/bindings/input/input-reset.txt | 34 ++++++++++++++
> > drivers/tty/sysrq.c | 54
> ++++++++++++++++++++++
> > 2 files changed, 88 insertions(+)
> > create mode 100644
> Documentation/devicetree/bindings/input/input-reset.txt
> >
> > diff --git a/Documentation/devicetree/bindings/input/input-reset.txt
> b/Documentation/devicetree/bindings/input/input-reset.txt
> > new file mode 100644
> > index 0000000..79504af
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/input-reset.txt
> > @@ -0,0 +1,34 @@
> > +Input: sysrq reset sequence
> > +
> > +A simple binding to represent a set of keys as described in
> > +include/uapi/linux/input.h. This is to communicate a
> > +sequence of keys to the sysrq driver. Upon holding the keys
> > +for a specified amount of time (if specified) the system is
> > +sync'ed and reset.
> > +
> > +Key sequences are global to the system but all the keys in a
> > +set must be coming from the same input device.
> > +
> > +The /chosen node should contain a 'linux,sysrq-reset-seq' child
> > +node to define a set of keys.
> > +
> > +Required property:
> > +sysrq-reset-seq: array of keycodes
> > +
> > +Optional property:
> > +timeout-ms: duration keys must be pressed together in microseconds
>
> milliseconds (ms) or microseconds (us)?
>
>
Right, milliseconds.
> > +before generating a sysrq
> > +
> > +Example:
> > +
> > + chosen {
> > + linux,sysrq-reset-seq {
> > + keyset = <0x03
> > + 0x04
> > + 0x0a>;
> > + timeout-ms = <3000>;
> > + };
> > + };
> > +
> > +Would represent KEY_2, KEY_3 and KEY_9.
> > +
> > diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> > index b51c154..01a8729 100644
> > --- a/drivers/tty/sysrq.c
> > +++ b/drivers/tty/sysrq.c
> > @@ -44,6 +44,7 @@
> > #include <linux/uaccess.h>
> > #include <linux/moduleparam.h>
> > #include <linux/jiffies.h>
> > +#include <linux/of.h>
> >
> > #include <asm/ptrace.h>
> > #include <asm/irq_regs.h>
> > @@ -671,6 +672,50 @@ static void sysrq_detect_reset_sequence(struct
> sysrq_state *state,
> > }
> > }
> >
> > +static void sysrq_of_get_keyreset_config(void)
> > +{
> > + unsigned short key;
> > + struct device_node *np;
> > + const struct property *prop;
> > + const __be32 *val;
> > + int count, i;
> > +
> > + np = of_find_node_by_path("/chosen/linux,sysrq-reset-seq");
> > + if (!np) {
> > + pr_debug("No sysrq node found");
> > + goto out;
> > + }
> > +
> > + prop = of_find_property(np, "keyset", NULL);
> > + if (!prop || !prop->value) {
> > + pr_err("Invalid input keyset");
> > + goto out;
> > + }
> > +
> > + count = prop->length / sizeof(u32);
> > + val = prop->value;
>
> None of the existing helpers to retrieve property arrays doesn't work here?
>
The problem here is that we never know how long the sequence will be. As
such the 'of_property_read_uXY_array' functions won't work. Or maybe
you're referring to another set of helpers... If so, please clarify.
>
> > +
> > + if (count > SYSRQ_KEY_RESET_MAX)
> > + count = SYSRQ_KEY_RESET_MAX;
> > +
> > + /* reset in case a __weak definition was present */
> > + sysrq_reset_seq_len = 0;
> > +
> > + for (i = 0; i < count; i++) {
> > + key = (unsigned short)be32_to_cpup(val++);
> > + if (key == KEY_RESERVED || key > KEY_MAX)
> > + break;
> > +
> > + sysrq_reset_seq[sysrq_reset_seq_len++] = key;
> > + }
> > +
> > + /* get reset timeout if any */
> > + of_property_read_u32(np, "timeout-ms", &sysrq_reset_downtime_ms);
> > +
> > + out:
> > + ;
> > +}
> > +
> > static void sysrq_reinject_alt_sysrq(struct work_struct *work)
> > {
> > struct sysrq_state *sysrq =
> > @@ -688,6 +733,7 @@ static void sysrq_reinject_alt_sysrq(struct
> work_struct *work)
> > input_inject_event(handle, EV_KEY, KEY_SYSRQ, 1);
> > input_inject_event(handle, EV_SYN, SYN_REPORT, 1);
> >
> > +
>
> spurious ws change
> > input_inject_event(handle, EV_KEY, KEY_SYSRQ, 0);
> > input_inject_event(handle, EV_KEY, alt_code, 0);
> > input_inject_event(handle, EV_SYN, SYN_REPORT, 1);
> > @@ -903,6 +949,7 @@ static inline void sysrq_register_handler(void)
> > int error;
> > int i;
> >
> > + /* first check if a __weak interface was instantiated */
> > for (i = 0; i < ARRAY_SIZE(sysrq_reset_seq); i++) {
> > key = platform_sysrq_reset_seq[i];
> > if (key == KEY_RESERVED || key > KEY_MAX)
> > @@ -911,6 +958,13 @@ static inline void sysrq_register_handler(void)
> > sysrq_reset_seq[sysrq_reset_seq_len++] = key;
> > }
> >
> > + /*
> > + * DT configuration takes precedence over anything
> > + * that would have been defined via the __weak
> > + * interface
> > + */
> > + sysrq_of_get_keyreset_config();
> > +
> > error = input_register_handler(&sysrq_handler);
> > if (error)
> > pr_err("Failed to register input handler, error %d",
> error);
> >
>
>
[-- Attachment #1.2: Type: text/html, Size: 8213 bytes --]
[-- Attachment #2: Type: text/plain, Size: 192 bytes --]
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
^ permalink raw reply
* Re: [RFC 7/8] HID: add transport driver documentation
From: Benjamin Tissoires @ 2013-07-16 10:32 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-input, Jiri Kosina, Henrik Rydberg, Oliver Neukum
In-Reply-To: <1373908217-16748-8-git-send-email-dh.herrmann@gmail.com>
Hi David,
On Mon, Jul 15, 2013 at 7:10 PM, David Herrmann <dh.herrmann@gmail.com>
wrote:
> hid-transport.txt describes the transport driver layout which is required
> by HID-core to work correctly. HID-core supports many different transport
> drivers and new drivers can be added for any kind of I/O system.
>
> The current layout doesn't really differentiate between intr and ctrl
> channels, which confuses some hid-drivers and may break devices. HIDP,
> USBHID and I2DHID all implement different semantics for each callback, so
typo: I2CHID
> our device drivers aren't really transport-driver-agnostic.
>
> To solve that, hid-transport.txt describes the layout of transport drivers
> that is required by HID core. Furthermore, it introduces some new
> callbacks which replace the current hid_get_raw_report() and
> hid_output_raw_report() hooks. These will be implemented in follow-up
> patches.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> Documentation/hid/hid-transport.txt | 299 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 299 insertions(+)
> create mode 100644 Documentation/hid/hid-transport.txt
>
> diff --git a/Documentation/hid/hid-transport.txt b/Documentation/hid/hid-transport.txt
> new file mode 100644
> index 0000000..034f11d
> --- /dev/null
> +++ b/Documentation/hid/hid-transport.txt
> @@ -0,0 +1,299 @@
> + HID I/O Transport Drivers
> + ===========================
> +
> +The HID subsystem is independent of the underlying transport driver. Initially,
> +only USB was supported, but other specifications adopted the HID design and
> +provided new transport drivers. The kernel includes at least support for USB,
> +Bluetooth, i2c and user-space I/O drivers.
IIRC, we should put I2C in capital letters.
> +
> +1) HID Bus
> +==========
> +
> +The HID subsystem is designed as a bus. Any I/O subsystem may provide HID
> +devices and register them with the HID bus. HID core then loads generic device
> +drivers on top of it. The transport drivers are responsible of raw data
> +transport and device setup/management. HID core is responsible of
> +report-parsing, report interpretation and the user-space API. Device specifics
> +and quirks are also handled by HID core and the HID device drivers.
Hmm, the quirks part is not exactly what is currently implemented.
Usbhid (Transport Driver) set and use quirks while you say here that
they are handled by hid-core.
I personally prefer the way it is documented here (quirks handled in
hid-core) so HIDP, i2c-hid and uhid will benefit from the dynamic quirks
already implemented in usbhid. But that will require another patch :)
> +
> + +-----------+ +-----------+ +-----------+ +-----------+
> + | Device #1 | | Device #i | | Device #j | | Device #k |
> + +-----------+ +-----------+ +-----------+ +-----------+
> + \\ // \\ //
> + +------------+ +------------+
> + | I/O Driver | | I/O Driver |
> + +------------+ +------------+
> + || ||
> + +------------------+ +------------------+
> + | Transport Driver | | Transport Driver |
> + +------------------+ +------------------+
> + \___ ___/
> + \ /
> + +----------------+
> + | HID Core |
> + +----------------+
> + / | | \
> + / | | \
> + ____________/ | | \_________________
> + / | | \
> + / | | \
> + +----------------+ +-----------+ +------------------+ +------------------+
> + | Generic Driver | | MT Driver | | Custom Driver #1 | | Custom Driver #2 |
> + +----------------+ +-----------+ +------------------+ +------------------+
> +
> +Example Drivers:
> + I/O: USB, I2C, Bluetooth-l2cap
> + Transport: USB-HID, I2C-HID, BT-HIDP
> +
> +Normally, a transport driver is specific for a given I/O driver. But the HID
> +design also allows transport drivers to work with different I/O systems at the
> +same time.
I do not follow you here. Which transport driver you have in mind?
> +
> +Everything below "HID Core" is simplified in this graph as it is only of
> +interest to HID device drivers. Transport drivers do not need to know the
> +specifics.
> +
> +1.1) Device Setup
> +-----------------
> +
> +I/O drivers normally provide hotplug detection or device enumeration APIs to the
> +transport drivers. Transport drivers use this to find any suitable HID device.
> +They allocate HID device objects and register them with HID core. Transport
> +drivers are not required to register themselves with HID core. HID core is never
> +aware of which transport drivers are available and is not interested in it. It
> +is only interested in devices.
> +
> +Transport drivers attach a constant "struct hid_ll_driver" object with each
> +device. Once a device is registered with HID core, the callbacks provided via
> +this struct are used by HID core to communicate with the device.
> +
> +Transport drivers are responsible of detecting device failures and unplugging.
> +HID core will operate an device as long as it is registered regardless of any
typo: ^^ "a"
> +device failures. Once transport drivers detect unplug or failure events, they
> +must unregister the device from HID core and HID core will stop using the
> +provided callbacks.
> +
> +1.2) Transport Driver Requirements
> +----------------------------------
> +
> +HID core requires transport drivers to follow a given design. A Transport
> +drivers must provide two bi-directional I/O channels to each HID device. These
> +channels might be multiplexed on a single physical channel, though.
> +
> + - Interrupt Channel (intr): The intr channel is used for asynchronous data
> + reports. No management commands or data acknowledgements are sent on this
> + channel. Any unrequested incoming or outgoing data report must be sent on
> + this channel and is never acknowledged by the remote side. Devices usually
> + send their input events on this channel. Outgoing events are normally
> + not send via intr, except if high throughput is required.
> + - Control Channel (ctrl): The ctrl channel is used for synchronous requests and
> + device management. Unrequested data input events must not be sent on this
> + channel and are normally ignored. Instead, devices only send management
> + events or answers to host requests on this channel.
> + Outgoing reports are usually sent on the ctrl channel via synchronous
> + SET_REPORT requests.
This is not entirely true for USB and I2C:
- for USB, OUTPUT reports can also be sent through a direct write on an URB.
- for I2C, the notion of intr vs ctrl is slightly different, but it
remains roughly the same. Let me detail it so that you can make your idea:
The HID descriptors declares three I2C registers:
* the "Input" register: this one correspond to the intr you are
describing. The difference is that this register is only used from the
communication from the device to the host (no outputs here). The big
interest in this register is that the communication is device initiated
thanks to an external line triggered by the device (in I2C, the
communication is only master (host) initiated).
* the "Command" register: this one corresponds to the ctrl channel.
This channel/register is host initiated too (like on the USB side), and
there is a slight head over due to the HID protocol regarding sending
output events.
* the "Output" register: this one corresponds to the URBs described in
USB. Every communication coming from the host on this channel is
considered as an OUTPUT report, reducing the HID protocol over head.
The HID/I2C also declares a forth register (the "data") register which
is used by the HID protocol while using the "Command" register.
To sum up, I think the differences between the channels are:
- Interrupt Channel (intr): asynchronous, device initiated, no
acknowledgements (beside the fact that data has been read), read only.
- Control Channel (ctrl): synchronous, host initiated, acknowledged
(because synchronous), read/write
- Output Channel (out): synchronous, host initiated, acknowledged
(because synchronous), write only
Then, the usbhid transport layer makes the ctrl and out channels
asynchronous...
> +
> +Communication between devices and HID core is mostly done via HID reports. A
> +report can be of one of three types:
> +
> + - INPUT Report: Input reports provide data from device to host. This
> + data may include button events, axis events, battery status or more. This
> + data is generated by the device and sent to the host without requiring
_with_ or without requiring explicit requests.
On I2C, the command GET_REPORT is even mandatory (which does not seems
to be the case for USB, given the amount of QUIRK_NO_INIT_REPORT we have.)
> + explicit requests. Devices can choose to send data continously or only on
My spell checker gives me a typo on: "continuously"
> + change.
> + - OUTPUT Report: Output reports change device states. They are sent from host
> + to device and may include LED requests, rumble requests or more. Output
> + reports are never sent from device to host, except if explicitly requested.
In this case, I think it would be better to say that Output reports are
never sent from device to host, but the host can _retrieve_ its current
state.
> + Hosts may choose to send output reports either continously or only on change.
My spell checker gives me a typo on: "continuously"
> + - FEATURE Report: Feature reports can be anything that doesn't belong into the
> + other two categories. Some of them are generic and defined by the HID spec,
> + but most of them are used as custom device extensions. For instance, they may
> + provide battery status information.
I disagree. FEATURE are specific reports which are read/write, and not
spontaneously emitted by the device. For instance, this allows the host
to change the current mode of the device (mouse emulation or multitouch
in the case I know most), or to set constants in the device (max
reported touches in my case, but this can be hardware macros for
keyboards, or something else).
> + Feature reports are never sent without requests. A host must explicitly
> + request a device to set or send a feature report. This also means, feature
I would prefer a "host must explicitly request a device to set or
_retrieve_ a feature report".
> + reports are never sent on the intr channel as this channel is asynchronous.
> +
> +INPUT and OUTPUT reports can be sent as pure data reports on the intr channel.
In my mind, OUTPUT reports are not sent through the intr channel
(because write only), but you already get my point I think :)
> +For INPUT reports this is the usual operational mode. But for OUTPUT reports,
> +this is rarely done as OUTPUT reports are normally quite rare. But devices are
> +free to make excessive use of asynchronous OUTPUT reports (for instance, custom
> +HID audio speakers make great use of it).
Ok, did not know about it.
> +
> +Raw reports must not be sent on the ctrl channel, though. Instead, the ctrl
> +channel provides synchronous GET/SET_REPORT requests.
> +
> + - GET_REPORT: A GET_REPORT request has a report ID as payload and is sent
> + from host to device. The device must answer with a data report for the
> + requested report ID on the ctrl channel as a synchronous acknowledgement.
Beware that the report ID is not mandatory in case the HID report
descriptors declares only 1 report without report ID. But I'm fine with
it anyway (it's understandable that way).
> + Only one GET_REPORT request can be pending for each device. This restriction
> + is enforced by HID core as several transport drivers don't allow multiple
> + simultaneous GET_REPORT requests.
> + Note that data reports which are sent as answer to a GET_REPORT request are
> + not handled as generic device events. That is, if a device does not operate
> + in continuous data reporting mode, an answer to GET_REPORT does not replace
> + the raw data report on the intr channel on state change.
> + GET_REPORT is only used by custom HID device drivers to query device state.
> + Normally, HID core caches any device state so this request is not necessary
> + on devices that follow the HID specs.
FYI, HID/I2C spec says: "GET_REPORT is often used by applications on
startup to retrieve the ``current state'' of the device rather than
waiting for the device to generate the next input/feature report".
And as under Linux applications do not talk directly to the hid devices,
I fully concurs to your point.
> + GET_REPORT requests can be sent for any of the 3 report types and shall
> + return the current report state of the device.
The HID/I2C spec explicitly says: "the DEVICE shall ignore a GET_REPORT
requests with the REPORT TYPE set to Output, as it is not used in this
specification." So under i2c, we can send GET_REPORT with OUTPUT, but we
will not get anything from the device (this is why it is forbidden by
the transport driver).
> + - SET_REPORT: A SET_REPORT request has a report ID plus data as payload. It is
> + sent from host to device and a device must update it's current report state
> + according to the given data. Any of the 3 report types can be used.
The HID/I2C spec explicitly says: "the DEVICE might choose to ignore
input SET_REPORT requests as meaningless."...
> + A device must answer with a synchronous acknowledgement. However, HID core
> + does not require transport drivers to forward this acknowledgement to HID
> + core.
> + Same as for GET_REPORT, only one SET_REPORT can be pending at a time. This
> + restriction is enforced by HID core as some transport drivers do not support
> + multiple synchronous SET_REPORT requests.
> +
> +Other ctrl-channel requests are supported by USB-HID but are not available
> +(or deprecated) in most other transport level specifications:
> +
> + - GET/SET_IDLE: Only used by USB-HID. Do not implement!
> + - GET/SET_PROTOCOL: Not used by HID core. Do not implement!
The I2C declares also:
- RESET: mandatory (reset the device at any time)
- SET_POWER: mandatory on the device side (request from host to device
to indicate preferred power setting).
> +
> +2) HID API
> +==========
> +
> +2.1) Initialization
> +-------------------
> +
> +Transport drivers normally use the following procedure to register a new device
> +with HID core:
> +
> + struct hid_device *hid;
> + int ret;
> +
> + hid = hid_allocate_device();
> + if (IS_ERR(hid)) {
> + ret = PTR_ERR(hid);
> + goto err_<...>;
> + }
> +
> + strlcpy(hid->name, <device-name-src>, 127);
> + strlcpy(hid->phys, <device-phys-src>, 63);
> + strlcpy(hid->uniq, <device-uniq-src>, 63);
> +
> + hid->ll_driver = &custom_ll_driver;
> + hid->bus = <device-bus>;
> + hid->vendor = <device-vendor>;
> + hid->product = <device-product>;
> + hid->version = <device-version>;
> + hid->country = <device-country>;
FYI, HID/I2C does not define any device-country field (I guess it will
come in a later release...)
> + hid->dev.parent = <pointer-to-parent-device>;
FYI, I have started implementing a devm API for HID (in the same way the
input devm API is implemented), and dev.parent should not be overwritten.
Anyway the two last comments are not requesting any changes in the document.
> + hid->driver_data = <transport-driver-data-field>;
> +
> + ret = hid_add_device(hid);
> + if (ret)
> + goto err_<...>;
> +
> +Once hid_add_device() is entered, HID core might use the callbacks provided in
> +"custom_ll_driver". To unregister a device, use:
^^^^
Maybe introduce a new paragraph (otherwise, it looks like
hid_destroy_device is called from HID core).
> +
> + hid_destroy_device(hid);
> +
> +Once hid_destroy_device() returns, HID core will no longer make use of any
> +driver callbacks.
> +
> +2.2) hid_ll_driver operations
> +-----------------------------
> +
> +The available HID callbacks are:
> + - int (*start) (struct hid_device *hdev)
> + Called from HID device drivers once they want to use the device. Transport
> + drivers can choose to setup their device in this callback. However, normally
> + devices are already set up before transport drivers register them to HID core
> + so this is mostly only used by USB-HID.
> +
> + - void (*stop) (struct hid_device *hdev)
> + Called from HID device drivers once they are done with a device. Transport
> + drivers can free any buffers and deinitialize the device. But note that
> + ->start() might be called again if another HID device driver is loaded on the
> + device.
> + Transport drivers are free to ignore it and deinitialize devices after they
> + destroyed them via hid_destroy_device().
> +
> + - int (*open) (struct hid_device *hdev)
> + Called from HID device drivers once they are interested in data reports.
> + Usually, while user-space didn't open any input API/etc., device drivers are
> + not interested in device data and transport drivers can put devices asleep.
> + However, once ->open() is called, transport drivers must be ready for I/O.
> + ->open() calls are never nested. So in between two ->open() calls there must
> + be a call to ->close().
This is not true in either USB or I2C. And I guess with every BT or UHID
devices. ->open() and ->close() are called upon open/close of the input
node (or hidraw, or hiddev). So If two clients are opening the same
input node, there will be two calls to ->open() without any call to
->close() in between.
I guess you mixed up this part with the ->start() ->stop() :)
> +
> + - void (*close) (struct hid_device *hdev)
> + Called from HID device drivers after ->open() was called but they are no
> + longer interested in device reports. (Usually if user-space closed any input
> + devices of the driver).
> + Transport drivers can put devices asleep and terminate any I/O. However,
> + ->start() may be called again if the device driver is interested in input
> + reports again.
> +
> + - int (*parse) (struct hid_device *hdev)
> + Called once during device setup after ->start() has been called. Transport
> + drivers must read the HID report-descriptor from the device and tell HID core
> + about it via hid_parse_report().
> +
> + - int (*power) (struct hid_device *hdev, int level)
> + Called by HID core to give PM hints to transport drivers. Usually this is
> + analogical to the ->open() and ->close() hints and redundant.
> +
> + - void (*request) (struct hid_device *hdev, struct hid_report *report,
> + int reqtype)
> + Send an HID request on the ctrl channel. "report" contains the report that
> + should be sent and "reqtype" the request type. Request-type can be
> + HID_REQ_SET_REPORT or HID_REQ_GET_REPORT.
> + This callback is optional. If not provided, HID core will assemble a raw
> + report following the HID specs and send it via the ->raw_request() callback.
> + The transport driver is free to implement this asynchronously.
> +
> + - int (*wait) (struct hid_device *hdev)
> + Used by HID core before calling ->request() again. A transport driver can use
> + it to wait for any pending requests to complete if only one request is
> + allowed at a time.
> +
> + - int (*raw_request) (struct hid_device *hdev, unsigned char reportnum,
> + __u8 *buf, size_t count, unsigned char rtype,
> + int reqtype)
> + Same as ->request() but provides the report as raw buffer. This request shall
> + be synchronous. A transport driver must not use ->wait() to complete such
> + requests.
2 questions/remarks here:
- is raw_request() meant to replace ->hid_output_raw_report() and
->hid_get_raw_report()?
- reportnum declared as an unsigned will be problematic regarding the
rare devices not having any report ID in their report descriptors.
> +
> + - int (*output_report) (struct hid_device *hdev, __u8 *buf, size_t len)
> + Send raw output report via intr channel. Used by some HID device drivers
> + which require high throughput for outgoing requests on the intr channel. This
> + must not cause SET_REPORT calls! This must be implemented as asynchronous
> + output report on the intr channel!
For me, there is something wrong here. The name infers that we are
trying to send an output_report directly (so through the USB URB or
through the output i2c register), but you are implementing it through
the intr channel... :-S
> +
> + - int (*hidinput_input_event) (struct input_dev *idev, unsigned int type,
> + unsigned int code, int value)
> + Obsolete callback used by logitech converters. It is called when userspace
> + writes input events to the input device (eg., EV_LED). A driver can use this
> + callback to convert it into an output report and send it to the device. If
> + this callback is not provided, HID core will use ->request() or
> + ->raw_request() respectively.
I bet there will be a way to make this work with logitech devices too
(if we implement a proper ->hid_output_raw_report() in each paired devices).
> +
> + - int (*idle) (struct hid_device *hdev, int report, int idle, int reqtype)
> + Perform SET/GET_IDLE request. Only used by USB-HID, do not implement!
> +
> +2.3) Data Path
> +--------------
> +
> +Transport drivers are responsible of reading data from I/O devices. They must
> +handle any state-tracking themselves. HID core does not implement protocol
I don't get the "state-tracking" here. The reports states should be
handled by core, and I do not see the other states (or you meant PM
states?).
> +handshakes or other management commands which can be required by the given HID
> +transport specification.
> +
> +Every raw data packet read from a device must be fed into HID core via
> +hid_input_report(). You must specify the channel-type (intr or ctrl) and report
> +type (input/output/feature). Under normal conditions, only input reports are
> +provided via this API.
> +
> +Responses to GET_REPORT requests via ->request() must also be provided via this
> +API. Responses to ->raw_request() are synchronous and must be intercepted by the
> +transport driver and not passed to hid_input_report().
> +Acknowledgements to SET_REPORT requests are not of interest to HID core.
> +
> +----------------------------------------------------
> +Written 2013, David Herrmann <dh.herrmann@gmail.com>
> --
> 1.8.3.2
>
done!
Many thanks for this David. It was very interesting and detailed. It
will make a great documentation.
Cheers,
Benjamin
^ permalink raw reply
* Re: [RFC 6/8] HID: uhid: use generic hidinput_input_event()
From: Benjamin Tissoires @ 2013-07-16 8:10 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-input, Jiri Kosina, Henrik Rydberg, Oliver Neukum
In-Reply-To: <1373908217-16748-7-git-send-email-dh.herrmann@gmail.com>
On Mon, Jul 15, 2013 at 7:10 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> HID core provides the same functionality and can convert the input event
> to a raw output report. We can thus drop UHID_OUTPUT_EV and rely on the
> mandatory UHID_OUTPUT.
>
> User-space wasn't able to do anything with UHID_OUTPUT_EV, anyway. They
> don't have access to the report fields.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
I personally don't use UHID_OUTPUT_EV in hid-replay (nor UHID_OUTPUT
either :-P ).
Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
^ permalink raw reply
* Re: [RFC 5/8] HID: i2c: use generic hidinput_input_event()
From: Benjamin Tissoires @ 2013-07-16 8:08 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-input, Jiri Kosina, Henrik Rydberg, Oliver Neukum
In-Reply-To: <1373908217-16748-6-git-send-email-dh.herrmann@gmail.com>
On Mon, Jul 15, 2013 at 7:10 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> HID core provides the same functionality, so drop the custom handler.
> Besides, the current handler doesn't schedule any outgoing report so it
> did not work, anyway.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
Yep, by re-reading it, the led are not updated at all... :(
Thanks!
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
^ permalink raw reply
* Re: [RFC 4/8] HID: usbhid: use generic hidinput_input_event()
From: Benjamin Tissoires @ 2013-07-16 8:06 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-input, Jiri Kosina, Henrik Rydberg, Oliver Neukum
In-Reply-To: <1373908217-16748-5-git-send-email-dh.herrmann@gmail.com>
On Mon, Jul 15, 2013 at 7:10 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> HID core provides the same functionality as we do, so drop the custom
> hidinput_input_event() handler.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
^ 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