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: > >> > >> > >> On 8/3/2018 1:56 PM, Greg Kroah-Hartman wrote: > >>> On Fri, Aug 03, 2018 at 11:02:33AM +0800, Songjun Wu wrote: > >>>> Previous implementation uses platform-dependent API to get the clock. > >>>> Those functions are not available for other SoC which uses the same IP. > >>>> The CCF (Common Clock Framework) have an abstraction based APIs for > >>>> clock. In future, the platform specific code will be removed when the > >>>> legacy soc use CCF as well. > >>>> Change to use CCF APIs to get clock and rate. So that different SoCs > >>>> can use the same driver. > >>>> > >>>> Signed-off-by: Songjun Wu <songjun.wu@xxxxxxxxxxxxxxx> > >>>> --- > >>>> > >>>> Changes in v2: None > >>>> > >>>> drivers/tty/serial/lantiq.c | 11 +++++++++++ > >>>> 1 file changed, 11 insertions(+) > >>>> > >>>> diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c > >>>> index 36479d66fb7c..35518ab3a80d 100644 > >>>> --- a/drivers/tty/serial/lantiq.c > >>>> +++ b/drivers/tty/serial/lantiq.c > >>>> @@ -26,7 +26,9 @@ > >>>> #include <linux/clk.h> > >>>> #include <linux/gpio.h> > >>>> +#ifdef CONFIG_LANTIQ > >>>> #include <lantiq_soc.h> > >>>> +#endif > >>> That is never how you do this in Linux, you know better. > >>> > >>> Please go and get this patchset reviewed and signed-off-by from other > >>> internal Intel kernel developers before resending it next time. It is > >>> their job to find and fix your basic errors like this, not ours. > >> Thank you for your comment. > >> Actually, we have discussed this issue internally. > >> We put the reason why we use "#ifdef CONFIG_LANTIQ" preprocessor in commit > >> message in "[PATCH v2 08/18] serial: intel: Get serial id from dts". > >> Please refer the commit message below. > >> > >> "#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC > >> macro is defined in lantiq_soc.h. > >> lantiq_soc.h is in arch path for legacy product support. > >> > >> arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h > >> > >> If "#ifdef preprocessor" is changed to > >> "if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled, > >> code using LTQ_EARLY_ASC is compiled. > >> Compilation will fail for no LTQ_EARLY_ASC defined. > > > > Sorry, but no. Why is this one tiny driver/chip somehow more "special" > > than all of the tens of thousands of other devices we support to warrent > > it getting some sort of special exception to do things differently? > > What happens to the next device that wants to do it this way? > > > > Our coding style and rules are there for a reason, do not violate them > > thinking your device is the only one that matters. > > > > Do it properly, again, you all know better than this. > > > > greg k-h > > > Hi Greg, > > The problem is that the Lantiq SoC code in arch/mips/lantiq does not use > the common clock framework, but it uses the clk framework directly. It > defines CONFIG_HAVE_CLK and CONFIG_CLKDEV_LOOKUP, but not > CONFIG_COMMON_CLK. The xRX500 SoC which is being added here is about 2 > generations more recent than the VR9/xRX200 SoC which is the latest > which is supported by the code in arch/mips/lantiq. With this new SoC we > switched to the common clock framework. This driver is used by the older > SoC and also by the new ones because this IP core is pretty similar in > all the SoCs. > 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. greg k-h
