linux-kernel - Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845

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: <YgEJ1M40AG9EuRPI@latitude>
Date:   Mon, 7 Feb 2022 13:00:20 +0100
From:   Jonathan Neuschäfer <j.neuschaefer@....net>
To:     Tyrone Ting <warp5tw@...il.com>
Cc:     avifishman70@...il.com, tmaimon77@...il.com, tali.perry1@...il.com,
        venture@...gle.com, yuenn@...gle.com, benjaminfair@...gle.com,
        robh+dt@...nel.org, krzysztof.kozlowski@...onical.com,
        semen.protsenko@...aro.org, yangyicong@...ilicon.com,
        wsa@...nel.org, jie.deng@...el.com, sven@...npeter.dev,
        bence98@....bme.hu, lukas.bulwahn@...il.com, arnd@...db.de,
        olof@...om.net, andriy.shevchenko@...ux.intel.com,
        tali.perry@...oton.com, Avi.Fishman@...oton.com,
        tomer.maimon@...oton.com, KWLIU@...oton.com, JJLIU0@...oton.com,
        kfting@...oton.com, devicetree@...r.kernel.org,
        openbmc@...ts.ozlabs.org, linux-i2c@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845

Hello,

On Mon, Feb 07, 2022 at 02:33:38PM +0800, Tyrone Ting wrote:
> From: Tyrone Ting <kfting@...oton.com>
> 
> NPCM8XX uses a similar i2c module as NPCM7XX.
> The only difference is that the internal HW FIFO
> is larger.
> 
> Related Makefile and Kconfig files are modified
> to support as well.
> 
> Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")

It's not really a bug fix, but rather an additional feature.
Therefore, I suggest removing the Fixes tag from this patch.

> Signed-off-by: Tyrone Ting <kfting@...oton.com>
> Signed-off-by: Tali Perry <tali.perry1@...il.com>
> ---
[...]
>  /* init register and default value required to enable module */
>  #define NPCM_I2CSEGCTL			0xE4
> +#ifdef CONFIG_ARCH_NPCM7XX
>  #define NPCM_I2CSEGCTL_INIT_VAL		0x0333F000
> +#else
> +#define NPCM_I2CSEGCTL_INIT_VAL		0x9333F000
> +#endif

This is going to cause problems when someone tries to compile a kernel
that runs on both NPCM7xx and NPCM8xx (because the driver will then only
work on NPCM7xx).

And every time another platform is added, this approach will make the
code less readable.

A more future-proof approach is probably to have a struct with chip-
specific data (such as the I2CSECCTL initialization value), which is
then selected via the .data field in of_device_id.


>  static const struct of_device_id npcm_i2c_bus_of_table[] = {
>  	{ .compatible = "nuvoton,npcm750-i2c", },
> +	{ .compatible = "nuvoton,npcm845-i2c", },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, npcm_i2c_bus_of_table);

e.g.:

	static const struct of_device_id npcm_i2c_bus_of_table[] = {
		{ .compatible = "nuvoton,npcm750-i2c", .data = &npcm750_info },
		{ .compatible = "nuvoton,npcm845-i2c", .data = &npcm845_info },
		{}
	};
	MODULE_DEVICE_TABLE(of, npcm_i2c_bus_of_table);


Best regards,
Jonathan

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