Message-ID: <cb57d136-b2aa-4433-8b44-c66e39b1c834@amd.com>
Date: Mon, 11 Aug 2025 16:32:11 +0530
From: "Gupta, Akshay" <Akshay.Gupta@....com>
To: Thomas Weißschuh <linux@...ssschuh.net>
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 7/28/2025 12:07 PM, Thomas Weißschuh wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> 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.
Ack, will remove. Thank you.
>> +#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.
Ack, will remove.
>
>> +#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.
Ack, will create #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();
Ack, will update.
>
>> +     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().
Will address with new patch to use guard(mutex)
>> +
>> +     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.
Will create separate patch to address this as this is used by i2c driver.
>
>> +     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.
Ack , will update.
>
>> +     /* 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.
Ack, will update.
>
>> +     dev_info(&i3cdev->dev, "Removed sbrmi-i3c driver\n");
> Unnecessary.
Ack.
>
>> +}
>> +
>> +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().

Idea was to keep I2C and I3C separate as the drivers will become complex 
with

new platform specific changes related to register address size.

I will update the existing i2c driver for now.

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

Powered by blists - more mailing lists