Hi Dmitry
Thanks for your comments
On 2020/04/01 0:08, Dmitry Osipenko wrote:
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?
right, I will use mutex lock to prevent such case.
also I think data->mxt_status.intp_triggered isn't necessary,
when lock is used.
+ 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));
+}
...
@@ -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?
module_param debug_state is added with permission 0,
so it's value won't change during driver operation
I think the INIT_DELAYED_WORK() and cancel_delayed_work_sync() should be
invoked unconditionally.
return 0;
}
@@ -4463,3 +4535,7 @@ module_i2c_driver(mxt_driver);
MODULE_AUTHOR("Joonyoung Shim <jy0922.shim@xxxxxxxxxxx>");
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?
since debug_state can't be updated via sysfs, so I think add it's
default in MODULE_PARM_DESC() is useful? what's your opinion?
Thanks,
Jiada
