Re: [PATCH 8/9] dev-serial: fix FTDI_GET_MDM_ST response

[Jason Andryuk <jandryuk@gmail.com>]

On Mon, Oct 26, 2020 at 7:21 AM Samuel Thibault <samuel.thibault@gnu.org> wrote:
>
> Mark Cave-Ayland, le lun. 26 oct. 2020 10:58:43 +0000, a ecrit:
> > On 26/10/2020 09:54, Samuel Thibault wrote:
> > > Mark Cave-Ayland, le lun. 26 oct. 2020 08:34:00 +0000, a ecrit:
> > > > The FTDI_GET_MDM_ST response should only return a single byte indicating the
> > > > modem status with bit 0 cleared (as documented in the Linux ftdi_sio.h header
> > > > file).
> > > >
> > > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > > > ---
> > > >   hw/usb/dev-serial.c | 5 ++---
> > > >   1 file changed, 2 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> > > > index 4c374d0790..fa734bcf54 100644
> > > > --- a/hw/usb/dev-serial.c
> > > > +++ b/hw/usb/dev-serial.c
> > > > @@ -360,9 +360,8 @@ static void usb_serial_handle_control(USBDevice *dev, USBPacket *p,
> > > >           /* TODO: TX ON/OFF */
> > > >           break;
> > > >       case VendorDeviceRequest | FTDI_GET_MDM_ST:
> > > > -        data[0] = usb_get_modem_lines(s) | 1;
> > > > -        data[1] = FTDI_THRE | FTDI_TEMT;
> > > > -        p->actual_length = 2;
> > > > +        data[0] = usb_get_modem_lines(s);
> > > > +        p->actual_length = 1;
> > >
> [...]
> > A quick test shows my Chipi-X returns 0x1 0x60 with nothing attached in
> > response to FTDI_SIO_GET_MODEM_STATUS_REQUEST: assuming the reply length
> > should be 2 bytes, the comment about B0-B3 being zero and the response from
> > my Chip-X above suggests that the "| 1" should still be dropped from the
> > response.
>
> Aurelien, you introduced the "| 1" in
>
> commit abb8a13918ecc1e8160aa78582de9d5224ea70df
> Author: Aurelien Jarno <aurelien@aurel32.net>
> Date:   Wed Aug 13 04:23:17 2008 +0000
>
>     usb-serial: add support for modem lines
>
> [...]
> @@ -357,9 +393,9 @@ static int usb_serial_handle_control(USBDevice *dev, int request, int value,
>          /* TODO: TX ON/OFF */
>          break;
>      case DeviceInVendor | FTDI_GET_MDM_ST:
> -        /* TODO: return modem status */
> -        data[0] = 0;
> -        ret = 1;
> +        data[0] = usb_get_modem_lines(s) | 1;
> +        data[1] = 0;
> +        ret = 2;
>          break;
>
> do you know exactly what it is for?

Hi,

I'm not particularly familiar with the FTDI USB serial devices.  I
found setting FTDI_THRE | FTDI_TEMT by comparing with real hardware.

A little searching found this:
https://elixir.bootlin.com/linux/latest/source/drivers/usb/serial/ftdi_sio.h#L541

That shows "B0   Reserved - must be 1", so maybe that is why "| 1" was added?

Regards,
Jason