Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <262060af-2c66-05c0-fbe4-cb452d807638@redhat.com>
Date:   Wed, 12 Jul 2023 20:04:33 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH 2/2] platform/x86: simatic-ipc: add auto-loading of hwmon
 modules
Content-Language: en-US, nl
To:     Henning Schild <henning.schild@siemens.com>,
        Mark Gross <markgross@kernel.org>,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
Cc:     Tobias Schaffner <tobias.schaffner@siemens.com>,
        Gerd Haeussler <gerd.haeussler.ext@siemens.com>
References: <20230711120842.30044-1-henning.schild@siemens.com>
 <20230711120842.30044-3-henning.schild@siemens.com>
 <20230712092147.061009b2@md1za8fc.ad001.siemens.net>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20230712092147.061009b2@md1za8fc.ad001.siemens.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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);
>>  }
>>  
>