Re: [PATCH] GPIO: Add GPIO support for the ACCES 104-IDIO-16

From: William Breathitt Gray
Date: Mon Oct 05 2015 - 19:40:57 EST


On 10/05/2015 04:29 AM, Linus Walleij wrote:
>> +struct a_104_idio_16_gpio {
>> + struct gpio_chip chip;
>> + spinlock_t lock;
>> + unsigned base;
>
> Isn't this void __iomem *base?

The 'base' member is used to hold the I/O port base address passed to the
inb/outb functions for port-mapped I/O operations. Since the addresses are
not dereferenced, I don't believe an __iomem pointer would be correct.

>> +static const unsigned A_104_IDIO_16_EXTENT = 8;
>
> Looks like it could be a #define A_104_IDIO_16_EXTENT 8

I used a const variable for the benefit of type-safety; I assumed the
compiler would optimize it. What is the advantage of a #define constant?

>> +static void __exit a_104_idio_16_exit(void)
>> +{
>> + pr_info("104-idio-16: Exiting 104-idio-16 module\n");
>> +
>> + gpiochip_remove(&gp.chip);
>
> Where is that &gp.chip? Not in this file. Nor should you use any globals.
>

I agree that using a global data structure isn't good practice, but I'm not
sure how else to expose the gpio_chip structure in the respective module
_init and _exit functions since they have void parameter lists. Would it be
more appropriate to use the platform device API in this situation to avoid
the global data structure?

>> +static int a_104_idio_16_gpio_get(struct gpio_chip *chip, unsigned offset)
>> +{
>> + struct a_104_idio_16_gpio *a104i16gp = to_a104i16gp(chip);
>> + const unsigned BIT_MASK = 1U << (offset-16);
>> +
>> + if (offset < 16)
>> + return 0;
>
> Always return 0, why? Is that really correct?

GPIO 0-15 are output-only. The kerneldoc for 'struct gpio_chip' states that
for output signals the get function should return the value actually sensed,
or zero. Since I cannot sense the output signals, I return zero in these cases.
Is this behavior correct?

- William

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