Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 30 Jul 2023 22:47:40 -0700
From:   'Dmitry Torokhov' <dmitry.torokhov@gmail.com>
To:     "Jingle.Wu" <jingle.wu@emc.com.tw>
Cc:     linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
        phoenix@emc.com.tw, josh.chen@emc.com.tw, dave.wang@emc.com.tw
Subject: Re: [PATCH]  Input: elan_i2c - Implement inhibit/uninhibit functions.
Message-ID: <ZMdK/Gnhb3p9NVCC@google.com>
References: <20230531090340.1035499-1-jingle.wu@emc.com.tw>
 <ZJ4MVbTnBT5sM5XR@google.com>
 <000001d9af07$3c003a20$b400ae60$@emc.com.tw>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <000001d9af07$3c003a20$b400ae60$@emc.com.tw>
Precedence: bulk

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