Messages in this thread | | | Date | Mon, 26 Dec 2022 16:12:07 +0800 | From | Tzung-Bi Shih <> | Subject | Re: [PATCH v12 1/3] platform/chrome: cros_ec_uart: Add cros-ec-uart transport layer |
| |
On Fri, Dec 09, 2022 at 09:26:22AM -0700, Mark Hasemeyer wrote: > diff --git a/drivers/platform/chrome/cros_ec_uart.c b/drivers/platform/chrome/cros_ec_uart.c [...] > +static int cros_ec_uart_rx_bytes(struct serdev_device *serdev, > + const u8 *data, > + size_t count) > +{ > + struct ec_host_response *response;
There are 2 "response" in the context: struct response_info vs. struct ec_host_response. The pointer `response` here is only used for setting `exp_len`.
I would suggest to introduce another pointer: struct response_info *resp = &ec_uart->response; So that most of the following "ec_uart->response." could simplify to "resp->".
[...] > + /* Read data_len if we received response header and if exp_len was not read before. */ > + if (ec_uart->response.size >= sizeof(*response) && > + ec_uart->response.exp_len == 0) { > + response = (struct ec_host_response *) ec_uart->response.data;
Please drop the extra space.
> +static int cros_ec_uart_pkt_xfer(struct cros_ec_device *ec_dev, > + struct cros_ec_command *ec_msg) > +{ > + struct cros_ec_uart *ec_uart = ec_dev->priv; > + struct serdev_device *serdev = ec_uart->serdev; > + struct ec_host_response *response;
Ditto, if using another pointer to struct response_info could simplify the function a bit.
> + unsigned int len; > + int ret, i; > + u8 sum = 0;
To make it clear, please drop the initialization and set it to 0 just right before it is used.
[...] > + /* Validate checksum */ > + for (i = 0; i < sizeof(*response) + response->data_len; i++) > + sum += ec_dev->din[i];
The function is long. Setting sum to 0 before the loop improves the readability.
> +static int cros_ec_uart_probe(struct serdev_device *serdev) > +{ > + struct device *dev = &serdev->dev; > + struct cros_ec_device *ec_dev; > + struct cros_ec_uart *ec_uart; > + int ret; > + > + ec_uart = devm_kzalloc(dev, sizeof(*ec_uart), GFP_KERNEL); > + if (!ec_uart) > + return -ENOMEM; > + > + ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL); > + if (!ec_dev) > + return -ENOMEM; > + > + ec_uart->serdev = serdev;
The assignment can move somewhere before calling cros_ec_uart_acpi_probe().
> + > + ret = devm_serdev_device_open(dev, ec_uart->serdev);
Using `ec_uart->serdev` here makes less sense. Please use `serdev` directly.
| |