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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 5/13/24 2:46 PM, Andy Shevchenko wrote:
> 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.

But that is a GNU / glibc extension. I would prefer to stick with plain libc
here, so that this can be build with other libc-s too. 
> 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?

the +1 code is for when the cmdline argument is likely wrong , -1 / 255 is
for IO errors.

> 
>> +	}
> 
> ...
> 
>> +		/* 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.

Ack will fix.

Regards,

Hans


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