Re: [PATCH V8 04/10] USB: f81232: implement read IIR/MSR with endpoint

2015-03-16 Thread Johan Hovold 
On Mon, Mar 16, 2015 at 11:08:29AM +0800, Peter Hung wrote:
 Hello,
 
 Johan Hovold 於 2015/3/14 下午 08:02 寫道:
  On Thu, Feb 26, 2015 at 06:02:10PM +0800, Peter Hung wrote:
  +  if (status != sizeof(*val)) {
  +  dev_err(port-dev, %s failed status: %d\n, __func__, status);
  +
  +  if (status == 0)
  +  status = -EIO;
  +  else
  +  status = usb_translate_errors(status);
 
  Could you rewrite this as
 
  if (status  0)
  status = usb_translate_errors(status);
  else
  status = 0;
 
 In my definition the return value of set/getregister(), 0 is success, 
 negative values are errors. The function usb_control_msg() return value 
 is success transmited/received byte. It's maybe return 0. I want to 
 treat 0 with error(-EIO). But if pass 0 to usb_translate_errors(), It 
 will return 0 back. So I need especially handle with status == 0.

I meant to write

if (status  0)
status = usb_translate_errors(status);
else
status = -EIO;

which I think is more readable.

Sorry for the confusion.

Johan
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V8 04/10] USB: f81232: implement read IIR/MSR with endpoint

2015-03-16 Thread Peter Hung 

Hello,

Johan Hovold 於 2015/3/16 下午 04:55 寫道:

On Mon, Mar 16, 2015 at 11:08:29AM +0800, Peter Hung wrote:

Could you rewrite this as

if (status  0)
status = usb_translate_errors(status);
else
status = 0;


In my definition the return value of set/getregister(), 0 is success,
negative values are errors. The function usb_control_msg() return value
is success transmited/received byte. It's maybe return 0. I want to
treat 0 with error(-EIO). But if pass 0 to usb_translate_errors(), It
will return 0 back. So I need especially handle with status == 0.


I meant to write

if (status  0)
status = usb_translate_errors(status);
else
status = -EIO;

which I think is more readable.


It's looks more readable of the style that you mentioned.
Thanks for your advice, I'll add it with next patch.

--
With Best Regards,
Peter Hung
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V8 04/10] USB: f81232: implement read IIR/MSR with endpoint

2015-03-15 Thread Peter Hung 

Hello,

Johan Hovold 於 2015/3/14 下午 08:02 寫道:

On Thu, Feb 26, 2015 at 06:02:10PM +0800, Peter Hung wrote:

+   if (status != sizeof(*val)) {
+   dev_err(port-dev, %s failed status: %d\n, __func__, status);
+
+   if (status == 0)
+   status = -EIO;
+   else
+   status = usb_translate_errors(status);


Could you rewrite this as

if (status  0)
status = usb_translate_errors(status);
else
status = 0;


In my definition the return value of set/getregister(), 0 is success, 
negative values are errors. The function usb_control_msg() return value 
is success transmited/received byte. It's maybe return 0. I want to 
treat 0 with error(-EIO). But if pass 0 to usb_translate_errors(), It 
will return 0 back. So I need especially handle with status == 0.


I'll keep this sections.

Thanks for review
--
With Best Regards,
Peter Hung
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V8 04/10] USB: f81232: implement read IIR/MSR with endpoint

2015-03-14 Thread Johan Hovold 
On Thu, Feb 26, 2015 at 06:02:10PM +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()
 function.
 
 Signed-off-by: Peter Hung hpeter+linux_ker...@gmail.com
 ---
  drivers/usb/serial/f81232.c | 126 
 +---
  1 file changed, 118 insertions(+), 8 deletions(-)
 
 diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
 index cf5b902..46a81ef 100644
 --- a/drivers/usb/serial/f81232.c
 +++ b/drivers/usb/serial/f81232.c
 @@ -31,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)

Could you indent the values using two tabs so the
INTERRUPT_ENABLE_REGISTER in a following patch will also fit?

 +
  #define CONTROL_DTR  0x01
  #define CONTROL_RTS  0x02
  
 @@ -49,19 +56,112 @@ struct f81232_private {
   struct mutex lock;
   u8 line_control;
   u8 modem_status;
 + struct work_struct interrupt_work;
 + struct usb_serial_port *port;
  };
  
 +static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 
 *val)
 +{
 + int status;
 + u8 *tmp;
 + struct usb_device *dev = port-serial-dev;
 +
 + tmp = kmalloc(sizeof(*val), GFP_KERNEL);
 + if (!tmp)
 + return -ENOMEM;
 +
 + status = usb_control_msg(dev,
 + usb_rcvctrlpipe(dev, 0),
 + F81232_REGISTER_REQUEST,
 + F81232_GET_REGISTER,
 + reg,
 + 0,
 + tmp,
 + sizeof(u8),

Please use sizeof(*val) here as well for consistency,

 + USB_CTRL_GET_TIMEOUT);
 + if (status != sizeof(*val)) {
 + dev_err(port-dev, %s failed status: %d\n, __func__, status);
 +
 + if (status == 0)
 + status = -EIO;
 + else
 + status = usb_translate_errors(status);

Could you rewrite this as

if (status  0)
status = usb_translate_errors(status);
else
status = 0;

 + } else {
 + status = 0;
 + *val = *tmp;
 + }
 +
 + kfree(tmp);
 + return status;
 +}

Same comments apply to set_register in a follow up patch.

Looks good otherwise,
Johan
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html