Re: [PATCH 16/16] h8300: misc functions

[Yoshinori Sato <ysato@users.sourceforge.jp>]

At Wed, 21 Jan 2015 10:01:59 +0100,
Arnd Bergmann wrote:
> 
> On Wednesday 21 January 2015 13:32:48 Yoshinori Sato wrote:
> 
> > +unsigned long clk_get_rate(struct clk *clk)
> > +{
> > +	return clk->parent->rate;
> > +}
> > +EXPORT_SYMBOL(clk_get_rate)
> > +
> > +int clk_enable(struct clk *clk)
> > +{
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(clk_enable)
> > +
> > +void clk_disable(struct clk *clk)
> > +{
> > +}
> > +EXPORT_SYMBOL(clk_disable)
> 
> If you are not too size constrained, you could consider using
> the common clk implementation from drivers/clk/.
> 
> > +static struct clk master_clk;
> > +
> > +static struct clk peripheral_clk = {
> > +	.parent = &master_clk,
> > +};
> > +
> > +static struct clk_lookup lookups[] = {
> > +	{
> > +		.con_id = "master_clk",
> > +		.clk = &master_clk,
> > +	},
> > +	{
> > +		.con_id = "peripheral_clk",
> > +		.clk = &peripheral_clk,
> > +	},
> > +};
> > +
> > +int __init h8300_clk_init(unsigned long hz)
> > +{
> > +	master_clk.rate = hz;
> > +	clkdev_add_table(lookups, ARRAY_SIZE(lookups));
> > +	return 0;
> > +}
> 
> Usually, we try to avoid global clock signal names to be requested by
> drivers. Instead you should list the clock input name for each device
> along with the device name, to allow drivers to be use the same string
> for the clock signal independent of how a device is wired.

OK.

> This would automatically work with dt.
> 
> > +EXPORT_SYMBOL(__gcc_bcmp);
> > +EXPORT_SYMBOL(__ashldi3);
> > +EXPORT_SYMBOL(__ashrdi3);
> > +EXPORT_SYMBOL(__cmpdi2);
> > +EXPORT_SYMBOL(__divdi3);
> > +EXPORT_SYMBOL(__divsi3);
> > +EXPORT_SYMBOL(__lshrdi3);
> > +EXPORT_SYMBOL(__moddi3);
> > +EXPORT_SYMBOL(__modsi3);
> > +EXPORT_SYMBOL(__muldi3);
> > +EXPORT_SYMBOL(__mulsi3);
> > +EXPORT_SYMBOL(__negdi2);
> > +EXPORT_SYMBOL(__ucmpdi2);
> > +EXPORT_SYMBOL(__udivdi3);
> > +EXPORT_SYMBOL(__udivmoddi4);
> 
> We don't normally use the __udivdi3/__udivmoddi4 functions from libgcc,
> instead Linux drivers are required to use the do_div() or div64()
> interfaces to do an expensive 64-bit division, to avoid accidentally
> doing an expensive computation that can be reduced to a cheaper 32-bit
> division. You can probably just drop these two symbols and still
> use libgcc, though that would not catch the built-in (non-loadable module)
> users.
> 
> An alternative would be to copy the implementation of the other functions
> from libgcc source into the kernel and just export the symbols you actually
> need. This also gives you better control over which implementation is
> used, to avoid a toolchain dependency in the kernel. Most architectures
> take this approach.

OK.
I will fix V2 patch.

> > diff --git a/arch/h8300/kernel/module.c b/arch/h8300/kernel/module.c
> > new file mode 100644
> > index 0000000..352aede
> > --- /dev/null
> > +++ b/arch/h8300/kernel/module.c
> > @@ -0,0 +1,75 @@
> > +#include <linux/moduleloader.h>
> > +#include <linux/elf.h>
> > +#include <linux/vmalloc.h>
> > +#include <linux/fs.h>
> > +#include <linux/string.h>
> > +#include <linux/kernel.h>
> > +
> > +#if 0
> > +#define DEBUGP printk
> > +#else
> > +#define DEBUGP(fmt...)
> > +#endif
> 
> You can use the existing pr_debug() to the same effect.
> 
> > diff --git a/arch/h8300/kernel/sys_h8300.c b/arch/h8300/kernel/sys_h8300.c
> > new file mode 100644
> > index 0000000..aa2302c
> > --- /dev/null
> > +++ b/arch/h8300/kernel/sys_h8300.c
> > @@ -0,0 +1,22 @@
> > +/*
> > + * linux/arch/h8300/kernel/sys_h8300.c
> > + *
> > + * This file contains various random system calls that
> > + * have a non-standard calling sequence on the H8/300
> > + * platform.
> > + */
> > +
> > +#include <asm/setup.h>
> > +#include <asm/uaccess.h>
> > +
> > +/* sys_cacheflush -- no support.  */
> > +asmlinkage int
> > +sys_cacheflush(unsigned long addr, int scope, int cache, unsigned long len)
> > +{
> > +	return -EINVAL;
> > +}
> > +
> > +asmlinkage int sys_getpagesize(void)
> > +{
> > +	return PAGE_SIZE;
> > +}
> 
> I think you can just drop these.
> 
> 	Arnd

OK
I will fix V2 patch.

-- 
Yoshinori Sato
<ysato@users.sourceforge.jp>