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

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

[wu guoxing]

we only use the gpio function of mc9s08dz60 mcu chip, so just
add the gpio driver, as this mcu will never be used in other board.

Signed-off-by: Wu Guoxing <b39297@freescale.com>
---
Changes since v1:
	1. add can_sleep = 1
	2. removed some useless return checks and local variable
	3. removed the static variable for i2c client.
	4. make init&exit function static

 drivers/gpio/Kconfig           |    6 ++
 drivers/gpio/Makefile          |    1 +
 drivers/gpio/gpio-mc9s08dz60.c |  167 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 174 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpio/gpio-mc9s08dz60.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d539efd..7f6beee 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -487,4 +487,10 @@ config GPIO_TPS65910
 	help
 	  Select this option to enable GPIO driver for the TPS65910
 	  chip family.
+
+config GPIO_MC9S08DZ60
+	bool "MX35 3DS BOARD MC9S08DZ60 GPIO functions"
+	depends on I2C && MACH_MX35_3DS
+	help
+	  Select this to enable the MC9S08DZ60 GPIO driver
 endif
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 9588948..85b8799 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -60,3 +60,4 @@ obj-$(CONFIG_GPIO_WM831X)	+= gpio-wm831x.o
 obj-$(CONFIG_GPIO_WM8350)	+= gpio-wm8350.o
 obj-$(CONFIG_GPIO_WM8994)	+= gpio-wm8994.o
 obj-$(CONFIG_GPIO_XILINX)	+= gpio-xilinx.o
