linux-kernel - Re: [v4 3/3] 3/3 spi: Cypress TTSP G3 SPI Device 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: <20110112184550.GB9168@core.coreip.homeip.net>
Date:	Wed, 12 Jan 2011 10:45:51 -0800
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Kevin McNeely <kev@...ress.com>
Cc:	David Brown <davidb@...eaurora.org>,
	Trilok Soni <tsoni@...eaurora.org>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Eric Miao <eric.y.miao@...il.com>,
	Mike Frysinger <vapier@...too.org>,
	Henrik Rydberg <rydberg@...omail.se>,
	Alan Cox <alan@...ux.intel.com>, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [v4 3/3] 3/3 spi: Cypress TTSP G3 SPI Device Driver

Hi Kevin,

On Tue, Jan 04, 2011 at 04:54:06PM -0800, Kevin McNeely wrote:
> +
> +static int spi_sync_tmo(struct cyttsp_spi *ts, struct spi_message *message)
> +{
> +	DECLARE_COMPLETION_ONSTACK(done);
> +	int status;
> +
> +	message->complete = spi_complete;
> +	message->context = &done;
> +	status = spi_async(ts->spi_client, message);
> +	if (status == 0) {
> +		int ret = wait_for_completion_interruptible_timeout(&done, HZ);
> +		if (!ret) {
> +			dev_dbg(ts->ops.dev, "%s: timeout\n", __func__);
> +			status = -EIO;

I do not believe we can do this and simply abort outstanding SPI
transfer. What if we unload the driver and destroy the device and then
SPI master will come around? And it does not look like SPI subsystem
allows to cancel outstanding requests...

Have you actually seen timeouts firing? Why isn't the same needed for
I2C interface?

> +
> +static s32 ttsp_spi_read_block_data(void *handle, u8 addr,
> +				    u8 length, void *data)
> +{
> +	struct cyttsp_spi *ts = container_of(handle, struct cyttsp_spi, ops);
> +	int retval;
> +
> +	retval = cyttsp_spi_xfer(CY_SPI_RD_OP, ts, addr, data, length);
> +	if (retval < 0)
> +		dev_dbg(ts->ops.dev,  "%s: ttsp_spi_read_block_data failed\n",
> +			__func__);
> +
> +	/*
> +	 * Do not print the above error if the data sync bytes were not found.
> +	 * This is a normal condition for the bootloader loader startup and need
> +	 * to retry until data sync bytes are found.
> +	 */
> +	if (retval > 0)
> +		retval = -1;	/* now signal fail; so retry can be done */

I am a bit confused here. First of all we do retries in
cyttsp_spi_xfer(). Then cyttsp_core does retry transfer as well (but i2c
methods do not retry). Can we settle on retrying in one place only?

> +
> +static s32 ttsp_spi_tch_ext(void *handle, void *values)
> +{
> +	struct cyttsp_spi *ts = container_of(handle, struct cyttsp_spi, ops);
> +	int retval = 0;
> +
> +	/*
> +	 * TODO: Add custom touch extension handling code here
> +	 * set: retval < 0 for any returned system errors,
> +	 *	retval = 0 if normal touch handling is required,
> +	 *	retval > 0 if normal touch handling is *not* required
> +	 */
> +
> +	if (!ts || !values)
> +		retval = -EINVAL;
> +
> +	return retval;

I question the utility of "per bus" extended touch handling. I can see
it being supplied from platform data but niot by interface code.

BTW, no need to resubmit patches not, it is just a discussion...

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/