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

[Greg KH]

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