Re: [PATCH v9 11/17] h8300: clock driver

[Arnd Bergmann <arnd@arndb.de>]

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.


> +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.

> 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