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@xxxxxxxxxxx>
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@xxxxxxxxxxx>
> + *
> + * 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
