Re: [PATCH v10 52/55] input: touchscreen: atmel_mxt_ts: Added sysfs entry for touchscreen status
From: Dmitry Osipenko <digetx@gmail.com>
Date: 2020-03-31 15:08:55
Also in:
lkml
31.03.2020 13:50, Jiada Wang пишет: ...
+static void mxt_watchdog_work(struct work_struct *work)
+{
+ struct mxt_data *data =
+ container_of(work, struct mxt_data, watchdog_work.work);
+ u16 info_buf;
+ int ret;
+
+ if (data->suspended || data->in_bootloader ||
+ data->mxt_status.intp_triggered)
+ goto sched_work;Won't it become a problem if other thread puts device into suspended / bootloader state in the same time?
+ ret = __mxt_read_reg(data->client, 0, sizeof(info_buf), &info_buf);
+
+ if (ret) {
+ data->mxt_status.error_count++;
+ data->mxt_status.dev_status = false;
+ } else {
+ data->mxt_status.dev_status = true;
+ }
+
+sched_work:
+ schedule_delayed_work(&data->watchdog_work,
+ msecs_to_jiffies(MXT_WATCHDOG_TIMEOUT));
+}...
quoted hunk ↗ jump to hunk
@@ -4329,6 +4390,12 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) msleep(MXT_RESET_TIME); } + if (debug_state) { + INIT_DELAYED_WORK(&data->watchdog_work, mxt_watchdog_work); + schedule_delayed_work(&data->watchdog_work, + msecs_to_jiffies(MXT_WATCHDOG_TIMEOUT)); + } + error = mxt_initialize(data); if (error) goto err_free_object;@@ -4343,6 +4410,8 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) return 0; err_free_object: + if (debug_state) + cancel_delayed_work_sync(&data->watchdog_work); mxt_free_input_device(data); mxt_free_object_table(data); if (data->reset_gpio) {@@ -4367,6 +4436,9 @@ static int mxt_remove(struct i2c_client *client) mxt_free_input_device(data); mxt_free_object_table(data); + if (debug_state) + cancel_delayed_work_sync(&data->watchdog_work);
What will happen if debug_state was false during of mxt_probe() and then the debug_state parameter was changed to true via sysfs? I think the INIT_DELAYED_WORK() and cancel_delayed_work_sync() should be invoked unconditionally.
quoted hunk ↗ jump to hunk
return 0; }@@ -4463,3 +4535,7 @@ module_i2c_driver(mxt_driver); MODULE_AUTHOR("Joonyoung Shim <jy0922.shim@samsung.com>"); MODULE_DESCRIPTION("Atmel maXTouch Touchscreen driver"); MODULE_LICENSE("GPL"); + +module_param(debug_state, bool, 0); +MODULE_PARM_DESC(debug_state, "Enable/Disable watchdog work to check device state (default=" + __MODULE_STRING(MXT_DEBUG_STATE) ")");
The "default=..." part of MODULE_PARM_DESC() probably isn't really useful and could be omitted, don't you think so?