Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Mon, 7 Feb 2022 13:00:20 +0100
From:   Jonathan =?utf-8?Q?Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>
To:     Tyrone Ting <warp5tw@gmail.com>
Cc:     avifishman70@gmail.com, tmaimon77@gmail.com, tali.perry1@gmail.com,
        venture@google.com, yuenn@google.com, benjaminfair@google.com,
        robh+dt@kernel.org, krzysztof.kozlowski@canonical.com,
        semen.protsenko@linaro.org, yangyicong@hisilicon.com,
        wsa@kernel.org, jie.deng@intel.com, sven@svenpeter.dev,
        bence98@sch.bme.hu, lukas.bulwahn@gmail.com, arnd@arndb.de,
        olof@lixom.net, andriy.shevchenko@linux.intel.com,
        tali.perry@nuvoton.com, Avi.Fishman@nuvoton.com,
        tomer.maimon@nuvoton.com, KWLIU@nuvoton.com, JJLIU0@nuvoton.com,
        kfting@nuvoton.com, devicetree@vger.kernel.org,
        openbmc@lists.ozlabs.org, linux-i2c@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845
Message-ID: <YgEJ1M40AG9EuRPI@latitude>
References: <20220207063338.6570-1-warp5tw@gmail.com>
 <20220207063338.6570-7-warp5tw@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="LFTVFI5jLgkqvDDv"
Content-Disposition: inline
In-Reply-To: <20220207063338.6570-7-warp5tw@gmail.com>
Precedence: bulk


--LFTVFI5jLgkqvDDv
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hello,

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

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@nuvoton.com>
> Signed-off-by: Tali Perry <tali.perry1@gmail.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[] =3D {
>  	{ .compatible =3D "nuvoton,npcm750-i2c", },
> +	{ .compatible =3D "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[] =3D {
		{ .compatible =3D "nuvoton,npcm750-i2c", .data =3D &npcm750_info },
		{ .compatible =3D "nuvoton,npcm845-i2c", .data =3D &npcm845_info },
		{}
	};
	MODULE_DEVICE_TABLE(of, npcm_i2c_bus_of_table);


Best regards,
Jonathan

--LFTVFI5jLgkqvDDv
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEvHAHGBBjQPVy+qvDCDBEmo7zX9sFAmIBCaYACgkQCDBEmo7z
X9uI7A/+MWPp/zPHPaH3lYE4rWHKtXP/hbBmF0vkBrZkvhLAh69d3QhFSCURD+7b
aC2EGxA6Jmnnbq4iGi76fSAge1D6KiDCpYvi3QLRSY5h66nEyvnAs2JlZZXMfBkQ
TTCWSzdULh1ow8+IdwzTuQ/ngCNk+Dar1R8tsPvZjfgjNA+huM/UuBNP6c8XY/pe
r1gGazpuFO5IittQwGVqW0f7WVdu7gBGsuOBlpdS4xMGdw5Zp2XSmMXPjESEI6Tp
lCycBXLdhzEokfA76PhOuRSjn6V3fyZYlw/iIxLh/vISBD+XtAt9RuvvqHmxIlS0
T+rqm3Kdn6BPb6xYxdBt2lySI6838dS7CvAI4ntu42bGTM+xboOSXm7RTUT0/PRV
cGV3Uo3vx9fLNWVjxLhEbuMYyGY1AbE5UuORWkij2XilAWrwOqvFUWWu0Xw5UnaA
SaWSZHU7/9QP16BCwoVt5Nv2HiygMddN50dZFAzcy2oLXoChLFThcrc5KGKvfX/4
ltXWQ4XI+7riX9QhoD9HX2SLwTNaX1V8nHojvzbflb7UIvujg8VSu2uBq6hRElDe
BrBEbtEkFYVzs2zkULWGsp9bQGcwJF3gVEWFTpxuLWS6xFB3kJqPhVffoqxL4NbD
W3HqOmJArx3js3avkgpp3LzI8hAAYfq+yP3Fibsmc9Z89mU5rYE=
=8qyc
-----END PGP SIGNATURE-----

--LFTVFI5jLgkqvDDv--