Re: [PATCH v2 2/2] tools arch x86: Add dell-uart-backlight-emulator

[Andy Shevchenko <andy@kernel.org>]

On Mon, May 13, 2024 at 01:15:51PM +0200, Hans de Goede wrote:
> Dell All In One (AIO) models released after 2017 use a backlight controller
> board connected to an UART.
> 
> Add a small emulator to allow development and testing of
> the drivers/platform/x86/dell/dell-uart-backlight.c driver for
> this board, without requiring access to an actual Dell All In One.

...

> +	if (argc != 2) {
> +		fprintf(stderr, "Invalid or missing arguments\n");
> +		fprintf(stderr, "Usage: %s <serial-port>\n", argv[0]);
> +		return 1;
> +	}
> +
> +	serial_fd = open(argv[1], O_RDWR | O_NOCTTY);
> +	if (serial_fd == -1) {
> +		fprintf(stderr, "Error opening %s: %s\n", argv[1], strerror(errno));
> +		return 1;

So, looking at the `man error` it works like your custom approach with an
additional things.

Also, don't you want to use either different error codes (above is +1 and all
below seems using -1), or be consistent and return -1 always?

> +	}

...

> +		/* Respond with <total-len> <cmd> <data...> <csum> */
> +		response[0] = len + 3; /* response length in bytes */
> +		response[1] = buf[1];  /* ack cmd */
> +		csum = dell_uart_checksum(response, len + 2);
> +		response[len + 2] = csum;

> +		ret = write(serial_fd, response, len + 3);

response[0] can be reused here.

> +		if (ret != (len + 3))

And here.

> +			fprintf(stderr, "Error writing %d bytes: %d\n",
> +				len + 3, ret);
> +	}

-- 
With Best Regards,
Andy Shevchenko