On 06/10/2011 04:50 AM, Somebody in the thread at some point said:
Hi -
Just some small comments...
It's worth running ./scripts/checkpatch.pl if you didn't already.
+#define DRIVER_NAME "da9052-gpio"
+static inline struct da9052_gpio_chip *to_da9052_gpio(struct gpio_chip *chip)
Don't need inline.
+ printk(KERN_INFO "Event received from GPIO8\n");
You can better use pr_info() or dev_info() if you have a struct device *
lying around.
+}
+
+static u8 create_gpio_config_value(u8 gpio_function, u8 gpio_type, u8
gpio_mode)
Consider separating return type and attributes on different line
static u8
create_gpio_config_value(...
lets you do a nice trick with grep ^functionname to find definition.
+{
+ /* The format is -
+ function - 2 bits
+ type - 1 bit
+ mode - 1 bit */
Big comment style needs to be
/*
* blah
* blah
*/
+ return gpio_function | (gpio_type<< 2) | (gpio_mode<< 3);
+}
+
+static s32 write_default_gpio_values(struct da9052 *da9052)
+{
+ struct da9052_ssc_msg msg;
+ u8 created_val = 0;
+
+#if (DA9052_GPIO_PIN_0 == DA9052_GPIO_CONFIG)
What's the story about this #if scheme? There's a lot of duplicated
code? Even if it's the right way need to explain why in the patch
commitlog due to anti-#if sentimenet out there.
+ da9052_lock(da9052);
+ msg.addr = DA9052_GPIO0001_REG;
+ msg.data = 0;
+
+ if (da9052->read(da9052,&msg)) {
+ da9052_unlock(da9052);
+ return -EIO;
+ }
It's less error-prone to handle exit from inside a locked region with
goto. Eg,
int ret = 0;
...
lock();
...
if (bad) {
ret = -EIO;
goto bail;
}
...
bail:
unlock();
return ret;
+s32 da9052_gpio_read_port(struct da9052_gpio_read_write *read_port,
+ struct da9052 *da9052)
+{
+ struct da9052_ssc_msg msg;
+ u8 shift_value = 0;
+ u8 port_functionality = 0;
It's normal to have a black line separate the local vars from the
function body, more than one function needs it.
+ msg.addr = (read_port->port_number / 2) + DA9052_GPIO0001_REG;
+ msg.data = 0;
+ da9052_lock(da9052);
+ if (da9052->read(da9052,&msg)) {
+ da9052_unlock(da9052);
+ return -EIO;
+ }
Again goto to a common unlock exit point is better / less code.
+ da9052_unlock(da9052);
+ port_functionality =
+ (read_port->port_number % 2) ?
+ ((msg.data& DA9052_GPIO_ODD_PORT_FUNCTIONALITY)>>
+ DA9052_GPIO_NIBBLE_SHIFT) :
+ (msg.data& DA9052_GPIO_EVEN_PORT_FUNCTIONALITY);
+
+ if (port_functionality != INPUT)
+ return DA9052_GPIO_INVALID_PORTNUMBER;
+
+ if (read_port->port_number>= (DA9052_GPIO_MAX_PORTNUMBER))
Don't need parenthesis around the constant... if it's a compound
expression put the parenthesis at the definition of the constant not at
the usage.
+ if (da9052->read_many(da9052, msg, loop_index)) {
Is this not better called "read_multiple"?
+ msg.data = msg.data | bit_pos;
You can use |= to simplify.
+ else
+ msg.data = (msg.data& ~(bit_pos));
Outer parenthesis can go away.
+ port_functionality =
+ (gpio_data->port_number % 2) ?
This can go on the same line as the =?
+ ((msg.data& DA9052_GPIO_ODD_PORT_FUNCTIONALITY)>>
+ DA9052_GPIO_NIBBLE_SHIFT) :
+ (msg.data& DA9052_GPIO_EVEN_PORT_FUNCTIONALITY);
+ if (port_functionality< INPUT)
+ return DA9052_GPIO_INVALID_PORTNUMBER;
+ if (gpio_data->gpio_config.input.type> ACTIVE_HIGH)
+ return DA9052_GPIO_INVALID_TYPE;
+ if (gpio_data->gpio_config.input.mode> DEBOUNCING_ON)
+ return DA9052_GPIO_INVALID_MODE;
+ function = gpio_data->gpio_function;
+ switch (function) {
+ case INPUT:
+ register_value = create_gpio_config_value(function,
+ gpio_data->gpio_config.input.type,
+ gpio_data->gpio_config.input.mode);
+ break;
+ case OUTPUT_OPENDRAIN:
+ case OUTPUT_PUSHPULL:
+ register_value = create_gpio_config_value(function,
+ gpio_data->gpio_config.input.type,
+ gpio_data->gpio_config.input.mode);
+ break;
breaks need to be tabbed in one more everywhere.
+static s32 da9052_gpio_read(struct gpio_chip *gc, u32 offset)
+{
+ struct da9052_gpio_chip *gpio;
+ gpio = to_da9052_gpio(gc);
+ gpio->read_write.port_number = offset;
Just a space, not tabs before =
-Andy
_______________________________________________
linaro-dev mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/linaro-dev
