diff mbox series

[3/3] hwmon: (oxp-sensors) Refactor init() and remove probe()

Message ID 20230717124013.38796-5-samsagax@gmail.com
State New
Headers
Series hwmon: (oxp-sensors) Refactor probe() and init() and remove devm_add_groups() |

Commit Message

Joaquín Ignacio Aramendía July 17, 2023, 12:40 p.m. UTC 
  Since the driver is not hotpluggable the probe() funtion is not used
more than once.

Move all attribute registration logic to the init() function.
---
 drivers/hwmon/oxp-sensors.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

Comments

Greg KH July 17, 2023, 1:45 p.m. UTC | #1 
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.

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
Guenter Roeck July 17, 2023, 3:02 p.m. UTC | #2 
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
Joaquín Ignacio Aramendía July 17, 2023, 4:40 p.m. UTC | #3 
Hello Guenter and Greg:

> > Again, as in patch 2/3, you forgot a signed-off-by line.

Will resubmit with proper Sign-off

> > 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?

The attribute is created in the same place as before this patch. And
works the same as before this patch.

I can drop this patch and only resubmit 1 and 2. Thanks for the review
to both of you.
diff mbox series

Patch

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);
+	if (IS_ERR(hwdev)) {
+		platform_device_unregister(oxp_platform_device);
+		platform_driver_unregister(&oxp_platform_driver);
+		return PTR_ERR(hwdev);
+	}
+
+	return 0;
 }
 
 static void __exit oxp_platform_exit(void)