Re: [PATCH v2 14/18] serial: intel: Add CCF support
From: Wu, Songjun
Date: Tue Aug 07 2018 - 03:18:52 EST
On 8/6/2018 5:29 PM, Geert Uytterhoeven wrote:
Hi Songjun,
On Mon, Aug 6, 2018 at 10:58 AM Wu, Songjun <songjun.wu@xxxxxxxxxxxxxxx> wrote:
On 8/6/2018 3:20 PM, Geert Uytterhoeven wrote:
On Mon, Aug 6, 2018 at 9:15 AM Wu, Songjun <songjun.wu@xxxxxxxxxxxxxxx> wrote:
On 8/5/2018 5:03 AM, Arnd Bergmann wrote:
On Sat, Aug 4, 2018 at 2:43 PM, Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote:
On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote:
On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote:
This patch makes it possible to use it with the legacy lantiq code and
also with the common clock framework. I see multiple options to fix this
problem.
1. The current approach to have it as a compile variant for a) legacy
lantiq arch code without common clock framework and b) support for SoCs
using the common clock framework.
2. Convert the lantiq arch code to the common clock framework. This
would be a good approach, but it need some efforts.
3. Remove the arch/mips/lantiq code. There are still users of this code.
4. Use the old APIs also for the new xRX500 SoC, I do not like this
approach.
5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally
available and provide some better wrapper code.
I don't really care what you do at this point in time, but you all
should know better than the crazy #ifdef is not allowed to try to
prevent/allow the inclusion of a .h file. Checkpatch might have even
warned you about it, right?
So do it correctly, odds are #5 is correct, as that makes it work like
any other device in the kernel. You are not unique here.
The best approach here would clearly be 2. We don't want platform
specific header files for doing things that should be completely generic.
Converting lantiq to the common-clk framework obviously requires
some work, but then again the whole arch/mips/lantiq/clk.c file
is fairly short and maybe not that hard to convert.
>From looking at arch/mips/lantiq/xway/sysctrl.c, it appears that you
already use the clkdev lookup mechanism for some devices without
using COMMON_CLK, so I would assume that you can also use those
for the remaining clks, which would be much simpler. It registers
one anonymous clk there as
clkdev_add_pmu("1e100c00.serial", NULL, 0, 0, PMU_ASC1);
so why not add replace that with two named clocks and just use
the same names in the DT for the newer chip?
Arnd
We discussed internally and have another solution for this issue.
Add one lantiq.h in the serial folder, and use "#ifdef preprocessor" in
lantiq.h,
also providing no-op stub functions in the #else case, then call those
functions
unconditionally from lantiq.c to avoid #ifdef in C file.
To support CCF in legacy product is another topic, is not included in
this patch.
The implementation is as followingï
#ifdef CONFIG_LANTIQ
#include <lantiq_soc.h>
#else
#define LTQ_EARLY_ASC 0
#define CPHYSADDR(_val) 0
static inline struct clk *clk_get_fpi(void)
{
return NULL;
}
#endif
Why not use clkdev_add(), as Arnd suggested?
That would be a 3-line patch without introducing a new header file and an ugly
#ifdef, which complicates compile coverage testing?
The reason we add a new head file is also for two macros(LTQ_EARLY_ASC
and CPHYSADDR)
used by legacy product. We need to provide the no-op stub for these two
macro for new product.
No you don't. The line number should not be obtained by comparing the
resource address with a hardcoded base address.
This is the previous code. Now the line number is obtained from dts.
We keep this code for the compatibility.
Referring to the conditional-compilation part in coding-style,
We add a header file to avoid using â#ifdefâ in C file.
Perhaps the override of port->line should just be removed, as IIRC, the serial
core has already filled in that field with the (next available) line number?
Gr{oetje,eeting}s,
Geert