Message-ID: <20201113020716.GC356503@dtor-ws>
Date:   Thu, 12 Nov 2020 18:07:16 -0800
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     KwangDeok Son <kdson@...itix.com>
Cc:     linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Input: add new touchpad driver for Zinitix IC

Hi KwangDeok,

On Thu, Nov 12, 2020 at 08:40:37PM +0900, KwangDeok Son wrote:
> Add new touchpad driver for Zinitix IC
> Supports five fingers multi-touch and firmware updates.
> It communicates with the device via an I2C bus.
> 
> Signed-off-by : KwangDeok Son <kdson@...itix.com>

Please run the patch through ./scripts/checkpatch.pl and fix warnings
from the tool.

> @@ -0,0 +1,1524 @@
> +/*
> + * Zinitics I2C Touchpad driver
> + *
> + * Copyright (c) 2020 ZINITIX Co.,Ltd
> + *
> + * Author: KwangDeok Son <kdson@...itix.com>
> + *
> + * Based on elan driver:
> + * Copyright (c) 2011-2013 ELAN Microelectronics Corp.
> + * copyright (c) 2011-2012 Cypress Semiconductor, Inc.
> + * copyright (c) 2011-2012 Google, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.

Please drop GPL notice, use SPDX tag instead.

> + *
> + * Trademarks are the property of their respective owners.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/kernel.h>
> +#include <linux/sched.h>

Please double-check that all includes are actually needed.

> +#include <linux/input.h>
> +#include <linux/uaccess.h>
> +#include <linux/jiffies.h>
> +#include <linux/completion.h>
> +#include <linux/of.h>
> +#include <asm/unaligned.h>

asm includes should go after linux ones.

> +
> +#include <linux/delay.h>
> +#include <linux/firmware.h>
> +#include <linux/i2c.h>
> +#include <linux/input/mt.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/regulator/consumer.h>
> +#include "zinitix_i2c.h"

We are not going to share any definitions with other modules, so put all
the constants in this file and drop the .h one.

> +int zinitix_i2c_write_reg(struct i2c_client *client, u16 reg, u16 value)
> +{
> +        int errno = 0;
> +        u8 buf[22] = {0x80, 0x01, 0x14, 0x00, 0x06, 0x00, };

Please add a #define for 22 constant.

> +
> +	buf[6] = reg & 0xFF;
> +	buf[7] = (reg>>8)&0xFF;
> +	buf[8] = value & 0xFF;;
> +	buf[9] = (value>>8)&0xFF;;
> +		

We have cpu_to_le16 and put_unaligned_le16 for this.

> +	errno = zinitix_i2c_write_buf(client, buf, 22);

sizeof(buf) instead of 22

> +
> +	return errno;
> +}
> +
> +int zinitix_i2c_write_data(struct i2c_client *client, u16 reg, 
> +                                const u8* value, u16 size)
> +{
> +        int errno = 0;
> +        u8 buf[22] = {0x80, 0x01, 0x14, 0x00, 0x06, 0x00, };
> +	buf[5] = size & 0xFF;
> +	buf[6] = reg & 0xFF;
> +	buf[7] = (reg>>8)&0xFF;
> +	memcpy(&buf[8], value, size);

Need to make sure we are not overwriting the buffer.

> +	errno = zinitix_i2c_write_buf(client, buf, 22);	
> +
> +	return errno;
> +}
> +
> +int zinitix_i2c_write_fwdata(struct i2c_client *client, u16 reg, 
> +                                const u8* value, u16 size)
> +{
> +        int errno = 0;
> +        u8 buf[22] = {0x80, 0x01, 0x14, 0x00, 0x06, 0x00, };
> +	buf[5] = size & 0xFF;
> +	
> +	memcpy(&buf[6], value, size);

Need to make sure we are not overwriting the buffer.

> +	
> +	errno = zinitix_i2c_write_buf(client, buf, 22);	
> +	
> +	return errno;
> +}
> +
> +u16 zinitix_i2c_read_reg(struct i2c_client *client, u16 reg, u16* value)
> +{
> +	u16 retval = 0;
> +	u8 outBuf[26] = {0x94, 0x1, 0x26, 0x3, 0x96, 0x1, 0x14, 0, 0x6, 0x1, };
> +	u8 inBuf[8] = {0, };
> +	
> +	//Sample

I prefer C-style comments.

> +	outBuf[10] = reg & 0xFF;
> +	outBuf[11] = (reg>>8)&0xFF;
> +	//Send
> +	retval = (u16)zinitix_i2c_write_buf(client, outBuf, 26);

Error handling?

> +	//Recv
> +	outBuf[2] = 0x16;
> +	outBuf[3] = 0x02;
> +	retval = (u16)zinitix_i2c_read_buf(client, outBuf,6, inBuf, 8);
> +	if (retval < 0) {

?? You are casing to u16 and then checking for negative value?

> +		dev_err(&client->dev, "reading cmd (0x%04x) fail.\n", reg);
> +		return retval;
> +	}
> +
> +	*value = (inBuf[6]&0xFF) | ((inBuf[7]<<8)&0xFF00);

le16_to_cpup(). Elsewhere too.

> +
> +static int get_descriptor(struct i2c_client *client, u8* val)
> +{
> +        struct touchpad_data *data = i2c_get_clientdata(client);
> +        int error;
> +        //Get HID Descriptor
> +        error = zinitix_i2c_read(client, ZINITIX_GET_DESC, 
> +                                val, ZINITIX_DESC_LENGTH);

Is this truly HID descriptor? Is device compatible with I2C HID
spec/Microsoft Precision point protocol? If so this driver is not needed
at all and we can use i2c-hid + hid-multitouch to handle the device.

> +
> +const struct zinitix_transport_ops zinitix_i2c_ops = {
> +	.initialize		= zinitix_i2c_initialize,
> +	.sleep_control		= zinitix_sleep_control,
> +	.reset		        = zinitix_reset,
> +	.get_max		= zinitix_i2c_get_max,
> +	.get_num_channel        = zinitix_i2c_get_num_channel,
> +	.prepare_fw_update	= zinitix_i2c_prepare_fw_update,
> +	.write_fw_block		= zinitix_i2c_write_fw_block,
> +	.read_fw_block		= zinitix_i2c_read_fw_block,
> +	.finish_fw_update	= zinitix_i2c_finish_fw_update,
> +	.get_report		= zinitix_i2c_get_report,
> +};

Are you planning on adding additional transports (such as SMB)? If not,
then this indirection is not needed.

Thanks.

-- 
Dmitry

Powered by blists - more mailing lists