Hi Giulio,

On Fri, Mar 05, 2021 at 05:38:34PM +0100, Giulio Benetti wrote:
> From: Giulio Benetti <giulio.bene...@micronovasrl.com>
> 
> This patch adds support for Hycon HY46XX.

Could you please tell me where patches 1/3 and 2/3. I am guessing they
are dealing with DT bindings and are most likely to go through my tree
after Rob's ACK.

> 
> Signed-off-by: Giulio Benetti <giulio.bene...@micronovasrl.com>
> ---
>  MAINTAINERS                              |   1 +
>  drivers/input/touchscreen/Kconfig        |  12 +
>  drivers/input/touchscreen/Makefile       |   1 +
>  drivers/input/touchscreen/hycon-hy46xx.c | 571 +++++++++++++++++++++++
>  4 files changed, 585 insertions(+)
>  create mode 100644 drivers/input/touchscreen/hycon-hy46xx.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3f83daf6b2bf..7a1331657e4b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8250,6 +8250,7 @@ M:      Giulio Benetti <giulio.bene...@micronovasrl.com>
>  L:   linux-in...@vger.kernel.org
>  S:   Maintained
>  F:   Documentation/devicetree/bindings/input/touchscreen/hycon,hy46xx.yaml
> +F:   drivers/input/touchscreen/hy46xx.c
>  
>  HYGON PROCESSOR SUPPORT
>  M:   Pu Wen <pu...@hygon.cn>
> diff --git a/drivers/input/touchscreen/Kconfig 
> b/drivers/input/touchscreen/Kconfig
> index 529614d364fe..5d4751d901cb 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1335,4 +1335,16 @@ config TOUCHSCREEN_ZINITIX
>         To compile this driver as a module, choose M here: the
>         module will be called zinitix.
>  
> +config TOUCHSCREEN_HYCON_HY46XX
> +     tristate "Hycon hy46xx touchscreen support"
> +     depends on I2C
> +     help
> +       Say Y here if you have a touchscreen using Hycon hy46xx,
> +       or something similar enough.
> +
> +       If unsure, say N.
> +
> +       To compile this driver as a module, choose M here: the
> +       module will be called hycon-hy46xx.
> +
>  endif
> diff --git a/drivers/input/touchscreen/Makefile 
> b/drivers/input/touchscreen/Makefile
> index 6233541e9173..8b68cf3a3e6d 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -112,3 +112,4 @@ obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023)    += 
> rohm_bu21023.o
>  obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW)     += raspberrypi-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_IQS5XX)     += iqs5xx.o
>  obj-$(CONFIG_TOUCHSCREEN_ZINITIX)    += zinitix.o
> +obj-$(CONFIG_TOUCHSCREEN_HYCON_HY46XX)       += hycon-hy46xx.o
> diff --git a/drivers/input/touchscreen/hycon-hy46xx.c 
> b/drivers/input/touchscreen/hycon-hy46xx.c
> new file mode 100644
> index 000000000000..ef0dee9a43a9
> --- /dev/null
> +++ b/drivers/input/touchscreen/hycon-hy46xx.c
> @@ -0,0 +1,571 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021
> + * Author(s): Giulio Benetti <giulio.bene...@micronovasrl.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/irq.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <asm/unaligned.h>
> +
> +#define HY46XX_CHKSUM_CODE           0x1
> +#define HY46XX_FINGER_NUM            0x2
> +#define HY46XX_CHKSUM_LEN            0x7
> +#define HY46XX_THRESHOLD             0x80
> +#define HY46XX_PROX_SENS_SW          0x81
> +#define HY46XX_GLOVE_EN                      0x84
> +#define HY46XX_REPORT_SPEED          0x88
> +#define HY46XX_PWR_NOISE_EN          0x89
> +#define HY46XX_FILTER_DATA           0x8A
> +#define HY46XX_GAIN                  0x92
> +#define HY46XX_EDGE_OFFSET           0x93
> +#define HY46XX_RX_NR_USED            0x94
> +#define HY46XX_TX_NR_USED            0x95
> +#define HY46XX_PWR_MODE                      0xA5
> +#define HY46XX_FW_VERSION            0xA6
> +#define HY46XX_LIB_VERSION           0xA7
> +#define HY46XX_TP_INFO                       0xA8
> +#define HY46XX_TP_CHIP_ID            0xA9
> +#define HY46XX_BOOT_VER                      0xB0
> +
> +#define HY46XX_TPLEN                 0x6
> +
> +#define HY46XX_MAX_SUPPORTED_POINTS  11
> +
> +#define TOUCH_EVENT_DOWN             0x00
> +#define TOUCH_EVENT_UP                       0x01
> +#define TOUCH_EVENT_CONTACT          0x02
> +#define TOUCH_EVENT_RESERVED         0x03
> +
> +struct hycon_hy46xx_data {
> +     struct i2c_client *client;
> +     struct input_dev *input;
> +     struct touchscreen_properties prop;
> +     struct regulator *vcc;
> +
> +     struct gpio_desc *reset_gpio;
> +
> +     struct mutex mutex;
> +
> +     int threshold;
> +     int proximity_sensor_switch;
> +     int glove_enable;
> +     int report_speed;
> +     int power_noise_enable;
> +     int filter_data;
> +     int gain;
> +     int edge_offset;
> +     int rx_number_used;
> +     int tx_number_used;
> +     int power_mode;
> +     int fw_version;
> +     int lib_version;
> +     int tp_information;
> +     int tp_chip_id;
> +     int bootloader_version;
> +};
> +
> +static int hycon_hy46xx_readwrite(struct i2c_client *client, u16 wr_len, u8 
> *wr_buf,
> +                         u16 rd_len, u8 *rd_buf)
> +{
> +     struct i2c_msg msgs[2];
> +     int i = 0;
> +     int ret;
> +
> +     if (wr_len) {
> +             msgs[i].addr  = client->addr;
> +             msgs[i].flags = 0;
> +             msgs[i].len = wr_len;
> +             msgs[i].buf = wr_buf;
> +             i++;
> +     }
> +     if (rd_len) {
> +             msgs[i].addr  = client->addr;
> +             msgs[i].flags = I2C_M_RD;
> +             msgs[i].len = rd_len;
> +             msgs[i].buf = rd_buf;
> +             i++;
> +     }
> +
> +     ret = i2c_transfer(client->adapter, msgs, i);
> +     if (ret < 0)
> +             return ret;
> +     if (ret != i)
> +             return -EIO;
> +
> +     return 0;
> +}

