[PATCH v2] input: add driver support for Sharp gp2ap002a00f proximity sensor

[PATCH v2] input: add driver support for Sharp gp2ap002a00f proximity sensor

[oskar.andero]

From: Courtney Cavin <courtney.cavin@sonyericsson.com>

Signed-off-by: Courtney Cavin <courtney.cavin@sonyericsson.com>
Signed-off-by: Oskar Andero <oskar.andero@sonyericsson.com>
---
 drivers/input/misc/Kconfig         |   11 ++
 drivers/input/misc/Makefile        |    1 +
 drivers/input/misc/gp2ap002a00f.c  |  286 ++++++++++++++++++++++++++++++++++++
 include/linux/input/gp2ap002a00f.h |   22 +++
 4 files changed, 320 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/misc/gp2ap002a00f.c
 create mode 100644 include/linux/input/gp2ap002a00f.h

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 22d875f..dee96a0 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -544,4 +544,15 @@ 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
+	default n
+	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 a244fc6..1681993 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_INPUT_CMA3000)		+= cma3000_d0x.o
 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_IXP4XX_BEEPER)	+= ixp4xx-beeper.o
 obj-$(CONFIG_INPUT_KEYSPAN_REMOTE)	+= keyspan_remote.o
diff --git a/drivers/input/misc/gp2ap002a00f.c b/drivers/input/misc/gp2ap002a00f.c
new file mode 100644
index 0000000..1c5ddf8
--- /dev/null
+++ b/drivers/input/misc/gp2ap002a00f.c
@@ -0,0 +1,286 @@
+/*
+ * Copyright (C) 2009,2010 Sony Ericsson Mobile Communications Inc.
+ *
+ * Author: Courtney Cavin <courtney.cavin@sonyericsson.com>
+ * Prepared for up-stream by: Oskar Andero <oskar.andero@sonyericsson.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2, as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/i2c.h>
+#include <linux/irq.h>
+#include <linux/slab.h>
+#include <linux/input.h>
+#include <linux/module.h>
+#include <linux/workqueue.h>
+#include <linux/interrupt.h>
+#include <linux/gpio.h>
+#include <linux/delay.h>
+#include <linux/input/gp2ap002a00f.h>
+
+struct gp2a_data {
+	struct input_dev *device;
+	const struct gp2a_platform_data *pdata;
+	struct i2c_client *i2c_client;
+};
+
+enum gp2a_addr {
+	GP2A_ADDR_PROX	= 0x0,
+	GP2A_ADDR_GAIN	= 0x1,
+	GP2A_ADDR_HYS	= 0x2,
+	GP2A_ADDR_CYCLE	= 0x3,
+	GP2A_ADDR_OPMOD	= 0x4,
+	GP2A_ADDR_CON	= 0x6
+};
+
+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_key(dt->device, SW_FRONT_PROXIMITY, !vo);
+	input_sync(dt->device);
+
+	return 0;
+}
+
+static irqreturn_t gp2a_irq(int irq, void *handle)
+{
+	struct gp2a_data *dt = handle;
+
+	gp2a_report(dt);
+
+	return IRQ_HANDLED;
+}
+
+static int gp2a_device_open(struct input_dev *dev)
+{
+	struct gp2a_data *dt = input_get_drvdata(dev);
+	int error;
+
+	error = gp2a_enable(dt);
+	if (error < 0) {
+		dev_err(&dev->dev, "unable to activate, err %d\n", error);
+		return error;
+	}
+
+	gp2a_report(dt);
+
+	return 0;
+}
+
+static void gp2a_device_close(struct input_dev *dev)
+{
+	struct gp2a_data *dt = input_get_drvdata(dev);
+	int error;
+
+	error = gp2a_disable(dt);
+	if (error < 0)
+		dev_err(&dev->dev, "unable to deactivate, err %d\n", error);
+}
+
+static int __devinit gp2a_probe(struct i2c_client *client,
+				const struct i2c_device_id *id)
+{
+	const struct gp2a_platform_data *pdata;
+	struct gp2a_data *dt;
+	int error;
+
+	pdata = client->dev.platform_data;
+	if (!pdata)
+		return -EINVAL;
+
+	if (pdata->hw_setup) {
+		error = pdata->hw_setup(client);
+		if (error < 0)
+			return error;
+	}
+
+	error = gpio_direction_input(pdata->vout_gpio);
+	if (error < 0)
+		goto err_hw_shutdown;
+
+	dt = kzalloc(sizeof(struct gp2a_data), GFP_KERNEL);
+	if (!dt) {
+		error = -ENOMEM;
+		goto err_hw_shutdown;
+	}
+
+	dt->pdata = pdata;
+	dt->i2c_client = client;
+	i2c_set_clientdata(client, dt);
+
+	error = gp2a_initialize(dt);
+	if (error < 0)
+		goto err_free_dt;
+
+	dt->device = input_allocate_device();
+	if (!dt->device) {
+		error = -ENOMEM;
+		goto err_free_dt;
+	}
+	input_set_drvdata(dt->device, dt);
+
+	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(&dt->device->dev, "irq request failed\n");
+		goto err_free_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_capability(dt->device, EV_KEY, SW_FRONT_PROXIMITY);
+
+	error = input_register_device(dt->device);
+	if (error) {
+		dev_err(&dt->device->dev, "device registration failed\n");
+		input_free_device(dt->device);
+		goto err_free_irq;
+	}
+
+	device_init_wakeup(&client->dev, pdata->wakeup);
+
+	return 0;
+
+err_free_irq:
+	free_irq(client->irq, dt);
+err_free_dt:
+	kfree(dt);
+err_hw_shutdown:
+	if (pdata->hw_shutdown)
+		pdata->hw_shutdown(client);
+	return error;
+}
+
+static int __devexit gp2a_remove(struct i2c_client *client)
+{
+	struct gp2a_data *dt = i2c_get_clientdata(client);
+
+	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);
+	kfree(dt);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+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;
+
+	if (device_may_wakeup(&client->dev)) {
+		enable_irq_wake(client->irq);
+	} else {
+		error = gp2a_disable(dt);
+		if (error < 0)
+			return error;
+	}
+
+	return 0;
+}
+
+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;
+
+	if (device_may_wakeup(&client->dev)) {
+		disable_irq_wake(client->irq);
+	} else {
+		error = gp2a_enable(dt);
+		if (error < 0)
+			return error;
+	}
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(gp2a_pm, gp2a_suspend, gp2a_resume);
+
+static const struct i2c_device_id gp2a_i2c_id[] = {
+	{ GP2A_I2C_NAME, 0 },
+	{ }
+};
+
+static struct i2c_driver gp2a_i2c_driver = {
+	.driver = {
+		.name	= GP2A_I2C_NAME,
+		.owner	= THIS_MODULE,
+		.pm	= &gp2a_pm
+	},
+	.probe		= gp2a_probe,
+	.remove		= __devexit_p(gp2a_remove),
+	.id_table	= gp2a_i2c_id
+};
+
+static int __init gp2a_init(void)
+{
+	return i2c_add_driver(&gp2a_i2c_driver);
+}
+
+static void __exit gp2a_exit(void)
+{
+	i2c_del_driver(&gp2a_i2c_driver);
+}
+
+module_init(gp2a_init);
+module_exit(gp2a_exit);
+
+MODULE_AUTHOR("Courtney Cavin <courtney.cavin@sonyericsson.com>");
+MODULE_DESCRIPTION("Sharp GP2AP002A00F I2C Proximity/Opto sensor driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/input/gp2ap002a00f.h b/include/linux/input/gp2ap002a00f.h
new file mode 100644
index 0000000..aad2fd4
--- /dev/null
+++ b/include/linux/input/gp2ap002a00f.h
@@ -0,0 +1,22 @@
+#ifndef _GP2AP002A00F_H_
+#define _GP2AP002A00F_H_
+
+#include <linux/i2c.h>
+
+#define GP2A_I2C_NAME "gp2ap002a00f"
+
+/**
+ * struct gp2a_platform_data - Sharp gp2ap002a00f proximity platform data
+ * @vout_gpio: The gpio connected to the object detected pin (VOUT)
+ * @wakeup: Set to true if the proximity can wake the device from suspend
+ * @hw_setup: Callback for setting up hardware such as gpios and vregs
+ * @hw_shutdown: Callback for properly shutting down hardware
+ */
+struct gp2a_platform_data {
+	int vout_gpio;
+	bool wakeup;
+	int (*hw_setup)(struct i2c_client *client);
+	int (*hw_shutdown)(struct i2c_client *client);
+};
+
+#endif
-- 
1.7.7

