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 ? >> >> >> >>> 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 >
