| Message ID | 20240502220554.2768611-1-nchatrad@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] sbrmi: Use regmap subsystem | expand |
On 5/2/24 15:05, Naveen Krishna Chatradhi wrote: > From: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com> > > - regmap subsystem provides multiple benefits over direct smbus APIs > - The susbsytem can be helpful in following cases > - Differnet types of bus (i2c/i3c) > - Different Register address size (1byte/2byte) > > Signed-off-by: Akshay Gupta <Akshay.Gupta@amd.com> > Signed-off-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com> > --- > drivers/hwmon/Kconfig | 1 + > drivers/hwmon/sbrmi.c | 61 ++++++++++++++++++++++++------------------- > 2 files changed, 35 insertions(+), 27 deletions(-) > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index a608264da87d..903a8ebbd2d7 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1759,6 +1759,7 @@ config SENSORS_SBTSI > config SENSORS_SBRMI > tristate "Emulated SB-RMI sensor" > depends on I2C > + select REGMAP_I2C > help > If you say yes here you get support for emulated RMI > sensors on AMD SoCs with APML interface connected to a BMC device. > diff --git a/drivers/hwmon/sbrmi.c b/drivers/hwmon/sbrmi.c > index 484703f0ea5f..901bd82d71d4 100644 > --- a/drivers/hwmon/sbrmi.c > +++ b/drivers/hwmon/sbrmi.c > @@ -14,6 +14,7 @@ > #include <linux/module.h> > #include <linux/mutex.h> > #include <linux/of.h> > +#include <linux/regmap.h> > > /* Do not allow setting negative power limit */ > #define SBRMI_PWR_MIN 0 > @@ -63,6 +64,7 @@ enum sbrmi_reg { > struct sbrmi_data { > struct i2c_client *client; There is only one user of this structure left, and it is for a questionable error message (questionable since all other errors are handled silently). I would suggest to drop both. > struct mutex lock; > + struct regmap *regmap; > u32 pwr_limit_max; > }; > > @@ -73,22 +75,21 @@ struct sbrmi_mailbox_msg { > u32 data_out; > }; > > -static int sbrmi_enable_alert(struct i2c_client *client) > +static int sbrmi_enable_alert(struct sbrmi_data *data) > { > - int ctrl; > + int ctrl, ret = 0; > Unnecessary initialization > /* > * Enable the SB-RMI Software alert status > * by writing 0 to bit 4 of Control register(0x1) > */ > - ctrl = i2c_smbus_read_byte_data(client, SBRMI_CTRL); > - if (ctrl < 0) > - return ctrl; > + ret = regmap_read(data->regmap, SBRMI_STATUS, &ctrl); > + if (ret < 0) > + return ret; > > if (ctrl & 0x10) { > ctrl &= ~0x10; > - return i2c_smbus_write_byte_data(client, > - SBRMI_CTRL, ctrl); > + return regmap_write(data->regmap, SBRMI_CTRL, ctrl); > } > > return 0; > @@ -97,6 +98,7 @@ static int sbrmi_enable_alert(struct i2c_client *client) > static int rmi_mailbox_xfer(struct sbrmi_data *data, > struct sbrmi_mailbox_msg *msg) > { > + unsigned int bytes = 0; Unnecessary initialization > int i, ret, retry = 10; > int sw_status; > u8 byte; > @@ -104,14 +106,12 @@ static int rmi_mailbox_xfer(struct sbrmi_data *data, > mutex_lock(&data->lock); > > /* Indicate firmware a command is to be serviced */ > - ret = i2c_smbus_write_byte_data(data->client, > - SBRMI_INBNDMSG7, START_CMD); > + ret = regmap_write(data->regmap, SBRMI_INBNDMSG7, START_CMD); > if (ret < 0) > goto exit_unlock; > > /* Write the command to SBRMI::InBndMsg_inst0 */ > - ret = i2c_smbus_write_byte_data(data->client, > - SBRMI_INBNDMSG0, msg->cmd); > + ret = regmap_write(data->regmap, SBRMI_INBNDMSG0, msg->cmd); > if (ret < 0) > goto exit_unlock; > > @@ -122,8 +122,7 @@ static int rmi_mailbox_xfer(struct sbrmi_data *data, > */ > for (i = 0; i < 4; i++) { > byte = (msg->data_in >> i * 8) & 0xff; > - ret = i2c_smbus_write_byte_data(data->client, > - SBRMI_INBNDMSG1 + i, byte); > + ret = regmap_write(data->regmap, SBRMI_INBNDMSG1 + i, byte); > if (ret < 0) > goto exit_unlock; > } > @@ -132,8 +131,7 @@ static int rmi_mailbox_xfer(struct sbrmi_data *data, > * Write 0x01 to SBRMI::SoftwareInterrupt to notify firmware to > * perform the requested read or write command > */ > - ret = i2c_smbus_write_byte_data(data->client, > - SBRMI_SW_INTERRUPT, TRIGGER_MAILBOX); > + ret = regmap_write(data->regmap, SBRMI_SW_INTERRUPT, TRIGGER_MAILBOX); > if (ret < 0) > goto exit_unlock; > > @@ -143,8 +141,7 @@ static int rmi_mailbox_xfer(struct sbrmi_data *data, > * of the requested command > */ > do { > - sw_status = i2c_smbus_read_byte_data(data->client, > - SBRMI_STATUS); > + ret = regmap_read(data->regmap, SBRMI_STATUS, &sw_status); > if (sw_status < 0) { > ret = sw_status; > goto exit_unlock; > @@ -168,11 +165,11 @@ static int rmi_mailbox_xfer(struct sbrmi_data *data, > */ > if (msg->read) { > for (i = 0; i < 4; i++) { > - ret = i2c_smbus_read_byte_data(data->client, > - SBRMI_OUTBNDMSG1 + i); > + ret = regmap_read(data->regmap, > + SBRMI_OUTBNDMSG1 + i, &bytes); > if (ret < 0) > goto exit_unlock; > - msg->data_out |= ret << i * 8; > + msg->data_out |= bytes << i * 8; > } > } > > @@ -180,9 +177,8 @@ static int rmi_mailbox_xfer(struct sbrmi_data *data, > * BMC must write 1'b1 to SBRMI::Status[SwAlertSts] to clear the > * ALERT to initiator > */ > - ret = i2c_smbus_write_byte_data(data->client, SBRMI_STATUS, > - sw_status | SW_ALERT_MASK); > - > + ret = regmap_write(data->regmap, SBRMI_STATUS, > + sw_status | SW_ALERT_MASK); > exit_unlock: > mutex_unlock(&data->lock); > return ret; > @@ -265,7 +261,7 @@ static umode_t sbrmi_is_visible(const void *data, > return 0; > } > > -static const struct hwmon_channel_info * const sbrmi_info[] = { > +static const struct hwmon_channel_info *sbrmi_info[] = { > HWMON_CHANNEL_INFO(power, > HWMON_P_INPUT | HWMON_P_CAP | HWMON_P_CAP_MAX), > NULL > @@ -302,6 +298,10 @@ static int sbrmi_probe(struct i2c_client *client) > struct device *dev = &client->dev; > struct device *hwmon_dev; > struct sbrmi_data *data; > + struct regmap_config sbrmi_i2c_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + }; > int ret; > > data = devm_kzalloc(dev, sizeof(struct sbrmi_data), GFP_KERNEL); > @@ -310,9 +310,12 @@ static int sbrmi_probe(struct i2c_client *client) > > data->client = client; > mutex_init(&data->lock); > + data->regmap = devm_regmap_init_i2c(client, &sbrmi_i2c_regmap_config); > + if (IS_ERR(data->regmap)) > + return PTR_ERR(data->regmap); > > /* Enable alert for SB-RMI sequence */ > - ret = sbrmi_enable_alert(client); > + ret = sbrmi_enable_alert(data); > if (ret < 0) > return ret; > > @@ -321,8 +324,12 @@ static int sbrmi_probe(struct i2c_client *client) > if (ret < 0) > return ret; > > - hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data, > - &sbrmi_chip_info, NULL); > + dev_set_drvdata(dev, (void *)data); > + Why would this be needed ? On a side note, the typecast is unnecessary. Guenter
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index a608264da87d..903a8ebbd2d7 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1759,6 +1759,7 @@ config SENSORS_SBTSI config SENSORS_SBRMI tristate "Emulated SB-RMI sensor" depends on I2C + select REGMAP_I2C help If you say yes here you get support for emulated RMI sensors on AMD SoCs with APML interface connected to a BMC device. diff --git a/drivers/hwmon/sbrmi.c b/drivers/hwmon/sbrmi.c index 484703f0ea5f..901bd82d71d4 100644 --- a/drivers/hwmon/sbrmi.c +++ b/drivers/hwmon/sbrmi.c @@ -14,6 +14,7 @@ #include <linux/module.h> #include <linux/mutex.h> #include <linux/of.h> +#include <linux/regmap.h> /* Do not allow setting negative power limit */ #define SBRMI_PWR_MIN 0 @@ -63,6 +64,7 @@ enum sbrmi_reg { struct sbrmi_data { struct i2c_client *client; struct mutex lock; + struct regmap *regmap; u32 pwr_limit_max; }; @@ -73,22 +75,21 @@ struct sbrmi_mailbox_msg { u32 data_out; }; -static int sbrmi_enable_alert(struct i2c_client *client) +static int sbrmi_enable_alert(struct sbrmi_data *data) { - int ctrl; + int ctrl, ret = 0; /* * Enable the SB-RMI Software alert status * by writing 0 to bit 4 of Control register(0x1) */ - ctrl = i2c_smbus_read_byte_data(client, SBRMI_CTRL); - if (ctrl < 0) - return ctrl; + ret = regmap_read(data->regmap, SBRMI_STATUS, &ctrl); + if (ret < 0) + return ret; if (ctrl & 0x10) { ctrl &= ~0x10; - return i2c_smbus_write_byte_data(client, - SBRMI_CTRL, ctrl); + return regmap_write(data->regmap, SBRMI_CTRL, ctrl); } return 0; @@ -97,6 +98,7 @@ static int sbrmi_enable_alert(struct i2c_client *client) static int rmi_mailbox_xfer(struct sbrmi_data *data, struct sbrmi_mailbox_msg *msg) { + unsigned int bytes = 0; int i, ret, retry = 10; int sw_status; u8 byte; @@ -104,14 +106,12 @@ static int rmi_mailbox_xfer(struct sbrmi_data *data, mutex_lock(&data->lock); /* Indicate firmware a command is to be serviced */ - ret = i2c_smbus_write_byte_data(data->client, - SBRMI_INBNDMSG7, START_CMD); + ret = regmap_write(data->regmap, SBRMI_INBNDMSG7, START_CMD); if (ret < 0) goto exit_unlock; /* Write the command to SBRMI::InBndMsg_inst0 */ - ret = i2c_smbus_write_byte_data(data->client, - SBRMI_INBNDMSG0, msg->cmd); + ret = regmap_write(data->regmap, SBRMI_INBNDMSG0, msg->cmd); if (ret < 0) goto exit_unlock; @@ -122,8 +122,7 @@ static int rmi_mailbox_xfer(struct sbrmi_data *data, */ for (i = 0; i < 4; i++) { byte = (msg->data_in >> i * 8) & 0xff; - ret = i2c_smbus_write_byte_data(data->client, - SBRMI_INBNDMSG1 + i, byte); + ret = regmap_write(data->regmap, SBRMI_INBNDMSG1 + i, byte); if (ret < 0) goto exit_unlock; } @@ -132,8 +131,7 @@ static int rmi_mailbox_xfer(struct sbrmi_data *data, * Write 0x01 to SBRMI::SoftwareInterrupt to notify firmware to * perform the requested read or write command */ - ret = i2c_smbus_write_byte_data(data->client, - SBRMI_SW_INTERRUPT, TRIGGER_MAILBOX); + ret = regmap_write(data->regmap, SBRMI_SW_INTERRUPT, TRIGGER_MAILBOX); if (ret < 0) goto exit_unlock; @@ -143,8 +141,7 @@ static int rmi_mailbox_xfer(struct sbrmi_data *data, * of the requested command */ do { - sw_status = i2c_smbus_read_byte_data(data->client, - SBRMI_STATUS); + ret = regmap_read(data->regmap, SBRMI_STATUS, &sw_status); if (sw_status < 0) { ret = sw_status; goto exit_unlock; @@ -168,11 +165,11 @@ static int rmi_mailbox_xfer(struct sbrmi_data *data, */ if (msg->read) { for (i = 0; i < 4; i++) { - ret = i2c_smbus_read_byte_data(data->client, - SBRMI_OUTBNDMSG1 + i); + ret = regmap_read(data->regmap, + SBRMI_OUTBNDMSG1 + i, &bytes); if (ret < 0) goto exit_unlock; - msg->data_out |= ret << i * 8; + msg->data_out |= bytes << i * 8; } } @@ -180,9 +177,8 @@ static int rmi_mailbox_xfer(struct sbrmi_data *data, * BMC must write 1'b1 to SBRMI::Status[SwAlertSts] to clear the * ALERT to initiator */ - ret = i2c_smbus_write_byte_data(data->client, SBRMI_STATUS, - sw_status | SW_ALERT_MASK); - + ret = regmap_write(data->regmap, SBRMI_STATUS, + sw_status | SW_ALERT_MASK); exit_unlock: mutex_unlock(&data->lock); return ret; @@ -265,7 +261,7 @@ static umode_t sbrmi_is_visible(const void *data, return 0; } -static const struct hwmon_channel_info * const sbrmi_info[] = { +static const struct hwmon_channel_info *sbrmi_info[] = { HWMON_CHANNEL_INFO(power, HWMON_P_INPUT | HWMON_P_CAP | HWMON_P_CAP_MAX), NULL @@ -302,6 +298,10 @@ static int sbrmi_probe(struct i2c_client *client) struct device *dev = &client->dev; struct device *hwmon_dev; struct sbrmi_data *data; + struct regmap_config sbrmi_i2c_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + }; int ret; data = devm_kzalloc(dev, sizeof(struct sbrmi_data), GFP_KERNEL); @@ -310,9 +310,12 @@ static int sbrmi_probe(struct i2c_client *client) data->client = client; mutex_init(&data->lock); + data->regmap = devm_regmap_init_i2c(client, &sbrmi_i2c_regmap_config); + if (IS_ERR(data->regmap)) + return PTR_ERR(data->regmap); /* Enable alert for SB-RMI sequence */ - ret = sbrmi_enable_alert(client); + ret = sbrmi_enable_alert(data); if (ret < 0) return ret; @@ -321,8 +324,12 @@ static int sbrmi_probe(struct i2c_client *client) if (ret < 0) return ret; - hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data, - &sbrmi_chip_info, NULL); + dev_set_drvdata(dev, (void *)data); + + hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, + data, + &sbrmi_chip_info, + NULL); return PTR_ERR_OR_ZERO(hwmon_dev); }