Am 21.03.22 um 15:51 schrieb Hans de Goede: > Hi, > > On 3/21/22 15:25, Werner Sembach wrote: >> Am 09.03.22 um 18:16 schrieb Hans de Goede: >>> Hi, >>> >>> On 3/8/22 18:05, Werner Sembach wrote: >>>> Merge i8042 quirk tables to reduce code duplication for devices that need >>>> more than one quirk. >>>> >>>> Also align quirkable options with command line parameters and make vendor >>>> wide quirks per device overwriteable on a per device basis. >>>> >>>> Some duplication on the ASUS devices is required to mirror the exact >>>> behaviour of the previous code. >>> Can you explain this a bit more ? >> Yes, see next patch > Next patch as in: > > https://lore.kernel.org/linux-input/20220308170523.783284-3-wse@xxxxxxxxxxxxxxxxxxx/ > > ? Or do you mean the next version of this patch-set ? Next patch as in next revision of this patch set. Sorry for causing confusion. > > >>> >>> >>>> Signed-off-by: Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx> >>>> Cc: stable@xxxxxxxxxxxxxxx >>>> --- >>>> drivers/input/serio/i8042-x86ia64io.h | 1125 ++++++++++++++----------- >>>> 1 file changed, 640 insertions(+), 485 deletions(-) >>>> >>>> diff --git a/drivers/input/serio/i8042-x86ia64io.h b/drivers/input/serio/i8042-x86ia64io.h >>>> index 148a7c5fd0e2..689b9ee3e742 100644 >>>> --- a/drivers/input/serio/i8042-x86ia64io.h >>>> +++ b/drivers/input/serio/i8042-x86ia64io.h >>>> @@ -67,675 +67,821 @@ static inline void i8042_write_command(int val) >>>> >>>> #include <linux/dmi.h> >>>> >>>> -static const struct dmi_system_id __initconst i8042_dmi_noloop_table[] = { >>>> +#define SERIO_QUIRK_NOKBD BIT(0) >>>> +#define SERIO_QUIRK_NOAUX BIT(1) >>>> +#define SERIO_QUIRK_NOMUX BIT(2) >>>> +#define SERIO_QUIRK_FORCEMUX BIT(3) >>>> +#define SERIO_QUIRK_UNLOCK BIT(4) >>>> +#define SERIO_QUIRK_PROBE_DEFER BIT(5) >>>> +#define SERIO_QUIRK_RESET_ALWAYS BIT(6) >>>> +#define SERIO_QUIRK_RESET_NEVER BIT(7) >>>> +#define SERIO_QUIRK_DIECT BIT(8) >>>> +#define SERIO_QUIRK_DUMBKBD BIT(9) >>>> +#define SERIO_QUIRK_NOLOOP BIT(10) >>>> +#define SERIO_QUIRK_NOTIMEOUT BIT(11) >>>> +#define SERIO_QUIRK_KBDRESET BIT(12) >>>> +#define SERIO_QUIRK_DRITEK BIT(13) >>>> +#define SERIO_QUIRK_NOPNP BIT(14) >>>> + >>>> +/* Quirk table for different mainboards. Options similar or identical to i8042 >>>> + * module parameters. >>>> + * ORDERING IS IMPORTANT! The first match will be apllied and the rest ignored. >>>> + * This allows entries to overwrite vendor wide quirks on a per device basis. >>>> + * Where this is irrelevant, entries are sorted case sensitive by DMI_SYS_VENDOR >>>> + * and/or DMI_BOARD_VENDOR to make it easier to avoid dublicate entries. >>>> + */ >>>> +static const struct dmi_system_id i8042_dmi_quirk_table[] __initconst = { >>> <snip> >>> >>>> @@ -1167,11 +1307,6 @@ static int __init i8042_pnp_init(void) >>>> bool pnp_data_busted = false; >>>> int err; >>>> >>>> -#ifdef CONFIG_X86 >>>> - if (dmi_check_system(i8042_dmi_nopnp_table)) >>>> - i8042_nopnp = true; >>>> -#endif >>>> - >>>> if (i8042_nopnp) { >>>> pr_info("PNP detection disabled\n"); >>>> return 0; >>> have you checked that i8042_platform_init() *always* >>> gets called before i8042_pnp_init()? >> As far as i can tell i8042_pnp_init() is only ever called inside i8042_platform_init() so moving this check from pnp >> init to platform init should be no problem. > Ok. > >>> Maybe just add something like this: >>> >>> #ifdef CONFIG_X86 >>> static void __init i8042_check_quirks(void) >>> { >>> const struct dmi_system_id *device_quirk_info; >>> uintptr_t quirks; >>> >>> device_quirk_info = dmi_first_match(i8042_dmi_quirk_table); >>> if (!device_quirk_info) >>> return; >>> >>> quirks = (uintptr_t)device_quirk_info->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; >>> } >>> >>> /* Do more quirk checks */ >>> } >>> #else >>> static inline void i8042_check_quirks(void) {} >>> #endif >>> >>> (above the declaration of i8042_pnp_init()) >>> >>> And call i8042_check_quirks() in both >>> i8042_platform_init() and i8042_platform_init() ? >>> >>> This also abstracts some of the CONFIG_X86 >>> ifdef-ery out of the other functions. >>> >>> >>>> @@ -1277,6 +1412,9 @@ static inline void i8042_pnp_exit(void) { } >>>> >>>> static int __init i8042_platform_init(void) >>>> { >>>> + bool i8042_reset_always_quirk = false; >>>> + bool i8042_reset_never_quirk = false; >>> I'm not a fan of these 2 helper variables, you can do this directly, >>> see above. >>> >>>> + const struct dmi_system_id *device_quirk_info; >>> All 3 these variables will trigger unused variable >>> settings when compiling without CONFIG_X86 set. Note >>> introducing the i8042_check_quirks() helper as I suggest >>> above avoids this without needing more #ifdef-ery. >> Fixed by moving it to its own function as you suggested. >> >> My original reasoning for the helper variables was that i didn't want to move the i8042_reset evaluation, but then did >> it anyways in the next patch after checking that pnp_init doesn't use the variable. > Ok. > > Regards, > > Hans > > > >>>> int retval; >>>> >>>> #ifdef CONFIG_X86 >>>> @@ -1297,6 +1435,44 @@ static int __init i8042_platform_init(void) >>>> i8042_kbd_irq = I8042_MAP_IRQ(1); >>>> i8042_aux_irq = I8042_MAP_IRQ(12); >>>> >>>> +#ifdef CONFIG_X86 >>>> + device_quirk_info = dmi_first_match(i8042_dmi_quirk_table); >>>> + if (device_quirk_info) { >>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_NOKBD) >>>> + i8042_nokbd = true; >>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_NOAUX) >>>> + i8042_noaux = true; >>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_NOMUX) >>>> + i8042_nomux = true; >>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_FORCEMUX) >>>> + i8042_nomux = false; >>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_UNLOCK) >>>> + i8042_unlock = true; >>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_PROBE_DEFER) >>>> + i8042_probe_defer = true; >>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_RESET_ALWAYS) >>>> + i8042_reset_always_quirk = true; >>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_RESET_NEVER) >>>> + i8042_reset_never_quirk = true; >>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_DIECT) >>>> + i8042_direct = true; >>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_DUMBKBD) >>>> + i8042_dumbkbd = true; >>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_NOLOOP) >>>> + i8042_noloop = true; >>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_NOTIMEOUT) >>>> + i8042_notimeout = true; >>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_KBDRESET) >>>> + i8042_kbdreset = true; >>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_DRITEK) >>>> + i8042_dritek = true; >>>> +#ifdef CONFIG_PNP >>>> + if ((uintptr_t)device_quirk_info->driver_data & SERIO_QUIRK_NOPNP) >>>> + i8042_nopnp = true; >>>> +#endif >>>> + } >>>> +#endif >>>> + >>>> retval = i8042_pnp_init(); >>>> if (retval) >>>> return retval; >>>> @@ -1308,34 +1484,13 @@ static int __init i8042_platform_init(void) >>>> #ifdef CONFIG_X86 >>>> /* Honor module parameter when value is not default */ >>>> if (i8042_reset == I8042_RESET_DEFAULT) { >>>> - if (dmi_check_system(i8042_dmi_reset_table)) >>>> + if (i8042_reset_always_quirk) >>>> i8042_reset = I8042_RESET_ALWAYS; >>>> >>>> - if (dmi_check_system(i8042_dmi_noselftest_table)) >>>> + if (i8042_reset_never_quirk) >>>> i8042_reset = I8042_RESET_NEVER; >>>> } >>>> >>>> - if (dmi_check_system(i8042_dmi_noloop_table)) >>>> - i8042_noloop = true; >>>> - >>>> - if (dmi_check_system(i8042_dmi_nomux_table)) >>>> - i8042_nomux = true; >>>> - >>>> - if (dmi_check_system(i8042_dmi_forcemux_table)) >>>> - i8042_nomux = false; >>>> - >>>> - if (dmi_check_system(i8042_dmi_notimeout_table)) >>>> - i8042_notimeout = true; >>>> - >>>> - if (dmi_check_system(i8042_dmi_dritek_table)) >>>> - i8042_dritek = true; >>>> - >>>> - if (dmi_check_system(i8042_dmi_kbdreset_table)) >>>> - i8042_kbdreset = true; >>>> - >>>> - if (dmi_check_system(i8042_dmi_probe_defer_table)) >>>> - i8042_probe_defer = true; >>>> - >>>> /* >>>> * A20 was already enabled during early kernel init. But some buggy >>>> * BIOSes (in MSI Laptops) require A20 to be enabled using 8042 to >>> Regards, >>> >>> Hans >>> >>> >> Regards, >> >> Werner >>