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]

Message-ID: <20160504225846.GA17403@dtor-ws>
Date:	Wed, 4 May 2016 15:58:46 -0700
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	"jeffrey.lin" <yajohn@...il.com>
Cc:	rydberg@...omail.se, grant.likely@...aro.org, robh+dt@...nel.org,
	jeesw@...fas.com, bleung@...omium.org, 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

Hi Jeffrey,

On Fri, Apr 29, 2016 at 05:45:13PM +0800, jeffrey.lin wrote:
> Raydium I2C touch driver.
> 

In my previous e-mail I requested you to enumerate changes that are made
to the driver, compared to the previous version(s). There were also a
few questions that I did not get answer for.

Also:

> +static int raydium_i2c_query_ts_info(struct raydium_data *ts)
> +{
> +	struct i2c_client *client = ts->client;
> +	int error, retry_cnt;
> +
> +	for (retry_cnt = 0; retry_cnt < MAX_RETRIES; retry_cnt++) {
> +		error = raydium_i2c_read(client, CMD_DATA_BANK,
> +			sizeof(ts->obj), (void *)&ts->obj);
> +			ts->obj.data_bank_addr =
> +				get_unaligned_le32(&ts->obj.data_bank_addr);
> +
> +		if (!error) {
> +			error = raydium_i2c_read(client, CMD_QUERY_BANK,
> +				sizeof(ts->query_bank_info),
> +				(void *)&ts->query_bank_info);
> +			if (!error) {
> +				error = raydium_i2c_read_message(client,
> +					ts->query_bank_info, sizeof(ts->info),
> +					(void *)&ts->info);
> +
> +				ts->info.hw_ver =
> +					get_unaligned_le32(&ts->info.hw_ver);
> +				ts->info.ft_ver =
> +					get_unaligned_le16(&ts->info.ft_ver);
> +				ts->info.x_max =
> +					get_unaligned_le16(&ts->info.x_max);
> +				ts->info.y_max =
> +					get_unaligned_le16(&ts->info.y_max);
> +				return 0;
> +			}
> +		}
> +	}
> +	dev_err(&client->dev, "Get touch data failed: %d\n", error);
> +
> +	return -EINVAL;
> +}
> +
> +static int raydium_i2c_fastboot(struct i2c_client *client)
> +{
> +	static const u8 boot_cmd[] = { 0x50, 0x00, 0x06, 0x20 };
> +	u8 buf[HEADER_SIZE];
> +	int error;
> +
> +	error = raydium_i2c_read_message(client,
> +		get_unaligned_be32(boot_cmd),
> +		sizeof(boot_cmd), buf);

I still can't figure out the logic in read/write mesage handling. Here
we see that device is supposedly reporting data_bank_addr as LE integer,
but query_bank_info is returned in native CPU endianness. With fastboot
command we assume that we are dealing with BE-encoded data and convert
it to CPU-endianness before using it.

However in raydium_i2c_read_message() we convert from CPU endianness to
BE and this comfuses me (does the device really return data in one
endianness but accepts in another endianness?

By the way, why do you have raydium_i2c_read_message() handle reads
above MAX_PKG_SIZE? There are no callers that want it.

Thanks.

-- 
Dmitry