Re: [PATCH] Input: add driver for TouchNetix aXiom touchscreen

[Kamel Bouhara <kamel.bouhara@bootlin.com>]

Le Mon, Sep 11, 2023 at 08:59:21AM +0200, Krzysztof Kozlowski a écrit :
> On 08/09/2023 17:32, Kamel Bouhara wrote:
> > Add a new driver for the TouchNetix's aXiom family of
> > multi-touch controller. This driver only support i2c
> > and can be later adapted for SPI and USB support.
> >
> > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > ---
> >  .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
>
>
> Thank you for your patch. There is something to discuss/improve.
>
> 1. Bindings are separate patches.
>
> I am surprised that you did not send this through your collegagues at
> Bootlin for some internal review. It would spot such easy things to fix.
>
> 2. Please use subject prefixes matching the subsystem. You can get them
> for example with `git log --oneline -- DIRECTORY_OR_FILE` on the
> directory your patch is touching.
>
> 3. Please run scripts/checkpatch.pl and fix reported warnings. Some
> warnings can be ignored, but the code here looks like it needs a fix.
> Feel free to get in touch if the warning is not clear.
>
> 4. Please use scripts/get_maintainers.pl to get a list of necessary
> people and lists to CC. It might happen, that command when run on an
> older kernel, gives you outdated entries. Therefore please be sure you
> base your patches on recent Linux kernel.
>
> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling. Performing review on untested code might be
> a waste of time, thus I will skip this patch entirely till you follow
> the process allowing the patch to be tested.
>
> Please kindly resend and include all necessary To/Cc entries.
>
> Limited review follows.

Hello Krzysztof,

Sorry for this late answer  and thanks for this not so limited review,
much appreciate :) !

