diff mbox series

[V4,09/11] clk: imx: add common imx_clk_hw_fixed functions

Message ID 1539504194-28289-10-git-send-email-aisheng.dong@nxp.com (mailing list archive)
State Changes Requested, archived
Headers show
Series clk: imx: add imx8qxp clock support | expand

Commit Message

Aisheng Dong Oct. 14, 2018, 8:08 a.m. UTC 
This may be used by both mmio and scu clks. So let's put it
into a common file.

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Michael Turquette <mturquette@baylibre.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/clk/imx/clk-common.h  | 16 ++++++++++++++++
 drivers/clk/imx/scu/clk-scu.h |  2 ++
 2 files changed, 18 insertions(+)
 create mode 100644 drivers/clk/imx/clk-common.h

Comments

Stephen Boyd Oct. 16, 2018, 9:32 p.m. UTC | #1 
Quoting A.s. Dong (2018-10-14 01:08:09)
> This may be used by both mmio and scu clks. So let's put it
> into a common file.
> 
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  drivers/clk/imx/clk-common.h  | 16 ++++++++++++++++
>  drivers/clk/imx/scu/clk-scu.h |  2 ++
>  2 files changed, 18 insertions(+)
>  create mode 100644 drivers/clk/imx/clk-common.h
> 
> diff --git a/drivers/clk/imx/clk-common.h b/drivers/clk/imx/clk-common.h
> new file mode 100644
> index 0000000..e3634a5
> --- /dev/null
> +++ b/drivers/clk/imx/clk-common.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright 2018 NXP
> + */
> +
> +#ifndef __IMX_CLK_COMMON_H
> +#define __IMX_CLK_COMMON_H
> +
> +#include <linux/clk-provider.h>
> +
> +static inline struct clk_hw *imx_clk_hw_fixed(const char *name, int rate)
> +{
> +       return clk_hw_register_fixed_rate(NULL, name, NULL, 0, rate);
> +}

Why do we need this wrapper file? Just call the registration function
directly with the right arguments instead of wrapping them in another
function please.
Aisheng Dong Oct. 17, 2018, 9:21 a.m. UTC | #2 
> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@kernel.org]
> Sent: Wednesday, October 17, 2018 5:33 AM
> Quoting A.s. Dong (2018-10-14 01:08:09)
> > This may be used by both mmio and scu clks. So let's put it into a
> > common file.
> >
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Cc: Stephen Boyd <sboyd@kernel.org>
> > Cc: Michael Turquette <mturquette@baylibre.com>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >  drivers/clk/imx/clk-common.h  | 16 ++++++++++++++++
> > drivers/clk/imx/scu/clk-scu.h |  2 ++
> >  2 files changed, 18 insertions(+)
> >  create mode 100644 drivers/clk/imx/clk-common.h
> >
> > diff --git a/drivers/clk/imx/clk-common.h
> > b/drivers/clk/imx/clk-common.h new file mode 100644 index
> > 0000000..e3634a5
> > --- /dev/null
> > +++ b/drivers/clk/imx/clk-common.h
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright 2018 NXP
> > + */
> > +
> > +#ifndef __IMX_CLK_COMMON_H
> > +#define __IMX_CLK_COMMON_H
> > +
> > +#include <linux/clk-provider.h>
> > +
> > +static inline struct clk_hw *imx_clk_hw_fixed(const char *name, int
> > +rate) {
> > +       return clk_hw_register_fixed_rate(NULL, name, NULL, 0, rate);
> > +}
> 
> Why do we need this wrapper file? 

This file clk-common.h is used to put the common bits between legacy clocks and scu clocks.
Only imx_clk_hw_fixed() is added in this patch.

> Just call the registration function directly
> with the right arguments instead of wrapping them in another function please.

That's legacy way and widely used before in order to save unnecessary parameters
for imx clocks.
e.g.
drivers/clk/imx/clk.h
static inline struct clk *imx_clk_fixed(const char *name, int rate)
{
        return clk_register_fixed_rate(NULL, name, NULL, 0, rate);
}

static inline struct clk *imx_clk_divider(const char *name, const char *parent,
                void __iomem *reg, u8 shift, u8 width)
{
        return clk_register_divider(NULL, name, parent, CLK_SET_RATE_PARENT,
                        reg, shift, width, 0, &imx_ccm_lock);
}

I can remove it if you dislike it.

Just one question, in later patch, another common function will also be added here:
static inline void imx_check_clk_hws(struct clk_hw *clks[], unsigned int count)
{
        unsigned int i;

        for (i = 0; i < count; i++) {
                if (IS_ERR(clks[i]))
                        pr_err("i.MX clk %u: register failed with %ld\n",
                                i, PTR_ERR(clks[i])); 
        }
}