Please see if your driver could be converted to regmap API instead of
"maked" i2c. It will allow using write-gather when system supports it,
etc.

> +
> +static bool hycon_hy46xx_check_checksum(struct hycon_hy46xx_data *tsdata, u8 
> *buf)
> +{
> +     u8 chksum = 0;
> +     int i;
> +
> +     for (i = 2; i < buf[HY46XX_CHKSUM_LEN]; i++)
> +             chksum += buf[i];
> +
> +     if (chksum == buf[HY46XX_CHKSUM_CODE])
> +             return true;
> +
> +     dev_err_ratelimited(&tsdata->client->dev,
> +                         "checksum error: 0x%02x expected, got 0x%02x\n",
> +                         chksum, buf[HY46XX_CHKSUM_CODE]);
> +
> +     return false;
> +}
> +
> +static irqreturn_t hycon_hy46xx_isr(int irq, void *dev_id)
> +{
> +     struct hycon_hy46xx_data *tsdata = dev_id;
> +     struct device *dev = &tsdata->client->dev;
> +     u8 rdbuf[0x44];
A #define here would be nice.
A #define here would be nice.

> +     u8 cmd;
> +     int i, x, y, id;
> +     int error;
> +
> +     memset(rdbuf, 0, sizeof(rdbuf));
> +
> +     error = hycon_hy46xx_readwrite(tsdata->client, 1, &cmd, sizeof(rdbuf), 
> rdbuf);

cmd could be any garbage (as it is uninitialized)? This requires a nice
comment as to why it is OK.

> +     if (error) {
> +             dev_err_ratelimited(dev, "Unable to fetch data, error: %d\n",
> +                                 error);
> +             goto out;
> +     }
> +
> +     if (!hycon_hy46xx_check_checksum(tsdata, rdbuf))
> +             goto out;
> +
> +     for (i = 0; i < HY46XX_MAX_SUPPORTED_POINTS; i++) {
> +             u8 *buf = &rdbuf[3 + (HY46XX_TPLEN * i)];
> +             int type = buf[0] >> 6;
> +
> +             if (type == TOUCH_EVENT_RESERVED)
> +                     continue;
> +
> +             x = get_unaligned_be16(buf) & 0x0fff;
> +             y = get_unaligned_be16(buf + 2) & 0x0fff;
> +
> +             id = (buf[2] >> 4) & 0x0f;

buf is u8 so no need to additionally mask after shifting.

> +
> +             input_mt_slot(tsdata->input, id);
> +             if (input_mt_report_slot_state(tsdata->input, MT_TOOL_FINGER,
> +                                            type != TOUCH_EVENT_UP))
> +                     touchscreen_report_pos(tsdata->input, &tsdata->prop,
> +                                            x, y, true);
> +     }
> +
> +     input_mt_report_pointer_emulation(tsdata->input, true);
> +     input_sync(tsdata->input);
> +
> +out:
> +     return IRQ_HANDLED;
> +}
> +
> +static int hycon_hy46xx_register_write(struct hycon_hy46xx_data *tsdata, u8 
> addr, u8 value)
> +{
> +     u8 wrbuf[2];
> +
> +     wrbuf[0] = addr;
> +     wrbuf[1] = value;
> +
> +     return hycon_hy46xx_readwrite(tsdata->client, 2, wrbuf, 0, NULL);
> +}
> +
> +static int hycon_hy46xx_register_read(struct hycon_hy46xx_data *tsdata, u8 
> addr)
> +{
> +     u8 wrbyte, rdbyte;
> +     int error;
> +
> +     wrbyte = addr;
> +     error = hycon_hy46xx_readwrite(tsdata->client, 1, &wrbyte, 1, &rdbyte);
> +     if (error)
> +             return error;
> +
> +     return rdbyte;
> +}
> +
> +struct hycon_hy46xx_attribute {
> +     struct device_attribute dattr;
> +     size_t field_offset;
> +     u8 address;
> +     u8 limit_low;
> +     u8 limit_high;
> +};
> +
> +#define HYCON_ATTR(_field, _mode, _address, _limit_low, _limit_high) \
> +     struct hycon_hy46xx_attribute hycon_hy46xx_attr_##_field = {            
> \
> +             .dattr = __ATTR(_field, _mode,                          \
> +                             hycon_hy46xx_setting_show,                      
> \
> +                             hycon_hy46xx_setting_store),                    
> \
> +             .field_offset = offsetof(struct hycon_hy46xx_data, _field),     
> \
> +             .address = _address,                                    \
> +             .limit_low = _limit_low,                                \
> +             .limit_high = _limit_high,                              \
> +     }
> +
> +static ssize_t hycon_hy46xx_setting_show(struct device *dev,
> +                                struct device_attribute *dattr, char *buf)
> +{
> +     struct i2c_client *client = to_i2c_client(dev);
> +     struct hycon_hy46xx_data *tsdata = i2c_get_clientdata(client);
> +     struct hycon_hy46xx_attribute *attr =
> +                     container_of(dattr, struct hycon_hy46xx_attribute, 
> dattr);
> +     u8 *field = (u8 *)tsdata + attr->field_offset;
> +     int val;
> +     size_t count = 0;
> +     int error = 0;
> +     u8 addr = attr->address;
> +
> +     mutex_lock(&tsdata->mutex);
> +
> +     val = hycon_hy46xx_register_read(tsdata, addr);
> +     if (val < 0) {
> +             error = val;
> +             dev_err(&tsdata->client->dev,
> +                     "Failed to fetch attribute %s, error %d\n",
> +                     dattr->attr.name, error);
> +             goto out;
> +     }
> +
> +     if (val != *field) {
> +             dev_warn(&tsdata->client->dev,
> +                      "%s: read (%d) and stored value (%d) differ\n",
> +                      dattr->attr.name, val, *field);
> +             *field = val;
> +     }
> +
> +     count = scnprintf(buf, PAGE_SIZE, "%d\n", val);
> +out:
> +     mutex_unlock(&tsdata->mutex);
> +     return error ?: count;
> +}
> +
> +static ssize_t hycon_hy46xx_setting_store(struct device *dev,
> +                                     struct device_attribute *dattr,
> +                                     const char *buf, size_t count)
> +{
> +     struct i2c_client *client = to_i2c_client(dev);
> +     struct hycon_hy46xx_data *tsdata = i2c_get_clientdata(client);
> +     struct hycon_hy46xx_attribute *attr =
> +                     container_of(dattr, struct hycon_hy46xx_attribute, 
> dattr);
> +     u8 *field = (u8 *)tsdata + attr->field_offset;
> +     unsigned int val;
> +     int error;
> +     u8 addr = attr->address;
> +
> +     mutex_lock(&tsdata->mutex);
> +
> +     error = kstrtouint(buf, 0, &val);
> +     if (error)
> +             goto out;
> +
> +     if (val < attr->limit_low || val > attr->limit_high) {
> +             error = -ERANGE;
> +             goto out;
> +     }
> +
> +     error = hycon_hy46xx_register_write(tsdata, addr, val);
> +     if (error) {
> +             dev_err(&tsdata->client->dev,
> +                     "Failed to update attribute %s, error: %d\n",
> +                     dattr->attr.name, error);
> +             goto out;
> +     }
> +     *field = val;

I am not sure why you want this check (I see that we have it in one
other driver and I really do not recall why we have it there either).

> +
> +out:
> +     mutex_unlock(&tsdata->mutex);
> +     return error ?: count;
> +}
> +
> +static HYCON_ATTR(threshold, 0644, HY46XX_THRESHOLD, 0, 255);
> +static HYCON_ATTR(proximity_sensor_switch, 0644,  HY46XX_PROX_SENS_SW, 0, 1);
> +static HYCON_ATTR(glove_enable, 0644, HY46XX_GLOVE_EN, 0, 1);
> +static HYCON_ATTR(report_speed, 0644, HY46XX_REPORT_SPEED, 0, 255);
> +static HYCON_ATTR(power_noise_enable, 0644, HY46XX_PWR_NOISE_EN, 0, 1);
> +static HYCON_ATTR(filter_data, 0644, HY46XX_FILTER_DATA, 0, 5);
> +static HYCON_ATTR(gain, 0644, HY46XX_GAIN, 0, 5);
> +static HYCON_ATTR(edge_offset, 0644, HY46XX_EDGE_OFFSET, 0, 5);
> +static HYCON_ATTR(fw_version, 0444, HY46XX_FW_VERSION, 0, 255);
> +static HYCON_ATTR(lib_version, 0444, HY46XX_LIB_VERSION, 0, 255);
> +static HYCON_ATTR(tp_information, 0444, HY46XX_TP_INFO, 0, 255);
> +static HYCON_ATTR(tp_chip_id, 0444, HY46XX_TP_CHIP_ID, 0, 255);
> +static HYCON_ATTR(bootloader_version, 0444, HY46XX_BOOT_VER, 0, 255);
> +
> +static struct attribute *hycon_hy46xx_attrs[] = {
> +     &hycon_hy46xx_attr_threshold.dattr.attr,
> +     &hycon_hy46xx_attr_proximity_sensor_switch.dattr.attr,
> +     &hycon_hy46xx_attr_glove_enable.dattr.attr,
> +     &hycon_hy46xx_attr_report_speed.dattr.attr,
> +     &hycon_hy46xx_attr_power_noise_enable.dattr.attr,
> +     &hycon_hy46xx_attr_filter_data.dattr.attr,
> +     &hycon_hy46xx_attr_gain.dattr.attr,
> +     &hycon_hy46xx_attr_edge_offset.dattr.attr,
> +     &hycon_hy46xx_attr_fw_version.dattr.attr,
> +     &hycon_hy46xx_attr_lib_version.dattr.attr,
> +     &hycon_hy46xx_attr_tp_information.dattr.attr,
> +     &hycon_hy46xx_attr_tp_chip_id.dattr.attr,
> +     &hycon_hy46xx_attr_bootloader_version.dattr.attr,
> +     NULL
> +};
> +
> +static const struct attribute_group hycon_hy46xx_attr_group = {
> +     .attrs = hycon_hy46xx_attrs,
> +};
> +
> +static void hycon_hy46xx_get_defaults(struct device *dev, struct 
> hycon_hy46xx_data *tsdata)
> +{
> +     u32 val;
> +     int error;
> +
> +     error = device_property_read_u32(dev, "threshold", &val);
> +     if (!error) {
> +             hycon_hy46xx_register_write(tsdata, HY46XX_THRESHOLD, val);
> +             tsdata->threshold = val;
> +     }
> +
> +     error = device_property_read_u32(dev, "proximity-sensor-switch", &val);
> +     if (!error) {
> +             hycon_hy46xx_register_write(tsdata, HY46XX_PROX_SENS_SW, val);
> +             tsdata->proximity_sensor_switch = val;
> +     }
> +
> +     error = device_property_read_u32(dev, "glove-enable", &val);
> +     if (!error) {
> +             hycon_hy46xx_register_write(tsdata, HY46XX_GLOVE_EN, val);
> +             tsdata->glove_enable = val;
> +     }
> +
> +     error = device_property_read_u32(dev, "report-speed", &val);
> +     if (!error) {
> +             hycon_hy46xx_register_write(tsdata, HY46XX_REPORT_SPEED, val);
> +             tsdata->report_speed = val;
> +     }
> +
> +     error = device_property_read_u32(dev, "power-noise-enable", &val);
> +     if (!error) {
> +             hycon_hy46xx_register_write(tsdata, HY46XX_PWR_NOISE_EN, val);
> +             tsdata->report_speed = val;
> +     }
> +
> +     error = device_property_read_u32(dev, "filter-data", &val);
> +     if (!error) {
> +             hycon_hy46xx_register_write(tsdata, HY46XX_FILTER_DATA, val);
> +             tsdata->report_speed = val;
> +     }
> +
> +     error = device_property_read_u32(dev, "gain", &val);
> +     if (!error) {
> +             hycon_hy46xx_register_write(tsdata, HY46XX_GAIN, val);
> +             tsdata->report_speed = val;
> +     }
> +
> +     error = device_property_read_u32(dev, "edge-offset", &val);
> +     if (!error) {
> +             hycon_hy46xx_register_write(tsdata, HY46XX_EDGE_OFFSET, val);
> +             tsdata->report_speed = val;
> +     }
> +}
> +
> +static void hycon_hy46xx_get_parameters(struct hycon_hy46xx_data *tsdata)
> +{
> +     tsdata->threshold = hycon_hy46xx_register_read(tsdata, 
> HY46XX_THRESHOLD);
> +     tsdata->proximity_sensor_switch =
> +             hycon_hy46xx_register_read(tsdata, HY46XX_PROX_SENS_SW);
> +     tsdata->glove_enable = hycon_hy46xx_register_read(tsdata, 
> HY46XX_GLOVE_EN);
> +     tsdata->report_speed =
> +             hycon_hy46xx_register_read(tsdata, HY46XX_REPORT_SPEED);
> +     tsdata->power_noise_enable =
> +             hycon_hy46xx_register_read(tsdata, HY46XX_PWR_NOISE_EN);
> +     tsdata->filter_data = hycon_hy46xx_register_read(tsdata, 
> HY46XX_FILTER_DATA);
> +     tsdata->gain = hycon_hy46xx_register_read(tsdata, HY46XX_GAIN);
> +     tsdata->edge_offset = hycon_hy46xx_register_read(tsdata, 
> HY46XX_EDGE_OFFSET);
> +     tsdata->rx_number_used =
> +             hycon_hy46xx_register_read(tsdata, HY46XX_RX_NR_USED);
> +     tsdata->tx_number_used =
> +             hycon_hy46xx_register_read(tsdata, HY46XX_TX_NR_USED);
> +     tsdata->power_mode = hycon_hy46xx_register_read(tsdata, 
> HY46XX_PWR_MODE);
> +     tsdata->fw_version = hycon_hy46xx_register_read(tsdata, 
> HY46XX_FW_VERSION);
> +     tsdata->lib_version = hycon_hy46xx_register_read(tsdata, 
> HY46XX_LIB_VERSION);
> +     tsdata->tp_information = hycon_hy46xx_register_read(tsdata, 
> HY46XX_TP_INFO);
> +     tsdata->tp_chip_id = hycon_hy46xx_register_read(tsdata, 
> HY46XX_TP_CHIP_ID);
> +     tsdata->bootloader_version =
> +             hycon_hy46xx_register_read(tsdata, HY46XX_BOOT_VER);
> +}
> +
> +static void hycon_hy46xx_disable_regulator(void *arg)
> +{
> +     struct hycon_hy46xx_data *data = arg;
> +
> +     regulator_disable(data->vcc);
> +}
> +
> +static int hycon_hy46xx_probe(struct i2c_client *client,
> +                                      const struct i2c_device_id *id)
> +{
> +     const struct hycon_hy46xx_i2c_chip_data *chip_data;

Where is this defined??

> +     struct hycon_hy46xx_data *tsdata;
> +     struct input_dev *input;
> +     unsigned long irq_flags;
> +     int error;
> +
> +     dev_dbg(&client->dev, "probing for HYCON HY46XX I2C\n");
> +
> +     tsdata = devm_kzalloc(&client->dev, sizeof(*tsdata), GFP_KERNEL);
> +     if (!tsdata)
> +             return -ENOMEM;
> +
> +     chip_data = device_get_match_data(&client->dev);
> +     if (!chip_data)
> +             chip_data = (const struct hycon_hy46xx_i2c_chip_data *)
> +                          id->driver_data;
> +     if (!chip_data) {
> +             dev_err(&client->dev, "invalid or missing chip data\n");
> +             return -EINVAL;
> +     }

You are not attaching any additional data to OF or legacy match tables,
so why are you doing this?

> +
> +     tsdata->vcc = devm_regulator_get(&client->dev, "vcc");
> +     if (IS_ERR(tsdata->vcc)) {
> +             error = PTR_ERR(tsdata->vcc);
> +             if (error != -EPROBE_DEFER)
> +                     dev_err(&client->dev,
> +                             "failed to request regulator: %d\n", error);
> +             return error;
> +     }
> +
> +     error = regulator_enable(tsdata->vcc);
> +     if (error < 0) {
> +             dev_err(&client->dev, "failed to enable vcc: %d\n", error);
> +             return error;
> +     }
> +
> +     error = devm_add_action_or_reset(&client->dev,
> +                                      hycon_hy46xx_disable_regulator,
> +                                      tsdata);
> +     if (error)
> +             return error;
> +
> +     tsdata->reset_gpio = devm_gpiod_get_optional(&client->dev,
> +                                                  "reset", GPIOD_OUT_LOW);
> +     if (IS_ERR(tsdata->reset_gpio)) {
> +             error = PTR_ERR(tsdata->reset_gpio);
> +             dev_err(&client->dev,
> +                     "Failed to request GPIO reset pin, error %d\n", error);
> +             return error;
> +     }
> +
> +     if (tsdata->reset_gpio) {
> +             usleep_range(5000, 6000);
> +             gpiod_set_value_cansleep(tsdata->reset_gpio, 1);
> +             usleep_range(5000, 6000);
> +             gpiod_set_value_cansleep(tsdata->reset_gpio, 0);
> +             msleep(1000);
> +     }
> +
> +     input = devm_input_allocate_device(&client->dev);
> +     if (!input) {
> +             dev_err(&client->dev, "failed to allocate input device.\n");
> +             return -ENOMEM;
> +     }
> +
> +     mutex_init(&tsdata->mutex);
> +     tsdata->client = client;
> +     tsdata->input = input;
> +
> +     hycon_hy46xx_get_defaults(&client->dev, tsdata);
> +     hycon_hy46xx_get_parameters(tsdata);
> +
> +     input->name = "Hycon Capacitive Touch";
> +     input->id.bustype = BUS_I2C;
> +     input->dev.parent = &client->dev;
> +
> +     input_set_abs_params(input, ABS_MT_POSITION_X,
> +                          0, -1, 0, 0);
> +     input_set_abs_params(input, ABS_MT_POSITION_Y,
> +                          0, -1, 0, 0);
> +
> +     touchscreen_parse_properties(input, true, &tsdata->prop);
> +
> +     error = input_mt_init_slots(input, HY46XX_MAX_SUPPORTED_POINTS,
> +                             INPUT_MT_DIRECT);
> +     if (error) {
> +             dev_err(&client->dev, "Unable to init MT slots.\n");
> +             return error;
> +     }
> +
> +     i2c_set_clientdata(client, tsdata);
> +
> +     irq_flags = irq_get_trigger_type(client->irq);
> +     if (irq_flags == IRQF_TRIGGER_NONE)
> +             irq_flags = IRQF_TRIGGER_FALLING;
> +     irq_flags |= IRQF_ONESHOT;

Just use IRQF_ONESHOT. The construct above is for compatibility when we
had drivers in the tree that were hard-coding trigger, and then we moved
to specify trigger in platform code/ACPI/DT and wanted to limit
regressions. Given this driver is brand new simply pass IRQF_ONESHOT
into devm_request_threaded_irq() below and call it a day.

> +
> +     error = devm_request_threaded_irq(&client->dev, client->irq,
> +                                     NULL, hycon_hy46xx_isr, irq_flags,
> +                                     client->name, tsdata);
> +     if (error) {
> +             dev_err(&client->dev, "Unable to request touchscreen IRQ.\n");
> +             return error;
> +     }
> +
> +     error = devm_device_add_group(&client->dev, &hycon_hy46xx_attr_group);
> +     if (error)
> +             return error;
> +
> +     error = input_register_device(input);
> +     if (error)
> +             return error;
> +
> +     dev_dbg(&client->dev,
> +             "HYCON HY46XX initialized: IRQ %d, Reset pin %d.\n",
> +             client->irq,
> +             tsdata->reset_gpio ? desc_to_gpio(tsdata->reset_gpio) : -1);
> +
> +     return 0;
> +}
> +
> +static const struct i2c_device_id hycon_hy46xx_id[] = {
> +     { .name = "hycon-hy4613" },
> +     { .name = "hycon-hy4614" },
> +     { .name = "hycon-hy4621" },
> +     { .name = "hycon-hy4623" },
> +     { .name = "hycon-hy4633" },
> +     { .name = "hycon-hy4635" },
> +     { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, hycon_hy46xx_id);
> +
> +static const struct of_device_id hycon_hy46xx_of_match[] = {
> +     { .compatible = "hycon,hycon-hy4613" },
> +     { .compatible = "hycon,hycon-hy4614" },
> +     { .compatible = "hycon,hycon-hy4621" },
> +     { .compatible = "hycon,hycon-hy4623" },
> +     { .compatible = "hycon,hycon-hy4633" },
> +     { .compatible = "hycon,hycon-hy4635" },
> +     { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, hycon_hy46xx_of_match);
> +
> +static struct i2c_driver hycon_hy46xx_driver = {
> +     .driver = {
> +             .name = "hycon_hy46xx",
> +             .of_match_table = hycon_hy46xx_of_match,
> +             .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +     },
> +     .id_table = hycon_hy46xx_id,
> +     .probe    = hycon_hy46xx_probe,
> +};
> +
> +module_i2c_driver(hycon_hy46xx_driver);
> +
> +MODULE_AUTHOR("Giulio Benetti <giulio.bene...@micronovasrl.com>");
> +MODULE_DESCRIPTION("HYCON HY46XX I2C Touchscreen Driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.25.1
> 

Thanks.

-- 
Dmitry

Reply via email to