Re: [PATCH v2] input: add driver support for Sharp gp2ap002a00f proximity sensor

[Dmitry Torokhov]

Hi Courtney,

On Mon, Nov 14, 2011 at 04:39:18PM +0100, oskar.andero@sonyericsson.com wrote:
> From: Courtney Cavin <courtney.cavin@sonyericsson.com>
> 

The driver looks much better, a few more comments still.

> +
> +#include <linux/i2c.h>
> +#include <linux/irq.h>
> +#include <linux/slab.h>
> +#include <linux/input.h>
> +#include <linux/module.h>
> +#include <linux/workqueue.h>

As Shubhrajyoti mentioned workqueue.h is probably not needed.

> +#include <linux/interrupt.h>
> +#include <linux/gpio.h>
> +#include <linux/delay.h>
> +#include <linux/input/gp2ap002a00f.h>
> +
> +struct gp2a_data {
> +	struct input_dev *device;
> +	const struct gp2a_platform_data *pdata;
> +	struct i2c_client *i2c_client;
> +};
> +
> +enum gp2a_addr {
> +	GP2A_ADDR_PROX	= 0x0,
> +	GP2A_ADDR_GAIN	= 0x1,
> +	GP2A_ADDR_HYS	= 0x2,
> +	GP2A_ADDR_CYCLE	= 0x3,
> +	GP2A_ADDR_OPMOD	= 0x4,
> +	GP2A_ADDR_CON	= 0x6
> +};
> +
> +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_key(dt->device, SW_FRONT_PROXIMITY, !vo);

Switch is not a key, so please use input_report_switch().

