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

["Wang, Jiada" <jiada_wang@mentor.com>]

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.
>