ibm-acpi-devel — ibm-acpi Linux driver development

Re: [ibm-acpi-devel] [PATCH] platform/x86: remove obsolete calls to ledtrig_audio_get

[Hans de G. <hde...@re...>]

Hi Heiner

On 1/13/24 18:03, Heiner Kallweit wrote:
> Since 64f67b5240db ("leds: trigger: audio: Add an activate callback to
> ensure the initial brightness is set") the audio triggers have an
> activate callback which sets the LED brightness as soon as the
> (default) trigger is bound to the LED device. So we can remove the
> call to ledtrig_audio_get.
> 
> Positive side effect: There's no code dependency to ledtrig-audio any
> longer.

Thank you for your patch since these drivers now no longer
depend on the ledtrig-audio module can you please remove the:

        select LEDS_TRIGGERS
        select LEDS_TRIGGER_AUDIO

lines from the Kconfig bits for the modified drivers?

Regards,

Hans



> 
> Signed-off-by: Heiner Kallweit <hka...@gm...>
> ---
>  drivers/platform/x86/asus-wmi.c              | 1 -
>  drivers/platform/x86/dell/dell-laptop.c      | 2 --
>  drivers/platform/x86/dell/dell-wmi-privacy.c | 1 -
>  drivers/platform/x86/huawei-wmi.c            | 1 -
>  drivers/platform/x86/thinkpad_acpi.c         | 1 -
>  5 files changed, 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 18be35fdb..21dee425e 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -1620,7 +1620,6 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
>  	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MICMUTE_LED)) {
>  		asus->micmute_led.name = "platform::micmute";
>  		asus->micmute_led.max_brightness = 1;
> -		asus->micmute_led.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
>  		asus->micmute_led.brightness_set_blocking = micmute_led_set;
>  		asus->micmute_led.default_trigger = "audio-micmute";
>  
> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
> index 658643835..42f7de2b4 100644
> --- a/drivers/platform/x86/dell/dell-laptop.c
> +++ b/drivers/platform/x86/dell/dell-laptop.c
> @@ -2252,7 +2252,6 @@ static int __init dell_init(void)
>  	if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
>  	    dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE) &&
>  	    !dell_privacy_has_mic_mute()) {
> -		micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
>  		ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
>  		if (ret < 0)
>  			goto fail_led;
> @@ -2261,7 +2260,6 @@ static int __init dell_init(void)
>  
>  	if (dell_smbios_find_token(GLOBAL_MUTE_DISABLE) &&
>  	    dell_smbios_find_token(GLOBAL_MUTE_ENABLE)) {
> -		mute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MUTE);
>  		ret = led_classdev_register(&platform_device->dev, &mute_led_cdev);
>  		if (ret < 0)
>  			goto fail_backlight;
> diff --git a/drivers/platform/x86/dell/dell-wmi-privacy.c b/drivers/platform/x86/dell/dell-wmi-privacy.c
> index c517bd45d..4d94603f7 100644
> --- a/drivers/platform/x86/dell/dell-wmi-privacy.c
> +++ b/drivers/platform/x86/dell/dell-wmi-privacy.c
> @@ -288,7 +288,6 @@ static int dell_privacy_leds_setup(struct device *dev)
>  	priv->cdev.max_brightness = 1;
>  	priv->cdev.brightness_set_blocking = dell_privacy_micmute_led_set;
>  	priv->cdev.default_trigger = "audio-micmute";
> -	priv->cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
>  	return devm_led_classdev_register(dev, &priv->cdev);
>  }
>  
> diff --git a/drivers/platform/x86/huawei-wmi.c b/drivers/platform/x86/huawei-wmi.c
> index 0ef1c46b6..dde139c69 100644
> --- a/drivers/platform/x86/huawei-wmi.c
> +++ b/drivers/platform/x86/huawei-wmi.c
> @@ -310,7 +310,6 @@ static void huawei_wmi_leds_setup(struct device *dev)
>  	huawei->cdev.max_brightness = 1;
>  	huawei->cdev.brightness_set_blocking = &huawei_wmi_micmute_led_set;
>  	huawei->cdev.default_trigger = "audio-micmute";
> -	huawei->cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
>  	huawei->cdev.dev = dev;
>  	huawei->cdev.flags = LED_CORE_SUSPENDRESUME;
>  
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index c4895e9bc..d1c9f91fd 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9285,7 +9285,6 @@ static int mute_led_init(struct ibm_init_struct *iibm)
>  			continue;
>  		}
>  
> -		mute_led_cdev[i].brightness = ledtrig_audio_get(i);
>  		err = led_classdev_register(&tpacpi_pdev->dev, &mute_led_cdev[i]);
>  		if (err < 0) {
>  			while (i--)

Re: [ibm-acpi-devel] [PATCH AUTOSEL 6.1 12/24] platform/x86: thinkpad_acpi: fix for incorrect fan reporting on some ThinkPad systems

[Pavel M. <pa...@uc...>]

Hi!

> [ Upstream commit 66e92e23a72761f5b53f970aeb1badc5fd92fc74 ]
> 
> Some ThinkPad systems ECFW use non-standard addresses for fan control
> and reporting. This patch adds support for such ECFW so that it can report
> the correct fan values.
> Tested on Thinkpads L13 Yoga Gen 2 and X13 Yoga Gen 2.

This is just a new feature, and is > 200 lines. We should not have
this in stable.

BR,
								Pavel





> Suggested-by: Mark Pearson <mpe...@sq...>
> Signed-off-by: Vishnu Sankar <vis...@gm...>
> Reviewed-by: Hans de Goede <hde...@re...>
> Reviewed-by: Ilpo Järvinen <ilp...@li...>
> Link: https://lore.kernel.org/r/202...@gm...
> Signed-off-by: Ilpo Järvinen <ilp...@li...>
> Signed-off-by: Sasha Levin <sa...@ke...>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 98 ++++++++++++++++++++++++----
>  1 file changed, 85 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 05a55bc31c796..6edd2e294750e 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -8149,8 +8149,19 @@ static struct ibm_struct volume_driver_data = {
>   * 	TPACPI_FAN_WR_TPEC is also available and should be used to
>   * 	command the fan.  The X31/X40/X41 seems to have 8 fan levels,
>   * 	but the ACPI tables just mention level 7.
> + *
> + * TPACPI_FAN_RD_TPEC_NS:
> + *	This mode is used for a few ThinkPads (L13 Yoga Gen2, X13 Yoga Gen2 etc.)
> + *	that are using non-standard EC locations for reporting fan speeds.
> + *	Currently these platforms only provide fan rpm reporting.
> + *
>   */
>  
> +#define FAN_RPM_CAL_CONST 491520	/* FAN RPM calculation offset for some non-standard ECFW */
> +
> +#define FAN_NS_CTRL_STATUS	BIT(2)		/* Bit which determines control is enabled or not */
> +#define FAN_NS_CTRL		BIT(4)		/* Bit which determines control is by host or EC */
> +
>  enum {					/* Fan control constants */
>  	fan_status_offset = 0x2f,	/* EC register 0x2f */
>  	fan_rpm_offset = 0x84,		/* EC register 0x84: LSB, 0x85 MSB (RPM)
> @@ -8158,6 +8169,11 @@ enum {					/* Fan control constants */
>  	fan_select_offset = 0x31,	/* EC register 0x31 (Firmware 7M)
>  					   bit 0 selects which fan is active */
>  
> +	fan_status_offset_ns = 0x93,	/* Special status/control offset for non-standard EC Fan1 */
> +	fan2_status_offset_ns = 0x96,	/* Special status/control offset for non-standard EC Fan2 */
> +	fan_rpm_status_ns = 0x95,	/* Special offset for Fan1 RPM status for non-standard EC */
> +	fan2_rpm_status_ns = 0x98,	/* Special offset for Fan2 RPM status for non-standard EC */
> +
>  	TP_EC_FAN_FULLSPEED = 0x40,	/* EC fan mode: full speed */
>  	TP_EC_FAN_AUTO	    = 0x80,	/* EC fan mode: auto fan control */
>  
> @@ -8168,6 +8184,7 @@ enum fan_status_access_mode {
>  	TPACPI_FAN_NONE = 0,		/* No fan status or control */
>  	TPACPI_FAN_RD_ACPI_GFAN,	/* Use ACPI GFAN */
>  	TPACPI_FAN_RD_TPEC,		/* Use ACPI EC regs 0x2f, 0x84-0x85 */
> +	TPACPI_FAN_RD_TPEC_NS,		/* Use non-standard ACPI EC regs (eg: L13 Yoga gen2 etc.) */
>  };
>  
>  enum fan_control_access_mode {
> @@ -8195,6 +8212,8 @@ static u8 fan_control_desired_level;
>  static u8 fan_control_resume_level;
>  static int fan_watchdog_maxinterval;
>  
> +static bool fan_with_ns_addr;
> +
>  static struct mutex fan_mutex;
>  
>  static void fan_watchdog_fire(struct work_struct *ignored);
> @@ -8325,6 +8344,15 @@ static int fan_get_status(u8 *status)
>  		}
>  
>  		break;
> +	case TPACPI_FAN_RD_TPEC_NS:
> +		/* Default mode is AUTO which means controlled by EC */
> +		if (!acpi_ec_read(fan_status_offset_ns, &s))
> +			return -EIO;
> +
> +		if (status)
> +			*status = s;
> +
> +		break;
>  
>  	default:
>  		return -ENXIO;
> @@ -8341,7 +8369,8 @@ static int fan_get_status_safe(u8 *status)
>  	if (mutex_lock_killable(&fan_mutex))
>  		return -ERESTARTSYS;
>  	rc = fan_get_status(&s);
> -	if (!rc)
> +	/* NS EC doesn't have register with level settings */
> +	if (!rc && !fan_with_ns_addr)
>  		fan_update_desired_level(s);
>  	mutex_unlock(&fan_mutex);
>  
> @@ -8368,7 +8397,13 @@ static int fan_get_speed(unsigned int *speed)
>  
>  		if (likely(speed))
>  			*speed = (hi << 8) | lo;
> +		break;
> +	case TPACPI_FAN_RD_TPEC_NS:
> +		if (!acpi_ec_read(fan_rpm_status_ns, &lo))
> +			return -EIO;
>  
> +		if (speed)
> +			*speed = lo ? FAN_RPM_CAL_CONST / lo : 0;
>  		break;
>  
>  	default:
> @@ -8380,7 +8415,7 @@ static int fan_get_speed(unsigned int *speed)
>  
>  static int fan2_get_speed(unsigned int *speed)
>  {
> -	u8 hi, lo;
> +	u8 hi, lo, status;
>  	bool rc;
>  
>  	switch (fan_status_access_mode) {
> @@ -8396,7 +8431,21 @@ static int fan2_get_speed(unsigned int *speed)
>  
>  		if (likely(speed))
>  			*speed = (hi << 8) | lo;
> +		break;
>  
> +	case TPACPI_FAN_RD_TPEC_NS:
> +		rc = !acpi_ec_read(fan2_status_offset_ns, &status);
> +		if (rc)
> +			return -EIO;
> +		if (!(status & FAN_NS_CTRL_STATUS)) {
> +			pr_info("secondary fan control not supported\n");
> +			return -EIO;
> +		}
> +		rc = !acpi_ec_read(fan2_rpm_status_ns, &lo);
> +		if (rc)
> +			return -EIO;
> +		if (speed)
> +			*speed = lo ? FAN_RPM_CAL_CONST / lo : 0;
>  		break;
>  
>  	default:
> @@ -8899,6 +8948,7 @@ static const struct attribute_group fan_driver_attr_group = {
>  #define TPACPI_FAN_2FAN		0x0002		/* EC 0x31 bit 0 selects fan2 */
>  #define TPACPI_FAN_2CTL		0x0004		/* selects fan2 control */
>  #define TPACPI_FAN_NOFAN	0x0008		/* no fan available */
> +#define TPACPI_FAN_NS		0x0010		/* For EC with non-Standard register addresses */
>  
>  static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
>  	TPACPI_QEC_IBM('1', 'Y', TPACPI_FAN_Q1),
> @@ -8917,6 +8967,8 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
>  	TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2CTL),	/* P1 / X1 Extreme (2nd gen) */
>  	TPACPI_Q_LNV3('N', '3', '0', TPACPI_FAN_2CTL),	/* P15 (1st gen) / P15v (1st gen) */
>  	TPACPI_Q_LNV3('N', '3', '7', TPACPI_FAN_2CTL),  /* T15g (2nd gen) */
> +	TPACPI_Q_LNV3('R', '1', 'F', TPACPI_FAN_NS),	/* L13 Yoga Gen 2 */
> +	TPACPI_Q_LNV3('N', '2', 'U', TPACPI_FAN_NS),	/* X13 Yoga Gen 2*/
>  	TPACPI_Q_LNV3('N', '1', 'O', TPACPI_FAN_NOFAN),	/* X1 Tablet (2nd gen) */
>  };
>  
> @@ -8951,18 +9003,27 @@ static int __init fan_init(struct ibm_init_struct *iibm)
>  		return -ENODEV;
>  	}
>  
> +	if (quirks & TPACPI_FAN_NS) {
> +		pr_info("ECFW with non-standard fan reg control found\n");
> +		fan_with_ns_addr = 1;
> +		/* Fan ctrl support from host is undefined for now */
> +		tp_features.fan_ctrl_status_undef = 1;
> +	}
> +
>  	if (gfan_handle) {
>  		/* 570, 600e/x, 770e, 770x */
>  		fan_status_access_mode = TPACPI_FAN_RD_ACPI_GFAN;
>  	} else {
>  		/* all other ThinkPads: note that even old-style
>  		 * ThinkPad ECs supports the fan control register */
> -		if (likely(acpi_ec_read(fan_status_offset,
> -					&fan_control_initial_status))) {
> +		if (fan_with_ns_addr ||
> +		    likely(acpi_ec_read(fan_status_offset, &fan_control_initial_status))) {
>  			int res;
>  			unsigned int speed;
>  
> -			fan_status_access_mode = TPACPI_FAN_RD_TPEC;
> +			fan_status_access_mode = fan_with_ns_addr ?
> +				TPACPI_FAN_RD_TPEC_NS : TPACPI_FAN_RD_TPEC;
> +
>  			if (quirks & TPACPI_FAN_Q1)
>  				fan_quirk1_setup();
>  			/* Try and probe the 2nd fan */
> @@ -8971,7 +9032,8 @@ static int __init fan_init(struct ibm_init_struct *iibm)
>  			if (res >= 0 && speed != FAN_NOT_PRESENT) {
>  				/* It responded - so let's assume it's there */
>  				tp_features.second_fan = 1;
> -				tp_features.second_fan_ctl = 1;
> +				/* fan control not currently available for ns ECFW */
> +				tp_features.second_fan_ctl = !fan_with_ns_addr;
>  				pr_info("secondary fan control detected & enabled\n");
>  			} else {
>  				/* Fan not auto-detected */
> @@ -9146,6 +9208,7 @@ static int fan_read(struct seq_file *m)
>  			       str_enabled_disabled(status), status);
>  		break;
>  
> +	case TPACPI_FAN_RD_TPEC_NS:
>  	case TPACPI_FAN_RD_TPEC:
>  		/* all except 570, 600e/x, 770e, 770x */
>  		rc = fan_get_status_safe(&status);
> @@ -9160,13 +9223,22 @@ static int fan_read(struct seq_file *m)
>  
>  		seq_printf(m, "speed:\t\t%d\n", speed);
>  
> -		if (status & TP_EC_FAN_FULLSPEED)
> -			/* Disengaged mode takes precedence */
> -			seq_printf(m, "level:\t\tdisengaged\n");
> -		else if (status & TP_EC_FAN_AUTO)
> -			seq_printf(m, "level:\t\tauto\n");
> -		else
> -			seq_printf(m, "level:\t\t%d\n", status);
> +		if (fan_status_access_mode == TPACPI_FAN_RD_TPEC_NS) {
> +			/*
> +			 * No full speed bit in NS EC
> +			 * EC Auto mode is set by default.
> +			 * No other levels settings available
> +			 */
> +			seq_printf(m, "level:\t\t%s\n", status & FAN_NS_CTRL ? "unknown" : "auto");
> +		} else {
> +			if (status & TP_EC_FAN_FULLSPEED)
> +				/* Disengaged mode takes precedence */
> +				seq_printf(m, "level:\t\tdisengaged\n");
> +			else if (status & TP_EC_FAN_AUTO)
> +				seq_printf(m, "level:\t\tauto\n");
> +			else
> +				seq_printf(m, "level:\t\t%d\n", status);
> +		}
>  		break;
>  
>  	case TPACPI_FAN_NONE:

-- 
People of Russia, stop Putin before his war on Ukraine escalates.

Re: [ibm-acpi-devel] [PATCH][next] platform/x86: thinkpad_acpi: remove redundant assignment to variable i

[Hans de G. <hde...@re...>]

Hi,

On 1/6/24 16:47, Colin Ian King wrote:
> The variable i is being initialized with the value 0 that is never
> read, it is being re-assigned 0 again in a for-loop statement later
> on. The initialization is redundant and can be removed.
> 
> The initialization of variable n can also be deferred after the
> sanity check on pointer n and the declaration of all the int variables
> can be combined as a final code clear-up.
> 
> Cleans up clang scan build warning:
> warning: Value stored to 'i' is never read [deadcode.DeadStores]
> 
> Signed-off-by: Colin Ian King <col...@gm...>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hde...@re...>

Regards,

Hans



> ---
>  drivers/platform/x86/thinkpad_acpi.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index c4895e9bc714..7bf91cfd3e51 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -6208,17 +6208,15 @@ static int thermal_get_sensor(int idx, s32 *value)
>  
>  static int thermal_get_sensors(struct ibm_thermal_sensors_struct *s)
>  {
> -	int res, i;
> -	int n;
> -
> -	n = 8;
> -	i = 0;
> +	int res, i, n;
>  
>  	if (!s)
>  		return -EINVAL;
>  
>  	if (thermal_read_mode == TPACPI_THERMAL_TPEC_16)
>  		n = 16;
> +	else
> +		n = 8;
>  
>  	for (i = 0 ; i < n; i++) {
>  		res = thermal_get_sensor(i, &s->temp[i]);

Re: [ibm-acpi-devel] [PATCH RFC] platform/x86: Lenovo sub directory

[Mark P. <mpe...@sq...>]

Thanks for the review Hans,

On Mon, Jan 8, 2024, at 9:17 AM, Hans de Goede wrote:
> Hi Mark,
>
> On 1/4/24 01:37, Mark Pearson wrote:
>> Looking for feedback if this is a good idea or not, and if I've missed
>> anything important.
>> 
>> Over the holidays it was raining and I was bored so I was toying with the
>> idea of moving some of the thinkpad_acpi functionality out of the file
>> into their own modules - the file is a bit of a beast. I'd like to try and
>> get any commonality between thinkpad, ideapad, etc where possible.
>> My plan was to first look at pulling out the platform_profile pieces and
>> then extend to other pieces (fans, temp, sensors, etc).
>> 
>> Doing this will, potentially, create a number of lenovo_xxx files and so it
>> seemed nice to put lenovo stuff in it's own subdirectory (in a similar way
>> to other vendors) before starting the exercise.
>> 
>> This was my attempt to see if it was easy - and it was.
>> 
>> Please let me know:
>> 
>> 1) Is this OK to do, or does it cause any problems?
>
> Moving the lenovo drivers and especially removing the duplicate
> functionality sounds good to me.
>
>> 2) Have I missed anything important?
>
> I have a few small remarks below, other then that this looks good
> to me.
>
>> 3) I don't want to tread on any toes - so if there is protocol to follow
>> with moving files please let me know :) (Or a preferred way to do such an
>> exercise)
>
> No special protocol for moving files.
>
>> 4) I don't have any ideapads to test with. I think this is low risk, but
>> if anybody is able to confirm nothing breaks please let me know.
>
> The moving should definitely be safe. For the refactoring planned on top
> would be good if you can test on some actual hw.
>
>> I will see if I can scrounge some HW from somewhere in the meantime.
>> 
>> I will need to rebase before proposing this officially, so please ignore
>> the fact that this is based off 6.7-rc1 and therefore a bit out of date.
>> 
>> Signed-off-by: Mark Pearson <mpe...@sq...>
>> ---
>>  .../admin-guide/laptops/thinkpad-acpi.rst     |   3 +-
>>  MAINTAINERS                                   |   6 +-
>>  drivers/platform/x86/Kconfig                  | 196 +---------------
>>  drivers/platform/x86/Makefile                 |  10 +-
>>  drivers/platform/x86/lenovo/Kconfig           | 211 ++++++++++++++++++
>>  drivers/platform/x86/lenovo/Makefile          |  13 ++
>>  .../x86/{ => lenovo}/ideapad-laptop.c         |   0
>>  .../x86/{ => lenovo}/ideapad-laptop.h         |   0
>>  .../platform/x86/{ => lenovo}/lenovo-ymc.c    |   0
>>  .../x86/{ => lenovo}/lenovo-yogabook.c        |   0
>>  drivers/platform/x86/{ => lenovo}/think-lmi.c |   2 +-
>>  drivers/platform/x86/{ => lenovo}/think-lmi.h |   0
>>  .../platform/x86/{ => lenovo}/thinkpad_acpi.c |   2 +-
>>  13 files changed, 238 insertions(+), 205 deletions(-)
>>  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}/ideapad-laptop.h (100%)
>>  rename drivers/platform/x86/{ => lenovo}/lenovo-ymc.c (100%)
>>  rename drivers/platform/x86/{ => lenovo}/lenovo-yogabook.c (100%)
>>  rename drivers/platform/x86/{ => lenovo}/think-lmi.c (99%)
>>  rename drivers/platform/x86/{ => lenovo}/think-lmi.h (100%)
>>  rename drivers/platform/x86/{ => lenovo}/thinkpad_acpi.c (99%)
>> 
>> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> index 98d304010170..55b79ee2bb26 100644
>> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> @@ -20,7 +20,8 @@ This driver used to be named ibm-acpi until kernel 2.6.21 and release
>>  0.13-20070314.  It used to be in the drivers/acpi tree, but it was
>>  moved to the drivers/misc tree and renamed to thinkpad-acpi for kernel
>>  2.6.22, and release 0.14.  It was moved to drivers/platform/x86 for
>> -kernel 2.6.29 and release 0.22.
>> +kernel 2.6.29 and release 0.22. It was moved to drivers/platform/x86/lenovo
>> +for kernel 6.8.
>
> The 6.8 merge window just opened so this is only going to land in the
> next cycle which will be 6.9 .
No problems.

>
>>  
>>  The driver is named "thinkpad-acpi".  In some places, like module
>>  names and log messages, "thinkpad_acpi" is used because of userspace
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 97f51d5ec1cf..c83ed9a51a44 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -10243,7 +10243,7 @@ M:	Ike Panhc <ik...@ca...>
>>  L:	pla...@vg...
>>  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...@gm...>
>> @@ -21637,14 +21637,14 @@ 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
>>  
>>  THINKPAD LMI DRIVER
>>  M:	Mark Pearson <mar...@le...>
>>  L:	pla...@vg...
>>  S:	Maintained
>>  F:	Documentation/ABI/testing/sysfs-class-firmware-attributes
>> -F:	drivers/platform/x86/think-lmi.?
>> +F:	drivers/platform/x86/lenovo/think-lmi.?
>>  
>>  THUNDERBOLT DMA TRAFFIC TEST DRIVER
>>  M:	Isaac Hazan <isa...@in...>
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index 7e69fdaccdd5..842ced89bd82 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -121,20 +121,6 @@ config GIGABYTE_WMI
>>  	  To compile this driver as a module, choose M here: the module will
>>  	  be called gigabyte-wmi.
>>  
>> -config YOGABOOK
>> -	tristate "Lenovo Yoga Book tablet key driver"
>> -	depends on ACPI_WMI
>> -	depends on INPUT
>> -	depends on I2C
>> -	select LEDS_CLASS
>> -	select NEW_LEDS
>> -	help
>> -	  Say Y here if you want to support the 'Pen' key and keyboard backlight
>> -	  control on the Lenovo Yoga Book tablets.
>> -
>> -	  To compile this driver as a module, choose M here: the module will
>> -	  be called lenovo-yogabook.
>> -
>>  config ACERHDF
>>  	tristate "Acer Aspire One temperature and fan driver"
>>  	depends on ACPI && THERMAL
>> @@ -430,6 +416,7 @@ config WIRELESS_HOTKEY
>>  	 To compile this driver as a module, choose M here: the module will
>>  	 be called wireless-hotkey.
>>  
>> +
>>  config IBM_RTL
>>  	tristate "Device driver to enable PRTL support"
>>  	depends on PCI
>
> stray blank line insertion, please drop this.
Ack

>
>
>> @@ -446,31 +433,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
>> -	select 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 LENOVO_YMC
>> -	tristate "Lenovo Yoga Tablet Mode Control"
>> -	depends on ACPI_WMI
>> -	depends on INPUT
>> -	select INPUT_SPARSEKMAP
>> -	help
>> -	  This driver maps the Tablet Mode Control switch to SW_TABLET_MODE input
>> -	  events for Lenovo Yoga notebooks.
>> -
>>  config SENSORS_HDAPS
>>  	tristate "Thinkpad Hard Drive Active Protection System (hdaps)"
>>  	depends on INPUT
>> @@ -489,162 +451,10 @@ 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 I2C
>> -	depends on DRM
>> -	select ACPI_PLATFORM_PROFILE
>> -	select DRM_PRIVACY_SCREEN
>> -	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.
>> -
>> -config THINKPAD_LMI
>> -	tristate "Lenovo WMI-based systems management driver"
>> -	depends on ACPI_WMI
>> -	select FW_ATTR_CLASS
>> -	help
>> -	  This driver allows changing BIOS settings on Lenovo machines whose
>> -	  BIOS support the WMI interface.
>> -
>> -	  To compile this driver as a module, choose M here: the module will
>> -	  be called think-lmi.
>> -
>>  source "drivers/platform/x86/intel/Kconfig"
>>  
>> +source "drivers/platform/x86/lenovo/Kconfig"
>> +
>>  config MSI_EC
>>  	tristate "MSI EC Extras"
>>  	depends on ACPI
>
> Random remark: it would certainly be good if we can reduce the number
> of THINKPAD_ACPI related Kconfig symbols ...
Agreed :)

