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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <039f9a44-80df-4795-b18d-5fc9c488ca37@t-8ch.de>
Date: Mon, 28 Jul 2025 08:37:55 +0200
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Akshay Gupta <akshay.gupta@....com>
Cc: linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org, 
	gregkh@...uxfoundation.org, arnd@...db.de, linux@...ck-us.net, Anand.Umarji@....com, 
	Naveen Krishna Chatradhi <naveenkrishna.chatradhi@....com>
Subject: Re: [PATCH v1 2/5] misc: amd-sbi: Add support for SB-RMI over I3C

On 2025-07-28 06:10:30+0000, Akshay Gupta wrote:
(...)

> 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>

Unnecessary include.

> +#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>

Ditto.

> +#include <linux/regmap.h>
> +#include "rmi-core.h"
> +
> +static int sbrmi_enable_alert(struct sbrmi_data *data)
> +{
> +	int ctrl, ret;
> +
> +	/*
> +	 * Enable the SB-RMI Software alert status
> +	 * by writing 0 to bit 4 of Control register(0x1)
> +	 */
> +	ret = regmap_read(data->regmap, SBRMI_CTRL, &ctrl);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ctrl & 0x10) {

Magic value? Use a define.

> +		ctrl &= ~0x10;
> +		return regmap_write(data->regmap, SBRMI_CTRL, ctrl);
> +	}
> +
> +	return 0;
> +}
> +
> +static int sbrmi_get_max_pwr_limit(struct sbrmi_data *data)
> +{
> +	struct apml_mbox_msg msg = { 0 };
> +	int ret;
> +
> +	msg.cmd = SBRMI_READ_PKG_MAX_PWR_LIMIT;
> +	ret = rmi_mailbox_xfer(data, &msg);
> +	if (ret < 0)
> +		return ret;
> +	data->pwr_limit_max = msg.mb_in_out;
> +
> +	return ret;
> +}
> +
> +static int sbrmi_i3c_probe(struct i3c_device *i3cdev)
> +{
> +	struct device *dev = &i3cdev->dev;

i3cdev_to_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);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	mutex_init(&data->lock);

devm_mutex_init().

> +
> +	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);

If you get rid of _remove(), then this can go away.

> +
> +	ret = create_hwmon_sensor_device(dev, data);

That's a very generic name to have exported.

> +	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);

create_misc_rmi_device() could use devm_add_action_or_reset() for the
misc deregister, simplifying the drivers code.

> +	/* Assign fops and parent of misc dev to NULL */
> +	data->sbrmi_misc_dev.fops = NULL;
> +	data->sbrmi_misc_dev.parent = NULL;

Why are these two needed? The data is freed anyways right after.

> +	dev_info(&i3cdev->dev, "Removed sbrmi-i3c driver\n");

Unnecessary.

> +}
> +
> +static const struct i3c_device_id sbrmi_i3c_id[] = {
> +	/* PID for AMD SBRMI device */
> +	I3C_DEVICE_EXTRA_INFO(0x112, 0x0, 0x2, NULL),
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i3c, sbrmi_i3c_id);
> +
> +static struct i3c_driver sbrmi_i3c_driver = {
> +	.driver = {
> +		.name = "sbrmi-i3c",
> +	},
> +	.probe = sbrmi_i3c_probe,
> +	.remove = sbrmi_i3c_remove,
> +	.id_table = sbrmi_i3c_id,
> +};
> +
> +module_i3c_driver(sbrmi_i3c_driver);

You could have the i2c and i3c drivers in the same module using
module_i3c_i2c_driver().

> +
> +MODULE_IMPORT_NS("AMD_SBRMI");
> +MODULE_AUTHOR("Akshay Gupta <akshay.gupta@....com>");
> +MODULE_AUTHOR("Naveen Krishna Chatradhi <naveenkrishna.chatradhi@....com>");
> +MODULE_DESCRIPTION("AMD SB-RMI driver over I3C");
> +MODULE_LICENSE("GPL");
> -- 
> 2.25.1
>