Re: [RFC PATCH 1/2] drivers/misc: add silex multipk driver

["Gupta, Nipun" <nipun.gupta@amd.com>]

On 12-03-2025 16:24, Krzysztof Kozlowski wrote:
> On 12/03/2025 10:54, Nipun Gupta wrote:
>> +
>>   SILICON LABS WIRELESS DRIVERS (for WFxxx series)
>>   M:	Jérôme Pouiller <jerome.pouiller@silabs.com>
>>   S:	Supported
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 56bc72c7ce4a..8c5c72c540a6 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -632,6 +632,12 @@ config MCHP_LAN966X_PCI
>>   	    - lan966x-miim (MDIO_MSCC_MIIM)
>>   	    - lan966x-switch (LAN966X_SWITCH)
>>
>> +config SILEX_MPK
>> +	tristate "Silex MultiPK driver"
>> +	depends on OF
> 
> Why can't this be compile tested?

yes will add the COMPILE_TEST

> 
> Which arch uses it? Which hardware?

It is supported on ARM. Will add ARM64 as dependent. It is supported on 
AMD versal series devices.

> 
>> +	help
>> +	  Enable Silex MultiPK support
>> +
>>   source "drivers/misc/c2port/Kconfig"
>>   source "drivers/misc/eeprom/Kconfig"
>>   source "drivers/misc/cb710/Kconfig"
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index 545aad06d088..456758b29f71 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -75,3 +75,4 @@ lan966x-pci-objs		:= lan966x_pci.o
>>   lan966x-pci-objs		+= lan966x_pci.dtbo.o
>>   obj-$(CONFIG_MCHP_LAN966X_PCI)	+= lan966x-pci.o
>>   obj-y				+= keba/
>> +obj-$(CONFIG_SILEX_MPK)		+= silex_mpk.o
>> diff --git a/drivers/misc/silex_mpk.c b/drivers/misc/silex_mpk.c
>> new file mode 100644
>> index 000000000000..e03579780761
>> --- /dev/null
>> +++ b/drivers/misc/silex_mpk.c
>> @@ -0,0 +1,860 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018-2021 Silex Insight sa
>> + * Copyright (c) 2018-2021 Beerten Engineering scs
>> + * Copyright (c) 2025 Advanced Micro Devices, Inc.
>> + */
>> +
>> +#include <linux/version.h>
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/device.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/slab.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of.h>
> 
> Drop, won't be needed.
> 
>> +#include <linux/of_platform.h>
> 
> Where do you use it?
> 
> 
>> +#include <linux/of_address.h>
> 
> Where do you use it?

Sure will drop unused include files.

> 
>> +#include <linux/cdev.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/io.h>
>> +#include <linux/idr.h>
>> +#include <linux/atomic.h>
>> +#include <linux/delay.h>
>> +#include <linux/eventfd.h>
>> +#include <linux/kthread.h>
>> +#include <linux/sched/signal.h>
>> +#include <linux/bitops.h>
>> +#include <uapi/linux/eventpoll.h>
>> +#include <uapi/misc/silex_mpk.h>
>> +
>> +#include "silex_mpk_defs.h"
> 
> 
> ...
> 
>> +
>> +static int multipk_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct multipk_dev *mpkdev;
>> +	struct resource *memres;
>> +	int irq, ret;
>> +
>> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	mpkdev = devm_kzalloc(dev, sizeof(*mpkdev), GFP_KERNEL);
>> +	if (!mpkdev)
>> +		return -ENOMEM;
>> +	mpkdev->dev = dev;
>> +
>> +	memres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	mpkdev->regs = devm_ioremap_resource(dev, memres);
> 
> Use wrapper for both.
> 
> 
>> +	if (IS_ERR(mpkdev->regs))
>> +		return PTR_ERR(mpkdev->regs);
>> +	mpkdev->regsphys = memres->start;
>> +	memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> +	mpkdev->dbrg_regs = devm_ioremap_resource(dev, memres);
> 
> Use wrapper for both.

Will add a wrapper function.

> 
> 
>> +	if (IS_ERR(mpkdev->dbrg_regs))
>> +		return PTR_ERR(mpkdev->dbrg_regs);
>> +	mpkdev->dbrg_regsphys = memres->start;
>> +	platform_set_drvdata(pdev, mpkdev);
>> +
>> +	/* Only a single IRQ is supported */
>> +	if (platform_irq_count(pdev) != 1)
>> +		return -ENODEV;
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0)
>> +		return -ENODEV;
>> +
>> +	return multipk_create_device(mpkdev, dev, irq);
>> +}
>> +
>> +static void multipk_remove(struct platform_device *pdev)
>> +{
>> +	struct multipk_dev *mpkdev = platform_get_drvdata(pdev);
>> +
>> +	multipk_remove_device(mpkdev);
>> +}
>> +
>> +static const struct of_device_id multipk_match[] = {
>> +	{ .compatible = "multipk" },
> 
> NAK, you do not have such compatible.

I will update to use xlnx/multipk as per the device tree (and yaml)

> 
>> +	{ },
>> +};
>> +
>> +static struct platform_driver multipk_pdrv = {
>> +	.probe = multipk_probe,
>> +	.remove = multipk_remove,
>> +	.driver = {
>> +		.name = DRIVER_NAME,
>> +		.of_match_table = of_match_ptr(multipk_match),
> 
> Drop of_match_ptr, you have warnings here.

Sure will drop.

> 
>> +	},
>> +};
>> +
>> +static int __init multipk_init(void)
>> +{
>> +	dev_t devt;
>> +	int ret;
>> +
>> +	multipk_class = class_create("multipk");
>> +	if (IS_ERR(multipk_class)) {
>> +		ret = PTR_ERR(multipk_class);
>> +		pr_err("can't register class\n");
>> +		goto err;
>> +	}
>> +	ret = alloc_chrdev_region(&devt, 0, MULTIPK_MAX_DEVICES, "multipk");
>> +	if (ret) {
>> +		pr_err("can't register character device\n");
>> +		goto err_class;
>> +	}
>> +	multipk_major = MAJOR(devt);
>> +	multipk_minor = MINOR(devt);
>> +
>> +	ret = platform_driver_register(&multipk_pdrv);
>> +	if (ret) {
>> +		pr_err("can't register platform driver\n");
>> +		goto err_unchr;
>> +	}
>> +
>> +	return 0;
>> +err_unchr:
>> +	unregister_chrdev_region(devt, MULTIPK_MAX_DEVICES);
>> +err_class:
>> +	class_destroy(multipk_class);
>> +err:
>> +	return ret;
>> +}
>> +
>> +static void __exit multipk_exit(void)
>> +{
>> +	platform_driver_unregister(&multipk_pdrv);
>> +
>> +	unregister_chrdev_region(MKDEV(multipk_major, 0), MULTIPK_MAX_DEVICES);
>> +
>> +	class_destroy(multipk_class);
>> +}
>> +
>> +module_init(multipk_init);
>> +module_exit(multipk_exit);
> 
> I don't understand why loading a standard driver creates all this on my
> device.

Will move it to probe rather than driver init.

> 
>> +
>> +MODULE_DESCRIPTION("Driver for Silex Multipk Asymmetric crypto accelerator");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:" DRIVER_NAME);
> Drop

Okay.

Thanks,
Nipun

> 
> Best regards,
> Krzysztof