Message ID | 20200331105051.58896-53-jiada_wang@mentor.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | atmel_mxt_ts misc | expand |
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)); > +} ... > @@ -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. > 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?
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@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? > 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
01.04.2020 15:51, Wang, Jiada пишет: > 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 Thank you for the clarification, I didn't realize that setting permission to 0 hides the parameter completely in sysfs. >> 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@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? >> > 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? If you want to keep the "default" kept in the description, then perhaps a simple "default=false" should be enough because nothing would be able to change it to true.
01.04.2020 17:33, Dmitry Osipenko пишет: > 01.04.2020 15:51, Wang, Jiada пишет: >> 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. Won't it be cleaner to stop/start the watchdog instead of messing with the locks? >>>> + 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 > > Thank you for the clarification, I didn't realize that setting > permission to 0 hides the parameter completely in sysfs. Anyways, I'm still thinking that the condition removal will make code cleaner a tad.
Hi Dmitry On 2020/04/02 1:10, Dmitry Osipenko wrote: > 01.04.2020 17:33, Dmitry Osipenko пишет: >> 01.04.2020 15:51, Wang, Jiada пишет: >>> 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. > > Won't it be cleaner to stop/start the watchdog instead of messing with > the locks? > will stop/start watchdog work when necessary in next version Thanks, Jiada >>>>> + 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 >> >> Thank you for the clarification, I didn't realize that setting >> permission to 0 hides the parameter completely in sysfs. > > Anyways, I'm still thinking that the condition removal will make code > cleaner a tad. >
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c index 14bd64d194b0..dda10bb51cb3 100644 --- a/drivers/input/touchscreen/atmel_mxt_ts.c +++ b/drivers/input/touchscreen/atmel_mxt_ts.c @@ -222,6 +222,7 @@ enum t100_type { #define MXT_CHG_DELAY 100 /* msec */ #define MXT_POWERON_DELAY 150 /* msec */ #define MXT_BOOTLOADER_WAIT 36E5 /* 1 minute */ +#define MXT_WATCHDOG_TIMEOUT 1000 /* msec */ /* Command to unlock bootloader */ #define MXT_UNLOCK_CMD_MSB 0xaa @@ -245,6 +246,9 @@ enum t100_type { #define DEBUG_MSG_MAX 200 +#define MXT_DEBUG_STATE false +static bool debug_state = MXT_DEBUG_STATE; + struct mxt_info { u8 family_id; u8 variant_id; @@ -317,6 +321,12 @@ struct mxt_flash { struct delayed_work work; }; +struct mxt_statusinfo { + bool dev_status; + bool intp_triggered; + u32 error_count; +}; + /* Each client has this additional data */ struct mxt_data { struct i2c_client *client; @@ -372,6 +382,8 @@ struct mxt_data { const char *pcfg_name; const char *input_name; struct mxt_flash *flash; + struct delayed_work watchdog_work; + struct mxt_statusinfo mxt_status; /* Cached parameters from object table */ u16 T5_address; @@ -1626,6 +1638,8 @@ static irqreturn_t mxt_interrupt(int irq, void *dev_id) struct mxt_data *data = dev_id; int ret; + data->mxt_status.intp_triggered = true; + if (data->in_bootloader) { complete(&data->chg_completion); @@ -1633,21 +1647,25 @@ static irqreturn_t mxt_interrupt(int irq, void *dev_id) cancel_delayed_work_sync(&data->flash->work); ret = mxt_check_bootloader(data); - return IRQ_RETVAL(ret); + ret = IRQ_RETVAL(ret); + goto exit; } - if (!data->object_table) - return IRQ_HANDLED; + if (!data->object_table) { + ret = IRQ_HANDLED; + goto exit; + } if (data->T44_address) ret = mxt_process_messages_t44(data); else ret = mxt_process_messages(data); - if (ret <= 0) - return IRQ_NONE; - else - return IRQ_HANDLED; + ret = (ret <= 0) ? IRQ_NONE : IRQ_HANDLED; + +exit: + data->mxt_status.intp_triggered = false; + return ret; } static int mxt_t6_command(struct mxt_data *data, u16 cmd_offset, @@ -2969,6 +2987,31 @@ static int mxt_bootloader_status(struct mxt_data *data) return 0; } +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; + + 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)); +} + static int mxt_initialize(struct mxt_data *data) { struct i2c_client *client = data->client; @@ -3956,6 +3999,22 @@ static const struct attribute_group mxt_fw_attr_group = { .attrs = mxt_fw_attrs, }; +static ssize_t touch_dev_stat_show(struct device *dev, struct + device_attribute * attr, char *buf) +{ + struct mxt_data *data = dev_get_drvdata(dev); + int ret = 0; + + if (data->mxt_status.dev_status) + data->mxt_status.error_count = 0; + + ret = snprintf(buf, PAGE_SIZE, "%d %d\n", data->mxt_status.dev_status, + data->mxt_status.error_count); + /* clear the error counter once it is read */ + data->mxt_status.error_count = 0; + return ret; +} + static DEVICE_ATTR_RO(fw_version); static DEVICE_ATTR_RO(hw_version); static DEVICE_ATTR(object, S_IRUGO, mxt_object_show, NULL); @@ -3965,6 +4024,7 @@ static DEVICE_ATTR_RW(debug_enable); static DEVICE_ATTR_RW(debug_v2_enable); static DEVICE_ATTR_RO(debug_notify); static DEVICE_ATTR_RW(t25_selftest); +static DEVICE_ATTR_RO(touch_dev_stat); static struct attribute *mxt_attrs[] = { &dev_attr_fw_version.attr, @@ -3976,6 +4036,7 @@ static struct attribute *mxt_attrs[] = { &dev_attr_debug_v2_enable.attr, &dev_attr_debug_notify.attr, &dev_attr_t25_selftest.attr, + &dev_attr_touch_dev_stat.attr, NULL }; @@ -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); + 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) ")");