Should we remove it as well?

Regards
Dong Aisheng
Stephen Boyd Oct. 17, 2018, 3:18 p.m. UTC | #3 
Quoting A.s. Dong (2018-10-17 02:21:57)
> > -----Original Message-----
> > From: Stephen Boyd [mailto:sboyd@kernel.org]
> > Sent: Wednesday, October 17, 2018 5:33 AM
> > Quoting A.s. Dong (2018-10-14 01:08:09)
> > > This may be used by both mmio and scu clks. So let's put it into a
> > > common file.
> > >
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > > Cc: Stephen Boyd <sboyd@kernel.org>
> > > Cc: Michael Turquette <mturquette@baylibre.com>
> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > ---
> > >  drivers/clk/imx/clk-common.h  | 16 ++++++++++++++++
> > > drivers/clk/imx/scu/clk-scu.h |  2 ++
> > >  2 files changed, 18 insertions(+)
> > >  create mode 100644 drivers/clk/imx/clk-common.h
> > >
> > > diff --git a/drivers/clk/imx/clk-common.h
> > > b/drivers/clk/imx/clk-common.h new file mode 100644 index
> > > 0000000..e3634a5
> > > --- /dev/null
> > > +++ b/drivers/clk/imx/clk-common.h
> > > @@ -0,0 +1,16 @@
> > > +/* SPDX-License-Identifier: GPL-2.0+ */
> > > +/*
> > > + * Copyright 2018 NXP
> > > + */
> > > +
> > > +#ifndef __IMX_CLK_COMMON_H
> > > +#define __IMX_CLK_COMMON_H
> > > +
> > > +#include <linux/clk-provider.h>
> > > +
> > > +static inline struct clk_hw *imx_clk_hw_fixed(const char *name, int
> > > +rate) {
> > > +       return clk_hw_register_fixed_rate(NULL, name, NULL, 0, rate);
> > > +}
> > 
> > Why do we need this wrapper file? 
> 
> This file clk-common.h is used to put the common bits between legacy clocks and scu clocks.
> Only imx_clk_hw_fixed() is added in this patch.
> 
> > Just call the registration function directly
> > with the right arguments instead of wrapping them in another function please.
> 
> That's legacy way and widely used before in order to save unnecessary parameters
> for imx clocks.
> e.g.
> drivers/clk/imx/clk.h
> static inline struct clk *imx_clk_fixed(const char *name, int rate)
> {
>         return clk_register_fixed_rate(NULL, name, NULL, 0, rate);
> }
> 
> static inline struct clk *imx_clk_divider(const char *name, const char *parent,
>                 void __iomem *reg, u8 shift, u8 width)
> {
>         return clk_register_divider(NULL, name, parent, CLK_SET_RATE_PARENT,
>                         reg, shift, width, 0, &imx_ccm_lock);
> }
> 
> I can remove it if you dislike it.
> 
> Just one question, in later patch, another common function will also be added here:
> static inline void imx_check_clk_hws(struct clk_hw *clks[], unsigned int count)
> {
>         unsigned int i;
> 
>         for (i = 0; i < count; i++) {
>                 if (IS_ERR(clks[i]))
>                         pr_err("i.MX clk %u: register failed with %ld\n",
>                                 i, PTR_ERR(clks[i])); 
>         }
> }
> 
> Should we remove it as well?
> 

Yes please remove this whole file.
diff mbox series

Patch

diff --git a/drivers/clk/imx/clk-common.h b/drivers/clk/imx/clk-common.h
new file mode 100644
index 0000000..e3634a5
--- /dev/null
+++ b/drivers/clk/imx/clk-common.h
@@ -0,0 +1,16 @@ 
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright 2018 NXP
+ */
+
+#ifndef __IMX_CLK_COMMON_H
+#define __IMX_CLK_COMMON_H
+
+#include <linux/clk-provider.h>
+
+static inline struct clk_hw *imx_clk_hw_fixed(const char *name, int rate)
+{
+	return clk_hw_register_fixed_rate(NULL, name, NULL, 0, rate);
+}
+
+#endif /* __IMX_CLK_COMMON_H */
diff --git a/drivers/clk/imx/scu/clk-scu.h b/drivers/clk/imx/scu/clk-scu.h
index 8cc58e4..4eeb252 100644
--- a/drivers/clk/imx/scu/clk-scu.h
+++ b/drivers/clk/imx/scu/clk-scu.h
@@ -11,6 +11,8 @@ 
 #include <linux/firmware/imx/sci.h>
 #include <linux/spinlock.h>
 
+#include "../clk-common.h"
+
 extern spinlock_t imx_ccm_lock;
 extern struct imx_sc_ipc *ccm_ipc_handle;