>
>
> >  MAINTAINERS                                   |   7 +
> >  drivers/input/touchscreen/Kconfig             |  11 +
> >  drivers/input/touchscreen/Makefile            |   1 +
> >  drivers/input/touchscreen/axiom_core.c        | 382 ++++++++++++++++++
> >  drivers/input/touchscreen/axiom_core.h        | 140 +++++++
> >  drivers/input/touchscreen/axiom_i2c.c         | 349 ++++++++++++++++
> >  7 files changed, 892 insertions(+)
> >  create mode 100644 drivers/input/touchscreen/axiom_core.c
> >  create mode 100644 drivers/input/touchscreen/axiom_core.h
> >  create mode 100644 drivers/input/touchscreen/axiom_i2c.c
> >
> > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> > index 573578db9509..b0a3ed451f15 100644
> > --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> > +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> > @@ -175,6 +175,8 @@ patternProperties:
> >    "^awinic,.*":
> >      description: Shanghai Awinic Technology Co., Ltd.
> >    "^axentia,.*":
> > +    description: TouchNetix
> > +  "^axiom,.*":
> >      description: Axentia Technologies AB
> >    "^axis,.*":
> >      description: Axis Communications AB
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 389fe9e38884..43add48257d8 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3373,6 +3373,13 @@ W:	https://ez.analog.com/linux-software-drivers
> >  F:	Documentation/devicetree/bindings/hwmon/adi,axi-fan-control.yaml
> >  F:	drivers/hwmon/axi-fan-control.c
> >
> > +AXIOM I2C TOUCHSCREEN DRIVER
> > +M:	Kamel Bouhara <kamel.bouhara@bootlin.com>
> > +L:	linux-input@vger.kernel.org
> > +S:	Maintained
> > +F:	drivers/input/touchscreen/axiom_core.c
> > +F:	drivers/input/touchscreen/axiom_i2.c
> > +
> >  AXXIA I2C CONTROLLER
> >  M:	Krzysztof Adamski <krzysztof.adamski@nokia.com>
> >  L:	linux-i2c@vger.kernel.org
> > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > index e3e2324547b9..08a770a0c5e5 100644
> > --- a/drivers/input/touchscreen/Kconfig
> > +++ b/drivers/input/touchscreen/Kconfig
> > @@ -150,6 +150,17 @@ config TOUCHSCREEN_AUO_PIXCIR
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called auo-pixcir-ts.
> >
> > +config TOUCHSCREEN_AXIOM_I2C
> > +	tristate "AXIOM based multi-touch panel controllers"
> > +	help
> > +	  Say Y here if you have a axiom touchscreen connected to
> > +	  your system.
> > +
> > +	  If unsure, say N.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called axiom_i2c.
> > +
> >  config TOUCHSCREEN_BU21013
> >  	tristate "BU21013 based touch panel controllers"
> >  	depends on I2C
> > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > index 62bd24f3ac8e..59a3234ddb09 100644
> > --- a/drivers/input/touchscreen/Makefile
> > +++ b/drivers/input/touchscreen/Makefile
> > @@ -18,6 +18,7 @@ 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_AUO_PIXCIR)	+= auo-pixcir-ts.o
> > +obj-$(CONFIG_TOUCHSCREEN_AXIOM_I2C)	+= axiom_core.o axiom_i2c.o
> >  obj-$(CONFIG_TOUCHSCREEN_BU21013)	+= bu21013_ts.o
> >  obj-$(CONFIG_TOUCHSCREEN_BU21029)	+= bu21029_ts.o
> >  obj-$(CONFIG_TOUCHSCREEN_CHIPONE_ICN8318)	+= chipone_icn8318.o
> > diff --git a/drivers/input/touchscreen/axiom_core.c b/drivers/input/touchscreen/axiom_core.c
> > new file mode 100644
> > index 000000000000..d381afd7fb84
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/axiom_core.c
> > @@ -0,0 +1,382 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * TouchNetix aXiom Touchscreen Driver
> > + *
> > + * Copyright (C) 2020-2023 TouchNetix Ltd.
> > + *
> > + * Author(s): Mark Satterthwaite <mark.satterthwaite@touchnetix.com>
> > + *            Pedro Torruella <pedro.torruella@touchnetix.com>
> > + *            Bart Prescott <bartp@baasheep.co.uk>
> > + *            Hannah Rossiter <hannah.rossiter@touchnetix.com>
> > + *
> > + */
> > +
> > +#include <linux/crc16.h>
> > +#include <linux/device.h>
> > +#include <linux/input.h>
> > +#include <linux/input/mt.h>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +
> > +#include "axiom_core.h"
> > +
> > +/**
> > + * Decodes and populates the local u31 structure.
> > + * Given a buffer of data read from page 0 of u31 in an aXiom device.
> > + */
> > +void axiom_get_dev_info(struct axiom_data *ts, char *info)
> > +{
> > +	struct axiom_devinfo *devinfo = &ts->devinfo;
> > +
> > +	if (devinfo) {
> > +		devinfo->bootloader_mode = ((info[1] & 0x80) != 0) ? 1 : 0;
> > +		devinfo->device_id = ((info[1] & 0x7F) << 8) | info[0];
> > +		devinfo->fw_minor = info[2];
> > +		devinfo->fw_major = info[3];
> > +		devinfo->fw_info_extra = (info[4]) | (info[5] << 8);
> > +		devinfo->bootloader_fw_ver_minor = info[6];
> > +		devinfo->bootloader_fw_ver_major = info[7];
> > +		devinfo->jedec_id = (info[8]) | (info[9] << 8);
> > +		devinfo->num_usages = info[10];
> > +		devinfo->silicon_revision = info[11];
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_get_dev_info);
>
> Nope

I guess this means no need to export the function ?

I agree this is not required currently as we only have support the i2c
variant.

>
> > +
> > +/**
> > + * Decodes and populates the local Usage Table.
> > + * Given a buffer of data read from page 1 onwards of u31 from an aXiom
> > + * device.
> > + */
> > +char axiom_populate_usage_table(struct axiom_data *ts, char *rx_data)
> > +{
> > +	u32 usage_id = 0;
> > +	char max_report_len = 0;
> > +	struct axiom_devinfo *device_info;
> > +	struct usage_entry *usage_table;
> > +
> > +	device_info = &ts->devinfo;
> > +	usage_table = ts->usage_table;
> > +
> > +	for (usage_id = 0; usage_id < device_info->num_usages; usage_id++) {
> > +		u16 offset = (usage_id * U31_BYTES_PER_USAGE);
> > +		char id = rx_data[offset + 0];
> > +		char start_page = rx_data[offset + 1];
> > +		char num_pages = rx_data[offset + 2];
> > +		char max_offset = ((rx_data[offset + 3] & 0x7F) + 1) * 2;
> > +
> > +		/* Store the entry into the usage table */
> > +		usage_table[usage_id].id = id;
> > +		usage_table[usage_id].is_report = ((num_pages == 0) ? 1 : 0);
> > +		usage_table[usage_id].start_page = start_page;
> > +		usage_table[usage_id].num_pages = num_pages;
> > +
> > +		dev_dbg(ts->pdev, "Usage %2u Info: %*ph\n", usage_id,
> > +			U31_BYTES_PER_USAGE, &rx_data[offset]);
> > +
> > +		/* Identify the max report length the module will receive */
> > +		if ((usage_table[usage_id].is_report)
> > +		    && (max_offset > max_report_len))
> > +			max_report_len = max_offset;
> > +	}
> > +	ts->usage_table_populated = true;
> > +
> > +	return max_report_len;
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_populate_usage_table);
> > +
> > +/**
> > + * Translate usage/page/offset triplet into physical address.
> > + *
> > + * @usage: groups of registers
> > + * @page: page to which the usage belongs to offset
> > + * @offset of the usage
> > + */
> > +u16
> > +usage_to_target_address(struct axiom_data *ts, char usage, char page,
> > +			char offset)
> > +{
> > +	struct axiom_devinfo *device_info;
> > +	struct usage_entry *usage_table;
> > +	u16 target_address;
> > +	u32 i;
> > +
> > +	device_info = &ts->devinfo;
> > +	usage_table = ts->usage_table;
> > +
> > +	/* At the moment the convention is that u31 is always at physical address 0x0 */
> > +	if (!ts->usage_table_populated && (usage == DEVINFO_USAGE_ID)) {
> > +		target_address = ((page << 8) + offset);
> > +	} else if (ts->usage_table_populated) {
> > +		for (i = 0; i < device_info->num_usages; i++) {
> > +			if (usage_table[i].id == usage) {
> > +				if (page < usage_table[i].num_pages) {
> > +					target_address =
> > +					    ((usage_table[i].start_page + page) << 8) + offset;
> > +				} else {
> > +					target_address = 0xFFFF;
> > +					dev_err(ts->pdev,
> > +						"Invalid usage table! usage: %u, page: %u, offset: %u\n",
> > +						usage, page, offset);
> > +				}
> > +				break;
> > +			}
> > +		}
> > +	} else {
> > +		target_address = 0xFFFF;
> > +		dev_err(ts->pdev, "Unpopulated usage table for usage: %u\n",
> > +			usage);
> > +	}
> > +
> > +	dev_dbg(ts->pdev, "target_address is 0x%04x for usage: %u page %u\n",
> > +		target_address, usage, page);
> > +
> > +	return target_address;
> > +}
> > +EXPORT_SYMBOL_GPL(usage_to_target_address);
>
> Nope, drop.
>
> > +
> > +void axiom_remove(struct axiom_data *ts)
> > +{
> > +	if (ts->usage_table)
> > +		ts->usage_table_populated = false;
> > +
> > +	if (ts->input_dev)
> > +		input_unregister_device(ts->input_dev);
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_remove);
>
> Nope, drop.
>
> > +
>
>
> > +
> > +/**
> > + * Take a raw buffer with u41 report data and decode it.
> > + * Also generate input events if needed.
> > + * @rx_buf: ptr to a byte array [0]: Usage number [1]: Status LSB [2]: Status MSB
> > + */
> > +void axiom_process_u41_report(struct axiom_data *ts, char *rx_buf)
> > +{
> > +	struct axiom_target_report target;
> > +	struct input_dev *input_dev = ts->input_dev;
> > +	bool update_done = false;
> > +	u16 target_status;
> > +	u32 i;
> > +
> > +	if (rx_buf[0] != 0x41) {
> > +		dev_err(ts->pdev,
> > +			"Data in buffer does not have expected u41 format.\n");
> > +		return;
> > +	}
> > +
> > +	target_status = ((rx_buf[1]) | (rx_buf[2] << 8));
> > +
> > +	for (i = 0; i < U41_MAX_TARGETS; i++) {
> > +		target.index = i;
> > +		target.present = ((target_status & (1 << i)) != 0) ? 1 : 0;
> > +		target.x = (rx_buf[(i * 4) + 3]) | (rx_buf[(i * 4) + 4] << 8);
> > +		target.y = (rx_buf[(i * 4) + 5]) | (rx_buf[(i * 4) + 6] << 8);
> > +		target.z = (s8) (rx_buf[i + 43]);
> > +		update_done |= axiom_process_u41_report_target(ts, &target);
> > +	}
> > +
> > +	if (update_done)
> > +		input_sync(input_dev);
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_process_u41_report);
>
> Nope, drop.
>
> > +
> > +void axiom_process_u46_report(struct axiom_data *ts, char *rx_buf)
> > +{
> > +	struct input_dev *input_dev = ts->input_dev;
> > +	u16 aux_value;
> > +	u32 event_value;
> > +	u32 i = 0;
> > +
> > +	for (i = 0; i < U46_AUX_CHANNELS; i++) {
> > +		aux_value =
> > +		    ((rx_buf[(i * 2) + 2] << 8) | (rx_buf[(i * 2) + 1])) &
> > +		    U46_AUX_MASK;
> > +		event_value = (i << 16) | (aux_value);
> > +		input_event(input_dev, EV_MSC, MSC_RAW, event_value);
> > +	}
> > +
> > +	input_mt_sync(input_dev);
> > +	input_sync(input_dev);
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_process_u46_report);
>
> Nope, drop.
>
> Clean your driver before sending upstream. :(
>
> > +
> > +/**
> > + * Support function to axiom_process_report.
> > + * It validates the crc and multiplexes the axiom reports to the appropriate
> > + * report handler
> > + */
> > +void axiom_process_report(struct axiom_data *ts, char *report_data)
> > +{
> > +	struct device *pdev = ts->pdev;
> > +	char usage = report_data[1];
> > +	u16 crc_report;
> > +	u16 crc_calc;
> > +	char len;
> > +
> > +	if ((report_data[0] & COMMS_OVERFLOW_MSK) != 0)
> > +		ts->report_overflow_counter++;
> > +
> > +	len = (report_data[0] & COMMS_REPORT_LEN_MSK) << 1;
> > +	if (len == 0) {
> > +		dev_err(pdev, "Zero length report discarded.\n");
> > +		return;
> > +	}
> > +
> > +	dev_dbg(pdev, "Payload Data %*ph\n", len, report_data);
> > +
> > +	/* Validate the report CRC */
> > +	crc_report = (report_data[len - 1] << 8) | (report_data[len - 2]);
> > +	/* Length is in 16 bit words and remove the size of the CRC16 itself */
> > +	crc_calc = crc16(0, report_data, (len - 2));
> > +
> > +	if (crc_calc != crc_report) {
> > +		dev_err(pdev,
> > +			"CRC mismatch! Expected: %#x, Calculated CRC: %#x.\n",
> > +			crc_report, crc_calc);
> > +		return;
> > +	}
> > +
> > +	switch (usage) {
> > +	case USAGE_2DCTS_REPORT_ID:
> > +		axiom_process_u41_report(ts, &report_data[1]);
> > +		break;
> > +
> > +	case USAGE_2AUX_REPORT_ID:
> > +		/* This is an aux report (force) */
> > +		axiom_process_u46_report(ts, &report_data[1]);
> > +		break;
> > +
> > +	case USAGE_2HB_REPORT_ID:
> > +		/* This is a heartbeat report */
> > +		break;
> > +
> > +	default:
> > +		break;
> > +	}
> > +
> > +	ts->report_counter++;
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_process_report);
>
> Nope, drop.
>
> > +
> > +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
> > +MODULE_DESCRIPTION("aXiom touchscreen core logic");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("axiom");
>
> ???? How can you have multiple drivers with the same alias? Even if your
> Makefile allowed it (but it doesn't!), it would not make sense.
>
>
> > diff --git a/drivers/input/touchscreen/axiom_core.h b/drivers/input/touchscreen/axiom_core.h
> > new file mode 100644
> > index 000000000000..f129d28671ff
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/axiom_core.h
> > @@ -0,0 +1,140 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * TouchNetix aXiom Touchscreen Driver
> > + *
> > + * Copyright (C) 2020-2023 TouchNetix Ltd.
> > + *
> > + * Author(s): Mark Satterthwaite <mark.satterthwaite@touchnetix.com>
> > + *            Pedro Torruella <pedro.torruella@touchnetix.com>
> > + *            Bart Prescott <bartp@baasheep.co.uk>
> > + *            Hannah Rossiter <hannah.rossiter@touchnetix.com>
> > + *
> > + * 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.
>
> OK, so this is some old, vendor crappy code. We do not have it since
> some years!

Yes :).

>
> > + *
>
> ...
>
> > +
> > +static void axiom_i2c_poll(struct input_dev *input_dev)
> > +{
> > +	struct axiom_data *ts = input_get_drvdata(input_dev);
> > +	char *rx_data = ts->rx_buf;
> > +
> > +	axiom_i2c_read(ts->client, REPORT_USAGE_ID, 0, rx_data,
> > +		       ts->max_report_len);
> > +	axiom_process_report(ts, rx_data);
> > +}
> > +
> > +/**
> > + * Retrieve, store and print the axiom device information
> > + */
> > +int axiom_discover(struct axiom_data *ts)
>
> This has to be static.

ok.

>
> > +{
> > +	char *rx_data = &ts->rx_buf[0];
> > +	struct device *pdev = ts->pdev;
> > +	int ret;
> > +
> > +	/* First the first page of u31 to get the device information and */
> > +	/* the number of usages */
> > +	ret =
> > +	    axiom_i2c_read(ts->client, DEVINFO_USAGE_ID, 0, rx_data,
> > +			   AX_U31_PAGE0_LENGTH);
> > +	if (ret)
> > +		return ret;
> > +
> > +	axiom_get_dev_info(ts, rx_data);
> > +
> > +	dev_dbg(pdev, "Data Decode:\n");
> > +	dev_dbg(pdev, "  Bootloader Mode: %u\n", ts->devinfo.bootloader_mode);
> > +	dev_dbg(pdev, "  Device ID      : %04x\n", ts->devinfo.device_id);
> > +	dev_dbg(pdev, "  Firmware Rev   : %02x.%02x\n", ts->devinfo.fw_major,
> > +		ts->devinfo.fw_minor);
> > +	dev_dbg(pdev, "  Bootloader Rev : %02x.%02x\n",
> > +		ts->devinfo.bootloader_fw_ver_major,
> > +		ts->devinfo.bootloader_fw_ver_minor);
> > +	dev_dbg(pdev, "  FW Extra Info  : %04x\n", ts->devinfo.fw_info_extra);
> > +	dev_dbg(pdev, "  Silicon        : %02x\n", ts->devinfo.jedec_id);
> > +	dev_dbg(pdev, "  Num Usages     : %04x\n", ts->devinfo.num_usages);
> > +
> > +	/* Read the second page of u31 to get the usage table */
> > +	ret = axiom_i2c_read(ts->client, DEVINFO_USAGE_ID, 1, rx_data,
> > +			     (U31_BYTES_PER_USAGE * ts->devinfo.num_usages));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ts->max_report_len = axiom_populate_usage_table(ts, rx_data);
> > +	dev_dbg(pdev, "Max Report Length: %u\n", ts->max_report_len);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_discover);
>
> Nope.
>
> > +
> > +/**
> > + * Rebaseline the touchscreen, effectively zero-ing it
> > + */
> > +void axiom_rebaseline(struct axiom_data *ts)
>
> Missing static.
>
> > +{
> > +	struct device *pdev = ts->pdev;
> > +	char buffer[8] = { 0 };
> > +	int ret;
> > +
> > +	memset(buffer, 0, sizeof(buffer));
> > +
> > +	buffer[0] = REBASELINE_CMD;
> > +
> > +	ret = axiom_i2c_write(ts->client, 0x02, 0, buffer, sizeof(buffer));
> > +	if (ret)
> > +		dev_err(pdev, "Rebaseline failed\n");
> > +
> > +	dev_info(pdev, "Capture Baseline Requested\n");
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_rebaseline);
>
> Nope.
>
> > +
> > +static irqreturn_t axiom_irq(int irq, void *handle)
> > +{
> > +	struct axiom_data *ts = handle;
> > +	u8 *rx_data = &ts->rx_buf[0];
> > +
> > +	axiom_i2c_read(ts->client, REPORT_USAGE_ID, 0, rx_data, ts->max_report_len);
> > +	axiom_process_report(ts, rx_data);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int axiom_i2c_probe(struct i2c_client *client)
> > +{
> > +	struct axiom_data *ts;
> > +	struct device *pdev = &client->dev;
> > +	struct input_dev *input_dev;
> > +	int error;
> > +	int target;
> > +
> > +	ts = devm_kzalloc(pdev, sizeof(*ts), GFP_KERNEL);
> > +	if (!ts)
> > +		return -ENOMEM;
> > +
> > +	ts->irq_gpio = devm_gpiod_get_optional(pdev, "irq", GPIOD_IN);
> > +	if (IS_ERR(ts->irq_gpio)) {
> > +		error = PTR_ERR(ts->irq_gpio);
> > +		dev_err(pdev, "failed to request irq GPIO: %d", error);
>
> Heh... syntax is: return dev_err_probe()
>
> > +		return error;
> > +	}
> > +
> > +	ts->reset_gpio = devm_gpiod_get_optional(pdev, "reset", GPIOD_OUT_HIGH);
>
> You do not use this GPIO at all. Dead code, especially that you keep the
> device in reset. This was absolutely never tested (unless with incorrect
> DTS...).

Actually this has been exclusively tested on a X86 platform with ACPI
overrides however this still need to be fixed indeed.

>
> > +	if (IS_ERR(ts->reset_gpio)) {
> > +		error = PTR_ERR(ts->reset_gpio);
>
> return dev_err_probe()
>
> > +		dev_err(pdev, "failed to request reset GPIO: %d", error);
> > +		return error;
> > +	}
> > +
> > +	if (ts->irq_gpio) {
> > +		error =
> > +		    devm_request_threaded_irq(pdev, client->irq, NULL,
> > +					      axiom_irq,
> > +					      IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> > +					      "axiom_irq", ts);
> > +		if (error != 0) {
> > +			dev_err(pdev, "Failed to request IRQ %u (error: %d)\n",
> > +				client->irq, error);
>
> return dev_err_probe()
>
> > +			return error;
> > +		}
> > +	}
> > +
> > +	ts->client = client;
> > +	ts->pdev = pdev;
> > +	ts->usage_table_populated = false;
> > +
> > +	i2c_set_clientdata(client, ts);
> > +
> > +	axiom_discover(ts);
> > +	axiom_rebaseline(ts);
> > +
> > +	input_dev = devm_input_allocate_device(ts->pdev);
> > +	if (!input_dev) {
> > +		dev_err(pdev, "Failed to allocate input device\n");
>
> I don't think we print memory allocation failures.
>
> > +		return -ENOMEM;
> > +	}
> > +
> > +	input_dev->name = "TouchNetix aXiom Touchscreen";
> > +	input_dev->phys = "input/axiom_ts";
> > +
> > +	/* Single Touch */
> > +	input_set_abs_params(input_dev, ABS_X, 0, 65535, 0, 0);
> > +	input_set_abs_params(input_dev, ABS_Y, 0, 65535, 0, 0);
> > +
> > +	/* Multi Touch */
> > +	/* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
> > +	input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0, 65535, 0, 0);
> > +	/* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
> > +	input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0, 65535, 0, 0);
> > +	input_set_abs_params(input_dev, ABS_MT_TOOL_TYPE, 0, MT_TOOL_MAX, 0, 0);
> > +	input_set_abs_params(input_dev, ABS_MT_DISTANCE, 0, 127, 0, 0);
> > +	input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 127, 0, 0);
> > +
> > +	/* Registers the axiom device as a touch screen instead of as a mouse pointer */
> > +	input_mt_init_slots(input_dev, U41_MAX_TARGETS, INPUT_MT_DIRECT);
> > +
> > +	input_set_capability(input_dev, EV_KEY, BTN_LEFT);
> > +
> > +	/* Enables the raw data for up to 4 force channels to be sent to the */
> > +	/* input subsystem */
> > +	set_bit(EV_REL, input_dev->evbit);
> > +	set_bit(EV_MSC, input_dev->evbit);
> > +	/* Declare that we support "RAW" Miscellaneous events */
> > +	set_bit(MSC_RAW, input_dev->mscbit);
> > +
> > +	if (!ts->irq_gpio) {
> > +		error = input_setup_polling(input_dev, axiom_i2c_poll);
> > +		if (error) {
> > +			dev_err(ts->pdev, "Unable to set up polling: %d\n",
> > +				error);
> > +			return error;
> > +		}
> > +		input_set_poll_interval(input_dev, POLL_INTERVAL_DEFAULT_MS);
> > +	}
> > +
> > +	error = input_register_device(input_dev);
> > +	if (error != 0) {
> > +		dev_err(ts->pdev,
> > +			"Could not register with Input Sub-system., error=%d\n",
> > +			error);
> > +		return error;
> > +	}
> > +
> > +	ts->input_dev = input_dev;
> > +	input_set_drvdata(ts->input_dev, ts);
> > +
> > +	dev_info(pdev, "AXIOM: I2C driver registered with Input Sub-System.\n");
>
> Drop silly tracing messages.
>
> > +
> > +	/* Delay just a smidge before enabling the IRQ */
> > +	udelay(10);
>
> ??? So where is the IRQ being enabled? This is confusing.

This need to move before IRQ is enabled.

>
> > +
> > +	/* Ensure that all reports are initialised to not be present. */
> > +	for (target = 0; target < U41_MAX_TARGETS; target++)
> > +		ts->targets[target].state = TARGET_STATE_NOT_PRESENT;
> > +
> > +	return 0;
> > +}
> > +
> > +static void axiom_i2c_remove(struct i2c_client *client)
> > +{
> > +	struct axiom_data *ts = i2c_get_clientdata(client);
> > +
> > +	axiom_remove(ts);
> > +}
> > +
> > +static const struct i2c_device_id axiom_i2c_id_table[] = {
> > +	{"ax54a"},
> > +	{},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, axiom_i2c_id_table);
> > +
> > +static const struct of_device_id axiom_i2c_dt_ids[] = {
> > +	{
> > +	 .compatible = "axiom,ax54a",
>
> That's not documented. And you would know it if you run basic checks
> before sending patches upstream.
>

I was actually suprised to found not all touchscreen drivers have
a documented binding and I know understand this is bad :) !

> Please run scripts/checkpatch.pl and fix reported warnings. Some
> warnings can be ignored, but the code here looks like it needs a fix.
> Feel free to get in touch if the warning is not clear.

OK, actually those are the only checkpatch warnings reported:

$ ./scripts/checkpatch.pl 0001-Input-add-driver-for-TouchNetix-aXiom-touchscreen.patch
WARNING: DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.rst

WARNING: DT compatible string "axiom,ax54a" appears un-documented -- check ./Documentation/devicetree/bindings/
#954: FILE: drivers/input/touchscreen/axiom_i2c.c:326:
+        .compatible = "axiom,ax54a",

total: 0 errors, 2 warnings, 916 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

>
> > +	 .data = "axiom",
>
> Why?

Actually not used, more clean up required.

>
> That's not readable. Indent it like in other drivers.
>
> > +	 },
> > +	{}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, axiom_i2c_dt_ids);
> > +
> > +static struct i2c_driver axiom_i2c_driver = {
> > +	.driver = {
> > +		   .name = "axiom_i2c",
> > +		   .of_match_table = of_match_ptr(axiom_i2c_dt_ids),
>
> Drop of_match_ptr(), it causes warnings in your code...

Ok.

>
> > +		   },
> > +	.id_table = axiom_i2c_id_table,
> > +	.probe = axiom_i2c_probe,
> > +	.remove = axiom_i2c_remove,
> > +};
> > +
> > +module_i2c_driver(axiom_i2c_driver);
> > +
> > +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
> > +MODULE_DESCRIPTION("aXiom touchscreen I2C bus driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("axiom");
>
> Nope, you cannot have such alias and it is not even needed.

Ack, thanks !

>
>
> Best regards,
> Krzysztof
>

--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com