| 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
| ||
|
Message-ID: <20150727213002.GE5144@dtor-ws>
Date: Mon, 27 Jul 2015 14:30:02 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Robert Dolca <robert.dolca@...il.com>
Cc: Robert Dolca <robert.dolca@...el.com>, linux-input@...r.kernel.org,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Henrik Rydberg <rydberg@...math.org>
Subject: Re: [PATCH] Add generic driver for Silead tochscreens
On Mon, Jul 20, 2015 at 03:05:44PM +0300, Robert Dolca wrote:
> Hi Dmitry,
>
> On Mon, Jul 20, 2015 at 9:51 AM, Dmitry Torokhov
> <dmitry.torokhov@...il.com> wrote:
> > On Fri, Jul 10, 2015 at 06:11:04PM +0300, Robert Dolca wrote:
> > > This driver adds support for Silead touchscreens. It has been tested
> > > with GSL1680 and GSL3680 touch panels.
> > >
> > > It supports ACPI and device tree enumeration. For ACPI you need ACPI 5.1+
> > > in order to be able to use named GPIOs. In device tree you can use 'interrupts'
> > > or a named GPIO for IRQ.
> > >
> > > Screen resolution, the maximum number of fingers supported and firmware name
> > > are configurable using ACPI/DT properties.
> > >
> > > Signed-off-by: Robert Dolca <robert.dolca@...el.com>
> > > ---
> > > drivers/input/touchscreen/Kconfig | 12 +
> > > drivers/input/touchscreen/Makefile | 1 +
> > > drivers/input/touchscreen/silead.c | 596 +++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 609 insertions(+)
> > > create mode 100644 drivers/input/touchscreen/silead.c
> > >
> > > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > > index 547f67d65..dd12f36 100644
> > > --- a/drivers/input/touchscreen/Kconfig
> > > +++ b/drivers/input/touchscreen/Kconfig
> > > @@ -1025,4 +1025,16 @@ config TOUCHSCREEN_ZFORCE
> > > To compile this driver as a module, choose M here: the
> > > module will be called zforce_ts.
> > >
> > > +config TOUCHSCREEN_SILEAD
> > > + tristate "Silead I2C touchscreen"
> > > + depends on I2C
> > > + help
> > > + Say Y here if you have the Silead touchscreen connected to
> > > + your system.
> > > +
> > > + If unsure, say N.
> > > +
> > > + To compile this driver as a module, choose M here: the
> > > + module will be called silead.
> > > +
> > > endif
> > > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > > index 44deea7..2c6beaa 100644
> > > --- a/drivers/input/touchscreen/Makefile
> > > +++ b/drivers/input/touchscreen/Makefile
> > > @@ -84,3 +84,4 @@ obj-$(CONFIG_TOUCHSCREEN_W90X900) += w90p910_ts.o
> > > obj-$(CONFIG_TOUCHSCREEN_SX8654) += sx8654.o
> > > obj-$(CONFIG_TOUCHSCREEN_TPS6507X) += tps6507x-ts.o
> > > obj-$(CONFIG_TOUCHSCREEN_ZFORCE) += zforce_ts.o
> > > +obj-$(CONFIG_TOUCHSCREEN_SILEAD) += silead.o
> > > diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
> > > new file mode 100644
> > > index 0000000..b7da344
> > > --- /dev/null
> > > +++ b/drivers/input/touchscreen/silead.c
> > > @@ -0,0 +1,596 @@
> > > +/* -------------------------------------------------------------------------
> > > + * Copyright (C) 2014-2015, Intel Corporation
> > > + *
> > > + * Derived from:
> > > + * gslX68X.c
> > > + * Copyright (C) 2010-2015, Shanghai Sileadinc Co.Ltd
> > > + *
> > > + * 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; either version 2 of the License, or
> > > + * (at your option) any later version.
> > > + *
> > > + * 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.
> > > + * ------------------------------------------------------------------------- */
> > > +
> > > +#include <linux/i2c.h>
> > > +#include <linux/module.h>
> > > +#include <linux/acpi.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/firmware.h>
> > > +#include <linux/input.h>
> > > +#include <linux/input/mt.h>
> > > +#include <linux/pm.h>
> > > +#include <linux/irq.h>
> > > +
> > > +#define SILEAD_TS_NAME "silead_ts"
> > > +
> > > +#define SILEAD_REG_RESET 0xE0
> > > +#define SILEAD_REG_DATA 0x80
> > > +#define SILEAD_REG_TOUCH_NR 0x80
> > > +#define SILEAD_REG_POWER 0xBC
> > > +#define SILEAD_REG_CLOCK 0xE4
> > > +#define SILEAD_REG_STATUS 0xB0
> > > +#define SILEAD_REG_ID 0xFC
> > > +#define SILEAD_REG_MEM_CHECK 0xB0
> > > +
> > > +#define SILEAD_STATUS_OK 0x5A5A5A5A
> > > +#define SILEAD_TS_DATA_LEN 44
> > > +#define SILEAD_CLOCK 0x04
> > > +
> > > +#define SILEAD_CMD_RESET 0x88
> > > +#define SILEAD_CMD_START 0x00
> > > +
> > > +#define SILEAD_POINT_DATA_LEN 0x04
> > > +#define SILEAD_POINT_X_OFF 0x02
> > > +#define SILEAD_POINT_ID_OFF 0x03
> > > +#define SILEAD_X_HSB_MASK 0xF0
> > > +#define SILEAD_TOUCH_ID_MASK 0x0F
> > > +
> > > +#define SILEAD_DP_X_MAX "resolution-x"
> > > +#define SILEAD_DP_Y_MAX "resolution-y"
> > > +#define SILEAD_DP_MAX_FINGERS "max-fingers"
> > > +#define SILEAD_DP_FW_NAME "fw-name"
> > > +#define SILEAD_PWR_GPIO_NAME "power"
> > > +
> > > +#define SILEAD_CMD_SLEEP_MIN 10000
> > > +#define SILEAD_CMD_SLEEP_MAX 20000
> > > +#define SILEAD_POWER_SLEEP 20
> > > +#define SILEAD_STARTUP_SLEEP 30
> > > +
> > > +enum silead_ts_power {
> > > + SILEAD_POWER_ON = 1,
> > > + SILEAD_POWER_OFF = 0
> > > +};
> > > +
> > > +struct silead_ts_data {
> > > + struct i2c_client *client;
> > > + struct gpio_desc *gpio_power;
> > > + struct input_dev *input;
> > > + const char *custom_fw_name;
> > > + char fw_name[I2C_NAME_SIZE];
> > > + u16 x_max;
> > > + u16 y_max;
> > > + u8 max_fingers;
> > > + u32 chip_id;
> > > +};
> > > +
> > > +struct silead_fw_data {
> > > + u32 offset;
> > > + u32 val;
> > > +};
> > > +
> > > +static int silead_ts_request_input_dev(struct silead_ts_data *data)
> > > +{
> > > + struct device *dev = &data->client->dev;
> > > + int ret;
> > > +
> > > + data->input = devm_input_allocate_device(dev);
> > > + if (!data->input) {
> > > + dev_err(dev,
> > > + "Failed to allocate input device\n");
> > > + return -ENOMEM;
> > > + }
> > > +
> > > + input_set_capability(data->input, EV_ABS, ABS_X);
> > > + input_set_capability(data->input, EV_ABS, ABS_Y);
> >
> > No need as you do input_set_abs_params() below.
>
> It was put here to filter out the invalid coordinates. Why should it be removed?
input_set_abs_params() takes care of setting capability bits so
input_set_capability() is not really needed.
>
> > > +
> > > + input_set_abs_params(data->input, ABS_MT_POSITION_X, 0,
> > > + data->x_max, 0, 0);
> > > + input_set_abs_params(data->input, ABS_MT_POSITION_Y, 0,
> > > + data->y_max, 0, 0);
> > > +
> > > + input_mt_init_slots(data->input, data->max_fingers,
> > > + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> > > +
> > > + data->input->name = SILEAD_TS_NAME;
> > > + data->input->phys = "input/ts";
> > > + data->input->id.bustype = BUS_I2C;
> > > +
> > > + ret = input_register_device(data->input);
> > > + if (ret) {
> > > + dev_err(dev, "Failed to register input device: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void silead_ts_report_touch(struct silead_ts_data *data, u16 x, u16 y,
> > > + u8 id)
> > > +{
> > > + input_mt_slot(data->input, id);
> > > + input_mt_report_slot_state(data->input, MT_TOOL_FINGER, true);
> > > + input_report_abs(data->input, ABS_MT_POSITION_X, x);
> > > + input_report_abs(data->input, ABS_MT_POSITION_Y, y);
> > > +}
> > > +
> > > +static void silead_ts_set_power(struct i2c_client *client,
> > > + enum silead_ts_power state)
> > > +{
> > > + struct silead_ts_data *data = i2c_get_clientdata(client);
> > > +
> > > + gpiod_set_value_cansleep(data->gpio_power, state);
> > > + msleep(SILEAD_POWER_SLEEP);
> > > +}
> > > +
> > > +static void silead_ts_read_data(struct i2c_client *client)
> > > +{
> > > + struct silead_ts_data *data = i2c_get_clientdata(client);
> > > + struct device *dev = &client->dev;
> > > + u8 buf[SILEAD_TS_DATA_LEN];
> > > + int x, y, id, touch_nr, ret, i, offset;
> > > +
> > > + ret = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_DATA,
> > > + SILEAD_TS_DATA_LEN, buf);
> > > + if (ret < 0) {
> > > + dev_err(dev, "Data read error %d\n", ret);
> > > + return;
> > > + }
> > > +
> > > + touch_nr = buf[0];
> > > +
> > > + if (touch_nr < 0)
> > > + return;
> > > +
> > > + dev_dbg(dev, "Touch number: %d\n", touch_nr);
> > > +
> > > + for (i = 1; i <= touch_nr; i++) {
> > > + offset = i * SILEAD_POINT_DATA_LEN;
> > > +
> > > + /* The last 4 bits are the touch id */
> > > + id = buf[offset + SILEAD_POINT_ID_OFF] & SILEAD_TOUCH_ID_MASK;
> > > +
> > > + /* The 1st 4 bits are part of X */
> > > + buf[offset + SILEAD_POINT_ID_OFF] =
> > > + (buf[offset + SILEAD_POINT_ID_OFF] & SILEAD_X_HSB_MASK)
> > > + >> 4;
> > > +
> > > + y = le16_to_cpup((__le16 *)(buf + offset));
> > > + x = le16_to_cpup((__le16 *)(buf + offset + SILEAD_POINT_X_OFF));
> > > +
> > > + dev_dbg(dev, "x=%d y=%d id=%d\n", x, y, id);
> > > + silead_ts_report_touch(data, x, y, id);
> > > + }
> > > +
> > > + input_mt_sync_frame(data->input);
> > > + input_sync(data->input);
> > > +}
> > > +
> > > +static int silead_ts_init(struct i2c_client *client)
> > > +{
> > > + struct silead_ts_data *data = i2c_get_clientdata(client);
> > > + int ret;
> > > +
> > > + ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> > > + SILEAD_CMD_RESET);
> > > + if (ret)
> > > + goto i2c_write_err;
> > > + usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > > +
> > > + ret = i2c_smbus_write_byte_data(client, SILEAD_REG_TOUCH_NR,
> > > + data->max_fingers);
> > > + if (ret)
> > > + goto i2c_write_err;
> > > + usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > > +
> > > + ret = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK, SILEAD_CLOCK);
> > > + if (ret)
> > > + goto i2c_write_err;
> > > + usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > > +
> > > + ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> > > + SILEAD_CMD_START);
> > > + if (ret)
> > > + goto i2c_write_err;
> > > + usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > > +
> > > + return 0;
> > > +
> > > +i2c_write_err:
> > > + dev_err(&client->dev, "Registers clear error %d\n", ret);
> > > + return ret;
> > > +}
> > > +
> > > +static int silead_ts_reset(struct i2c_client *client)
> > > +{
> > > + int ret;
> > > +
> > > + ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> > > + SILEAD_CMD_RESET);
> > > + if (ret)
> > > + goto i2c_write_err;
> > > + usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > > +
> > > + ret = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK, SILEAD_CLOCK);
> > > + if (ret)
> > > + goto i2c_write_err;
> > > + usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > > +
> > > + ret = i2c_smbus_write_byte_data(client, SILEAD_REG_POWER,
> > > + SILEAD_CMD_START);
> > > + if (ret)
> > > + goto i2c_write_err;
> > > + usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> > > +
> > > + return 0;
> > > +
> > > +i2c_write_err:
> > > + dev_err(&client->dev, "Chip reset error %d\n", ret);
> > > + return ret;
> > > +}
> > > +
> > > +static int silead_ts_startup(struct i2c_client *client)
> > > +{
> > > + int ret;
> > > +
> > > + ret = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET, 0x00);
> > > + if (ret) {
> > > + dev_err(&client->dev, "Startup error %d\n", ret);
> > > + return ret;
> > > + }
> > > + msleep(SILEAD_STARTUP_SLEEP);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int silead_ts_load_fw(struct i2c_client *client)
> > > +{
> > > + struct device *dev = &client->dev;
> > > + struct silead_ts_data *data = i2c_get_clientdata(client);
> > > + unsigned int fw_size, i;
> > > + const struct firmware *fw;
> > > + struct silead_fw_data *fw_data;
> > > + int ret;
> > > +
> > > + dev_dbg(dev, "Firmware file name: %s", data->fw_name);
> > > +
> > > + if (data->custom_fw_name)
> > > + ret = request_firmware(&fw, data->custom_fw_name, dev);
> > > + else
> > > + ret = request_firmware(&fw, data->fw_name, dev);
> > > +
> > > + if (ret) {
> > > + dev_err(dev, "Firmware request error %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + fw_size = fw->size / sizeof(*fw_data);
> > > + fw_data = (struct silead_fw_data *)fw->data;
> > > +
> > > + for (i = 0; i < fw_size; i++) {
> > > + ret = i2c_smbus_write_i2c_block_data(client, fw_data[i].offset,
> > > + 4, (u8 *)&fw_data[i].val);
> > > + if (ret) {
> > > + dev_err(dev, "Firmware load error %d\n", ret);
> > > + goto release_fw_err;
> > > + }
> > > + }
> > > +
> > > + release_firmware(fw);
> > > + return 0;
> > > +
> > > +release_fw_err:
> > > + release_firmware(fw);
> > > + return ret;
> > > +}
> > > +
> > > +static u32 silead_ts_get_status(struct i2c_client *client)
> > > +{
> > > + int ret;
> > > + u32 status;
> > > +
> > > + ret = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_STATUS, 4,
> > > + (u8 *)&status);
> >
> > Endianness.
> >
> > > + if (ret < 0) {
> > > + dev_err(&client->dev, "Status read error %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + return status;
> > > +}
> > > +
> > > +static int silead_ts_get_id(struct i2c_client *client)
> > > +{
> > > + struct silead_ts_data *data = i2c_get_clientdata(client);
> > > + int ret;
> > > +
> > > + ret = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_ID, 4,
> > > + (u8 *)&data->chip_id);
> >
> > Endianness.
> >
> > > + if (ret < 0) {
> > > + dev_err(&client->dev, "Chip ID read error %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int silead_ts_setup(struct i2c_client *client)
> > > +{
> > > + struct silead_ts_data *data = i2c_get_clientdata(client);
> > > + struct device *dev = &client->dev;
> > > + int ret;
> > > + u32 status;
> > > +
> > > + silead_ts_set_power(client, SILEAD_POWER_OFF);
> > > + silead_ts_set_power(client, SILEAD_POWER_ON);
> > > +
> > > + ret = silead_ts_get_id(client);
> > > + if (ret)
> > > + return ret;
> > > + dev_dbg(dev, "Chip ID: 0x%8X", data->chip_id);
> > > +
> > > + ret = silead_ts_init(client);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = silead_ts_reset(client);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = silead_ts_load_fw(client);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = silead_ts_startup(client);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + status = silead_ts_get_status(client);
> > > + if (status != SILEAD_STATUS_OK) {
> > > + dev_err(dev, "Initialization error, status: 0x%X\n", status);
> > > + return -ENODEV;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static irqreturn_t silead_ts_threaded_irq_handler(int irq, void *id)
> > > +{
> > > + struct silead_ts_data *data = (struct silead_ts_data *)id;
> >
> > No need to cast from void *.
> >
> > > + struct i2c_client *client = data->client;
> > > +
> > > + silead_ts_read_data(client);
> > > +
> > > + return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int silead_ts_read_props(struct i2c_client *client)
> > > +{
> > > + struct silead_ts_data *data = i2c_get_clientdata(client);
> > > + struct device *dev = &client->dev;
> > > + int ret;
> > > +
> > > + ret = device_property_read_u16(dev, SILEAD_DP_X_MAX, &data->x_max);
> > > + if (ret) {
> > > + dev_err(dev, "Resolution X read error %d\n", ret);
> > > + goto error;
> > > + }
> >
> > Can we use the standard touchscreen properties as described in
> > Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
> >
> > > +
> > > + ret = device_property_read_u16(dev, SILEAD_DP_Y_MAX, &data->y_max);
> > > + if (ret) {
> > > + dev_err(dev, "Resolution Y read error %d\n", ret);
> > > + goto error;
> > > + }
> > > +
> > > + ret = device_property_read_u8(dev, SILEAD_DP_MAX_FINGERS,
> > > + &data->max_fingers);
> > > + if (ret) {
> > > + dev_err(dev, "Max fingers read error %d\n", ret);
> > > + goto error;
> > > + }
> >
> > Can you query the controller capabilities to get number of fingers
> > supported? What controls it? The chip? Firmware?
>
> In the documentation from the vendor there is no register for this. I
> will ask them.
>
> > > +
> > > + ret = device_property_read_string(dev, SILEAD_DP_FW_NAME,
> > > + &data->custom_fw_name);
> > > + if (ret)
> > > + dev_info(dev, "Firmware file name read error. Using default.");
> >
> > Why do we need this?
>
> Even if there is the same chip, if the panel is different you need a
> different firmware. The firmware file name defaults to the chip name
> but can be overwritten.
But when you compose your userland image you can make sure you are
putting the needed firmware into the right place. There is no really
need to add the code to kernel when you can rename in userspace.
>
> > > +
> > > + dev_dbg(dev, "X max = %d, Y max = %d, max fingers = %d",
> > > + data->x_max, data->y_max, data->max_fingers);
> > > +
> > > + return 0;
> > > +
> > > +error:
> > > + return ret;
> > > +}
> > > +
> > > +#ifdef CONFIG_ACPI
> > > +static const struct acpi_device_id silead_ts_acpi_match[];
> > > +
> > > +static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
> > > + const struct i2c_device_id *id)
> > > +{
> > > + const struct acpi_device_id *acpi_id;
> > > + struct device *dev = &data->client->dev;
> > > + int i;
> > > +
> > > + if (ACPI_HANDLE(dev)) {
> > > + acpi_id = acpi_match_device(silead_ts_acpi_match, dev);
> > > + if (!acpi_id)
> > > + return -ENODEV;
> > > +
> > > + sprintf(data->fw_name, "%s.fw", acpi_id->id);
> > > +
> > > + for (i = 0; i < strlen(data->fw_name); i++)
> > > + data->fw_name[i] = tolower(data->fw_name[i]);
> > > + } else {
> > > + sprintf(data->fw_name, "%s.fw", id->name);
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +#else
> > > +static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
> > > + const struct i2c_device_id *id)
> > > +{
> > > + sprintf(data->fw_name, "%s.fw", id->name);
> > > + return 0;
> > > +}
> > > +#endif
> > > +
> > > +static int silead_ts_probe(struct i2c_client *client,
> > > + const struct i2c_device_id *id)
> > > +{
> > > + struct silead_ts_data *data;
> > > + struct device *dev = &client->dev;
> > > + int ret;
> > > +
> > > + if (!i2c_check_functionality(client->adapter,
> > > + I2C_FUNC_I2C |
> > > + I2C_FUNC_SMBUS_READ_I2C_BLOCK |
> > > + I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
> > > + dev_err(dev, "I2C functionality check failed\n");
> > > + return -ENXIO;
> > > + }
> > > +
> > > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > > + if (!data)
> > > + return -ENOMEM;
> > > +
> > > + i2c_set_clientdata(client, data);
> > > + data->client = client;
> > > +
> > > + ret = silead_ts_set_default_fw_name(data, id);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + /* If the IRQ is not filled by DT or ACPI subsytem
> > > + * we can't continue without it */
> > > + if (client->irq <= 0)
> > > + return -ENODEV;
> > > +
> > > + /* Power GPIO pin */
> > > + data->gpio_power = devm_gpiod_get(dev, SILEAD_PWR_GPIO_NAME,
> > > + GPIOD_OUT_LOW);
> > > + if (IS_ERR(data->gpio_power)) {
> > > + dev_err(dev, "Shutdown GPIO request failed\n");
> > > + return PTR_ERR(data->gpio_power);
> > > + }
> > > +
> > > + ret = silead_ts_read_props(client);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = silead_ts_setup(client);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = silead_ts_request_input_dev(data);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> > > + silead_ts_threaded_irq_handler,
> > > + IRQF_ONESHOT | IRQ_TYPE_EDGE_RISING,
> > > + client->name, data);
> > > + if (ret) {
> > > + dev_err(dev, "IRQ request failed %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + dev_dbg(dev, "Probing succeded\n");
> > > + return 0;
> > > +}
> > > +
> > > +#ifdef CONFIG_PM_SLEEP
> > > +static int silead_ts_suspend(struct device *dev)
> >
> > Instead of guarding with ifdef let's annotate with __maybe_unused. This
> > provides better compile coverage.
> >
> > > +{
> > > + struct i2c_client *client = to_i2c_client(dev);
> > > +
> > > + silead_ts_set_power(client, SILEAD_POWER_OFF);
> > > + return 0;
> > > +}
> > > +
> > > +static int silead_ts_resume(struct device *dev)
> > > +{
> > > + struct i2c_client *client = to_i2c_client(dev);
> > > + int ret, status;
> >
> > Personal preference: can we call here and elsewhere variables that store
> > error codes only "error" and not "ret"?
> >
> > > +
> > > + silead_ts_set_power(client, SILEAD_POWER_ON);
> > > +
> > > + ret = silead_ts_reset(client);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = silead_ts_startup(client);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + status = silead_ts_get_status(client);
> > > + if (status != SILEAD_STATUS_OK) {
> > > + dev_err(dev, "Resume error, status: 0x%X\n", status);
> > > + return -ENODEV;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static SIMPLE_DEV_PM_OPS(silead_ts_pm, silead_ts_suspend, silead_ts_resume);
> >
> > Pull it out of ifdef and then you do not need #ifdef in driver
> > initializer.
> >
> > > +#endif
> > > +
> > > +static const struct i2c_device_id silead_ts_id[] = {
> > > + { "gsl1680", 0 },
> > > + { "gsl1688", 0 },
> > > + { "gsl3670", 0 },
> > > + { "gsl3675", 0 },
> > > + { "gsl3692", 0 },
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, silead_ts_id);
> > > +
> > > +#ifdef CONFIG_ACPI
> > > +static const struct acpi_device_id silead_ts_acpi_match[] = {
> > > + { "GSL1680", 0 },
> > > + { "GSL1688", 0 },
> > > + { "GSL3670", 0 },
> > > + { "GSL3675", 0 },
> > > + { "GSL3692", 0 },
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(acpi, silead_ts_acpi_match);
> > > +#endif
> > > +
> > > +static struct i2c_driver silead_ts_driver = {
> > > + .probe = silead_ts_probe,
> > > + .id_table = silead_ts_id,
> > > + .driver = {
> > > + .name = SILEAD_TS_NAME,
> > > + .owner = THIS_MODULE,
> > > +#ifdef CONFIG_ACPI
> > > + .acpi_match_table = ACPI_PTR(silead_ts_acpi_match),
> > > +#endif
> >
> > No need for ifdef if you are using ACPI_PTR.
> >
> > > +#ifdef CONFIG_PM_SLEEP
> > > + .pm = &silead_ts_pm,
> > > +#endif
> > > + },
> > > +};
> > > +module_i2c_driver(silead_ts_driver);
> > > +
> > > +MODULE_AUTHOR("Robert Dolca <robert.dolca@...el.com>");
> > > +MODULE_DESCRIPTION("Silead I2C touchscreen driver");
> > > +MODULE_LICENSE("GPL");
> > > --
> > > 1.9.1
> > >
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists