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:   Mon, 28 Feb 2022 23:54:45 -0800
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     Werner Sembach <wse@tuxedocomputers.com>
Cc:     Hans de Goede <hdegoede@redhat.com>, tiwai@suse.de,
        mpdesouza@suse.com, arnd@arndb.de, samuel@cavoj.net,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] input/i8042: Add TUXEDO/Clevo devices to i8042 quirk
 tables
Message-ID: <Yh3RRT7xgY+PJfrQ@google.com>
References: <20220228114819.32949-1-wse@tuxedocomputers.com>
 <af476269-0722-218d-0fe6-404a9bab736f@redhat.com>
 <a6bfc728-dd47-84fa-1587-3af3049cb0c9@tuxedocomputers.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a6bfc728-dd47-84fa-1587-3af3049cb0c9@tuxedocomputers.com>
Precedence: bulk

Hi,

On Mon, Feb 28, 2022 at 07:50:55PM +0100, Werner Sembach wrote:
> Am 28.02.22 um 14:00 schrieb Hans de Goede:
> > Hi all,
> >
> > On 2/28/22 12:48, Werner Sembach wrote:
> >> A lot of modern Clevo barebones have touchpad and/or keyboard issues after
> >> suspend, fixable with reset + nomux + nopnp + noloop. Luckily, none of them
> >> have an external PS/2 port so this can safely be set for all of them.
> >>
> >> I'm not entirely sure if every device listed really needs all four quirks,
> >> but after testing and production use. No negative effects could be
> >> observed when setting all four.
> >>
> >> The list is quite massive as neither the TUXEDO nor the Clevo dmi strings
> >> have been very consistent historically. I tried to keep the list as short
> >> as possible without risking on missing an affected device.
> >>
> >> This is revision 2 where the Clevo N150CU barebone is removed again, as it
> >> might have problems with the fix and needs further investigations. Also
> >> the SchenkerTechnologiesGmbH System-/Board-Vendor string variations are
> >> added.
> >>
> >> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> >> Cc: stable@vger.kernel.org
> > Looking at the patch I think it would be better to split this into
> > 2 patches":
> >
> > 1. Merge all the existing separate tables into 1 table and use the dmi_system_id.driver_data
> > field to store which combination of the 4 quirks apply to which models.
> >
> > This will already help reducing the tables since some of the models are
> > already listed in 2 or more tables. So you would get something like this:
> >
> > #define SERIO_QUIRK_RESET		BIT(0)
> > #define SERIO_QUIRK_NOMUX		BIT(1)
> > #define SERIO_QUIRK_NOPNP		BIT(2)
> > #define SERIO_QUIRK_NOLOOP		BIT(3)
> > #define SERIO_QUIRK_NOSELFTEST		BIT(4)
> > // etc.
> >
> > static const struct dmi_system_id i8042_dmi_quirk_table[] __initconst = {
> >         {
> >                 /* Entroware Proteus */
> >                 .matches = {
> >                         DMI_MATCH(DMI_SYS_VENDOR, "Entroware"),
> >                         DMI_MATCH(DMI_PRODUCT_NAME, "Proteus"),
> >                         DMI_MATCH(DMI_PRODUCT_VERSION, "EL07R4"),
> >                 },
> > 		.driver_data = (void *)(SERIO_QUIRK_RESET | SERIO_QUIRK_NOMUX)
> >         },
> > 	{}
> > };
> >
> > I picked the Entroware EL07R4 as example here because it needs both the reset and nomux quirks.
> >
> > And then when checking the quirks do:
> >
> > #ifdef CONFIG_X86
> > 	const struct dmi_system_id *dmi_id;
> > 	long quirks = 0;
> >
> > 	dmi_id = dmi_first_match(i8042_dmi_quirk_table);
> > 	if (dmi_id)
> > 		quirks = (long)dmi_id->driver_data;
> >
> > 	if (i8042_reset == I8042_RESET_DEFAULT) {
> > 		if (quirks & SERIO_QUIRK_RESET)
> > 			i8042_reset = I8042_RESET_ALWAYS;
> > 		if (quirks & SERIO_QUIRK_NOSELFTEST)
> > 			i8042_reset = I8042_RESET_NEVER;
> > 	}
> >
> > 	//etc.
> >
> >
> > This way you can reduce all the tables to just 1 table. Please
> > also sort the table alphabetically, first by vendor, then sub-sort
> > by model. This way you can find more entries to merge and it
> > is a good idea to have big tables like this sorted in some way
> > regardless.
> >
> >
> > And then once this big refactoring patch is done (sorry), you
> > can add a second patch on top:
> >
> > 2. Add the models you want to quirk to the new merged tabled
> > and now you only need to add 1 table entry per model, rather
> > then 4, making the patch much smaller.
> >
> >
> > This is a refactoring which IMHO we should likely already
> > have done a while ago, but now with your patch it really is
> > time we do this.
> >
> > I hope the above makes sense, if not don't hesitate to ask
> > questions. Also note this is how *I* would do this, but
> > I'm not the input subsys-maintainer, ultimately this is
> > Dmitry's call and he may actually dislike with I'm proposing!
> Yes, it does make sense. I could follow you and I too think it's a good idea. I will hopefully find time to work on this
> refactoring in the next days.

Yes, I think this is a great idea as we have many instances where
the same entries are present in several tables.

> >
> > I don't expect that Dmitry will dislike this, but you never know.
> >
> > Also unfortunately Dmitry lately has only a limited amount of
> > time to spend on input subsys maintenance so in my experience
> > it may be a while before you get a reply from Dmitry.
> 
> Ok, thanks for the info. As I wrote in the other mail, I was worried (or paranoid xD) that I got flagged as spam or
> something.

It did indeed, I am not sure why. This does not invalidate what Hans
said - lately I was not able to spend as much time on input as I wanted.

Regarding this patch - it looks like board names are pretty unique in
many cases, so I wonder if we could not save some memory by omitting the
vendor info (especially because some, like "Notebook", are very generic
anyways) and go simply by the board.

Thanks.

-- 
Dmitry