Re: [PATCH v6 1/2] Input: add zinitix touchscreen driver

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Michael,

On Thu, Oct 01, 2020 at 02:29:48PM +0200, michael.srba@seznam.cz wrote:
> From: Michael Srba <Michael.Srba@seznam.cz>
> 
> Add support for the bt541 touchscreen IC from zinitix, loosely based on
> downstream driver. The driver currently supports multitouch (5 touch points).
> The bt541 seems to support touch keys, but the support was not added because
> that functionality is not being utilized by the touchscreen used for testing.
> Based on the similartities between downstream drivers, it seems likely that
> other similar touchscreen ICs can be supported with this driver in the future.
> 
> Signed-off-by: Michael Srba <Michael.Srba@seznam.cz>
> ---
>  changes in v2: applied fixes per recommendation, added support for suspend/resume handling
>  changes in v3: added support for working in different touch point report modes 
>  		(modes 1 and 2 are now supported). mode 2 seems to work just fine
>  		on Samsung Galaxy A3 (2015), and also works on Samsung Galaxy S4 
>  		Mini Value Edition (where mode 1 doesn't seem to work because 
>  		of different firmware version). It is expected that other fw 
>  		versions, and models other than bt541, may have either mode broken
>  		(vendor doesn't use it -> they don't care) .
>  changes in v4: - removed mode 1 for now to simplify the code and improve it's chances 
>  		to get accepted.
>  		- added runtime pm to save power while not using the ts
>  		- refactored i2c helper functions
> 		-  
>  changes in v5: - send the actual intended V4 (sorry)
>  changes in v6: - remove unused include
>  		- other fixes per reccomendation

Applied with some edits:

> +
> +static int zinitix_read_data(struct i2c_client *client, u16 reg, u8 *values, size_t length)
> +{
> +	int ret;
> +	__le16 reg_le = cpu_to_le16(reg);
> +

Added a comment that single i2c_transfer will not work.

> +	ret = i2c_master_send(client, (u8 *)&reg_le, sizeof(reg_le));
> +	if (ret != sizeof(reg_le))
> +		return ret < 0 ? ret : -EIO;
> +
> +	ret = i2c_master_recv(client, values, length);
> +	if (ret != length)
> +		return ret;

Changed to:

		return ret < 0 ? ret : -EIO;

> +	error = zinitix_write_u16(client, BT541_X_RESOLUTION, (u16)bt541->prop.max_x);

No need for explicit cast as far as I can see, removed.

> +
> +	for (i = 0; i < MAX_SUPPORTED_FINGER_NUM; i++) {
> +		if (!(touch_event.point_coord[i].sub_status & SUB_BIT_EXIST))
> +			continue;
> +
> +		input_mt_slot(bt541->input_dev, i);
> +		input_mt_report_slot_state(bt541->input_dev, MT_TOOL_FINGER, true);
> +		touchscreen_report_pos(bt541->input_dev, &bt541->prop,
> +				       le16_to_cpu(touch_event.point_coord[i].x),
> +				       le16_to_cpu(touch_event.point_coord[i].y), true);
> +		input_report_abs(bt541->input_dev, ABS_MT_TOUCH_MAJOR,
> +				 touch_event.point_coord[i].width);

Factored out into a helper.

> +
> +	error = input_register_device(bt541->input_dev);
> +	if (error) {
> +		dev_err(&bt541->client->dev,
> +			"Failed to register input device: %d", error);
> +		return error;
> +	}
> +
> +	input_set_drvdata(bt541->input_dev, bt541);

This is too late, as open and close that use input_get_drvdata() can be
called before input_register_device() returns. Moved earlier.

> +
> +	return 0;
> +}
> +
> +static int zinitix_ts_probe(struct i2c_client *client, const struct i2c_device_id *id)

Dropped the last argument and used as probe_new() callback.

> +
> +static int __maybe_unused zinitix_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct bt541_ts_data *bt541 = i2c_get_clientdata(client);
> +
> +	/* Release all fingers */
> +	input_mt_sync_frame(bt541->input_dev);
> +	input_sync(bt541->input_dev);

As I mentioned, if we need this, this should be done in input core and
not in each individual driver.

I dropped this chunk for now.

Thanks.

-- 
Dmitry