Received-SPF: pass (google.com: domain of linux-kernel+bounces-46886-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 31 Jan 2024 18:14:41 +0100
From: Kamel Bouhara <kamel.bouhara@bootlin.com>
To: Jeff LaBundy <jeff@labundy.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Henrik Rydberg <rydberg@bitmath.org>, linux-input@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	Marco Felsch <m.felsch@pengutronix.de>,
	catalin.popescu@leica-geosystems.com,
	mark.satterthwaite@touchnetix.com, bartp@baasheep.co.uk,
	hannah.rossiter@touchnetix.com,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Gregory Clement <gregory.clement@bootlin.com>,
	bsp-development.geo@leica-geosystems.com
Subject: Re: [PATCH v5 3/3] Input: Add TouchNetix axiom i2c touchscreen driver
Message-ID: <20240131171441.GA3005@kb-xps>
References: <20231211121430.1689139-1-kamel.bouhara@bootlin.com>
 <20231211121430.1689139-4-kamel.bouhara@bootlin.com>
 <ZY5An58Rffrcpfpn@nixie71>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZY5An58Rffrcpfpn@nixie71>

On Thu, Dec 28, 2023 at 09:44:31PM -0600, Jeff LaBundy wrote:
> Hi Kamel,
>

Hello Jeff,

[...]

> > +
> > +/*
> > + * Support function to axiom_process_u41_report.
> > + * Generates input-subsystem events for every target.
> > + * After calling this function the caller shall issue
> > + * a Sync to the input sub-system.
> > + */
> > +static bool axiom_process_u41_report_target(struct axiom_data *ts,
> > +					    struct axiom_target_report *target)
> > +{
> > +	struct input_dev *input_dev = ts->input_dev;
> > +	struct axiom_u41_target *target_prev_state;
> > +	enum axiom_target_state current_state;
> > +	bool update = false;
> > +	int slot;
> > +
> > +	/* Verify the target index */
> > +	if (target->index >= AXIOM_U41_MAX_TARGETS) {
> > +		dev_dbg(ts->dev, "Invalid target index! %u\n", target->index);
>
> Should this be dev_err()?
>
> > +		return false;
> > +	}
> > +
> > +	target_prev_state = &ts->targets[target->index];
> > +
> > +	current_state = AXIOM_TARGET_STATE_NOT_PRESENT;
> > +
> > +	if (target->present) {
> > +		if (target->z >= 0)
> > +			current_state = AXIOM_TARGET_STATE_TOUCHING;
> > +		else if (target->z > AXIOM_PROX_LEVEL && target->z < 0)
> > +			current_state = AXIOM_TARGET_STATE_HOVER;
> > +		else if (target->z == AXIOM_PROX_LEVEL)
> > +			current_state = AXIOM_TARGET_STATE_PROX;
> > +	}
> > +
> > +	if (target_prev_state->state == current_state &&
> > +	    target_prev_state->x == target->x &&
> > +	    target_prev_state->y == target->y &&
> > +	    target_prev_state->z == target->z) {
> > +		return false;
> > +	}
>
> No need for curly braces here; please refer to the kernel style guidelines.
>
> > +
> > +	slot = target->index;
> > +
> > +	dev_dbg(ts->dev, "U41 Target T%u, slot:%u present:%u, x:%u, y:%u, z:%d\n",
> > +		target->index, slot, target->present,
> > +		target->x, target->y, target->z);
> > +
> > +	switch (current_state) {
> > +	case AXIOM_TARGET_STATE_NOT_PRESENT:
> > +	case AXIOM_TARGET_STATE_PROX:
> > +		if (!target_prev_state->insert)
> > +			break;
> > +		update = true;
> > +		target_prev_state->insert = false;
> > +		input_mt_slot(input_dev, slot);
> > +
> > +		if (!slot)
> > +			input_report_key(input_dev, BTN_TOUCH, 0);
> > +
> > +		input_mt_report_slot_inactive(input_dev);
> > +		/*
> > +		 * make sure the previous coordinates are
> > +		 * all off screen when the finger comes back
> > +		 */
> > +		target->x = 65535;
> > +		target->y = 65535;
> > +		target->z = AXIOM_PROX_LEVEL;
> > +		break;
> > +	case AXIOM_TARGET_STATE_HOVER:
> > +	case AXIOM_TARGET_STATE_TOUCHING:
> > +		target_prev_state->insert = true;
> > +		update = true;
> > +		input_mt_slot(input_dev, slot);
> > +		input_report_abs(input_dev, ABS_MT_TRACKING_ID, slot);
> > +		input_report_abs(input_dev, ABS_MT_POSITION_X, target->x);
> > +		input_report_abs(input_dev, ABS_X, target->x);
>
> You do not need to explicitly report ABS_X and ABS_Y values, as calling
> input_mt_sync_frame() effectively takes care of this by way of pointer
> emulation.
>

After double checking/testing this, it doesn't seems to
report ABS_X/Y values anymore, are you sure about this ? Maybe I missed
some extra flag in input_mt_init_slots() for that ?

Regards,

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