[3/6] misc: sbrmi: Use regmap subsystem

Commit Message

Gupta, Akshay July 4, 2024, 11:16 a.m. UTC

- 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 <[e-mail]>
Reviewed-by: Naveen Krishna Chatradhi <[e-mail]>
---
 drivers/misc/amd-sb/sbrmi-core.c | 29 ++++++++++++-----------------
 drivers/misc/amd-sb/sbrmi-i2c.c  | 25 ++++++++++++++++---------
 include/misc/amd-sb.h            |  6 +++---
 3 files changed, 31 insertions(+), 29 deletions(-)

Comments

[e-mail] July 4, 2024, 11:53 a.m. UTC | #1

On Thu, Jul 04, 2024 at 11:16:21AM +0000, Akshay Gupta wrote:
> - regmap subsystem provides multiple benefits over direct smbus APIs

In what way, please document.

> - The susbsytem can be helpful in following cases
>   - Differnet types of bus (i2c/i3c)
>   - Different Register address size (1byte/2byte)

Is that what is happening here?  I don't see i3c support...

> --- a/include/misc/amd-sb.h
> +++ b/include/misc/amd-sb.h
> @@ -7,8 +7,8 @@
>  #define _AMD_SB_H_
>  
>  #include <linux/mutex.h>
> -#include <linux/i2c.h>

Why remove this?

>  #include <linux/platform_device.h>
> +#include <linux/regmap.h>
>  /*
>   * SB-RMI supports soft mailbox service request to MP1 (power management
>   * firmware) through SBRMI inbound/outbound message registers.
> @@ -23,11 +23,11 @@ enum sbrmi_msg_id {
>  
>  /* Each client has this additional data */
>  struct sbrmi_data {
> -	struct i2c_client *client;
> +	struct regmap *regmap;
>  	struct mutex lock;
>  	struct platform_device *pdev;
>  	u32 pwr_limit_max;
> -};
> +} __packed;

Why is this suddenly required to be __packed?

Isn't that going to cause more problems than it is worth?  And why is it
worth it at all?

thanks,

greg k-h

Gupta, Akshay July 4, 2024, 2:26 p.m. UTC | #2

On 7/4/2024 5:23 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:21AM +0000, Akshay Gupta wrote:
>> - regmap subsystem provides multiple benefits over direct smbus APIs
> In what way, please document.
Answered below.
>
>> - The susbsytem can be helpful in following cases
>>    - Differnet types of bus (i2c/i3c)
>>    - Different Register address size (1byte/2byte)
> Is that what is happening here?  I don't see i3c support...

This is a preparation for

1. I3C support: planning to add support, on AMD platforms BMC can 
communicate with SBRMI over I2C/I3C

2. patch 6/6 in patch series, is adding support for 2 bytes SBRMI 
register address size

>> --- a/include/misc/amd-sb.h
>> +++ b/include/misc/amd-sb.h
>> @@ -7,8 +7,8 @@
>>   #define _AMD_SB_H_
>>
>>   #include <linux/mutex.h>
>> -#include <linux/i2c.h>
> Why remove this?
In this patch "struct i2c_client" is removed, so no need for the header 
inclusion.
>
>>   #include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>>   /*
>>    * SB-RMI supports soft mailbox service request to MP1 (power management
>>    * firmware) through SBRMI inbound/outbound message registers.
>> @@ -23,11 +23,11 @@ enum sbrmi_msg_id {
>>
>>   /* Each client has this additional data */
>>   struct sbrmi_data {
>> -     struct i2c_client *client;
>> +     struct regmap *regmap;
>>        struct mutex lock;
>>        struct platform_device *pdev;
>>        u32 pwr_limit_max;
>> -};
>> +} __packed;
> Why is this suddenly required to be __packed?
>
> Isn't that going to cause more problems than it is worth?  And why is it
> worth it at all?
I will look into this and update.
>
> thanks,
>
> greg k-h

Patch

diff --git a/drivers/misc/amd-sb/sbrmi-core.c b/drivers/misc/amd-sb/sbrmi-core.c
index 5ca0a9ec8341..b2437a4044ac 100644
--- a/drivers/misc/amd-sb/sbrmi-core.c
+++ b/drivers/misc/amd-sb/sbrmi-core.c
@@ -9,6 +9,7 @@ 
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/mutex.h>
+#include <linux/regmap.h>
 #include <misc/amd-sb.h>
 
 /* Mask for Status Register bit[1] */
@@ -44,6 +45,7 @@  enum sbrmi_reg {
 int rmi_mailbox_xfer(struct sbrmi_data *data,
 		     struct sbrmi_mailbox_msg *msg)
 {
+	unsigned int bytes;
 	int i, ret, retry = 10;
 	int sw_status;
 	u8 byte;
@@ -51,14 +53,12 @@  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;
 
@@ -69,8 +69,7 @@  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;
 	}
@@ -79,8 +78,7 @@  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;
 
@@ -90,8 +88,7 @@  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;
@@ -102,8 +99,6 @@  int rmi_mailbox_xfer(struct sbrmi_data *data,
 	} while (retry--);
 
 	if (retry < 0) {
-		dev_err(&data->client->dev,
-			"Firmware fail to indicate command completion\n");
 		ret = -EIO;
 		goto exit_unlock;
 	}
@@ -115,11 +110,11 @@  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;
 		}
 	}
 
@@ -127,8 +122,8 @@  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);
diff --git a/drivers/misc/amd-sb/sbrmi-i2c.c b/drivers/misc/amd-sb/sbrmi-i2c.c
index b593bbdd78e0..539539c42754 100644
--- a/drivers/misc/amd-sb/sbrmi-i2c.c
+++ b/drivers/misc/amd-sb/sbrmi-i2c.c
@@ -13,26 +13,26 @@ 
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/regmap.h>
 #include <misc/amd-sb.h>
 
 #define SBRMI_CTRL	0x1
 
-static int sbrmi_enable_alert(struct i2c_client *client)
+static int sbrmi_enable_alert(struct sbrmi_data *data)
 {
-	int ctrl;
+	int ctrl, ret;
 
 	/*
 	 * 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_CTRL, &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;
@@ -57,17 +57,24 @@  static int sbrmi_i2c_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->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);
 	if (!data)
 		return -ENOMEM;
 
-	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;
 
diff --git a/include/misc/amd-sb.h b/include/misc/amd-sb.h
index 79b76dd6068a..71abb3035f48 100644
--- a/include/misc/amd-sb.h
+++ b/include/misc/amd-sb.h
@@ -7,8 +7,8 @@ 
 #define _AMD_SB_H_
 
 #include <linux/mutex.h>
-#include <linux/i2c.h>
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
 /*
  * SB-RMI supports soft mailbox service request to MP1 (power management
  * firmware) through SBRMI inbound/outbound message registers.
@@ -23,11 +23,11 @@  enum sbrmi_msg_id {
 
 /* Each client has this additional data */
 struct sbrmi_data {
-	struct i2c_client *client;
+	struct regmap *regmap;
 	struct mutex lock;
 	struct platform_device *pdev;
 	u32 pwr_limit_max;
-};
+} __packed;
 
 struct sbrmi_mailbox_msg {
 	u8 cmd;