Re: [PATCH v10 52/55] input: touchscreen: atmel_mxt_ts: Added sysfs entry for touchscreen status

[Dmitry Osipenko]

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.