Hi Ilpo,
Thank you for the review.
On 5/13/24 10:34 AM, Ilpo Järvinen wrote:
> On Sun, 12 May 2024, Hans de Goede wrote:
<snip>
>> + dell_bl->status = 0;
>> + dell_bl->resp = resp;
>> + dell_bl->resp_idx = 0;
>> + dell_bl->resp_max_len = resp_max_len;
>> + dell_bl->pending_cmd = cmd[1];
>> +
>> + /* The TTY buffer should be big enough to take the entire cmd in one go */
>> + ret = serdev_device_write_buf(to_serdev_device(dell_bl->dev), cmd, cmd_len);
>> + if (ret != cmd_len) {
>> + dev_err(dell_bl->dev, "Error writing command: %d\n", ret);
>> + ret = (ret < 0) ? ret : -EIO;
>> + goto out;
>> + }
>> +
>> + ret = wait_event_timeout(dell_bl->wait_queue, dell_bl->status, DELL_BL_TIMEOUT);
>> + if (ret == 0) {
>> + dev_err(dell_bl->dev, "Timed out waiting for response.\n");
>> + dell_bl->status = -ETIMEDOUT;
>> + }
>> +
>> + if (dell_bl->status == 1)
>> + ret = 0;
>> + else
>> + ret = dell_bl->status;
>
> I wonder if it would make dell_bl->status easier to follow if you'd first
> make it -EBUSY instead of 0 and set it to 0 on success?
>
> It would basically be normal errno behavior without extra values then and
> you wouldn't need to map it into return value here.
Ack, done for v2.
>
>> +out:
>> + mutex_unlock(&dell_bl->mutex);
>> + return ret;
>> +}
>> +
>> +static int dell_uart_set_brightness(struct dell_uart_backlight *dell_bl, int brightness)
>> +{
>> + /*
>> + * Set Brightness level: Application uses this command to set brightness.
>> + * Command: 0x8A 0x0B <brightness-level> Checksum (Length:4 Type:0x0A Cmd:0x0B)
>> + * <brightness-level> ranges from 0~100.
>
> Why ~ character, is this just - ?
Yes just -, the ~ came from the original driver I based this on I'll fix this for v2.
>> + * Return data: 0x03 0x0B 0xF1 (Length:3 Cmd:0x0B Checksum:0xF1)
>
> All these commands return header + echo cmd + (optional) data + checksum.
> I'm not sure why they all need a comment about it...
>
> It's also slightly misleading to call it "Return data" which can be
> misinterpreted to mean the return value of this function which is not
> correct (code calls it resp(onse) anyway so if it's necessary, use
> response data instead).
>
>> + */
>> + u8 set_brightness[] = { 0x8A, 0x0B, 0x00, 0x00 };
>
> Use #defines instead of literals.
Ack, will fix for v2.
>
> I think it makes the entire comments about the commands mostly useless
> when these are converted into properly named defines.
Ack, will drop.
>
>> + u8 resp[3];
>> +
>> + set_brightness[2] = brightness;
>> + set_brightness[3] = dell_uart_checksum(set_brightness, 3);
>
> Also, couldn't these be accessed through a struct to eliminate most of the
> magic indexes?
With the checksum at the end, this would require a VLA in the middle
of the struct (get_version return buffer contains more then 1 dat byte)
We could treat the checksum as an extra data byte, but then we are
mixing struct usage for some fields + using an array of bytes
approach for the data + checksum. For consistency I prefer to just
stick with one approach which means using the array of bytes approach
for everything.
>
>> + return dell_uart_bl_command(dell_bl, set_brightness, ARRAY_SIZE(set_brightness),
>> + resp, ARRAY_SIZE(resp));
>> +}
>> +
>> +static int dell_uart_get_brightness(struct dell_uart_backlight *dell_bl)
>> +{
>> + /*
>> + * Get Brightness level: Application uses this command to get brightness.
>> + * Command: 0x6A 0x0C 0x89 (Length:3 Type:0x0A Cmd:0x0C Checksum:0x89)
>> + * Return data: 0x04 0x0C Data Checksum
>> + * (Length:4 Cmd:0x0C Data:<brightness level>
>> + * Checksum: SUM(Length and Cmd and Data) xor 0xFF)
>> + * <brightness level> ranges from 0~100.
>
> ~ -> - ?
>
>> + */
>> + const u8 get_brightness[] = { 0x6A, 0x0C, 0x89 };
>> + u8 resp[4];
>> + int ret;
>> +
>> + ret = dell_uart_bl_command(dell_bl, get_brightness, ARRAY_SIZE(get_brightness),
>> + resp, ARRAY_SIZE(resp));
>> + if (ret)
>> + return ret;
>> +
>> + if (resp[0] != 4) {
>
> sizeof(resp)
Ack.
> but isn't this already checked when reading it??
No dell_uart_bl_receive() checks that the response will fit in the supplied
buffer and that it has a valid checksum but the controller may send a
response smaller then the passed in buffer and it will actually do this for
the get_version command.
>> + dev_err(dell_bl->dev, "Unexpected get brightness response length: %d\n", resp[0]);
>> + return -EIO;
>> + }
>> +
>> + if (resp[2] > 100) {
>
> Add #define.
Ack.
<snip>
>> +/*
>> + * There is no command to get backlight power status,
>> + * so we set the backlight power to "on" while initializing,
>> + * and then track and report its status by power variable
>
> Missing .
Ack.
<snip>
>> +static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, size_t len)
>> +{
>> + struct dell_uart_backlight *dell_bl = serdev_device_get_drvdata(serdev);
>> + size_t i;
>> + u8 csum;
>> +
>> + dev_dbg(dell_bl->dev, "Recv: %*ph\n", (int)len, data);
>> +
>> + /* Throw away unexpected bytes / remainder of response after an error */
>> + if (dell_bl->status) {
>
> As mentioned above, != -EBUSY ?
>
Ack, done for v2.
>> + dev_warn(dell_bl->dev, "Bytes received out of band, dropping them.\n");
>> + return len;
>> + }
>> +
>> + for (i = 0; i < len; i++) {
>> + dell_bl->resp[dell_bl->resp_idx] = data[i];
>> +
>> + switch (dell_bl->resp_idx) {
>> + case 0: /* Length byte */
>> + dell_bl->resp_len = dell_bl->resp[0];
>> + if (dell_bl->resp_len < DELL_BL_MIN_RESP_SIZE) {
>> + dev_err(dell_bl->dev, "Response length too small %d < %d\n",
>> + dell_bl->resp_len, DELL_BL_MIN_RESP_SIZE);
>> + dell_bl->status = -EIO;
>> + goto wakeup;
>> + } else if (dell_bl->resp_len > dell_bl->resp_max_len) {
>
> Unnecessary else because of the goto.
>
>> + dev_err(dell_bl->dev, "Response length too big %d > %d\n",
>> + dell_bl->resp_len, dell_bl->resp_max_len);
>> + dell_bl->status = -EIO;
>> + goto wakeup;
>> + }
>> + break;
>> + case 1: /* CMD byte */
>> + if (dell_bl->resp[1] != dell_bl->pending_cmd) {
>> + dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n",
>> + dell_bl->resp[1], dell_bl->pending_cmd);
>> + dell_bl->status = -EIO;
>> + goto wakeup;
>> + }
>> + break;
>> + }
>> +
>> + dell_bl->resp_idx++;
>> + if (dell_bl->resp_idx < dell_bl->resp_len)
>> + continue;
>> +
>> + csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1);
>> + if (dell_bl->resp[dell_bl->resp_len - 1] != csum) {
>> + dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n",
>> + dell_bl->resp[dell_bl->resp_len - 1], csum);
>> + dell_bl->status = -EIO;
>> + goto wakeup;
>> + }
>
> Why is the checksum calculation and check inside the loop??
The loop iterates over received bytes, which may contain extra data after the response, the:
dell_bl->resp_idx++;
if (dell_bl->resp_idx < dell_bl->resp_len)
continue;
continues looping until we have received all the expected bytes. So here, past this
check, we are are at the point where we have a complete response and then we verify it.
And on successful verification wake-up any waiters.
>
>> + dell_bl->status = 1; /* Success */
>
> As mentioned above, change this to = 0
Ack, done.
?
>
>> + goto wakeup;
>
> Huh? Now I'm totally lost how the control flow is supposed to go in this
> function. Can you rethink this loop so it actual makes sense and doesn't
> misuse gotos like this?
This is the receive() callback from the UART the loop consumes bytes received
by the UART. The gotos stop consuming bytes in 2 cases:
1. An error (unexpected data) is encountered.
2. A complete frame has been successfully received.
The checking of the checksum + goto wakeup at the end of the loop is for 2.
The:
return len;
after the loop indicates to the UART / tty-layer that all passed data
has been consumed and this path gets hit when the driver needs to wait
for more data because the response is not complete yet.
>> +
>> +wakeup:
>> + wake_up(&dell_bl->wait_queue);
>> + return i + 1;
>> +}
>> +
<snip>
>> + memset(&props, 0, sizeof(struct backlight_properties));
>
> Just assigned = {} when declaring.
Ack.
>> + props.type = BACKLIGHT_PLATFORM;
>> + props.brightness = ret;
>> + props.max_brightness = 100;
>
> Use #define.
Ack.
<snip>
>> + ret = serdev_device_add(serdev);
>> + if (ret) {
>> + dev_err(&pdev->dev, "error %d adding serdev\n", ret);
>> + serdev_device_put(serdev);
>> + return ret;
>> + }
>> +
>> + ret = serdev_device_driver_register(&dell_uart_bl_serdev_driver);
>> + if (ret) {
>> + serdev_device_remove(serdev);
>> + return ret;
>> + }
>> +
>> + /*
>> + * serdev device <-> driver matching relies on OF or ACPI matches and
>> + * neither is available here, manually bind the driver.
>> + */
>> + ret = device_driver_attach(&dell_uart_bl_serdev_driver.driver, &serdev->dev);
>> + if (ret) {
>> + serdev_device_driver_unregister(&dell_uart_bl_serdev_driver);
>> + serdev_device_remove(serdev);
>> + return ret;
>> + }
>
> The last two error branch could use normal rollback with goto.
Ack, will fix for v2.
Regards,
Hans
