Re: [PATCH] Input: elan_i2c - Implement inhibit/uninhibit functions.

['Dmitry Torokhov' <dmitry.torokhov@gmail.com>]

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