RE: [PATCH v15 1/4] usb: Add support for Intel LJCA device

["Wu, Wentong" <wentong.wu@xxxxxxxxx>]

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