>
>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>> index c7a18e95ad8c..ccf3610cb34b 100644
>> --- a/drivers/platform/x86/Makefile
>> +++ b/drivers/platform/x86/Makefile
>> @@ -58,18 +58,16 @@ obj-$(CONFIG_X86_PLATFORM_DRIVERS_HP)	+= hp/
>>  # Hewlett Packard Enterprise
>>  obj-$(CONFIG_UV_SYSFS)       += uv_sysfs.o
>>  
>> -# IBM Thinkpad and Lenovo
>> +# IBM Thinkpad
>>  obj-$(CONFIG_IBM_RTL)		+= ibm_rtl.o
>> -obj-$(CONFIG_IDEAPAD_LAPTOP)	+= ideapad-laptop.o
>> -obj-$(CONFIG_LENOVO_YMC)	+= lenovo-ymc.o
>>  obj-$(CONFIG_SENSORS_HDAPS)	+= hdaps.o
>> -obj-$(CONFIG_THINKPAD_ACPI)	+= thinkpad_acpi.o
>> -obj-$(CONFIG_THINKPAD_LMI)	+= think-lmi.o
>> -obj-$(CONFIG_YOGABOOK)		+= lenovo-yogabook.o
>>  
>>  # Intel
>>  obj-y				+= intel/
>>  
>> +# Lenovo
>> +obj-$(CONFIG_X86_PLATFORM_DRIVERS_LENOVO) += lenovo/
>> +
>>  # MSI
>>  obj-$(CONFIG_MSI_EC)		+= msi-ec.o
>>  obj-$(CONFIG_MSI_LAPTOP)	+= msi-laptop.o
>> diff --git a/drivers/platform/x86/lenovo/Kconfig b/drivers/platform/x86/lenovo/Kconfig
>> new file mode 100644
>> index 000000000000..a4de6f5b841d
>> --- /dev/null
>> +++ b/drivers/platform/x86/lenovo/Kconfig
>> @@ -0,0 +1,211 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +#
>> +# Lenovo X86 Platform Specific Drivers
>> +#
>> +
>> +menuconfig X86_PLATFORM_DRIVERS_LENOVO
>> +	bool "Lenovo X86 Platform Specific Device Drivers"
>> +	default y
>> +	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 YOGABOOK
>> +	tristate "Lenovo Yoga Book tablet key driver"
>> +	depends on ACPI_WMI
>> +	depends on INPUT
>> +	depends on I2C
>> +	select LEDS_CLASS
>> +	select NEW_LEDS
>> +	help
>> +	  Say Y here if you want to support the 'Pen' key and keyboard backlight
>> +	  control on the Lenovo Yoga Book tablets.
>> +
>> +	  To compile this driver as a module, choose M here: the module will
>> +	  be called lenovo-yogabook.
>> +
>> +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
>> +	select 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 LENOVO_YMC
>> +	tristate "Lenovo Yoga Tablet Mode Control"
>> +	depends on ACPI_WMI
>> +	depends on INPUT
>> +	select INPUT_SPARSEKMAP
>> +	help
>> +	  This driver maps the Tablet Mode Control switch to SW_TABLET_MODE input
>> +	  events for Lenovo Yoga notebooks.
>> +
>> +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 I2C
>> +	depends on DRM
>> +	select ACPI_PLATFORM_PROFILE
>> +	select DRM_PRIVACY_SCREEN
>> +	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.
>> +
>> +config THINKPAD_LMI
>> +	tristate "Lenovo WMI-based systems management driver"
>> +	depends on ACPI_WMI
>> +	select FW_ATTR_CLASS
>> +	help
>> +	  This driver allows changing BIOS settings on Lenovo machines whose
>> +	  BIOS support the WMI interface.
>> +
>> +	  To compile this driver as a module, choose M here: the module will
>> +	  be called think-lmi.
>> +
>> +endif # X86_PLATFORM_DRIVERS_LENOVO
>
> Maybe sort the entries alphabetically, putting the 2 yoga drivers at the end ?
>
> I assume all these entries are just moved and no changes wrt
> depends / selects have been made ?
Yes - I was deliberately trying to minimise the changes.
Only thing is I did make X86_PLATFORM_DRIVERS_LENOVO have default 'y' because I was worried when this got picked up if distro's didn't have this set then Lenovo platforms would stop working and I would get yelled at....a lot. 
Everything else is the same as before.

>
>> diff --git a/drivers/platform/x86/lenovo/Makefile b/drivers/platform/x86/lenovo/Makefile
>> new file mode 100644
>> index 000000000000..4f8d6ed369b8
>> --- /dev/null
>> +++ b/drivers/platform/x86/lenovo/Makefile
>> @@ -0,0 +1,13 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +#
>> +# Makefile for linux/drivers/platform/x86/lenovo
>> +# Lenovo x86 Platform-Specific Drivers
>> +#
>> +
>> +obj-$(CONFIG_IBM_RTL)		+= ibm_rtl.o
>
> This looks wrong, since you are keeping this in the main
> drivers/platform/x86/Makefile and Kconfig
Ooops - yes, my mistake.
Originally I moved hdaps and ibm_rtl into the Lenovo directory but on further consideration I decided that was a bad idea as these are originally IBM files (pre the business getting sold to Lenovo) and I should leave them where they were.
I missed this when reverting my change. I'll clean this up. Apologies.

>
>> +obj-$(CONFIG_IDEAPAD_LAPTOP)	+= ideapad-laptop.o
>> +obj-$(CONFIG_LENOVO_YMC)	+= lenovo-ymc.o
>> +obj-$(CONFIG_SENSORS_HDAPS)	+= hdaps.o
>
> same remark for the hdaps driver.
ditto above.

>
>> +obj-$(CONFIG_THINKPAD_ACPI)	+= thinkpad_acpi.o
>> +obj-$(CONFIG_THINKPAD_LMI)	+= think-lmi.o
>> +obj-$(CONFIG_YOGABOOK)		+= lenovo-yogabook.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/ideapad-laptop.h b/drivers/platform/x86/lenovo/ideapad-laptop.h
>> similarity index 100%
>> rename from drivers/platform/x86/ideapad-laptop.h
>> rename to drivers/platform/x86/lenovo/ideapad-laptop.h
>> diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo/lenovo-ymc.c
>> similarity index 100%
>> rename from drivers/platform/x86/lenovo-ymc.c
>> rename to drivers/platform/x86/lenovo/lenovo-ymc.c
>> diff --git a/drivers/platform/x86/lenovo-yogabook.c b/drivers/platform/x86/lenovo/lenovo-yogabook.c
>> similarity index 100%
>> rename from drivers/platform/x86/lenovo-yogabook.c
>> rename to drivers/platform/x86/lenovo/lenovo-yogabook.c
>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/lenovo/think-lmi.c
>> similarity index 99%
>> rename from drivers/platform/x86/think-lmi.c
>> rename to drivers/platform/x86/lenovo/think-lmi.c
>> index 3a396b763c49..bf688df50856 100644
>> --- a/drivers/platform/x86/think-lmi.c
>> +++ b/drivers/platform/x86/lenovo/think-lmi.c
>> @@ -19,7 +19,7 @@
>>  #include <linux/types.h>
>>  #include <linux/dmi.h>
>>  #include <linux/wmi.h>
>> -#include "firmware_attributes_class.h"
>> +#include "../firmware_attributes_class.h"
>>  #include "think-lmi.h"
>>  
>>  static bool debug_support;
>> diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/lenovo/think-lmi.h
>> similarity index 100%
>> rename from drivers/platform/x86/think-lmi.h
>> rename to drivers/platform/x86/lenovo/think-lmi.h
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
>> similarity index 99%
>> rename from drivers/platform/x86/thinkpad_acpi.c
>> rename to drivers/platform/x86/lenovo/thinkpad_acpi.c
>> index d0b5fd4137bc..7d085d4e02ee 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
>> @@ -80,7 +80,7 @@
>>  #include <sound/core.h>
>>  #include <sound/initval.h>
>>  
>> -#include "dual_accel_detect.h"
>> +#include "../dual_accel_detect.h"
>>  
>>  /* ThinkPad CMOS commands */
>>  #define TP_CMOS_VOLUME_DOWN	0
>
> Regards,
>
> Hans

I have another thinkpad_acpi patch I'm working on that is more important (or at least functionally useful) so I think I'll get that proposed and reviewed first; and then redo this series on the latest with the fixes.
Thanks for the review!

Mark

Re: [ibm-acpi-devel] [PATCH RFC] platform/x86: Lenovo sub directory

[Hans de G. <hde...@re...>]

Hi Mark,

On 1/4/24 01:37, Mark Pearson wrote:
> Looking for feedback if this is a good idea or not, and if I've missed
> anything important.
> 
> Over the holidays it was raining and I was bored so I was toying with the
> idea of moving some of the thinkpad_acpi functionality out of the file
> into their own modules - the file is a bit of a beast. I'd like to try and
> get any commonality between thinkpad, ideapad, etc where possible.
> My plan was to first look at pulling out the platform_profile pieces and
> then extend to other pieces (fans, temp, sensors, etc).
> 
> Doing this will, potentially, create a number of lenovo_xxx files and so it
> seemed nice to put lenovo stuff in it's own subdirectory (in a similar way
> to other vendors) before starting the exercise.
> 
> This was my attempt to see if it was easy - and it was.
> 
> Please let me know:
> 
> 1) Is this OK to do, or does it cause any problems?

Moving the lenovo drivers and especially removing the duplicate
functionality sounds good to me.

> 2) Have I missed anything important?

I have a few small remarks below, other then that this looks good
to me.

