subject:"\[PATCH V6 02\/10\] USB\: f81232\: implement read IIR\/MSR with endpoint"

Re: [PATCH V6 02/10] USB: f81232: implement read IIR/MSR with endpoint

2015-02-17 Thread Johan Hovold

On Mon, Feb 16, 2015 at 03:57:54PM +0800, Peter Hung wrote:
> The interrupt Endpoint will report current IIR. If we got IIR with MSR Changed
> , We will do read MSR with interrupt_work worker to do f81232_read_msr() func.
> 
> Signed-off-by: Peter Hung 
> ---
>  drivers/usb/serial/f81232.c | 109 
> 
>  1 file changed, 100 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 669a2f2..ec4609d 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -23,6 +23,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  static const struct usb_device_id id_table[] = {
>   { USB_DEVICE(0x1934, 0x0706) },
> @@ -30,6 +31,13 @@ static const struct usb_device_id id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(usb, id_table);
>  
> +/* USB Control EP parameter */
> +#define F81232_REGISTER_REQUEST 0xA0
> +#define F81232_GET_REGISTER 0xc0

Indent these define values using a tab as well.

> +
> +#define SERIAL_BASE_ADDRESS 0x0120
> +#define MODEM_STATUS_REGISTER  (0x06 + SERIAL_BASE_ADDRESS)
> +
>  #define CONTROL_DTR  0x01
>  #define CONTROL_RTS  0x02
>  
> @@ -48,19 +56,92 @@ struct f81232_private {
>   spinlock_t lock;
>   u8 line_control;
>   u8 modem_status;
> +

Newline not needed.

> + struct work_struct interrupt_work;
> + struct usb_serial_port *port;
>  };
>  
> -static void f81232_update_line_status(struct usb_serial_port *port,
> +static int f81232_get_register(struct usb_serial_port *port,
> +   u16 reg, u8 *data)

No need to break this line any more.

> +{
> + int status;
> + struct usb_device *dev = port->serial->dev;
> +
> + status = usb_control_msg(dev,
> +  usb_rcvctrlpipe(dev, 0),
> +  F81232_REGISTER_REQUEST,
> +  F81232_GET_REGISTER,
> +  reg,
> +  0,
> +  data,
> +  sizeof(*data),
> +  USB_CTRL_GET_TIMEOUT);

There's some odd indentation above and in other patches as well (in this
case two tabs and one space). Please either just tabs or, if you prefer,
add additional spaces to align all the arguments with the first.

> + if (status < 0)

Zero is actually also an error here (returned length was 0). You may
want to let this function return -EIO in that case (and perhaps also
return 0 on success) or you need to fix this at every call site.

> + dev_err(>dev, "%s status: %d\n", __func__, status);

Please spell out that there was an error, for example,

"%s failed: %d\n"

> +
> + return status;
> +}

Could you add the get/set register helpers in a separate preparatory patch?

> +
> +static void f81232_read_msr(struct usb_serial_port *port)
> +{
> + int status;
> + unsigned long flags;
> + u8 current_msr;
> + struct tty_struct *tty;
> + struct f81232_private *priv = usb_get_serial_port_data(port);
> +
> + status = f81232_get_register(port, MODEM_STATUS_REGISTER,
> + _msr);

You cannot use a variable on the stack for usb_control_msg as the buffer
must be DMA-able (on all platforms). Use kzalloc.

> + if (status < 0) {
> + /* Retain the error even reported in f81232_get_register()
> +  to make debug easily :D */

No need for this comment.

> + dev_err(>dev, "%s fail, status: %d\n", __func__, status);
> + return;
> + }
> +
> + if (!(current_msr & UART_MSR_ANY_DELTA))
> + return;
> +
> + tty = tty_port_tty_get(>port);
> + if (tty) {
> + if (current_msr & UART_MSR_DDCD) {
> + usb_serial_handle_dcd_change(port, tty,
> + current_msr & UART_MSR_DCD);

Please indent continuation lines at least two tabs further (i.e add one
more tab here).

> + }
> +
> + tty_kref_put(tty);
> + }

You should rewrite this so you only get the tty reference if you're
actually gonna use it.

> +
> + spin_lock_irqsave(>lock, flags);
> + priv->modem_status = current_msr;
> + spin_unlock_irqrestore(>lock, flags);
> +
> + wake_up_interruptible(>port.delta_msr_wait);
> +}
> +
> +static void f81232_update_modem_status(struct usb_serial_port *port,

This one should still be called f81232_update_line_status.

> unsigned char *data,
> unsigned int actual_length)

Just call it length or size, you can use size_t.

>  {
> - /*
> -  * FIXME: Update port->icount, and call
> -  *
> -  *  wake_up_interruptible(>port.delta_msr_wait);
> -  *
> -  *on MSR changes.
> -  */
> + struct f81232_private *priv = usb_get_serial_port_data(port);
> +
> + if

Re: [PATCH V6 02/10] USB: f81232: implement read IIR/MSR with endpoint

2015-02-17 Thread Johan Hovold

On Mon, Feb 16, 2015 at 03:57:54PM +0800, Peter Hung wrote:
 The interrupt Endpoint will report current IIR. If we got IIR with MSR Changed
 , We will do read MSR with interrupt_work worker to do f81232_read_msr() func.
 
 Signed-off-by: Peter Hung hpeter+linux_ker...@gmail.com
 ---
  drivers/usb/serial/f81232.c | 109 
 
  1 file changed, 100 insertions(+), 9 deletions(-)
 
 diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
 index 669a2f2..ec4609d 100644
 --- a/drivers/usb/serial/f81232.c
 +++ b/drivers/usb/serial/f81232.c
 @@ -23,6 +23,7 @@
  #include linux/uaccess.h
  #include linux/usb.h
  #include linux/usb/serial.h
 +#include linux/serial_reg.h
  
  static const struct usb_device_id id_table[] = {
   { USB_DEVICE(0x1934, 0x0706) },
 @@ -30,6 +31,13 @@ static const struct usb_device_id id_table[] = {
  };
  MODULE_DEVICE_TABLE(usb, id_table);
  
 +/* USB Control EP parameter */
 +#define F81232_REGISTER_REQUEST 0xA0
 +#define F81232_GET_REGISTER 0xc0

Indent these define values using a tab as well.

 +
 +#define SERIAL_BASE_ADDRESS 0x0120
 +#define MODEM_STATUS_REGISTER  (0x06 + SERIAL_BASE_ADDRESS)
 +
  #define CONTROL_DTR  0x01
  #define CONTROL_RTS  0x02
  
 @@ -48,19 +56,92 @@ struct f81232_private {
   spinlock_t lock;
   u8 line_control;
   u8 modem_status;
 +

Newline not needed.

 + struct work_struct interrupt_work;
 + struct usb_serial_port *port;
  };
  
 -static void f81232_update_line_status(struct usb_serial_port *port,
 +static int f81232_get_register(struct usb_serial_port *port,
 +   u16 reg, u8 *data)

No need to break this line any more.

 +{
 + int status;
 + struct usb_device *dev = port-serial-dev;
 +
 + status = usb_control_msg(dev,
 +  usb_rcvctrlpipe(dev, 0),
 +  F81232_REGISTER_REQUEST,
 +  F81232_GET_REGISTER,
 +  reg,
 +  0,
 +  data,
 +  sizeof(*data),
 +  USB_CTRL_GET_TIMEOUT);

There's some odd indentation above and in other patches as well (in this
case two tabs and one space). Please either just tabs or, if you prefer,
add additional spaces to align all the arguments with the first.

 + if (status  0)

Zero is actually also an error here (returned length was 0). You may
want to let this function return -EIO in that case (and perhaps also
return 0 on success) or you need to fix this at every call site.

 + dev_err(port-dev, %s status: %d\n, __func__, status);

Please spell out that there was an error, for example,

%s failed: %d\n

 +
 + return status;
 +}

Could you add the get/set register helpers in a separate preparatory patch?

 +
 +static void f81232_read_msr(struct usb_serial_port *port)
 +{
 + int status;
 + unsigned long flags;
 + u8 current_msr;
 + struct tty_struct *tty;
 + struct f81232_private *priv = usb_get_serial_port_data(port);
 +
 + status = f81232_get_register(port, MODEM_STATUS_REGISTER,
 + current_msr);

You cannot use a variable on the stack for usb_control_msg as the buffer
must be DMA-able (on all platforms). Use kzalloc.

 + if (status  0) {
 + /* Retain the error even reported in f81232_get_register()
 +  to make debug easily :D */

No need for this comment.

 + dev_err(port-dev, %s fail, status: %d\n, __func__, status);
 + return;
 + }
 +
 + if (!(current_msr  UART_MSR_ANY_DELTA))
 + return;
 +
 + tty = tty_port_tty_get(port-port);
 + if (tty) {
 + if (current_msr  UART_MSR_DDCD) {
 + usb_serial_handle_dcd_change(port, tty,
 + current_msr  UART_MSR_DCD);

Please indent continuation lines at least two tabs further (i.e add one
more tab here).

 + }
 +
 + tty_kref_put(tty);
 + }

You should rewrite this so you only get the tty reference if you're
actually gonna use it.

 +
 + spin_lock_irqsave(priv-lock, flags);
 + priv-modem_status = current_msr;
 + spin_unlock_irqrestore(priv-lock, flags);
 +
 + wake_up_interruptible(port-port.delta_msr_wait);
 +}
 +
 +static void f81232_update_modem_status(struct usb_serial_port *port,

This one should still be called f81232_update_line_status.

 unsigned char *data,
 unsigned int actual_length)

Just call it length or size, you can use size_t.

  {
 - /*
 -  * FIXME: Update port-icount, and call
 -  *
 -  *  wake_up_interruptible(port-port.delta_msr_wait);
 -  *
 -  *on MSR changes.
 -  */
 + struct f81232_private *priv = usb_get_serial_port_data(port);
 +
 + if (!actual_length)

[PATCH V6 02/10] USB: f81232: implement read IIR/MSR with endpoint

2015-02-15 Thread Peter Hung

The interrupt Endpoint will report current IIR. If we got IIR with MSR Changed
, We will do read MSR with interrupt_work worker to do f81232_read_msr() func.

Signed-off-by: Peter Hung 
---
 drivers/usb/serial/f81232.c | 109 
 1 file changed, 100 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 669a2f2..ec4609d 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 
 static const struct usb_device_id id_table[] = {
{ USB_DEVICE(0x1934, 0x0706) },
@@ -30,6 +31,13 @@ static const struct usb_device_id id_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, id_table);
 
+/* USB Control EP parameter */
+#define F81232_REGISTER_REQUEST 0xA0
+#define F81232_GET_REGISTER 0xc0
+
+#define SERIAL_BASE_ADDRESS   0x0120
+#define MODEM_STATUS_REGISTER  (0x06 + SERIAL_BASE_ADDRESS)
+
 #define CONTROL_DTR0x01
 #define CONTROL_RTS0x02
 
@@ -48,19 +56,92 @@ struct f81232_private {
spinlock_t lock;
u8 line_control;
u8 modem_status;
+
+   struct work_struct interrupt_work;
+   struct usb_serial_port *port;
 };
 
-static void f81232_update_line_status(struct usb_serial_port *port,
+static int f81232_get_register(struct usb_serial_port *port,
+ u16 reg, u8 *data)
+{
+   int status;
+   struct usb_device *dev = port->serial->dev;
+
+   status = usb_control_msg(dev,
+usb_rcvctrlpipe(dev, 0),
+F81232_REGISTER_REQUEST,
+F81232_GET_REGISTER,
+reg,
+0,
+data,
+sizeof(*data),
+USB_CTRL_GET_TIMEOUT);
+   if (status < 0)
+   dev_err(>dev, "%s status: %d\n", __func__, status);
+
+   return status;
+}
+
+static void f81232_read_msr(struct usb_serial_port *port)
+{
+   int status;
+   unsigned long flags;
+   u8 current_msr;
+   struct tty_struct *tty;
+   struct f81232_private *priv = usb_get_serial_port_data(port);
+
+   status = f81232_get_register(port, MODEM_STATUS_REGISTER,
+   _msr);
+   if (status < 0) {
+   /* Retain the error even reported in f81232_get_register()
+to make debug easily :D */
+   dev_err(>dev, "%s fail, status: %d\n", __func__, status);
+   return;
+   }
+
+   if (!(current_msr & UART_MSR_ANY_DELTA))
+   return;
+
+   tty = tty_port_tty_get(>port);
+   if (tty) {
+   if (current_msr & UART_MSR_DDCD) {
+   usb_serial_handle_dcd_change(port, tty,
+   current_msr & UART_MSR_DCD);
+   }
+
+   tty_kref_put(tty);
+   }
+
+   spin_lock_irqsave(>lock, flags);
+   priv->modem_status = current_msr;
+   spin_unlock_irqrestore(>lock, flags);
+
+   wake_up_interruptible(>port.delta_msr_wait);
+}
+
+static void f81232_update_modem_status(struct usb_serial_port *port,
  unsigned char *data,
  unsigned int actual_length)
 {
-   /*
-* FIXME: Update port->icount, and call
-*
-*  wake_up_interruptible(>port.delta_msr_wait);
-*
-*on MSR changes.
-*/
+   struct f81232_private *priv = usb_get_serial_port_data(port);
+
+   if (!actual_length)
+   return;
+
+   switch (data[0] & 0x07) {
+   case 0x00: /* msr change */
+   dev_dbg(>dev, "IIR: MSR Change: %x\n", data[0]);
+   schedule_work(>interrupt_work);
+   break;
+   case 0x02: /* tx-empty */
+   break;
+   case 0x04: /* rx data available */
+   break;
+   case 0x06: /* lsr change */
+   /* we can forget it. the LSR will read from bulk-in */
+   dev_dbg(>dev, "IIR: LSR Change: %x\n", data[0]);
+   break;
+   }
 }
 
 static void f81232_read_int_callback(struct urb *urb)
@@ -91,7 +172,7 @@ static void f81232_read_int_callback(struct urb *urb)
usb_serial_debug_data(>dev, __func__,
  urb->actual_length, urb->transfer_buffer);
 
-   f81232_update_line_status(port, data, actual_length);
+   f81232_update_modem_status(port, data, actual_length);
 
 exit:
retval = usb_submit_urb(urb, GFP_ATOMIC);
@@ -270,6 +351,14 @@ static int f81232_ioctl(struct tty_struct *tty,
return -ENOIOCTLCMD;
 }
 
+static void  f81232_interrupt_work(struct work_struct *work)
+{
+   struct f81232_private *priv =
+   container_of(work, struct f81232_private, interrupt_work);
+
+

[PATCH V6 02/10] USB: f81232: implement read IIR/MSR with endpoint

2015-02-15 Thread Peter Hung

The interrupt Endpoint will report current IIR. If we got IIR with MSR Changed
, We will do read MSR with interrupt_work worker to do f81232_read_msr() func.

Signed-off-by: Peter Hung hpeter+linux_ker...@gmail.com
---
 drivers/usb/serial/f81232.c | 109 
 1 file changed, 100 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 669a2f2..ec4609d 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -23,6 +23,7 @@
 #include linux/uaccess.h
 #include linux/usb.h
 #include linux/usb/serial.h
+#include linux/serial_reg.h
 
 static const struct usb_device_id id_table[] = {
{ USB_DEVICE(0x1934, 0x0706) },
@@ -30,6 +31,13 @@ static const struct usb_device_id id_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, id_table);
 
+/* USB Control EP parameter */
+#define F81232_REGISTER_REQUEST 0xA0
+#define F81232_GET_REGISTER 0xc0
+
+#define SERIAL_BASE_ADDRESS   0x0120
+#define MODEM_STATUS_REGISTER  (0x06 + SERIAL_BASE_ADDRESS)
+
 #define CONTROL_DTR0x01
 #define CONTROL_RTS0x02
 
@@ -48,19 +56,92 @@ struct f81232_private {
spinlock_t lock;
u8 line_control;
u8 modem_status;
+
+   struct work_struct interrupt_work;
+   struct usb_serial_port *port;
 };
 
-static void f81232_update_line_status(struct usb_serial_port *port,
+static int f81232_get_register(struct usb_serial_port *port,
+ u16 reg, u8 *data)
+{
+   int status;
+   struct usb_device *dev = port-serial-dev;
+
+   status = usb_control_msg(dev,
+usb_rcvctrlpipe(dev, 0),
+F81232_REGISTER_REQUEST,
+F81232_GET_REGISTER,
+reg,
+0,
+data,
+sizeof(*data),
+USB_CTRL_GET_TIMEOUT);
+   if (status  0)
+   dev_err(port-dev, %s status: %d\n, __func__, status);
+
+   return status;
+}
+
+static void f81232_read_msr(struct usb_serial_port *port)
+{
+   int status;
+   unsigned long flags;
+   u8 current_msr;
+   struct tty_struct *tty;
+   struct f81232_private *priv = usb_get_serial_port_data(port);
+
+   status = f81232_get_register(port, MODEM_STATUS_REGISTER,
+   current_msr);
+   if (status  0) {
+   /* Retain the error even reported in f81232_get_register()
+to make debug easily :D */
+   dev_err(port-dev, %s fail, status: %d\n, __func__, status);
+   return;
+   }
+
+   if (!(current_msr  UART_MSR_ANY_DELTA))
+   return;
+
+   tty = tty_port_tty_get(port-port);
+   if (tty) {
+   if (current_msr  UART_MSR_DDCD) {
+   usb_serial_handle_dcd_change(port, tty,
+   current_msr  UART_MSR_DCD);
+   }
+
+   tty_kref_put(tty);
+   }
+
+   spin_lock_irqsave(priv-lock, flags);
+   priv-modem_status = current_msr;
+   spin_unlock_irqrestore(priv-lock, flags);
+
+   wake_up_interruptible(port-port.delta_msr_wait);
+}
+
+static void f81232_update_modem_status(struct usb_serial_port *port,
  unsigned char *data,
  unsigned int actual_length)
 {
-   /*
-* FIXME: Update port-icount, and call
-*
-*  wake_up_interruptible(port-port.delta_msr_wait);
-*
-*on MSR changes.
-*/
+   struct f81232_private *priv = usb_get_serial_port_data(port);
+
+   if (!actual_length)
+   return;
+
+   switch (data[0]  0x07) {
+   case 0x00: /* msr change */
+   dev_dbg(port-dev, IIR: MSR Change: %x\n, data[0]);
+   schedule_work(priv-interrupt_work);
+   break;
+   case 0x02: /* tx-empty */
+   break;
+   case 0x04: /* rx data available */
+   break;
+   case 0x06: /* lsr change */
+   /* we can forget it. the LSR will read from bulk-in */
+   dev_dbg(port-dev, IIR: LSR Change: %x\n, data[0]);
+   break;
+   }
 }
 
 static void f81232_read_int_callback(struct urb *urb)
@@ -91,7 +172,7 @@ static void f81232_read_int_callback(struct urb *urb)
usb_serial_debug_data(port-dev, __func__,
  urb-actual_length, urb-transfer_buffer);
 
-   f81232_update_line_status(port, data, actual_length);
+   f81232_update_modem_status(port, data, actual_length);
 
 exit:
retval = usb_submit_urb(urb, GFP_ATOMIC);
@@ -270,6 +351,14 @@ static int f81232_ioctl(struct tty_struct *tty,
return -ENOIOCTLCMD;
 }
 
+static void  f81232_interrupt_work(struct work_struct *work)
+{
+   struct

Re: [PATCH V6 02/10] USB: f81232: implement read IIR/MSR with endpoint

Re: [PATCH V6 02/10] USB: f81232: implement read IIR/MSR with endpoint

[PATCH V6 02/10] USB: f81232: implement read IIR/MSR with endpoint

[PATCH V6 02/10] USB: f81232: implement read IIR/MSR with endpoint

4 matches

Site Navigation

Mail list logo

Footer information