| Message ID | 20210204203933.559752-1-pobrn@protonmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | platform/x86: thinkpad_acpi/ideapad-laptop: move into subfolder and extract common DYTC code | expand |
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@dell.com/ > > 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%) >
Hi 2021. február 12., péntek 9:40 keltezéssel, Hans de Goede írta: > [...] > 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. > How exactly do you imagine that? In separate (e.g. "lenovo-dytc") kernel module? And I assume platform profile registration/etc. should be done by shared code as well? > 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, Barnabás Pőcze
Hi, On 2/12/21 10:21 AM, Barnabás Pőcze wrote: > Hi > > > 2021. február 12., péntek 9:40 keltezéssel, Hans de Goede írta: > >> [...] >> 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. >> > > How exactly do you imagine that? In separate (e.g. "lenovo-dytc") kernel module? That would be an option in that case this module should have a non user selectable Kconfig option and then be select-ed by both the thinkpad_acpi and ideapad-laptop Kconfig bits. Note that the plan is to move to select for ACPI_PLATFORM_PROFILE too, see: https://github.com/linux-surface/kernel/commit/d849e0e069042cbc83636496f66c09e52db4eb01 But I'm fine with just having everything as static inline in a header too, the overhead of having another module is likely going to mean that there will be no disk-space saving and only one of the 2 will ever be loaded in memory. So arguably just having a header with everything static inline is more efficient and it means a simpler/cleaner Kconfig so I have a slight preference for just having a header with all the functions as static inline functions. The goal here is source-code size reduction, compiled-code size reduction is less important. > And I assume platform profile registration/etc. should be done by shared code as well? Yes (if possible). Regards, Hans
Kconfing and Makefile based on https://lore.kernel.org/platform-driver-x86/20210203195832.2950605-1-mario.limonciello@dell.com/ 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 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%)