| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 13 Mar 2017 13:55:56 +0000
From: "Vogelaar, Patrick" <Patrick.Vogelaar@...atronik.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
Dmitry Torokhov <dmitry.torokhov@...il.com>
Subject: AW: [PATCH v2 1/2] add driver for cypress cy8cmbr3102
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 <Patrick.Vogelaar@...atronik.com>
>> ---
>> 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 <patrick.vogelaar@...atronik.com>
>> + * 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 <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/i2c.h>
>> +#include <linux/input.h>
>> +#include <linux/input-polldev.h>
>> +#include <linux/of.h>
>> +
>> +/* 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
><patrick.vogelaar@...atronik.com>");
>> +MODULE_DESCRIPTION("Cypress CY8CMBR3102 CapSense Express
>> +controller"); MODULE_LICENSE("GPL");
>MODULE_VERSION(DRV_VERSION);
>> --
>> 2.7.4
>>
>
>Thanks.
>
>--
>Dmitry
Thanks
Patrick
Powered by blists - more mailing lists