Attention is currently required from: Xiang Wang, Stefan Reinauer.

View Change

7 comments:

• Commit Message:

  • Patch Set #1, Line 10: some SBC

    Which one have you tested this on?

• File sysfsgpio.c:

  • Patch Set #1, Line 36: #define GPIO_DIRECTION "/sys/class/gpio/gpio%d/direction"

    The only definition that gets used twice is `GPIO_PATH`. The others are only used once.

  • Patch Set #1, Line 38: #define EXIST_PATH(path) (access((path), F_OK) == 0

    cute but just inline it, the indirection is not necessary.

    It's only used twice, so I don't think this provides any benefit. If anything, you could make it a static function and let the compiler decide whether to inline it or not.

  • Patch Set #1, Line 42: int pin;

    Looks like the code only uses the string representation of `pin`. To reduce `snprintf` usage, how about having a `char pin_str[8];` here?

  • Patch Set #1, Line 47:

    static struct pin_desc pin_cs = {
    	.name = "cs",
    	.fd_direction = -1,
    	.fd_value = -1
    };
    
    static struct pin_desc pin_sck = {
    	.name = "sck",
    	.fd_direction = -1,
    	.fd_value = -1
    };
    
    static struct pin_desc pin_mosi = {
    	.name = "mosi",
    	.fd_direction = -1,
    	.fd_value = -1
    };
    
    static struct pin_desc pin_miso = {
    	.name = "miso",
    	.fd_direction = -1,
    	.fd_value = -1
    };
    

    I would suggest making another struct to hold all pins in a single global variable:

     struct sysfsgpio_pins {
     	struct pin_desc cs;
     	struct pin_desc sck;
     	struct pin_desc mosi;
     	struct pin_desc miso;
     };
     
     static struct sysfsgpio_pins pins = {
     	.cs = {
     		.name = "cs",
     		.fd_direction = -1,
     		.fd_value = -1,
     	},
     	.sck = {
     		.name = "sck",
     		.fd_direction = -1,
     		.fd_value = -1,
     	},
     	.mosi = {
     		.name = "mosi",
     		.fd_direction = -1,
     		.fd_value = -1,
     	},
     	.miso = {
     		.name = "miso",
     		.fd_direction = -1,
     		.fd_value = -1,
     	},
     };

  • Patch Set #1, Line 80: if (ret == (int)strlen(str))

    To avoid casting to int, I would test for failure here with `ret < strlen(str)`.

  • Patch Set #1, Line 92: snprintf(s, sizeof(s), "%d", desc->pin);

    If you can avoid clobbering the buffer contents with this snprintf call (see suggestion to use a `pin_str` cache in global state), you should be able to reuse the buffer contents for the direction and value files.

To view, visit change 49254. To unsubscribe, or for help writing mail filters, visit settings.