> 3) I don't want to tread on any toes - so if there is protocol to follow
> with moving files please let me know :) (Or a preferred way to do such an
> exercise)

No special protocol for moving files.

> 4) I don't have any ideapads to test with. I think this is low risk, but
> if anybody is able to confirm nothing breaks please let me know.

The moving should definitely be safe. For the refactoring planned on top
would be good if you can test on some actual hw.

> I will see if I can scrounge some HW from somewhere in the meantime.
> 
> I will need to rebase before proposing this officially, so please ignore
> the fact that this is based off 6.7-rc1 and therefore a bit out of date.
> 
> Signed-off-by: Mark Pearson <mpe...@sq...>
> ---
>  .../admin-guide/laptops/thinkpad-acpi.rst     |   3 +-
>  MAINTAINERS                                   |   6 +-
>  drivers/platform/x86/Kconfig                  | 196 +---------------
>  drivers/platform/x86/Makefile                 |  10 +-
>  drivers/platform/x86/lenovo/Kconfig           | 211 ++++++++++++++++++
>  drivers/platform/x86/lenovo/Makefile          |  13 ++
>  .../x86/{ => lenovo}/ideapad-laptop.c         |   0
>  .../x86/{ => lenovo}/ideapad-laptop.h         |   0
>  .../platform/x86/{ => lenovo}/lenovo-ymc.c    |   0
>  .../x86/{ => lenovo}/lenovo-yogabook.c        |   0
>  drivers/platform/x86/{ => lenovo}/think-lmi.c |   2 +-
>  drivers/platform/x86/{ => lenovo}/think-lmi.h |   0
>  .../platform/x86/{ => lenovo}/thinkpad_acpi.c |   2 +-
>  13 files changed, 238 insertions(+), 205 deletions(-)
>  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}/ideapad-laptop.h (100%)
>  rename drivers/platform/x86/{ => lenovo}/lenovo-ymc.c (100%)
>  rename drivers/platform/x86/{ => lenovo}/lenovo-yogabook.c (100%)
>  rename drivers/platform/x86/{ => lenovo}/think-lmi.c (99%)
>  rename drivers/platform/x86/{ => lenovo}/think-lmi.h (100%)
>  rename drivers/platform/x86/{ => lenovo}/thinkpad_acpi.c (99%)
> 
> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> index 98d304010170..55b79ee2bb26 100644
> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> @@ -20,7 +20,8 @@ This driver used to be named ibm-acpi until kernel 2.6.21 and release
>  0.13-20070314.  It used to be in the drivers/acpi tree, but it was
>  moved to the drivers/misc tree and renamed to thinkpad-acpi for kernel
>  2.6.22, and release 0.14.  It was moved to drivers/platform/x86 for
> -kernel 2.6.29 and release 0.22.
> +kernel 2.6.29 and release 0.22. It was moved to drivers/platform/x86/lenovo
> +for kernel 6.8.

The 6.8 merge window just opened so this is only going to land in the
next cycle which will be 6.9 .

>  
>  The driver is named "thinkpad-acpi".  In some places, like module
>  names and log messages, "thinkpad_acpi" is used because of userspace
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 97f51d5ec1cf..c83ed9a51a44 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10243,7 +10243,7 @@ M:	Ike Panhc <ik...@ca...>
>  L:	pla...@vg...
>  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...@gm...>
> @@ -21637,14 +21637,14 @@ 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
>  
>  THINKPAD LMI DRIVER
>  M:	Mark Pearson <mar...@le...>
>  L:	pla...@vg...
>  S:	Maintained
>  F:	Documentation/ABI/testing/sysfs-class-firmware-attributes
> -F:	drivers/platform/x86/think-lmi.?
> +F:	drivers/platform/x86/lenovo/think-lmi.?
>  
>  THUNDERBOLT DMA TRAFFIC TEST DRIVER
>  M:	Isaac Hazan <isa...@in...>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 7e69fdaccdd5..842ced89bd82 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -121,20 +121,6 @@ config GIGABYTE_WMI
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called gigabyte-wmi.
>  
> -config YOGABOOK
> -	tristate "Lenovo Yoga Book tablet key driver"
> -	depends on ACPI_WMI
> -	depends on INPUT
> -	depends on I2C
> -	select LEDS_CLASS
> -	select NEW_LEDS
> -	help
> -	  Say Y here if you want to support the 'Pen' key and keyboard backlight
> -	  control on the Lenovo Yoga Book tablets.
> -
> -	  To compile this driver as a module, choose M here: the module will
> -	  be called lenovo-yogabook.
> -
>  config ACERHDF
>  	tristate "Acer Aspire One temperature and fan driver"
>  	depends on ACPI && THERMAL
> @@ -430,6 +416,7 @@ config WIRELESS_HOTKEY
>  	 To compile this driver as a module, choose M here: the module will
>  	 be called wireless-hotkey.
>  
> +
>  config IBM_RTL
>  	tristate "Device driver to enable PRTL support"
>  	depends on PCI

stray blank line insertion, please drop this.


> @@ -446,31 +433,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
> -	select 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 LENOVO_YMC
> -	tristate "Lenovo Yoga Tablet Mode Control"
> -	depends on ACPI_WMI
> -	depends on INPUT
> -	select INPUT_SPARSEKMAP
> -	help
> -	  This driver maps the Tablet Mode Control switch to SW_TABLET_MODE input
> -	  events for Lenovo Yoga notebooks.
> -
>  config SENSORS_HDAPS
>  	tristate "Thinkpad Hard Drive Active Protection System (hdaps)"
>  	depends on INPUT
> @@ -489,162 +451,10 @@ 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 I2C
> -	depends on DRM
> -	select ACPI_PLATFORM_PROFILE
> -	select DRM_PRIVACY_SCREEN
> -	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.
> -
> -config THINKPAD_LMI
> -	tristate "Lenovo WMI-based systems management driver"
> -	depends on ACPI_WMI
> -	select FW_ATTR_CLASS
> -	help
> -	  This driver allows changing BIOS settings on Lenovo machines whose
> -	  BIOS support the WMI interface.
> -
> -	  To compile this driver as a module, choose M here: the module will
> -	  be called think-lmi.
> -
>  source "drivers/platform/x86/intel/Kconfig"
>  
> +source "drivers/platform/x86/lenovo/Kconfig"
> +
>  config MSI_EC
>  	tristate "MSI EC Extras"
>  	depends on ACPI

Random remark: it would certainly be good if we can reduce the number
of THINKPAD_ACPI related Kconfig symbols ...

> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index c7a18e95ad8c..ccf3610cb34b 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -58,18 +58,16 @@ obj-$(CONFIG_X86_PLATFORM_DRIVERS_HP)	+= hp/
>  # Hewlett Packard Enterprise
>  obj-$(CONFIG_UV_SYSFS)       += uv_sysfs.o
>  
> -# IBM Thinkpad and Lenovo
> +# IBM Thinkpad
>  obj-$(CONFIG_IBM_RTL)		+= ibm_rtl.o
> -obj-$(CONFIG_IDEAPAD_LAPTOP)	+= ideapad-laptop.o
> -obj-$(CONFIG_LENOVO_YMC)	+= lenovo-ymc.o
>  obj-$(CONFIG_SENSORS_HDAPS)	+= hdaps.o
> -obj-$(CONFIG_THINKPAD_ACPI)	+= thinkpad_acpi.o
> -obj-$(CONFIG_THINKPAD_LMI)	+= think-lmi.o
> -obj-$(CONFIG_YOGABOOK)		+= lenovo-yogabook.o
>  
>  # Intel
>  obj-y				+= intel/
>  
> +# Lenovo
> +obj-$(CONFIG_X86_PLATFORM_DRIVERS_LENOVO) += lenovo/
> +
>  # MSI
>  obj-$(CONFIG_MSI_EC)		+= msi-ec.o
>  obj-$(CONFIG_MSI_LAPTOP)	+= msi-laptop.o
> diff --git a/drivers/platform/x86/lenovo/Kconfig b/drivers/platform/x86/lenovo/Kconfig
> new file mode 100644
> index 000000000000..a4de6f5b841d
> --- /dev/null
> +++ b/drivers/platform/x86/lenovo/Kconfig
> @@ -0,0 +1,211 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Lenovo X86 Platform Specific Drivers
> +#
> +
> +menuconfig X86_PLATFORM_DRIVERS_LENOVO
> +	bool "Lenovo X86 Platform Specific Device Drivers"
> +	default y
> +	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 YOGABOOK
> +	tristate "Lenovo Yoga Book tablet key driver"
> +	depends on ACPI_WMI
> +	depends on INPUT
> +	depends on I2C
> +	select LEDS_CLASS
> +	select NEW_LEDS
> +	help
> +	  Say Y here if you want to support the 'Pen' key and keyboard backlight
> +	  control on the Lenovo Yoga Book tablets.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called lenovo-yogabook.
> +
> +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
> +	select 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 LENOVO_YMC
> +	tristate "Lenovo Yoga Tablet Mode Control"
> +	depends on ACPI_WMI
> +	depends on INPUT
> +	select INPUT_SPARSEKMAP
> +	help
> +	  This driver maps the Tablet Mode Control switch to SW_TABLET_MODE input
> +	  events for Lenovo Yoga notebooks.
> +
> +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 I2C
> +	depends on DRM
> +	select ACPI_PLATFORM_PROFILE
> +	select DRM_PRIVACY_SCREEN
> +	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.
> +
> +config THINKPAD_LMI
> +	tristate "Lenovo WMI-based systems management driver"
> +	depends on ACPI_WMI
> +	select FW_ATTR_CLASS
> +	help
> +	  This driver allows changing BIOS settings on Lenovo machines whose
> +	  BIOS support the WMI interface.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called think-lmi.
> +
> +endif # X86_PLATFORM_DRIVERS_LENOVO

Maybe sort the entries alphabetically, putting the 2 yoga drivers at the end ?

I assume all these entries are just moved and no changes wrt
depends / selects have been made ?

> diff --git a/drivers/platform/x86/lenovo/Makefile b/drivers/platform/x86/lenovo/Makefile
> new file mode 100644
> index 000000000000..4f8d6ed369b8
> --- /dev/null
> +++ b/drivers/platform/x86/lenovo/Makefile
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for linux/drivers/platform/x86/lenovo
> +# Lenovo x86 Platform-Specific Drivers
> +#
> +
> +obj-$(CONFIG_IBM_RTL)		+= ibm_rtl.o

This looks wrong, since you are keeping this in the main
drivers/platform/x86/Makefile and Kconfig

> +obj-$(CONFIG_IDEAPAD_LAPTOP)	+= ideapad-laptop.o
> +obj-$(CONFIG_LENOVO_YMC)	+= lenovo-ymc.o
> +obj-$(CONFIG_SENSORS_HDAPS)	+= hdaps.o

same remark for the hdaps driver.

> +obj-$(CONFIG_THINKPAD_ACPI)	+= thinkpad_acpi.o
> +obj-$(CONFIG_THINKPAD_LMI)	+= think-lmi.o
> +obj-$(CONFIG_YOGABOOK)		+= lenovo-yogabook.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/ideapad-laptop.h b/drivers/platform/x86/lenovo/ideapad-laptop.h
> similarity index 100%
> rename from drivers/platform/x86/ideapad-laptop.h
> rename to drivers/platform/x86/lenovo/ideapad-laptop.h
> diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo/lenovo-ymc.c
> similarity index 100%
> rename from drivers/platform/x86/lenovo-ymc.c
> rename to drivers/platform/x86/lenovo/lenovo-ymc.c
> diff --git a/drivers/platform/x86/lenovo-yogabook.c b/drivers/platform/x86/lenovo/lenovo-yogabook.c
> similarity index 100%
> rename from drivers/platform/x86/lenovo-yogabook.c
> rename to drivers/platform/x86/lenovo/lenovo-yogabook.c
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/lenovo/think-lmi.c
> similarity index 99%
> rename from drivers/platform/x86/think-lmi.c
> rename to drivers/platform/x86/lenovo/think-lmi.c
> index 3a396b763c49..bf688df50856 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/lenovo/think-lmi.c
> @@ -19,7 +19,7 @@
>  #include <linux/types.h>
>  #include <linux/dmi.h>
>  #include <linux/wmi.h>
> -#include "firmware_attributes_class.h"
> +#include "../firmware_attributes_class.h"
>  #include "think-lmi.h"
>  
>  static bool debug_support;
> diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/lenovo/think-lmi.h
> similarity index 100%
> rename from drivers/platform/x86/think-lmi.h
> rename to drivers/platform/x86/lenovo/think-lmi.h
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> similarity index 99%
> rename from drivers/platform/x86/thinkpad_acpi.c
> rename to drivers/platform/x86/lenovo/thinkpad_acpi.c
> index d0b5fd4137bc..7d085d4e02ee 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> @@ -80,7 +80,7 @@
>  #include <sound/core.h>
>  #include <sound/initval.h>
>  
> -#include "dual_accel_detect.h"
> +#include "../dual_accel_detect.h"
>  
>  /* ThinkPad CMOS commands */
>  #define TP_CMOS_VOLUME_DOWN	0

Regards,

Hans

[ibm-acpi-devel] [PATCH RFC] platform/x86: Lenovo sub directory

[Mark P. <mpe...@sq...>]

Looking for feedback if this is a good idea or not, and if I've missed
anything important.

Over the holidays it was raining and I was bored so I was toying with the
idea of moving some of the thinkpad_acpi functionality out of the file
into their own modules - the file is a bit of a beast. I'd like to try and
get any commonality between thinkpad, ideapad, etc where possible.
My plan was to first look at pulling out the platform_profile pieces and
then extend to other pieces (fans, temp, sensors, etc).

Doing this will, potentially, create a number of lenovo_xxx files and so it
seemed nice to put lenovo stuff in it's own subdirectory (in a similar way
to other vendors) before starting the exercise.

This was my attempt to see if it was easy - and it was.

Please let me know:

1) Is this OK to do, or does it cause any problems?
2) Have I missed anything important?
3) I don't want to tread on any toes - so if there is protocol to follow
with moving files please let me know :) (Or a preferred way to do such an
exercise)
4) I don't have any ideapads to test with. I think this is low risk, but
if anybody is able to confirm nothing breaks please let me know.
I will see if I can scrounge some HW from somewhere in the meantime.

I will need to rebase before proposing this officially, so please ignore
the fact that this is based off 6.7-rc1 and therefore a bit out of date.

Signed-off-by: Mark Pearson <mpe...@sq...>
---
 .../admin-guide/laptops/thinkpad-acpi.rst     |   3 +-
 MAINTAINERS                                   |   6 +-
 drivers/platform/x86/Kconfig                  | 196 +---------------
 drivers/platform/x86/Makefile                 |  10 +-
 drivers/platform/x86/lenovo/Kconfig           | 211 ++++++++++++++++++
 drivers/platform/x86/lenovo/Makefile          |  13 ++
 .../x86/{ => lenovo}/ideapad-laptop.c         |   0
 .../x86/{ => lenovo}/ideapad-laptop.h         |   0
 .../platform/x86/{ => lenovo}/lenovo-ymc.c    |   0
 .../x86/{ => lenovo}/lenovo-yogabook.c        |   0
 drivers/platform/x86/{ => lenovo}/think-lmi.c |   2 +-
 drivers/platform/x86/{ => lenovo}/think-lmi.h |   0
 .../platform/x86/{ => lenovo}/thinkpad_acpi.c |   2 +-
 13 files changed, 238 insertions(+), 205 deletions(-)
 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}/ideapad-laptop.h (100%)
 rename drivers/platform/x86/{ => lenovo}/lenovo-ymc.c (100%)
 rename drivers/platform/x86/{ => lenovo}/lenovo-yogabook.c (100%)
 rename drivers/platform/x86/{ => lenovo}/think-lmi.c (99%)
 rename drivers/platform/x86/{ => lenovo}/think-lmi.h (100%)
 rename drivers/platform/x86/{ => lenovo}/thinkpad_acpi.c (99%)

diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
index 98d304010170..55b79ee2bb26 100644
--- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
+++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
@@ -20,7 +20,8 @@ This driver used to be named ibm-acpi until kernel 2.6.21 and release
 0.13-20070314.  It used to be in the drivers/acpi tree, but it was
 moved to the drivers/misc tree and renamed to thinkpad-acpi for kernel
 2.6.22, and release 0.14.  It was moved to drivers/platform/x86 for
-kernel 2.6.29 and release 0.22.
+kernel 2.6.29 and release 0.22. It was moved to drivers/platform/x86/lenovo
+for kernel 6.8.
 
 The driver is named "thinkpad-acpi".  In some places, like module
 names and log messages, "thinkpad_acpi" is used because of userspace
diff --git a/MAINTAINERS b/MAINTAINERS
index 97f51d5ec1cf..c83ed9a51a44 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10243,7 +10243,7 @@ M:	Ike Panhc <ik...@ca...>
 L:	pla...@vg...
 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...@gm...>
@@ -21637,14 +21637,14 @@ 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
 
 THINKPAD LMI DRIVER
 M:	Mark Pearson <mar...@le...>
 L:	pla...@vg...
 S:	Maintained
 F:	Documentation/ABI/testing/sysfs-class-firmware-attributes
-F:	drivers/platform/x86/think-lmi.?
+F:	drivers/platform/x86/lenovo/think-lmi.?
 
 THUNDERBOLT DMA TRAFFIC TEST DRIVER
 M:	Isaac Hazan <isa...@in...>
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 7e69fdaccdd5..842ced89bd82 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -121,20 +121,6 @@ config GIGABYTE_WMI
 	  To compile this driver as a module, choose M here: the module will
 	  be called gigabyte-wmi.
 
-config YOGABOOK
-	tristate "Lenovo Yoga Book tablet key driver"
-	depends on ACPI_WMI
-	depends on INPUT
-	depends on I2C
-	select LEDS_CLASS
-	select NEW_LEDS
-	help
-	  Say Y here if you want to support the 'Pen' key and keyboard backlight
-	  control on the Lenovo Yoga Book tablets.
-
-	  To compile this driver as a module, choose M here: the module will
-	  be called lenovo-yogabook.
-
 config ACERHDF
 	tristate "Acer Aspire One temperature and fan driver"
 	depends on ACPI && THERMAL
