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

["Arnd Bergmann" <arnd@arndb.de>]

On Wed, Sep 14, 2022, at 4:29 PM, Manjunatha Venkatesh wrote:

> NXP has SR1XX family of UWB Subsystems (UWBS) devices. SR1XX SOCs
> are FiRa Compliant. SR1XX SOCs are flash less devices and they need
> Firmware Download on every device boot. More details on the SR1XX Family
> can be found at https://www.nxp.com/products/:UWB-TRIMENSION
> 
> The sr1xx driver work the SR1XX Family of UWBS, and uses UWB Controller
> Interface (UCI).  The corresponding details are available in the FiRa
> Consortium Website (https://www.firaconsortium.org/).

I know nothing about UWB, so I have no idea if the user interface
you propose here makes sense. My guess is that there is a good chance
that there are other implementations of UWB that would not work
with this specific driver interface, so you probably need a
slightly higher-level abstraction.

We had an older subsystem that was called UWB and that got removed
a while ago:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/staging/uwb?id=caa6772db4c1deb5d9add48e95d6eab50699ee5e

Is that the same UWB or something completely different?

> +#define SR1XX_MAGIC 0xEA
> +#define SR1XX_SET_PWR _IOW(SR1XX_MAGIC, 0x01, long)
> +#define SR1XX_SET_FWD _IOW(SR1XX_MAGIC, 0x02, long)

This should be in a uapi header.

> +static int sr1xx_dev_open(struct inode *inode, struct file *filp)
> +{
> +	struct sr1xx_dev *sr1xx_dev =
> +	    container_of(filp->private_data, struct sr1xx_dev, sr1xx_device);
> +
> +	filp->private_data = sr1xx_dev;

This looks dangerous if the file gets opened more than once
and filp->private_data can have two different values.

> +static long sr1xx_dev_ioctl(struct file *filp, unsigned int cmd,
> +			    unsigned long arg)
> +{
> +	int ret = 0;
> +	struct sr1xx_dev *sr1xx_dev = NULL;
> +
> +	sr1xx_dev = filp->private_data;
> +
> +	switch (cmd) {
> +	case SR1XX_SET_PWR:
> +		if (arg == PWR_ENABLE) {
> +			gpio_set_value(sr1xx_dev->ce_gpio, 1);
> +			usleep_range(10000, 12000);

The usage of 'arg' does not match the definition of the command
number, which expects a pointer to 'long'. If you want to keep
the behavior, I suggest changing the #define.

> +static void sr1xx_wait_for_irq_gpio_low(struct sr1xx_dev *sr1xx_dev)
> +{
> +	int retry_count = 0;
> +
> +	do {
> +		udelay(10);
> +		retry_count++;
> +		if (retry_count == MAX_RETRY_COUNT_FOR_HANDSHAKE) {
> +			dev_info(&sr1xx_dev->spi->dev,
> +				 "Slave not released the IRQ even after 10ms");
> +			break;
> +		}
> +	} while (gpio_get_value(sr1xx_dev->irq_gpio));
> +}

The way to wait for a timeout is to compare against the timestamp
before the loop, using e.g. "time_before(jiffies, timeout)"
or possibly using ktime_get() instead of jiffies if you want to
be more accurate.

10ms is really too long for a busy-loop anyway, so better use
usleep_range() from a non-atomic context.

> +/* Possible fops on the sr1xx device */
> +static const struct file_operations sr1xx_dev_fops = {
> +	.owner = THIS_MODULE,
> +	.read = sr1xx_dev_read,
> +	.write = sr1xx_dev_write,
> +	.open = sr1xx_dev_open,
> +	.unlocked_ioctl = sr1xx_dev_ioctl,
> +};

There should be a .compat_ioctl callback, either using the
same sr1xx_dev_ioctl function if you keep using the 'arg'
value directly, or 'compat_ptr_ioctl()' if you move to
pointers to arguments, or a custom function if you have
a mix of the two.

> +static int sr1xx_probe(struct spi_device *spi)
...
> +	ret = sr1xx_hw_setup(&spi->dev, &platform_data);
> +	if (ret < 0) {
> +		dev_err(&spi->dev, "Failed hw_setup\n");
> +		goto err_setup;
> +	}
....
> +
> +	sr1xx_dev->spi = spi;
> +	sr1xx_dev->sr1xx_device.minor = MISC_DYNAMIC_MINOR;
> +	sr1xx_dev->sr1xx_device.name = "sr1xx";
> +	sr1xx_dev->sr1xx_device.fops = &sr1xx_dev_fops;
> +	sr1xx_dev->sr1xx_device.parent = &spi->dev;
> +	sr1xx_dev->irq_gpio = desc_to_gpio(platform_data.gpiod_irq);
> +	sr1xx_dev->ce_gpio = desc_to_gpio(platform_data.gpiod_ce);
> +	sr1xx_dev->spi_handshake_gpio =
> +	    desc_to_gpio(platform_data.gpiod_spi_handshake);

The temporary 'platform_data' structure seems useless here,
just fold its members into the sr1xx_dev structure itself.
No need to store both a gpio descriptor and a number, you
can simplify this to always use the descriptor.

> +	sr1xx_dev->tx_buffer = kzalloc(SR1XX_TXBUF_SIZE, GFP_KERNEL);
> +	sr1xx_dev->rx_buffer = kzalloc(SR1XX_RXBUF_SIZE, GFP_KERNEL);
> +	if (!sr1xx_dev->tx_buffer) {
> +		ret = -ENOMEM;
> +		goto err_exit;
> +	}
> +	if (!sr1xx_dev->rx_buffer) {
> +		ret = -ENOMEM;
> +		goto err_exit;
> +	}
> +
> +	sr1xx_dev->spi->irq = gpio_to_irq(sr1xx_dev->irq_gpio);
> +	if (sr1xx_dev->spi->irq < 0) {
> +		dev_err(&spi->dev, "gpio_to_irq request failed gpio = 0x%x\n",
> +			sr1xx_dev->irq_gpio);
> +		goto err_exit;
> +	}

Instead of gpio_to_irq(), the DT binding should probably
list the interrupt directly using the "interrupts" property
pointing to the gpio controller.

> +
> +static const struct acpi_device_id sr1xx_acpi_match[] = {
> +	{"PRP0001", 0},
> +	{"", 0},
> +};

As far as I understand, you are not supposed to list
compatiblity with PRP0001 when using this on a PC, the
ACPI subsystem will instead look at the of_device_id
table.

> +static const struct dev_pm_ops sr1xx_dev_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(sr1xx_dev_suspend, sr1xx_dev_resume)
> +};

Use SYSTEM_SLEEP_PM_OPS() instead of SET_SYSTEM_SLEEP_PM_OPS()
to avoid a warning about unused functions.

> +static int __init sr1xx_dev_init(void)
> +{
> +	return spi_register_driver(&sr1xx_driver);
> +}
> +
> +module_init(sr1xx_dev_init);
> +
> +static void __exit sr1xx_dev_exit(void)
> +{
> +	spi_unregister_driver(&sr1xx_driver);
> +}
> +
> +module_exit(sr1xx_dev_exit);

module_spi_driver()