[PATCH 1/2] [GPIOLib] GPIO Interrupt counter support v2

[Greg KH]

The "v2" should be in the [patch 1/2 v2] area, not in the main part of
the subject: as it doesn't make much sense to add that as a changelog
entry, right?

Then put what you changed in v2 below the --- line, so that people know
you listened to their review comments.

On Wed, May 28, 2014 at 12:17:48PM +0200, Federico Fuga wrote:
> This patch add a per-pin interrupt counter to GPIOs.
> This way, GPIOs Interrupt will increment an internal counter that
> can be accessed through sysfs. Counter can be read and written
> from user space.
> 
> Signed-off-by: Federico Fuga <fuga at studiofuga.com>
> ---
>  drivers/gpio/Kconfig       |  9 +++++
>  drivers/gpio/gpiolib.c     | 92 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/asm-generic/gpio.h | 25 +++++++++++++
>  3 files changed, 126 insertions(+)
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index b2450ba..cbdc62e 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -81,6 +81,15 @@ config GPIO_SYSFS
>  	  Kernel drivers may also request that a particular GPIO be
>  	  exported to userspace; this can be useful when debugging.
>  
> +config GPIO_COUNTER
> +	bool "GPIO Interrupt counter"
> +	help
> +	  Say Y here to add a per-pin interrupt counter to GPIOs.

To all gpios?

> +	  This way, GPIOs Interrupt will increment an internal counter that
> +	  can be accessed through sysfs. Counter can be read and written
> +	  from user space.

If you are adding new sysfs files, you have to have a Documentation/ABI/
update as well.

> +#ifdef CONFIG_GPIO_COUNTER
> +	unsigned long    counter;    /* IRQ Events counter */
> +#endif

Why #ifdef this?  Is it so big that it can't just always be in the
structure?  In fact, why not always keep track of this and only export
it if needed?  Or why not always just export it, no need for a config
option at all, who would want to turn it off?

>  };
>  static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
>  
> @@ -583,9 +587,57 @@ static ssize_t gpio_active_low_store(struct device *dev,
>  static const DEVICE_ATTR(active_low, 0644,
>  		gpio_active_low_show, gpio_active_low_store);
>  
> +#ifdef CONFIG_GPIO_COUNTER
> +static ssize_t counter_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	struct gpio_desc	*desc = dev_get_drvdata(dev);
> +	ssize_t			status;
> +
> +	mutex_lock(&sysfs_lock);
> +
> +	if (!test_bit(FLAG_EXPORT, &desc->flags)) {
> +		status = -EIO;

Why is this an error?

> +	} else {
> +		long		value;
> +
> +		status = kstrtol(buf, 10, &value);
> +		if (status == 0)
> +			desc->counter = value;
> +	}
> +
> +	mutex_unlock(&sysfs_lock);
> +
> +	return status ? : size;

Be explicit and write out your logic and don't use ? : style in the
kernel.

> +
> +}

Unneeded empty line :(


thanks,

greg k-h