[PATCH v4] ARM/mx35/3ds: gpio: add mc9s08dz60 gpio function

On Thu, Nov 10, 2011 at 10:24:10AM +0800, y at shlinux1.ap.freescale.net wrote:

This list is sorted alphabetically.  Please keep it that way.

static inline

+{
+	return container_of(gc, struct mc9s08dz60, chip);
+}
+
+
+static void gpio_to_reg_and_bit(int offset, u8 *reg, u8 *bit)

Protect against namespace collisions and put a mc9s... prefix on this
function.

+{
+	*reg = 0x20 + offset / GPIO_NUM_PER_GROUP;
+	*bit = offset % GPIO_NUM_PER_GROUP;
+}
+
+static int mc9s08dz60_get_value(struct gpio_chip *gc, unsigned offset)
+{
+	int ret = 0;
+	u8 reg, bit;
+	s32 value;
+	struct mc9s08dz60 *mc9s = to_mc9s08dz60(gc);
+
+	gpio_to_reg_and_bit(offset, &reg, &bit);
+	value = i2c_smbus_read_byte_data(mc9s->client, reg);
+	if (value >= 0)
+		ret = (value >> bit) & 0x1;
+
+	return ret;

	return (value >= 0) ? (value >> bit) & 1 : 0;

+}
+
+static int _mc9s08dz60_set(struct mc9s08dz60 *mc9s, unsigned offset, int val)

Single leader underscore '_' is reserved.  Use either two underscores
'__' or none at all.

+{
+	u8 reg, bit;
+	s32 value;
+
+	gpio_to_reg_and_bit(offset, &reg, &bit);
+	value = i2c_smbus_read_byte_data(mc9s->client, reg);
+	if (value >= 0) {
+		if (val)
+			value |= 1 << bit;
+		else
+			value &= ~(1 << bit);
+
+		return i2c_smbus_write_byte_data(mc9s->client, reg, value);
+	} else
+		return value;
+
+}
+
+
+static void mc9s08dz60_set_value(struct gpio_chip *gc, unsigned offset, int val)
+{
+	struct mc9s08dz60 *mc9s = to_mc9s08dz60(gc);
+
+	_mc9s08dz60_set(mc9s, offset, val);
+}
+
+static int mc9s08dz60_direction_output(struct gpio_chip *gc,
+				       unsigned offset, int val)
+{
+	struct mc9s08dz60 *mc9s = to_mc9s08dz60(gc);
+
+	return _mc9s08dz60_set(mc9s, offset, val);
+}
+
+static int mc9s08dz60_probe(struct i2c_client *client,
+			    const struct i2c_device_id *id)
+{
+	int ret = 0;
+	struct mc9s08dz60 *mc9s;
+
+	mc9s = kzalloc(sizeof(*mc9s), GFP_KERNEL);
+	if (!mc9s)
+		return -ENOMEM;
+
+	mc9s->chip.label = client->name;
+	mc9s->chip.base = -1;
+	mc9s->chip.dev = &client->dev;
+	mc9s->chip.owner = THIS_MODULE;
+	mc9s->chip.ngpio = GPIO_NUM;
+	mc9s->chip.can_sleep = 1;
+	mc9s->chip.get = mc9s08dz60_get_value;
+	mc9s->chip.set = mc9s08dz60_set_value;
+	mc9s->chip.direction_output = mc9s08dz60_direction_output;
+	mc9s->client = client;
+	i2c_set_clientdata(client, mc9s);
+
+	ret = gpiochip_add(&mc9s->chip);
+	if (ret)
+		goto error;
+
+	return 0;
+
+error:

Tip: insert one space before a label so that diff will show the
function name on context output instead of the label name.

+	i2c_set_clientdata(client, NULL);
+	kfree(mc9s);
+	return ret;
+}
+
+static int mc9s08dz60_remove(struct i2c_client *client)
+{
+	struct mc9s08dz60 *mc9s;
+	int ret;
+
+	mc9s = i2c_get_clientdata(client);
+
+	i2c_set_clientdata(client, NULL);
+
+	ret = gpiochip_remove(&mc9s->chip);
+	if (!ret)
+		kfree(mc9s);
+
+	return 0;
+
+}
+
+static const struct i2c_device_id mc9s08dz60_id[] = {
+	{"mc9s08dz60", 0},
+	{},
+};
+
+MODULE_DEVICE_TABLE(i2c, mc9s08dz60_id);
+
+static struct i2c_driver mc9s08dz60_i2c_driver = {
+	.driver = {.owner = THIS_MODULE,
+		   .name = "mc9s08dz60",
+		   },

Please use this indentation for static initializers:
	.driver = {
		.owner = THIS_MODULE,
		.name = "mc9s08dz60",
	},

+	.probe = mc9s08dz60_probe,
+	.remove = mc9s08dz60_remove,
+	.id_table = mc9s08dz60_id,
+};
+
+static int __init mc9s08dz60_init(void)
+{
+	return i2c_add_driver(&mc9s08dz60_i2c_driver);
+}
+
+static void __exit mc9s08dz60_exit(void)
+{
+	i2c_del_driver(&mc9s08dz60_i2c_driver);
+}
+
+module_init(mc9s08dz60_init);
+module_exit(mc9s08dz60_exit);

module_i2c_driver() is preferred now.

+
+MODULE_AUTHOR("Freescale Semiconductor, Inc."
+		"Wu Guoxing [off-list ref]");

There is no space between "...Inc." and "Wu...", which is probably not
what you want.  :-)

Otherwise looks good.  One respin and it should be ready for merging.

g.