| Message ID | 20240704111624.1583460-3-akshay.gupta@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | misc: add amd side-band functionality | expand |
On Thu, Jul 04, 2024 at 11:16:20AM +0000, Akshay Gupta wrote: > - AMD provides socket power information from out of band > which can be read by sensors. > - platform driver will probe drivers/hwmon/sbrmi as a platform device > and share the sbrmi device data. So you are "splitting" a real device into different ones using a platform device? THat's not ok, and an abuse of the platform api. Please use the correct one for that instead. > > Signed-off-by: Akshay Gupta <akshay.gupta@amd.com> > Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com> > --- > drivers/misc/amd-sb/sbrmi-i2c.c | 25 ++++++++++++++++++++++++- > include/misc/amd-sb.h | 2 ++ > 2 files changed, 26 insertions(+), 1 deletion(-) > > diff --git a/drivers/misc/amd-sb/sbrmi-i2c.c b/drivers/misc/amd-sb/sbrmi-i2c.c > index c4903d9e9f0f..b593bbdd78e0 100644 > --- a/drivers/misc/amd-sb/sbrmi-i2c.c > +++ b/drivers/misc/amd-sb/sbrmi-i2c.c > @@ -72,7 +72,29 @@ static int sbrmi_i2c_probe(struct i2c_client *client) > return ret; > > /* Cache maximum power limit */ > - return sbrmi_get_max_pwr_limit(data); > + ret = sbrmi_get_max_pwr_limit(data); > + if (ret < 0) > + return ret; > + > + dev_set_drvdata(dev, (void *)data); No need to cast, right? > + data->pdev = platform_device_register_data(dev, "sbrmi-hwmon", > + PLATFORM_DEVID_NONE, Yeah, that's not ok. Please do this correctly, as this is NOT a platform device, but rather a made-up one that you just created out of no where. Instead use the correct apis for that. > + data, > + sizeof(struct sbrmi_data)); > + if (IS_ERR(data->pdev)) { > + pr_err("unable to register platform device for sbrmi-hwmon\n"); > + return PTR_ERR(data->pdev); You don't need to unwind anything else here? > + } > + return ret; > +} > + > +static void sbrmi_i2c_remove(struct i2c_client *client) > +{ > + struct sbrmi_data *data = dev_get_drvdata(&client->dev); > + > + if (!data) > + return; How can that happen? thanks, greg k-h
On 7/4/2024 5:20 PM, Greg KH wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > On Thu, Jul 04, 2024 at 11:16:20AM +0000, Akshay Gupta wrote: >> - AMD provides socket power information from out of band >> which can be read by sensors. >> - platform driver will probe drivers/hwmon/sbrmi as a platform device >> and share the sbrmi device data. > So you are "splitting" a real device into different ones using a > platform device? THat's not ok, and an abuse of the platform api. > Please use the correct one for that instead. is it ok to call API, devm_hwmon_device_register_with_info() from misc driver which will require to defining the hwmon ops in here. > >> Signed-off-by: Akshay Gupta <akshay.gupta@amd.com> >> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com> >> --- >> drivers/misc/amd-sb/sbrmi-i2c.c | 25 ++++++++++++++++++++++++- >> include/misc/amd-sb.h | 2 ++ >> 2 files changed, 26 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/misc/amd-sb/sbrmi-i2c.c b/drivers/misc/amd-sb/sbrmi-i2c.c >> index c4903d9e9f0f..b593bbdd78e0 100644 >> --- a/drivers/misc/amd-sb/sbrmi-i2c.c >> +++ b/drivers/misc/amd-sb/sbrmi-i2c.c >> @@ -72,7 +72,29 @@ static int sbrmi_i2c_probe(struct i2c_client *client) >> return ret; >> >> /* Cache maximum power limit */ >> - return sbrmi_get_max_pwr_limit(data); >> + ret = sbrmi_get_max_pwr_limit(data); >> + if (ret < 0) >> + return ret; >> + >> + dev_set_drvdata(dev, (void *)data); > No need to cast, right? Yes, will update. > >> + data->pdev = platform_device_register_data(dev, "sbrmi-hwmon", >> + PLATFORM_DEVID_NONE, > Yeah, that's not ok. Please do this correctly, as this is NOT a > platform device, but rather a made-up one that you just created out of > no where. Instead use the correct apis for that. > >> + data, >> + sizeof(struct sbrmi_data)); >> + if (IS_ERR(data->pdev)) { >> + pr_err("unable to register platform device for sbrmi-hwmon\n"); >> + return PTR_ERR(data->pdev); > You don't need to unwind anything else here? Yes, not required. > > > >> + } >> + return ret; >> +} >> + >> +static void sbrmi_i2c_remove(struct i2c_client *client) >> +{ >> + struct sbrmi_data *data = dev_get_drvdata(&client->dev); >> + >> + if (!data) >> + return; > How can that happen? Its just a safe check, once we remove platform driver this will not be required. > greg k-h
diff --git a/drivers/misc/amd-sb/sbrmi-i2c.c b/drivers/misc/amd-sb/sbrmi-i2c.c index c4903d9e9f0f..b593bbdd78e0 100644 --- a/drivers/misc/amd-sb/sbrmi-i2c.c +++ b/drivers/misc/amd-sb/sbrmi-i2c.c @@ -72,7 +72,29 @@ static int sbrmi_i2c_probe(struct i2c_client *client) return ret; /* Cache maximum power limit */ - return sbrmi_get_max_pwr_limit(data); + ret = sbrmi_get_max_pwr_limit(data); + if (ret < 0) + return ret; + + dev_set_drvdata(dev, (void *)data); + data->pdev = platform_device_register_data(dev, "sbrmi-hwmon", + PLATFORM_DEVID_NONE, + data, + sizeof(struct sbrmi_data)); + if (IS_ERR(data->pdev)) { + pr_err("unable to register platform device for sbrmi-hwmon\n"); + return PTR_ERR(data->pdev); + } + return ret; +} + +static void sbrmi_i2c_remove(struct i2c_client *client) +{ + struct sbrmi_data *data = dev_get_drvdata(&client->dev); + + if (!data) + return; + platform_device_unregister(data->pdev); } static const struct i2c_device_id sbrmi_id[] = { @@ -95,6 +117,7 @@ static struct i2c_driver sbrmi_driver = { .of_match_table = of_match_ptr(sbrmi_of_match), }, .probe = sbrmi_i2c_probe, + .remove = sbrmi_i2c_remove, .id_table = sbrmi_id, }; diff --git a/include/misc/amd-sb.h b/include/misc/amd-sb.h index e1a012fcdff9..79b76dd6068a 100644 --- a/include/misc/amd-sb.h +++ b/include/misc/amd-sb.h @@ -8,6 +8,7 @@ #include <linux/mutex.h> #include <linux/i2c.h> +#include <linux/platform_device.h> /* * SB-RMI supports soft mailbox service request to MP1 (power management * firmware) through SBRMI inbound/outbound message registers. @@ -24,6 +25,7 @@ enum sbrmi_msg_id { struct sbrmi_data { struct i2c_client *client; struct mutex lock; + struct platform_device *pdev; u32 pwr_limit_max; };