Re: [PATCH 1/2] platform/x86: Add new Dell UART backlight driver

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4465 bytes --]

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.