+obj-$(CONFIG_GPIO_MC9S08DZ60) += gpio-mc9s08dz60.o
diff --git a/drivers/gpio/gpio-mc9s08dz60.c b/drivers/gpio/gpio-mc9s08dz60.c
new file mode 100644
index 0000000..c41eb16
--- /dev/null
+++ b/drivers/gpio/gpio-mc9s08dz60.c
@@ -0,0 +1,167 @@
+/*
+ * Copyright 2009-2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ *
+ * Author: Wu Guoxing <b39297@freescale.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/gpio.h>
+
+#define GPIO_GROUP_NUM 2
+#define GPIO_NUM_PER_GROUP 8
+#define GPIO_NUM (GPIO_GROUP_NUM*GPIO_NUM_PER_GROUP)
+
+struct mc9s08dz60 {
+	struct i2c_client *client;
+	struct gpio_chip chip;
+};
+
+static struct mc9s08dz60 *to_mc9s08dz60(struct gpio_chip *gc)
+{
+	return container_of(gc, struct mc9s08dz60, chip);
+}
+
+
+static void gpio_to_reg_and_bit(int offset, u8 *reg, u8 *bit)
+{
+	*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;
+}
+
+static void mc9s08dz60_set_value(struct gpio_chip *gc, unsigned offset, int val)
+{
+	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) {
+		if (val)
+			value |= 1 << bit;
+		else
+			value &= ~(1 << bit);
+
+		i2c_smbus_write_byte_data(mc9s->client, reg, value);
+	}
+}
+
+static int mc9s08dz60_direction_output(struct gpio_chip *gc,
+				       unsigned offset, int val)
+{
+	mc9s08dz60_set_value(gc, offset, val);
+	return 0;
+}
+
+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;
+
+	ret = gpiochip_add(&mc9s->chip);
+	if (ret)
+		goto error;
+
+	i2c_set_clientdata(client, mc9s);
+
+	return 0;
+
+error:
+	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",
+		   },
+	.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_AUTHOR("Freescale Semiconductor, Inc."
+		"Wu Guoxing <b39297@freescale.com>");
+MODULE_DESCRIPTION("mc9s08dz60 gpio function on mx35 3ds board");
+MODULE_LICENSE("GPL v2");
-- 
1.7.1

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

[Sascha Hauer]

On Fri, Nov 04, 2011 at 06:12:31PM +0800, wu guoxing wrote:
> we only use the gpio function of mc9s08dz60 mcu chip, so just
> add the gpio driver, as this mcu will never be used in other board.
> 
> Signed-off-by: Wu Guoxing <b39297@freescale.com>

Looks good to me.

Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>

> ---
> Changes since v1:
> 	1. add can_sleep = 1
> 	2. removed some useless return checks and local variable
> 	3. removed the static variable for i2c client.
> 	4. make init&exit function static
> 
>  drivers/gpio/Kconfig           |    6 ++
>  drivers/gpio/Makefile          |    1 +
>  drivers/gpio/gpio-mc9s08dz60.c |  167 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 174 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/gpio/gpio-mc9s08dz60.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index d539efd..7f6beee 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -487,4 +487,10 @@ config GPIO_TPS65910
>  	help
>  	  Select this option to enable GPIO driver for the TPS65910
>  	  chip family.
> +
> +config GPIO_MC9S08DZ60
> +	bool "MX35 3DS BOARD MC9S08DZ60 GPIO functions"
> +	depends on I2C && MACH_MX35_3DS
> +	help
> +	  Select this to enable the MC9S08DZ60 GPIO driver
>  endif
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 9588948..85b8799 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -60,3 +60,4 @@ obj-$(CONFIG_GPIO_WM831X)	+= gpio-wm831x.o
>  obj-$(CONFIG_GPIO_WM8350)	+= gpio-wm8350.o
>  obj-$(CONFIG_GPIO_WM8994)	+= gpio-wm8994.o
>  obj-$(CONFIG_GPIO_XILINX)	+= gpio-xilinx.o
> +obj-$(CONFIG_GPIO_MC9S08DZ60) += gpio-mc9s08dz60.o
> diff --git a/drivers/gpio/gpio-mc9s08dz60.c b/drivers/gpio/gpio-mc9s08dz60.c
> new file mode 100644
> index 0000000..c41eb16
> --- /dev/null
> +++ b/drivers/gpio/gpio-mc9s08dz60.c
> @@ -0,0 +1,167 @@
> +/*
> + * Copyright 2009-2011 Freescale Semiconductor, Inc. All Rights Reserved.
> + *
> + * Author: Wu Guoxing <b39297@freescale.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/gpio.h>
> +
> +#define GPIO_GROUP_NUM 2
> +#define GPIO_NUM_PER_GROUP 8
> +#define GPIO_NUM (GPIO_GROUP_NUM*GPIO_NUM_PER_GROUP)
> +
> +struct mc9s08dz60 {
> +	struct i2c_client *client;
> +	struct gpio_chip chip;
> +};
> +
> +static struct mc9s08dz60 *to_mc9s08dz60(struct gpio_chip *gc)
> +{
> +	return container_of(gc, struct mc9s08dz60, chip);
> +}
> +
> +
> +static void gpio_to_reg_and_bit(int offset, u8 *reg, u8 *bit)
> +{
> +	*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;
> +}
> +
> +static void mc9s08dz60_set_value(struct gpio_chip *gc, unsigned offset, int val)
> +{
> +	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) {
> +		if (val)
> +			value |= 1 << bit;
> +		else
> +			value &= ~(1 << bit);
> +
> +		i2c_smbus_write_byte_data(mc9s->client, reg, value);
> +	}
> +}
> +
> +static int mc9s08dz60_direction_output(struct gpio_chip *gc,
> +				       unsigned offset, int val)
> +{
> +	mc9s08dz60_set_value(gc, offset, val);
> +	return 0;
> +}
> +
> +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;
> +
> +	ret = gpiochip_add(&mc9s->chip);
> +	if (ret)
> +		goto error;
> +
> +	i2c_set_clientdata(client, mc9s);
> +
> +	return 0;
> +
> +error:
> +	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",
> +		   },
> +	.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_AUTHOR("Freescale Semiconductor, Inc."
> +		"Wu Guoxing <b39297@freescale.com>");
> +MODULE_DESCRIPTION("mc9s08dz60 gpio function on mx35 3ds board");
> +MODULE_LICENSE("GPL v2");
> -- 
> 1.7.1
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Marc Kleine-Budde]

On 11/04/2011 11:12 AM, wu guoxing wrote:
> we only use the gpio function of mc9s08dz60 mcu chip, so just
> add the gpio driver, as this mcu will never be used in other board.

see comments inline

Marc
> 
> Signed-off-by: Wu Guoxing <b39297@freescale.com>
> ---
> Changes since v1:
> 	1. add can_sleep = 1
> 	2. removed some useless return checks and local variable
> 	3. removed the static variable for i2c client.
> 	4. make init&exit function static
> 
>  drivers/gpio/Kconfig           |    6 ++
>  drivers/gpio/Makefile          |    1 +
>  drivers/gpio/gpio-mc9s08dz60.c |  167 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 174 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/gpio/gpio-mc9s08dz60.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index d539efd..7f6beee 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -487,4 +487,10 @@ config GPIO_TPS65910
>  	help
>  	  Select this option to enable GPIO driver for the TPS65910
>  	  chip family.
> +
> +config GPIO_MC9S08DZ60
> +	bool "MX35 3DS BOARD MC9S08DZ60 GPIO functions"
> +	depends on I2C && MACH_MX35_3DS
> +	help
> +	  Select this to enable the MC9S08DZ60 GPIO driver
>  endif
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 9588948..85b8799 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -60,3 +60,4 @@ obj-$(CONFIG_GPIO_WM831X)	+= gpio-wm831x.o
>  obj-$(CONFIG_GPIO_WM8350)	+= gpio-wm8350.o
>  obj-$(CONFIG_GPIO_WM8994)	+= gpio-wm8994.o
>  obj-$(CONFIG_GPIO_XILINX)	+= gpio-xilinx.o
> +obj-$(CONFIG_GPIO_MC9S08DZ60) += gpio-mc9s08dz60.o
> diff --git a/drivers/gpio/gpio-mc9s08dz60.c b/drivers/gpio/gpio-mc9s08dz60.c
> new file mode 100644
> index 0000000..c41eb16
> --- /dev/null
> +++ b/drivers/gpio/gpio-mc9s08dz60.c
> @@ -0,0 +1,167 @@
> +/*
> + * Copyright 2009-2011 Freescale Semiconductor, Inc. All Rightos Reserved.
> + *
> + * Author: Wu Guoxing <b39297@freescale.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/gpio.h>
> +
> +#define GPIO_GROUP_NUM 2
> +#define GPIO_NUM_PER_GROUP 8
> +#define GPIO_NUM (GPIO_GROUP_NUM*GPIO_NUM_PER_GROUP)
> +
> +struct mc9s08dz60 {
> +	struct i2c_client *client;
> +	struct gpio_chip chip;
> +};
> +
> +static struct mc9s08dz60 *to_mc9s08dz60(struct gpio_chip *gc)
> +{
> +	return container_of(gc, struct mc9s08dz60, chip);
> +}
> +
> +
> +static void gpio_to_reg_and_bit(int offset, u8 *reg, u8 *bit)
> +{
> +	*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;
> +}
> +
> +static void mc9s08dz60_set_value(struct gpio_chip *gc, unsigned offset, int val)
> +{
> +	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) {
> +		if (val)
> +			value |= 1 << bit;
> +		else
> +			value &= ~(1 << bit);
> +
> +		i2c_smbus_write_byte_data(mc9s->client, reg, value);
> +	}

what about propagating potential error values here?

> +}
> +
> +static int mc9s08dz60_direction_output(struct gpio_chip *gc,
> +				       unsigned offset, int val)
> +{
> +	mc9s08dz60_set_value(gc, offset, val);
> +	return 0;

dito

> +}
> +
> +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;
> +
> +	ret = gpiochip_add(&mc9s->chip);
> +	if (ret)
> +		goto error;
> +
> +	i2c_set_clientdata(client, mc9s);

Is it racy if you first add the gpio chip and then set the i2c client data?

> +
> +	return 0;
> +
> +error:
> +	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;

return 0 if gpiochip_remove() fails?

> +
> +}
> +
> +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",
> +		   },
> +	.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_AUTHOR("Freescale Semiconductor, Inc."
> +		"Wu Guoxing <b39297@freescale.com>");
> +MODULE_DESCRIPTION("mc9s08dz60 gpio function on mx35 3ds board");
> +MODULE_LICENSE("GPL v2");


-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111109/c6439cd2/attachment.sig>

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

[Fabio Estevam]

On Wed, Nov 9, 2011 at 1:46 PM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>> +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;
>
> return 0 if gpiochip_remove() fails?

See Russell's comment about returning error on this remove function:
http://www.spinics.net/lists/arm-kernel/msg146501.html

Regards,

Fabio Estevam

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

[Sascha Hauer]

On Wed, Nov 09, 2011 at 04:46:00PM +0100, Marc Kleine-Budde wrote:
> On 11/04/2011 11:12 AM, wu guoxing wrote:
> > we only use the gpio function of mc9s08dz60 mcu chip, so just
> > add the gpio driver, as this mcu will never be used in other board.
> 
> see comments inline
> 
> Marc
> > 
> > Signed-off-by: Wu Guoxing <b39297@freescale.com>
> > ---
> > Changes since v1:
> > 	1. add can_sleep = 1
> > 	2. removed some useless return checks and local variable
> > 	3. removed the static variable for i2c client.
> > 	4. make init&exit function static
> > 
> >  drivers/gpio/Kconfig           |    6 ++
> >  drivers/gpio/Makefile          |    1 +
> >  drivers/gpio/gpio-mc9s08dz60.c |  167 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 174 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/gpio/gpio-mc9s08dz60.c
> > 
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index d539efd..7f6beee 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -487,4 +487,10 @@ config GPIO_TPS65910
> >  	help
> >  	  Select this option to enable GPIO driver for the TPS65910
> >  	  chip family.
> > +
> > +config GPIO_MC9S08DZ60
> > +	bool "MX35 3DS BOARD MC9S08DZ60 GPIO functions"
> > +	depends on I2C && MACH_MX35_3DS
> > +	help
> > +	  Select this to enable the MC9S08DZ60 GPIO driver
> >  endif
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index 9588948..85b8799 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -60,3 +60,4 @@ obj-$(CONFIG_GPIO_WM831X)	+= gpio-wm831x.o
> >  obj-$(CONFIG_GPIO_WM8350)	+= gpio-wm8350.o
> >  obj-$(CONFIG_GPIO_WM8994)	+= gpio-wm8994.o
> >  obj-$(CONFIG_GPIO_XILINX)	+= gpio-xilinx.o
> > +obj-$(CONFIG_GPIO_MC9S08DZ60) += gpio-mc9s08dz60.o
> > diff --git a/drivers/gpio/gpio-mc9s08dz60.c b/drivers/gpio/gpio-mc9s08dz60.c
> > new file mode 100644
> > index 0000000..c41eb16
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-mc9s08dz60.c
> > @@ -0,0 +1,167 @@
> > +/*
> > + * Copyright 2009-2011 Freescale Semiconductor, Inc. All Rightos Reserved.
> > + *
> > + * Author: Wu Guoxing <b39297@freescale.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/i2c.h>
> > +#include <linux/gpio.h>
> > +
> > +#define GPIO_GROUP_NUM 2
> > +#define GPIO_NUM_PER_GROUP 8
> > +#define GPIO_NUM (GPIO_GROUP_NUM*GPIO_NUM_PER_GROUP)
> > +
> > +struct mc9s08dz60 {
> > +	struct i2c_client *client;
> > +	struct gpio_chip chip;
> > +};
> > +
> > +static struct mc9s08dz60 *to_mc9s08dz60(struct gpio_chip *gc)
> > +{
> > +	return container_of(gc, struct mc9s08dz60, chip);
> > +}
> > +
> > +
> > +static void gpio_to_reg_and_bit(int offset, u8 *reg, u8 *bit)
> > +{
> > +	*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;
> > +}
> > +
> > +static void mc9s08dz60_set_value(struct gpio_chip *gc, unsigned offset, int val)
> > +{
> > +	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) {
> > +		if (val)
> > +			value |= 1 << bit;
> > +		else
> > +			value &= ~(1 << bit);
> > +
> > +		i2c_smbus_write_byte_data(mc9s->client, reg, value);
> > +	}
> 
> what about propagating potential error values here?

This is a void function.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Marc Kleine-Budde]

On 11/09/2011 05:51 PM, Fabio Estevam wrote:
> On Wed, Nov 9, 2011 at 1:46 PM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>> +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;
>>
>> return 0 if gpiochip_remove() fails?
> 
> See Russell's comment about returning error on this remove function:
> http://www.spinics.net/lists/arm-kernel/msg146501.html

tnx for the hint - I must have missed that comment.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111109/76e79ff8/attachment.sig>

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

[Wu Guoxing-B39297]

Hi Marc:
  
  I have changed the position of i2c_set_clientdata and add a return value for mc9s08dz60_direction_output in the v4 patch.
Also, please review the latest v4 patch that sent, the other two are sent by mistake.

  Thanks for the comments!

Best Regards
Wu Guoxing 

-----Original Message-----
From: Marc Kleine-Budde [mailto:mkl at pengutronix.de] 
Sent: Wednesday, November 09, 2011 11:46 PM
To: Wu Guoxing-B39297
Cc: linux-arm-kernel at lists.infradead.org; grant.likely at secretlab.ca; s.hauer at pengutronix.de; shawn.guo at linaro.org
Subject: Re: [PATCH v3] ARM/mx35/3ds: gpio: add mc9s08dz60 gpio function

On 11/04/2011 11:12 AM, wu guoxing wrote:
> we only use the gpio function of mc9s08dz60 mcu chip, so just add the 
> gpio driver, as this mcu will never be used in other board.

see comments inline

Marc
> 
> Signed-off-by: Wu Guoxing <b39297@freescale.com>
> ---
> Changes since v1:
> 	1. add can_sleep = 1
> 	2. removed some useless return checks and local variable
> 	3. removed the static variable for i2c client.
> 	4. make init&exit function static
> 
>  drivers/gpio/Kconfig           |    6 ++
>  drivers/gpio/Makefile          |    1 +
>  drivers/gpio/gpio-mc9s08dz60.c |  167 
> ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 174 insertions(+), 0 deletions(-)  create mode 
> 100644 drivers/gpio/gpio-mc9s08dz60.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 
> d539efd..7f6beee 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -487,4 +487,10 @@ config GPIO_TPS65910
>  	help
>  	  Select this option to enable GPIO driver for the TPS65910
>  	  chip family.
> +
> +config GPIO_MC9S08DZ60
> +	bool "MX35 3DS BOARD MC9S08DZ60 GPIO functions"
> +	depends on I2C && MACH_MX35_3DS
> +	help
> +	  Select this to enable the MC9S08DZ60 GPIO driver
>  endif
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 
> 9588948..85b8799 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -60,3 +60,4 @@ obj-$(CONFIG_GPIO_WM831X)	+= gpio-wm831x.o
>  obj-$(CONFIG_GPIO_WM8350)	+= gpio-wm8350.o
>  obj-$(CONFIG_GPIO_WM8994)	+= gpio-wm8994.o
>  obj-$(CONFIG_GPIO_XILINX)	+= gpio-xilinx.o
> +obj-$(CONFIG_GPIO_MC9S08DZ60) += gpio-mc9s08dz60.o
> diff --git a/drivers/gpio/gpio-mc9s08dz60.c 
> b/drivers/gpio/gpio-mc9s08dz60.c new file mode 100644 index 
> 0000000..c41eb16
> --- /dev/null
> +++ b/drivers/gpio/gpio-mc9s08dz60.c
> @@ -0,0 +1,167 @@
> +/*
> + * Copyright 2009-2011 Freescale Semiconductor, Inc. All Rightos Reserved.
> + *
> + * Author: Wu Guoxing <b39297@freescale.com>
> + *
> + * This program is free software; you can redistribute it and/or 
> +modify
> + * it under the terms of the GNU General Public License as published 
> +by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/gpio.h>
> +
> +#define GPIO_GROUP_NUM 2
> +#define GPIO_NUM_PER_GROUP 8
> +#define GPIO_NUM (GPIO_GROUP_NUM*GPIO_NUM_PER_GROUP)
> +
> +struct mc9s08dz60 {
> +	struct i2c_client *client;
> +	struct gpio_chip chip;
> +};
> +
> +static struct mc9s08dz60 *to_mc9s08dz60(struct gpio_chip *gc) {
> +	return container_of(gc, struct mc9s08dz60, chip); }
> +
> +
> +static void gpio_to_reg_and_bit(int offset, u8 *reg, u8 *bit) {
> +	*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;
> +}
> +
> +static void mc9s08dz60_set_value(struct gpio_chip *gc, unsigned 
> +offset, int val) {
> +	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) {
> +		if (val)
> +			value |= 1 << bit;
> +		else
> +			value &= ~(1 << bit);
> +
> +		i2c_smbus_write_byte_data(mc9s->client, reg, value);
> +	}

what about propagating potential error values here?

> +}
> +
> +static int mc9s08dz60_direction_output(struct gpio_chip *gc,
> +				       unsigned offset, int val)
> +{
> +	mc9s08dz60_set_value(gc, offset, val);
> +	return 0;

dito

> +}
> +
> +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;
> +
> +	ret = gpiochip_add(&mc9s->chip);
> +	if (ret)
> +		goto error;
> +
> +	i2c_set_clientdata(client, mc9s);

Is it racy if you first add the gpio chip and then set the i2c client data?

> +
> +	return 0;
> +
> +error:
> +	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;

return 0 if gpiochip_remove() fails?

> +
> +}
> +
> +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",
> +		   },
> +	.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_AUTHOR("Freescale Semiconductor, Inc."
> +		"Wu Guoxing <b39297@freescale.com>"); 
> +MODULE_DESCRIPTION("mc9s08dz60 gpio function on mx35 3ds board"); 
> +MODULE_LICENSE("GPL v2");


-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |