Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx series chip

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Sep 14, 2022 at 07:59:44PM +0530, Manjunatha Venkatesh wrote:
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -471,6 +471,17 @@ config HISI_HIKEY_USB
>  	  switching between the dual-role USB-C port and the USB-A host ports
>  	  using only one USB controller.
> 
> +config NXP_UWB
> +    tristate "NXP UCI(Uwb Command Interface) protocol driver support"
> +    depends on SPI
> +    help
> +      This option enables the UWB driver for NXP sr1xx device.
> +      Such device supports UCI packet structure, FiRa compliant.
> +
> +      Say Y here to compile support for nxp-sr1xx into the kernel or
> +      say M to compile it as a module. The module will be called
> +      nxp-sr1xx.ko

No tabs?

> +
>  config OPEN_DICE
>  	tristate "Open Profile for DICE driver"
>  	depends on OF_RESERVED_MEM
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 2be8542616dd..ee8ca32c66f6 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -60,4 +60,5 @@ obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
>  obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o
>  obj-$(CONFIG_HI6421V600_IRQ)	+= hi6421v600-irq.o
>  obj-$(CONFIG_OPEN_DICE)		+= open-dice.o
>  obj-$(CONFIG_VCPU_STALL_DETECTOR)	+= vcpu_stall_detector.o
> +obj-$(CONFIG_NXP_UWB) 		+= nxp-sr1xx.o
> diff --git a/drivers/misc/nxp-sr1xx.c b/drivers/misc/nxp-sr1xx.c
> new file mode 100644
> index 000000000000..6ca9a2b54b86
> --- /dev/null
> +++ b/drivers/misc/nxp-sr1xx.c
> @@ -0,0 +1,794 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)

Please no.  If you really want to dual-license your Linux kernel code,
that's fine, but I will insist that you get a signed-off-by from your
corporate lawyer so that I know that they agree with this and are
willing to handle all of the complex issues that this entails as it will
require work on their side over time.

If that's not worth bothering your lawyers over, please just stick with
GPL as the only license.


> +/*
> + * Copyright 2018-2022 NXP.
> + *
> + * SPI driver for UWB SR1xx
> + * Author: Manjunatha Venkatesh <manjunatha.venkatesh@nxp.com>
> + */
> +
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_gpio.h>
> +#include <linux/spi/spi.h>
> +#include <linux/uaccess.h>
> +
> +#define SR1XX_MAGIC 0xEA
> +#define SR1XX_SET_PWR _IOW(SR1XX_MAGIC, 0x01, long)
> +#define SR1XX_SET_FWD _IOW(SR1XX_MAGIC, 0x02, long)

You can't stick ioctl command definitions in a .c file that userspace
never sees.  How are your userspace tools supposed to know what the
ioctl is and how it is defined?

How was this ever tested and where is your userspace code that interacts
with this code?

thanks,

greg k-h