Date:	Sun, 4 Dec 2011 00:24:48 -0800
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	oskar.andero@...yericsson.com
Cc:	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
	jic23@....ac.uk, aghayal@...eaurora.org,
	Courtney Cavin <courtney.cavin@...yericsson.com>
Subject: Re: [PATCH v5] input: add driver support for Sharp gp2ap002a00f
 proximity sensor

Hi Oskar,

On Wed, Nov 23, 2011 at 02:40:35PM +0100, oskar.andero@...yericsson.com wrote:
> From: Courtney Cavin <courtney.cavin@...yericsson.com>
> 
> This driver adds support for Sharp's GP2AP002A00F proximity sensor. The
> proximity is measured as a binary switch, i.e. an object is either
> detected or not detected. Hence, this driver is implemented as a switch
> that reports SW_FRONT_PROXIMITY.
> 

I was looking at the driver before applying it to my tree and I realized
that there are a few issues I'd like to have fixed:

- we may exit suspend/resume methods with mutex held if enable/disable
  fails;
- we need to do gpio_regiest/gpio_free on the vout_gpio;
- the driver should depend on GENERIC_GPIO;
- we need to use the same "dev" when reporting errors/warnings and not
  mix up i2c_client->dev and input->dev;
- we need to set up client->dev as parent of our input device.

Does the patch below applied on top of your patch works for you?

Thanks.

-- 
Dmitry

Input: gp2ap002a00f - misc changes

From: Dmitry Torokhov <dmitry.torokhov@...il.com>

Signed-off-by: Dmitry Torokhov <dtor@...l.ru>
---

 drivers/input/misc/Kconfig        |   21 ++--
 drivers/input/misc/Makefile       |    2 
 drivers/input/misc/gp2ap002a00f.c |  174 +++++++++++++++++++------------------
 3 files changed, 101 insertions(+), 96 deletions(-)


diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index a62fcdc..7b46781 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -179,6 +179,17 @@ config INPUT_APANEL
 	 To compile this driver as a module, choose M here: the module will
 	 be called apanel.
 
+config INPUT_GP2A
+	tristate "Sharp GP2AP002A00F I2C Proximity/Opto sensor driver"
+	depends on I2C
+	depends on GENERIC_GPIO
+	help
+	  Say Y here if you have a Sharp GP2AP002A00F proximity/als combo-chip
+	  hooked to an I2C bus.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called gp2ap002a00f.
+
 config INPUT_GPIO_TILT_POLLED
 	tristate "Polled GPIO tilt switch"
 	depends on GENERIC_GPIO
@@ -558,14 +569,4 @@ config INPUT_XEN_KBDDEV_FRONTEND
 	  To compile this driver as a module, choose M here: the
 	  module will be called xen-kbdfront.
 
-config INPUT_GP2A
-	tristate "Sharp GP2AP002A00F I2C Proximity/Opto sensor driver"
-	depends on I2C
-	help
-	  Say Y here if you have a Sharp GP2AP002A00F proximity/als combo-chip
-	  hooked to an I2C bus.
-
-	  To compile this driver as a module, choose M here: the
-	  module will be called gp2ap002a00f.
-
 endif
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index dd99a75..46671a8 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -23,8 +23,8 @@ obj-$(CONFIG_INPUT_CMA3000_I2C)		+= cma3000_d0x_i2c.o
 obj-$(CONFIG_INPUT_COBALT_BTNS)		+= cobalt_btns.o
 obj-$(CONFIG_INPUT_DM355EVM)		+= dm355evm_keys.o
 obj-$(CONFIG_INPUT_GP2A)		+= gp2ap002a00f.o
-obj-$(CONFIG_HP_SDC_RTC)		+= hp_sdc_rtc.o
 obj-$(CONFIG_INPUT_GPIO_TILT_POLLED)	+= gpio_tilt_polled.o
+obj-$(CONFIG_HP_SDC_RTC)		+= hp_sdc_rtc.o
 obj-$(CONFIG_INPUT_IXP4XX_BEEPER)	+= ixp4xx-beeper.o
 obj-$(CONFIG_INPUT_KEYSPAN_REMOTE)	+= keyspan_remote.o
 obj-$(CONFIG_INPUT_KXTJ9)		+= kxtj9.o
diff --git a/drivers/input/misc/gp2ap002a00f.c b/drivers/input/misc/gp2ap002a00f.c
index 5689a2b..71fba8c 100644
--- a/drivers/input/misc/gp2ap002a00f.c
+++ b/drivers/input/misc/gp2ap002a00f.c
@@ -20,7 +20,7 @@
 #include <linux/input/gp2ap002a00f.h>
 
 struct gp2a_data {
-	struct input_dev *device;
+	struct input_dev *input;
 	const struct gp2a_platform_data *pdata;
 	struct i2c_client *i2c_client;
 };
@@ -39,48 +39,12 @@ enum gp2a_controls {
 	GP2A_CTRL_SSD	= 0x01
 };
 
