[RFC PATCH 0/2] platform/x86: thinkpad_acpi/ideapad-laptop: move into subfolder and extract common DYTC code

[RFC PATCH 0/2] platform/x86: thinkpad_acpi/ideapad-laptop: move into subfolder and extract common DYTC code

[Barnabás Pőcze]

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%)

-- 
2.30.0

[RFC PATCH 1/2] platform/x86: move thinkpad_acpi and ideapad-laptop into lenovo subfolder

[Barnabás Pőcze]

For better manageability and as preparation for extracting the common
DYTC code from both drivers, move them into a dedicated folder.

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>

 create mode 100644 drivers/platform/x86/lenovo/Kconfig
 create mode 100644 drivers/platform/x86/lenovo/Makefile
 rename drivers/platform/x86/{ => lenovo}/ideapad-laptop.c (100%)
 rename drivers/platform/x86/{ => lenovo}/thinkpad_acpi.c (100%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 34bfa5c1aec5..ddbec78fbac4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8584,7 +8584,7 @@ M:	Ike Panhc <ike.pan@canonical.com>
 L:	platform-driver-x86@vger.kernel.org
 S:	Maintained
 W:	http://launchpad.net/ideapad-laptop
-F:	drivers/platform/x86/ideapad-laptop.c
+F:	drivers/platform/x86/lenovo/ideapad-laptop.c
 
 IDEAPAD LAPTOP SLIDEBAR DRIVER
 M:	Andrey Moiseev <o2g.org.ru@gmail.com>
@@ -17648,7 +17648,7 @@ S:	Maintained
 W:	http://ibm-acpi.sourceforge.net
 W:	http://thinkwiki.org/wiki/Ibm-acpi
 T:	git git://repo.or.cz/linux-2.6/linux-acpi-2.6/ibm-acpi-2.6.git
-F:	drivers/platform/x86/thinkpad_acpi.c
+F:	drivers/platform/x86/lenovo/thinkpad_acpi.c
 
 THUNDERBOLT DMA TRAFFIC TEST DRIVER
 M:	Isaac Hazan <isaac.hazan@intel.com>
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 56353e8c792a..200e9961348a 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -442,22 +442,6 @@ config IBM_RTL
 	 state = 0 (BIOS SMIs on)
 	 state = 1 (BIOS SMIs off)
 
-config IDEAPAD_LAPTOP
-	tristate "Lenovo IdeaPad Laptop Extras"
-	depends on ACPI
-	depends on RFKILL && INPUT
-	depends on SERIO_I8042
-	depends on BACKLIGHT_CLASS_DEVICE
-	depends on ACPI_VIDEO || ACPI_VIDEO = n
-	depends on ACPI_WMI || ACPI_WMI = n
-	depends on ACPI_PLATFORM_PROFILE
-	select INPUT_SPARSEKMAP
-	select NEW_LEDS
-	select LEDS_CLASS
-	help
-	  This is a driver for Lenovo IdeaPad netbooks contains drivers for
-	  rfkill switch, hotkey, fan control and backlight control.
-
 config SENSORS_HDAPS
 	tristate "Thinkpad Hard Drive Active Protection System (hdaps)"
 	depends on INPUT
@@ -476,145 +460,7 @@ config SENSORS_HDAPS
 	  Say Y here if you have an applicable laptop and want to experience
 	  the awesome power of hdaps.
 
-config THINKPAD_ACPI
-	tristate "ThinkPad ACPI Laptop Extras"
-	depends on ACPI
-	depends on ACPI_BATTERY
-	depends on INPUT
-	depends on RFKILL || RFKILL = n
-	depends on ACPI_VIDEO || ACPI_VIDEO = n
-	depends on BACKLIGHT_CLASS_DEVICE
-	depends on ACPI_PLATFORM_PROFILE
-	select HWMON
-	select NVRAM
-	select NEW_LEDS
-	select LEDS_CLASS
-	select LEDS_TRIGGERS
-	select LEDS_TRIGGER_AUDIO
-	help
-	  This is a driver for the IBM and Lenovo ThinkPad laptops. It adds
-	  support for Fn-Fx key combinations, Bluetooth control, video
-	  output switching, ThinkLight control, UltraBay eject and more.
-	  For more information about this driver see
-	  <file:Documentation/admin-guide/laptops/thinkpad-acpi.rst> and
-	  <http://ibm-acpi.sf.net/> .
-
-	  This driver was formerly known as ibm-acpi.
-
-	  Extra functionality will be available if the rfkill (CONFIG_RFKILL)
-	  and/or ALSA (CONFIG_SND) subsystems are available in the kernel.
-	  Note that if you want ThinkPad-ACPI to be built-in instead of
-	  modular, ALSA and rfkill will also have to be built-in.
-
-	  If you have an IBM or Lenovo ThinkPad laptop, say Y or M here.
-
-config THINKPAD_ACPI_ALSA_SUPPORT
-	bool "Console audio control ALSA interface"
-	depends on THINKPAD_ACPI
-	depends on SND
-	depends on SND = y || THINKPAD_ACPI = SND
-	default y
-	help
-	  Enables monitoring of the built-in console audio output control
-	  (headphone and speakers), which is operated by the mute and (in
-	  some ThinkPad models) volume hotkeys.
-
-	  If this option is enabled, ThinkPad-ACPI will export an ALSA card
-	  with a single read-only mixer control, which should be used for
-	  on-screen-display feedback purposes by the Desktop Environment.
-
-	  Optionally, the driver will also allow software control (the
-	  ALSA mixer will be made read-write).  Please refer to the driver
-	  documentation for details.
-
-	  All IBM models have both volume and mute control.  Newer Lenovo
-	  models only have mute control (the volume hotkeys are just normal
-	  keys and volume control is done through the main HDA mixer).
-
-config THINKPAD_ACPI_DEBUGFACILITIES
-	bool "Maintainer debug facilities"
-	depends on THINKPAD_ACPI
-	help
-	  Enables extra stuff in the thinkpad-acpi which is completely useless
-	  for normal use.  Read the driver source to find out what it does.
-
-	  Say N here, unless you were told by a kernel maintainer to do
-	  otherwise.
-
-config THINKPAD_ACPI_DEBUG
-	bool "Verbose debug mode"
-	depends on THINKPAD_ACPI
-	help
-	  Enables extra debugging information, at the expense of a slightly
-	  increase in driver size.
-
-	  If you are not sure, say N here.
-
-config THINKPAD_ACPI_UNSAFE_LEDS
-	bool "Allow control of important LEDs (unsafe)"
-	depends on THINKPAD_ACPI
-	help
-	  Overriding LED state on ThinkPads can mask important
-	  firmware alerts (like critical battery condition), or misled
-	  the user into damaging the hardware (undocking or ejecting
-	  the bay while buses are still active), etc.
-
-	  LED control on the ThinkPad is write-only (with very few
-	  exceptions on very ancient models), which makes it
-	  impossible to know beforehand if important information will
-	  be lost when one changes LED state.
-
-	  Users that know what they are doing can enable this option
-	  and the driver will allow control of every LED, including
-	  the ones on the dock stations.
-
-	  Never enable this option on a distribution kernel.
-
-	  Say N here, unless you are building a kernel for your own
-	  use, and need to control the important firmware LEDs.
-
-config THINKPAD_ACPI_VIDEO
-	bool "Video output control support"
-	depends on THINKPAD_ACPI
-	default y
-	help
-	  Allows the thinkpad_acpi driver to provide an interface to control
-	  the various video output ports.
-
-	  This feature often won't work well, depending on ThinkPad model,
-	  display state, video output devices in use, whether there is a X
-	  server running, phase of the moon, and the current mood of
-	  Schroedinger's cat.  If you can use X.org's RandR to control
-	  your ThinkPad's video output ports instead of this feature,
-	  don't think twice: do it and say N here to save memory and avoid
-	  bad interactions with X.org.
-
-	  NOTE: access to this feature is limited to processes with the
-	  CAP_SYS_ADMIN capability, to avoid local DoS issues in platforms
-	  where it interacts badly with X.org.
-
-	  If you are not sure, say Y here but do try to check if you could
-	  be using X.org RandR instead.
-
-config THINKPAD_ACPI_HOTKEY_POLL
-	bool "Support NVRAM polling for hot keys"
-	depends on THINKPAD_ACPI
-	default y
-	help
-	  Some thinkpad models benefit from NVRAM polling to detect a few of
-	  the hot key press events.  If you know your ThinkPad model does not
-	  need to do NVRAM polling to support any of the hot keys you use,
-	  unselecting this option will save about 1kB of memory.
-
-	  ThinkPads T40 and newer, R52 and newer, and X31 and newer are
-	  unlikely to need NVRAM polling in their latest BIOS versions.
-
-	  NVRAM polling can detect at most the following keys: ThinkPad/Access
-	  IBM, Zoom, Switch Display (fn+F7), ThinkLight, Volume up/down/mute,
-	  Brightness up/down, Display Expand (fn+F8), Hibernate (fn+F12).
-
-	  If you are not sure, say Y here.  The driver enables polling only if
-	  it is strictly necessary to do so.
+source "drivers/platform/x86/lenovo/Kconfig"
 
 config INTEL_ATOMISP2_LED
 	tristate "Intel AtomISP2 camera LED driver"
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 60d554073749..2e6b30f0113c 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -56,10 +56,9 @@ obj-$(CONFIG_TC1100_WMI)	+= tc1100-wmi.o
 obj-$(CONFIG_UV_SYSFS)       += uv_sysfs.o
 
 # IBM Thinkpad and Lenovo
-obj-$(CONFIG_IBM_RTL)		+= ibm_rtl.o
-obj-$(CONFIG_IDEAPAD_LAPTOP)	+= ideapad-laptop.o
-obj-$(CONFIG_SENSORS_HDAPS)	+= hdaps.o
-obj-$(CONFIG_THINKPAD_ACPI)	+= thinkpad_acpi.o
+obj-$(CONFIG_IBM_RTL)			  += ibm_rtl.o
+obj-$(CONFIG_SENSORS_HDAPS)		  += hdaps.o
+obj-$(CONFIG_X86_PLATFORM_DRIVERS_LENOVO) += lenovo/
 
 # Intel
 obj-$(CONFIG_INTEL_ATOMISP2_LED)	+= intel_atomisp2_led.o
diff --git a/drivers/platform/x86/lenovo/Kconfig b/drivers/platform/x86/lenovo/Kconfig
new file mode 100644
index 000000000000..47d35cdddf4d
--- /dev/null
+++ b/drivers/platform/x86/lenovo/Kconfig
@@ -0,0 +1,177 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Lenovo x86 platform drivers
+#
+
+menuconfig X86_PLATFORM_DRIVERS_LENOVO
+	bool "Lenovo x86 platform drivers"
+	default n
+	depends on X86_PLATFORM_DEVICES
+	help
+	  Say Y here to get to see options for device drivers for various
+	  Lenovo x86 platforms, including vendor-specific laptop extension drivers.
+	  This option alone does not add any kernel code.
+
+	  If you say N, all options in this submenu will be skipped and disabled.
+
+if X86_PLATFORM_DRIVERS_LENOVO
+
+config IDEAPAD_LAPTOP
+	tristate "Lenovo IdeaPad Laptop Extras"
+	default m
+	depends on ACPI
+	depends on RFKILL && INPUT
+	depends on SERIO_I8042
+	depends on BACKLIGHT_CLASS_DEVICE
+	depends on ACPI_VIDEO || ACPI_VIDEO = n
+	depends on ACPI_WMI || ACPI_WMI = n
+	depends on ACPI_PLATFORM_PROFILE
+	select INPUT_SPARSEKMAP
+	select NEW_LEDS
+	select LEDS_CLASS
+	help
+	  This is a driver for Lenovo IdeaPad netbooks contains drivers for
+	  rfkill switch, hotkey, fan control and backlight control.
+
+config THINKPAD_ACPI
+	tristate "ThinkPad ACPI Laptop Extras"
+	default m
+	depends on ACPI
+	depends on ACPI_BATTERY
+	depends on INPUT
+	depends on RFKILL || RFKILL = n
+	depends on ACPI_VIDEO || ACPI_VIDEO = n
+	depends on BACKLIGHT_CLASS_DEVICE
+	depends on ACPI_PLATFORM_PROFILE
+	select HWMON
+	select NVRAM
+	select NEW_LEDS
+	select LEDS_CLASS
+	select LEDS_TRIGGERS
+	select LEDS_TRIGGER_AUDIO
+	help
+	  This is a driver for the IBM and Lenovo ThinkPad laptops. It adds
+	  support for Fn-Fx key combinations, Bluetooth control, video
+	  output switching, ThinkLight control, UltraBay eject and more.
+	  For more information about this driver see
+	  <file:Documentation/admin-guide/laptops/thinkpad-acpi.rst> and
+	  <http://ibm-acpi.sf.net/> .
+
+	  This driver was formerly known as ibm-acpi.
+
+	  Extra functionality will be available if the rfkill (CONFIG_RFKILL)
+	  and/or ALSA (CONFIG_SND) subsystems are available in the kernel.
+	  Note that if you want ThinkPad-ACPI to be built-in instead of
+	  modular, ALSA and rfkill will also have to be built-in.
+
+	  If you have an IBM or Lenovo ThinkPad laptop, say Y or M here.
+
+config THINKPAD_ACPI_ALSA_SUPPORT
+	bool "Console audio control ALSA interface"
+	depends on THINKPAD_ACPI
+	depends on SND
+	depends on SND = y || THINKPAD_ACPI = SND
+	default y
+	help
+	  Enables monitoring of the built-in console audio output control
+	  (headphone and speakers), which is operated by the mute and (in
+	  some ThinkPad models) volume hotkeys.
+
+	  If this option is enabled, ThinkPad-ACPI will export an ALSA card
+	  with a single read-only mixer control, which should be used for
+	  on-screen-display feedback purposes by the Desktop Environment.
+
+	  Optionally, the driver will also allow software control (the
+	  ALSA mixer will be made read-write).  Please refer to the driver
+	  documentation for details.
+
+	  All IBM models have both volume and mute control.  Newer Lenovo
+	  models only have mute control (the volume hotkeys are just normal
+	  keys and volume control is done through the main HDA mixer).
+
+config THINKPAD_ACPI_DEBUGFACILITIES
+	bool "Maintainer debug facilities"
+	depends on THINKPAD_ACPI
+	help
+	  Enables extra stuff in the thinkpad-acpi which is completely useless
+	  for normal use.  Read the driver source to find out what it does.
+
+	  Say N here, unless you were told by a kernel maintainer to do
+	  otherwise.
+
+config THINKPAD_ACPI_DEBUG
+	bool "Verbose debug mode"
+	depends on THINKPAD_ACPI
+	help
+	  Enables extra debugging information, at the expense of a slightly
+	  increase in driver size.
+
+	  If you are not sure, say N here.
+
+config THINKPAD_ACPI_UNSAFE_LEDS
+	bool "Allow control of important LEDs (unsafe)"
+	depends on THINKPAD_ACPI
+	help
+	  Overriding LED state on ThinkPads can mask important
+	  firmware alerts (like critical battery condition), or misled
+	  the user into damaging the hardware (undocking or ejecting
+	  the bay while buses are still active), etc.
+
+	  LED control on the ThinkPad is write-only (with very few
+	  exceptions on very ancient models), which makes it
+	  impossible to know beforehand if important information will
+	  be lost when one changes LED state.
+
+	  Users that know what they are doing can enable this option
+	  and the driver will allow control of every LED, including
+	  the ones on the dock stations.
+
+	  Never enable this option on a distribution kernel.
+
+	  Say N here, unless you are building a kernel for your own
+	  use, and need to control the important firmware LEDs.
+
+config THINKPAD_ACPI_VIDEO
+	bool "Video output control support"
+	depends on THINKPAD_ACPI
+	default y
+	help
+	  Allows the thinkpad_acpi driver to provide an interface to control
+	  the various video output ports.
+
+	  This feature often won't work well, depending on ThinkPad model,
+	  display state, video output devices in use, whether there is a X
+	  server running, phase of the moon, and the current mood of
+	  Schroedinger's cat.  If you can use X.org's RandR to control
+	  your ThinkPad's video output ports instead of this feature,
+	  don't think twice: do it and say N here to save memory and avoid
+	  bad interactions with X.org.
+
+	  NOTE: access to this feature is limited to processes with the
+	  CAP_SYS_ADMIN capability, to avoid local DoS issues in platforms
+	  where it interacts badly with X.org.
+
+	  If you are not sure, say Y here but do try to check if you could
+	  be using X.org RandR instead.
+
+config THINKPAD_ACPI_HOTKEY_POLL
+	bool "Support NVRAM polling for hot keys"
+	depends on THINKPAD_ACPI
+	default y
+	help
+	  Some thinkpad models benefit from NVRAM polling to detect a few of
+	  the hot key press events.  If you know your ThinkPad model does not
+	  need to do NVRAM polling to support any of the hot keys you use,
+	  unselecting this option will save about 1kB of memory.
+
+	  ThinkPads T40 and newer, R52 and newer, and X31 and newer are
+	  unlikely to need NVRAM polling in their latest BIOS versions.
+
+	  NVRAM polling can detect at most the following keys: ThinkPad/Access
+	  IBM, Zoom, Switch Display (fn+F7), ThinkLight, Volume up/down/mute,
+	  Brightness up/down, Display Expand (fn+F8), Hibernate (fn+F12).
+
+	  If you are not sure, say Y here.  The driver enables polling only if
+	  it is strictly necessary to do so.
+
+endif # X86_PLATFORM_DRIVERS_LENOVO
diff --git a/drivers/platform/x86/lenovo/Makefile b/drivers/platform/x86/lenovo/Makefile
new file mode 100644
index 000000000000..c84d0ae5db3d
--- /dev/null
+++ b/drivers/platform/x86/lenovo/Makefile
@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for linux/drivers/platform/x86/lenovo
+# Lenovo x86 platform drivers
+#
+
+obj-$(CONFIG_IDEAPAD_LAPTOP) += ideapad-laptop.o
+obj-$(CONFIG_THINKPAD_ACPI)  += thinkpad_acpi.o
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
similarity index 100%
rename from drivers/platform/x86/ideapad-laptop.c
rename to drivers/platform/x86/lenovo/ideapad-laptop.c
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
similarity index 100%
rename from drivers/platform/x86/thinkpad_acpi.c
rename to drivers/platform/x86/lenovo/thinkpad_acpi.c
-- 
2.30.0

[RFC PATCH 2/2] platform/x86: thinkpad_acpi/ideapad-laptop: extract common DYTC constants/functions

[Barnabás Pőcze]

Extract common DYTC constants/functions into a dedicated header file
to avoid code duplication. Rename DYTC_MODE_BALANCE to DYTC_MODE_BALANCED
and DYTC_MODE_PERFORM to DYTC_MODE_PERFORMANCE to be consistent
with the platform_profile_option enum.

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>

 create mode 100644 drivers/platform/x86/lenovo/dytc.h

diff --git a/drivers/platform/x86/lenovo/dytc.h b/drivers/platform/x86/lenovo/dytc.h
new file mode 100644
index 000000000000..9ec341bdef70
--- /dev/null
+++ b/drivers/platform/x86/lenovo/dytc.h
@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+#ifndef _LENOVO_DYTC_H_
+#define _LENOVO_DYTC_H_
+
+#include <linux/platform_profile.h>
+
+#define DYTC_CMD_QUERY         0 /* To get DYTC status - enable/revision */
+#define DYTC_CMD_SET           1 /* To enable/disable IC function mode */
+#define DYTC_CMD_GET           2 /* To get current IC function and mode */
+#define DYTC_CMD_RESET       511 /* To reset back to default */
+
+#define DYTC_QUERY_ENABLE_BIT  8 /* Bit        8 - 0 = disabled, 1 = enabled */
+#define DYTC_QUERY_SUBREV_BIT 16 /* Bits 16 - 27 - sub revision */
+#define DYTC_QUERY_REV_BIT    28 /* Bits 28 - 31 - revision */
+
+#define DYTC_GET_FUNCTION_BIT  8 /* Bits  8 - 11 - function setting */
+#define DYTC_GET_MODE_BIT     12 /* Bits 12 - 15 - mode setting */
+#define DYTC_GET_LAPMODE_BIT  17 /* Bit       17 - set when in lapmode */
+
+#define DYTC_SET_FUNCTION_BIT 12 /* Bits 12 - 15 - function setting */
+#define DYTC_SET_MODE_BIT     16 /* Bits 16 - 19 - mode setting */
+#define DYTC_SET_VALID_BIT    20 /* Bit       20 - 1 = on, 0 = off */
+
+#define DYTC_FUNCTION_STD      0 /* Function =  0, standard mode */
+#define DYTC_FUNCTION_CQL      1 /* Function =  1, lap mode */
+#define DYTC_FUNCTION_MMC     11 /* Function = 11, desk mode */
+
+#define DYTC_MODE_PERFORMANCE  2 /* High power mode aka performance */
+#define DYTC_MODE_LOW_POWER    3 /* Low power mode aka quiet */
+#define DYTC_MODE_BALANCED    15 /* Default mode aka balanced */
+
+#define DYTC_SET_COMMAND(function, mode, on) \
+	(DYTC_CMD_SET | (function) << DYTC_SET_FUNCTION_BIT | \
+	 (mode) << DYTC_SET_MODE_BIT | \
+	 (on) << DYTC_SET_VALID_BIT)
+
+#define DYTC_DISABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCED, 0)
+
+#define DYTC_ENABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCED, 1)
+
+static int inline convert_dytc_to_profile(int dytcmode, enum platform_profile_option *profile)
+{
+	switch (dytcmode) {
+	case DYTC_MODE_LOW_POWER:
+		*profile = PLATFORM_PROFILE_LOW_POWER;
+		break;
+	case DYTC_MODE_BALANCED:
+		*profile =  PLATFORM_PROFILE_BALANCED;
+		break;
+	case DYTC_MODE_PERFORMANCE:
+		*profile =  PLATFORM_PROFILE_PERFORMANCE;
+		break;
+	default: /* Unknown mode */
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static inline int convert_profile_to_dytc(enum platform_profile_option profile, int *dytcmode)
+{
+	switch (profile) {
+	case PLATFORM_PROFILE_LOW_POWER:
+		*dytcmode = DYTC_MODE_LOW_POWER;
+		break;
+	case PLATFORM_PROFILE_BALANCED:
+		*dytcmode = DYTC_MODE_BALANCED;
+		break;
+	case PLATFORM_PROFILE_PERFORMANCE:
+		*dytcmode = DYTC_MODE_PERFORMANCE;
+		break;
+	default: /* Unknown profile */
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+#endif /* _LENOVO_DYTC_H_ */
diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
index 6cb5ad4be231..387c8c86b8bb 100644
--- a/drivers/platform/x86/lenovo/ideapad-laptop.c
+++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
@@ -35,6 +35,8 @@
 
 #include <dt-bindings/leds/common.h>
 
+#include "dytc.h"
+
 #define IDEAPAD_RFKILL_DEV_NUM	3
 
 #if IS_ENABLED(CONFIG_ACPI_WMI)
@@ -676,76 +678,6 @@ static const struct attribute_group ideapad_attribute_group = {
 /*
  * DYTC Platform profile
  */
-#define DYTC_CMD_QUERY        0 /* To get DYTC status - enable/revision */
-#define DYTC_CMD_SET          1 /* To enable/disable IC function mode */
-#define DYTC_CMD_GET          2 /* To get current IC function and mode */
-#define DYTC_CMD_RESET    0x1ff /* To reset back to default */
-
-#define DYTC_QUERY_ENABLE_BIT 8  /* Bit        8 - 0 = disabled, 1 = enabled */
-#define DYTC_QUERY_SUBREV_BIT 16 /* Bits 16 - 27 - sub revision */
-#define DYTC_QUERY_REV_BIT    28 /* Bits 28 - 31 - revision */
-
-#define DYTC_GET_FUNCTION_BIT 8  /* Bits  8-11 - function setting */
-#define DYTC_GET_MODE_BIT     12 /* Bits 12-15 - mode setting */
-
-#define DYTC_SET_FUNCTION_BIT 12 /* Bits 12-15 - function setting */
-#define DYTC_SET_MODE_BIT     16 /* Bits 16-19 - mode setting */
-#define DYTC_SET_VALID_BIT    20 /* Bit     20 - 1 = on, 0 = off */
-
-#define DYTC_FUNCTION_STD     0  /* Function = 0, standard mode */
-#define DYTC_FUNCTION_CQL     1  /* Function = 1, lap mode */
-#define DYTC_FUNCTION_MMC     11 /* Function = 11, desk mode */
-
-#define DYTC_MODE_PERFORM     2  /* High power mode aka performance */
-#define DYTC_MODE_LOW_POWER       3  /* Low power mode aka quiet */
-#define DYTC_MODE_BALANCE   0xF  /* Default mode aka balanced */
-
-#define DYTC_SET_COMMAND(function, mode, on) \
-	(DYTC_CMD_SET | (function) << DYTC_SET_FUNCTION_BIT | \
-	 (mode) << DYTC_SET_MODE_BIT | \
-	 (on) << DYTC_SET_VALID_BIT)
-
-#define DYTC_DISABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCE, 0)
-
-#define DYTC_ENABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCE, 1)
-
-static int convert_dytc_to_profile(int dytcmode, enum platform_profile_option *profile)
-{
-	switch (dytcmode) {
-	case DYTC_MODE_LOW_POWER:
-		*profile = PLATFORM_PROFILE_LOW_POWER;
-		break;
-	case DYTC_MODE_BALANCE:
-		*profile =  PLATFORM_PROFILE_BALANCED;
-		break;
-	case DYTC_MODE_PERFORM:
-		*profile =  PLATFORM_PROFILE_PERFORMANCE;
-		break;
-	default: /* Unknown mode */
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-static int convert_profile_to_dytc(enum platform_profile_option profile, int *perfmode)
-{
-	switch (profile) {
-	case PLATFORM_PROFILE_LOW_POWER:
-		*perfmode = DYTC_MODE_LOW_POWER;
-		break;
-	case PLATFORM_PROFILE_BALANCED:
-		*perfmode = DYTC_MODE_BALANCE;
-		break;
-	case PLATFORM_PROFILE_PERFORMANCE:
-		*perfmode = DYTC_MODE_PERFORM;
-		break;
-	default: /* Unknown profile */
-		return -EOPNOTSUPP;
-	}
-
-	return 0;
-}
 
 /*
  * dytc_profile_get: Function to register with platform_profile
diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
index b881044b31b0..61990fc9870c 100644
--- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
+++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
@@ -74,6 +74,8 @@
 #include <acpi/battery.h>
 #include <acpi/video.h>
 
+#include "dytc.h"
+
 /* ThinkPad CMOS commands */
 #define TP_CMOS_VOLUME_DOWN	0
 #define TP_CMOS_VOLUME_UP	1
@@ -9845,9 +9847,6 @@ static struct ibm_struct lcdshadow_driver_data = {
  * Thinkpad sensor interfaces
  */
 
-#define DYTC_CMD_GET          2 /* To get current IC function and mode */
-#define DYTC_GET_LAPMODE_BIT 17 /* Set when in lapmode */
-
 #define PALMSENSOR_PRESENT_BIT 0 /* Determine if psensor present */
 #define PALMSENSOR_ON_BIT      1 /* psensor status */
 
@@ -9999,79 +9998,11 @@ static struct ibm_struct proxsensor_driver_data = {
  * DYTC Platform Profile interface
  */
 
-#define DYTC_CMD_QUERY        0 /* To get DYTC status - enable/revision */
-#define DYTC_CMD_SET          1 /* To enable/disable IC function mode */
-#define DYTC_CMD_RESET    0x1ff /* To reset back to default */
-
-#define DYTC_QUERY_ENABLE_BIT 8  /* Bit        8 - 0 = disabled, 1 = enabled */
-#define DYTC_QUERY_SUBREV_BIT 16 /* Bits 16 - 27 - sub revision */
-#define DYTC_QUERY_REV_BIT    28 /* Bits 28 - 31 - revision */
-
-#define DYTC_GET_FUNCTION_BIT 8  /* Bits  8-11 - function setting */
-#define DYTC_GET_MODE_BIT     12 /* Bits 12-15 - mode setting */
-
-#define DYTC_SET_FUNCTION_BIT 12 /* Bits 12-15 - function setting */
-#define DYTC_SET_MODE_BIT     16 /* Bits 16-19 - mode setting */
-#define DYTC_SET_VALID_BIT    20 /* Bit     20 - 1 = on, 0 = off */
-
-#define DYTC_FUNCTION_STD     0  /* Function = 0, standard mode */
-#define DYTC_FUNCTION_CQL     1  /* Function = 1, lap mode */
-#define DYTC_FUNCTION_MMC     11 /* Function = 11, desk mode */
-
-#define DYTC_MODE_PERFORM     2  /* High power mode aka performance */
-#define DYTC_MODE_LOWPOWER    3  /* Low power mode */
-#define DYTC_MODE_BALANCE   0xF  /* Default mode aka balanced */
-
-#define DYTC_SET_COMMAND(function, mode, on) \
-	(DYTC_CMD_SET | (function) << DYTC_SET_FUNCTION_BIT | \
-	 (mode) << DYTC_SET_MODE_BIT | \
-	 (on) << DYTC_SET_VALID_BIT)
-
-#define DYTC_DISABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCE, 0)
-
-#define DYTC_ENABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCE, 1)
-
 static bool dytc_profile_available;
 static enum platform_profile_option dytc_current_profile;
 static atomic_t dytc_ignore_event = ATOMIC_INIT(0);
 static DEFINE_MUTEX(dytc_mutex);
 
-static int convert_dytc_to_profile(int dytcmode, enum platform_profile_option *profile)
-{
-	switch (dytcmode) {
-	case DYTC_MODE_LOWPOWER:
-		*profile = PLATFORM_PROFILE_LOW_POWER;
-		break;
-	case DYTC_MODE_BALANCE:
-		*profile =  PLATFORM_PROFILE_BALANCED;
-		break;
-	case DYTC_MODE_PERFORM:
-		*profile =  PLATFORM_PROFILE_PERFORMANCE;
-		break;
-	default: /* Unknown mode */
-		return -EINVAL;
-	}
-	return 0;
-}
-
-static int convert_profile_to_dytc(enum platform_profile_option profile, int *perfmode)
-{
-	switch (profile) {
-	case PLATFORM_PROFILE_LOW_POWER:
-		*perfmode = DYTC_MODE_LOWPOWER;
-		break;
-	case PLATFORM_PROFILE_BALANCED:
-		*perfmode = DYTC_MODE_BALANCE;
-		break;
-	case PLATFORM_PROFILE_PERFORMANCE:
-		*perfmode = DYTC_MODE_PERFORM;
-		break;
-	default: /* Unknown profile */
-		return -EOPNOTSUPP;
-	}
-	return 0;
-}
-
 /*
  * dytc_profile_get: Function to register with platform_profile
  * handler. Returns current platform profile.
-- 
2.30.0

Re: [External] [RFC PATCH 2/2] platform/x86: thinkpad_acpi/ideapad-laptop: extract common DYTC constants/functions

[Mark Pearson]

Thanks for doing this Barnabas - feel like I should have contributed to
this exercise :)

Looks excellent

Mark

On 04/02/2021 15:40, Barnabás Pőcze wrote:
> Extract common DYTC constants/functions into a dedicated header file
> to avoid code duplication. Rename DYTC_MODE_BALANCE to DYTC_MODE_BALANCED
> and DYTC_MODE_PERFORM to DYTC_MODE_PERFORMANCE to be consistent
> with the platform_profile_option enum.
> 
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> 
>  create mode 100644 drivers/platform/x86/lenovo/dytc.h
> 
> diff --git a/drivers/platform/x86/lenovo/dytc.h b/drivers/platform/x86/lenovo/dytc.h
> new file mode 100644
> index 000000000000..9ec341bdef70
> --- /dev/null
> +++ b/drivers/platform/x86/lenovo/dytc.h
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +#ifndef _LENOVO_DYTC_H_
> +#define _LENOVO_DYTC_H_
> +
> +#include <linux/platform_profile.h>
> +
> +#define DYTC_CMD_QUERY         0 /* To get DYTC status - enable/revision */
> +#define DYTC_CMD_SET           1 /* To enable/disable IC function mode */
> +#define DYTC_CMD_GET           2 /* To get current IC function and mode */
> +#define DYTC_CMD_RESET       511 /* To reset back to default */
> +
> +#define DYTC_QUERY_ENABLE_BIT  8 /* Bit        8 - 0 = disabled, 1 = enabled */
> +#define DYTC_QUERY_SUBREV_BIT 16 /* Bits 16 - 27 - sub revision */
> +#define DYTC_QUERY_REV_BIT    28 /* Bits 28 - 31 - revision */
> +
> +#define DYTC_GET_FUNCTION_BIT  8 /* Bits  8 - 11 - function setting */
> +#define DYTC_GET_MODE_BIT     12 /* Bits 12 - 15 - mode setting */
> +#define DYTC_GET_LAPMODE_BIT  17 /* Bit       17 - set when in lapmode */
> +
> +#define DYTC_SET_FUNCTION_BIT 12 /* Bits 12 - 15 - function setting */
> +#define DYTC_SET_MODE_BIT     16 /* Bits 16 - 19 - mode setting */
> +#define DYTC_SET_VALID_BIT    20 /* Bit       20 - 1 = on, 0 = off */
> +
> +#define DYTC_FUNCTION_STD      0 /* Function =  0, standard mode */
> +#define DYTC_FUNCTION_CQL      1 /* Function =  1, lap mode */
> +#define DYTC_FUNCTION_MMC     11 /* Function = 11, desk mode */
> +
> +#define DYTC_MODE_PERFORMANCE  2 /* High power mode aka performance */
> +#define DYTC_MODE_LOW_POWER    3 /* Low power mode aka quiet */
> +#define DYTC_MODE_BALANCED    15 /* Default mode aka balanced */
> +
> +#define DYTC_SET_COMMAND(function, mode, on) \
> +	(DYTC_CMD_SET | (function) << DYTC_SET_FUNCTION_BIT | \
> +	 (mode) << DYTC_SET_MODE_BIT | \
> +	 (on) << DYTC_SET_VALID_BIT)
> +
> +#define DYTC_DISABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCED, 0)
> +
> +#define DYTC_ENABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCED, 1)
> +
> +static int inline convert_dytc_to_profile(int dytcmode, enum platform_profile_option *profile)
> +{
> +	switch (dytcmode) {
> +	case DYTC_MODE_LOW_POWER:
> +		*profile = PLATFORM_PROFILE_LOW_POWER;
> +		break;
> +	case DYTC_MODE_BALANCED:
> +		*profile =  PLATFORM_PROFILE_BALANCED;
> +		break;
> +	case DYTC_MODE_PERFORMANCE:
> +		*profile =  PLATFORM_PROFILE_PERFORMANCE;
> +		break;
> +	default: /* Unknown mode */
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static inline int convert_profile_to_dytc(enum platform_profile_option profile, int *dytcmode)
> +{
> +	switch (profile) {
> +	case PLATFORM_PROFILE_LOW_POWER:
> +		*dytcmode = DYTC_MODE_LOW_POWER;
> +		break;
> +	case PLATFORM_PROFILE_BALANCED:
> +		*dytcmode = DYTC_MODE_BALANCED;
> +		break;
> +	case PLATFORM_PROFILE_PERFORMANCE:
> +		*dytcmode = DYTC_MODE_PERFORMANCE;
> +		break;
> +	default: /* Unknown profile */
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +#endif /* _LENOVO_DYTC_H_ */
> diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
> index 6cb5ad4be231..387c8c86b8bb 100644
> --- a/drivers/platform/x86/lenovo/ideapad-laptop.c
> +++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
> @@ -35,6 +35,8 @@
>  
>  #include <dt-bindings/leds/common.h>
>  
> +#include "dytc.h"
> +
>  #define IDEAPAD_RFKILL_DEV_NUM	3
>  
>  #if IS_ENABLED(CONFIG_ACPI_WMI)
> @@ -676,76 +678,6 @@ static const struct attribute_group ideapad_attribute_group = {
>  /*
>   * DYTC Platform profile
>   */
> -#define DYTC_CMD_QUERY        0 /* To get DYTC status - enable/revision */
> -#define DYTC_CMD_SET          1 /* To enable/disable IC function mode */
> -#define DYTC_CMD_GET          2 /* To get current IC function and mode */
> -#define DYTC_CMD_RESET    0x1ff /* To reset back to default */
> -
> -#define DYTC_QUERY_ENABLE_BIT 8  /* Bit        8 - 0 = disabled, 1 = enabled */
> -#define DYTC_QUERY_SUBREV_BIT 16 /* Bits 16 - 27 - sub revision */
> -#define DYTC_QUERY_REV_BIT    28 /* Bits 28 - 31 - revision */
> -
> -#define DYTC_GET_FUNCTION_BIT 8  /* Bits  8-11 - function setting */
> -#define DYTC_GET_MODE_BIT     12 /* Bits 12-15 - mode setting */
> -
> -#define DYTC_SET_FUNCTION_BIT 12 /* Bits 12-15 - function setting */
> -#define DYTC_SET_MODE_BIT     16 /* Bits 16-19 - mode setting */
> -#define DYTC_SET_VALID_BIT    20 /* Bit     20 - 1 = on, 0 = off */
> -
> -#define DYTC_FUNCTION_STD     0  /* Function = 0, standard mode */
> -#define DYTC_FUNCTION_CQL     1  /* Function = 1, lap mode */
> -#define DYTC_FUNCTION_MMC     11 /* Function = 11, desk mode */
> -
> -#define DYTC_MODE_PERFORM     2  /* High power mode aka performance */
> -#define DYTC_MODE_LOW_POWER       3  /* Low power mode aka quiet */
> -#define DYTC_MODE_BALANCE   0xF  /* Default mode aka balanced */
> -
> -#define DYTC_SET_COMMAND(function, mode, on) \
> -	(DYTC_CMD_SET | (function) << DYTC_SET_FUNCTION_BIT | \
> -	 (mode) << DYTC_SET_MODE_BIT | \
> -	 (on) << DYTC_SET_VALID_BIT)
> -
> -#define DYTC_DISABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCE, 0)
> -
> -#define DYTC_ENABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCE, 1)
> -
> -static int convert_dytc_to_profile(int dytcmode, enum platform_profile_option *profile)
> -{
> -	switch (dytcmode) {
> -	case DYTC_MODE_LOW_POWER:
> -		*profile = PLATFORM_PROFILE_LOW_POWER;
> -		break;
> -	case DYTC_MODE_BALANCE:
> -		*profile =  PLATFORM_PROFILE_BALANCED;
> -		break;
> -	case DYTC_MODE_PERFORM:
> -		*profile =  PLATFORM_PROFILE_PERFORMANCE;
> -		break;
> -	default: /* Unknown mode */
> -		return -EINVAL;
> -	}
> -
> -	return 0;
> -}
> -
> -static int convert_profile_to_dytc(enum platform_profile_option profile, int *perfmode)
> -{
> -	switch (profile) {
> -	case PLATFORM_PROFILE_LOW_POWER:
> -		*perfmode = DYTC_MODE_LOW_POWER;
> -		break;
> -	case PLATFORM_PROFILE_BALANCED:
> -		*perfmode = DYTC_MODE_BALANCE;
> -		break;
> -	case PLATFORM_PROFILE_PERFORMANCE:
> -		*perfmode = DYTC_MODE_PERFORM;
> -		break;
> -	default: /* Unknown profile */
> -		return -EOPNOTSUPP;
> -	}
> -
> -	return 0;
> -}
>  
>  /*
>   * dytc_profile_get: Function to register with platform_profile
> diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> index b881044b31b0..61990fc9870c 100644
> --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> @@ -74,6 +74,8 @@
>  #include <acpi/battery.h>
>  #include <acpi/video.h>
>  
> +#include "dytc.h"
> +
>  /* ThinkPad CMOS commands */
>  #define TP_CMOS_VOLUME_DOWN	0
>  #define TP_CMOS_VOLUME_UP	1
> @@ -9845,9 +9847,6 @@ static struct ibm_struct lcdshadow_driver_data = {
>   * Thinkpad sensor interfaces
>   */
>  
> -#define DYTC_CMD_GET          2 /* To get current IC function and mode */
> -#define DYTC_GET_LAPMODE_BIT 17 /* Set when in lapmode */
> -
>  #define PALMSENSOR_PRESENT_BIT 0 /* Determine if psensor present */
>  #define PALMSENSOR_ON_BIT      1 /* psensor status */
>  
> @@ -9999,79 +9998,11 @@ static struct ibm_struct proxsensor_driver_data = {
>   * DYTC Platform Profile interface
>   */
>  
> -#define DYTC_CMD_QUERY        0 /* To get DYTC status - enable/revision */
> -#define DYTC_CMD_SET          1 /* To enable/disable IC function mode */
> -#define DYTC_CMD_RESET    0x1ff /* To reset back to default */
> -
> -#define DYTC_QUERY_ENABLE_BIT 8  /* Bit        8 - 0 = disabled, 1 = enabled */
> -#define DYTC_QUERY_SUBREV_BIT 16 /* Bits 16 - 27 - sub revision */
> -#define DYTC_QUERY_REV_BIT    28 /* Bits 28 - 31 - revision */
> -
> -#define DYTC_GET_FUNCTION_BIT 8  /* Bits  8-11 - function setting */
> -#define DYTC_GET_MODE_BIT     12 /* Bits 12-15 - mode setting */
> -
> -#define DYTC_SET_FUNCTION_BIT 12 /* Bits 12-15 - function setting */
> -#define DYTC_SET_MODE_BIT     16 /* Bits 16-19 - mode setting */
> -#define DYTC_SET_VALID_BIT    20 /* Bit     20 - 1 = on, 0 = off */
> -
> -#define DYTC_FUNCTION_STD     0  /* Function = 0, standard mode */
> -#define DYTC_FUNCTION_CQL     1  /* Function = 1, lap mode */
> -#define DYTC_FUNCTION_MMC     11 /* Function = 11, desk mode */
> -
> -#define DYTC_MODE_PERFORM     2  /* High power mode aka performance */
> -#define DYTC_MODE_LOWPOWER    3  /* Low power mode */
> -#define DYTC_MODE_BALANCE   0xF  /* Default mode aka balanced */
> -
> -#define DYTC_SET_COMMAND(function, mode, on) \
> -	(DYTC_CMD_SET | (function) << DYTC_SET_FUNCTION_BIT | \
> -	 (mode) << DYTC_SET_MODE_BIT | \
> -	 (on) << DYTC_SET_VALID_BIT)
> -
> -#define DYTC_DISABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCE, 0)
> -
> -#define DYTC_ENABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCE, 1)
> -
>  static bool dytc_profile_available;
>  static enum platform_profile_option dytc_current_profile;
>  static atomic_t dytc_ignore_event = ATOMIC_INIT(0);
>  static DEFINE_MUTEX(dytc_mutex);
>  
> -static int convert_dytc_to_profile(int dytcmode, enum platform_profile_option *profile)
> -{
> -	switch (dytcmode) {
> -	case DYTC_MODE_LOWPOWER:
> -		*profile = PLATFORM_PROFILE_LOW_POWER;
> -		break;
> -	case DYTC_MODE_BALANCE:
> -		*profile =  PLATFORM_PROFILE_BALANCED;
> -		break;
> -	case DYTC_MODE_PERFORM:
> -		*profile =  PLATFORM_PROFILE_PERFORMANCE;
> -		break;
> -	default: /* Unknown mode */
> -		return -EINVAL;
> -	}
> -	return 0;
> -}
> -
> -static int convert_profile_to_dytc(enum platform_profile_option profile, int *perfmode)
> -{
> -	switch (profile) {
> -	case PLATFORM_PROFILE_LOW_POWER:
> -		*perfmode = DYTC_MODE_LOWPOWER;
> -		break;
> -	case PLATFORM_PROFILE_BALANCED:
> -		*perfmode = DYTC_MODE_BALANCE;
> -		break;
> -	case PLATFORM_PROFILE_PERFORMANCE:
> -		*perfmode = DYTC_MODE_PERFORM;
> -		break;
> -	default: /* Unknown profile */
> -		return -EOPNOTSUPP;
> -	}
> -	return 0;
> -}
> -
>  /*
>   * dytc_profile_get: Function to register with platform_profile
>   * handler. Returns current platform profile.
> 

Re: [RFC PATCH 0/2] platform/x86: thinkpad_acpi/ideapad-laptop: move into subfolder and extract common DYTC code

[Hans de Goede]

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%)
> 

Re: [RFC PATCH 0/2] platform/x86: thinkpad_acpi/ideapad-laptop: move into subfolder and extract common DYTC code

[Barnabás Pőcze]

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

Re: [RFC PATCH 0/2] platform/x86: thinkpad_acpi/ideapad-laptop: move into subfolder and extract common DYTC code

[Hans de Goede]

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