Re: [PATCH 3/5] mfd/hwmon sbrmi: Use regmap subsystem

[Guenter Roeck <linux@roeck-us.net>]

On 5/30/24 04:23, Naveen Krishna Chatradhi wrote:
> From: Akshay Gupta <akshay.gupta@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>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>

The "hwmon" in the subject should be dropped.

> ---
>   drivers/mfd/Kconfig        |  2 +-
>   drivers/mfd/sbrmi-core.c   | 30 ++++++++++++------------------
>   drivers/mfd/sbrmi-i2c.c    | 25 ++++++++++++++++---------
>   include/linux/mfd/amd-sb.h |  6 +++---
>   4 files changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 0411cb29b6df..d89513e5a06b 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1444,8 +1444,8 @@ config MFD_STMPE
>   
>   config MFD_SBRMI_I2C
>           tristate "APML SBRMI support"
> -        depends on I2C
>           select MFD_CORE
> +        select REGMAP_I2C
>           help
>             APML RMI core support for AMD out of band management
>   	  This driver can also be built as a module. If so, the module will
> diff --git a/drivers/mfd/sbrmi-core.c b/drivers/mfd/sbrmi-core.c
> index d872b5107b36..5617b91787ba 100644
> --- a/drivers/mfd/sbrmi-core.c
> +++ b/drivers/mfd/sbrmi-core.c
> @@ -7,9 +7,9 @@
>    */
>   #include <linux/delay.h>
>   #include <linux/err.h>
> -#include <linux/i2c.h>
>   #include <linux/mfd/amd-sb.h>
>   #include <linux/mutex.h>
> +#include <linux/regmap.h>
>   
>   /* Mask for Status Register bit[1] */
>   #define SW_ALERT_MASK	0x2
> @@ -44,6 +44,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 +52,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 +68,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 +77,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 +87,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;

This is wrong. It should be
		if (ret < 0)
			goto exit_unlock;

>   			goto exit_unlock;
> @@ -102,8 +98,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 +109,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 +121,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/mfd/sbrmi-i2c.c b/drivers/mfd/sbrmi-i2c.c
> index c19f0b3eb0cd..bdf15a7a2167 100644
> --- a/drivers/mfd/sbrmi-i2c.c
> +++ b/drivers/mfd/sbrmi-i2c.c
> @@ -15,6 +15,7 @@
>   #include <linux/module.h>
>   #include <linux/mutex.h>
>   #include <linux/of.h>
> +#include <linux/regmap.h>
>   
>   #define SBRMI_CTRL	0x1
>   
> @@ -22,22 +23,21 @@ static struct mfd_cell apml_sbrmi[] = {
>   	{ .name = "sbrmi-hwmon" },
>   };
>   
> -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;
> @@ -62,17 +62,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/linux/mfd/amd-sb.h b/include/linux/mfd/amd-sb.h
> index 7805f31fb6ea..977b8228ffa1 100644
> --- a/include/linux/mfd/amd-sb.h
> +++ b/include/linux/mfd/amd-sb.h
> @@ -7,7 +7,7 @@
>   #define _AMD_SB_H_
>   
>   #include <linux/mutex.h>
> -#include <linux/i2c.h>
> +#include <linux/regmap.h>
>   /*
>    * SB-RMI supports soft mailbox service request to MP1 (power management
>    * firmware) through SBRMI inbound/outbound message registers.
> @@ -22,10 +22,10 @@ enum sbrmi_msg_id {
>   
>   /* Each client has this additional data */
>   struct sbrmi_data {
> -	struct i2c_client *client;
> +	struct regmap *regmap;
>   	struct mutex lock;
>   	u32 pwr_limit_max;
> -};
> +} __packed;
>   
>   struct sbrmi_mailbox_msg {
>   	u8 cmd;