At Mon, 27 Apr 2015 11:04:19 +0200,
Arnd Bergmann wrote:
>
> On Monday 27 April 2015 14:35:18 Yoshinori Sato wrote:
> > +static struct platform_driver cpg_driver = {
> > + .driver = {
> > + .name = DEVNAME,
> > + },
> > + .probe = clk_probe,
> > +};
> > +
> > +early_platform_init(DEVNAME, &cpg_driver);
> > +
> > +static struct platform_device clk_device = {
> > + .name = DEVNAME,
> > + .id = 0,
> > +};
> > +
> > +static struct platform_device *devices[] __initdata = {
> > + &clk_device,
> > +};
> > +
> > +int __init h8300_clk_init(int hz)
> > +{
> > + static int master_hz;
> > +
> > + master_hz = hz;
> > + clk_device.dev.platform_data = &master_hz;
> > + early_platform_add_devices(devices,
> > + ARRAY_SIZE(devices));
> > + early_platform_driver_register_all(DEVNAME);
> > + early_platform_driver_probe(DEVNAME, 1, 0);
> > + return 0;
> > +}
>
> Clock drivers are generally not 'platform_drivers'. Please do one of two
> things:
>
> a) use CLK_OF_DECLARE() to register a probe function, and use a device
> tree for describing your hardware
>
> b) rename clk_probe() to h8300_clk_init() and just call that function
> from the architecture code.
>
OK.
I'll think of "a".
>
> > +int __init h8300_clk_init(int hz)
> > +{
> > + static int master_hz;
> > +
> > + master_hz = hz;
> > + clk_device.dev.platform_data = &master_hz;
> > + early_platform_add_devices(devices,
> > + ARRAY_SIZE(devices));
> > + early_platform_driver_register_all(DEVNAME);
> > + early_platform_driver_probe(DEVNAME, 1, 0);
> > + return 0;
> > +}
>
> Here you have the same code again, which means the two files are mutually
> exclusive. This is generally not a good idea. Instead, it's better to
> make it possible to build a kernel that supports all the hardware, even
> if in practice you would not want to run that kernel.
>
> If you use CLK_OF_DECLARE(), it becomes trivial to make the two files
> coexist, otherwise use two different function names here and make the
> architecture code decide which one to call.
OK.
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index df69531..931860b 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -675,6 +675,18 @@ static inline void clk_writel(u32 val, u32 __iomem *reg)
> > iowrite32be(val, reg);
> > }
> >
> > +#elif IS_ENABLED(CONFIG_H8300)
> > +
> > +static inline u32 clk_readl(u32 __iomem *reg)
> > +{
> > + return __raw_readb(reg);
> > +}
> > +
> > +static inline void clk_writel(u32 val, u32 __iomem *reg)
> > +{
> > + __raw_writeb(val, reg);
> > +}
> > +
> > #else /* platform dependent I/O accessors */
>
> Why not use the same code as powerpc here? Drivers should normally
> not use __raw_* accessors, and the ioread32be on powerpc should work here
> as well.
>
> Arnd
Clock control regiser is 8bit.
So need byte access.
--
Yoshinori Sato
<ysato@xxxxxxxxxxxxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
