Hi Barnabás, On 2/4/21 9:39 PM, Barnabás Pőcze wrote: > Kconfing and Makefile based on > https://lore.kernel.org/platform-driver-x86/20210203195832.2950605-1-mario.limonciello@xxxxxxxx/ > > Barnabás Pőcze (2): > platform/x86: move thinkpad_acpi and ideapad-laptop into lenovo > subfolder > platform/x86: thinkpad_acpi/ideapad-laptop: extract common DYTC > constants/functions Thank you for this patch series. I like the series but I would like to see the 3th patch to go even further wrt removing duplication between thinkpad_acpi and ideapad-laptop. The big difference between the 2 drivers is thinkpad_acpi relying on global variables while ideapad-laptop stores everything in a driver data-struct. What you can do is add a struct which holds all the necessary data for the dytc code, struct ideapad_dytc_priv is a start for this I guess. Lets say we rename struct ideapad_dytc_priv to struct dytc_priv, then thinkpad_acpi.c can have a: static struct dytc_priv *dytc_priv; And there can be a shared dytc probe function like this: static int dytc_profile_init(struct dytc_priv **dytc_priv_ret) { struct dytc_priv *dytc_priv; ... *dytc_priv_ret = dytc_priv; return NULL; error: // clean stuff *dytc_priv_ret = NULL; return err; } Which thinkpad_acpi can then call on its global dytc_priv pointer: err = dytc_profile_init(&dytc_priv); Where as ideapad_laptop would use the pointer inside its data struct: err = dytc_profile_init(&priv->dytc); I think this way we should be able to share almost all of the dytc code not just the 2 convert functions. One difference between the 2 which I'm aware of is that ideapad_laptop caches the handle, where as thinkpad_acpi looks it up everytime. Caching obviously is better, so the shared code should just cache it (add a copy of the handle pointer to the dytc_priv struct). I guess you may hit some other issues but those should all be fixable. So over all I think sharing most of the dytc code should be doable and we really should move in that direction. Note it would be best to do the further duplication removal in a third patch, or even in multiple further patches to make reviewing this easier. Regards, Hans > > MAINTAINERS | 4 +- > drivers/platform/x86/Kconfig | 156 +-------------- > drivers/platform/x86/Makefile | 7 +- > drivers/platform/x86/lenovo/Kconfig | 177 ++++++++++++++++++ > drivers/platform/x86/lenovo/Makefile | 8 + > drivers/platform/x86/lenovo/dytc.h | 79 ++++++++ > .../x86/{ => lenovo}/ideapad-laptop.c | 72 +------ > .../platform/x86/{ => lenovo}/thinkpad_acpi.c | 73 +------- > 8 files changed, 274 insertions(+), 302 deletions(-) > create mode 100644 drivers/platform/x86/lenovo/Kconfig > create mode 100644 drivers/platform/x86/lenovo/Makefile > create mode 100644 drivers/platform/x86/lenovo/dytc.h > rename drivers/platform/x86/{ => lenovo}/ideapad-laptop.c (94%) > rename drivers/platform/x86/{ => lenovo}/thinkpad_acpi.c (99%) >
