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

[Wang, Jiada]

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.