Re: [PATCH v4 3/3] selftest/gpio: add gpio test case

[Shuah Khan <shuahkh@xxxxxxxxxxxxxxx>]

On 10/21/2016 05:54 AM, Linus Walleij wrote:
> On Fri, Oct 14, 2016 at 4:48 AM, Bamvor Jian Zhang
> <bamvor.zhangjian@xxxxxxxxxx> wrote:
> 
>> From: Bamvor Jian Zhang <bamvor.zhangjian@xxxxxxxxxx>
>>
>> This test script try to do whitebox testing for gpio subsystem(
>> based on gpiolib). It manipulate gpio device through chardev or
>> sysfs and check the result from debugfs. This script test
>> gpio-mockup through chardev by default.
>>
>> In details, it test the following things:
>> 1.  Add single, multi gpiochip to do overlap check.
>> 2.  Test direction and output value for valid pin.
>> 3.  Test dynamic allocation of gpio base.
>>
>> Run "tools/testing/selftests/gpio/gpio-mockup.sh -h" to know how to
>> use it.
>>
>> Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@xxxxxxxxxx>
> 
> I like the overall idea with this, but some comments:
> 
>> +CFLAGS += -O2 -g -std=gnu99 -Wall -I../../../../usr/include/
> 
> Is this really what people are doing?
> 
> Isn't -I/usr/include the right way to express this?
> 
>> +LDLIBS += -lmount -I/usr/include/libmount
>> +
>> +$(BINARIES): ../../../gpio/gpio-utils.o ../../../../usr/include/linux/gpio.h
>> +
>> +../../../gpio/gpio-utils.o:
>> +       make ARCH=$(ARCH) CROSS_COMPILE=$(CROSS_COMPILE) -C ../../../gpio
> 
> Ah, that is clever. I hope I can get an ACK from Shuah if this is
> how we're supposed to handle cross dependencies of common
> helper code in the kernel.
> 
> It looks a bit spaghetti, unfortunately.

Yeah it does. Let's get this in for now and we can try to improve
it.

Sorry for the delay, here is the ack


> 
>> +../../../../usr/include/linux/gpio.h:
>> +       make -C ../../../.. headers_install
> 
> Don't do this please, you would have to be root and it's very
> fragile. How does tests in general resolve dependencies on
> kernel headers? Please look around a bit and check what
> they do. I think they just expect them to be installed.

Yeah - it is a good idea to have the user install the headers.
It shouldn't be done in the script without use consent.

> 
> I like the tests overall, but I'm a bit suspicious about the
> sysfs tests. Maybe these should atleast print something
> about the sysfs ABI being deprecated.

Has this concern been addressed. Once the above are fixed.

Acked-by: Shuah Khan <shuahkh@xxxxxxxxxxxxxxx>

> 
> Anyways I merged the first two patches so now we only have to
> figure out this final patch!

Acked-by: Shuah Khan <shuahkh@xxxxxxxxxxxxxxx>


> 
> Yours,
> Linus Walleij
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html