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;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <89c6c7e8-0407-b6bb-7085-be11efce2524@roeck-us.net>
Date:   Mon, 17 Jul 2023 08:02:58 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Content-Language: en-US
To:     Greg KH <gregkh@linuxfoundation.org>,
        =?UTF-8?Q?Joaqu=c3=adn_Ignacio_Aramend=c3=ada?= 
        <samsagax@gmail.com>
Cc:     linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230717124013.38796-2-samsagax@gmail.com>
 <20230717124013.38796-5-samsagax@gmail.com>
 <2023071739-remedy-sloping-64f6@gregkh>
From:   Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH 3/3] hwmon: (oxp-sensors) Refactor init() and remove
 probe()
In-Reply-To: <2023071739-remedy-sloping-64f6@gregkh>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 7/17/23 06:45, Greg KH wrote:
> On Mon, Jul 17, 2023 at 09:40:06AM -0300, Joaquín Ignacio Aramendía wrote:
>> Since the driver is not hotpluggable the probe() funtion is not used
>> more than once.
>>
>> Move all attribute registration logic to the init() function.
> 
> Again, as in patch 2/3, you forgot a signed-off-by line.
> 
> But this change isn't correct, just because a device is not
> hotpluggable, does not mean it should not be using probe/release, in
> fact just the opposite, it should be using that and NOT init.
> 
> But I understand why you changed the init call in patch 2/3, that is ok,
> this isn't because:
> 
>> ---
>>   drivers/hwmon/oxp-sensors.c | 33 ++++++++++++++++-----------------
>>   1 file changed, 16 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
>> index c70d9355eeba..39de49c8a392 100644
>> --- a/drivers/hwmon/oxp-sensors.c
>> +++ b/drivers/hwmon/oxp-sensors.c
>> @@ -431,32 +431,20 @@ static const struct hwmon_chip_info oxp_ec_chip_info = {
>>   	.info = oxp_platform_sensors,
>>   };
>>   
>> -/* Initialization logic */
>> -static int oxp_platform_probe(struct platform_device *pdev)
>> -{
>> -	const struct dmi_system_id *dmi_entry;
>> -	struct device *dev = &pdev->dev;
>> -	struct device *hwdev;
>> -
>> -	hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
>> -						     &oxp_ec_chip_info, NULL);
>> -
>> -	return PTR_ERR_OR_ZERO(hwdev);
>> -}
>> -
>>   static struct platform_driver oxp_platform_driver = {
>>   	.driver = {
>>   		.name = "oxp-platform",
>>   		.dev_groups = oxp_ec_groups,
>>   	},
>> -	.probe = oxp_platform_probe,
>>   };
>>   
>>   static struct platform_device *oxp_platform_device;
>>   
>> +/* Initialization logic */
>>   static int __init oxp_platform_init(void)
>>   {
>>   	const struct dmi_system_id *dmi_entry;
>> +	struct device *hwdev;
>>   
>>   	/*
>>   	 * Have to check for AMD processor here because DMI strings are the
>> @@ -472,10 +460,21 @@ static int __init oxp_platform_init(void)
>>   	board = (enum oxp_board)(unsigned long)dmi_entry->driver_data;
>>   
>>   	oxp_platform_device =
>> -		platform_create_bundle(&oxp_platform_driver,
>> -				       oxp_platform_probe, NULL, 0, NULL, 0);
>> +		platform_create_bundle(&oxp_platform_driver, NULL, NULL, 0,
>> +				       NULL, 0);
>> +	if (IS_ERR(oxp_platform_device))
>> +		return PTR_ERR(oxp_platform_device);
>>   
>> -	return PTR_ERR_OR_ZERO(oxp_platform_device);
>> +	hwdev = devm_hwmon_device_register_with_info(&oxp_platform_device->dev,
>> +						     "oxpec", NULL,
>> +						     &oxp_ec_chip_info, NULL);
> 
> You are creating a fake platform device out of no where here, which is
> tied to nothing, which isn't ok.  Keep it in the proper device tree and
> have it be passed to you by the driver core in the probe() function.
> 

This is a system with dmi data, so it won't support devicetree. Other
than that, you are correct, this patch is definitely not a good idea
and needs to be dropped.

Thanks,
Guenter

> I think you will see that this changed where in /sys/devices/ your
> device is now, right?
> 
> 
>> +	if (IS_ERR(hwdev)) {
>> +		platform_device_unregister(oxp_platform_device);
> 
> Making fake platform devices is generally never a good idea, please
> don't do that.
> 
> thanks,
> 
> greg k-h