linux-kernel - Re: [PATCH] driver: input :touchscreen : add Raydium I2C touch driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 02 Mar 2016 18:44:10 -0800
From:	Joe Perches <joe@...ches.com>
To:	"jeffrey.lin" <yajohn@...il.com>, dmitry.torokhov@...il.com,
	rydberg@...omail.se, grant.likely@...aro.org, robh+dt@...nel.org,
	jeesw@...fas.com, bleung@...omium.org, scott.liu@....com.tw
Cc:	jeffrey.lin@...-ic.com, roger.yang@...-ic.com, KP.li@...-ic.com,
	albert.shieh@...-ic.com, linux-kernel@...r.kernel.org,
	linux-input@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH] driver: input :touchscreen : add Raydium I2C touch
 driver

On Thu, 2016-03-03 at 10:29 +0800, jeffrey.lin wrote:
> This patch is porting Raydium I2C touch driver. Developer can enable raydium touch driver by modifying define "CONFIG_TOUCHSCREEN_RM_TS".

trivial comments:

> diff --git a/drivers/input/touchscreen/raydium_i2c_ts.c b/drivers/input/touchscreen/raydium_i2c_ts.c
[]
> +static int raydium_i2c_send(struct i2c_client *client,
> +	u8 addr, u8 *data, size_t len)
> +{
> +	u8 buf[MAX_PACKET_SIZE + 1];
> +	int i, tries = 0;
> +
> +	if (len > MAX_PACKET_SIZE)
> +		return -EINVAL;
> +
> +	buf[0] = addr & 0xff;

Seems pointless to use & 0xff when both
buf and addr are u8

> +	for (i = 0; i < len; i++)
> +		buf[i + 1] = *data++;

memcpy.  Maybe:

	buf[0] = addr;
	memcpy(&buf[1], data, len);

> +static int raydium_i2c_send_message(struct i2c_client *client,
> +	size_t len, void *data)
> +{
> +	int error;
> +	u8 buf[HEADER_SIZE], ii;
> +
> +	for (ii = 0; ii < HEADER_SIZE; ii++)
> +		buf[ii] = ((u8 *)data)[3 - ii];
> +	/*set data bank*/
> +	error = raydium_i2c_send(client, CMD_BANK_SWITCH,
> +		(u8 *)buf, HEADER_SIZE);
> +
> +	/*send messange*/

typo, message

> +	if (!error)
> +		error = raydium_i2c_send(client, buf[ADDR_INDEX], buf, len);
> +
> +	return error;
> +}
> +
> +static int raydium_i2c_sw_reset(struct i2c_client *client)
> +{
> +	const u8 soft_rst_cmd[] = { 0x01, 0x04, 0x00, 0x00, 0x40};

static

> +	int error;
> +
> +	error = raydium_i2c_send_message(client,
> +			1, (void *)soft_rst_cmd);

could be better on a single line

> +static int raydium_i2c_fastboot(struct i2c_client *client)
> +{
> +	const u8 boot_cmd[] = { 0x20, 0x06, 0x00, 0x50 };
> +	u8 buf[HEADER_SIZE];
> +	int error;
> +
> +	error = raydium_i2c_read_message(client,
> +		get_unaligned_be32(boot_cmd),
> +		sizeof(boot_cmd),
> +		buf);
> +
> +	if (error) {
> +		dev_err(&client->dev, "boot failed: %d\n", error);
> +		return error;
> +	} else if (buf[0] == RM_BOOT_BLDR) {

unnecessary else

> +		dev_dbg(&client->dev, "boot in fastboot mode\n");
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(&client->dev, "boot success -- 0x%x\n", client->addr);
> +	return 0;
> +}
> +
> +static int raydium_i2c_initialize(struct raydium_data *ts)
> +{
> +	struct i2c_client *client = ts->client;
> +	int error, retry_cnt;
> +	const u8 recov_packet[] = { 0x04, 0x81 };

static and etc...

generally, it'd be nice to align to open parenthesis and
to more maximally fill lines to 80 chars instead of using
relatively short line lengths and multiple line statements.