Re: [PATCH v2 14/18] serial: intel: Add CCF support

[Greg Kroah-Hartman]

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