Re: [PATCH 2/2] platform/x86: simatic-ipc: add auto-loading of hwmon modules

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 7/12/23 09:21, Henning Schild wrote:
> Am Tue, 11 Jul 2023 14:08:42 +0200
> schrieb Henning Schild <henning.schild@siemens.com>:
> 
>> In order to know which hwmon modules to load one would have to usually
>> first probe from user-land i.e. with sensors-detect and create a
>> config for each machine. But here we know exactly what machines we
>> are dealing with, so we can request those howmon modules without
>> user-mode detection and config files.
>>
>> Signed-off-by: Henning Schild <henning.schild@siemens.com>
>> ---
>>  drivers/platform/x86/simatic-ipc.c | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/platform/x86/simatic-ipc.c
>> b/drivers/platform/x86/simatic-ipc.c index 71487216d33f..403dc231bef7
>> 100644 --- a/drivers/platform/x86/simatic-ipc.c
>> +++ b/drivers/platform/x86/simatic-ipc.c
>> @@ -153,6 +153,21 @@ static int register_platform_devices(u32
>> station_id) return 0;
>>  }
>>  
>> +static void request_additional_modules(u32 station_id)
>> +{
>> +	switch (station_id) {
>> +	case SIMATIC_IPC_IPC227G:
>> +	case SIMATIC_IPC_IPC277G:
>> +	case SIMATIC_IPC_IPCBX_39A:
>> +	case SIMATIC_IPC_IPCPX_39A:
>> +		request_module("nct6775");
>> +		break;
>> +	default:
>> +		request_module("emc1403");
>> +		break;
> 
> This one will be hard to maintain since every new model would choose
> the default path. Requesting emc1403 on a device where that would not
> do anything is not a problem, but still. And people might forget to
> even look at this and maybe name a module that should be used instead.
> 
> I will send a v2 where an array of module names will become part of
> device_modes. The array would hold all additional modules which do not
> autoload. Then the module w83627hf_wdt used for some models can also be
> part of that and no longer be modeled with wdtmode.
> 
> Should anyone have objections on the whole idea of requesting additional
> modules, please already speak up.

This sounds like a good plan to me, no objections from my side.

Regards,

Hans




>> +	}
>> +}
>> +
>>  static int __init simatic_ipc_init_module(void)
>>  {
>>  	const struct dmi_system_id *match;
>> @@ -170,6 +185,8 @@ static int __init simatic_ipc_init_module(void)
>>  		return 0;
>>  	}
>>  
>> +	request_additional_modules(station_id);
>> +
>>  	return register_platform_devices(station_id);
>>  }
>>  
>