Hi Christian,
On Thu, Jan 30, 2014 at 10:29:45AM +0100, Christian Gmeiner wrote:
> This driver is quite simple and only supports the Touch
> Reporting Protocol.
Pretty clean and neat, just a few comments.
>
> Signed-off-by: Christian Gmeiner <christian.gmeiner@xxxxxxxxx>
> ---
> drivers/input/touchscreen/Kconfig | 12 ++
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/ar1021_i2c.c | 201 ++++++++++++++++++++++++++++++++
> 3 files changed, 214 insertions(+)
> create mode 100644 drivers/input/touchscreen/ar1021_i2c.c
>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 07e9e82..15cc9a1 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -86,6 +86,18 @@ config TOUCHSCREEN_AD7879_SPI
> To compile this driver as a module, choose M here: the
> module will be called ad7879-spi.
>
> +config TOUCHSCREEN_AR1021_I2C
> + tristate "Microchip AR1021 i2c touchscreen"
> + depends on I2C && OF
> + help
> + Say Y here if you have the Microchip AR1021 touchscreen controller
> + chip in your system.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called microchip_ar1021_i2c.
s/microchip_ar1021_i2c/ar1021_i2c
> +
> config TOUCHSCREEN_ATMEL_MXT
> tristate "Atmel mXT I2C Touchscreen"
> depends on I2C
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 62801f2..efaa328 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_TOUCHSCREEN_AD7879) += ad7879.o
> obj-$(CONFIG_TOUCHSCREEN_AD7879_I2C) += ad7879-i2c.o
> obj-$(CONFIG_TOUCHSCREEN_AD7879_SPI) += ad7879-spi.o
> obj-$(CONFIG_TOUCHSCREEN_ADS7846) += ads7846.o
> +obj-$(CONFIG_TOUCHSCREEN_AR1021_I2C) += ar1021_i2c.o
> obj-$(CONFIG_TOUCHSCREEN_ATMEL_MXT) += atmel_mxt_ts.o
> obj-$(CONFIG_TOUCHSCREEN_ATMEL_TSADCC) += atmel_tsadcc.o
> obj-$(CONFIG_TOUCHSCREEN_AUO_PIXCIR) += auo-pixcir-ts.o
> diff --git a/drivers/input/touchscreen/ar1021_i2c.c b/drivers/input/touchscreen/ar1021_i2c.c
> new file mode 100644
> index 0000000..c77ce05
> --- /dev/null
> +++ b/drivers/input/touchscreen/ar1021_i2c.c
> @@ -0,0 +1,201 @@
> +/*
> + * Microchip AR1021 driver for I2C
> + *
> + * Author: Christian Gmeiner <christian.gmeiner@xxxxxxxxx>
> + *
> + * License: GPL as published by the FSF.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/input.h>
> +#include <linux/of.h>
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/gpio.h>
> +#include <asm/unaligned.h>
> +
> +#define AR1021_TOCUH_PKG_SIZE 5
> +
> +struct ar1021_i2c {
> + struct i2c_client *client;
> + struct input_dev *input;
> + u8 data[AR1021_TOCUH_PKG_SIZE];
> +};
> +
> +static irqreturn_t ar1021_i2c_irq(int irq, void *dev_id)
> +{
> + struct ar1021_i2c *ar1021 = dev_id;
> + struct input_dev *input = ar1021->input;
> + u8 *data = ar1021->data;
> + unsigned int x, y, button;
> + int error;
> +
> + error = i2c_master_recv(ar1021->client,
> + ar1021->data, sizeof(ar1021->data));
> + if (error < 0)
> + goto out;
> +
> + button = !(data[0] & BIT(0));
> + x = ((data[2] & 0x1f) << 7) | (data[1] & 0x7f);
> + y = ((data[4] & 0x1f) << 7) | (data[3] & 0x7f);
> +
> + input_report_key(input, BTN_TOUCH, button);
> + input_report_abs(input, ABS_X, x);
> + input_report_abs(input, ABS_Y, y);
> + input_sync(input);
> +
> +out:
> + return IRQ_HANDLED;
> +}
> +
> +static int ar1021_i2c_open(struct input_dev *dev)
> +{
> + struct ar1021_i2c *wac_i2c = input_get_drvdata(dev);
> + struct i2c_client *client = wac_i2c->client;
> +
> + enable_irq(client->irq);
> +
> + return 0;
> +}
> +
> +static void ar1021_i2c_close(struct input_dev *dev)
> +{
> + struct ar1021_i2c *wac_i2c = input_get_drvdata(dev);
> + struct i2c_client *client = wac_i2c->client;
> +
> + disable_irq(client->irq);
> +}
> +
> +static int ar1021_i2c_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct ar1021_i2c *ar1021;
> + struct input_dev *input;
> + int error;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> + dev_err(&client->dev, "i2c_check_functionality error\n");
> + return -EIO;
> + }
> +
> + ar1021 = devm_kzalloc(client->dev, sizeof(*ar1021), GFP_KERNEL);
> + input = input_allocate_device();
Use devm_input_allocate_device() and later devm_request_threaded_irq()
as well.
> + if (!ar1021 || !input) {
> + error = -ENOMEM;
> + goto err_free_mem;
> + }
> +
> + ar1021->client = client;
> + ar1021->input = input;
> +
> + input->name = "ar1021 I2C Touchscreen";
> + input->id.bustype = BUS_I2C;
> + input->dev.parent = &client->dev;
> + input->open = ar1021_i2c_open;
> + input->close = ar1021_i2c_close;
> +
> + input->evbit[0] |= BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> +
> + __set_bit(BTN_TOOL_PEN, input->keybit);
> +
> + input_set_abs_params(input, ABS_X, 0, 4095, 0, 0);
> + input_set_abs_params(input, ABS_Y, 0, 4095, 0, 0);
> +
> + input_set_drvdata(input, ar1021);
> +
> + error = request_threaded_irq(client->irq, NULL, ar1021_i2c_irq,
> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> + "ar1021_i2c", ar1021);
> + if (error) {
> + dev_err(&client->dev,
> + "Failed to enable IRQ, error: %d\n", error);
> + goto err_free_mem;
> + }
> +
> + /* Disable the IRQ, we'll enable it in wac_i2c_open() */
No, not in wac_i2c_open ;) It looks like you might want to update
copyright notice to mentioned what driver you used as a base...
> + disable_irq(client->irq);
> +
> + error = input_register_device(ar1021->input);
> + if (error) {
> + dev_err(&client->dev,
> + "Failed to register input device, error: %d\n", error);
> + goto err_free_irq;
> + }
> +
> + i2c_set_clientdata(client, ar1021);
> + return 0;
> +
> +err_free_irq:
> + free_irq(client->irq, ar1021);
> +err_free_mem:
> + input_free_device(input);
> +
> + return error;
> +}
> +
> +static int ar1021_i2c_remove(struct i2c_client *client)
> +{
> + struct ar1021_i2c *ar1021 = i2c_get_clientdata(client);
> +
> + free_irq(client->irq, ar1021);
> + input_unregister_device(ar1021->input);
> +
> + return 0;
> +}
If you use devm throughout you won't need ar1021_i2c_remove method at
all.
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int ar1021_i2c_suspend(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> +
> + disable_irq(client->irq);
> +
> + return 0;
> +}
> +
> +static int ar1021_i2c_resume(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> +
> + enable_irq(client->irq);
You do not want to enable IRQ if there are no users (nobody opened
device).
> +
> + return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(ar1021_i2c_pm, ar1021_i2c_suspend, ar1021_i2c_resume);
> +
> +static const struct i2c_device_id ar1021_i2c_id[] = {
> + { "MICROCHIP_AR1021_I2C", 0 },
> + { },
> +};
> +MODULE_DEVICE_TABLE(i2c, ar1021_i2c_id);
> +
> +#ifdef CONFIG_OF
> +static struct of_device_id ar1021_i2c_of_match[] = {
> + { .compatible = "mc,ar1021-i2c", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, ar1021_i2c_of_match);
> +#endif
> +
> +static struct i2c_driver ar1021_i2c_driver = {
> + .driver = {
> + .name = "ar1021_i2c",
> + .owner = THIS_MODULE,
> + .pm = &ar1021_i2c_pm,
> + .of_match_table = of_match_ptr(ar1021_i2c_of_match),
> + },
> +
> + .probe = ar1021_i2c_probe,
> + .remove = ar1021_i2c_remove,
> + .id_table = ar1021_i2c_id,
> +};
> +module_i2c_driver(ar1021_i2c_driver);
> +
> +MODULE_AUTHOR("Christian Gmeiner <christian.gmeiner@xxxxxxxxx>");
> +MODULE_DESCRIPTION("Microchip AR1021 I2C Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:ar1021_i2c");
Why platform if it is I2C driver? This MODULE_ALIAS is not needed at
all.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
