Re: [PATCH v2 1/4] clk: hisilicon: Add helper functions for platform driver

[Stephen Boyd]

Quoting David Yang (2023-03-29 00:50:50)
> diff --git a/drivers/clk/hisilicon/clk.h b/drivers/clk/hisilicon/clk.h
> index 7a9b42e1b027..def94498e14d 100644
> --- a/drivers/clk/hisilicon/clk.h
> +++ b/drivers/clk/hisilicon/clk.h
> @@ -103,6 +105,38 @@ struct hisi_gate_clock {
>         const char              *alias;
>  };
>  
> +struct hisi_clocks {
> +       int nr;
> +
> +       int (*prologue)(struct device *dev, struct hisi_clock_data *data);
> +
> +       const struct hisi_fixed_rate_clock *fixed_rate_clks;
> +       int fixed_rate_clks_num;

size_t for all these num types and 'nr'?

> +
> +       const struct hisi_fixed_factor_clock *fixed_factor_clks;
> +       int fixed_factor_clks_num;
> +
> +       const struct hisi_mux_clock *mux_clks;
> +       int mux_clks_num;
> +
> +       const struct hisi_phase_clock *phase_clks;
> +       int phase_clks_num;
> +
> +       const struct hisi_divider_clock *divider_clks;
> +       int divider_clks_num;
> +
> +       const struct hisi_gate_clock *gate_clks;
> +       int gate_clks_num;
> +
> +       const struct hisi_gate_clock *gate_sep_clks;
> +       int gate_sep_clks_num;
> +
> +       const void *customized_clks;
> +       int customized_clks_num;
> +       int (*clk_register_customized)(struct device *dev, const void *clks,
> +                                      int num, struct hisi_clock_data *data);
> +};
> +
>  struct clk *hisi_register_clkgate_sep(struct device *, const char *,
>                                 const char *, unsigned long,
>                                 void __iomem *, u8,
> @@ -134,7 +169,16 @@ void hisi_clk_register_gate_sep(const struct hisi_gate_clock *,
>  void hi6220_clk_register_divider(const struct hi6220_divider_clock *,
>                                 int, struct hisi_clock_data *);
>  
> -#define hisi_clk_unregister(type) \
> +int hisi_clk_register(struct device *dev, const struct hisi_clocks *clks,
> +                     struct hisi_clock_data *data);
> +
> +/* helper functions for platform driver */
> +
> +int hisi_clk_early_init(struct device_node *np, const struct hisi_clocks *clks);
> +int hisi_clk_probe(struct platform_device *pdev);
> +int hisi_clk_remove(struct platform_device *pdev);
> +
> +#define hisi_clk_unregister_fn(type) \
>  static inline \
>  void hisi_clk_unregister_##type(const struct hisi_##type##_clock *clks, \
>                                 int nums, struct hisi_clock_data *data) \
> @@ -148,10 +192,10 @@ void hisi_clk_unregister_##type(const struct hisi_##type##_clock *clks, \
>         } \
>  }
>  
> -hisi_clk_unregister(fixed_rate)
> -hisi_clk_unregister(fixed_factor)
> -hisi_clk_unregister(mux)
> -hisi_clk_unregister(divider)
> -hisi_clk_unregister(gate)
> +hisi_clk_unregister_fn(fixed_rate)
> +hisi_clk_unregister_fn(fixed_factor)
> +hisi_clk_unregister_fn(mux)
> +hisi_clk_unregister_fn(divider)
> +hisi_clk_unregister_fn(gate)

What does this hunk have to do with the $subject patch? Can you break
this series down into smaller patches? This is probably clearing the way
for hisi_clk_unregister() being an actual function, but I don't know. It
can 

>  
>  #endif /* __HISI_CLK_H */
> diff --git a/drivers/clk/hisilicon/crg.h b/drivers/clk/hisilicon/crg.h
> index 803f6ba6d7a2..d9544f1f2625 100644
> --- a/drivers/clk/hisilicon/crg.h
> +++ b/drivers/clk/hisilicon/crg.h
> @@ -22,4 +22,9 @@ struct hisi_crg_dev {
>         const struct hisi_crg_funcs *funcs;
>  };
>  
> +/* helper functions for platform driver */
> +
> +int hisi_crg_probe(struct platform_device *pdev);
> +int hisi_crg_remove(struct platform_device *pdev);
> +
>  #endif /* __HISI_CRG_H */
> diff --git a/drivers/clk/hisilicon/reset.c b/drivers/clk/hisilicon/reset.c
> index 93cee17db8b1..3464e3b4d708 100644
> --- a/drivers/clk/hisilicon/reset.c
> +++ b/drivers/clk/hisilicon/reset.c
> @@ -5,12 +5,16 @@
>   * Copyright (c) 2015-2016 HiSilicon Technologies Co., Ltd.
>   */
>  
> +#include <linux/device.h>
>  #include <linux/io.h>
> +#include <linux/kernel.h>
>  #include <linux/of_address.h>
> -#include <linux/platform_device.h>

Why?

> +#include <linux/of_device.h>

Why?

>  #include <linux/reset-controller.h>
> -#include <linux/slab.h>

Why?

>  #include <linux/spinlock.h>
> +
> +#include "clk.h"
> +#include "crg.h"
>  #include "reset.h"
>  
>  #define        HISI_RESET_BIT_MASK     0x1f