Hi Greg, Thanks for your review > From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> > > > +struct ljca_adapter { > > + struct usb_interface *intf; > > + struct usb_device *usb_dev; > > + struct device *dev; > > + > > + unsigned int rx_pipe; > > + unsigned int tx_pipe; > > + > > + /* urb for recv */ > > + struct urb *rx_urb; > > + /* buffer for recv */ > > + void *rx_buf; > > + unsigned int rx_len; > > + > > + /* external buffer for recv */ > > + void *ex_buf; > > Shouldn't buffers be u8*? Ack, this has been fixed in my local version. > > > + > > +/* process command ack */ > > +static void ljca_handle_cmd_ack(struct ljca_adapter *adap, > > + struct ljca_msg *header) > > +{ > > + struct ljca_msg *tx_header = adap->tx_buf; > > + unsigned int actual_len = 0; > > + unsigned int ibuf_len; > > + unsigned long flags; > > + void *ibuf; > > + > > + spin_lock_irqsave(&adap->lock, flags); > > Why not use the functionality in cleanup.h for this lock? Makes this function > much simpler. Ack, I understand the cleanup.h now, and use the functionality as much as I can. > > > + > > + if (tx_header->type != header->type || tx_header->cmd != header->cmd) > { > > + spin_unlock_irqrestore(&adap->lock, flags); > > + > > No need for a blank line. > > And how can these things happen? No need to return an error if this is the case? Ack, removed the blank line and added error print here. > > > +static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd, > > + const void *obuf, unsigned int obuf_len, void *ibuf, > > + unsigned int ibuf_len, bool ack, unsigned long timeout) > > That's a lot of function parameters, whyh so many? > > And why void *? That should never be used in an internal function where you > know the real type. Ack, have switched void * to real type > > > +{ > > > +#else > > +static void ljca_auxdev_acpi_bind(struct ljca_adapter *adap, > > + struct auxiliary_device *auxdev, > > + u64 adr, u8 id) > > +{ > > +} > > +#endif > > Can't this go in a .h file? #ifdef in .c files are frowned apon. Ack, this has been removed > > > --- /dev/null > > +++ b/include/linux/usb/ljca.h > > + > > +struct ljca_client { > > + u8 type; > > + u8 id; > > + struct list_head link; > > + struct auxiliary_device auxdev; > > + struct ljca_adapter *adapter; > > + > > + void *context; > > + ljca_event_cb_t event_cb; > > + /* lock to protect event_cb */ > > + spinlock_t event_cb_lock; > > +}; > > + > > +struct ljca_gpio_info { > > + unsigned int num; > > + DECLARE_BITMAP(valid_pin_map, LJCA_MAX_GPIO_NUM); }; > > + > > +struct ljca_i2c_info { > > + u8 id; > > + u8 capacity; > > + u8 intr_pin; > > +}; > > + > > +struct ljca_spi_info { > > + u8 id; > > + u8 capacity; > > +}; > > No documentation for these other public structures? Ack, This has been addressed. If no more comments, I will send out v16 for review. Thanks Wentong > > thanks, > > greg k-h