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

[Guenter Roeck]

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