Hi Suma,
Thank you for the patch.
On 9/6/23 09:12, Suma Hegde wrote:
> From: Suma Hegde <suma.hegde@xxxxxxx>
>
> Having a separate platform device structure helps in future, to
> contain platform specific variables and other data.
>
> Signed-off-by: Suma Hegde <suma.hegde@xxxxxxx>
> Reviewed-by: Naveen Krishna Chatradhi <nchatrad@xxxxxxx>
> ---
> Changes since v1:
> 1. defined HSMP_CDEV_NAME and HSMP_DEVNODE_NAME macros
>
> drivers/platform/x86/amd/hsmp.c | 56 +++++++++++++++++++++------------
> 1 file changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
> index 31382ef52efb..94c65320bdcd 100644
> --- a/drivers/platform/x86/amd/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp.c
> @@ -47,9 +47,23 @@
> #define HSMP_INDEX_REG 0xc4
> #define HSMP_DATA_REG 0xc8
>
> -static struct semaphore *hsmp_sem;
> +#define HSMP_CDEV_NAME "hsmp_cdev"
> +#define HSMP_DEVNODE_NAME "hsmp"
>
> -static struct miscdevice hsmp_device;
> +struct hsmp_socket {
> + struct semaphore hsmp_sem;
> + u16 sock_ind;
> +};
> +
> +struct hsmp_plat_device {
> + struct miscdevice hsmp_device;
> + struct hsmp_socket *sock;
> + struct device *dev;
> +};
> +
> +static u16 num_sockets;
Overall this patch looks good to me, but since
num_sockets indicates the size of the plat_dev.sock array,
num_sockets should IMHO itself also be part of
struct hsmp_plat_device.
(and all usages of "num_sockets" should then be replaced by
"plat_dev.num_sockets")
Other then that this looks good to me.
Regards,
Hans
> +
> +static struct hsmp_plat_device plat_dev;
>
> static int amd_hsmp_rdwr(struct pci_dev *root, u32 address,
> u32 *value, bool write)
> @@ -188,6 +202,7 @@ static int validate_message(struct hsmp_message *msg)
>
> int hsmp_send_message(struct hsmp_message *msg)
> {
> + struct hsmp_socket *sock = &plat_dev.sock[msg->sock_ind];
> struct amd_northbridge *nb;
> int ret;
>
> @@ -208,14 +223,13 @@ int hsmp_send_message(struct hsmp_message *msg)
> * In SMP system timeout of 100 millisecs should
> * be enough for the previous thread to finish the operation
> */
> - ret = down_timeout(&hsmp_sem[msg->sock_ind],
> - msecs_to_jiffies(HSMP_MSG_TIMEOUT));
> + ret = down_timeout(&sock->hsmp_sem, msecs_to_jiffies(HSMP_MSG_TIMEOUT));
> if (ret < 0)
> return ret;
>
> ret = __hsmp_send_message(nb->root, msg);
>
> - up(&hsmp_sem[msg->sock_ind]);
> + up(&sock->hsmp_sem);
>
> return ret;
> }
> @@ -321,28 +335,31 @@ static int hsmp_pltdrv_probe(struct platform_device *pdev)
> {
> int i;
>
> - hsmp_sem = devm_kzalloc(&pdev->dev,
> - (amd_nb_num() * sizeof(struct semaphore)),
> - GFP_KERNEL);
> - if (!hsmp_sem)
> + plat_dev.sock = devm_kzalloc(&pdev->dev,
> + (num_sockets * sizeof(struct hsmp_socket)),
> + GFP_KERNEL);
> + if (!plat_dev.sock)
> return -ENOMEM;
> + plat_dev.dev = &pdev->dev;
>
> - for (i = 0; i < amd_nb_num(); i++)
> - sema_init(&hsmp_sem[i], 1);
> + for (i = 0; i < num_sockets; i++) {
> + sema_init(&plat_dev.sock[i].hsmp_sem, 1);
> + plat_dev.sock[i].sock_ind = i;
> + }
>
> - hsmp_device.name = "hsmp_cdev";
> - hsmp_device.minor = MISC_DYNAMIC_MINOR;
> - hsmp_device.fops = &hsmp_fops;
> - hsmp_device.parent = &pdev->dev;
> - hsmp_device.nodename = "hsmp";
> - hsmp_device.mode = 0644;
> + plat_dev.hsmp_device.name = HSMP_CDEV_NAME;
> + plat_dev.hsmp_device.minor = MISC_DYNAMIC_MINOR;
> + plat_dev.hsmp_device.fops = &hsmp_fops;
> + plat_dev.hsmp_device.parent = &pdev->dev;
> + plat_dev.hsmp_device.nodename = HSMP_DEVNODE_NAME;
> + plat_dev.hsmp_device.mode = 0644;
>
> - return misc_register(&hsmp_device);
> + return misc_register(&plat_dev.hsmp_device);
> }
>
> static void hsmp_pltdrv_remove(struct platform_device *pdev)
> {
> - misc_deregister(&hsmp_device);
> + misc_deregister(&plat_dev.hsmp_device);
> }
>
> static struct platform_driver amd_hsmp_driver = {
> @@ -358,7 +375,6 @@ static struct platform_device *amd_hsmp_platdev;
> static int __init hsmp_plt_init(void)
> {
> int ret = -ENODEV;
> - u16 num_sockets;
> int i;
>
> if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD || boot_cpu_data.x86 < 0x19) {
