Re: [PATCH RFC v2 3/5] Input: melfas-mip4 - add support for touchkey input events

[Kaustabh Chakraborty <kauschluss@disroot.org>]

On 2025-06-29 05:12, Dmitry Torokhov wrote:
> Hi Kaustabh,
> 
> On Fri, Jun 13, 2025 at 01:11:35AM +0530, Kaustabh Chakraborty wrote:
>> The MIP4 protocol are supposed to support touchscreens, touchkeys, and
>> combo-devices. The driver handles touchscreen events, but touchkey
>> events are unimplemented.
> 
> I am confused, because I clearly see the driver parsing and forwarding
> key events. It appears that this patch adds the ability to set the
> keymap via device tree instead of relying on userspace to load it.

Yeah, you're correct. Though I'm not sure this is the correct way to do
what I'm trying to do. What I want is a driver which will drive both
touchscreen and pure-touchkey devices.

I feel like a separate driver is how it should be done, although the
protocol is same and it can be done here...

I had tried to get some input (hence the RFC) about this but I couldn't.
Either my wording wasn't clear or I haven't been able to communicate it
properly.

Sorry for bringing this out of the blue in a thread.

> 
> Please adjust the patch description.
> 
>> 
>> Implement them. If compiled with devicetree support, the driver 
>> attempts
>> to retrieve keycodes from a property named "linux,keycodes".
>> 
>> Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
>> ---
>>  drivers/input/touchscreen/melfas_mip4.c | 32 
>> ++++++++++++++++++++++++++++++--
>>  1 file changed, 30 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/input/touchscreen/melfas_mip4.c 
>> b/drivers/input/touchscreen/melfas_mip4.c
>> index 
>> a6946e3d8376d7e9b4c26f4194409e0ba78bb075..061ac353bc7a2e28f17581411af81f35c89733a1 
>> 100644
>> --- a/drivers/input/touchscreen/melfas_mip4.c
>> +++ b/drivers/input/touchscreen/melfas_mip4.c
>> @@ -169,7 +169,7 @@ struct mip4_ts {
>>  	unsigned int event_format;
>> 
>>  	unsigned int key_num;
>> -	unsigned short key_code[MIP4_MAX_KEYS];
>> +	unsigned int key_code[MIP4_MAX_KEYS];
>> 
>>  	bool wake_irq_enabled;
>> 
>> @@ -337,8 +337,13 @@ static int mip4_query_device(struct mip4_ts *ts)
>>  			ts->ppm_x, ts->ppm_y);
>> 
>>  		/* Key ts */
>> -		if (ts->node_key > 0)
>> +		if (ts->node_key > MIP4_MAX_KEYS) {
>> +			dev_warn(&ts->client->dev,
>> +				"Too many keys (%u) found\n", ts->node_key);
>> +			ts->key_num = MIP4_MAX_KEYS;
>> +		} else {
>>  			ts->key_num = ts->node_key;
>> +		}
> 
> I believe this is a bugfix. Please extract it into a separate patch.

Agree, will do so.

> 
>>  	}
>> 
>>  	/* Protocol */
>> @@ -1080,6 +1085,7 @@ static int mip4_flash_fw(struct mip4_ts *ts,
>>  			 const u8 *fw_data, u32 fw_size, u32 fw_offset)
>>  {
>>  	struct i2c_client *client = ts->client;
>> +	unsigned int i;
>>  	int offset;
>>  	u16 buf_addr;
>>  	int error, error2;
>> @@ -1149,6 +1155,11 @@ static int mip4_flash_fw(struct mip4_ts *ts,
>>  	input_abs_set_res(ts->input, ABS_X, ts->ppm_x);
>>  	input_abs_set_res(ts->input, ABS_Y, ts->ppm_y);
>> 
>> +	for (i = 0; i < ts->key_num; i++) {
>> +		if (ts->key_code[i])
>> +			input_set_capability(ts->input, EV_KEY, ts->key_code[i]);
>> +	}
>> +
>>  	return error ? error : 0;
>>  }
>> 
>> @@ -1425,6 +1436,7 @@ static int mip4_probe(struct i2c_client *client)
>>  {
>>  	struct mip4_ts *ts;
>>  	struct input_dev *input;
>> +	unsigned int i;
>>  	int error;
>> 
>>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>> @@ -1471,6 +1483,17 @@ static int mip4_probe(struct i2c_client 
>> *client)
>> 
>>  	input_set_drvdata(input, ts);
>> 
>> +#ifdef CONFIG_OF
>> +	error = of_property_read_u32_array(client->dev.of_node, 
>> "linux,keycodes",
>> +					   ts->key_code, ts->key_num);
>> +	if (error && ts->key_num) {
>> +		dev_warn(&client->dev,
>> +			 "Failed to get codes for %u supported keys", ts->key_num);
>> +		/* Disable touchkey support */
>> +		ts->key_num = 0;
>> +	}
> 
> Please use generic device properties (device_property_read_u32_array())
> and drop the dependency on OF.
> 
>> +#endif
>> +
>>  	input->keycode = ts->key_code;
>>  	input->keycodesize = sizeof(*ts->key_code);
>>  	input->keycodemax = ts->key_num;
>> @@ -1491,6 +1514,11 @@ static int mip4_probe(struct i2c_client 
>> *client)
>>  	if (error)
>>  		return error;
>> 
>> +	for (i = 0; i < ts->key_num; i++) {
>> +		if (ts->key_code[i])
>> +			input_set_capability(input, EV_KEY, ts->key_code[i]);
>> +	}
>> +
>>  	i2c_set_clientdata(client, ts);
>> 
>>  	error = devm_request_threaded_irq(&client->dev, client->irq,
>> 
> 
> Thanks.
> 
> --
> Dmitry