On Tue, Dec 20, 2022 at 09:17:47PM +0530, Manjunatha Venkatesh wrote: > Ultra-wideband (UWB) is a short-range wireless communication protocol. > > 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/). > > Internally driver will handle two modes of operation. > 1.HBCI mode (sr1xx BootROM Code Interface) > Firmware download uses HBCI ptotocol packet structure which is > Nxp proprietary,Firmware File(.bin) stored in user space context > and during device init sequence pick the firmware packet in chunk > and send it to the driver with write() api call. > > After the firmware download sequence at the end UWBS will > send device status notification and its indication of device entered > UCI mode. > Here after any command/response/notification will follow > UCI packet structure. > > 2.UCI mode (UWB Command interface) > Once Firmware download finishes sr1xx will switch to UCI mode. > Then driver exchange command/response/notification as per the FIRA UCI > standard format between user space and sr1xx device. > Any response or notification received from sr1xx through SPI line > will convey to user space. > > Its Interrupt based driver and IO Handshake needed with SR1XX Family of > SOCs. > This driver needs dts config update as per the sr1xx data sheet. > Corresponding document available in Documentation/devicetree/bindings/uwb > > Link: https://lore.kernel.org/r/dfe167cf-5d4e-6fc7-c954-25f719b1e843@xxxxxxx Why is this here pointing to an old version of the patch? > Signed-off-by: Manjunatha Venkatesh <manjunatha.venkatesh@xxxxxxx> > Signed-off-by: Kwame Adwere <kwame.adwere@xxxxxxx> > --- > Changes since v5: > - Moved ioctl command definitions into header file. > - Version 5 patch review comments addressed. > - Corporate lawyer sign-off updated. Again, you are not documenting _WHY_ you want a dual license here, which is what I asked for yesterday, why ignore that and ask us to review this again? > +#define UCI_HEADER_LEN 4 > +#define HBCI_HEADER_LEN 4 > +#define UCI_PAYLOAD_LEN_OFFSET 3 > + > +#define UCI_EXT_PAYLOAD_LEN_IND_OFFSET 1 > +#define UCI_EXT_PAYLOAD_LEN_IND_OFFSET_MASK 0x80 > +#define UCI_EXT_PAYLOAD_LEN_OFFSET 2 > +#define UCI_MT_MASK 0xE0 > + > +#define SR1XX_TXBUF_SIZE 4200 > +#define SR1XX_RXBUF_SIZE 4200 > +#define SR1XX_MAX_TX_BUF_SIZE 4200 > + > +#define MAX_RETRY_COUNT_FOR_IRQ_CHECK 100 > +#define MAX_RETRY_COUNT_FOR_HANDSHAKE 1000 Please use tabs. > +/* Macro to define SPI clock frequency */ > +#define SR1XX_SPI_CLOCK 16000000L > +#define WAKEUP_SRC_TIMEOUT (2000) > + > +/* Maximum UCI packet size supported from the driver */ > +#define MAX_UCI_PKT_SIZE 4200 > + > +/* Device specific macro and structure */ > +struct sr1xx_dev { > + wait_queue_head_t read_wq; /* Wait queue for read interrupt */ > + struct spi_device *spi; /* Spi device structure */ > + struct miscdevice sr1xx_device; /* Char device as misc driver */ > + unsigned int ce_gpio; /* SW reset gpio */ > + unsigned int irq_gpio; /* SR1XX will interrupt host for any ntf */ > + unsigned int spi_handshake_gpio; /* Host ready to read data */ > + bool irq_enabled; /* Flag to indicate disable/enable irq sequence */ > + bool irq_received; /* Flag to indicate that irq is received */ > + spinlock_t irq_enabled_lock; /* Spin lock for read irq */ > + unsigned char *tx_buffer; /* Transmit buffer */ > + unsigned char *rx_buffer; /* Receive buffer */ > + unsigned int write_count; /* Holds nubers of byte written */ > + unsigned int read_count; /* Hold nubers of byte read */ > + struct mutex > + sr1xx_access_lock; /* Lock used to synchronize read and write */ > + size_t total_bytes_to_read; /* Total bytes read from the device */ > + bool is_extended_len_bit_set; /* Variable to check ext payload Len */ > + bool read_abort_requested; /* Used to indicate read abort request */ > + bool is_fw_dwnld_enabled; /* Used to indicate fw download mode */ > + int mode; /* Indicate write or read mode */ > + long timeout_in_ms; /* Wait event interrupt timeout in ms */ If you want to document a structure (which is a good thing), please do so in a way we can understand it (i.e. with kernel doc format like we do everywhere else in the kernel), not like this which is almost impossible to follow. > +}; > + > +enum spi_status_codes { > + TRANSCEIVE_SUCCESS, > + TRANSCEIVE_FAIL, > + IRQ_WAIT_REQUEST, > + IRQ_WAIT_TIMEOUT > +}; > + > +/* Spi write/read operation mode */ > +enum spi_operation_modes { SR1XX_WRITE_MODE, SR1XX_READ_MODE }; Why is this a global symbol? > +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; > + return 0; > +} > + > +static void sr1xx_disable_irq(struct sr1xx_dev *sr1xx_dev) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&sr1xx_dev->irq_enabled_lock, flags); > + if ((sr1xx_dev->irq_enabled)) { > + disable_irq_nosync(sr1xx_dev->spi->irq); > + sr1xx_dev->irq_received = true; > + sr1xx_dev->irq_enabled = false; > + } > + spin_unlock_irqrestore(&sr1xx_dev->irq_enabled_lock, flags); > +} > + > +static void sr1xx_enable_irq(struct sr1xx_dev *sr1xx_dev) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&sr1xx_dev->irq_enabled_lock, flags); > + if (!sr1xx_dev->irq_enabled) { > + enable_irq(sr1xx_dev->spi->irq); > + sr1xx_dev->irq_enabled = true; > + sr1xx_dev->irq_received = false; > + } > + spin_unlock_irqrestore(&sr1xx_dev->irq_enabled_lock, flags); > +} > + > +static irqreturn_t sr1xx_dev_irq_handler(int irq, void *dev_id) > +{ > + struct sr1xx_dev *sr1xx_dev = dev_id; > + > + sr1xx_disable_irq(sr1xx_dev); > + /* Wake up waiting readers */ > + wake_up(&sr1xx_dev->read_wq); > + if (device_may_wakeup(&sr1xx_dev->spi->dev)) > + pm_wakeup_event(&sr1xx_dev->spi->dev, WAKEUP_SRC_TIMEOUT); > + return IRQ_HANDLED; > +} > + > +static long sr1xx_dev_ioctl(struct file *filp, unsigned int cmd, > + unsigned long arg) > +{ > + long ret = 0; > + struct sr1xx_dev *sr1xx_dev = NULL; > + > + sr1xx_dev = filp->private_data; > + if (sr1xx_dev == NULL) { > + ret = -EINVAL; > + pr_err("%s sr1xx_dev is NULL\n", __func__); > + return ret; > + } > + switch (cmd) { > + case SR1XX_SET_PWR: > + if (arg == PWR_ENABLE) { > + gpio_set_value(sr1xx_dev->ce_gpio, 1); > + usleep_range(10000, 12000); > + } else if (arg == PWR_DISABLE) { > + gpio_set_value(sr1xx_dev->ce_gpio, 0); > + sr1xx_disable_irq(sr1xx_dev); > + usleep_range(10000, 12000); > + } else if (arg == ABORT_READ_PENDING) { > + sr1xx_dev->read_abort_requested = true; > + sr1xx_disable_irq(sr1xx_dev); > + /* Wake up waiting readers */ > + wake_up(&sr1xx_dev->read_wq); > + } > + break; > + case SR1XX_SET_FWD: > + if (arg == 1) { > + sr1xx_dev->is_fw_dwnld_enabled = true; > + sr1xx_dev->read_abort_requested = false; > + } else if (arg == 0) { > + sr1xx_dev->is_fw_dwnld_enabled = false; > + } > + break; > + default: > + dev_err(&sr1xx_dev->spi->dev, " Error case"); So userspace can spam the kernel log by sending your driver invalid ioctls? Please never do that. > + ret = -EINVAL; Isn't this the wrong error value for an incorrect ioctl command? > + } > + return ret; > +} > + > +/** > + * sr1xx_wait_for_irq_gpio_low > + * > + * Function to wait till irq gpio goes low state > + * > + */ Odd commenting style, please either use the correct kerneldoc format, or a simpler one, or nothing at all. Please do not make up your own format style just for a single driver as that is unmaintainable, right? Also, I see no justification here for why this has to be a kernel driver, OR where the userspace code is that controls this. I've stopped here :( greg k-h
