Re: [PATCH v9 2/4] Input: add core support for Goodix Berlin Touchscreen IC

[Neil Armstrong <neil.armstrong@xxxxxxxxxx>]

Hi Dmitry,

On 23/10/2023 06:37, Dmitry Torokhov wrote:
> Hi Neil,
>
> On Sat, Oct 21, 2023 at 01:09:24PM +0200, Neil Armstrong wrote:
> > +static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
> > +{
> > +	u8 afe_data[GOODIX_BERLIN_IC_INFO_MAX_LEN];
>
> You probably already saw the kernel test robot message, I think we
> should allocate this buffer in the heap (and free it once its no longer
> needed).

Indeed I haven't received it on 9 patch submitting, anyway moved it to head.

> > +	__le16 length_raw;
> > +	u16 length;
> > +	int error;
> > +
> > +	error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
> > +				&length_raw, sizeof(length_raw));
> > +	if (error) {
> > +		dev_info(cd->dev, "failed get ic info length, %d\n", error);
> > +		return error;
> > +	}
> > +
> > +	length = le16_to_cpu(length_raw);
> > +	if (length >= GOODIX_BERLIN_IC_INFO_MAX_LEN) {
> > +		dev_info(cd->dev, "invalid ic info length %d\n", length);
> > +		return -EINVAL;
> > +	}
> > +
> > +	error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
> > +				afe_data, length);
> > +	if (error) {
> > +		dev_info(cd->dev, "failed get ic info data, %d\n", error);
> > +		return error;
> > +	}
> > +
> > +	/* check whether the data is valid (ex. bus default values) */
> > +	if (goodix_berlin_is_dummy_data(cd, (const uint8_t *)afe_data, length)) {
>
> This cast is not needed.
>
> > +		dev_err(cd->dev, "fw info data invalid\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (!goodix_berlin_checksum_valid((const uint8_t *)afe_data, length)) {
>
> This cast is not needed either.
>
> > +		dev_info(cd->dev, "fw info checksum error\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	error = goodix_berlin_convert_ic_info(cd, afe_data, length);
> > +	if (error)
> > +		return error;
> > +
> > +	/* check some key info */
> > +	if (!cd->touch_data_addr) {
> > +		dev_err(cd->dev, "touch_data_addr is null\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
> > +				       const void *buf, int touch_num)
> > +{
> > +	const struct goodix_berlin_touch_data *touch_data = buf;
> > +	int i;
> > +
> > +	/* Check for data validity */
> > +	for (i = 0; i < touch_num; i++) {
> > +		unsigned int id;
> > +
> > +		id = FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK, touch_data[i].id);
> > +
> > +		if (id >= GOODIX_BERLIN_MAX_TOUCH) {
> > +			dev_warn(cd->dev, "invalid finger id %d\n", id);
> > +			return;
>
> Is it important to abort entire packet if one if the slots has incorrect
> data? Can we simply skip over these contacts?

Indeed, merged the for loops and simply skip the invalid finger id.

> > +		}
> > +	}
> > +
> > +	/* Report finger touches */
> > +	for (i = 0; i < touch_num; i++) {
> > +		input_mt_slot(cd->input_dev,
> > +			      FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK,
> > +					touch_data[i].id));
> > +		input_mt_report_slot_state(cd->input_dev, MT_TOOL_FINGER, true);
> > +
> > +		touchscreen_report_pos(cd->input_dev, &cd->props,
> > +				       __le16_to_cpu(touch_data[i].x),
> > +				       __le16_to_cpu(touch_data[i].y),
> > +				       true);
> > +		input_report_abs(cd->input_dev, ABS_MT_TOUCH_MAJOR,
> > +				 __le16_to_cpu(touch_data[i].w));
> > +	}
> > +
> > +	input_mt_sync_frame(cd->input_dev);
> > +	input_sync(cd->input_dev);
> > +}
> > +
> > +static void goodix_berlin_touch_handler(struct goodix_berlin_core *cd,
> > +					const void *pre_buf, u32 pre_buf_len)
> > +{
> > +	static u8 buffer[GOODIX_BERLIN_IRQ_READ_LEN(GOODIX_BERLIN_MAX_TOUCH)];
>
> No, please no static data. The drivers should be ready to handle more
> than one device on a system. If the buffer is large-ish put it into
> goodix_berlin_core.

This is a typo from vendor kernel, I didn't want a static buffer here...

The buffer is only 26 bytes, it's small enough to stay here in non-static.

> > +	u8 point_type, touch_num;
> > +	int error;
> > +
> > +	/* copy pre-data to buffer */
> > +	memcpy(buffer, pre_buf, pre_buf_len);
> > +
> > +	touch_num = FIELD_GET(GOODIX_BERLIN_TOUCH_COUNT_MASK,
> > +			      buffer[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
> > +
> > +	if (touch_num > GOODIX_BERLIN_MAX_TOUCH) {
> > +		dev_warn(cd->dev, "invalid touch num %d\n", touch_num);
> > +		return;
> > +	}
> > +
> > +	if (touch_num) {
> > +		/* read more data if more than 2 touch events */
> > +		if (unlikely(touch_num > 2)) {
> > +			error = regmap_raw_read(cd->regmap,
> > +						cd->touch_data_addr + pre_buf_len,
> > +						&buffer[pre_buf_len],
> > +						(touch_num - 2) * GOODIX_BERLIN_BYTES_PER_POINT);
> > +			if (error) {
> > +				dev_err_ratelimited(cd->dev, "failed to get touch data, %d\n",
> > +						    error);
> > +				return;
> > +			}
> > +		}
> > +
> > +		point_type = FIELD_GET(GOODIX_BERLIN_POINT_TYPE_MASK,
> > +				       buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
> > +
> > +		if (point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS ||
> > +		    point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER) {
> > +			dev_warn_once(cd->dev, "Stylus event type not handled\n");
> > +			return;
> > +		}
> > +
> > +		if (!goodix_berlin_checksum_valid(&buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
> > +						  touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2)) {
> > +			dev_err(cd->dev, "touch data checksum error, data: %*ph\n",
> > +				touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2,
> > +				&buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
> > +			return;
> > +		}
> > +	}
> > +
> > +	goodix_berlin_parse_finger(cd, &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
> > +				   touch_num);
> > +}
> > +
> > +static int goodix_berlin_request_handle_reset(struct goodix_berlin_core *cd)
> > +{
> > +	gpiod_set_value(cd->reset_gpio, 1);
> > +	usleep_range(2000, 2100);
> > +	gpiod_set_value(cd->reset_gpio, 0);
> > +
> > +	msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
>
> The reset line handling is optional, we should skip waits if there is
> no GPIO defined for the board.

Ack, instead I only call this if gpio is valid.

> > +
> > +	return 0;
> > +}
> > +
> > +static irqreturn_t goodix_berlin_threadirq_func(int irq, void *data)
> > +{
> > +	struct goodix_berlin_core *cd = data;
> > +	u8 buf[GOODIX_BERLIN_IRQ_READ_LEN(2)];
> > +	u8 event_status;
> > +	int error;
> > +
> > +	/* First, read buffer with space for 2 touch events */
> > +	error = regmap_raw_read(cd->regmap, cd->touch_data_addr, buf,
> > +				GOODIX_BERLIN_IRQ_READ_LEN(2));
> > +	if (error) {
> > +		dev_err_ratelimited(cd->dev, "failed get event head data, %d\n", error);
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	if (buf[GOODIX_BERLIN_STATUS_OFFSET] == 0)
> > +		return IRQ_HANDLED;
> > +
> > +	if (!goodix_berlin_checksum_valid(buf, GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN)) {
> > +		dev_warn_ratelimited(cd->dev, "touch head checksum err : %*ph\n",
> > +				     GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN, buf);
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	event_status = buf[GOODIX_BERLIN_STATUS_OFFSET];
> > +
> > +	if (event_status & GOODIX_BERLIN_TOUCH_EVENT)
> > +		goodix_berlin_touch_handler(cd, buf, GOODIX_BERLIN_IRQ_READ_LEN(2));
> > +
> > +	if (event_status & GOODIX_BERLIN_REQUEST_EVENT) {
> > +		switch (buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]) {
> > +		case GOODIX_BERLIN_REQUEST_CODE_RESET:
> > +			goodix_berlin_request_handle_reset(cd);
> > +			break;
> > +
> > +		default:
> > +			dev_warn(cd->dev, "unsupported request code 0x%x\n",
> > +				 buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
> > +		}
> > +	}
> > +
> > +	/* Clear up status field */
> > +	regmap_write(cd->regmap, cd->touch_data_addr, 0);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int goodix_berlin_input_dev_config(struct goodix_berlin_core *cd,
> > +					  const struct input_id *id)
> > +{
> > +	struct input_dev *input_dev;
> > +	int error;
> > +
> > +	input_dev = devm_input_allocate_device(cd->dev);
> > +	if (!input_dev)
> > +		return -ENOMEM;
> > +
> > +	cd->input_dev = input_dev;
> > +	input_set_drvdata(input_dev, cd);
> > +
> > +	input_dev->name = "Goodix Berlin Capacitive TouchScreen";
> > +	input_dev->phys = "input/ts";
> > +
> > +	input_dev->id = *id;
> > +
> > +	input_set_abs_params(cd->input_dev, ABS_MT_POSITION_X, 0, SZ_64K - 1, 0, 0);
> > +	input_set_abs_params(cd->input_dev, ABS_MT_POSITION_Y, 0, SZ_64K - 1, 0, 0);
> > +	input_set_abs_params(cd->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> > +
> > +	touchscreen_parse_properties(cd->input_dev, true, &cd->props);
> > +
> > +	error = input_mt_init_slots(cd->input_dev, GOODIX_BERLIN_MAX_TOUCH,
> > +				    INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> > +	if (error)
> > +		return error;
> > +
> > +	return input_register_device(cd->input_dev);
>
> Please in functions with multiple possible failure paths use format
>
> 	error = op(...);
> 	if (error)
> 		return error;
>
> 	return 0;

Ack, will check for this in the whole patchset.

Thanks for review,
Neil

> Thanks.