On Wed, Sep 06, 2023 at 11:22:57AM +0800, Wentong Wu wrote:
> +struct ljca_bank_descriptor {
> + u8 bank_id;
> + u8 pin_num;
> +
> + /* 1 bit for each gpio, 1 means valid */
> + u32 valid_pins;
> +} __packed;
This is an unaligned access, do you mean to have that?
> +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*?
> +static void ljca_handle_event(struct ljca_adapter *adap,
> + struct ljca_msg *header)
> +{
> + struct ljca_client *client;
> +
> + list_for_each_entry(client, &adap->client_list, link) {
> + /*
> + * FIXME: currently only GPIO register event callback.
When is this fixme going to be addressed?
> + * firmware message structure should include id when
> + * multiple same type clients register event callback.
> + */
> + if (client->type == header->type) {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&client->event_cb_lock, flags);
> + client->event_cb(client->context, header->cmd,
> + header->data, header->len);
> + spin_unlock_irqrestore(&client->event_cb_lock, flags);
> +
> + break;
> + }
> + }
> +}
> +
> +/* 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.
> +
> + 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?
> +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.
> +{
> + unsigned int msg_len = sizeof(struct ljca_msg) + obuf_len;
> + struct ljca_msg *header = adap->tx_buf;
> + unsigned long flags;
> + unsigned int actual;
> + int ret = 0;
> +
> + if (adap->disconnect)
> + return -ENODEV;
> +
> + if (msg_len > adap->tx_buf_len)
> + return -EINVAL;
> +
> + mutex_lock(&adap->mutex);
> +
> + spin_lock_irqsave(&adap->lock, flags);
2 locks? Why 2 locks for the same structure?
> +
> + header->type = type;
> + header->cmd = cmd;
> + header->len = obuf_len;
> + if (obuf)
> + memcpy(header->data, obuf, obuf_len);
> +
> + header->flags = LJCA_CMPL_FLAG | (ack ? LJCA_ACK_FLAG : 0);
> +
> + adap->ex_buf = ibuf;
> + adap->ex_buf_len = ibuf_len;
> + adap->actual_length = 0;
> +
> + spin_unlock_irqrestore(&adap->lock, flags);
> +
> + reinit_completion(&adap->cmd_completion);
> +
> + usb_autopm_get_interface(adap->intf);
> +
> + ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header,
> + msg_len, &actual, LJCA_WRITE_TIMEOUT_MS);
This function is slow. Really slow. You drop the spinlock which is
good, but the mutex is still held. Does this call have to be
synchronous?
> +
> + usb_autopm_put_interface(adap->intf);
> +
> + if (!ret && ack) {
> + ret = wait_for_completion_timeout(&adap->cmd_completion,
> + timeout);
> + if (ret < 0) {
> + goto out;
> + } if (!ret) {
> + ret = -ETIMEDOUT;
> + goto out;
> + }
> + }
> + ret = adap->actual_length;
> +
> +out:
> + spin_lock_irqsave(&adap->lock, flags);
> + adap->ex_buf = NULL;
> + adap->ex_buf_len = 0;
> +
> + memset(header, 0, sizeof(*header));
Why?
> + spin_unlock_irqrestore(&adap->lock, flags);
> +
> + mutex_unlock(&adap->mutex);
> +
> + return ret;
> +}
> +
> +int ljca_transfer(struct ljca_client *client, u8 cmd, const void *obuf,
Again, drop all void * please, use real types in your apis.
> +#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.
> +static int ljca_enumerate_clients(struct ljca_adapter *adap)
> +{
> + int ret;
> +
> + ret = ljca_reset_handshake(adap);
> + if (ret)
> + return ret;
> +
> + ret = ljca_enumerate_gpio(adap);
> + if (ret)
> + dev_warn(adap->dev, "enumerate GPIO error\n");
> +
> + ret = ljca_enumerate_i2c(adap);
> + if (ret)
> + dev_warn(adap->dev, "enumerate I2C error\n");
> +
> + ret = ljca_enumerate_spi(adap);
> + if (ret)
> + dev_warn(adap->dev, "enumerate SPI error\n");
You warn about these things, but keep on saying the code is working
properly with a return of:
> + return 0;
That's not good. Why not unwind properly and handle the error?
> --- /dev/null
> +++ b/include/linux/usb/ljca.h
> @@ -0,0 +1,113 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2023, Intel Corporation. All rights reserved.
> + */
> +#ifndef _LINUX_USB_LJCA_H_
> +#define _LINUX_USB_LJCA_H_
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/list.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +
> +#define LJCA_MAX_GPIO_NUM 64
> +
> +#define auxiliary_dev_to_ljca_client(auxiliary_dev) \
> + container_of(auxiliary_dev, struct ljca_client, auxdev)
> +
> +struct ljca_adapter;
> +
> +/**
> + * typedef ljca_event_cb_t - event callback function signature
> + *
> + * @context: the execution context of who registered this callback
> + * @cmd: the command from device for this event
> + * @evt_data: the event data payload
> + * @len: the event data payload length
> + *
> + * The callback function is called in interrupt context and the data payload is
> + * only valid during the call. If the user needs later access of the data, it
> + * must copy it.
> + */
> +typedef void (*ljca_event_cb_t)(void *context, u8 cmd, const void *evt_data, int len);
> +
> +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?
thanks,
greg k-h
