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()