> +	input_sync(dt->device);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t gp2a_irq(int irq, void *handle)
> +{
> +	struct gp2a_data *dt = handle;
> +
> +	gp2a_report(dt);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int gp2a_device_open(struct input_dev *dev)
> +{
> +	struct gp2a_data *dt = input_get_drvdata(dev);
> +	int error;
> +
> +	error = gp2a_enable(dt);
> +	if (error < 0) {
> +		dev_err(&dev->dev, "unable to activate, err %d\n", error);
> +		return error;
> +	}
> +
> +	gp2a_report(dt);
> +
> +	return 0;
> +}
> +
> +static void gp2a_device_close(struct input_dev *dev)
> +{
> +	struct gp2a_data *dt = input_get_drvdata(dev);
> +	int error;
> +
> +	error = gp2a_disable(dt);
> +	if (error < 0)
> +		dev_err(&dev->dev, "unable to deactivate, err %d\n", error);
> +}
> +
> +static int __devinit gp2a_probe(struct i2c_client *client,
> +				const struct i2c_device_id *id)
> +{
> +	const struct gp2a_platform_data *pdata;
> +	struct gp2a_data *dt;
> +	int error;
> +
> +	pdata = client->dev.platform_data;
> +	if (!pdata)
> +		return -EINVAL;
> +
> +	if (pdata->hw_setup) {
> +		error = pdata->hw_setup(client);
> +		if (error < 0)
> +			return error;
> +	}
> +
> +	error = gpio_direction_input(pdata->vout_gpio);
> +	if (error < 0)
> +		goto err_hw_shutdown;
> +
> +	dt = kzalloc(sizeof(struct gp2a_data), GFP_KERNEL);
> +	if (!dt) {
> +		error = -ENOMEM;
> +		goto err_hw_shutdown;
> +	}
> +
> +	dt->pdata = pdata;
> +	dt->i2c_client = client;
> +	i2c_set_clientdata(client, dt);
> +
> +	error = gp2a_initialize(dt);
> +	if (error < 0)
> +		goto err_free_dt;
> +
> +	dt->device = input_allocate_device();
> +	if (!dt->device) {
> +		error = -ENOMEM;
> +		goto err_free_dt;
> +	}
> +	input_set_drvdata(dt->device, dt);
> +
> +	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(&dt->device->dev, "irq request failed\n");

You are leaking dt->device here. Please add a separate label for
input_free_device(dt->device).

> +		goto err_free_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_capability(dt->device, EV_KEY, SW_FRONT_PROXIMITY);

Should be EV_SW instead of EV_KEY.

> +
> +	error = input_register_device(dt->device);
> +	if (error) {
> +		dev_err(&dt->device->dev, "device registration failed\n");
> +		input_free_device(dt->device);

If you swap request_threaded_irq() and input_register_device() so that
registration is the last action error handling will be much simpler.

> +		goto err_free_irq;
> +	}
> +
> +	device_init_wakeup(&client->dev, pdata->wakeup);
> +
> +	return 0;
> +
> +err_free_irq:
> +	free_irq(client->irq, dt);
> +err_free_dt:
> +	kfree(dt);
> +err_hw_shutdown:
> +	if (pdata->hw_shutdown)
> +		pdata->hw_shutdown(client);
> +	return error;
> +}
> +
> +static int __devexit gp2a_remove(struct i2c_client *client)
> +{
> +	struct gp2a_data *dt = i2c_get_clientdata(client);
> +
> +	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);
> +	kfree(dt);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +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;
> +
> +	if (device_may_wakeup(&client->dev)) {
> +		enable_irq_wake(client->irq);
> +	} else {

This needs locking WRT open/close. Please acquire dt->device->mutex and
only disable if dt->device->users != 0. Similar shoudl be done for
resume.

> +		error = gp2a_disable(dt);
> +		if (error < 0)
> +			return error;
> +	}
> +
> +	return 0;
> +}
> +
> +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;
> +
> +	if (device_may_wakeup(&client->dev)) {
> +		disable_irq_wake(client->irq);
> +	} else {
> +		error = gp2a_enable(dt);
> +		if (error < 0)
> +			return error;
> +	}
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(gp2a_pm, gp2a_suspend, gp2a_resume);
> +
> +static const struct i2c_device_id gp2a_i2c_id[] = {
> +	{ GP2A_I2C_NAME, 0 },
> +	{ }
> +};
> +
> +static struct i2c_driver gp2a_i2c_driver = {
> +	.driver = {
> +		.name	= GP2A_I2C_NAME,
> +		.owner	= THIS_MODULE,
> +		.pm	= &gp2a_pm
> +	},
> +	.probe		= gp2a_probe,
> +	.remove		= __devexit_p(gp2a_remove),
> +	.id_table	= gp2a_i2c_id
> +};
> +
> +static int __init gp2a_init(void)
> +{
> +	return i2c_add_driver(&gp2a_i2c_driver);
> +}
> +
> +static void __exit gp2a_exit(void)
> +{
> +	i2c_del_driver(&gp2a_i2c_driver);
> +}
> +
> +module_init(gp2a_init);
> +module_exit(gp2a_exit);
> +
> +MODULE_AUTHOR("Courtney Cavin <courtney.cavin@sonyericsson.com>");
> +MODULE_DESCRIPTION("Sharp GP2AP002A00F I2C Proximity/Opto sensor driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/input/gp2ap002a00f.h b/include/linux/input/gp2ap002a00f.h
> new file mode 100644
> index 0000000..aad2fd4
> --- /dev/null
> +++ b/include/linux/input/gp2ap002a00f.h
> @@ -0,0 +1,22 @@
> +#ifndef _GP2AP002A00F_H_
> +#define _GP2AP002A00F_H_
> +
> +#include <linux/i2c.h>
> +
> +#define GP2A_I2C_NAME "gp2ap002a00f"
> +
> +/**
> + * struct gp2a_platform_data - Sharp gp2ap002a00f proximity platform data
> + * @vout_gpio: The gpio connected to the object detected pin (VOUT)
> + * @wakeup: Set to true if the proximity can wake the device from suspend
> + * @hw_setup: Callback for setting up hardware such as gpios and vregs
> + * @hw_shutdown: Callback for properly shutting down hardware
> + */
> +struct gp2a_platform_data {
> +	int vout_gpio;
> +	bool wakeup;
> +	int (*hw_setup)(struct i2c_client *client);
> +	int (*hw_shutdown)(struct i2c_client *client);
> +};
> +
> +#endif
> -- 
> 1.7.7
> 

Thanks.

-- 
Dmitry

Re: [PATCH v2] input: add driver support for Sharp gp2ap002a00f proximity sensor

[oskar.andero]

Hi Shubhrajyoti,

Thanks for reviewing!

On 17:52 Mon 14 Nov     , Shubhrajyoti Datta wrote:
>    Hi Courtney,
>    Some minor comments
> 
>    On Mon, Nov 14, 2011 at 9:09 PM, <[1]oskar.andero@sonyericsson.com>
>    wrote:
> 
>      From: Courtney Cavin <[2]courtney.cavin@sonyericsson.com>
>      Signed-off-by: Courtney Cavin <[3]courtney.cavin@sonyericsson.com>
>      Signed-off-by: Oskar Andero <[4]oskar.andero@sonyericsson.com>
>      ---
>       drivers/input/misc/Kconfig         |   11 ++
>       drivers/input/misc/Makefile        |    1 +
>       drivers/input/misc/gp2ap002a00f.c  |  286
>      ++++++++++++++++++++++++++++++++++++
>       include/linux/input/gp2ap002a00f.h |   22 +++
>       4 files changed, 320 insertions(+), 0 deletions(-)
>       create mode 100644 drivers/input/misc/gp2ap002a00f.c
>       create mode 100644 include/linux/input/gp2ap002a00f.h
>      diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
>      index 22d875f..dee96a0 100644
>      --- a/drivers/input/misc/Kconfig
>      +++ b/drivers/input/misc/Kconfig
>      @@ -544,4 +544,15 @@ 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
>      +       default n
> 
>    you may want to drop it as default is anyways n.
> 
>      +       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 a244fc6..1681993 100644
>      --- a/drivers/input/misc/Makefile
>      +++ b/drivers/input/misc/Makefile
>      @@ -22,6 +22,7 @@ obj-$(CONFIG_INPUT_CMA3000)           +=
>      cma3000_d0x.o
>       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_IXP4XX_BEEPER)      += ixp4xx-beeper.o
>       obj-$(CONFIG_INPUT_KEYSPAN_REMOTE)     += keyspan_remote.o
>      diff --git a/drivers/input/misc/gp2ap002a00f.c
>      b/drivers/input/misc/gp2ap002a00f.c
>      new file mode 100644
>      index 0000000..1c5ddf8
>      --- /dev/null
>      +++ b/drivers/input/misc/gp2ap002a00f.c
>      @@ -0,0 +1,286 @@
>      +/*
>      + * Copyright (C) 2009,2010 Sony Ericsson Mobile Communications Inc.
> 
>    May want to check the year.
> 
>      + *
>      + * Author: Courtney Cavin <[5]courtney.cavin@sonyericsson.com>
>      + * Prepared for up-stream by: Oskar Andero
>      <[6]oskar.andero@sonyericsson.com>
> 
>      + *
>      + * This program is free software; you can redistribute it and/or
>      modify
>      + * it under the terms of the GNU General Public License version 2,
>      as
>      + * published by the Free Software Foundation.
>      + */
>      +
>      +#include <linux/i2c.h>
>      +#include <linux/irq.h>
>      +#include <linux/slab.h>
>      +#include <linux/input.h>
>      +#include <linux/module.h>
>      +#include <linux/workqueue.h>
> 
>    Are all these header files needed?
> 
>      +#include <linux/interrupt.h>
>      +#include <linux/gpio.h>
>      +#include <linux/delay.h>
>      +#include <linux/input/gp2ap002a00f.h>
>      +
>      +struct gp2a_data {
>      +       struct input_dev *device;
>      +       const struct gp2a_platform_data *pdata;
>      +       struct i2c_client *i2c_client;
>      +};
>      +
>      +enum gp2a_addr {
>      +       GP2A_ADDR_PROX  = 0x0,
>      +       GP2A_ADDR_GAIN  = 0x1,
>      +       GP2A_ADDR_HYS   = 0x2,
>      +       GP2A_ADDR_CYCLE = 0x3,
>      +       GP2A_ADDR_OPMOD = 0x4,
>      +       GP2A_ADDR_CON   = 0x6
>      +};
>      +
>      +enum gp2a_controls {
>      +       GP2A_CTRL_SSD   = 0x01
> 
>    Not a comment really
>    Could you explain SSD?

SSD means Software shutdown according to the datasheet where 0 puts the
chip in shutdown and 1 in normal operation. I'll add a comment in the code.

-Oskar

Re: [PATCH v2] input: add driver support for Sharp gp2ap002a00f proximity sensor

[oskar.andero]

Hi Dmitry,

On 18:03 Mon 14 Nov     , Dmitry Torokhov wrote:
> Hi Courtney,
> 
> On Mon, Nov 14, 2011 at 04:39:18PM +0100, oskar.andero@sonyericsson.com wrote:
> > From: Courtney Cavin <courtney.cavin@sonyericsson.com>
> > 
> 
> The driver looks much better, a few more comments still.
> 
> > +
> > +#include <linux/i2c.h>
> > +#include <linux/irq.h>
> > +#include <linux/slab.h>
> > +#include <linux/input.h>
> > +#include <linux/module.h>
> > +#include <linux/workqueue.h>
> 
> As Shubhrajyoti mentioned workqueue.h is probably not needed.
> 
> > +#include <linux/interrupt.h>
> > +#include <linux/gpio.h>
> > +#include <linux/delay.h>
> > +#include <linux/input/gp2ap002a00f.h>
> > +
> > +struct gp2a_data {
> > +	struct input_dev *device;
> > +	const struct gp2a_platform_data *pdata;
> > +	struct i2c_client *i2c_client;
> > +};
> > +
> > +enum gp2a_addr {
> > +	GP2A_ADDR_PROX	= 0x0,
> > +	GP2A_ADDR_GAIN	= 0x1,
> > +	GP2A_ADDR_HYS	= 0x2,
> > +	GP2A_ADDR_CYCLE	= 0x3,
> > +	GP2A_ADDR_OPMOD	= 0x4,
> > +	GP2A_ADDR_CON	= 0x6
> > +};
> > +
> > +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_key(dt->device, SW_FRONT_PROXIMITY, !vo);
> 
> Switch is not a key, so please use input_report_switch().

Ok. Agreed.

> > +	input_sync(dt->device);
> > +
> > +	return 0;
> > +}
> > +
> > +static irqreturn_t gp2a_irq(int irq, void *handle)
> > +{
> > +	struct gp2a_data *dt = handle;
> > +
> > +	gp2a_report(dt);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int gp2a_device_open(struct input_dev *dev)
> > +{
> > +	struct gp2a_data *dt = input_get_drvdata(dev);
> > +	int error;
> > +
> > +	error = gp2a_enable(dt);
> > +	if (error < 0) {
> > +		dev_err(&dev->dev, "unable to activate, err %d\n", error);
> > +		return error;
> > +	}
> > +
> > +	gp2a_report(dt);
> > +
> > +	return 0;
> > +}
> > +
> > +static void gp2a_device_close(struct input_dev *dev)
> > +{
> > +	struct gp2a_data *dt = input_get_drvdata(dev);
> > +	int error;
> > +
> > +	error = gp2a_disable(dt);
> > +	if (error < 0)
> > +		dev_err(&dev->dev, "unable to deactivate, err %d\n", error);
> > +}
> > +
> > +static int __devinit gp2a_probe(struct i2c_client *client,
> > +				const struct i2c_device_id *id)
> > +{
> > +	const struct gp2a_platform_data *pdata;
> > +	struct gp2a_data *dt;
> > +	int error;
> > +
> > +	pdata = client->dev.platform_data;
> > +	if (!pdata)
> > +		return -EINVAL;
> > +
> > +	if (pdata->hw_setup) {
> > +		error = pdata->hw_setup(client);
> > +		if (error < 0)
> > +			return error;
> > +	}
> > +
> > +	error = gpio_direction_input(pdata->vout_gpio);
> > +	if (error < 0)
> > +		goto err_hw_shutdown;
> > +
> > +	dt = kzalloc(sizeof(struct gp2a_data), GFP_KERNEL);
> > +	if (!dt) {
> > +		error = -ENOMEM;
> > +		goto err_hw_shutdown;
> > +	}
> > +
> > +	dt->pdata = pdata;
> > +	dt->i2c_client = client;
> > +	i2c_set_clientdata(client, dt);
> > +
> > +	error = gp2a_initialize(dt);
> > +	if (error < 0)
> > +		goto err_free_dt;
> > +
> > +	dt->device = input_allocate_device();
> > +	if (!dt->device) {
> > +		error = -ENOMEM;
> > +		goto err_free_dt;
> > +	}
> > +	input_set_drvdata(dt->device, dt);
> > +
> > +	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(&dt->device->dev, "irq request failed\n");
> 
> You are leaking dt->device here. Please add a separate label for
> input_free_device(dt->device).

Yes, you are right.

> > +		goto err_free_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_capability(dt->device, EV_KEY, SW_FRONT_PROXIMITY);
> 
> Should be EV_SW instead of EV_KEY.
> 
> > +
> > +	error = input_register_device(dt->device);
> > +	if (error) {
> > +		dev_err(&dt->device->dev, "device registration failed\n");
> > +		input_free_device(dt->device);
> 
> If you swap request_threaded_irq() and input_register_device() so that
> registration is the last action error handling will be much simpler.

Sure. I'll look in to it.

> > +		goto err_free_irq;
> > +	}
> > +
> > +	device_init_wakeup(&client->dev, pdata->wakeup);
> > +
> > +	return 0;
> > +
> > +err_free_irq:
> > +	free_irq(client->irq, dt);
> > +err_free_dt:
> > +	kfree(dt);
> > +err_hw_shutdown:
> > +	if (pdata->hw_shutdown)
> > +		pdata->hw_shutdown(client);
> > +	return error;
> > +}
> > +
> > +static int __devexit gp2a_remove(struct i2c_client *client)
> > +{
> > +	struct gp2a_data *dt = i2c_get_clientdata(client);
> > +
> > +	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);
> > +	kfree(dt);
> > +
> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +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;
> > +
> > +	if (device_may_wakeup(&client->dev)) {
> > +		enable_irq_wake(client->irq);
> > +	} else {
> 
> This needs locking WRT open/close. Please acquire dt->device->mutex and
> only disable if dt->device->users != 0. Similar shoudl be done for
> resume.

I see what you mean. I will fix it.

> > +		error = gp2a_disable(dt);
> > +		if (error < 0)
> > +			return error;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +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;
> > +
> > +	if (device_may_wakeup(&client->dev)) {
> > +		disable_irq_wake(client->irq);
> > +	} else {
> > +		error = gp2a_enable(dt);
> > +		if (error < 0)
> > +			return error;
> > +	}
> > +
> > +	return 0;
> > +}
> > +#endif
> > +
> > +static SIMPLE_DEV_PM_OPS(gp2a_pm, gp2a_suspend, gp2a_resume);
> > +
> > +static const struct i2c_device_id gp2a_i2c_id[] = {
> > +	{ GP2A_I2C_NAME, 0 },
> > +	{ }
> > +};
> > +
> > +static struct i2c_driver gp2a_i2c_driver = {
> > +	.driver = {
> > +		.name	= GP2A_I2C_NAME,
> > +		.owner	= THIS_MODULE,
> > +		.pm	= &gp2a_pm
> > +	},
> > +	.probe		= gp2a_probe,
> > +	.remove		= __devexit_p(gp2a_remove),
> > +	.id_table	= gp2a_i2c_id
> > +};
> > +
> > +static int __init gp2a_init(void)
> > +{
> > +	return i2c_add_driver(&gp2a_i2c_driver);
> > +}
> > +
> > +static void __exit gp2a_exit(void)
> > +{
> > +	i2c_del_driver(&gp2a_i2c_driver);
> > +}
> > +
> > +module_init(gp2a_init);
> > +module_exit(gp2a_exit);
> > +
> > +MODULE_AUTHOR("Courtney Cavin <courtney.cavin@sonyericsson.com>");
> > +MODULE_DESCRIPTION("Sharp GP2AP002A00F I2C Proximity/Opto sensor driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/input/gp2ap002a00f.h b/include/linux/input/gp2ap002a00f.h
> > new file mode 100644
> > index 0000000..aad2fd4
> > --- /dev/null
> > +++ b/include/linux/input/gp2ap002a00f.h
> > @@ -0,0 +1,22 @@
> > +#ifndef _GP2AP002A00F_H_
> > +#define _GP2AP002A00F_H_
> > +
> > +#include <linux/i2c.h>
> > +
> > +#define GP2A_I2C_NAME "gp2ap002a00f"
> > +
> > +/**
> > + * struct gp2a_platform_data - Sharp gp2ap002a00f proximity platform data
> > + * @vout_gpio: The gpio connected to the object detected pin (VOUT)
> > + * @wakeup: Set to true if the proximity can wake the device from suspend
> > + * @hw_setup: Callback for setting up hardware such as gpios and vregs
> > + * @hw_shutdown: Callback for properly shutting down hardware
> > + */
> > +struct gp2a_platform_data {
> > +	int vout_gpio;
> > +	bool wakeup;
> > +	int (*hw_setup)(struct i2c_client *client);
> > +	int (*hw_shutdown)(struct i2c_client *client);
> > +};
> > +
> > +#endif
> > -- 
> > 1.7.7
> > 

Thanks for your comments! I'll prepare v3 later today.

-Oskar

Re: [PATCH v2] input: add driver support for Sharp gp2ap002a00f proximity sensor

[oskar.andero]

Hi again Dmitry,

Just a quick question before I post v3.

On 18:03 Mon 14 Nov     , Dmitry Torokhov wrote:
> Hi Courtney,
> 
> On Mon, Nov 14, 2011 at 04:39:18PM +0100, oskar.andero@sonyericsson.com wrote:
> > From: Courtney Cavin <courtney.cavin@sonyericsson.com>
> > 
> 
> The driver looks much better, a few more comments still.
> 
> > +
> > +#include <linux/i2c.h>
> > +#include <linux/irq.h>
> > +#include <linux/slab.h>
> > +#include <linux/input.h>
> > +#include <linux/module.h>
> > +#include <linux/workqueue.h>
> 
> As Shubhrajyoti mentioned workqueue.h is probably not needed.
> 
> > +#include <linux/interrupt.h>
> > +#include <linux/gpio.h>
> > +#include <linux/delay.h>
> > +#include <linux/input/gp2ap002a00f.h>
> > +
> > +struct gp2a_data {
> > +	struct input_dev *device;
> > +	const struct gp2a_platform_data *pdata;
> > +	struct i2c_client *i2c_client;
> > +};
> > +
> > +enum gp2a_addr {
> > +	GP2A_ADDR_PROX	= 0x0,
> > +	GP2A_ADDR_GAIN	= 0x1,
> > +	GP2A_ADDR_HYS	= 0x2,
> > +	GP2A_ADDR_CYCLE	= 0x3,
> > +	GP2A_ADDR_OPMOD	= 0x4,
> > +	GP2A_ADDR_CON	= 0x6
> > +};
> > +
> > +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_key(dt->device, SW_FRONT_PROXIMITY, !vo);
> 
> Switch is not a key, so please use input_report_switch().
> 
> > +	input_sync(dt->device);
> > +
> > +	return 0;
> > +}
> > +
> > +static irqreturn_t gp2a_irq(int irq, void *handle)
> > +{
> > +	struct gp2a_data *dt = handle;
> > +
> > +	gp2a_report(dt);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int gp2a_device_open(struct input_dev *dev)
> > +{
> > +	struct gp2a_data *dt = input_get_drvdata(dev);
> > +	int error;
> > +
> > +	error = gp2a_enable(dt);
> > +	if (error < 0) {
> > +		dev_err(&dev->dev, "unable to activate, err %d\n", error);
> > +		return error;
> > +	}
> > +
> > +	gp2a_report(dt);
> > +
> > +	return 0;
> > +}
> > +
> > +static void gp2a_device_close(struct input_dev *dev)
> > +{
> > +	struct gp2a_data *dt = input_get_drvdata(dev);
> > +	int error;
> > +
> > +	error = gp2a_disable(dt);
> > +	if (error < 0)
> > +		dev_err(&dev->dev, "unable to deactivate, err %d\n", error);
> > +}
> > +
> > +static int __devinit gp2a_probe(struct i2c_client *client,
> > +				const struct i2c_device_id *id)
> > +{
> > +	const struct gp2a_platform_data *pdata;
> > +	struct gp2a_data *dt;
> > +	int error;
> > +
> > +	pdata = client->dev.platform_data;
> > +	if (!pdata)
> > +		return -EINVAL;
> > +
> > +	if (pdata->hw_setup) {
> > +		error = pdata->hw_setup(client);
> > +		if (error < 0)
> > +			return error;
> > +	}
> > +
> > +	error = gpio_direction_input(pdata->vout_gpio);
> > +	if (error < 0)
> > +		goto err_hw_shutdown;
> > +
> > +	dt = kzalloc(sizeof(struct gp2a_data), GFP_KERNEL);
> > +	if (!dt) {
> > +		error = -ENOMEM;
> > +		goto err_hw_shutdown;
> > +	}
> > +
> > +	dt->pdata = pdata;
> > +	dt->i2c_client = client;
> > +	i2c_set_clientdata(client, dt);
> > +
> > +	error = gp2a_initialize(dt);
> > +	if (error < 0)
> > +		goto err_free_dt;
> > +
> > +	dt->device = input_allocate_device();
> > +	if (!dt->device) {
> > +		error = -ENOMEM;
> > +		goto err_free_dt;
> > +	}
> > +	input_set_drvdata(dt->device, dt);
> > +
> > +	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(&dt->device->dev, "irq request failed\n");
> 
> You are leaking dt->device here. Please add a separate label for
> input_free_device(dt->device).
> 
> > +		goto err_free_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_capability(dt->device, EV_KEY, SW_FRONT_PROXIMITY);
> 
> Should be EV_SW instead of EV_KEY.
> 
> > +
> > +	error = input_register_device(dt->device);
> > +	if (error) {
> > +		dev_err(&dt->device->dev, "device registration failed\n");
> > +		input_free_device(dt->device);
> 
> If you swap request_threaded_irq() and input_register_device() so that
> registration is the last action error handling will be much simpler.
>

I am getting a bit confused here, since you asked me to swap the order
of request_threaded_irq() and input_register_device() in my previous
version as well. Swapping again will take us back to square one or maybe
I am misinterpreting your comment?

Anyways, this is what I am planning to do for v3:

error = input_register_device(dt->device);
if (error) {
	...
	goto err_free_dt;
}
...
error = request_threaded_irq(client->irq, NULL, ...
if (error) {
	...
	goto err_free_unregister;
}
...
err_free_unregister:
        input_unregister_device(dt->device);
err_free_dt:
        kfree(dt);
err_hw_shutdown:
        if (pdata->hw_shutdown)
                pdata->hw_shutdown(client);
        return error;

The above takes care of the leaking input_device and keeps the error handling
pretty compact.
Does this sound reasonable?

-Oskar

Re: [PATCH v2] input: add driver support for Sharp gp2ap002a00f proximity sensor

[Dmitry Torokhov]

On Tue, Nov 15, 2011 at 08:56:29AM +0100, oskar.andero@sonyericsson.com wrote:
> Hi again Dmitry,
> 
> Just a quick question before I post v3.
> 
> On 18:03 Mon 14 Nov     , Dmitry Torokhov wrote:
> > Hi Courtney,
> > 
> > On Mon, Nov 14, 2011 at 04:39:18PM +0100, oskar.andero@sonyericsson.com wrote:
> > > From: Courtney Cavin <courtney.cavin@sonyericsson.com>
> > > 
> > 
> > The driver looks much better, a few more comments still.
> > 
> > > +
> > > +#include <linux/i2c.h>
> > > +#include <linux/irq.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/input.h>
> > > +#include <linux/module.h>
> > > +#include <linux/workqueue.h>
> > 
> > As Shubhrajyoti mentioned workqueue.h is probably not needed.
> > 
> > > +#include <linux/interrupt.h>
> > > +#include <linux/gpio.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/input/gp2ap002a00f.h>
> > > +
> > > +struct gp2a_data {
> > > +	struct input_dev *device;
> > > +	const struct gp2a_platform_data *pdata;
> > > +	struct i2c_client *i2c_client;
> > > +};
> > > +
> > > +enum gp2a_addr {
> > > +	GP2A_ADDR_PROX	= 0x0,
> > > +	GP2A_ADDR_GAIN	= 0x1,
> > > +	GP2A_ADDR_HYS	= 0x2,
> > > +	GP2A_ADDR_CYCLE	= 0x3,
> > > +	GP2A_ADDR_OPMOD	= 0x4,
> > > +	GP2A_ADDR_CON	= 0x6
> > > +};
> > > +
> > > +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_key(dt->device, SW_FRONT_PROXIMITY, !vo);
> > 
> > Switch is not a key, so please use input_report_switch().
> > 
> > > +	input_sync(dt->device);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static irqreturn_t gp2a_irq(int irq, void *handle)
> > > +{
> > > +	struct gp2a_data *dt = handle;
> > > +
> > > +	gp2a_report(dt);
> > > +
> > > +	return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int gp2a_device_open(struct input_dev *dev)
> > > +{
> > > +	struct gp2a_data *dt = input_get_drvdata(dev);
> > > +	int error;
> > > +
> > > +	error = gp2a_enable(dt);
> > > +	if (error < 0) {
> > > +		dev_err(&dev->dev, "unable to activate, err %d\n", error);
> > > +		return error;
> > > +	}
> > > +
> > > +	gp2a_report(dt);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void gp2a_device_close(struct input_dev *dev)
> > > +{
> > > +	struct gp2a_data *dt = input_get_drvdata(dev);
> > > +	int error;
> > > +
> > > +	error = gp2a_disable(dt);
> > > +	if (error < 0)
> > > +		dev_err(&dev->dev, "unable to deactivate, err %d\n", error);
> > > +}
> > > +
> > > +static int __devinit gp2a_probe(struct i2c_client *client,
> > > +				const struct i2c_device_id *id)
> > > +{
> > > +	const struct gp2a_platform_data *pdata;
> > > +	struct gp2a_data *dt;
> > > +	int error;
> > > +
> > > +	pdata = client->dev.platform_data;
> > > +	if (!pdata)
> > > +		return -EINVAL;
> > > +
> > > +	if (pdata->hw_setup) {
> > > +		error = pdata->hw_setup(client);
> > > +		if (error < 0)
> > > +			return error;
> > > +	}
> > > +
> > > +	error = gpio_direction_input(pdata->vout_gpio);
> > > +	if (error < 0)
> > > +		goto err_hw_shutdown;
> > > +
> > > +	dt = kzalloc(sizeof(struct gp2a_data), GFP_KERNEL);
> > > +	if (!dt) {
> > > +		error = -ENOMEM;
> > > +		goto err_hw_shutdown;
> > > +	}
> > > +
> > > +	dt->pdata = pdata;
> > > +	dt->i2c_client = client;
> > > +	i2c_set_clientdata(client, dt);
> > > +
> > > +	error = gp2a_initialize(dt);
> > > +	if (error < 0)
> > > +		goto err_free_dt;
> > > +
> > > +	dt->device = input_allocate_device();
> > > +	if (!dt->device) {
> > > +		error = -ENOMEM;
> > > +		goto err_free_dt;
> > > +	}
> > > +	input_set_drvdata(dt->device, dt);
> > > +
> > > +	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(&dt->device->dev, "irq request failed\n");
> > 
> > You are leaking dt->device here. Please add a separate label for
> > input_free_device(dt->device).
> > 
> > > +		goto err_free_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_capability(dt->device, EV_KEY, SW_FRONT_PROXIMITY);
> > 
> > Should be EV_SW instead of EV_KEY.
> > 
> > > +
> > > +	error = input_register_device(dt->device);
> > > +	if (error) {
> > > +		dev_err(&dt->device->dev, "device registration failed\n");
> > > +		input_free_device(dt->device);
> > 
> > If you swap request_threaded_irq() and input_register_device() so that
> > registration is the last action error handling will be much simpler.
> >
> 
> I am getting a bit confused here, since you asked me to swap the order
> of request_threaded_irq() and input_register_device() in my previous
> version as well. Swapping again will take us back to square one or maybe
> I am misinterpreting your comment?

Gah, sorry, a bit of boilerplate slipped in. You already have the calls
in right order, you just need proper labels.


	input_dev = input_allocate_device();
	if (!input_dev) {
		dev_err(...);
		error = -ENOMEM;
		goto err_free_mem;
	}

	... set up input device fully ...

	error = request_threaded_irq(...);
	if (error) {
		dev_err(...);
		goto err_free_input_dev;
	}

	error = input_register_device(input_dev);
	if (error) {
		dev_err(...);
		goto err_free_irq;
	}

	i2c_set_clientdata(client, dt);

	return 0;

err_free_irq:
	free_irq(client->irq, dt);
err_free_input_dev:
	input_free_device(input_dev);
err_free_mem:
	kfree(dt);
err_hw_shutdown:
	...
	return error;
}

-- 
Dmitry

Re: [PATCH v2] input: add driver support for Sharp gp2ap002a00f proximity sensor

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1446 bytes --]

Hi,

On Tue, Nov 15, 2011 at 12:29:26AM -0800, Dmitry Torokhov wrote:
> > > > +	error = input_register_device(dt->device);
> > > > +	if (error) {
> > > > +		dev_err(&dt->device->dev, "device registration failed\n");
> > > > +		input_free_device(dt->device);
> > > 
> > > If you swap request_threaded_irq() and input_register_device() so that
> > > registration is the last action error handling will be much simpler.
> > >
> > 
> > I am getting a bit confused here, since you asked me to swap the order
> > of request_threaded_irq() and input_register_device() in my previous
> > version as well. Swapping again will take us back to square one or maybe
> > I am misinterpreting your comment?
> 
> Gah, sorry, a bit of boilerplate slipped in. You already have the calls
> in right order, you just need proper labels.
> 
> 
> 	input_dev = input_allocate_device();
> 	if (!input_dev) {
> 		dev_err(...);
> 		error = -ENOMEM;
> 		goto err_free_mem;
> 	}
> 
> 	... set up input device fully ...
> 
> 	error = request_threaded_irq(...);
> 	if (error) {
> 		dev_err(...);
> 		goto err_free_input_dev;
> 	}
> 
> 	error = input_register_device(input_dev);
> 	if (error) {
> 		dev_err(...);
> 		goto err_free_irq;
> 	}

are you sure this is right order ? Won't this create a very small
timeframe where we could try to call input_report_switch() and
input_sync() on an unregistered input device ??

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] input: add driver support for Sharp gp2ap002a00f proximity sensor

[Dmitry Torokhov]

Hi Felipe,

On Tue, Nov 15, 2011 at 10:34:55AM +0200, Felipe Balbi wrote:
> Hi,
> 
> On Tue, Nov 15, 2011 at 12:29:26AM -0800, Dmitry Torokhov wrote:
> > > > > +	error = input_register_device(dt->device);
> > > > > +	if (error) {
> > > > > +		dev_err(&dt->device->dev, "device registration failed\n");
> > > > > +		input_free_device(dt->device);
> > > > 
> > > > If you swap request_threaded_irq() and input_register_device() so that
> > > > registration is the last action error handling will be much simpler.
> > > >
> > > 
> > > I am getting a bit confused here, since you asked me to swap the order
> > > of request_threaded_irq() and input_register_device() in my previous
> > > version as well. Swapping again will take us back to square one or maybe
> > > I am misinterpreting your comment?
> > 
> > Gah, sorry, a bit of boilerplate slipped in. You already have the calls
> > in right order, you just need proper labels.
> > 
> > 
> > 	input_dev = input_allocate_device();
> > 	if (!input_dev) {
> > 		dev_err(...);
> > 		error = -ENOMEM;
> > 		goto err_free_mem;
> > 	}
> > 
> > 	... set up input device fully ...
> > 
> > 	error = request_threaded_irq(...);
> > 	if (error) {
> > 		dev_err(...);
> > 		goto err_free_input_dev;
> > 	}
> > 
> > 	error = input_register_device(input_dev);
> > 	if (error) {
> > 		dev_err(...);
> > 		goto err_free_irq;
> > 	}
> 
> are you sure this is right order ? Won't this create a very small
> timeframe where we could try to call input_report_switch() and
> input_sync() on an unregistered input device ??
> 

Yes, this is fine. As long as input device was allocated with
input_allocate_device() it can be used by input_report_*() and
input_sync() even tough device may not be registered yet - the events
will simply be dropped.

Thanks.

-- 
Dmitry

Re: [PATCH v2] input: add driver support for Sharp gp2ap002a00f proximity sensor

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1953 bytes --]

Hi,

On Tue, Nov 15, 2011 at 12:46:09AM -0800, Dmitry Torokhov wrote:
> > On Tue, Nov 15, 2011 at 12:29:26AM -0800, Dmitry Torokhov wrote:
> > > > > > +	error = input_register_device(dt->device);
> > > > > > +	if (error) {
> > > > > > +		dev_err(&dt->device->dev, "device registration failed\n");
> > > > > > +		input_free_device(dt->device);
> > > > > 
> > > > > If you swap request_threaded_irq() and input_register_device() so that
> > > > > registration is the last action error handling will be much simpler.
> > > > >
> > > > 
> > > > I am getting a bit confused here, since you asked me to swap the order
> > > > of request_threaded_irq() and input_register_device() in my previous
> > > > version as well. Swapping again will take us back to square one or maybe
> > > > I am misinterpreting your comment?
> > > 
> > > Gah, sorry, a bit of boilerplate slipped in. You already have the calls
> > > in right order, you just need proper labels.
> > > 
> > > 
> > > 	input_dev = input_allocate_device();
> > > 	if (!input_dev) {
> > > 		dev_err(...);
> > > 		error = -ENOMEM;
> > > 		goto err_free_mem;
> > > 	}
> > > 
> > > 	... set up input device fully ...
> > > 
> > > 	error = request_threaded_irq(...);
> > > 	if (error) {
> > > 		dev_err(...);
> > > 		goto err_free_input_dev;
> > > 	}
> > > 
> > > 	error = input_register_device(input_dev);
> > > 	if (error) {
> > > 		dev_err(...);
> > > 		goto err_free_irq;
> > > 	}
> > 
> > are you sure this is right order ? Won't this create a very small
> > timeframe where we could try to call input_report_switch() and
> > input_sync() on an unregistered input device ??
> > 
> 
> Yes, this is fine. As long as input device was allocated with
> input_allocate_device() it can be used by input_report_*() and
> input_sync() even tough device may not be registered yet - the events
> will simply be dropped.

good to know, thanks :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] input: add driver support for Sharp gp2ap002a00f proximity sensor

[anish kumar]

On Tue, 2011-11-15 at 08:34 +0100, oskar.andero@sonyericsson.com wrote:
> Hi Dmitry,
> 
> On 18:03 Mon 14 Nov     , Dmitry Torokhov wrote:
> > Hi Courtney,
> > 
> > On Mon, Nov 14, 2011 at 04:39:18PM +0100, oskar.andero@sonyericsson.com wrote:
> > > From: Courtney Cavin <courtney.cavin@sonyericsson.com>
> > > 
> > 
> > The driver looks much better, a few more comments still.
> > 
> > > +
> > > +#include <linux/i2c.h>
> > > +#include <linux/irq.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/input.h>
> > > +#include <linux/module.h>
> > > +#include <linux/workqueue.h>
> > 
> > As Shubhrajyoti mentioned workqueue.h is probably not needed.
> > 
> > > +#include <linux/interrupt.h>
> > > +#include <linux/gpio.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/input/gp2ap002a00f.h>
> > > +
> > > +struct gp2a_data {
> > > +	struct input_dev *device;
> > > +	const struct gp2a_platform_data *pdata;
> > > +	struct i2c_client *i2c_client;
> > > +};
> > > +
> > > +enum gp2a_addr {
> > > +	GP2A_ADDR_PROX	= 0x0,
> > > +	GP2A_ADDR_GAIN	= 0x1,
> > > +	GP2A_ADDR_HYS	= 0x2,
> > > +	GP2A_ADDR_CYCLE	= 0x3,
> > > +	GP2A_ADDR_OPMOD	= 0x4,
> > > +	GP2A_ADDR_CON	= 0x6
> > > +};
> > > +
> > > +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_key(dt->device, SW_FRONT_PROXIMITY, !vo);
> > 
> > Switch is not a key, so please use input_report_switch().
> 
> Ok. Agreed.
> 
> > > +	input_sync(dt->device);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static irqreturn_t gp2a_irq(int irq, void *handle)
> > > +{
> > > +	struct gp2a_data *dt = handle;
> > > +
> > > +	gp2a_report(dt);
> > > +
> > > +	return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int gp2a_device_open(struct input_dev *dev)
> > > +{
> > > +	struct gp2a_data *dt = input_get_drvdata(dev);
> > > +	int error;
> > > +
> > > +	error = gp2a_enable(dt);
> > > +	if (error < 0) {
> > > +		dev_err(&dev->dev, "unable to activate, err %d\n", error);
> > > +		return error;
> > > +	}
> > > +
> > > +	gp2a_report(dt);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void gp2a_device_close(struct input_dev *dev)
> > > +{
> > > +	struct gp2a_data *dt = input_get_drvdata(dev);
> > > +	int error;
> > > +
> > > +	error = gp2a_disable(dt);
> > > +	if (error < 0)
> > > +		dev_err(&dev->dev, "unable to deactivate, err %d\n", error);
> > > +}
> > > +
> > > +static int __devinit gp2a_probe(struct i2c_client *client,
> > > +				const struct i2c_device_id *id)
> > > +{
> > > +	const struct gp2a_platform_data *pdata;
> > > +	struct gp2a_data *dt;
> > > +	int error;
> > > +
> > > +	pdata = client->dev.platform_data;
> > > +	if (!pdata)
> > > +		return -EINVAL;
> > > +
> > > +	if (pdata->hw_setup) {
> > > +		error = pdata->hw_setup(client);
> > > +		if (error < 0)
> > > +			return error;
> > > +	}
> > > +
> > > +	error = gpio_direction_input(pdata->vout_gpio);
> > > +	if (error < 0)
> > > +		goto err_hw_shutdown;
> > > +
> > > +	dt = kzalloc(sizeof(struct gp2a_data), GFP_KERNEL);
> > > +	if (!dt) {
> > > +		error = -ENOMEM;
> > > +		goto err_hw_shutdown;
> > > +	}
> > > +
> > > +	dt->pdata = pdata;
> > > +	dt->i2c_client = client;
> > > +	i2c_set_clientdata(client, dt);
> > > +
> > > +	error = gp2a_initialize(dt);
> > > +	if (error < 0)
> > > +		goto err_free_dt;
> > > +
> > > +	dt->device = input_allocate_device();
> > > +	if (!dt->device) {
> > > +		error = -ENOMEM;
> > > +		goto err_free_dt;
> > > +	}
> > > +	input_set_drvdata(dt->device, dt);
> > > +
> > > +	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(&dt->device->dev, "irq request failed\n");
> > 
> > You are leaking dt->device here. Please add a separate label for
> > input_free_device(dt->device).
> 
> Yes, you are right.
> 
> > > +		goto err_free_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_capability(dt->device, EV_KEY, SW_FRONT_PROXIMITY);
> > 
> > Should be EV_SW instead of EV_KEY.
> > 
> > > +
> > > +	error = input_register_device(dt->device);
> > > +	if (error) {
> > > +		dev_err(&dt->device->dev, "device registration failed\n");
> > > +		input_free_device(dt->device);
> > 
> > If you swap request_threaded_irq() and input_register_device() so that
> > registration is the last action error handling will be much simpler.
> 
> Sure. I'll look in to it.
> 
> > > +		goto err_free_irq;
> > > +	}
> > > +
> > > +	device_init_wakeup(&client->dev, pdata->wakeup);
> > > +
> > > +	return 0;
> > > +
> > > +err_free_irq:
> > > +	free_irq(client->irq, dt);
> > > +err_free_dt:
> > > +	kfree(dt);
> > > +err_hw_shutdown:
> > > +	if (pdata->hw_shutdown)
> > > +		pdata->hw_shutdown(client);
> > > +	return error;
> > > +}
> > > +
> > > +static int __devexit gp2a_remove(struct i2c_client *client)
> > > +{
> > > +	struct gp2a_data *dt = i2c_get_clientdata(client);
> > > +
> > > +	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);
> > > +	kfree(dt);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +#ifdef CONFIG_PM_SLEEP
> > > +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;
> > > +
> > > +	if (device_may_wakeup(&client->dev)) {
> > > +		enable_irq_wake(client->irq);
> > > +	} else {
> > 
> > This needs locking WRT open/close. Please acquire dt->device->mutex and
> > only disable if dt->device->users != 0. Similar shoudl be done for
> > resume.
> 
> I see what you mean. I will fix it. 
Is my understanding correct?You are going to disable the device 
only when there are users of the driver and not disable the device
otherwise.As anyway if there are no users the driver would have been
already disabled right?
> 
> > > +		error = gp2a_disable(dt);
> > > +		if (error < 0)
> > > +			return error;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +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;
> > > +
> > > +	if (device_may_wakeup(&client->dev)) {
> > > +		disable_irq_wake(client->irq);
> > > +	} else {
> > > +		error = gp2a_enable(dt);
> > > +		if (error < 0)
> > > +			return error;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +#endif
> > > +
> > > +static SIMPLE_DEV_PM_OPS(gp2a_pm, gp2a_suspend, gp2a_resume);
> > > +
> > > +static const struct i2c_device_id gp2a_i2c_id[] = {
> > > +	{ GP2A_I2C_NAME, 0 },
> > > +	{ }
> > > +};
> > > +
> > > +static struct i2c_driver gp2a_i2c_driver = {
> > > +	.driver = {
> > > +		.name	= GP2A_I2C_NAME,
> > > +		.owner	= THIS_MODULE,
> > > +		.pm	= &gp2a_pm
> > > +	},
> > > +	.probe		= gp2a_probe,
> > > +	.remove		= __devexit_p(gp2a_remove),
> > > +	.id_table	= gp2a_i2c_id
> > > +};
> > > +
> > > +static int __init gp2a_init(void)
> > > +{
> > > +	return i2c_add_driver(&gp2a_i2c_driver);
> > > +}
> > > +
> > > +static void __exit gp2a_exit(void)
> > > +{
> > > +	i2c_del_driver(&gp2a_i2c_driver);
> > > +}
> > > +
> > > +module_init(gp2a_init);
> > > +module_exit(gp2a_exit);
> > > +
> > > +MODULE_AUTHOR("Courtney Cavin <courtney.cavin@sonyericsson.com>");
> > > +MODULE_DESCRIPTION("Sharp GP2AP002A00F I2C Proximity/Opto sensor driver");
> > > +MODULE_LICENSE("GPL v2");
> > > diff --git a/include/linux/input/gp2ap002a00f.h b/include/linux/input/gp2ap002a00f.h
> > > new file mode 100644
> > > index 0000000..aad2fd4
> > > --- /dev/null
> > > +++ b/include/linux/input/gp2ap002a00f.h
> > > @@ -0,0 +1,22 @@
> > > +#ifndef _GP2AP002A00F_H_
> > > +#define _GP2AP002A00F_H_
> > > +
> > > +#include <linux/i2c.h>
> > > +
> > > +#define GP2A_I2C_NAME "gp2ap002a00f"
> > > +
> > > +/**
> > > + * struct gp2a_platform_data - Sharp gp2ap002a00f proximity platform data
> > > + * @vout_gpio: The gpio connected to the object detected pin (VOUT)
> > > + * @wakeup: Set to true if the proximity can wake the device from suspend
> > > + * @hw_setup: Callback for setting up hardware such as gpios and vregs
> > > + * @hw_shutdown: Callback for properly shutting down hardware
> > > + */
> > > +struct gp2a_platform_data {
> > > +	int vout_gpio;
> > > +	bool wakeup;
> > > +	int (*hw_setup)(struct i2c_client *client);
> > > +	int (*hw_shutdown)(struct i2c_client *client);
> > > +};
> > > +
> > > +#endif
> > > -- 
> > > 1.7.7
> > > 
> 
> Thanks for your comments! I'll prepare v3 later today.
> 
> -Oskar
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] input: add driver support for Sharp gp2ap002a00f proximity sensor

[oskar.andero]

> > > > +#ifdef CONFIG_PM_SLEEP
> > > > +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;
> > > > +
> > > > +	if (device_may_wakeup(&client->dev)) {
> > > > +		enable_irq_wake(client->irq);
> > > > +	} else {
> > > 
> > > This needs locking WRT open/close. Please acquire dt->device->mutex and
> > > only disable if dt->device->users != 0. Similar shoudl be done for
> > > resume.
> > 
> > I see what you mean. I will fix it. 
> Is my understanding correct?You are going to disable the device 
> only when there are users of the driver and not disable the device
> otherwise.As anyway if there are no users the driver would have been
> already disabled right?

Please, see PATCH v3 for the answer to your question.

-Oskar

Re: [PATCH v2] input: add driver support for Sharp gp2ap002a00f proximity sensor

[anish kumar]

On Wed, 2011-11-16 at 16:56 +0100, oskar.andero@sonyericsson.com wrote:
> > > > > +#ifdef CONFIG_PM_SLEEP
> > > > > +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;
> > > > > +
> > > > > +	if (device_may_wakeup(&client->dev)) {
> > > >  > +		enable_irq_wake(client->irq);
> > > > > +	} else {
> > > > 
> > > > This needs locking WRT open/close. Please acquire dt->device->mutex and
> > > > only disable if dt->device->users != 0. Similar shoudl be done for
> > > > resume.
> > > 
> > > I see what you mean. I will fix it. 
> > Is my understanding correct?You are going to disable the device 
> > only when there are users of the driver and not disable the device
> > otherwise.As anyway if there are no users the driver would have been
> > already disabled right?
> 
> Please, see PATCH v3 for the answer to your question.

Perhaps I didn't explain what I wanted to know.Can you kindly let me
know explain below statement by dmitry:
"This needs locking WRT open/close. Please acquire dt->device->mutex and
only disable if dt->device->users != 0. Similar shoudl be done for
resume"

Re: [PATCH v2] input: add driver support for Sharp gp2ap002a00f proximity sensor

[Dmitry Torokhov]

On Thu, Nov 17, 2011 at 12:39:12AM +0900, anish kumar wrote:
> On Tue, 2011-11-15 at 08:34 +0100, oskar.andero@sonyericsson.com wrote:
> > > > +#ifdef CONFIG_PM_SLEEP
> > > > +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;
> > > > +
> > > > +	if (device_may_wakeup(&client->dev)) {
> > > > +		enable_irq_wake(client->irq);
> > > > +	} else {
> > > 
> > > This needs locking WRT open/close. Please acquire dt->device->mutex and
> > > only disable if dt->device->users != 0. Similar shoudl be done for
> > > resume.
> > 
> > I see what you mean. I will fix it. 
> Is my understanding correct?You are going to disable the device 
> only when there are users of the driver and not disable the device
> otherwise.As anyway if there are no users the driver would have been
> already disabled right?

Right, if there are no usrs device should already be in low power mode
and we do not need anything to do here. Additionally, if device has no
users we do not wantt o power it on in esume() as this will only cause
us to waste power.

Thanks.

-- 
Dmitry