Re: [PATCH 2/2] platform/x86: thinkpad_acpi: Move HWMON initialization to tpacpi_hwmon_pdriver's probe

["Kurt Borja" <kuurtb@gmail.com>]

Hi Mark,

On Tue Feb 18, 2025 at 11:50 AM -05, Mark Pearson wrote:
> Hi Kurt,
>
> On Fri, Feb 14, 2025, at 7:03 PM, Kurt Borja wrote:
>> Let the driver core manage the lifetime of the HWMON device, by
>> registering it inside tpacpi_hwmon_pdriver's probe and using
>> devm_hwmon_device_register_with_groups().
>>
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> ---
>>  drivers/platform/x86/thinkpad_acpi.c | 44 +++++++++++-----------------
>>  1 file changed, 17 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c 
>> b/drivers/platform/x86/thinkpad_acpi.c
>> index ad9de48cc122..a7e82157bd67 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -367,7 +367,6 @@ static struct {
>>  	u32 beep_needs_two_args:1;
>>  	u32 mixer_no_level_control:1;
>>  	u32 battery_force_primary:1;
>> -	u32 sensors_pdrv_registered:1;
>>  	u32 hotkey_poll_active:1;
>>  	u32 has_adaptive_kbd:1;
>>  	u32 kbd_lang:1;
>> @@ -11815,12 +11814,10 @@ static void thinkpad_acpi_module_exit(void)
>>  {
>>  	tpacpi_lifecycle = TPACPI_LIFE_EXITING;
>> 
>> -	if (tpacpi_hwmon)
>> -		hwmon_device_unregister(tpacpi_hwmon);
>> -	if (tp_features.sensors_pdrv_registered)
>> +	if (tpacpi_sensors_pdev) {
>>  		platform_driver_unregister(&tpacpi_hwmon_pdriver);
>> -	if (tpacpi_sensors_pdev)
>>  		platform_device_unregister(tpacpi_sensors_pdev);
>> +	}
>> 
>>  	if (tpacpi_pdev) {
>>  		platform_driver_unregister(&tpacpi_pdriver);
>> @@ -11891,6 +11888,17 @@ static int __init tpacpi_pdriver_probe(struct 
>> platform_device *pdev)
>>  	return ret;
>>  }
>> 
>> +static int __init tpacpi_hwmon_pdriver_probe(struct platform_device *pdev)
>> +{
>> +	tpacpi_hwmon = devm_hwmon_device_register_with_groups(
>> +		&tpacpi_sensors_pdev->dev, TPACPI_NAME, NULL, tpacpi_hwmon_groups);
>> +
>> +	if (IS_ERR(tpacpi_hwmon))
>> +		pr_err("unable to register hwmon device\n");
>> +
>> +	return PTR_ERR_OR_ZERO(tpacpi_hwmon);
>> +}
>> +
>>  static int __init thinkpad_acpi_module_init(void)
>>  {
>>  	const struct dmi_system_id *dmi_id;
>> @@ -11964,37 +11972,19 @@ static int __init thinkpad_acpi_module_init(void)
>>  		return ret;
>>  	}
>> 
>> -	tpacpi_sensors_pdev = platform_device_register_simple(
>> -						TPACPI_HWMON_DRVR_NAME,
>> -						PLATFORM_DEVID_NONE, NULL, 0);
>> +	tpacpi_sensors_pdev = platform_create_bundle(&tpacpi_hwmon_pdriver,
>> +						     tpacpi_hwmon_pdriver_probe,
>> +						     NULL, 0, NULL, 0);
>>  	if (IS_ERR(tpacpi_sensors_pdev)) {
>>  		ret = PTR_ERR(tpacpi_sensors_pdev);
>>  		tpacpi_sensors_pdev = NULL;
>> -		pr_err("unable to register hwmon platform device\n");
>> +		pr_err("unable to register hwmon platform device/driver bundle\n");
>>  		thinkpad_acpi_module_exit();
>>  		return ret;
>>  	}
>> 
>>  	tpacpi_lifecycle = TPACPI_LIFE_RUNNING;
>> 
>> -	ret = platform_driver_register(&tpacpi_hwmon_pdriver);
>> -	if (ret) {
>> -		pr_err("unable to register hwmon platform driver\n");
>> -		thinkpad_acpi_module_exit();
>> -		return ret;
>> -	}
>> -	tp_features.sensors_pdrv_registered = 1;
>> -
>> -	tpacpi_hwmon = hwmon_device_register_with_groups(
>> -		&tpacpi_sensors_pdev->dev, TPACPI_NAME, NULL, tpacpi_hwmon_groups);
>> -	if (IS_ERR(tpacpi_hwmon)) {
>> -		ret = PTR_ERR(tpacpi_hwmon);
>> -		tpacpi_hwmon = NULL;
>> -		pr_err("unable to register hwmon device\n");
>> -		thinkpad_acpi_module_exit();
>> -		return ret;
>> -	}
>> -
>>  	return 0;
>>  }
>> 
>> -- 
>> 2.48.1
>
> Thanks for doing this.

Glad to help :)

>
> For the series - all looks good and I tested on a X1 Carbon 12 and confirmed the Thinkpad devices are there under /sys/devices/thinkpad_acpi and /sys/class/hwmon. Didn't find any issues.
>
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>

Thank you! Making changes to this driver is a bit scary.

-- 
 ~ Kurt

>
> Mark