From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752873AbdCMN4U convert rfc822-to-8bit (ORCPT ); Mon, 13 Mar 2017 09:56:20 -0400 Received: from mout.kundenserver.de ([217.72.192.74]:62414 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751934AbdCMN4M (ORCPT ); Mon, 13 Mar 2017 09:56:12 -0400 From: "Vogelaar, Patrick" To: Dmitry Torokhov CC: "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-input@vger.kernel.org" , Dmitry Torokhov Subject: AW: [PATCH v2 1/2] add driver for cypress cy8cmbr3102 Thread-Topic: [PATCH v2 1/2] add driver for cypress cy8cmbr3102 Thread-Index: AQHSjA1mG+WegE4xyESn+ijyVeEWEqGO1/6AgAQRMHA= Date: Mon, 13 Mar 2017 13:55:56 +0000 Message-ID: <87C4E377863DEB4690D8443B34EE975DF40627@M-EX1.GT.local> References: <1487659221-14665-1-git-send-email-Patrick.Vogelaar@gigatronik.com> <1487659221-14665-2-git-send-email-Patrick.Vogelaar@gigatronik.com> <20170311004259.GC7384@dtor-ws> In-Reply-To: <20170311004259.GC7384@dtor-ws> Accept-Language: de-DE, en-US Content-Language: de-DE X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [192.168.208.38] x-kse-attachmentfiltering-interceptor-info: protection disabled x-kse-serverinfo: M-EX1.GT.local, 9 x-kse-antivirus-interceptor-info: scan successful x-kse-antivirus-info: Clean, bases: 13.03.2017 09:01:00 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-KSE-AntiSpam-Interceptor-Info: internally-submitted e-mail X-KSE-AttachmentFiltering-Interceptor-Info: protection disabled X-Provags-ID: V03:K0:JcRv4PvDZlCVVkNGT42pMUZ0csS5wOsi1yhqn2CTUkitqlDuXQR Lnb0yaYPKT7Dz9hDFg7jv04uKyks8Y+c9WhrCQ7y2kGd2l7kv+HlQsQZMpD/83Qm92OPV5C f7v1ptWCjIHi/3aS6M0wyIytbYnxG5kUqP/+zVKj6Ui5CiEexcjl4xPE79z78G/xI27feXZ LOuYFsQ9V0wIvrbTarMvQ== X-UI-Out-Filterresults: notjunk:1;V01:K0:C0uJDQI6ltI=:bT12JNejKw5q4Ks4clRy8W wdJOWW6ImjRiH7xGVSlo9g5amJ1EOl5Ve730tE81YeIh0gLp/FtD1ydOqB/4Lv5dgadz3UMuO XxyqWFHjh/gplEkO/5/YFxGgr+ghbEGyzLZm4XvNl7bt/+EBf5LP2MCrNbHyNw4jbUGrOK/q9 5XfJ7vLzVy3lbFcKU8kWuYvg9LlXvXKjM7Do/9Kd8Ks14mmOtuDCmP4e6f6IPNy1G8SYLKJA+ 6JHsAmDEzbUwfDoE3pbZgVmiEL8Of2nljjawL/S0B9aHH52yaoZ7OrDAnU3KEHeq5osgalERJ UV/z9nSxC8+qLjBaIizVW+GMilisQhRIaJ4c5dYgHE5dVN2Y2pnNWs01B/EghyO2hgEAxAMJC gB9fYIKOH+QCpbdmN14eufJBI7wC6n52lHRObvwo37xGo1wDuttc514T2xD9YrJ5ZXdoOvYVR M9coPrN4zudarCOWd8rdnVzbv+ytzvx7K0XzNGe++nYVkTZ2pAqyE7qBhlYn2/it6zNaVMjhT VUuV9OLhxRHp3N9lwGQJeqztzXgp9etWc42cgJRkqpzdIL7nATwZzgS9A0Qq1vETrWv+xPA3f eouf1c3eU0hQQ6C49gbFQXqtcMLbKtWz0o+M8pwTHv4nPAvsqwZ3NsolWmU1x26kB8fbht8My A4nBsM3vl/7eg4IorlbmXkHw7d/UlJ6K77MjpBjGaBNnCo426nLH3Jp5zAAQ7kNN4oZOI6Sv6 5NhxdsFakwzbAgQZ Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Dimtry, Thanks for the feedback. I will prepare a v3 of the patch with the things you pointed out fixed and a more meaningful description and changelog. I have a few questions regarding your comments (see below). >Hi Patrick, > >On Tue, Feb 21, 2017 at 07:40:20AM +0100, Patrick Vogelaar wrote: >> Version 2: >> * removed .owner >> * based on branch next > >A better changelog would be nice: the kid of device, limitations, etc. > >> >> Signed-off-by: Patrick Vogelaar >> --- >> drivers/input/misc/Kconfig | 12 +++ >> drivers/input/misc/Makefile | 1 + >> drivers/input/misc/cy8cmbr3102.c | 221 >> +++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 234 insertions(+) >> create mode 100644 drivers/input/misc/cy8cmbr3102.c >> >> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig >> index 5b6c522..be3071e 100644 >> --- a/drivers/input/misc/Kconfig >> +++ b/drivers/input/misc/Kconfig >> @@ -822,4 +822,16 @@ config INPUT_HISI_POWERKEY >> To compile this driver as a module, choose M here: the >> module will be called hisi_powerkey. >> >> +config INPUT_CY8CMBR3102 >> + tristate "Cypress CY8CMBR3102 CapSense Express controller" >> + depends on I2C >> + select INPUT_POLLDEV >> + default n > >"default n" can be omitted. > >> + help >> + Say yes here to enable support for the Cypress CY8CMBR3102 >> + CapSense Express controller. >> + >> + To compile this driver as a module, choose M here: the module >> + will be called cy8cmbr3102. >> + >> endif >> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile >> index b10523f..f9959ed 100644 >> --- a/drivers/input/misc/Makefile >> +++ b/drivers/input/misc/Makefile >> @@ -77,3 +77,4 @@ obj-$(CONFIG_INPUT_WM831X_ON) += >wm831x-on.o >> obj-$(CONFIG_INPUT_XEN_KBDDEV_FRONTEND) += xen-kbdfront.o >> obj-$(CONFIG_INPUT_YEALINK) += yealink.o >> obj-$(CONFIG_INPUT_IDEAPAD_SLIDEBAR) += ideapad_slidebar.o >> +obj-$(CONFIG_INPUT_CY8CMBR3102) += cy8cmbr3102.o >> diff --git a/drivers/input/misc/cy8cmbr3102.c >> b/drivers/input/misc/cy8cmbr3102.c >> new file mode 100644 >> index 0000000..ed67a63 >> --- /dev/null >> +++ b/drivers/input/misc/cy8cmbr3102.c >> @@ -0,0 +1,221 @@ >> +/* Driver for Cypress CY8CMBR3102 CapSense Express Controller >> + * >> + * (C) 2017 by Gigatronik Technologies GmbH >> + * Author: Patrick Vogelaar >> + * All rights reserved. >> + * >> + * This code is based on mma8450.c and atmel_captouch.c. >> + * >> + * This program is free software; you can redistribute it and/or >> +modify >> + * it under the terms of the GNU General Public License as published >> +by >> + * the Free Software Foundation; version 2 of the License. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * NOTE: This implementation does not implement the full range of >> +functions the >> + * Cypress CY8CMBR3102 CapSense Express controller provides. It only >> +implements >> + * its use for connected touch buttons (yet). >> + */ >> + >> +#define DRV_VERSION "0.1" >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +/* I2C Registers */ >> +#define CY8CMBR3102_DEVICE_ID_REG 0x90 >> +#define CY8CMBR3102_BUTTON_STAT 0xAA >> + >> + >> +#define CY8CMBR3102_MAX_NUM_OF_BUTTONS 0x02 >> +#define CY8CMBR3102_DRV_NAME "cy8cmbr3102" >> +#define CY8CMBR3102_POLL_INTERVAL 200 >> +#define CY8CMBR3102_POLL_INTERVAL_MAX 300 >> +#define CY8CMBR3102_DEVICE_ID 2561 >> +#define CY8CMBR3102_MAX_RETRY 5 >> + >> +/* >> + * @i2c_client: I2C slave device client pointer >> + * @idev: Input (polled) device pointer > >This kind of comments are not really helpful and just clutter the source. >Anyone looking at the structure can see that they are dealing with i2c client >pointer and polled input device pointer. > >> + * @num_btn: Number of buttons >> + * @keycodes: map of button# to KeyCode >> + * @cy8cmbr3102_lock: mutex lock >> + */ >> +struct cy8cmbr3102_device { >> + struct i2c_client *client; >> + struct input_polled_dev *idev; >> + u32 num_btn; >> + u32 keycodes[CY8CMBR3102_MAX_NUM_OF_BUTTONS]; >> + struct mutex cy8cmbr3102_lock; > >I do not think you need this mutex: we will not run several instances of poll >function simultaneously. > >> +}; >> + >> +static const struct i2c_device_id cy8cmbr3102_idtable[] = { >> + {"cy8cmbr3102", 0}, >> + {} > >Extra indent. > >> +}; >> +MODULE_DEVICE_TABLE(i2c, cy8cmbr3102_idtable); > >Please move to the driver structure. > >> + >> +static void cy8cmbr3102_poll(struct input_polled_dev *idev) { >> + struct cy8cmbr3102_device *dev = idev->private; >> + int i, ret, btn_state; >> + >> + mutex_lock(&dev->cy8cmbr3102_lock); >> + >> + ret = i2c_smbus_read_word_data(dev->client, >CY8CMBR3102_BUTTON_STAT); >> + if (ret < 0) { >> + dev_err(&dev->client->dev, "i2c io error: %d\n", ret); >> + mutex_unlock(&dev->cy8cmbr3102_lock); >> + return; >> + } >> + >> + for (i = 0; i < dev->num_btn; i++) { >> + btn_state = ret & (0x1 << i); >> + input_report_key(idev->input, dev->keycodes[i], btn_state); > > input_report_key(idev->input, dev->keycodes[i], ret & BIT(i)); > >> + input_sync(idev->input); >> + } >> + >> + mutex_unlock(&dev->cy8cmbr3102_lock); >> +} >> + >> +static int cy8cmbr3102_remove(struct i2c_client *client) { >> + struct cy8cmbr3102_device *dev = i2c_get_clientdata(client); >> + struct input_polled_dev *idev = dev->idev; >> + >> + dev_dbg(&client->dev, "%s\n", __func__); >> + >> + mutex_destroy(&dev->cy8cmbr3102_lock); >> + input_unregister_polled_device(idev); > >Not needed with devm. In fact, this whole function is not needed. >> + >> + return 0; >> +} >> + >> +static int cy8cmbr3102_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + struct cy8cmbr3102_device *drvdata; >> + struct device *dev = &client->dev; >> + struct device_node *node; >> + int i, err, ret; >> + >> + dev_dbg(&client->dev, "%s\n", __func__); >> + >> + if (!i2c_check_functionality(client->adapter, >I2C_FUNC_SMBUS_BYTE_DATA | >> + > I2C_FUNC_SMBUS_WORD_DATA | >> + > I2C_FUNC_SMBUS_I2C_BLOCK)) { >> + dev_err(dev, "needed i2c functionality is not supported\n"); >> + return -EINVAL; >> + } >> + >> + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); >> + if (!drvdata) >> + return -ENOMEM; >> + >> + drvdata->client = client; >> + i2c_set_clientdata(client, drvdata); >> + >> + /* device is in low-power mode and needs to be waken up */ >> + for (i = 0; (i < CY8CMBR3102_MAX_RETRY) && >> + (ret != CY8CMBR3102_DEVICE_ID); i++) >{ >> + ret = i2c_smbus_read_word_data(drvdata->client, >> + CY8CMBR3102_DEVICE_ID_REG); > >Weird indent. > >> + dev_dbg(dev, "DEVICE_ID (%i): %i\n", i, ret); >> + } >> + >> + if (ret < 0) { >> + dev_err(&client->dev, "i2c io error: %d\n", ret); >> + return -EIO; > > return ret; > >> + } else if (ret != CY8CMBR3102_DEVICE_ID) { >> + dev_err(&client->dev, "read device ID %i is not equal to >%i!\n", >> + ret, CY8CMBR3102_DEVICE_ID); >> + return -ENXIO; >> + } >> + dev_dbg(dev, "device identified by device ID\n"); >> + >> + drvdata->idev = devm_input_allocate_polled_device(dev); >> + if (!drvdata->idev) { >> + dev_err(dev, "failed to allocate input device\n"); >> + return -ENOMEM; >> + } >> + Is it still necessary to check if a of_node is available if I use the generic device properties. I tried to find a more generic way for this but couldn't find anything >> + node = dev->of_node; >> + if (!node) { >> + dev_err(dev, "failed to find matching node in device tree\n"); >> + return -EINVAL; >> + } > >Please drop. > >> + >> + if (of_property_read_bool(node, "autorepeat")) > >Use generic device properties: > > if (device_property_read_bool(...)) > >> + __set_bit(EV_REP, drvdata->idev->input->evbit); >> + >> + drvdata->num_btn = of_property_count_u32_elems(node, >> +"linux,keycodes"); > > size = device_property_read_u32_array(dev, "linux,keycodes", NULL, >0); > ... etc. > >Se matrix-keymap.c for parsing. > > >> + if (drvdata->num_btn > CY8CMBR3102_MAX_NUM_OF_BUTTONS) >> + drvdata->num_btn = >CY8CMBR3102_MAX_NUM_OF_BUTTONS; >> + >> + err = of_property_read_u32_array(node, "linux,keycodes", >> + drvdata->keycodes, drvdata->num_btn); >> + >> + if (err) { >> + dev_err(dev, "failed to read linux,keycodes property: %d\n", >> + err); >> + return err; >> + } >> + >> + for (i = 0; i < drvdata->num_btn; i++) >> + __set_bit(drvdata->keycodes[i], drvdata->idev->input- >>keybit); >> + >> + drvdata->idev->input->id.bustype = BUS_I2C; >> + drvdata->idev->input->id.product = 0x3102; >> + drvdata->idev->input->id.version = 0; >> + drvdata->idev->input->name = CY8CMBR3102_DRV_NAME; >> + drvdata->idev->poll = cy8cmbr3102_poll; >> + drvdata->idev->poll_interval = CY8CMBR3102_POLL_INTERVAL; >> + drvdata->idev->poll_interval_max = >CY8CMBR3102_POLL_INTERVAL_MAX; >> + drvdata->idev->private = drvdata; >> + drvdata->idev->input->keycode = drvdata->keycodes; >> + drvdata->idev->input->keycodemax = drvdata->num_btn; >> + drvdata->idev->input->keycodesize = sizeof(drvdata->keycodes[0]); >> + __set_bit(EV_KEY, drvdata->idev->input->evbit); >> + >> + mutex_init(&drvdata->cy8cmbr3102_lock); >> + >> + err = input_register_polled_device(drvdata->idev); >> + if (err) >> + return err; >> + >> + dev_info(&client->dev, "chip found, driver version " DRV_VERSION >> +"\n"); > What exactly should I drop here? The return 0? If so, could you please explain to me why. Thanks >Please drop. > >> + return 0; >> +} >> + >> +#ifdef CONFIG_OF >> +static const struct of_device_id of_cy8cmbr3102_match[] = { >> + {.compatible = "cypress,cy8cmbr3102", }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, of_cy8cmbr3102_match); #endif >> + >> +static struct i2c_driver cy8cmbr3102_driver = { >> + .driver = { >> + .name = "cy8cmbr3102", >> + .of_match_table = >of_match_ptr(of_cy8cmbr3102_match), >> + }, >> + .probe = cy8cmbr3102_probe, >> + .remove = cy8cmbr3102_remove, >> + .id_table = cy8cmbr3102_idtable, >> +}; >> +module_i2c_driver(cy8cmbr3102_driver); >> + >> +MODULE_AUTHOR("Patrick Vogelaar >"); >> +MODULE_DESCRIPTION("Cypress CY8CMBR3102 CapSense Express >> +controller"); MODULE_LICENSE("GPL"); >MODULE_VERSION(DRV_VERSION); >> -- >> 2.7.4 >> > >Thanks. > >-- >Dmitry Thanks Patrick From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Vogelaar, Patrick" Subject: AW: [PATCH v2 1/2] add driver for cypress cy8cmbr3102 Date: Mon, 13 Mar 2017 13:55:56 +0000 Message-ID: <87C4E377863DEB4690D8443B34EE975DF40627@M-EX1.GT.local> References: <1487659221-14665-1-git-send-email-Patrick.Vogelaar@gigatronik.com> <1487659221-14665-2-git-send-email-Patrick.Vogelaar@gigatronik.com> <20170311004259.GC7384@dtor-ws> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT Return-path: In-Reply-To: <20170311004259.GC7384@dtor-ws> Content-Language: de-DE Sender: linux-input-owner@vger.kernel.org Cc: "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-input@vger.kernel.org" , Dmitry Torokhov List-Id: devicetree@vger.kernel.org Hi Dimtry, Thanks for the feedback. I will prepare a v3 of the patch with the things you pointed out fixed and a more meaningful description and changelog. I have a few questions regarding your comments (see below). >Hi Patrick, > >On Tue, Feb 21, 2017 at 07:40:20AM +0100, Patrick Vogelaar wrote: >> Version 2: >> * removed .owner >> * based on branch next > >A better changelog would be nice: the kid of device, limitations, etc. > >> >> Signed-off-by: Patrick Vogelaar >> --- >> drivers/input/misc/Kconfig | 12 +++ >> drivers/input/misc/Makefile | 1 + >> drivers/input/misc/cy8cmbr3102.c | 221 >> +++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 234 insertions(+) >> create mode 100644 drivers/input/misc/cy8cmbr3102.c >> >> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig >> index 5b6c522..be3071e 100644 >> --- a/drivers/input/misc/Kconfig >> +++ b/drivers/input/misc/Kconfig >> @@ -822,4 +822,16 @@ config INPUT_HISI_POWERKEY >> To compile this driver as a module, choose M here: the >> module will be called hisi_powerkey. >> >> +config INPUT_CY8CMBR3102 >> + tristate "Cypress CY8CMBR3102 CapSense Express controller" >> + depends on I2C >> + select INPUT_POLLDEV >> + default n > >"default n" can be omitted. > >> + help >> + Say yes here to enable support for the Cypress CY8CMBR3102 >> + CapSense Express controller. >> + >> + To compile this driver as a module, choose M here: the module >> + will be called cy8cmbr3102. >> + >> endif >> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile >> index b10523f..f9959ed 100644 >> --- a/drivers/input/misc/Makefile >> +++ b/drivers/input/misc/Makefile >> @@ -77,3 +77,4 @@ obj-$(CONFIG_INPUT_WM831X_ON) += >wm831x-on.o >> obj-$(CONFIG_INPUT_XEN_KBDDEV_FRONTEND) += xen-kbdfront.o >> obj-$(CONFIG_INPUT_YEALINK) += yealink.o >> obj-$(CONFIG_INPUT_IDEAPAD_SLIDEBAR) += ideapad_slidebar.o >> +obj-$(CONFIG_INPUT_CY8CMBR3102) += cy8cmbr3102.o >> diff --git a/drivers/input/misc/cy8cmbr3102.c >> b/drivers/input/misc/cy8cmbr3102.c >> new file mode 100644 >> index 0000000..ed67a63 >> --- /dev/null >> +++ b/drivers/input/misc/cy8cmbr3102.c >> @@ -0,0 +1,221 @@ >> +/* Driver for Cypress CY8CMBR3102 CapSense Express Controller >> + * >> + * (C) 2017 by Gigatronik Technologies GmbH >> + * Author: Patrick Vogelaar >> + * All rights reserved. >> + * >> + * This code is based on mma8450.c and atmel_captouch.c. >> + * >> + * This program is free software; you can redistribute it and/or >> +modify >> + * it under the terms of the GNU General Public License as published >> +by >> + * the Free Software Foundation; version 2 of the License. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * NOTE: This implementation does not implement the full range of >> +functions the >> + * Cypress CY8CMBR3102 CapSense Express controller provides. It only >> +implements >> + * its use for connected touch buttons (yet). >> + */ >> + >> +#define DRV_VERSION "0.1" >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +/* I2C Registers */ >> +#define CY8CMBR3102_DEVICE_ID_REG 0x90 >> +#define CY8CMBR3102_BUTTON_STAT 0xAA >> + >> + >> +#define CY8CMBR3102_MAX_NUM_OF_BUTTONS 0x02 >> +#define CY8CMBR3102_DRV_NAME "cy8cmbr3102" >> +#define CY8CMBR3102_POLL_INTERVAL 200 >> +#define CY8CMBR3102_POLL_INTERVAL_MAX 300 >> +#define CY8CMBR3102_DEVICE_ID 2561 >> +#define CY8CMBR3102_MAX_RETRY 5 >> + >> +/* >> + * @i2c_client: I2C slave device client pointer >> + * @idev: Input (polled) device pointer > >This kind of comments are not really helpful and just clutter the source. >Anyone looking at the structure can see that they are dealing with i2c client >pointer and polled input device pointer. > >> + * @num_btn: Number of buttons >> + * @keycodes: map of button# to KeyCode >> + * @cy8cmbr3102_lock: mutex lock >> + */ >> +struct cy8cmbr3102_device { >> + struct i2c_client *client; >> + struct input_polled_dev *idev; >> + u32 num_btn; >> + u32 keycodes[CY8CMBR3102_MAX_NUM_OF_BUTTONS]; >> + struct mutex cy8cmbr3102_lock; > >I do not think you need this mutex: we will not run several instances of poll >function simultaneously. > >> +}; >> + >> +static const struct i2c_device_id cy8cmbr3102_idtable[] = { >> + {"cy8cmbr3102", 0}, >> + {} > >Extra indent. > >> +}; >> +MODULE_DEVICE_TABLE(i2c, cy8cmbr3102_idtable); > >Please move to the driver structure. > >> + >> +static void cy8cmbr3102_poll(struct input_polled_dev *idev) { >> + struct cy8cmbr3102_device *dev = idev->private; >> + int i, ret, btn_state; >> + >> + mutex_lock(&dev->cy8cmbr3102_lock); >> + >> + ret = i2c_smbus_read_word_data(dev->client, >CY8CMBR3102_BUTTON_STAT); >> + if (ret < 0) { >> + dev_err(&dev->client->dev, "i2c io error: %d\n", ret); >> + mutex_unlock(&dev->cy8cmbr3102_lock); >> + return; >> + } >> + >> + for (i = 0; i < dev->num_btn; i++) { >> + btn_state = ret & (0x1 << i); >> + input_report_key(idev->input, dev->keycodes[i], btn_state); > > input_report_key(idev->input, dev->keycodes[i], ret & BIT(i)); > >> + input_sync(idev->input); >> + } >> + >> + mutex_unlock(&dev->cy8cmbr3102_lock); >> +} >> + >> +static int cy8cmbr3102_remove(struct i2c_client *client) { >> + struct cy8cmbr3102_device *dev = i2c_get_clientdata(client); >> + struct input_polled_dev *idev = dev->idev; >> + >> + dev_dbg(&client->dev, "%s\n", __func__); >> + >> + mutex_destroy(&dev->cy8cmbr3102_lock); >> + input_unregister_polled_device(idev); > >Not needed with devm. In fact, this whole function is not needed. >> + >> + return 0; >> +} >> + >> +static int cy8cmbr3102_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + struct cy8cmbr3102_device *drvdata; >> + struct device *dev = &client->dev; >> + struct device_node *node; >> + int i, err, ret; >> + >> + dev_dbg(&client->dev, "%s\n", __func__); >> + >> + if (!i2c_check_functionality(client->adapter, >I2C_FUNC_SMBUS_BYTE_DATA | >> + > I2C_FUNC_SMBUS_WORD_DATA | >> + > I2C_FUNC_SMBUS_I2C_BLOCK)) { >> + dev_err(dev, "needed i2c functionality is not supported\n"); >> + return -EINVAL; >> + } >> + >> + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); >> + if (!drvdata) >> + return -ENOMEM; >> + >> + drvdata->client = client; >> + i2c_set_clientdata(client, drvdata); >> + >> + /* device is in low-power mode and needs to be waken up */ >> + for (i = 0; (i < CY8CMBR3102_MAX_RETRY) && >> + (ret != CY8CMBR3102_DEVICE_ID); i++) >{ >> + ret = i2c_smbus_read_word_data(drvdata->client, >> + CY8CMBR3102_DEVICE_ID_REG); > >Weird indent. > >> + dev_dbg(dev, "DEVICE_ID (%i): %i\n", i, ret); >> + } >> + >> + if (ret < 0) { >> + dev_err(&client->dev, "i2c io error: %d\n", ret); >> + return -EIO; > > return ret; > >> + } else if (ret != CY8CMBR3102_DEVICE_ID) { >> + dev_err(&client->dev, "read device ID %i is not equal to >%i!\n", >> + ret, CY8CMBR3102_DEVICE_ID); >> + return -ENXIO; >> + } >> + dev_dbg(dev, "device identified by device ID\n"); >> + >> + drvdata->idev = devm_input_allocate_polled_device(dev); >> + if (!drvdata->idev) { >> + dev_err(dev, "failed to allocate input device\n"); >> + return -ENOMEM; >> + } >> + Is it still necessary to check if a of_node is available if I use the generic device properties. I tried to find a more generic way for this but couldn't find anything >> + node = dev->of_node; >> + if (!node) { >> + dev_err(dev, "failed to find matching node in device tree\n"); >> + return -EINVAL; >> + } > >Please drop. > >> + >> + if (of_property_read_bool(node, "autorepeat")) > >Use generic device properties: > > if (device_property_read_bool(...)) > >> + __set_bit(EV_REP, drvdata->idev->input->evbit); >> + >> + drvdata->num_btn = of_property_count_u32_elems(node, >> +"linux,keycodes"); > > size = device_property_read_u32_array(dev, "linux,keycodes", NULL, >0); > ... etc. > >Se matrix-keymap.c for parsing. > > >> + if (drvdata->num_btn > CY8CMBR3102_MAX_NUM_OF_BUTTONS) >> + drvdata->num_btn = >CY8CMBR3102_MAX_NUM_OF_BUTTONS; >> + >> + err = of_property_read_u32_array(node, "linux,keycodes", >> + drvdata->keycodes, drvdata->num_btn); >> + >> + if (err) { >> + dev_err(dev, "failed to read linux,keycodes property: %d\n", >> + err); >> + return err; >> + } >> + >> + for (i = 0; i < drvdata->num_btn; i++) >> + __set_bit(drvdata->keycodes[i], drvdata->idev->input- >>keybit); >> + >> + drvdata->idev->input->id.bustype = BUS_I2C; >> + drvdata->idev->input->id.product = 0x3102; >> + drvdata->idev->input->id.version = 0; >> + drvdata->idev->input->name = CY8CMBR3102_DRV_NAME; >> + drvdata->idev->poll = cy8cmbr3102_poll; >> + drvdata->idev->poll_interval = CY8CMBR3102_POLL_INTERVAL; >> + drvdata->idev->poll_interval_max = >CY8CMBR3102_POLL_INTERVAL_MAX; >> + drvdata->idev->private = drvdata; >> + drvdata->idev->input->keycode = drvdata->keycodes; >> + drvdata->idev->input->keycodemax = drvdata->num_btn; >> + drvdata->idev->input->keycodesize = sizeof(drvdata->keycodes[0]); >> + __set_bit(EV_KEY, drvdata->idev->input->evbit); >> + >> + mutex_init(&drvdata->cy8cmbr3102_lock); >> + >> + err = input_register_polled_device(drvdata->idev); >> + if (err) >> + return err; >> + >> + dev_info(&client->dev, "chip found, driver version " DRV_VERSION >> +"\n"); > What exactly should I drop here? The return 0? If so, could you please explain to me why. Thanks >Please drop. > >> + return 0; >> +} >> + >> +#ifdef CONFIG_OF >> +static const struct of_device_id of_cy8cmbr3102_match[] = { >> + {.compatible = "cypress,cy8cmbr3102", }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, of_cy8cmbr3102_match); #endif >> + >> +static struct i2c_driver cy8cmbr3102_driver = { >> + .driver = { >> + .name = "cy8cmbr3102", >> + .of_match_table = >of_match_ptr(of_cy8cmbr3102_match), >> + }, >> + .probe = cy8cmbr3102_probe, >> + .remove = cy8cmbr3102_remove, >> + .id_table = cy8cmbr3102_idtable, >> +}; >> +module_i2c_driver(cy8cmbr3102_driver); >> + >> +MODULE_AUTHOR("Patrick Vogelaar >"); >> +MODULE_DESCRIPTION("Cypress CY8CMBR3102 CapSense Express >> +controller"); MODULE_LICENSE("GPL"); >MODULE_VERSION(DRV_VERSION); >> -- >> 2.7.4 >> > >Thanks. > >-- >Dmitry Thanks Patrick