Re: [PATCH v1 2/5] misc: amd-sbi: Add support for SB-RMI over I3C

[Krzysztof Kozlowski <krzk@kernel.org>]

On 28/07/2025 08:10, Akshay Gupta wrote:
> AMD EPYC platforms with zen5 and later support APML(SB-RMI)
> connection to the BMC over I3C bus for improved efficiency.
> Add the same functionality supported by rmi-i2c.c over I3C bus.
> 
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> Signed-off-by: Akshay Gupta <akshay.gupta@amd.com>
> ---
>  drivers/misc/amd-sbi/Kconfig   |  15 +++-
>  drivers/misc/amd-sbi/Makefile  |   2 +
>  drivers/misc/amd-sbi/rmi-i3c.c | 133 +++++++++++++++++++++++++++++++++
>  3 files changed, 148 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/misc/amd-sbi/rmi-i3c.c
> 
> diff --git a/drivers/misc/amd-sbi/Kconfig b/drivers/misc/amd-sbi/Kconfig
> index 9b1abeb6ab1a..838cf98805d9 100644
> --- a/drivers/misc/amd-sbi/Kconfig
> +++ b/drivers/misc/amd-sbi/Kconfig
> @@ -15,10 +15,21 @@ config AMD_SBRMI_I2C
>  	  This driver can also be built as a module. If so, the module will
>  	  be called sbrmi-i2c.
>  
> +config AMD_SBRMI_I3C
> +	tristate "AMD side band RMI support over I3C"
> +	depends on I3C
> +	select AMD_SBRMI
> +	select REGMAP_I3C
> +	help
> +	  Side band RMI over I3C support for AMD out of band management.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called sbrmi-i3c.
> +
>  config AMD_SBRMI_HWMON
>  	bool "SBRMI hardware monitoring"
> -	depends on AMD_SBRMI_I2C && HWMON
> -	depends on !(AMD_SBRMI_I2C=y && HWMON=m)
> +	depends on (AMD_SBRMI_I2C || AMD_SBRMI_I3C) && HWMON
> +	depends on !(AMD_SBRMI_I2C=y && HWMON=m) || !(AMD_SBRMI_I3C=y && HWMON=m)
>  	help
>  	  This provides support for RMI device hardware monitoring. If enabled,
>  	  a hardware monitoring device will be created for each socket in
> diff --git a/drivers/misc/amd-sbi/Makefile b/drivers/misc/amd-sbi/Makefile
> index 6f3090fb9ff3..e43d4058a0f0 100644
> --- a/drivers/misc/amd-sbi/Makefile
> +++ b/drivers/misc/amd-sbi/Makefile
> @@ -4,3 +4,5 @@ sbrmi_core-y			:= rmi-core.o
>  obj-$(CONFIG_AMD_SBRMI_HWMON)	+= rmi-hwmon.o
>  sbrmi-i2c-y			:= rmi-i2c.o
>  obj-$(CONFIG_AMD_SBRMI_I2C)	+= sbrmi-i2c.o
> +sbrmi-i3c-y  			:= rmi-i3c.o
> +obj-$(CONFIG_AMD_SBRMI_I3C)	+= sbrmi-i3c.o
> diff --git a/drivers/misc/amd-sbi/rmi-i3c.c b/drivers/misc/amd-sbi/rmi-i3c.c
> new file mode 100644
> index 000000000000..b960743afad1
> --- /dev/null
> +++ b/drivers/misc/amd-sbi/rmi-i3c.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * rmi-i3c.c - Side band RMI over I3C support for AMD out
> + *             of band management
> + *
> + * Copyright (C) 2025 Advanced Micro Devices, Inc.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i3c/device.h>
> +#include <linux/i3c/master.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>

Where do you use it?

> +#include <linux/regmap.h>
> +#include "rmi-core.h"


...

> +static int sbrmi_i3c_probe(struct i3c_device *i3cdev)
> +{
> +	struct device *dev = &i3cdev->dev;
> +	struct sbrmi_data *data;
> +	struct regmap_config sbrmi_i3c_regmap_config = {
> +		.reg_bits = 8,
> +		.val_bits = 8,
> +	};
> +	int ret;
> +
> +	data = devm_kzalloc(dev, sizeof(struct sbrmi_data), GFP_KERNEL);

sizeof(*). Use recent coding style, not some old, downstream code.

> +	if (!data)
> +		return -ENOMEM;
> +
> +	mutex_init(&data->lock);
> +
> +	data->regmap = devm_regmap_init_i3c(i3cdev, &sbrmi_i3c_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return PTR_ERR(data->regmap);
> +
> +	/* Enable alert for SB-RMI sequence */
> +	ret = sbrmi_enable_alert(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Cache maximum power limit */
> +	ret = sbrmi_get_max_pwr_limit(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * AMD APML I3C devices support static address
> +	 */
> +	if (i3cdev->desc->info.static_addr)
> +		data->dev_static_addr = i3cdev->desc->info.static_addr;
> +	else
> +		data->dev_static_addr = i3cdev->desc->info.dyn_addr;
> +
> +	dev_set_drvdata(dev, data);
> +
> +	ret = create_hwmon_sensor_device(dev, data);
> +	if (ret < 0)
> +		return ret;
> +	return create_misc_rmi_device(data, dev);
> +}
> +
> +static void sbrmi_i3c_remove(struct i3c_device *i3cdev)
> +{
> +	struct sbrmi_data *data = dev_get_drvdata(&i3cdev->dev);
> +
> +	misc_deregister(&data->sbrmi_misc_dev);
> +	/* Assign fops and parent of misc dev to NULL */
> +	data->sbrmi_misc_dev.fops = NULL;
> +	data->sbrmi_misc_dev.parent = NULL;
> +	dev_info(&i3cdev->dev, "Removed sbrmi-i3c driver\n");

No, drop. This is useless.



Best regards,
Krzysztof