Hi Jingle,
On Wed, Jul 05, 2023 at 02:09:18PM +0800, Jingle.Wu wrote:
> HI Dmitry:
>
> 1.
> > +static void elan_input_lid_event(struct input_handle *handle,
> > +unsigned
> int type,
> > + unsigned int code, int value) {
> > + struct elan_tp_data *data, *n;
> > +
> > + if (type == EV_SW && code == SW_LID) {
> > + list_for_each_entry_safe(data, n,
> &elan_devices_with_lid_handler,
> > +list) {
>
> Why do you need the "_safe()" variant here?
> -> Because I took the above link as reference.
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3
> 578852/7/drivers/hid/hid-multitouch.c#394
It is wrong there too. The _safe() variant protects list traversal when
the list is being modified by the same thread, here we do not do that.
>
> 2.
> > + data->lid_switch = true;
> > + INIT_LIST_HEAD(&data->list);
> > + INIT_WORK(&data->lid_work, lid_work_handler);
> > + list_add_tail(&data->list, &elan_devices_with_lid_handler);
>
> It looks like you call elan_create_lid_handler() from elan_probe() which
> means it can be called several times (we should not assume there is only one
> controller), I do not see it being destroyed in remove() either, so it will
> break if you bind/unbind the driver.
>
> I also not sure why you need the list of you have a handler per device.
>
> -> If the elan_create_lid_handler() function not be put into probe(), the
> lid
> handler would be invalid in elan private data("struct elan_tp_data *data").
> Or do you have any suggestions for it? Thanks.
The handler's connect() is called for each matching device so you can
tie it up at that time.
>
> 3.
> > + error = elan_create_lid_handler(data);
> > + if (error)
> > + dev_err(dev, "failed to create lid handler: %d\n", error);
>
> Do we need this on _ALL_ devices with ELan controllers, or just certain
> ones? If we need this on all devices how did it work before?
>
> -> Yes, we need this on all device.
> In Chromeos kernel v5.4 before, the elan_inhibit()/uninhibit function was
> built and worked
> successfully in elan_i2c_driver.
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads
> /chromeos-5.4/drivers/input/mouse/elan_i2c_core.c#353
We have the Elan touchpad driver without this functionality for many
years. I am aware that certain devices need this, but the fact that
Chrome OS kernel 5.4 (which is only used on a subset of Chromebooks) has
it does not necessarily mean that this functionality is needed on _ALL_
devices.
Thanks.
--
Dmitry