@@ -430,6 +416,7 @@ config WIRELESS_HOTKEY
 	 To compile this driver as a module, choose M here: the module will
 	 be called wireless-hotkey.
 
+
 config IBM_RTL
 	tristate "Device driver to enable PRTL support"
 	depends on PCI
@@ -446,31 +433,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
-	select 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 LENOVO_YMC
-	tristate "Lenovo Yoga Tablet Mode Control"
-	depends on ACPI_WMI
-	depends on INPUT
-	select INPUT_SPARSEKMAP
-	help
-	  This driver maps the Tablet Mode Control switch to SW_TABLET_MODE input
-	  events for Lenovo Yoga notebooks.
-
 config SENSORS_HDAPS
 	tristate "Thinkpad Hard Drive Active Protection System (hdaps)"
 	depends on INPUT
@@ -489,162 +451,10 @@ 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 I2C
-	depends on DRM
-	select ACPI_PLATFORM_PROFILE
-	select DRM_PRIVACY_SCREEN
-	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.
-
-config THINKPAD_LMI
-	tristate "Lenovo WMI-based systems management driver"
-	depends on ACPI_WMI
-	select FW_ATTR_CLASS
-	help
-	  This driver allows changing BIOS settings on Lenovo machines whose
-	  BIOS support the WMI interface.
-
-	  To compile this driver as a module, choose M here: the module will
-	  be called think-lmi.
-
 source "drivers/platform/x86/intel/Kconfig"
 
+source "drivers/platform/x86/lenovo/Kconfig"
+
 config MSI_EC
 	tristate "MSI EC Extras"
 	depends on ACPI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index c7a18e95ad8c..ccf3610cb34b 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -58,18 +58,16 @@ obj-$(CONFIG_X86_PLATFORM_DRIVERS_HP)	+= hp/
 # Hewlett Packard Enterprise
 obj-$(CONFIG_UV_SYSFS)       += uv_sysfs.o
 
-# IBM Thinkpad and Lenovo
+# IBM Thinkpad
 obj-$(CONFIG_IBM_RTL)		+= ibm_rtl.o
-obj-$(CONFIG_IDEAPAD_LAPTOP)	+= ideapad-laptop.o
-obj-$(CONFIG_LENOVO_YMC)	+= lenovo-ymc.o
 obj-$(CONFIG_SENSORS_HDAPS)	+= hdaps.o
-obj-$(CONFIG_THINKPAD_ACPI)	+= thinkpad_acpi.o
-obj-$(CONFIG_THINKPAD_LMI)	+= think-lmi.o
-obj-$(CONFIG_YOGABOOK)		+= lenovo-yogabook.o
 
 # Intel
 obj-y				+= intel/
 
+# Lenovo
+obj-$(CONFIG_X86_PLATFORM_DRIVERS_LENOVO) += lenovo/
+
 # MSI
 obj-$(CONFIG_MSI_EC)		+= msi-ec.o
 obj-$(CONFIG_MSI_LAPTOP)	+= msi-laptop.o
diff --git a/drivers/platform/x86/lenovo/Kconfig b/drivers/platform/x86/lenovo/Kconfig
new file mode 100644
index 000000000000..a4de6f5b841d
--- /dev/null
+++ b/drivers/platform/x86/lenovo/Kconfig
@@ -0,0 +1,211 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Lenovo X86 Platform Specific Drivers
+#
+
+menuconfig X86_PLATFORM_DRIVERS_LENOVO
+	bool "Lenovo X86 Platform Specific Device Drivers"
+	default y
+	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 YOGABOOK
+	tristate "Lenovo Yoga Book tablet key driver"
+	depends on ACPI_WMI
+	depends on INPUT
+	depends on I2C
+	select LEDS_CLASS
+	select NEW_LEDS
+	help
+	  Say Y here if you want to support the 'Pen' key and keyboard backlight
+	  control on the Lenovo Yoga Book tablets.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called lenovo-yogabook.
+
+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
+	select 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 LENOVO_YMC
+	tristate "Lenovo Yoga Tablet Mode Control"
+	depends on ACPI_WMI
+	depends on INPUT
+	select INPUT_SPARSEKMAP
+	help
+	  This driver maps the Tablet Mode Control switch to SW_TABLET_MODE input
+	  events for Lenovo Yoga notebooks.
+
+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 I2C
+	depends on DRM
+	select ACPI_PLATFORM_PROFILE
+	select DRM_PRIVACY_SCREEN
+	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.
+
+config THINKPAD_LMI
+	tristate "Lenovo WMI-based systems management driver"
+	depends on ACPI_WMI
+	select FW_ATTR_CLASS
+	help
+	  This driver allows changing BIOS settings on Lenovo machines whose
+	  BIOS support the WMI interface.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called think-lmi.
+
+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..4f8d6ed369b8
--- /dev/null
+++ b/drivers/platform/x86/lenovo/Makefile
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for linux/drivers/platform/x86/lenovo
+# Lenovo x86 Platform-Specific Drivers
+#
+
+obj-$(CONFIG_IBM_RTL)		+= ibm_rtl.o
+obj-$(CONFIG_IDEAPAD_LAPTOP)	+= ideapad-laptop.o
+obj-$(CONFIG_LENOVO_YMC)	+= lenovo-ymc.o
+obj-$(CONFIG_SENSORS_HDAPS)	+= hdaps.o
+obj-$(CONFIG_THINKPAD_ACPI)	+= thinkpad_acpi.o
+obj-$(CONFIG_THINKPAD_LMI)	+= think-lmi.o
+obj-$(CONFIG_YOGABOOK)		+= lenovo-yogabook.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/ideapad-laptop.h b/drivers/platform/x86/lenovo/ideapad-laptop.h
similarity index 100%
rename from drivers/platform/x86/ideapad-laptop.h
rename to drivers/platform/x86/lenovo/ideapad-laptop.h
diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo/lenovo-ymc.c
similarity index 100%
rename from drivers/platform/x86/lenovo-ymc.c
rename to drivers/platform/x86/lenovo/lenovo-ymc.c
diff --git a/drivers/platform/x86/lenovo-yogabook.c b/drivers/platform/x86/lenovo/lenovo-yogabook.c
similarity index 100%
rename from drivers/platform/x86/lenovo-yogabook.c
rename to drivers/platform/x86/lenovo/lenovo-yogabook.c
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/lenovo/think-lmi.c
similarity index 99%
rename from drivers/platform/x86/think-lmi.c
rename to drivers/platform/x86/lenovo/think-lmi.c
index 3a396b763c49..bf688df50856 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/lenovo/think-lmi.c
@@ -19,7 +19,7 @@
 #include <linux/types.h>
 #include <linux/dmi.h>
 #include <linux/wmi.h>
-#include "firmware_attributes_class.h"
+#include "../firmware_attributes_class.h"
 #include "think-lmi.h"
 
 static bool debug_support;
diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/lenovo/think-lmi.h
similarity index 100%
rename from drivers/platform/x86/think-lmi.h
rename to drivers/platform/x86/lenovo/think-lmi.h
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
similarity index 99%
rename from drivers/platform/x86/thinkpad_acpi.c
rename to drivers/platform/x86/lenovo/thinkpad_acpi.c
index d0b5fd4137bc..7d085d4e02ee 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
@@ -80,7 +80,7 @@
 #include <sound/core.h>
 #include <sound/initval.h>
 
-#include "dual_accel_detect.h"
+#include "../dual_accel_detect.h"
 
 /* ThinkPad CMOS commands */
 #define TP_CMOS_VOLUME_DOWN	0
-- 
2.43.0

