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 bitstype - 1 bitmode - 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