On Thu, May 06, 2021 at 01:44:54PM +0100, Sean Young wrote: > See http://www.usbuirt.com/ No proper commit message? > Signed-off-by: Sean Young <sean@xxxxxxxx> > +// Read IR in wideband mode. The byte stream is delivered in packets, > +// and the values which come in two bytes may straddle two packets > +static void uirt_wideband(struct uirt *uirt, u32 len) > +{ > + uint i, duration, carrier, pulses; > + > + for (i = 2; i < len; i++) { > + switch (uirt->rx_state) { > + case RX_STATE_INTERSPACE_HIGH: > + uirt->rx_state = RX_STATE_INTERSPACE_LOW; > + break; > + case RX_STATE_INTERSPACE_LOW: > + uirt->rx_state = RX_STATE_ON_HIGH; > + break; > + case RX_STATE_ON_HIGH: > + uirt->high = uirt->in[i]; > + uirt->rx_state = RX_STATE_ON_LOW; > + break; > + case RX_STATE_ON_LOW: > + // duration is in 400ns units > + duration = (uirt->high << 8) | uirt->in[i]; > + uirt->last_duration = duration; > + ir_raw_event_store(uirt->rc, &((struct ir_raw_event) { > + .duration = DIV_ROUND_CLOSEST(duration * 2, 5), > + .pulse = true, > + })); > + uirt->rx_state = RX_STATE_FREQ_HIGH; > + break; > + case RX_STATE_FREQ_HIGH: > + if (uirt->in[i] & 0x80) { > + uirt->high = uirt->in[i] & 0x7f; > + uirt->rx_state = RX_STATE_FREQ_LOW; > + break; > + } > + > + uirt->high = 0; > + fallthrough; > + case RX_STATE_FREQ_LOW: > + pulses = (uirt->high << 8) | uirt->in[i]; > + if (pulses) { > + dev_dbg(uirt->dev, "carrier duration %u pulses %u", > + uirt->last_duration, pulses); > + > + // calculate the Hz of pulses in duration 400ns units > + carrier = DIV_ROUND_CLOSEST_ULL(pulses * 10000000ull, > + uirt->last_duration * 4); Looks like uirt->last_duration comes from the external device and you don't have any sanity checks to prevent division by zero here. > + ir_raw_event_store(uirt->rc, &((struct ir_raw_event) { > + .carrier = carrier, > + .carrier_report = true, > + })); > + } > + uirt->rx_state = RX_STATE_OFF_HIGH; > + break; > + case RX_STATE_OFF_HIGH: > + if (uirt->in[i] == 0xff) { > + ir_raw_event_store(uirt->rc, &((struct ir_raw_event) { > + .duration = IR_TIMEOUT, > + .timeout = true, > + })); > + uirt->rx_state = RX_STATE_INTERSPACE_HIGH; > + } else { > + uirt->high = uirt->in[i]; > + uirt->rx_state = RX_STATE_OFF_LOW; > + } > + break; > + case RX_STATE_OFF_LOW: > + // duration is in 400ns units > + duration = (uirt->high << 8) | uirt->in[i]; > + ir_raw_event_store(uirt->rc, &((struct ir_raw_event) { > + .duration = DIV_ROUND_CLOSEST(duration * 2, 5), > + .pulse = false, > + })); > + uirt->rx_state = RX_STATE_ON_HIGH; > + break; > + } > + } > + > + ir_raw_event_handle(uirt->rc); > +} > + > +static void uirt_response(struct uirt *uirt, u32 len) > +{ > + int i; > + > + dev_dbg(uirt->dev, "state:%d data: %*phN\n", uirt->cmd_state, len, uirt->in); > + > + // Do we have more IR to transmit > + if (uirt->cmd_state == CMD_STATE_STREAMING_TX && len >= 2 && > + uirt->tx_len && uirt->in[0] & FTDI_RS0_CTS) { > + u32 len; > + int err; > + > + len = min_t(u32, uirt->tx_len, MAX_PACKET); > + > + memcpy(uirt->out, uirt->tx_buf, len); > + uirt->urb_out->transfer_buffer_length = len; > + > + uirt->tx_len -= len; > + uirt->tx_buf += len; > + > + err = usb_submit_urb(uirt->urb_out, GFP_ATOMIC); > + if (err != 0) > + dev_warn(uirt->dev, > + "failed to submit out urb: %d\n", err); > + } > + > + // if we only have two bytes, it just gives us the serial line status > + if (len <= 2) > + return; > + > + switch (uirt->cmd_state) { > + case CMD_STATE_GETVERSION: > + if (len == 10) { > + // check checksum > + u8 checksum = 0; > + > + for (i = 2; i < len; i++) > + checksum += uirt->in[i]; > + > + if (checksum != 0) { > + dev_err(uirt->dev, "checksum does not match: %*phN\n", > + len, uirt->in); > + return; > + } > + > + dev_info(uirt->dev, > + "USB-UIRT firmware v%u.%u protocol v%u.%u %02u-%02u-%04u", > + uirt->in[2], uirt->in[3], uirt->in[4], > + uirt->in[5], uirt->in[6], uirt->in[7], > + 2000 + uirt->in[8]); > + > + complete(&uirt->cmd_done); > + uirt->cmd_state = CMD_STATE_IRDATA; > + return; > + } > + break; > + case CMD_STATE_DOTXRAW: > + case CMD_STATE_STREAMING_TX: > + case CMD_STATE_SETMODERAW: > + case CMD_STATE_SETMODEWIDEBAND: > + if (len == 3) { > + switch (uirt->in[2]) { > + case 0x20: > + // 0x20 transmitting is expected during streaming tx > + if (uirt->cmd_state == CMD_STATE_STREAMING_TX) > + return; > + > + if (uirt->cmd_state == CMD_STATE_DOTXRAW) > + complete(&uirt->cmd_done); > + else > + dev_err(uirt->dev, "device transmitting"); > + break; > + case 0x21: > + if (uirt->tx_len) { > + dev_err(uirt->dev, "tx completed with %u left to send", > + uirt->tx_len); > + } else { > + if (uirt->cmd_state == CMD_STATE_SETMODERAW) > + uirt->wideband = false; > + if (uirt->cmd_state == CMD_STATE_SETMODEWIDEBAND) > + uirt->wideband = true; > + > + complete(&uirt->cmd_done); > + } > + break; > + case 0x80: > + dev_err(uirt->dev, "checksum error"); > + break; > + case 0x81: > + dev_err(uirt->dev, "timeout"); > + break; > + case 0x82: > + dev_err(uirt->dev, "command error"); > + break; > + default: > + dev_err(uirt->dev, "unknown response"); > + } > + > + uirt->cmd_state = CMD_STATE_IRDATA; > + return; > + } > + default: > + break; > + } > + > + if (uirt->wideband) > + uirt_wideband(uirt, len); > + else > + uirt_raw_mode(uirt, len); > +} Your code assumes that you'll always get one message per transfer, but since the device uses a regular FTDI chip with a FIFO this isn't guaranteed. > + > +static void uirt_out_callback(struct urb *urb) > +{ > + struct uirt *uirt = urb->context; > + > + if (urb->status) > + dev_warn(uirt->dev, "out urb status: %d\n", urb->status); Disconnecting a device shouldn't print a warning. > +} > + > +static void uirt_in_callback(struct urb *urb) > +{ > + struct uirt *uirt = urb->context; > + int ret; > + > + if (urb->status == 0) > + uirt_response(uirt, urb->actual_length); > + else > + dev_dbg(uirt->dev, "in urb status: %d\n", urb->status); > + > + ret = usb_submit_urb(urb, GFP_ATOMIC); > + if (ret && ret != -ENODEV) > + dev_warn(uirt->dev, "failed to resubmit urb: %d\n", ret); > +} > +static int init_ftdi(struct usb_device *udev) > +{ > + int err; > + > + // set the baud rate > + err = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), > + FTDI_SIO_SET_BAUDRATE_REQUEST, > + FTDI_SIO_SET_BAUDRATE_REQUEST_TYPE, > + 0x4009, 0x0001, > + NULL, 0, USB_CTRL_SET_TIMEOUT); > + if (err) > + return err; > + > + // enabling rts/cts flow control > + err = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), > + FTDI_SIO_SET_FLOW_CTRL_REQUEST, > + FTDI_SIO_SET_FLOW_CTRL_REQUEST_TYPE, > + 0, FTDI_SIO_RTS_CTS_HS, > + NULL, 0, USB_CTRL_SET_TIMEOUT); > + if (err) > + return err; Does the device UART actually have RTS wired up? > + > + // Set latency in milliseconds. The USB-UIRT will generate a > + // urb every latency milliseconds (IR or not), so this should be > + // set as high as possible. > + // > + // The USB-UIRT has a IR timeout of 14ms (i.e. it sends the 0xff > + // byte after 14ms of IR silence. So if we set the timeout to a higher > + // value, the 0xff just gets sent in a separate packet, and there > + // is a small delay for the IR to be processed. > + return usb_control_msg(udev, usb_sndctrlpipe(udev, 0), > + FTDI_SIO_SET_LATENCY_TIMER_REQUEST, > + FTDI_SIO_SET_LATENCY_TIMER_REQUEST_TYPE, > + 50, 0, Magic constant; it's not entirely clear from the code (or comment) what 50 is here. The latency go as high as 255 IIRC. > + NULL, 0, USB_CTRL_SET_TIMEOUT); > +} > + > +static int uirt_probe(struct usb_interface *intf, > + const struct usb_device_id *id) > +{ > + struct usb_host_interface *idesc = intf->cur_altsetting; > + struct usb_device *usbdev = interface_to_usbdev(intf); > + struct usb_endpoint_descriptor *ep_in = NULL; > + struct usb_endpoint_descriptor *ep_out = NULL; > + struct usb_endpoint_descriptor *ep = NULL; > + struct uirt *uirt; > + struct rc_dev *rc; > + struct urb *urb; > + int i, pipe, err = -ENOMEM; > + > + for (i = 0; i < idesc->desc.bNumEndpoints; i++) { > + ep = &idesc->endpoint[i].desc; > + > + if (!ep_in && usb_endpoint_is_bulk_in(ep) && > + usb_endpoint_maxp(ep) == MAX_PACKET) > + ep_in = ep; > + > + if (!ep_out && usb_endpoint_is_bulk_out(ep) && > + usb_endpoint_maxp(ep) == MAX_PACKET) > + ep_out = ep; > + } You should use usb_find_common_endpoints() instead of open coding even if you'd still need to check the endpoint size with your implementation. > + > + if (!ep_in || !ep_out) { > + dev_err(&intf->dev, "required endpoints not found\n"); > + return -ENODEV; > + } > + > + uirt = kzalloc(sizeof(*uirt), GFP_KERNEL); > + if (!uirt) > + return -ENOMEM; > + > + uirt->in = kmalloc(MAX_PACKET, GFP_KERNEL); > + if (!uirt->in) > + goto free_uirt; > + > + uirt->out = kmalloc(MAX_PACKET, GFP_KERNEL); > + if (!uirt->out) > + goto free_uirt; > + > + rc = rc_allocate_device(RC_DRIVER_IR_RAW); > + if (!rc) > + goto free_uirt; > + > + urb = usb_alloc_urb(0, GFP_KERNEL); > + if (!urb) > + goto free_rcdev; > + > + pipe = usb_rcvbulkpipe(usbdev, ep_in->bEndpointAddress); > + usb_fill_bulk_urb(urb, usbdev, pipe, uirt->in, MAX_PACKET, > + uirt_in_callback, uirt); > + uirt->urb_in = urb; > + > + urb = usb_alloc_urb(0, GFP_KERNEL); > + if (!urb) > + goto free_rcdev; > + > + pipe = usb_sndbulkpipe(usbdev, ep_out->bEndpointAddress); > + usb_fill_bulk_urb(urb, usbdev, pipe, uirt->out, MAX_PACKET, > + uirt_out_callback, uirt); > + > + uirt->dev = &intf->dev; > + uirt->usbdev = usbdev; > + uirt->rc = rc; > + uirt->urb_out = urb; > + uirt->rx_state = RX_STATE_INTERSPACE_HIGH; > + > + err = usb_submit_urb(uirt->urb_in, GFP_KERNEL); > + if (err != 0) { > + dev_err(uirt->dev, "failed to submit read urb: %d\n", err); > + goto free_rcdev; > + } > + > + err = init_ftdi(usbdev); > + if (err) { > + dev_err(uirt->dev, "failed to setup ftdi: %d\n", err); > + goto free_rcdev; > + } > + > + err = uirt_setup(uirt); > + if (err) > + goto free_rcdev; > + > + usb_make_path(usbdev, uirt->phys, sizeof(uirt->phys)); > + > + rc->device_name = "USB-UIRT"; > + rc->driver_name = KBUILD_MODNAME; > + rc->input_phys = uirt->phys; > + usb_to_input_id(usbdev, &rc->input_id); > + rc->dev.parent = &intf->dev; > + rc->priv = uirt; > + rc->tx_ir = uirt_tx; > + rc->s_tx_carrier = uirt_set_tx_carrier; > + rc->s_learning_mode = uirt_set_rx_wideband; > + rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER; > + rc->map_name = RC_MAP_RC6_MCE; > + rc->rx_resolution = UNIT_US; > + rc->timeout = IR_TIMEOUT; > + > + uirt_set_tx_carrier(rc, 38000); > + > + err = rc_register_device(rc); > + if (err) > + goto free_rcdev; > + > + usb_set_intfdata(intf, uirt); > + > + return 0; > + > +free_rcdev: > + usb_kill_urb(uirt->urb_in); > + usb_kill_urb(uirt->urb_out); Looks lazy to kill the URBs before they've been allocated, better to add another label even if it's not a bug. > + usb_free_urb(uirt->urb_in); > + usb_free_urb(uirt->urb_out); > + rc_free_device(rc); > +free_uirt: > + kfree(uirt->in); > + kfree(uirt->out); > + kfree(uirt); > + return err; > +} Johan