Re: [ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: fix kernel-doc warnings

[Mark P. <mpe...@sq...>]

On Wed, Dec 6, 2023, at 11:45 AM, Randy Dunlap wrote:
> Hi Mark,
>
> On 12/6/23 07:30, Mark Pearson wrote:
>> Hi Randy
>> 
>> On Wed, Dec 6, 2023, at 1:01 AM, Randy Dunlap wrote:
>>> Add a function's return description and don't misuse "/**" for
>>> non-kernel-doc comments to prevent warnings from scripts/kernel-doc.
>>>
>>> thinkpad_acpi.c:523: warning: No description found for return value of 
>>> 'tpacpi_check_quirks'
>>> thinkpad_acpi.c:9307: warning: This comment starts with '/**', but 
>>> isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
>>> thinkpad_acpi.c:9307: warning: missing initial short description on 
>>> line:
>>>  * This evaluates a ACPI method call specific to the battery
>>>
>>> Signed-off-by: Randy Dunlap <rd...@in...>
>>> Cc: Henrique de Moraes Holschuh <hm...@hm...>
>>> Cc: Hans de Goede <hde...@re...>
>>> Cc: "Ilpo Järvinen" <ilp...@li...>
>>> CC: ibm...@li...
>>> CC: pla...@vg...
>>> ---
>>>  drivers/platform/x86/thinkpad_acpi.c |    6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff -- a/drivers/platform/x86/thinkpad_acpi.c 
>>> b/drivers/platform/x86/thinkpad_acpi.c
>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>> @@ -512,10 +512,10 @@ struct tpacpi_quirk {
>>>   * Iterates over a quirks list until one is found that matches the
>>>   * ThinkPad's vendor, BIOS and EC model.
>>>   *
>>> - * Returns 0 if nothing matches, otherwise returns the quirks field of
>>> + * Returns: %0 if nothing matches, otherwise returns the quirks field 
>> 
>> Just being nosy - what does %0 do?
>> I assume it helps with the return value but was intrigued how it is used and where
>
> It causes the output to be formatted as a CONSTANT (probably monospaced font,
> but no guarantees on that).
Ah - cool. Thanks!
>
>> 
>>> of
>>>   * the matching &struct tpacpi_quirk entry.
>>>   *
>>> - * The match criteria is: vendor, ec and bios much match.
>>> + * The match criteria is: vendor, ec and bios must match.
>> I can't for the life of me see what is different here. What am I missing? 
>
> s/much/must/
Man....I need to go to the opticians :)

>
>> 
>>>   */
>>>  static unsigned long __init tpacpi_check_quirks(
>>>  			const struct tpacpi_quirk *qlist,
>>> @@ -9303,7 +9303,7 @@ static struct tpacpi_battery_driver_data
>>>
>>>  /* ACPI helpers/functions/probes */
>>>
>>> -/**
>>> +/*
>>>   * This evaluates a ACPI method call specific to the battery
>>>   * ACPI extension. The specifics are that an error is marked
>>>   * in the 32rd bit of the response, so we just check that here.
>>>
>>>
>>> _______________________________________________
>>> ibm-acpi-devel mailing list
>>> ibm...@li...
>>> https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
>> 
>> Thanks
>> Mark
>
> -- 
> ~Randy

Looks good to me! 
Reviewed-by: mpe...@sq...

Mark

Re: [ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: fix kernel-doc warnings

[Randy D. <rd...@in...>]

Hi Mark,

On 12/6/23 07:30, Mark Pearson wrote:
> Hi Randy
> 
> On Wed, Dec 6, 2023, at 1:01 AM, Randy Dunlap wrote:
>> Add a function's return description and don't misuse "/**" for
>> non-kernel-doc comments to prevent warnings from scripts/kernel-doc.
>>
>> thinkpad_acpi.c:523: warning: No description found for return value of 
>> 'tpacpi_check_quirks'
>> thinkpad_acpi.c:9307: warning: This comment starts with '/**', but 
>> isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
>> thinkpad_acpi.c:9307: warning: missing initial short description on 
>> line:
>>  * This evaluates a ACPI method call specific to the battery
>>
>> Signed-off-by: Randy Dunlap <rd...@in...>
>> Cc: Henrique de Moraes Holschuh <hm...@hm...>
>> Cc: Hans de Goede <hde...@re...>
>> Cc: "Ilpo Järvinen" <ilp...@li...>
>> CC: ibm...@li...
>> CC: pla...@vg...
>> ---
>>  drivers/platform/x86/thinkpad_acpi.c |    6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff -- a/drivers/platform/x86/thinkpad_acpi.c 
>> b/drivers/platform/x86/thinkpad_acpi.c
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -512,10 +512,10 @@ struct tpacpi_quirk {
>>   * Iterates over a quirks list until one is found that matches the
>>   * ThinkPad's vendor, BIOS and EC model.
>>   *
>> - * Returns 0 if nothing matches, otherwise returns the quirks field of
>> + * Returns: %0 if nothing matches, otherwise returns the quirks field 
> 
> Just being nosy - what does %0 do?
> I assume it helps with the return value but was intrigued how it is used and where

It causes the output to be formatted as a CONSTANT (probably monospaced font,
but no guarantees on that).

> 
>> of
>>   * the matching &struct tpacpi_quirk entry.
>>   *
>> - * The match criteria is: vendor, ec and bios much match.
>> + * The match criteria is: vendor, ec and bios must match.
> I can't for the life of me see what is different here. What am I missing? 

s/much/must/

> 
>>   */
>>  static unsigned long __init tpacpi_check_quirks(
>>  			const struct tpacpi_quirk *qlist,
>> @@ -9303,7 +9303,7 @@ static struct tpacpi_battery_driver_data
>>
>>  /* ACPI helpers/functions/probes */
>>
>> -/**
>> +/*
>>   * This evaluates a ACPI method call specific to the battery
>>   * ACPI extension. The specifics are that an error is marked
>>   * in the 32rd bit of the response, so we just check that here.
>>
>>
>> _______________________________________________
>> ibm-acpi-devel mailing list
>> ibm...@li...
>> https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
> 
> Thanks
> Mark

-- 
~Randy

Re: [ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: fix kernel-doc warnings

[Mark P. <mpe...@sq...>]

Hi Randy

On Wed, Dec 6, 2023, at 1:01 AM, Randy Dunlap wrote:
> Add a function's return description and don't misuse "/**" for
> non-kernel-doc comments to prevent warnings from scripts/kernel-doc.
>
> thinkpad_acpi.c:523: warning: No description found for return value of 
> 'tpacpi_check_quirks'
> thinkpad_acpi.c:9307: warning: This comment starts with '/**', but 
> isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
> thinkpad_acpi.c:9307: warning: missing initial short description on 
> line:
>  * This evaluates a ACPI method call specific to the battery
>
> Signed-off-by: Randy Dunlap <rd...@in...>
> Cc: Henrique de Moraes Holschuh <hm...@hm...>
> Cc: Hans de Goede <hde...@re...>
> Cc: "Ilpo Järvinen" <ilp...@li...>
> CC: ibm...@li...
> CC: pla...@vg...
> ---
>  drivers/platform/x86/thinkpad_acpi.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff -- a/drivers/platform/x86/thinkpad_acpi.c 
> b/drivers/platform/x86/thinkpad_acpi.c
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -512,10 +512,10 @@ struct tpacpi_quirk {
>   * Iterates over a quirks list until one is found that matches the
>   * ThinkPad's vendor, BIOS and EC model.
>   *
> - * Returns 0 if nothing matches, otherwise returns the quirks field of
> + * Returns: %0 if nothing matches, otherwise returns the quirks field 

Just being nosy - what does %0 do?
I assume it helps with the return value but was intrigued how it is used and where

> of
>   * the matching &struct tpacpi_quirk entry.
>   *
> - * The match criteria is: vendor, ec and bios much match.
> + * The match criteria is: vendor, ec and bios must match.
I can't for the life of me see what is different here. What am I missing? 

>   */
>  static unsigned long __init tpacpi_check_quirks(
>  			const struct tpacpi_quirk *qlist,
> @@ -9303,7 +9303,7 @@ static struct tpacpi_battery_driver_data
> 
>  /* ACPI helpers/functions/probes */
> 
> -/**
> +/*
>   * This evaluates a ACPI method call specific to the battery
>   * ACPI extension. The specifics are that an error is marked
>   * in the 32rd bit of the response, so we just check that here.
>
>
> _______________________________________________
> ibm-acpi-devel mailing list
> ibm...@li...
> https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Thanks
Mark

[ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: fix kernel-doc warnings

[Randy D. <rd...@in...>]

Add a function's return description and don't misuse "/**" for
non-kernel-doc comments to prevent warnings from scripts/kernel-doc.

thinkpad_acpi.c:523: warning: No description found for return value of 'tpacpi_check_quirks'
thinkpad_acpi.c:9307: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
thinkpad_acpi.c:9307: warning: missing initial short description on line:
 * This evaluates a ACPI method call specific to the battery

Signed-off-by: Randy Dunlap <rd...@in...>
Cc: Henrique de Moraes Holschuh <hm...@hm...>
Cc: Hans de Goede <hde...@re...>
Cc: "Ilpo Järvinen" <ilp...@li...>
CC: ibm...@li...
CC: pla...@vg...
---
 drivers/platform/x86/thinkpad_acpi.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff -- a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -512,10 +512,10 @@ struct tpacpi_quirk {
  * Iterates over a quirks list until one is found that matches the
  * ThinkPad's vendor, BIOS and EC model.
  *
- * Returns 0 if nothing matches, otherwise returns the quirks field of
+ * Returns: %0 if nothing matches, otherwise returns the quirks field of
  * the matching &struct tpacpi_quirk entry.
  *
- * The match criteria is: vendor, ec and bios much match.
+ * The match criteria is: vendor, ec and bios must match.
  */
 static unsigned long __init tpacpi_check_quirks(
 			const struct tpacpi_quirk *qlist,
@@ -9303,7 +9303,7 @@ static struct tpacpi_battery_driver_data
 
 /* ACPI helpers/functions/probes */
 
-/**
+/*
  * This evaluates a ACPI method call specific to the battery
  * ACPI extension. The specifics are that an error is marked
  * in the 32rd bit of the response, so we just check that here.

Re: [ibm-acpi-devel] [PATCH v3] platform/x86: Add support for improved performance mode

[Mark P. <mpe...@sq...>]

Hi Breno,

Thanks for the review!

On Wed, Nov 22, 2023, at 2:44 PM, Breno Leitao wrote:
> On Mon, Nov 13, 2023 at 11:54:33AM -0500, Mark Pearson wrote:
>> @@ -10355,6 +10361,17 @@ static int dytc_profile_set(struct platform_profile_handler *pprof,
>>  	if (err)
>>  		goto unlock;
>>  
>> +	/* Set TMS mode appropriately (enable for performance), if available */
>> +	if (dytc_ultraperf_cap) {
>> +		int cmd;
>> +
>> +		cmd = DYTC_SET_COMMAND(DYTC_FUNCTION_TMS, DYTC_NOMODE,
>> +				       profile == PLATFORM_PROFILE_PERFORMANCE);
>> +		err = dytc_command(cmd, &output);
>> +		if (err)
>> +			return err;
>
> Aren't you returning holding the 'dytc_mutex' mutex?
>
> From what I understand, in the first line of this function you get the lock,
> and release later, at the exit, so, returning without releasing the lock might
> be dangerous. Here is a summary of how I read this function with your change:
>
>
> 	mutex_lock_interruptible(&dytc_mutex);
> 	...
> 	err = dytc_command(cmd, &output);
> 	if (err)
> 		return err;
>
> unlock:
> 	mutex_unlock(&dytc_mutex);
> 	return err;
>
>
> I think "goto unlock" might solve it.

Yep - you're right. Good catch. 
Will fix in the next revision.

Thank you
Mark

Re: [ibm-acpi-devel] [PATCH v3] platform/x86: Add support for improved performance mode

[Mark P. <mpe...@sq...>]

On Mon, Nov 13, 2023, at 12:15 PM, Mark Pearson wrote:
> Thanks Ilpo,
>
> On Mon, Nov 13, 2023, at 11:59 AM, Ilpo Järvinen wrote:
>> On Mon, 13 Nov 2023, Mark Pearson wrote:
>>
>>> Some new Thinkpads have a new improved performance mode available.
>>> Add support to make this mode usable.
>>> 
>>> To avoid having to create a new profile, just use the improved performance
>>> mode in place of the existing performance mode, when available.
>>> 
>>> Tested on P14s AMD G4 AMD.
>>> 
>>> Signed-off-by: Mark Pearson <mpe...@sq...>
>>> ---
>>> Changes in v2: updated implementation for DYTC_UP_SUPPORT define
>>> Changes in v3: 
>>>  - Add in missing BIT for define, somehow lost in previous commit
>>>  - Cosmetic clean-ups
>>> 
>>>  drivers/platform/x86/thinkpad_acpi.c | 28 ++++++++++++++++++++++++++++
>>>  1 file changed, 28 insertions(+)
>>> 
>>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>>> index ad460417f901..3a9d2cc71b6a 100644
>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>> @@ -10136,6 +10136,7 @@ static struct ibm_struct proxsensor_driver_data = {
>>>  
>>>  #define DYTC_CMD_SET          1 /* To enable/disable IC function mode */
>>>  #define DYTC_CMD_MMC_GET      8 /* To get current MMC function and mode */
>>> +#define DYTC_CMD_UP_CAP     0xA /* To get Ultra-performance capability */
>>>  #define DYTC_CMD_RESET    0x1ff /* To reset back to default */
>>>  
>>>  #define DYTC_CMD_FUNC_CAP     3 /* To get DYTC capabilities */
>>> @@ -10152,6 +10153,7 @@ static struct ibm_struct proxsensor_driver_data = {
>>>  
>>>  #define DYTC_FUNCTION_STD     0  /* Function = 0, standard mode */
>>>  #define DYTC_FUNCTION_CQL     1  /* Function = 1, lap mode */
>>> +#define DYTC_FUNCTION_TMS     9  /* Function = 9, TMS mode */
>>>  #define DYTC_FUNCTION_MMC     11 /* Function = 11, MMC mode */
>>>  #define DYTC_FUNCTION_PSC     13 /* Function = 13, PSC mode */
>>>  #define DYTC_FUNCTION_AMT     15 /* Function = 15, AMT mode */
>>> @@ -10163,11 +10165,14 @@ static struct ibm_struct proxsensor_driver_data = {
>>>  #define DYTC_MODE_MMC_LOWPOWER 3  /* Low power mode */
>>>  #define DYTC_MODE_MMC_BALANCE  0xF  /* Default mode aka balanced */
>>>  #define DYTC_MODE_MMC_DEFAULT  0  /* Default mode from MMC_GET, aka balanced */
>>> +#define DYTC_NOMODE            0xF  /* When Function does not have a mode */
>>>  
>>>  #define DYTC_MODE_PSC_LOWPOWER 3  /* Low power mode */
>>>  #define DYTC_MODE_PSC_BALANCE  5  /* Default mode aka balanced */
>>>  #define DYTC_MODE_PSC_PERFORM  7  /* High power mode aka performance */
>>>  
>>> +#define DYTC_UP_SUPPORT    BIT(8)  /* Ultra-performance (TMS) mode support */
>>> +
>>>  #define DYTC_ERR_MASK       0xF  /* Bits 0-3 in cmd result are the error result */
>>>  #define DYTC_ERR_SUCCESS      1  /* CMD completed successful */
>>>  
>>> @@ -10185,6 +10190,7 @@ static enum platform_profile_option dytc_current_profile;
>>>  static atomic_t dytc_ignore_event = ATOMIC_INIT(0);
>>>  static DEFINE_MUTEX(dytc_mutex);
>>>  static int dytc_capabilities;
>>> +static bool dytc_ultraperf_cap;
>>>  static bool dytc_mmc_get_available;
>>>  static int profile_force;
>>>  
>>> @@ -10355,6 +10361,17 @@ static int dytc_profile_set(struct platform_profile_handler *pprof,
>>>  	if (err)
>>>  		goto unlock;
>>>  
>>> +	/* Set TMS mode appropriately (enable for performance), if available */
>>> +	if (dytc_ultraperf_cap) {
>>> +		int cmd;
>>> +
>>> +		cmd = DYTC_SET_COMMAND(DYTC_FUNCTION_TMS, DYTC_NOMODE,
>>> +				       profile == PLATFORM_PROFILE_PERFORMANCE);
>>> +		err = dytc_command(cmd, &output);
>>> +		if (err)
>>> +			return err;
>>> +	}
>>> +
>>>  	if (dytc_capabilities & BIT(DYTC_FC_MMC)) {
>>>  		if (profile == PLATFORM_PROFILE_BALANCED) {
>>>  			/*
>>> @@ -10429,6 +10446,7 @@ static struct platform_profile_handler dytc_profile = {
>>>  static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
>>>  {
>>>  	int err, output;
>>> +	int cmd;
>>>  
>>>  	/* Setup supported modes */
>>>  	set_bit(PLATFORM_PROFILE_LOW_POWER, dytc_profile.choices);
>>> @@ -10484,6 +10502,16 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
>>>  		dbg_printk(TPACPI_DBG_INIT, "No DYTC support available\n");
>>>  		return -ENODEV;
>>>  	}
>>> +	err = dytc_command(DYTC_CMD_UP_CAP, &output);
>>
>> Hmm, are you missing error handling here?
>>
> Doh....yes, it should check that. My bad.
>
> I'll hold off a day or two on the next patch so I'm not spamming the 
> list in case there is other feedback, and do a fix for that. 
> I don't want the driver to error out on this - but it shouldn't check 
> (and potentially enable) the feature if that register read fails. I 
> will go and double check on some older platforms too as a sanity check.
>
> Thanks for the review.

Just an update, as it's taking me longer to get what I thought would be the final version of this patch done.

I did some regression testing on other platforms, as a sanity check, and found on the Z16 G1 that it is reporting this feature as supported, but unfortunately it doesn't work (it switches to low power mode)

This is a FW issue so I'm asking the FW team for clarification on why I'm seeing this, why it isn't working, and how to fix it. 
This is going to take some time unfortunately, so this patch will be delayed a bit. Apologies to anybody waiting for this improvement.

Thanks
Mark

Re: [ibm-acpi-devel] [PATCH v3] platform/x86: Add support for improved performance mode

[Mark P. <mpe...@sq...>]

Thanks Ilpo,

On Mon, Nov 13, 2023, at 11:59 AM, Ilpo Järvinen wrote:
> On Mon, 13 Nov 2023, Mark Pearson wrote:
>
>> Some new Thinkpads have a new improved performance mode available.
>> Add support to make this mode usable.
>> 
>> To avoid having to create a new profile, just use the improved performance
>> mode in place of the existing performance mode, when available.
>> 
>> Tested on P14s AMD G4 AMD.
>> 
>> Signed-off-by: Mark Pearson <mpe...@sq...>
>> ---
>> Changes in v2: updated implementation for DYTC_UP_SUPPORT define
>> Changes in v3: 
>>  - Add in missing BIT for define, somehow lost in previous commit
>>  - Cosmetic clean-ups
>> 
>>  drivers/platform/x86/thinkpad_acpi.c | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>> 
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index ad460417f901..3a9d2cc71b6a 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -10136,6 +10136,7 @@ static struct ibm_struct proxsensor_driver_data = {
>>  
>>  #define DYTC_CMD_SET          1 /* To enable/disable IC function mode */
>>  #define DYTC_CMD_MMC_GET      8 /* To get current MMC function and mode */
>> +#define DYTC_CMD_UP_CAP     0xA /* To get Ultra-performance capability */
>>  #define DYTC_CMD_RESET    0x1ff /* To reset back to default */
>>  
>>  #define DYTC_CMD_FUNC_CAP     3 /* To get DYTC capabilities */
>> @@ -10152,6 +10153,7 @@ static struct ibm_struct proxsensor_driver_data = {
>>  
>>  #define DYTC_FUNCTION_STD     0  /* Function = 0, standard mode */
>>  #define DYTC_FUNCTION_CQL     1  /* Function = 1, lap mode */
>> +#define DYTC_FUNCTION_TMS     9  /* Function = 9, TMS mode */
>>  #define DYTC_FUNCTION_MMC     11 /* Function = 11, MMC mode */
>>  #define DYTC_FUNCTION_PSC     13 /* Function = 13, PSC mode */
>>  #define DYTC_FUNCTION_AMT     15 /* Function = 15, AMT mode */
>> @@ -10163,11 +10165,14 @@ static struct ibm_struct proxsensor_driver_data = {
>>  #define DYTC_MODE_MMC_LOWPOWER 3  /* Low power mode */
>>  #define DYTC_MODE_MMC_BALANCE  0xF  /* Default mode aka balanced */
>>  #define DYTC_MODE_MMC_DEFAULT  0  /* Default mode from MMC_GET, aka balanced */
>> +#define DYTC_NOMODE            0xF  /* When Function does not have a mode */
>>  
>>  #define DYTC_MODE_PSC_LOWPOWER 3  /* Low power mode */
>>  #define DYTC_MODE_PSC_BALANCE  5  /* Default mode aka balanced */
>>  #define DYTC_MODE_PSC_PERFORM  7  /* High power mode aka performance */
>>  
>> +#define DYTC_UP_SUPPORT    BIT(8)  /* Ultra-performance (TMS) mode support */
>> +
>>  #define DYTC_ERR_MASK       0xF  /* Bits 0-3 in cmd result are the error result */
>>  #define DYTC_ERR_SUCCESS      1  /* CMD completed successful */
>>  
>> @@ -10185,6 +10190,7 @@ static enum platform_profile_option dytc_current_profile;
>>  static atomic_t dytc_ignore_event = ATOMIC_INIT(0);
>>  static DEFINE_MUTEX(dytc_mutex);
>>  static int dytc_capabilities;
>> +static bool dytc_ultraperf_cap;
>>  static bool dytc_mmc_get_available;
>>  static int profile_force;
>>  
>> @@ -10355,6 +10361,17 @@ static int dytc_profile_set(struct platform_profile_handler *pprof,
>>  	if (err)
>>  		goto unlock;
>>  
>> +	/* Set TMS mode appropriately (enable for performance), if available */
>> +	if (dytc_ultraperf_cap) {
>> +		int cmd;
>> +
>> +		cmd = DYTC_SET_COMMAND(DYTC_FUNCTION_TMS, DYTC_NOMODE,
>> +				       profile == PLATFORM_PROFILE_PERFORMANCE);
>> +		err = dytc_command(cmd, &output);
>> +		if (err)
>> +			return err;
>> +	}
>> +
>>  	if (dytc_capabilities & BIT(DYTC_FC_MMC)) {
>>  		if (profile == PLATFORM_PROFILE_BALANCED) {
>>  			/*
>> @@ -10429,6 +10446,7 @@ static struct platform_profile_handler dytc_profile = {
>>  static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
>>  {
>>  	int err, output;
>> +	int cmd;
>>  
>>  	/* Setup supported modes */
>>  	set_bit(PLATFORM_PROFILE_LOW_POWER, dytc_profile.choices);
>> @@ -10484,6 +10502,16 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
>>  		dbg_printk(TPACPI_DBG_INIT, "No DYTC support available\n");
>>  		return -ENODEV;
>>  	}
>> +	err = dytc_command(DYTC_CMD_UP_CAP, &output);
>
> Hmm, are you missing error handling here?
>
Doh....yes, it should check that. My bad.

I'll hold off a day or two on the next patch so I'm not spamming the list in case there is other feedback, and do a fix for that. 
I don't want the driver to error out on this - but it shouldn't check (and potentially enable) the feature if that register read fails. I will go and double check on some older platforms too as a sanity check.

Thanks for the review.
Mark

[ibm-acpi-devel] [PATCH v3] platform/x86: Add support for improved performance mode

[Mark P. <mpe...@sq...>]

Some new Thinkpads have a new improved performance mode available.
Add support to make this mode usable.

To avoid having to create a new profile, just use the improved performance
mode in place of the existing performance mode, when available.

Tested on P14s AMD G4 AMD.

Signed-off-by: Mark Pearson <mpe...@sq...>
---
Changes in v2: updated implementation for DYTC_UP_SUPPORT define
Changes in v3: 
 - Add in missing BIT for define, somehow lost in previous commit
 - Cosmetic clean-ups

 drivers/platform/x86/thinkpad_acpi.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index ad460417f901..3a9d2cc71b6a 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -10136,6 +10136,7 @@ static struct ibm_struct proxsensor_driver_data = {
 
 #define DYTC_CMD_SET          1 /* To enable/disable IC function mode */
 #define DYTC_CMD_MMC_GET      8 /* To get current MMC function and mode */
+#define DYTC_CMD_UP_CAP     0xA /* To get Ultra-performance capability */
 #define DYTC_CMD_RESET    0x1ff /* To reset back to default */
 
 #define DYTC_CMD_FUNC_CAP     3 /* To get DYTC capabilities */
@@ -10152,6 +10153,7 @@ static struct ibm_struct proxsensor_driver_data = {
 
 #define DYTC_FUNCTION_STD     0  /* Function = 0, standard mode */
 #define DYTC_FUNCTION_CQL     1  /* Function = 1, lap mode */
+#define DYTC_FUNCTION_TMS     9  /* Function = 9, TMS mode */
 #define DYTC_FUNCTION_MMC     11 /* Function = 11, MMC mode */
 #define DYTC_FUNCTION_PSC     13 /* Function = 13, PSC mode */
 #define DYTC_FUNCTION_AMT     15 /* Function = 15, AMT mode */
@@ -10163,11 +10165,14 @@ static struct ibm_struct proxsensor_driver_data = {
 #define DYTC_MODE_MMC_LOWPOWER 3  /* Low power mode */
 #define DYTC_MODE_MMC_BALANCE  0xF  /* Default mode aka balanced */
 #define DYTC_MODE_MMC_DEFAULT  0  /* Default mode from MMC_GET, aka balanced */
+#define DYTC_NOMODE            0xF  /* When Function does not have a mode */
 
 #define DYTC_MODE_PSC_LOWPOWER 3  /* Low power mode */
 #define DYTC_MODE_PSC_BALANCE  5  /* Default mode aka balanced */
 #define DYTC_MODE_PSC_PERFORM  7  /* High power mode aka performance */
 
+#define DYTC_UP_SUPPORT    BIT(8)  /* Ultra-performance (TMS) mode support */
+
 #define DYTC_ERR_MASK       0xF  /* Bits 0-3 in cmd result are the error result */
 #define DYTC_ERR_SUCCESS      1  /* CMD completed successful */
 
@@ -10185,6 +10190,7 @@ static enum platform_profile_option dytc_current_profile;
 static atomic_t dytc_ignore_event = ATOMIC_INIT(0);
 static DEFINE_MUTEX(dytc_mutex);
 static int dytc_capabilities;
+static bool dytc_ultraperf_cap;
 static bool dytc_mmc_get_available;
 static int profile_force;
 
@@ -10355,6 +10361,17 @@ static int dytc_profile_set(struct platform_profile_handler *pprof,
 	if (err)
 		goto unlock;
 
+	/* Set TMS mode appropriately (enable for performance), if available */
+	if (dytc_ultraperf_cap) {
+		int cmd;
+
+		cmd = DYTC_SET_COMMAND(DYTC_FUNCTION_TMS, DYTC_NOMODE,
+				       profile == PLATFORM_PROFILE_PERFORMANCE);
+		err = dytc_command(cmd, &output);
+		if (err)
+			return err;
+	}
+
 	if (dytc_capabilities & BIT(DYTC_FC_MMC)) {
 		if (profile == PLATFORM_PROFILE_BALANCED) {
 			/*
@@ -10429,6 +10446,7 @@ static struct platform_profile_handler dytc_profile = {
 static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
 {
 	int err, output;
+	int cmd;
 
 	/* Setup supported modes */
 	set_bit(PLATFORM_PROFILE_LOW_POWER, dytc_profile.choices);
@@ -10484,6 +10502,16 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
 		dbg_printk(TPACPI_DBG_INIT, "No DYTC support available\n");
 		return -ENODEV;
 	}
+	err = dytc_command(DYTC_CMD_UP_CAP, &output);
+	dytc_ultraperf_cap = output & DYTC_UP_SUPPORT;
+	if (dytc_ultraperf_cap) {
+		pr_debug("TMS is supported\n");
+		/* Disable TMS by default - only use with performance mode */
+		cmd = DYTC_SET_COMMAND(DYTC_FUNCTION_TMS, DYTC_NOMODE, 0);
+		err = dytc_command(cmd, &output);
+		if (err)
+			return err;
+	}
 
 	dbg_printk(TPACPI_DBG_INIT,
 			"DYTC version %d: thermal mode available\n", dytc_version);
-- 
2.41.0

Re: [ibm-acpi-devel] [PATCH v2] platform/x86: Add support for improved performance mode

[Mark P. <mpe...@sq...>]

On Mon, Nov 13, 2023, at 9:20 AM, Ilpo Järvinen wrote:
> On Mon, 13 Nov 2023, Mark Pearson wrote:
>
>> Some new Thinkpads have a new improved performance mode available.
>> Add support to make this mode usable.
>> 
>> To avoid having to create a new profile, just use the improved performance
>> mode in place of the existing performance mode, when available.
>> 
>> Tested on P14s AMD G4 AMD.
>> 
>> Signed-off-by: Mark Pearson <mpe...@sq...>
>> ---
>> Changes in v2: updated implementation for DYTC_UP_SUPPORT define
>> 
>>  drivers/platform/x86/thinkpad_acpi.c | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>> 
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index ad460417f901..ed8860caa9c1 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -10136,6 +10136,7 @@ static struct ibm_struct proxsensor_driver_data = {
>>  
>>  #define DYTC_CMD_SET          1 /* To enable/disable IC function mode */
>>  #define DYTC_CMD_MMC_GET      8 /* To get current MMC function and mode */
>> +#define DYTC_CMD_UP_CAP     0xA /* To get Ultra-performance capability */
>>  #define DYTC_CMD_RESET    0x1ff /* To reset back to default */
>>  
>>  #define DYTC_CMD_FUNC_CAP     3 /* To get DYTC capabilities */
>> @@ -10152,6 +10153,7 @@ static struct ibm_struct proxsensor_driver_data = {
>>  
>>  #define DYTC_FUNCTION_STD     0  /* Function = 0, standard mode */
>>  #define DYTC_FUNCTION_CQL     1  /* Function = 1, lap mode */
>> +#define DYTC_FUNCTION_TMS     9  /* Function = 9, TMS mode */
>>  #define DYTC_FUNCTION_MMC     11 /* Function = 11, MMC mode */
>>  #define DYTC_FUNCTION_PSC     13 /* Function = 13, PSC mode */
>>  #define DYTC_FUNCTION_AMT     15 /* Function = 15, AMT mode */
>> @@ -10163,11 +10165,14 @@ static struct ibm_struct proxsensor_driver_data = {
>>  #define DYTC_MODE_MMC_LOWPOWER 3  /* Low power mode */
>>  #define DYTC_MODE_MMC_BALANCE  0xF  /* Default mode aka balanced */
>>  #define DYTC_MODE_MMC_DEFAULT  0  /* Default mode from MMC_GET, aka balanced */
>> +#define DYTC_NOMODE            0xF  /* When Function does not have a mode */
>>  
>>  #define DYTC_MODE_PSC_LOWPOWER 3  /* Low power mode */
>>  #define DYTC_MODE_PSC_BALANCE  5  /* Default mode aka balanced */
>>  #define DYTC_MODE_PSC_PERFORM  7  /* High power mode aka performance */
>>  
>> +#define DYTC_UP_SUPPORT        8  /* Ultra-performance (TMS) mode support */
>
> You forgot to add BIT() here. (But took it away from the other place 
> so this patch is actually broken currently I think).
>
Oh....that's odd. I definitely added it (and tested it)...so I'm not sure how I messed that up in the actual commit :(

Apologies - I'll re-post with it corrected.

>> +
>>  #define DYTC_ERR_MASK       0xF  /* Bits 0-3 in cmd result are the error result */
>>  #define DYTC_ERR_SUCCESS      1  /* CMD completed successful */
>>  
>> @@ -10185,6 +10190,7 @@ static enum platform_profile_option dytc_current_profile;
>>  static atomic_t dytc_ignore_event = ATOMIC_INIT(0);
>>  static DEFINE_MUTEX(dytc_mutex);
>>  static int dytc_capabilities;
>> +static bool dytc_ultraperf_cap; /* ultra performance capable */
>
> I think the comment is repeating the same information already given in the 
> variable name so just drop the comment.
Ack

Mark

Re: [ibm-acpi-devel] [PATCH] platform/x86: Add support for improved performance mode

[Mark P. <mpe...@sq...>]

Hi Ilpo

On Fri, Nov 10, 2023, at 7:37 AM, Ilpo Järvinen wrote:
> On Thu, 9 Nov 2023, Mark Pearson wrote:
>> On Thu, Nov 9, 2023, at 5:10 AM, Ilpo Järvinen wrote:
>> > On Wed, 8 Nov 2023, Mark Pearson wrote:
<snip>
>> 
>> >
>> > Looking into the driver a bit more, there are a few other defines which 
>> > could also move BIT() from the code into defines. Please tell if you're 
>> > going to look at those because if not, I might try to make the patches.
>> 
>> Happy to look at doing that as I'm playing around with this driver anyway.
>
> Okay, thanks.
>
Just a quick note - I pushed v2 for this patch, and I'll tackle the other BIT changes separately (rather than adding it as part of a series). Looking through the code I wanted to spend more time on that piece - I'm not ignoring it :)

Mark

[ibm-acpi-devel] [PATCH v2] platform/x86: Add support for improved performance mode

[Mark P. <mpe...@sq...>]

Some new Thinkpads have a new improved performance mode available.
Add support to make this mode usable.

To avoid having to create a new profile, just use the improved performance
mode in place of the existing performance mode, when available.

Tested on P14s AMD G4 AMD.

Signed-off-by: Mark Pearson <mpe...@sq...>
---
Changes in v2: updated implementation for DYTC_UP_SUPPORT define

 drivers/platform/x86/thinkpad_acpi.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index ad460417f901..ed8860caa9c1 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -10136,6 +10136,7 @@ static struct ibm_struct proxsensor_driver_data = {
 
 #define DYTC_CMD_SET          1 /* To enable/disable IC function mode */
 #define DYTC_CMD_MMC_GET      8 /* To get current MMC function and mode */
+#define DYTC_CMD_UP_CAP     0xA /* To get Ultra-performance capability */
 #define DYTC_CMD_RESET    0x1ff /* To reset back to default */
 
 #define DYTC_CMD_FUNC_CAP     3 /* To get DYTC capabilities */
@@ -10152,6 +10153,7 @@ static struct ibm_struct proxsensor_driver_data = {
 
 #define DYTC_FUNCTION_STD     0  /* Function = 0, standard mode */
 #define DYTC_FUNCTION_CQL     1  /* Function = 1, lap mode */
+#define DYTC_FUNCTION_TMS     9  /* Function = 9, TMS mode */
 #define DYTC_FUNCTION_MMC     11 /* Function = 11, MMC mode */
 #define DYTC_FUNCTION_PSC     13 /* Function = 13, PSC mode */
 #define DYTC_FUNCTION_AMT     15 /* Function = 15, AMT mode */
@@ -10163,11 +10165,14 @@ static struct ibm_struct proxsensor_driver_data = {
 #define DYTC_MODE_MMC_LOWPOWER 3  /* Low power mode */
 #define DYTC_MODE_MMC_BALANCE  0xF  /* Default mode aka balanced */
 #define DYTC_MODE_MMC_DEFAULT  0  /* Default mode from MMC_GET, aka balanced */
+#define DYTC_NOMODE            0xF  /* When Function does not have a mode */
 
 #define DYTC_MODE_PSC_LOWPOWER 3  /* Low power mode */
 #define DYTC_MODE_PSC_BALANCE  5  /* Default mode aka balanced */
 #define DYTC_MODE_PSC_PERFORM  7  /* High power mode aka performance */
 
+#define DYTC_UP_SUPPORT        8  /* Ultra-performance (TMS) mode support */
+
 #define DYTC_ERR_MASK       0xF  /* Bits 0-3 in cmd result are the error result */
 #define DYTC_ERR_SUCCESS      1  /* CMD completed successful */
 
@@ -10185,6 +10190,7 @@ static enum platform_profile_option dytc_current_profile;
 static atomic_t dytc_ignore_event = ATOMIC_INIT(0);
 static DEFINE_MUTEX(dytc_mutex);
 static int dytc_capabilities;
+static bool dytc_ultraperf_cap; /* ultra performance capable */
 static bool dytc_mmc_get_available;
 static int profile_force;
 
@@ -10355,6 +10361,17 @@ static int dytc_profile_set(struct platform_profile_handler *pprof,
 	if (err)
 		goto unlock;
 
+	/* Set TMS mode appropriately (enable for performance), if available */
+	if (dytc_ultraperf_cap) {
+		int cmd;
+
+		cmd = DYTC_SET_COMMAND(DYTC_FUNCTION_TMS, DYTC_NOMODE,
+				       profile == PLATFORM_PROFILE_PERFORMANCE);
+		err = dytc_command(cmd, &output);
+		if (err)
+			return err;
+	}
+
 	if (dytc_capabilities & BIT(DYTC_FC_MMC)) {
 		if (profile == PLATFORM_PROFILE_BALANCED) {
 			/*
@@ -10429,6 +10446,7 @@ static struct platform_profile_handler dytc_profile = {
 static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
 {
 	int err, output;
+	int cmd;
 
 	/* Setup supported modes */
 	set_bit(PLATFORM_PROFILE_LOW_POWER, dytc_profile.choices);
@@ -10484,6 +10502,16 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
 		dbg_printk(TPACPI_DBG_INIT, "No DYTC support available\n");
 		return -ENODEV;
 	}
+	err = dytc_command(DYTC_CMD_UP_CAP, &output);
+	dytc_ultraperf_cap = output & DYTC_UP_SUPPORT ? true : false;
+	if (dytc_ultraperf_cap) {
+		pr_debug("TMS is supported\n");
+		/* Disable TMS by default - only use with performance mode */
+		cmd = DYTC_SET_COMMAND(DYTC_FUNCTION_TMS, DYTC_NOMODE, 0);
+		err = dytc_command(cmd, &output);
+		if (err)
+			return err;
+	}
 
 	dbg_printk(TPACPI_DBG_INIT,
 			"DYTC version %d: thermal mode available\n", dytc_version);
-- 
2.41.0

Re: [ibm-acpi-devel] [PATCH] platform/x86: Add support for improved performance mode

[Mark P. <mpe...@sq...>]

Hi Ilpo,

On Thu, Nov 9, 2023, at 5:10 AM, Ilpo Järvinen wrote:
> On Wed, 8 Nov 2023, Mark Pearson wrote:
>
>> Some new Thinkpads have a new improved performance mode available.
>> Add support to make this mode usable.
>> 
>> To avoid having to create a new profile, just use the improved performance
>> mode in place of the existing performance mode, when available.
>> 
>> Tested on T14 AMD G4 AMD.
>> 
>> Signed-off-by: Mark Pearson <mpe...@sq...>
>> ---
>>  drivers/platform/x86/thinkpad_acpi.c | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>> 
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index ad460417f901..eba701ab340e 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -10136,6 +10136,7 @@ static struct ibm_struct proxsensor_driver_data = {
>>  
>>  #define DYTC_CMD_SET          1 /* To enable/disable IC function mode */
>>  #define DYTC_CMD_MMC_GET      8 /* To get current MMC function and mode */
>> +#define DYTC_CMD_UP_CAP     0xA /* To get Ultra-performance capability */
>>  #define DYTC_CMD_RESET    0x1ff /* To reset back to default */
>>  
>>  #define DYTC_CMD_FUNC_CAP     3 /* To get DYTC capabilities */
>> @@ -10152,6 +10153,7 @@ static struct ibm_struct proxsensor_driver_data = {
>>  
>>  #define DYTC_FUNCTION_STD     0  /* Function = 0, standard mode */
>>  #define DYTC_FUNCTION_CQL     1  /* Function = 1, lap mode */
>> +#define DYTC_FUNCTION_TMS     9  /* Function = 9, TMS mode */
>>  #define DYTC_FUNCTION_MMC     11 /* Function = 11, MMC mode */
>>  #define DYTC_FUNCTION_PSC     13 /* Function = 13, PSC mode */
>>  #define DYTC_FUNCTION_AMT     15 /* Function = 15, AMT mode */
>> @@ -10163,11 +10165,14 @@ static struct ibm_struct proxsensor_driver_data = {
>>  #define DYTC_MODE_MMC_LOWPOWER 3  /* Low power mode */
>>  #define DYTC_MODE_MMC_BALANCE  0xF  /* Default mode aka balanced */
>>  #define DYTC_MODE_MMC_DEFAULT  0  /* Default mode from MMC_GET, aka balanced */
>> +#define DYTC_NOMODE            0xF  /* When Function does not have a mode */
>>  
>>  #define DYTC_MODE_PSC_LOWPOWER 3  /* Low power mode */
>>  #define DYTC_MODE_PSC_BALANCE  5  /* Default mode aka balanced */
>>  #define DYTC_MODE_PSC_PERFORM  7  /* High power mode aka performance */
>>  
>> +#define DYTC_UP_SUPPORT_BIT    8  /* Bit 8 - 1 = supported, 0 = not */
>
> It would be preferrable to comment what is supported rather than have a 
> comment like above which isn't particularly helpful.

OK - so  just have:
#define DYTC_UP_SUPPORT_BIT    8  /* Ultra-performance (TMS) mode support */

Or...reading ahead in the review this should actually be
#define DYTC_UP_SUPPORT_BIT    BIT(8)  /* Ultra-performance (TMS) mode support */

>
>>  #define DYTC_ERR_MASK       0xF  /* Bits 0-3 in cmd result are the error result */
>>  #define DYTC_ERR_SUCCESS      1  /* CMD completed successful */
>>  
>> @@ -10185,6 +10190,7 @@ static enum platform_profile_option dytc_current_profile;
>>  static atomic_t dytc_ignore_event = ATOMIC_INIT(0);
>>  static DEFINE_MUTEX(dytc_mutex);
>>  static int dytc_capabilities;
>> +static bool dytc_ultraperf_cap; /* ultra performance capable */
>>  static bool dytc_mmc_get_available;
>>  static int profile_force;
>>  
>> @@ -10355,6 +10361,17 @@ static int dytc_profile_set(struct platform_profile_handler *pprof,
>>  	if (err)
>>  		goto unlock;
>>  
>> +	/* Set TMS mode appropriately (enable for performance), if available */
>> +	if (dytc_ultraperf_cap) {
>> +		int cmd;
>> +
>> +		cmd = DYTC_SET_COMMAND(DYTC_FUNCTION_TMS, DYTC_NOMODE,
>> +				       profile == PLATFORM_PROFILE_PERFORMANCE);
>> +		err = dytc_command(cmd, &output);
>> +		if (err)
>> +			return err;
>> +	}
>> +
>>  	if (dytc_capabilities & BIT(DYTC_FC_MMC)) {
>>  		if (profile == PLATFORM_PROFILE_BALANCED) {
>>  			/*
>> @@ -10429,6 +10446,7 @@ static struct platform_profile_handler dytc_profile = {
>>  static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
>>  {
>>  	int err, output;
>> +	int cmd;
>>  
>>  	/* Setup supported modes */
>>  	set_bit(PLATFORM_PROFILE_LOW_POWER, dytc_profile.choices);
>> @@ -10484,6 +10502,16 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
>>  		dbg_printk(TPACPI_DBG_INIT, "No DYTC support available\n");
>>  		return -ENODEV;
>>  	}
>> +	err = dytc_command(DYTC_CMD_UP_CAP, &output);
>> +	dytc_ultraperf_cap = output & BIT(DYTC_UP_SUPPORT_BIT) ? true : false;
>
> It would be better to put this BIT() into the define itself and remove 
> _BIT from the name because it doesn't really add that much information.
> Since you're assigning to bool, ? true : false construct is not required 
> but implicit cast will handle it for you. So in the end, this line would 
> be:
>
> 	dytc_ultraperf_cap = output & DYTC_UP_SUPPORT;

Agreed. I will make that change.
I'll wait and see if there is any more feedback and then do that with a v2 patch.

>
> Looking into the driver a bit more, there are a few other defines which 
> could also move BIT() from the code into defines. Please tell if you're 
> going to look at those because if not, I might try to make the patches.

Happy to look at doing that as I'm playing around with this driver anyway.

Thanks for the review!
Mark

[ibm-acpi-devel] [PATCH] platform/x86: Add support for improved performance mode

[Mark P. <mpe...@sq...>]

Some new Thinkpads have a new improved performance mode available.
Add support to make this mode usable.

To avoid having to create a new profile, just use the improved performance
mode in place of the existing performance mode, when available.

Tested on T14 AMD G4 AMD.

Signed-off-by: Mark Pearson <mpe...@sq...>
---
 drivers/platform/x86/thinkpad_acpi.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index ad460417f901..eba701ab340e 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -10136,6 +10136,7 @@ static struct ibm_struct proxsensor_driver_data = {
 
 #define DYTC_CMD_SET          1 /* To enable/disable IC function mode */
 #define DYTC_CMD_MMC_GET      8 /* To get current MMC function and mode */
+#define DYTC_CMD_UP_CAP     0xA /* To get Ultra-performance capability */
 #define DYTC_CMD_RESET    0x1ff /* To reset back to default */
 
 #define DYTC_CMD_FUNC_CAP     3 /* To get DYTC capabilities */
@@ -10152,6 +10153,7 @@ static struct ibm_struct proxsensor_driver_data = {
 
 #define DYTC_FUNCTION_STD     0  /* Function = 0, standard mode */
 #define DYTC_FUNCTION_CQL     1  /* Function = 1, lap mode */
+#define DYTC_FUNCTION_TMS     9  /* Function = 9, TMS mode */
 #define DYTC_FUNCTION_MMC     11 /* Function = 11, MMC mode */
 #define DYTC_FUNCTION_PSC     13 /* Function = 13, PSC mode */
 #define DYTC_FUNCTION_AMT     15 /* Function = 15, AMT mode */
@@ -10163,11 +10165,14 @@ static struct ibm_struct proxsensor_driver_data = {
 #define DYTC_MODE_MMC_LOWPOWER 3  /* Low power mode */
 #define DYTC_MODE_MMC_BALANCE  0xF  /* Default mode aka balanced */
 #define DYTC_MODE_MMC_DEFAULT  0  /* Default mode from MMC_GET, aka balanced */
+#define DYTC_NOMODE            0xF  /* When Function does not have a mode */
 
 #define DYTC_MODE_PSC_LOWPOWER 3  /* Low power mode */
 #define DYTC_MODE_PSC_BALANCE  5  /* Default mode aka balanced */
 #define DYTC_MODE_PSC_PERFORM  7  /* High power mode aka performance */
 
+#define DYTC_UP_SUPPORT_BIT    8  /* Bit 8 - 1 = supported, 0 = not */
+
 #define DYTC_ERR_MASK       0xF  /* Bits 0-3 in cmd result are the error result */
 #define DYTC_ERR_SUCCESS      1  /* CMD completed successful */
 
@@ -10185,6 +10190,7 @@ static enum platform_profile_option dytc_current_profile;
 static atomic_t dytc_ignore_event = ATOMIC_INIT(0);
 static DEFINE_MUTEX(dytc_mutex);
 static int dytc_capabilities;
+static bool dytc_ultraperf_cap; /* ultra performance capable */
 static bool dytc_mmc_get_available;
 static int profile_force;
 
@@ -10355,6 +10361,17 @@ static int dytc_profile_set(struct platform_profile_handler *pprof,
 	if (err)
 		goto unlock;
 
+	/* Set TMS mode appropriately (enable for performance), if available */
+	if (dytc_ultraperf_cap) {
+		int cmd;
+
+		cmd = DYTC_SET_COMMAND(DYTC_FUNCTION_TMS, DYTC_NOMODE,
+				       profile == PLATFORM_PROFILE_PERFORMANCE);
+		err = dytc_command(cmd, &output);
+		if (err)
+			return err;
+	}
+
 	if (dytc_capabilities & BIT(DYTC_FC_MMC)) {
 		if (profile == PLATFORM_PROFILE_BALANCED) {
 			/*
@@ -10429,6 +10446,7 @@ static struct platform_profile_handler dytc_profile = {
 static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
 {
 	int err, output;
+	int cmd;
 
 	/* Setup supported modes */
 	set_bit(PLATFORM_PROFILE_LOW_POWER, dytc_profile.choices);
@@ -10484,6 +10502,16 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
 		dbg_printk(TPACPI_DBG_INIT, "No DYTC support available\n");
 		return -ENODEV;
 	}
+	err = dytc_command(DYTC_CMD_UP_CAP, &output);
+	dytc_ultraperf_cap = output & BIT(DYTC_UP_SUPPORT_BIT) ? true : false;
+	if (dytc_ultraperf_cap) {
+		pr_debug("TMS is supported\n");
+		/* Disable TMS by default - only use with performance mode */
+		cmd = DYTC_SET_COMMAND(DYTC_FUNCTION_TMS, DYTC_NOMODE, 0);
+		err = dytc_command(cmd, &output);
+		if (err)
+			return err;
+	}
 
 	dbg_printk(TPACPI_DBG_INIT,
 			"DYTC version %d: thermal mode available\n", dytc_version);
-- 
2.41.0

Re: [ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: replace deprecated strncpy with memcpy

[Mark P. <mpe...@sq...>]

On Fri, Oct 20, 2023, at 1:52 PM, Justin Stitt wrote:
> strncpy() is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous
> interfaces.
>
> We expect ec_fw_string to be NUL-terminated based on its use with format
> strings in thinkpad_acpi.c:
> 11241 | pr_notice("ThinkPad firmware release %s doesn't match the known 
> patterns\n",
> 11242 |     ec_fw_string);
>
> Moreover, NUL-padding is not required since ec_fw_string is explicitly
> zero-initialized:
> 11185 | char ec_fw_string[18] = {0};
>
> When carefully copying bytes from one buffer to another in
> pre-determined blocks (like what's happening here with dmi_data):
>
> |       static void find_new_ec_fwstr(const struct dmi_header *dm, void 
> *private)
> |       {
> |       	char *ec_fw_string = (char *) private;
> |       	const char *dmi_data = (const char *)dm;
> |       	/*
> |       	 * ThinkPad Embedded Controller Program Table on newer models
> |       	 *
> |       	 * Offset |  Name                | Width  | Description
> |       	 * ----------------------------------------------------
> |       	 *  0x00  | Type                 | BYTE   | 0x8C
> |       	 *  0x01  | Length               | BYTE   |
> |       	 *  0x02  | Handle               | WORD   | Varies
> |       	 *  0x04  | Signature            | BYTEx6 | ASCII for "LENOVO"
> |       	 *  0x0A  | OEM struct offset    | BYTE   | 0x0B
> |       	 *  0x0B  | OEM struct number    | BYTE   | 0x07, for this 
> structure
> |       	 *  0x0C  | OEM struct revision  | BYTE   | 0x01, for this 
> format
> |       	 *  0x0D  | ECP version ID       | STR ID |
> |       	 *  0x0E  | ECP release date     | STR ID |
> |       	 */
> |
> |       	/* Return if data structure not match */
> |       	if (dm->type != 140 || dm->length < 0x0F ||
> |       	memcmp(dmi_data + 4, "LENOVO", 6) != 0 ||
> |       	dmi_data[0x0A] != 0x0B || dmi_data[0x0B] != 0x07 ||
> |       	dmi_data[0x0C] != 0x01)
> |       		return;
> |
> |       	/* fwstr is the first 8byte string  */
> |       	strncpy(ec_fw_string, dmi_data + 0x0F, 8);
>
> ... we shouldn't be using a C string api. Let's instead use memcpy() as
> this more properly relays the intended behavior.
>
> Do note that ec_fw_string will still end up being NUL-terminated since
> we are memcpy'ing only 8 bytes into a buffer full of 18 zeroes. There's
> still some trailing NUL-bytes there. To ensure this behavior, let's add
> a BUILD_BUG_ON checking the length leaves space for at least one
> trailing NUL-byte.
>
> Link: 
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings 
> [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: Kees Cook <kee...@ch...>
> Signed-off-by: Justin Stitt <jus...@go...>
> ---
> Note: build-tested only.
>
> Found with: $ rg "strncpy\("
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c 
> b/drivers/platform/x86/thinkpad_acpi.c
> index 41584427dc32..bd9e06f5b860 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -11144,6 +11144,8 @@ static char __init tpacpi_parse_fw_id(const 
> char * const s,
>  	return '\0';
>  }
> 
> +#define EC_FW_STRING_LEN 18
> +
>  static void find_new_ec_fwstr(const struct dmi_header *dm, void 
> *private)
>  {
>  	char *ec_fw_string = (char *) private;
> @@ -11172,7 +11174,8 @@ static void find_new_ec_fwstr(const struct 
> dmi_header *dm, void *private)
>  		return;
> 
>  	/* fwstr is the first 8byte string  */
> -	strncpy(ec_fw_string, dmi_data + 0x0F, 8);
> +	BUILD_BUG_ON(EC_FW_STRING_LEN <= 8);
> +	memcpy(ec_fw_string, dmi_data + 0x0F, 8);
>  }
> 
>  /* returns 0 - probe ok, or < 0 - probe error.
> @@ -11182,7 +11185,7 @@ static int __must_check __init get_thinkpad_model_data(
>  						struct thinkpad_id_data *tp)
>  {
>  	const struct dmi_device *dev = NULL;
> -	char ec_fw_string[18] = {0};
> +	char ec_fw_string[EC_FW_STRING_LEN] = {0};
>  	char const *s;
>  	char t;
> 
>
> ---
> base-commit: dab3e01664eaddae965699f1fec776609db0ea9d
> change-id: 20231019-strncpy-drivers-platform-x86-thinkpad_acpi-c-7a733d087ef7
>
> Best regards,
> --
> Justin Stitt <jus...@go...>

Looks good to me.
Reviewed-by: Mark Pearson <mpe...@sq...>

Re: [ibm-acpi-devel] [PATCH v6] platform/x86: thinkpad_acpi: sysfs interface to auxmac

[Hans de G. <hde...@re...>]

Hi,

On 9/26/23 22:21, Fernando Eckhardt Valle wrote:
> Newer Thinkpads have a feature called MAC Address Pass-through.
> This patch provides a sysfs interface that userspace can use
> to get this auxiliary mac address.
> 
> Signed-off-by: Fernando Eckhardt Valle <fe...@ip...>
> ---
> Changes in v6:
> - New adjustment to the strcpy() offset.
> - Added is_visible attribute.
> Changes in v5:
> - Repeated code deleted.
> - Adjusted offset of a strscpy().
> Changes in v4:
> - strscpy() in all string copies.
> Changes in v3:
> - Added null terminator to auxmac string when copying auxiliary
> mac address value.
> Changes in v2:
> - Added documentation.
> - All handling of the auxmac value is done in the _init function.

Thanks, this looks good to me now:

Reviewed-by: Hans de Goede <hde...@re...>

Regards,

Hans


> ---
>  .../admin-guide/laptops/thinkpad-acpi.rst     | 20 +++++
>  drivers/platform/x86/thinkpad_acpi.c          | 88 +++++++++++++++++++
>  2 files changed, 108 insertions(+)
> 
> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> index e27a1c3f6..98d304010 100644
> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> @@ -53,6 +53,7 @@ detailed description):
>  	- Lap mode sensor
>  	- Setting keyboard language
>  	- WWAN Antenna type
> +	- Auxmac
>  
>  A compatibility table by model and feature is maintained on the web
>  site, http://ibm-acpi.sf.net/. I appreciate any success or failure
> @@ -1511,6 +1512,25 @@ Currently 2 antenna types are supported as mentioned below:
>  The property is read-only. If the platform doesn't have support the sysfs
>  class is not created.
>  
> +Auxmac
> +------
> +
> +sysfs: auxmac
> +
> +Some newer Thinkpads have a feature called MAC Address Pass-through. This
> +feature is implemented by the system firmware to provide a system unique MAC,
> +that can override a dock or USB ethernet dongle MAC, when connected to a
> +network. This property enables user-space to easily determine the MAC address
> +if the feature is enabled.
> +
> +The values of this auxiliary MAC are:
> +
> +        cat /sys/devices/platform/thinkpad_acpi/auxmac
> +
> +If the feature is disabled, the value will be 'disabled'.
> +
> +This property is read-only.
> +
>  Adaptive keyboard
>  -----------------
>  
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index d70c89d32..9c19624a7 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -10785,6 +10785,89 @@ static struct ibm_struct dprc_driver_data = {
>  	.name = "dprc",
>  };
>  
> +/*
> + * Auxmac
> + *
> + * This auxiliary mac address is enabled in the bios through the
> + * MAC Address Pass-through feature. In most cases, there are three
> + * possibilities: Internal Mac, Second Mac, and disabled.
> + *
> + */
> +
> +#define AUXMAC_LEN 12
> +#define AUXMAC_START 9
> +#define AUXMAC_STRLEN 22
> +#define AUXMAC_BEGIN_MARKER 8
> +#define AUXMAC_END_MARKER 21
> +
> +static char auxmac[AUXMAC_LEN + 1];
> +
> +static int auxmac_init(struct ibm_init_struct *iibm)
> +{
> +	acpi_status status;
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *obj;
> +
> +	status = acpi_evaluate_object(NULL, "\\MACA", NULL, &buffer);
> +
> +	if (ACPI_FAILURE(status))
> +		return -ENODEV;
> +
> +	obj = buffer.pointer;
> +
> +	if (obj->type != ACPI_TYPE_STRING || obj->string.length != AUXMAC_STRLEN) {
> +		pr_info("Invalid buffer for MAC address pass-through.\n");
> +		goto auxmacinvalid;
> +	}
> +
> +	if (obj->string.pointer[AUXMAC_BEGIN_MARKER] != '#' ||
> +	    obj->string.pointer[AUXMAC_END_MARKER] != '#') {
> +		pr_info("Invalid header for MAC address pass-through.\n");
> +		goto auxmacinvalid;
> +	}
> +
> +	if (strncmp(obj->string.pointer + AUXMAC_START, "XXXXXXXXXXXX", AUXMAC_LEN) != 0)
> +		strscpy(auxmac, obj->string.pointer + AUXMAC_START, sizeof(auxmac));
> +	else
> +		strscpy(auxmac, "disabled", sizeof(auxmac));
> +
> +free:
> +	kfree(obj);
> +	return 0;
> +
> +auxmacinvalid:
> +	strscpy(auxmac, "unavailable", sizeof(auxmac));
> +	goto free;
> +}
> +
> +static struct ibm_struct auxmac_data = {
> +	.name = "auxmac",
> +};
> +
> +static ssize_t auxmac_show(struct device *dev,
> +			   struct device_attribute *attr,
> +			   char *buf)
> +{
> +	return sysfs_emit(buf, "%s\n", auxmac);
> +}
> +static DEVICE_ATTR_RO(auxmac);
> +
> +static umode_t auxmac_attr_is_visible(struct kobject *kobj,
> +				      struct attribute *attr, int n)
> +{
> +	return auxmac[0] == 0 ? 0 : attr->mode;
> +}
> +
> +static struct attribute *auxmac_attributes[] = {
> +	&dev_attr_auxmac.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group auxmac_attr_group = {
> +	.is_visible = auxmac_attr_is_visible,
> +	.attrs = auxmac_attributes,
> +};
> +
>  /* --------------------------------------------------------------------- */
>  
>  static struct attribute *tpacpi_driver_attributes[] = {
> @@ -10843,6 +10926,7 @@ static const struct attribute_group *tpacpi_groups[] = {
>  	&proxsensor_attr_group,
>  	&kbdlang_attr_group,
>  	&dprc_attr_group,
> +	&auxmac_attr_group,
>  	NULL,
>  };
>  
> @@ -11414,6 +11498,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
>  		.init = tpacpi_dprc_init,
>  		.data = &dprc_driver_data,
>  	},
> +	{
> +		.init = auxmac_init,
> +		.data = &auxmac_data,
> +	},
>  };
>  
>  static int __init set_ibm_param(const char *val, const struct kernel_param *kp)

Re: [ibm-acpi-devel] [PATCH v5] platform/x86: thinkpad_acpi: sysfs interface to auxmac

[Hans de G. <hde...@re...>]

Hi,

On 9/26/23 20:42, Fernando Eckhardt Valle (FIPT) wrote:
> Thanks for the review Hans, I'll send a new version with some adjustments.
> 
>> Note this is just a preference you keen keep this as is
>> if you want,
> I liked the Ilpo suggestion, with two 'gotos' is eliminated repeated code. If everything is ok, I prefer it this way.

If you prefer the 2 goto solution from v5 then keeping it as is is fine.

Regards,

Hans




> ________________________________________
> From: Hans de Goede <hde...@re...>
> Sent: Tuesday, September 26, 2023 7:23 AM
> To: Fernando Eckhardt Valle (FIPT); ilp...@li...; mpe...@sq...; co...@lw...; hm...@hm...; mar...@ke...; lin...@vg...; lin...@vg...; ibm...@li...; pla...@vg...
> Subject: Re: [PATCH v5] platform/x86: thinkpad_acpi: sysfs interface to auxmac
> 
> Hi,
> 
> It looks like I just reviewed an old version, reviewing this version now ...
> 
> On 9/25/23 20:41, Fernando Eckhardt Valle wrote:
>> Newer Thinkpads have a feature called MAC Address Pass-through.
>> This patch provides a sysfs interface that userspace can use
>> to get this auxiliary mac address.
>>
>> Signed-off-by: Fernando Eckhardt Valle <fe...@ip...>
>> ---
>> Changes in v5:
>> - Repeated code deleted.
>> - Adjusted offset of a strscpy().
>> Changes in v4:
>> - strscpy() in all string copies.
>> Changes in v3:
>> - Added null terminator to auxmac string when copying auxiliary
>> mac address value.
>> Changes in v2:
>> - Added documentation.
>> - All handling of the auxmac value is done in the _init function.
>> ---
>>  .../admin-guide/laptops/thinkpad-acpi.rst     | 20 +++++
>>  drivers/platform/x86/thinkpad_acpi.c          | 81 +++++++++++++++++++
>>  2 files changed, 101 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> index e27a1c3f6..98d304010 100644
>> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> @@ -53,6 +53,7 @@ detailed description):
>>       - Lap mode sensor
>>       - Setting keyboard language
>>       - WWAN Antenna type
>> +     - Auxmac
>>
>>  A compatibility table by model and feature is maintained on the web
>>  site, http://ibm-acpi.sf.net/. I appreciate any success or failure
>> @@ -1511,6 +1512,25 @@ Currently 2 antenna types are supported as mentioned below:
>>  The property is read-only. If the platform doesn't have support the sysfs
>>  class is not created.
>>
>> +Auxmac
>> +------
>> +
>> +sysfs: auxmac
>> +
>> +Some newer Thinkpads have a feature called MAC Address Pass-through. This
>> +feature is implemented by the system firmware to provide a system unique MAC,
>> +that can override a dock or USB ethernet dongle MAC, when connected to a
>> +network. This property enables user-space to easily determine the MAC address
>> +if the feature is enabled.
>> +
>> +The values of this auxiliary MAC are:
>> +
>> +        cat /sys/devices/platform/thinkpad_acpi/auxmac
>> +
>> +If the feature is disabled, the value will be 'disabled'.
>> +
>> +This property is read-only.
>> +
>>  Adaptive keyboard
>>  -----------------
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index d70c89d32..2324ebb46 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -10785,6 +10785,82 @@ static struct ibm_struct dprc_driver_data = {
>>       .name = "dprc",
>>  };
>>
>> +/*
>> + * Auxmac
>> + *
>> + * This auxiliary mac address is enabled in the bios through the
>> + * MAC Address Pass-through feature. In most cases, there are three
>> + * possibilities: Internal Mac, Second Mac, and disabled.
>> + *
>> + */
>> +
>> +#define AUXMAC_LEN 12
>> +#define AUXMAC_START 9
>> +#define AUXMAC_STRLEN 22
>> +#define AUXMAC_BEGIN_MARKER 8
>> +#define AUXMAC_END_MARKER 21
>> +
>> +static char auxmac[AUXMAC_LEN + 1];
>> +
>> +static int auxmac_init(struct ibm_init_struct *iibm)
>> +{
>> +     acpi_status status;
>> +     struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>> +     union acpi_object *obj;
>> +
>> +     status = acpi_evaluate_object(NULL, "\\MACA", NULL, &buffer);
>> +
>> +     if (ACPI_FAILURE(status))
>> +             return -ENODEV;
> 
> In this code path you don't initialize the "auxmac" buffer at all,
> but your auxmac_attr_group does not have an is_visible callback,
> so the auxmax sysfs attr will still show up.
> 
> Please add an is_visible callback and retuern 0 (not visible)
> when auxmac[0] == 0; See existing is_visible code for some
> examples.
> 
>> +
>> +     obj = buffer.pointer;
>> +
>> +     if (obj->type != ACPI_TYPE_STRING || obj->string.length != AUXMAC_STRLEN) {
>> +             pr_info("Invalid buffer for MAC address pass-through.\n");
>> +             goto auxmacinvalid;
>> +     }
>> +
>> +     if (obj->string.pointer[AUXMAC_BEGIN_MARKER] != '#' ||
>> +         obj->string.pointer[AUXMAC_END_MARKER] != '#') {
>> +             pr_info("Invalid header for MAC address pass-through.\n");
>> +             goto auxmacinvalid;
>> +     }
>> +
>> +     if (strncmp(obj->string.pointer + AUXMAC_START, "XXXXXXXXXXXX", AUXMAC_LEN) != 0)
>> +             strscpy(auxmac, obj->string.pointer + AUXMAC_START, AUXMAC_LEN + 1);
> 
> Please use sizeof(auxmac) as last parameter to strscpy() here.
> 
>> +     else
>> +             strscpy(auxmac, "disabled", AUXMAC_LEN);
> 
> Please use sizeof(auxmac) as last parameter to strscpy() here.
> 
> Also note how you pass 2 different dest-sizes for the same dest buffer before,
> which looks weird ...
> 
> 
>> +
>> +free:
>> +     kfree(obj);
>> +     return 0;
>> +
>> +auxmacinvalid:
>> +     strscpy(auxmac, "unavailable", AUXMAC_LEN);
>> +     goto free;
>> +}
> 
> I'm not liking the goto dance here, I would prefer:
> 
>         kfree(obj);
>         return 0;
> 
> auxmacinvalid:
>         strscpy(auxmac, "unavailable", AUXMAC_LEN);
>         kfree(obj);
>         return 0;
> 
> It is quite normal for an error-exit path to repeat a kfree().
> 
> Note this is just a preference you keen keep this as is
> if you want, but to me the goto free which jumps up looks
> pretty weird.
> 
> Regards,
> 
> Hans
> 
> 
> 
>> +
>> +static struct ibm_struct auxmac_data = {
>> +     .name = "auxmac",
>> +};
>> +
>> +static ssize_t auxmac_show(struct device *dev,
>> +                        struct device_attribute *attr,
>> +                        char *buf)
>> +{
>> +     return sysfs_emit(buf, "%s\n", auxmac);
>> +}
>> +static DEVICE_ATTR_RO(auxmac);
>> +
>> +static struct attribute *auxmac_attributes[] = {
>> +     &dev_attr_auxmac.attr,
>> +     NULL
>> +};
>> +
>> +static const struct attribute_group auxmac_attr_group = {
>> +     .attrs = auxmac_attributes,
>> +};
>> +
>>  /* --------------------------------------------------------------------- */
>>
>>  static struct attribute *tpacpi_driver_attributes[] = {
>> @@ -10843,6 +10919,7 @@ static const struct attribute_group *tpacpi_groups[] = {
>>       &proxsensor_attr_group,
>>       &kbdlang_attr_group,
>>       &dprc_attr_group,
>> +     &auxmac_attr_group,
>>       NULL,
>>  };
>>
>> @@ -11414,6 +11491,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
>>               .init = tpacpi_dprc_init,
>>               .data = &dprc_driver_data,
>>       },
>> +     {
>> +             .init = auxmac_init,
>> +             .data = &auxmac_data,
>> +     },
>>  };
>>
>>  static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
> 
> 

Re: [ibm-acpi-devel] [PATCH v5] platform/x86: thinkpad_acpi: sysfs interface to auxmac

[Hans de G. <hde...@re...>]

Hi,

It looks like I just reviewed an old version, reviewing this version now ...

On 9/25/23 20:41, Fernando Eckhardt Valle wrote:
> Newer Thinkpads have a feature called MAC Address Pass-through.
> This patch provides a sysfs interface that userspace can use
> to get this auxiliary mac address.
> 
> Signed-off-by: Fernando Eckhardt Valle <fe...@ip...>
> ---
> Changes in v5:
> - Repeated code deleted.
> - Adjusted offset of a strscpy().
> Changes in v4:
> - strscpy() in all string copies.
> Changes in v3:
> - Added null terminator to auxmac string when copying auxiliary
> mac address value.
> Changes in v2:
> - Added documentation.
> - All handling of the auxmac value is done in the _init function.
> ---
>  .../admin-guide/laptops/thinkpad-acpi.rst     | 20 +++++
>  drivers/platform/x86/thinkpad_acpi.c          | 81 +++++++++++++++++++
>  2 files changed, 101 insertions(+)
> 
> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> index e27a1c3f6..98d304010 100644
> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> @@ -53,6 +53,7 @@ detailed description):
>  	- Lap mode sensor
>  	- Setting keyboard language
>  	- WWAN Antenna type
> +	- Auxmac
>  
>  A compatibility table by model and feature is maintained on the web
>  site, http://ibm-acpi.sf.net/. I appreciate any success or failure
> @@ -1511,6 +1512,25 @@ Currently 2 antenna types are supported as mentioned below:
>  The property is read-only. If the platform doesn't have support the sysfs
>  class is not created.
>  
> +Auxmac
> +------
> +
> +sysfs: auxmac
> +
> +Some newer Thinkpads have a feature called MAC Address Pass-through. This
> +feature is implemented by the system firmware to provide a system unique MAC,
> +that can override a dock or USB ethernet dongle MAC, when connected to a
> +network. This property enables user-space to easily determine the MAC address
> +if the feature is enabled.
> +
> +The values of this auxiliary MAC are:
> +
> +        cat /sys/devices/platform/thinkpad_acpi/auxmac
> +
> +If the feature is disabled, the value will be 'disabled'.
> +
> +This property is read-only.
> +
>  Adaptive keyboard
>  -----------------
>  
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index d70c89d32..2324ebb46 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -10785,6 +10785,82 @@ static struct ibm_struct dprc_driver_data = {
>  	.name = "dprc",
>  };
>  
> +/*
> + * Auxmac
> + *
> + * This auxiliary mac address is enabled in the bios through the
> + * MAC Address Pass-through feature. In most cases, there are three
> + * possibilities: Internal Mac, Second Mac, and disabled.
> + *
> + */
> +
> +#define AUXMAC_LEN 12
> +#define AUXMAC_START 9
> +#define AUXMAC_STRLEN 22
> +#define AUXMAC_BEGIN_MARKER 8
> +#define AUXMAC_END_MARKER 21
> +
> +static char auxmac[AUXMAC_LEN + 1];
> +
> +static int auxmac_init(struct ibm_init_struct *iibm)
> +{
> +	acpi_status status;
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *obj;
> +
> +	status = acpi_evaluate_object(NULL, "\\MACA", NULL, &buffer);
> +
> +	if (ACPI_FAILURE(status))
> +		return -ENODEV;

In this code path you don't initialize the "auxmac" buffer at all,
but your auxmac_attr_group does not have an is_visible callback,
so the auxmax sysfs attr will still show up.

Please add an is_visible callback and retuern 0 (not visible)
when auxmac[0] == 0; See existing is_visible code for some
examples.

> +
> +	obj = buffer.pointer;
> +
> +	if (obj->type != ACPI_TYPE_STRING || obj->string.length != AUXMAC_STRLEN) {
> +		pr_info("Invalid buffer for MAC address pass-through.\n");
> +		goto auxmacinvalid;
> +	}
> +
> +	if (obj->string.pointer[AUXMAC_BEGIN_MARKER] != '#' ||
> +	    obj->string.pointer[AUXMAC_END_MARKER] != '#') {
> +		pr_info("Invalid header for MAC address pass-through.\n");
> +		goto auxmacinvalid;
> +	}
> +
> +	if (strncmp(obj->string.pointer + AUXMAC_START, "XXXXXXXXXXXX", AUXMAC_LEN) != 0)
> +		strscpy(auxmac, obj->string.pointer + AUXMAC_START, AUXMAC_LEN + 1);

Please use sizeof(auxmac) as last parameter to strscpy() here.

> +	else
> +		strscpy(auxmac, "disabled", AUXMAC_LEN);

Please use sizeof(auxmac) as last parameter to strscpy() here.

Also note how you pass 2 different dest-sizes for the same dest buffer before,
which looks weird ...


> +
> +free:
> +	kfree(obj);
> +	return 0;
> +
> +auxmacinvalid:
> +	strscpy(auxmac, "unavailable", AUXMAC_LEN);
> +	goto free;
> +}

I'm not liking the goto dance here, I would prefer:

	kfree(obj);
	return 0;

auxmacinvalid:
	strscpy(auxmac, "unavailable", AUXMAC_LEN);
	kfree(obj);
	return 0;

It is quite normal for an error-exit path to repeat a kfree().

Note this is just a preference you keen keep this as is
if you want, but to me the goto free which jumps up looks
pretty weird.

Regards,

Hans



> +
> +static struct ibm_struct auxmac_data = {
> +	.name = "auxmac",
> +};
> +
> +static ssize_t auxmac_show(struct device *dev,
> +			   struct device_attribute *attr,
> +			   char *buf)
> +{
> +	return sysfs_emit(buf, "%s\n", auxmac);
> +}
> +static DEVICE_ATTR_RO(auxmac);
> +
> +static struct attribute *auxmac_attributes[] = {
> +	&dev_attr_auxmac.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group auxmac_attr_group = {
> +	.attrs = auxmac_attributes,
> +};
> +
>  /* --------------------------------------------------------------------- */
>  
>  static struct attribute *tpacpi_driver_attributes[] = {
> @@ -10843,6 +10919,7 @@ static const struct attribute_group *tpacpi_groups[] = {
>  	&proxsensor_attr_group,
>  	&kbdlang_attr_group,
>  	&dprc_attr_group,
> +	&auxmac_attr_group,
>  	NULL,
>  };
>  
> @@ -11414,6 +11491,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
>  		.init = tpacpi_dprc_init,
>  		.data = &dprc_driver_data,
>  	},
> +	{
> +		.init = auxmac_init,
> +		.data = &auxmac_data,
> +	},
>  };
>  
>  static int __init set_ibm_param(const char *val, const struct kernel_param *kp)

Re: [ibm-acpi-devel] [PATCH v4] platform/x86: thinkpad_acpi: sysfs interface to auxmac

[Hans de G. <hde...@re...>]

Hi,

On 9/21/23 16:36, Fernando Eckhardt Valle wrote:
> Newer Thinkpads have a feature called MAC Address Pass-through.
> This patch provides a sysfs interface that userspace can use
> to get this auxiliary mac address.
> 
> Signed-off-by: Fernando Eckhardt Valle <fe...@ip...>
> ---
> Changes in v4:
> - strscpy() in all string copies.
> Changes in v3:
> - Added null terminator to auxmac string when copying auxiliary
> mac address value.
> Changes in v2:
> - Added documentation.
> - All handling of the auxmac value is done in the _init function.
> ---
>  .../admin-guide/laptops/thinkpad-acpi.rst     | 20 +++++
>  drivers/platform/x86/thinkpad_acpi.c          | 79 +++++++++++++++++++
>  2 files changed, 99 insertions(+)
> 
> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> index e27a1c3f6..98d304010 100644
> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> @@ -53,6 +53,7 @@ detailed description):
>  	- Lap mode sensor
>  	- Setting keyboard language
>  	- WWAN Antenna type
> +	- Auxmac
>  
>  A compatibility table by model and feature is maintained on the web
>  site, http://ibm-acpi.sf.net/. I appreciate any success or failure
> @@ -1511,6 +1512,25 @@ Currently 2 antenna types are supported as mentioned below:
>  The property is read-only. If the platform doesn't have support the sysfs
>  class is not created.
>  
> +Auxmac
> +------
> +
> +sysfs: auxmac
> +
> +Some newer Thinkpads have a feature called MAC Address Pass-through. This
> +feature is implemented by the system firmware to provide a system unique MAC,
> +that can override a dock or USB ethernet dongle MAC, when connected to a
> +network. This property enables user-space to easily determine the MAC address
> +if the feature is enabled.
> +
> +The values of this auxiliary MAC are:
> +
> +        cat /sys/devices/platform/thinkpad_acpi/auxmac
> +
> +If the feature is disabled, the value will be 'disabled'.
> +
> +This property is read-only.
> +
>  Adaptive keyboard
>  -----------------
>  
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index d70c89d32..f430cc9ed 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -10785,6 +10785,80 @@ static struct ibm_struct dprc_driver_data = {
>  	.name = "dprc",
>  };
>  
> +/*
> + * Auxmac
> + *
> + * This auxiliary mac address is enabled in the bios through the
> + * MAC Address Pass-through feature. In most cases, there are three
> + * possibilities: Internal Mac, Second Mac, and disabled.
> + *
> + */
> +
> +#define AUXMAC_LEN 12
> +#define AUXMAC_START 9
> +#define AUXMAC_STRLEN 22
> +#define AUXMAC_BEGIN_MARKER 8
> +#define AUXMAC_END_MARKER 21
> +
> +static char auxmac[AUXMAC_LEN + 1];
> +
> +static int auxmac_init(struct ibm_init_struct *iibm)
> +{
> +	acpi_status status;
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *obj;
> +
> +	status = acpi_evaluate_object(NULL, "\\MACA", NULL, &buffer);
> +
> +	if (ACPI_FAILURE(status))
> +		return -ENODEV;
> +
> +	obj = buffer.pointer;
> +
> +	if (obj->type != ACPI_TYPE_STRING || obj->string.length != AUXMAC_STRLEN) {
> +		pr_info("Invalid buffer for MAC address pass-through.\n");
> +		strscpy(auxmac, "unavailable", AUXMAC_LEN);

Please use sizeof(auxmac) as last parameter to strscpy() here.

> +		goto auxmacinvalid;
> +	}
> +
> +	if (obj->string.pointer[AUXMAC_BEGIN_MARKER] != '#' ||
> +	    obj->string.pointer[AUXMAC_END_MARKER] != '#') {
> +		pr_info("Invalid header for MAC address pass-through.\n");
> +		strscpy(auxmac, "unavailable", AUXMAC_LEN);

Please use sizeof(auxmac) as last parameter to strscpy() here.


> +		goto auxmacinvalid;
> +	}
> +
> +	if (strncmp(obj->string.pointer + AUXMAC_START, "XXXXXXXXXXXX", AUXMAC_LEN) != 0)
> +		strscpy(auxmac, obj->string.pointer + AUXMAC_START, AUXMAC_LEN + 1);

Please use sizeof(auxmac) as last parameter to strscpy() here.

> +	else
> +		strscpy(auxmac, "disabled", AUXMAC_START);

Please use sizeof(auxmac) as last parameter to strscpy() here.

(using AUXMAC_START here really makes no sense at all)

Regards,

Hans



> +
> +auxmacinvalid:
> +	kfree(obj);
> +	return 0;
> +}
> +
> +static struct ibm_struct auxmac_data = {
> +	.name = "auxmac",
> +};
> +
> +static ssize_t auxmac_show(struct device *dev,
> +			   struct device_attribute *attr,
> +			   char *buf)
> +{
> +	return sysfs_emit(buf, "%s\n", auxmac);
> +}
> +static DEVICE_ATTR_RO(auxmac);
> +
> +static struct attribute *auxmac_attributes[] = {
> +	&dev_attr_auxmac.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group auxmac_attr_group = {
> +	.attrs = auxmac_attributes,
> +};
> +
>  /* --------------------------------------------------------------------- */
>  
>  static struct attribute *tpacpi_driver_attributes[] = {
> @@ -10843,6 +10917,7 @@ static const struct attribute_group *tpacpi_groups[] = {
>  	&proxsensor_attr_group,
>  	&kbdlang_attr_group,
>  	&dprc_attr_group,
> +	&auxmac_attr_group,
>  	NULL,
>  };
>  
> @@ -11414,6 +11489,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
>  		.init = tpacpi_dprc_init,
>  		.data = &dprc_driver_data,
>  	},
> +	{
> +		.init = auxmac_init,
> +		.data = &auxmac_data,
> +	},
>  };
>  
>  static int __init set_ibm_param(const char *val, const struct kernel_param *kp)

Re: [ibm-acpi-devel] WARNING at drivers/acpi/platform_profile.c:74 in platform_profile_show()

[Mark P. <mpe...@sq...>]

Hi Hans

On Mon, Sep 25, 2023, at 4:57 AM, Hans de Goede wrote:
<snip>
>
> Mark, can you please take a look at this (it is a thinkpad_acpi dytc issue)?
Ack

<snip>
> Regards,
>
> Hans
>
> p.s.
>
> Mark, maybe should add you to the MAINTAINERS section for thinkpad_acpi ?
Happy to do so if that makes sense and is OK with everyone. 
I assume I just push a patch proposing that change, or is there some nomination process?