| Message ID | 20230717124013.38796-5-samsagax@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (oxp-sensors) Refactor probe() and init() and remove devm_add_groups() | expand |
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
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
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 --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)