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

[tali.perry at nuvoton.com]

>-----Original Message-----
>From: Krzysztof Kozlowski <krzysztof.kozlowski at canonical.com>
>Sent: Monday, February 7, 2022 5:27 PM
>To: Jonathan Neuschäfer <j.neuschaefer at gmx.net>; Tyrone Ting <warp5tw at gmail.com>
>Cc: avifishman70 at gmail.com; tmaimon77 at gmail.com; tali.perry1 at gmail.com; venture at google.com; yuenn at google.com; benjaminfair at google.com; robh+dt at kernel.org; semen.protsenko at linaro.org; yangyicong at hisilicon.com; wsa at kernel.org; jie.deng at intel.com; sven at svenpeter.dev; bence98 at sch.bme.hu; lukas.bulwahn at gmail.com; arnd at arndb.de; olof at lixom.net; andriy.shevchenko at linux.intel.com; IS20 Tali Perry <tali.perry at nuvoton.com>; IS20 Avi Fishman <Avi.Fishman at nuvoton.com>; IS20 Tomer Maimon <tomer.maimon at nuvoton.com>; CS20 KWLiu <KWLIU at nuvoton.com>; CS20 JJLiu0 <JJLIU0 at nuvoton.com>; CS20 KFTing <KFTING at nuvoton.com>; devicetree at vger.kernel.org; openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org; linux-kernel at vger.kernel.org
>Subject: Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845
>
>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 at 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 at nuvoton.com>
>>> Signed-off-by: Tali Perry <tali.perry1 at 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?
>


NPCM7xx and NPCM8xx are very different devices.
They share same driver sources for some of the modules but it's not ABI.
Users cannot compile a single kernel with two separate DTS.
In case of the i2c controller, the npcm7xx has a 16 byte HW FIFO,
and the NPCM8xx has 32 bytes HW FIFO.
This also means that registers fields are slightly different.
For init data we can move it to the DTS, but register field sizes
can't be handled with this approach.



>>
>> 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

Thanks for the review!
Tali Perry, Nuvoton.
________________________________
________________________________
 The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Nuvoton is strictly prohibited; and any information in this email irrelevant to the official business of Nuvoton shall be deemed as neither given nor endorsed by Nuvoton.