Re: [RFC v3 06/16] clk: starfive: Add JH8100 System clock generator driver

[Stephen Boyd <sboyd@kernel.org>]

Quoting Sia Jee Heng (2024-01-10 05:31:18)
> diff --git a/drivers/clk/starfive/clk-starfive-jh8100-sys.c b/drivers/clk/starfive/clk-starfive-jh8100-sys.c
> new file mode 100644
> index 000000000000..6d7e750dce82
> --- /dev/null
> +++ b/drivers/clk/starfive/clk-starfive-jh8100-sys.c
> @@ -0,0 +1,415 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * StarFive JH8100 System Clock Driver
> + *
> + * Copyright (C) 2023 StarFive Technology Co., Ltd.
> + *
> + * Author: Jee Heng Sia <jeeheng.sia@starfivetech.com>
> + *
> + */
> +
> +#include <linux/clk.h>

Drop this unused include.

> +#include <linux/auxiliary_bus.h>
> +#include <linux/clk-provider.h>
> +#include <linux/init.h>
> +#include <linux/io.h>

Is this include used in this file?

> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include <soc/starfive/reset-starfive-common.h>
> +
> +#include <dt-bindings/clock/starfive,jh8100-crg.h>
> +
> +#include "clk-starfive-jh8100.h"
> +
> +#define JH8100_SYSCLK_NUM_CLKS                 (JH8100_SYSCLK_NNE_ICG_EN + 1)
> +
[...]
> +
> +static void jh8100_reset_adev_release(struct device *dev)
> +{
> +       struct auxiliary_device *adev = to_auxiliary_dev(dev);
> +       struct starfive_reset_adev *rdev = to_starfive_reset_adev(adev);
> +
> +       kfree(rdev);
> +}
> +
> +int jh8100_reset_controller_register(struct starfive_clk_priv *priv,

Just pass 'dev' and 'base' instead.

> +                                    const char *adev_name,
> +                                    u32 adev_id)
> +{
> +       struct starfive_reset_adev *rdev;
> +       struct auxiliary_device *adev;
> +       int ret;
> +
> +       rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
> +       if (!rdev)
> +               return -ENOMEM;
> +
> +       rdev->base = priv->base;
> +
> +       adev = &rdev->adev;
> +       adev->name = adev_name;
> +       adev->dev.parent = priv->dev;
> +       adev->dev.release = jh8100_reset_adev_release;
> +       adev->id = adev_id;
> +
> +       ret = auxiliary_device_init(adev);
> +       if (ret)
> +               return ret;
> +
> +       ret = auxiliary_device_add(adev);
> +       if (ret) {
> +               auxiliary_device_uninit(adev);
> +               return ret;
> +       }
> +
> +       return devm_add_action_or_reset(priv->dev,
> +                                       jh8100_reset_unregister_adev, adev);
> +}
> +EXPORT_SYMBOL_GPL(jh8100_reset_controller_register);

Move this to drivers/reset/ please.

> +
> +static int __init jh8100_syscrg_probe(struct platform_device *pdev)
> +{
> +       struct starfive_clk_priv *priv;
> +       unsigned int idx;
> +       int ret;
> +
> +       priv = devm_kzalloc(&pdev->dev,
> +                           struct_size(priv, reg, JH8100_SYSCLK_NUM_CLKS),
> +                           GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       spin_lock_init(&priv->rmw_lock);
> +       priv->dev = &pdev->dev;
> +       priv->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(priv->base))
> +               return PTR_ERR(priv->base);
[...]
> +
> +       ret = devm_of_clk_add_hw_provider(&pdev->dev, jh8100_sysclk_get, priv);
> +       if (ret)
> +               return ret;
> +
> +       return jh8100_reset_controller_register(priv, "rst-sys", 0);
> +}
> +
> +static const struct of_device_id jh8100_syscrg_match[] = {
> +       { .compatible = "starfive,jh8100-syscrg" },
> +       { /* sentinel */ }
> +};
> +
> +static struct platform_driver jh8100_syscrg_driver = {
> +       .driver = {
> +               .name = "clk-starfive-jh8100-sys",
> +               .of_match_table = jh8100_syscrg_match,
> +               .suppress_bind_attrs = true,
> +       },
> +};
> +builtin_platform_driver_probe(jh8100_syscrg_driver, jh8100_syscrg_probe);
> diff --git a/drivers/clk/starfive/clk-starfive-jh8100.h b/drivers/clk/starfive/clk-starfive-jh8100.h
> new file mode 100644
> index 000000000000..9b69a56fe5fc
> --- /dev/null
> +++ b/drivers/clk/starfive/clk-starfive-jh8100.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __CLK_STARFIVE_JH8100_H
> +#define __CLK_STARFIVE_JH8100_H
> +
> +#include "clk-starfive-common.h"

Drop this include.

> +
> +int jh8100_reset_controller_register(struct starfive_clk_priv *priv,

Forward declare starfive_clk_priv instead.

> +                                    const char *adev_name,
> +                                    u32 adev_id);

Why is this header here at all?