Re: [PATCH 2/2 v3] i2c: mt7621: Add MediaTek MT7621/7628/7688 I2C driver

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 2349 bytes --]

Hi Stefan,

On Tue, Jun 04, 2019 at 01:34:07PM +0200, Stefan Roese wrote:
> This patch adds a driver for the I2C controller found on the MediaTek
> MT7621/7628/7688 SoC's. The base version of this driver was done by
> Steven Liu (according to the copyright and MODULE_AUTHOR lines). It
> can be found in the OpenWRT repositories (v4.14 at the time I looked).
> 
> The base driver had many issues, which are disccussed here:
> 
> https://en.forum.labs.mediatek.com/t/openwrt-15-05-loads-non-working-i2c-kernel-module-for-mt7688/1286/3
> 
> From this link an enhanced driver version (complete rewrite, mayor
> changes: support clock stretching, repeated start, ACK handling and
> unlimited message length) from Jan Breuer can be found here:
> 
> https://gist.github.com/j123b567/9b555b635c2b4069d716b24198546954
> 
> This patch now adds this enhanced I2C driver to mainline.
> 
> Changes by Stefan Roese for upstreaming:
> - Add devicetree bindings
> - checkpatch clean
> - Use module_platform_driver()
> - Minor cosmetic enhancements
> - Removed IO warpped functions
> - Use readl_relaxed_poll_timeout() and drop poll_down_timeout()
> - Removed superfluous barrier() in mtk_i2c_reset()
> - Use i2c_8bit_addr_from_msg()
> - Added I2C_FUNC_PROTOCOL_MANGLING
> - Removed adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
> 
> Signed-off-by: Stefan Roese <sr@denx.de>

Mostly good, really minor nits left.

> +config I2C_MT7621
> +	tristate "MT7621/MT7628 I2C Controller"
> +	depends on (RALINK && (SOC_MT7620 || SOC_MT7621)) || COMPILE_TEST
> +	select OF_I2C

OF_I2C is gone since 2013. Didn't Kconfig complain?

> +
> +#define MT76XX_I2C_INPUT_CLOCK	40000000

Add a comment here explaining that this is a temporary solution until
proper clock support is added?

> +	ret = readl_relaxed_poll_timeout(i2c->base + REG_SM0CTL1_REG,
> +					 val, !(val & SM0CTL1_TRI),
> +					 10, TIMEOUT_MS * 1000);

Yay, much better.

> +	if (i2c->bus_freq == 0) {
> +		dev_warn(i2c->dev,
> +			 "clock-frequency 0 not supported, using 1\n");
> +		i2c->bus_freq = 1;
> +	}

A bus frequency of 1 Hz does neither make much sense. -EINVAL because
there is surely something broken then?

> +MODULE_AUTHOR("Steven Liu <steven_liu@mediatek.com>");

His address is known to be broken!

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]