Re: [PATCH RFC] Input: Add Microchip AR1021 i2c touchscreen

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Dmitry.


> 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.
>

Thanks for the review.

>>
>> 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
>

ups

>> +
>>  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.
>

Thats a very good idea...

>> +     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...
>

Will do :)

>> +     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);


Do I need the i2c_set_clientdata(..) call at all?

>> +     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.
>

Correct.. will do it in the final patch.

>> +
>> +#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).
>

Okay.. but then I also do not need the disable_irq(..) call in
ar1021_i2c_suspend
and can totally remove the PM stuff - or?

>> +
>> +     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.

Ok

Thanks
--
Christian Gmeiner, MSc
--
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




[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux