Hi,
On 5/13/24 3:14 PM, Ilpo Järvinen wrote:
> On Mon, 13 May 2024, Hans de Goede wrote:
>> On 5/13/24 2:12 PM, Ilpo Järvinen wrote:
>>> On Mon, 13 May 2024, Hans de Goede wrote:
>>>> On 5/13/24 10:34 AM, Ilpo Järvinen wrote:
>>>>> On Sun, 12 May 2024, Hans de Goede wrote:
>
>>>>>> +
>>>>>> + 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.
>>>
>>> So effectively you want to terminate the loop on two conditions here:
>>>
>>> a) dell_bl->resp_idx == dell_bl->resp_len (complete frame)
>>> a) if i == len (not yet received a full frame)
>>>
>>> Why not code those rather than the current goto & continue madness?
>>>
>>> Then, after the loop, you can test:
>>>
>>> if (dell_bl->resp_idx == dell_bl->resp_len) {
>>> // calc checksum, etc.
>>> }
>>>
>>> ?
>>
>> Ok, I've added the following change for v3:
>>
>> diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c
>> index bf5b12efcb19..66d8c6ddcb83 100644
>> --- a/drivers/platform/x86/dell/dell-uart-backlight.c
>> +++ b/drivers/platform/x86/dell/dell-uart-backlight.c
>> @@ -87,6 +87,7 @@ static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl,
>> dell_bl->status = -EBUSY;
>> dell_bl->resp = resp;
>> dell_bl->resp_idx = 0;
>> + dell_bl->resp_len = -1; /* Invalid / unset */
>> dell_bl->resp_max_len = resp_max_len;
>> dell_bl->pending_cmd = cmd[1];
>>
>> @@ -219,7 +219,7 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data,
>> return len;
>> }
>>
>> - for (i = 0; i < len; i++) {
>> + for (i = 0; i < len && dell_bl->resp_idx != dell_bl->resp_len; i++, dell_bl->resp_idx++) {
>> dell_bl->resp[dell_bl->resp_idx] = data[i];
>>
>> switch (dell_bl->resp_idx) {
>> @@ -228,46 +228,41 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data,
>> if (dell_bl->resp_len < MIN_RESP_LEN) {
>> dev_err(dell_bl->dev, "Response length too small %d < %d\n",
>> dell_bl->resp_len, MIN_RESP_LEN);
>> - dell_bl->status = -EIO;
>> - goto wakeup;
>> + goto error;
>> }
>>
>> if (dell_bl->resp_len > dell_bl->resp_max_len) {
>> 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;
>> + goto error;
>> }
>> break;
>> case RESP_CMD: /* CMD byte */
>> if (dell_bl->resp[RESP_CMD] != dell_bl->pending_cmd) {
>> dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n",
>> dell_bl->resp[RESP_CMD], dell_bl->pending_cmd);
>> - dell_bl->status = -EIO;
>> - goto wakeup;
>> + goto error;
>> }
>> break;
>> }
>> + }
>>
>> - dell_bl->resp_idx++;
>> - if (dell_bl->resp_idx < dell_bl->resp_len)
>> - continue;
>> -
>> + if (dell_bl->resp_idx == dell_bl->resp_len) {
>> 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;
>> + goto error;
>> }
>> -
>> dell_bl->status = 0; /* Success */
>> - goto wakeup;
>> + wake_up(&dell_bl->wait_queue);
>> + return i;
>> }
>>
>> return len;
>>
>> -wakeup:
>> +error:
>> + dell_bl->status = -EIO;
>> wake_up(&dell_bl->wait_queue);
>> return i + 1;
>> }
>
> Thanks, this is way easier to follow.
I'm glad you like it.
There is a little bug in this version though, the goto error on the checksum fail
case returns i + i, which should be i in that case, I'll just drop the goto there and
instead always use the return i already present at the end of the
"if (dell_bl->resp_idx == dell_bl->resp_len) { }" block.
Regards,
hans
>
