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

Wu, Wentong
2023-09-12T03:50:10Z

Hi Greg,

Thanks for your review

> From: Greg KH <gregkh@linuxfoundation.org>
>
> > +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