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

["Jonathan Neuschäfer" <j.neuschaefer@gmx.net>]

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

Hello,

On Mon, Feb 07, 2022 at 02:33:38PM +0800, Tyrone Ting wrote:
> From: Tyrone Ting <kfting@nuvoton.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@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[] = {
>  	{ .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

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