-static int gp2a_enable(struct gp2a_data *dt)
-{
-	return i2c_smbus_write_byte_data(dt->i2c_client, GP2A_ADDR_OPMOD,
-					 GP2A_CTRL_SSD);
-}
-
-static int gp2a_disable(struct gp2a_data *dt)
-{
-	return i2c_smbus_write_byte_data(dt->i2c_client, GP2A_ADDR_OPMOD,
-					 0x00);
-}
-
-static int __devinit gp2a_initialize(struct gp2a_data *dt)
-{
-	int error;
-
-	error = i2c_smbus_write_byte_data(dt->i2c_client, GP2A_ADDR_GAIN,
-					  0x08);
-	if (error < 0)
-		return error;
-
-	error = i2c_smbus_write_byte_data(dt->i2c_client, GP2A_ADDR_HYS,
-					  0xc2);
-	if (error < 0)
-		return error;
-
-	error = i2c_smbus_write_byte_data(dt->i2c_client, GP2A_ADDR_CYCLE,
-					  0x04);
-	if (error < 0)
-		return error;
-
-	error = gp2a_disable(dt);
-
-	return error;
-}
-
 static int gp2a_report(struct gp2a_data *dt)
 {
 	int vo = gpio_get_value(dt->pdata->vout_gpio);
 
-	input_report_switch(dt->device, SW_FRONT_PROXIMITY, !vo);
-	input_sync(dt->device);
+	input_report_switch(dt->input, SW_FRONT_PROXIMITY, !vo);
+	input_sync(dt->input);
 
 	return 0;
 }
@@ -94,6 +58,18 @@ static irqreturn_t gp2a_irq(int irq, void *handle)
 	return IRQ_HANDLED;
 }
 
