linux-kernel - Re: [RESEND PATCH v3 3/5] i2c: i2c-stm32f7: add 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, 13 Sep 2017 23:26:39 +0200
From:   Wolfram Sang <wsa@...-dreams.de>
To:     Pierre-Yves MORDRET <pierre-yves.mordret@...com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Russell King <linux@...linux.org.uk>,
        linux-i2c@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [RESEND PATCH v3 3/5] i2c: i2c-stm32f7: add driver

Hi,

thanks for this driver!

> +/**
> + * struct stm32f7_i2c_spec - private i2c specification timing
> + * @rate: I2C bus speed (Hz)
> + * @rate_min: 80% of I2C bus speed (Hz)
> + * @rate_max: 120% of I2C bus speed (Hz)

You would generate a clock which is higher than the requested one?
This is highly unusual. Any special reason?

> + * @fall_max: Max fall time of both SDA and SCL signals (ns)
> + * @rise_max: Max rise time of both SDA and SCL signals (ns)
> + * @hddat_min: Min data hold time (ns)
> + * @vddat_max: Max data valid time (ns)
> + * @sudat_min: Min data setup time (ns)
> + * @l_min: Min low period of the SCL clock (ns)
> + * @h_min: Min high period of the SCL clock (ns)
> + */
> +static struct stm32f7_i2c_spec i2c_specs[] = {
> +	[STM32_I2C_SPEED_STANDARD] = {
> +		.rate = 100000,
> +		.rate_min = 8000,

This is not 80%. Typo?

> +		.rate_max = 120000,
> +		.fall_max = 300,
> +		.rise_max = 1000,
> +		.hddat_min = 0,
> +		.vddat_max = 3450,
> +		.sudat_min = 250,
> +		.l_min = 4700,
> +		.h_min = 4000,
> +	},

...

> +	/*
> +	 * Among Prescaler possibilities discovered above figures out SCL Low
> +	 * and High Period. Provided:
> +	 * - SCL Low Period has to be higher than Low Period of tehs SCL Clock

tehs?

> +	 *   defined by I2C Specification. I2C Clock has to be lower than
> +	 *   (SCL Low Period - Analog/Digital filters) / 4.
> +	 * - SCL High Period has to be lower than High Period of the SCL Clock
> +	 *   defined by I2C Specification
> +	 * - I2C Clock has to be lower than SCL High Period
> +	 */

...

> +	/* NACK received */
> +	if (status & STM32F7_I2C_ISR_NACKF) {
> +		dev_dbg(i2c_dev->dev, "<%s>: Receive NACK\n", __func__);
> +		writel_relaxed(STM32F7_I2C_ICR_NACKCF, base + STM32F7_I2C_ICR);
> +		f7_msg->result = -EBADE;

-ENXIO (see Documentation/i2c/fault-codes)

...

> +	timeout = wait_for_completion_timeout(&i2c_dev->complete,
> +					      i2c_dev->adap.timeout);
> +	ret = f7_msg->result;
> +
> +	if (!timeout) {
> +		dev_dbg(i2c_dev->dev, "Access to slave 0x%x timed out\n",
> +			i2c_dev->msg->addr);
> +		ret = -ETIMEDOUT;
> +	}

Could you rename the variable to time_left? It looks strange, basically:

	if (!timeout)
		return -ETIMEDOUT

...

> +	adap->retries = 0;

Why no retries when you check for arbitration lost?

> +	adap->algo = &stm32f7_i2c_algo;
> +	adap->dev.parent = &pdev->dev;
> +	adap->dev.of_node = pdev->dev.of_node;
> +
> +	init_completion(&i2c_dev->complete);
> +
> +	ret = i2c_add_adapter(adap);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to add adapter\n");

Please remove, the core will print info when adding fails.


Rest looks good!

Thanks,

   Wolfram


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)