Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 11 May 2020 19:35:46 -0700
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     Jiada Wang <jiada_wang@mentor.com>
Cc:     nick@shmanahar.org, jikos@kernel.org,
        benjamin.tissoires@redhat.com, bsz@semihalf.com,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
        erosca@de.adit-jv.com, Andrew_Gabbasov@mentor.com
Subject: Re: [PATCH v11 08/56] Input: atmel_mxt_ts - implement T15 Key Array
 support
Message-ID: <20200512023546.GB89269@dtor-ws>
References: <20200508055656.96389-1-jiada_wang@mentor.com>
 <20200508055656.96389-9-jiada_wang@mentor.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200508055656.96389-9-jiada_wang@mentor.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, May 07, 2020 at 10:56:08PM -0700, Jiada Wang wrote:
> From: Nick Dyer <nick.dyer@itdev.co.uk>
> 
> There is a key array object in many maXTouch chips which allows some X/Y
> lines to be used as a key array. This patch maps them to a series of keys
> which may be configured in a platform data array.
> 
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> Acked-by: Benson Leung <bleung@chromium.org>
> Acked-by: Yufeng Shen <miletus@chromium.org>
> (cherry picked from ndyer/linux/for-upstream commit 15bb074b5abf3a101f7b79544213f1c110ea4cab)
> [gdavis: Resolve forward port conflicts due to applying upstream
> 	 commit 96a938aa214e ("Input: atmel_mxt_ts - remove platform
> 	 data support").]
> Signed-off-by: George G. Davis <george_davis@mentor.com>
> [jiada: Fix compilation warning]
> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 85 ++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index df2e0ba76e63..d05249b02781 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -314,6 +314,9 @@ struct mxt_data {
>  	struct mxt_dbg dbg;
>  	struct gpio_desc *reset_gpio;
>  	bool use_retrigen_workaround;
> +	unsigned long t15_keystatus;
> +	int t15_num_keys;
> +	const unsigned int *t15_keymap;
>  
>  	/* Cached parameters from object table */
>  	u16 T5_address;
> @@ -324,6 +327,8 @@ struct mxt_data {
>  	u16 T71_address;
>  	u8 T9_reportid_min;
>  	u8 T9_reportid_max;
> +	u8 T15_reportid_min;
> +	u8 T15_reportid_max;
>  	u16 T18_address;
>  	u8 T19_reportid;
>  	u8 T42_reportid_min;
> @@ -987,6 +992,38 @@ static void mxt_proc_t100_message(struct mxt_data *data, u8 *message)
>  	data->update_input = true;
>  }
>  
> +static void mxt_proc_t15_messages(struct mxt_data *data, u8 *msg)
> +{
> +	struct input_dev *input_dev = data->input_dev;
> +	struct device *dev = &data->client->dev;
> +	int key;
> +	bool curr_state, new_state;
> +	bool sync = false;
> +	unsigned long keystates = le32_to_cpu((__force __le32)msg[2]);

?

It is a byte. Just say

	unsigned long keystates = msg[2];

> +
> +	for (key = 0; key < data->t15_num_keys; key++) {
> +		curr_state = test_bit(key, &data->t15_keystatus);
> +		new_state = test_bit(key, &keystates);
> +
> +		if (!curr_state && new_state) {
> +			dev_dbg(dev, "T15 key press: %u\n", key);
> +			__set_bit(key, &data->t15_keystatus);
> +			input_event(input_dev, EV_KEY,
> +				    data->t15_keymap[key], 1);
> +			sync = true;
> +		} else if (curr_state && !new_state) {
> +			dev_dbg(dev, "T15 key release: %u\n", key);
> +			__clear_bit(key, &data->t15_keystatus);
> +			input_event(input_dev, EV_KEY,
> +				    data->t15_keymap[key], 0);
> +			sync = true;
> +		}
> +	}
> +
> +	if (sync)
> +		input_sync(input_dev);

I wonder if the following is not simpler:

	unsigned long changed_keys;
	...

	changed_keys = keystates ^ data->t15_keystatus;
	for_each_set_bit(key, &changed_keys, data->t15_num_keys) {
		pressed = test_bit(key, &keystates);
		input_event(input_dev, EV_KEY,
			    data->t15_keymap[key], pressed);
		dev_dbg(dev, "T15 key %s: %u\n",
			pressed ? "press" : "release", key);
	}

	if (changed_keys)
		input_sync(input_dev);

	 data->t15_keystatus = keystates;

...

> +	if (device_property_present(dev, buttons_property)) {
> +		n_keys = device_property_read_u32_array(dev, buttons_property,
> +							NULL, 0);
> +		if (n_keys <= 0) {
> +			error = n_keys < 0 ? n_keys : -EINVAL;
> +			dev_err(dev, "invalid/malformed '%s' property: %d\n",
> +				buttons_property, error);
> +			return error;
> +		}
> +
> +		buttonmap = devm_kmalloc_array(dev, n_keys, sizeof(*buttonmap),
> +					       GFP_KERNEL);
> +		if (!buttonmap)
> +			return -ENOMEM;

So it is 8 keys max, I'd simply embed this into data. On 64 bit arch it
will occupy the same space as the pointer you use to reference it.

Can you also validate that we do not get too many keys in DT?

Also, set keycode/keycodemax/keycodesize in input device so userspace
can adjust keymap if needed?

Thanks.

-- 
Dmitry