+static int gp2a_enable(struct gp2a_data *dt)
+{
+	return i2c_smbus_write_byte_data(dt->i2c_client, GP2A_ADDR_OPMOD,
+					 GP2A_CTRL_SSD);
+}
+
+static int gp2a_disable(struct gp2a_data *dt)
+{
+	return i2c_smbus_write_byte_data(dt->i2c_client, GP2A_ADDR_OPMOD,
+					 0x00);
+}
+
 static int gp2a_device_open(struct input_dev *dev)
 {
 	struct gp2a_data *dt = input_get_drvdata(dev);
@@ -101,7 +77,8 @@ static int gp2a_device_open(struct input_dev *dev)
 
 	error = gp2a_enable(dt);
 	if (error < 0) {
-		dev_err(&dev->dev, "unable to activate, err %d\n", error);
+		dev_err(&dt->i2c_client->dev,
+			"unable to activate, err %d\n", error);
 		return error;
 	}
 
@@ -117,17 +94,41 @@ static void gp2a_device_close(struct input_dev *dev)
 
 	error = gp2a_disable(dt);
 	if (error < 0)
-		dev_err(&dev->dev, "unable to deactivate, err %d\n", error);
+		dev_err(&dt->i2c_client->dev,
+			"unable to deactivate, err %d\n", error);
+}
+
+static int __devinit gp2a_initialize(struct gp2a_data *dt)
+{
+	int error;
+
+	error = i2c_smbus_write_byte_data(dt->i2c_client, GP2A_ADDR_GAIN,
+					  0x08);
+	if (error < 0)
+		return error;
+
+	error = i2c_smbus_write_byte_data(dt->i2c_client, GP2A_ADDR_HYS,
+					  0xc2);
+	if (error < 0)
+		return error;
+
+	error = i2c_smbus_write_byte_data(dt->i2c_client, GP2A_ADDR_CYCLE,
+					  0x04);
+	if (error < 0)
+		return error;
+
+	error = gp2a_disable(dt);
+
+	return error;
 }
 
 static int __devinit gp2a_probe(struct i2c_client *client,
 				const struct i2c_device_id *id)
 {
-	const struct gp2a_platform_data *pdata;
+	const struct gp2a_platform_data *pdata = client->dev.platform_data;
 	struct gp2a_data *dt;
 	int error;
 
-	pdata = client->dev.platform_data;
 	if (!pdata)
 		return -EINVAL;
 
@@ -137,63 +138,67 @@ static int __devinit gp2a_probe(struct i2c_client *client,
 			return error;
 	}
 
-	error = gpio_direction_input(pdata->vout_gpio);
-	if (error < 0)
+	error = gpio_request_one(pdata->vout_gpio, GPIOF_IN, GP2A_I2C_NAME);
+	if (error)
 		goto err_hw_shutdown;
 
 	dt = kzalloc(sizeof(struct gp2a_data), GFP_KERNEL);
 	if (!dt) {
 		error = -ENOMEM;
-		goto err_hw_shutdown;
+		goto err_free_gpio;
 	}
 
 	dt->pdata = pdata;
 	dt->i2c_client = client;
-	i2c_set_clientdata(client, dt);
 
 	error = gp2a_initialize(dt);
 	if (error < 0)
 		goto err_free_mem;
 
-	dt->device = input_allocate_device();
-	if (!dt->device) {
+	dt->input = input_allocate_device();
+	if (!dt->input) {
 		error = -ENOMEM;
 		goto err_free_mem;
 	}
-	input_set_drvdata(dt->device, dt);
 
-	dt->device->open = gp2a_device_open;
-	dt->device->close = gp2a_device_close;
-	dt->device->name = GP2A_I2C_NAME;
-	dt->device->id.bustype = BUS_I2C;
+	input_set_drvdata(dt->input, dt);
+
+	dt->input->open = gp2a_device_open;
+	dt->input->close = gp2a_device_close;
+	dt->input->name = GP2A_I2C_NAME;
+	dt->input->id.bustype = BUS_I2C;
+	dt->input->dev.parent = &client->dev;
 
-	input_set_capability(dt->device, EV_SW, SW_FRONT_PROXIMITY);
+	input_set_capability(dt->input, EV_SW, SW_FRONT_PROXIMITY);
 
-	error = request_threaded_irq(client->irq, NULL,
-			gp2a_irq, IRQF_TRIGGER_RISING |
-			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+	error = request_threaded_irq(client->irq, NULL, gp2a_irq,
+			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
+				IRQF_ONESHOT,
 			GP2A_I2C_NAME, dt);
 	if (error) {
 		dev_err(&client->dev, "irq request failed\n");
 		goto err_free_input_dev;
 	}
 
-	error = input_register_device(dt->device);
+	error = input_register_device(dt->input);
 	if (error) {
 		dev_err(&client->dev, "device registration failed\n");
 		goto err_free_irq;
 	}
 
 	device_init_wakeup(&client->dev, pdata->wakeup);
+	i2c_set_clientdata(client, dt);
 
 	return 0;
 
 err_free_irq:
 	free_irq(client->irq, dt);
 err_free_input_dev:
-	input_free_device(dt->device);
+	input_free_device(dt->input);
 err_free_mem:
 	kfree(dt);
+err_free_gpio:
+	gpio_free(pdata->vout_gpio);
 err_hw_shutdown:
 	if (pdata->hw_shutdown)
 		pdata->hw_shutdown(client);
@@ -203,15 +208,20 @@ err_hw_shutdown:
 static int __devexit gp2a_remove(struct i2c_client *client)
 {
 	struct gp2a_data *dt = i2c_get_clientdata(client);
+	const struct gp2a_platform_data *pdata = dt->pdata;
 
 	device_init_wakeup(&client->dev, false);
 
 	free_irq(client->irq, dt);
-	input_unregister_device(dt->device);
-	if (dt->pdata->hw_shutdown)
-		dt->pdata->hw_shutdown(client);
+
+	input_unregister_device(dt->input);
 	kfree(dt);
 
+	gpio_free(pdata->vout_gpio);
+
+	if (pdata->hw_shutdown)
+		pdata->hw_shutdown(client);
+
 	return 0;
 }
 
@@ -220,42 +230,36 @@ static int gp2a_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct gp2a_data *dt = i2c_get_clientdata(client);
-	int error;
-
-	mutex_lock(&dt->device->mutex);
+	int retval = 0;
 
 	if (device_may_wakeup(&client->dev)) {
 		enable_irq_wake(client->irq);
-	} else if (dt->device->users) {
-		error = gp2a_disable(dt);
-		if (error < 0)
-			return error;
+	} else {
+		mutex_lock(&dt->input->mutex);
+		if (dt->input->users)
+			retval = gp2a_disable(dt);
+		mutex_unlock(&dt->input->mutex);
 	}
 
-	mutex_unlock(&dt->device->mutex);
-
-	return 0;
+	return retval;
 }
 
 static int gp2a_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct gp2a_data *dt = i2c_get_clientdata(client);
-	int error;
-
-	mutex_lock(&dt->device->mutex);
+	int retval = 0;
 
 	if (device_may_wakeup(&client->dev)) {
 		disable_irq_wake(client->irq);
-	} else if (dt->device->users) {
-		error = gp2a_enable(dt);
-		if (error < 0)
-			return error;
+	} else {
+		mutex_lock(&dt->input->mutex);
+		if (dt->input->users)
+			retval = gp2a_enable(dt);
+		mutex_unlock(&dt->input->mutex);
 	}
 
-	mutex_unlock(&dt->device->mutex);
-
-	return 0;
+	return retval;
 }
 #endif
 
@@ -270,11 +274,11 @@ static struct i2c_driver gp2a_i2c_driver = {
 	.driver = {
 		.name	= GP2A_I2C_NAME,
 		.owner	= THIS_MODULE,
-		.pm	= &gp2a_pm
+		.pm	= &gp2a_pm,
 	},
 	.probe		= gp2a_probe,
 	.remove		= __devexit_p(gp2a_remove),
-	.id_table	= gp2a_i2c_id
+	.id_table	= gp2a_i2c_id,
 };
 
 static int __init gp2a_init(void)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists