Messages in this thread | | | Date | Mon, 7 Feb 2022 16:26:30 +0100 | Subject | Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845 | From | Krzysztof Kozlowski <> |
| |
On 07/02/2022 13:00, Jonathan Neuschäfer wrote: > 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).
Yes, good catch.
The NPCM7XX is multiplatform, I guess NPCM8xx will be as well, so this looks like an invalid code. How such code is supposed to work on multiplatform kernel?
> > 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.
Best regards, Krzysztof
| |