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

[Federico Fuga]

Ok, I'll fix the tagline.


Il giorno 28/mag/2014, alle ore 16:34, Greg KH <greg at kroah.com> ha scritto:

>> +#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?
> 


I don't know, just leaving the freedom to choose. what do you suggest?


>> 
>> +#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?
> 



i copied a similar function in the driver, indeed if gpio isn't exported, the function should not  be accessible. But the same for the function I copied.
Anyway, it seems pleonastic, so I'll remove it.



>> +	} 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.


Same as above, I'm going to fix it.



> 
>> +
>> +}
> 
> Unneeded empty line :(
> 


damn :-) 
Thanks!


Federico