Messages in this thread | | | Subject | Re: [PATCH v10 52/55] input: touchscreen: atmel_mxt_ts: Added sysfs entry for touchscreen status | From | "Wang, Jiada" <> | Date | Mon, 6 Apr 2020 15:59:25 +0900 |
| |
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. >
| |