From: Johan Hovold <johan@kernel.org>
To: Sean Young <sean@mess.org>
Cc: linux-media@vger.kernel.org, linux-usb@vger.kernel.org,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Jon Rhees <support@usbuirt.com>, Oliver Neukum <oneukum@suse.com>
Subject: Re: [PATCH v3 2/3] media: rc: new driver for USB-UIRT device
Date: Mon, 17 May 2021 11:38:10 +0200 [thread overview]
Message-ID: <YKI5gthIZBkzHFzz@hovoldconsulting.com> (raw)
In-Reply-To: <20210515095241.GB31801@gofer.mess.org>
On Sat, May 15, 2021 at 10:52:41AM +0100, Sean Young wrote:
> On Fri, May 14, 2021 at 01:38:11PM +0200, Johan Hovold wrote:
> > On Thu, May 06, 2021 at 01:44:54PM +0100, Sean Young wrote:
> > > See http://www.usbuirt.com/
> >
> > No proper commit message?
>
> No sure what to say here what's not already in the first line of the
> commit.
>
> Maybe I could mention the fact this uses fdti usb serial chip.
That'd be good. Perhaps mention why you chose to implement a kernel
driver for this device which appears to already be supported by the lirc
daemon from user space too.
> > > +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.
>
> I guess you're talking about that data can straddle multiple packets
> because there is a fifo involved, or there can be more data before/after
> a command response.
Right.
> Let me see what I can do about that.
You'd get buffering for free if you let the tty layer handle this...
> > > +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?
>
> I don't know TBH. However it does have CTS wired up.
When using a serial driver, RTS would be asserted on open and
(typically) deasserted on close. Not sure it matters.
The FTDI driver would also clear the receive FIFO when opening the port
I believe.
Johan
next prev parent reply other threads:[~2021-05-17 9:46 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-06 12:44 [PATCH v3 0/3] IR driver for USB-UIRT device Sean Young
2021-05-06 12:44 ` [PATCH v3 1/3] USB: serial: move ftdi_sio.h into include directories Sean Young
2021-05-14 11:16 ` Johan Hovold
2021-05-06 12:44 ` [PATCH v3 2/3] media: rc: new driver for USB-UIRT device Sean Young
2021-05-14 11:38 ` Johan Hovold
2021-05-15 9:52 ` Sean Young
2021-05-17 9:38 ` Johan Hovold [this message]
2021-05-06 12:44 ` [PATCH v3 3/3] USB: serial: blacklist USB-UIRT when driver is selected Sean Young
2021-05-14 11:40 ` Johan Hovold
2021-05-15 9:56 ` Sean Young
2021-05-17 9:40 ` Johan Hovold
2021-05-10 8:15 ` [PATCH v3 0/3] IR driver for USB-UIRT device Johan Hovold
2021-05-11 10:32 ` Sean Young
2021-05-14 11:16 ` Johan Hovold
2021-05-15 9:22 ` Sean Young
2021-05-17 9:30 ` Johan Hovold
2021-05-17 10:35 ` Sean Young
2021-05-17 12:35 ` Sean Young
2021-05-20 13:40 ` Johan Hovold
2021-05-20 13:31 ` Johan Hovold
2021-05-21 11:39 ` Sean Young
2021-06-23 12:48 ` Johan Hovold
2021-05-25 12:25 ` Oliver Neukum
2021-06-23 13:10 ` Johan Hovold
2021-06-24 9:13 ` Sean Young
2021-06-24 9:41 ` Johan Hovold
2021-06-25 8:08 ` Sean Young
2021-07-02 10:44 ` Johan Hovold
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YKI5gthIZBkzHFzz@hovoldconsulting.com \
--to=johan@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=oneukum@suse.com \
--cc=sean@mess.org \
--cc=support@usbuirt.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.