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

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

Hi,

On 5/12/24 9:32 PM, Andy Shevchenko wrote:
> On Sun, May 12, 2024 at 7:24 PM Hans de Goede <hdegoede@redhat.com> 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.
> 
> ...
> 
>> +++ b/tools/arch/x86/dell-uart-backlight-emulator/Makefile
>> @@ -0,0 +1,19 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Makefile for Intel Software Defined Silicon provisioning tool
>> +
>> +dell-uart-backlight-emulator: dell-uart-backlight-emulator.c
>> +
>> +BINDIR ?= /usr/bin
>> +
>> +override CFLAGS += -O2 -Wall
>> +
>> +%: %.c
>> +       $(CC) $(CFLAGS) -o $@ $< $(LDFLAGS)
>> +
>> +.PHONY : clean
>> +clean :
>> +       @rm -f dell-uart-backlight-emulator
>> +
>> +install : dell-uart-backlight-emulator
>> +       install -d $(DESTDIR)$(BINDIR)
>> +       install -m 755 -p dell-uart-backlight-emulator $(DESTDIR)$(BINDIR)/dell-uart-backlight-emulator
> 
> Is it possible to fix this to (at least) honour `make O=...` cases?
> (See, e.g., tools/gpio.)

I'll take a look at what the tools/gpio Makefile is doing.

> 
> ...
> 
>> +/* read() will return -1 on SIGINT / SIGTERM causing the mainloop to cleanly exit */
> 
> Interesting...  usually we handle error codes, such as EAGAIN and
> EINTR from read() syscall separately.

EAGAIN cannot happen since the fd is kept in its default blocking
mode. Other errors are also not expected to happen and would likely
lead to aborting the program anyway.

So just having an empty signal handler and then exit on the
EINTR error from read() is a nice KISS way to exit the main loop.

> 
>> +void signalhdlr(int signum)
>> +{
>> +}
> 
> ...
> 
>> +               fprintf(stderr, "Error opening %s: %s\n", argv[1], strerror(errno));
> 
>> +               fprintf(stderr, "Error getting tcattr: %s\n", strerror(errno));
> 
> (and so on)
> 
> Wouldn't perror() call be better?

perror() takes a fixed string, so for your first example it won't work since that
requires printf style formatted string support and once I made the choice there
to use fprintf(stderr, ) I used it everywhere for consistency.

> 
> ...
> 
>> +               switch ((buf[0] << 8) | buf[1]) {
> 
> byteorder.h is part of UAPI, you can use it, but OTOH it might be too
> complicated for the small thing like this.
> 
>> +               }
> 
> ...
> 
>> +       return ret;
> 
> Hmm... Hopefully you checked the possible returned codes, in user
> space it's only a positive 8-bit value used.

That is a good point, actually in normal use ret will be (len + 3)
from the last write() call done in the loop. So you're right that
needs some work.

Regards,

Hans