mbox series

[00/18] clk: meson: move all private clk IDs to public dt-bindings headers

Message ID 20230607-topic-amlogic-upstream-clkid-public-migration-v1-0-9676afa6b22c@linaro.org
Headers show
Series clk: meson: move all private clk IDs to public dt-bindings headers | expand

Message

Neil Armstrong June 7, 2023, 10:56 a.m. UTC 
After some complaints in the upstreaming of the A1 clock drivers,
S4 clock driver and a tentative to use some of the private DSI
clocks in [1], it has been decided to move out all the "private"
clk IDs to public dt-bindings headers.

For that we must get rid of the "NR_CLKS" define and use
ARRAY_SIZE() to get the count of hw_clks, then we can move
the IDs and do some cleanup.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Neil Armstrong (18):
      clk: meson: migrate meson-eeclk out of hw_onecell_data to drop NR_CLKS
      clk: meson: migrate meson-aoclk out of hw_onecell_data to drop NR_CLKS
      clk: meson: migrate a1 clock drivers out of hw_onecell_data to drop NR_CLKS
      clk: meson: migrate meson8b out of hw_onecell_data to drop NR_CLKS
      clk: meson: migrate axg-audio out of hw_onecell_data to drop NR_CLKS
      dt-bindings: clk: gxbb-clkc: expose all clock ids
      dt-bindings: clk: axg-clkc: expose all clock ids
      dt-bindings: clk: g12a-clks: expose all clock ids
      dt-bindings: clk: g12a-aoclkc: expose all clock ids
      dt-bindings: clk: meson8b-clkc: expose all clock ids
      dt-bindings: clk: amlogic,a1-peripherals-clkc: expose all clock ids
      dt-bindings: clk: amlogic,a1-pll-clkc: expose all clock ids
      dt-bindings: clk: axg-audio-clkc: expose all clock ids
      clk: meson: aoclk: move bindings include to main driver
      clk: meson: eeclk: move bindings include to main driver
      clk: meson: a1: move bindings include to main driver
      clk: meson: meson8b: move bindings include to main driver
      clk: meson: axg-audio: move bindings include to main driver

 drivers/clk/meson/a1-peripherals.c                 |  345 ++---
 drivers/clk/meson/a1-peripherals.h                 |   67 -
 drivers/clk/meson/a1-pll.c                         |   59 +-
 drivers/clk/meson/a1-pll.h                         |   19 -
 drivers/clk/meson/axg-aoclk.c                      |   46 +-
 drivers/clk/meson/axg-aoclk.h                      |   18 -
 drivers/clk/meson/axg-audio.c                      |  858 +++++------
 drivers/clk/meson/axg-audio.h                      |   75 -
 drivers/clk/meson/axg.c                            |  283 ++--
 drivers/clk/meson/axg.h                            |   63 -
 drivers/clk/meson/g12a-aoclk.c                     |   70 +-
 drivers/clk/meson/g12a-aoclk.h                     |   32 -
 drivers/clk/meson/g12a.c                           | 1483 ++++++++++----------
 drivers/clk/meson/g12a.h                           |  145 --
 drivers/clk/meson/gxbb-aoclk.c                     |   12 +-
 drivers/clk/meson/gxbb-aoclk.h                     |   15 -
 drivers/clk/meson/gxbb.c                           |  844 ++++++-----
 drivers/clk/meson/gxbb.h                           |   81 --
 drivers/clk/meson/meson-aoclk.c                    |   22 +-
 drivers/clk/meson/meson-aoclk.h                    |    3 +-
 drivers/clk/meson/meson-eeclk.c                    |   22 +-
 drivers/clk/meson/meson-eeclk.h                    |    3 +-
 drivers/clk/meson/meson8b.c                        | 1335 +++++++++---------
 drivers/clk/meson/meson8b.h                        |  117 --
 .../clock/amlogic,a1-peripherals-clkc.h            |   53 +
 include/dt-bindings/clock/amlogic,a1-pll-clkc.h    |    5 +
 include/dt-bindings/clock/axg-audio-clkc.h         |   65 +
 include/dt-bindings/clock/axg-clkc.h               |   48 +
 include/dt-bindings/clock/g12a-aoclkc.h            |    7 +
 include/dt-bindings/clock/g12a-clkc.h              |  130 ++
 include/dt-bindings/clock/gxbb-clkc.h              |   65 +
 include/dt-bindings/clock/meson8b-clkc.h           |   97 ++
 32 files changed, 3205 insertions(+), 3282 deletions(-)
---
base-commit: 84af914404dbc01f388c440cac72428784b8a161
change-id: 20230607-topic-amlogic-upstream-clkid-public-migration-fc1c67c44858

Best regards,

Comments

Jerome Brunet June 8, 2023, 12:45 p.m. UTC | #1 
>  
> +struct meson_a1_pll_clks {
> +	struct clk_hw **hw_clks;
> +	unsigned int hw_clk_num;
> +};
> +
> +static struct meson_a1_pll_clks a1_pll_clks = {
> +	.hw_clks = a1_pll_hw_clks,
> +	.hw_clk_num = ARRAY_SIZE(a1_pll_hw_clks),
> +};
> +
> +static struct clk_hw *meson_a1_pll_hw_get(struct of_phandle_args *clkspec, void *clk_data)
> +{
> +	const struct meson_a1_pll_clks *data = clk_data;
> +	unsigned int idx = clkspec->args[0];
> +
> +	if (idx >= data->hw_clk_num) {
> +		pr_err("%s: invalid index %u\n", __func__, idx);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	return data->hw_clks[idx];
> +}

I'd prefer to have a single struct type and and single custom
callback for the different SoC please.
Dmitry Rokosov June 9, 2023, 11:48 a.m. UTC | #2 
Hello Neil,

On Thu, Jun 08, 2023 at 02:53:50PM +0200, Neil Armstrong wrote:
> On 08/06/2023 14:45, Jerome Brunet wrote:
> > > +struct meson_a1_pll_clks {
> > > +	struct clk_hw **hw_clks;
> > > +	unsigned int hw_clk_num;
> > > +};
> > > +
> > > +static struct meson_a1_pll_clks a1_pll_clks = {
> > > +	.hw_clks = a1_pll_hw_clks,
> > > +	.hw_clk_num = ARRAY_SIZE(a1_pll_hw_clks),
> > > +};
> > > +
> > > +static struct clk_hw *meson_a1_pll_hw_get(struct of_phandle_args *clkspec, void *clk_data)
> > > +{
> > > +	const struct meson_a1_pll_clks *data = clk_data;
> > > +	unsigned int idx = clkspec->args[0];
> > > +
> > > +	if (idx >= data->hw_clk_num) {
> > > +		pr_err("%s: invalid index %u\n", __func__, idx);
> > > +		return ERR_PTR(-EINVAL);
> > > +	}
> > > +
> > > +	return data->hw_clks[idx];
> > > +}
> > 
> > I'd prefer to have a single struct type and and single custom
> > callback for the different SoC please.
> 
> Sure, I've written a common code for that, but I have a hard time finding
> a proper naming for it... so I choosed meson-clkc since it could have
> more common helper code for duplicated code over the clk driver:
> 
> ===================================><============================================================================
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index 8ce846fdbe43..9070dcfd9e71 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -30,6 +30,9 @@ config COMMON_CLK_MESON_VID_PLL_DIV
>  	tristate
>  	select COMMON_CLK_MESON_REGMAP
> 
> +config COMMON_CLK_MESON_CLKC
> +	tristate
> +
>  config COMMON_CLK_MESON_AO_CLKC
>  	tristate
>  	select COMMON_CLK_MESON_REGMAP
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index d5288662881d..13c6db466986 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  # Amlogic clock drivers
> 
> +obj-$(CONFIG_COMMON_CLK_MESON_CLKC) += meson-clkc.o
>  obj-$(CONFIG_COMMON_CLK_MESON_AO_CLKC) += meson-aoclk.o
>  obj-$(CONFIG_COMMON_CLK_MESON_CPU_DYNDIV) += clk-cpu-dyndiv.o
>  obj-$(CONFIG_COMMON_CLK_MESON_DUALDIV) += clk-dualdiv.o
> diff --git a/drivers/clk/meson/meson-clkc.c b/drivers/clk/meson/meson-clkc.c
> new file mode 100644
> index 000000000000..fa98b9d09011
> --- /dev/null
> +++ b/drivers/clk/meson/meson-clkc.c
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2023 Neil Armstrong <neil.armstrong@linaro.org>
> + */
> +
> +#include <linux/of_device.h>
> +#include <linux/clk-provider.h>
> +#include <linux/module.h>
> +#include "meson-clkc.h"
> +
> +struct clk_hw *meson_clk_hw_get(struct of_phandle_args *clkspec, void *clk_hw_data)
> +{
> +	const struct meson_clk_hw_data *data = clk_hw_data;
> +	unsigned int idx = clkspec->args[0];
> +
> +	if (idx >= data->num) {
> +		pr_err("%s: invalid index %u\n", __func__, idx);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	return data->hws[idx];
> +}
> +EXPORT_SYMBOL_GPL(meson_clk_hw_get);
> +
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/clk/meson/meson-clkc.h b/drivers/clk/meson/meson-clkc.h
> new file mode 100644
> index 000000000000..e3bad2aa17eb
> --- /dev/null
> +++ b/drivers/clk/meson/meson-clkc.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> +/*
> + * Copyright (c) 2023 Neil Armstrong <neil.armstrong@linaro.org>
> + */
> +
> +#ifndef __MESON_HW_CLKC_H__
> +#define __MESON_HW_CLKC_H__
> +
> +#include <linux/of_device.h>
> +#include <linux/clk-provider.h>
> +
> +struct meson_clk_hw_data {
> +	struct clk_hw	**hws;
> +	unsigned int	num;
> +};
> +
> +struct clk_hw *meson_clk_hw_get(struct of_phandle_args *clkspec, void *clk_hw_data);
> +
> +#endif
> ===================================><============================================================================
> 
> If it's ok I'll send a v2 using this.
> 
> Thanks,
> Neil

In addition, I propose consolidating the probe() routine of the a1
clocks into a common part, which can be utilized for s4 and other
similar clocks. This solution was presented in the early a1 review
stages of this patch set.

https://lore.kernel.org/linux-amlogic/20221201225703.6507-7-ddrokosov@sberdevices.ru/

Maybe, it can be generalized for the all meson clock controller drivers.
Neil Armstrong June 9, 2023, 12:47 p.m. UTC | #3 
Hi,

On 09/06/2023 13:48, Dmitry Rokosov wrote:
> Hello Neil,
> 
> On Thu, Jun 08, 2023 at 02:53:50PM +0200, Neil Armstrong wrote:
>> On 08/06/2023 14:45, Jerome Brunet wrote:
>>>> +struct meson_a1_pll_clks {
>>>> +	struct clk_hw **hw_clks;
>>>> +	unsigned int hw_clk_num;
>>>> +};
>>>> +
>>>> +static struct meson_a1_pll_clks a1_pll_clks = {
>>>> +	.hw_clks = a1_pll_hw_clks,
>>>> +	.hw_clk_num = ARRAY_SIZE(a1_pll_hw_clks),
>>>> +};
>>>> +
>>>> +static struct clk_hw *meson_a1_pll_hw_get(struct of_phandle_args *clkspec, void *clk_data)
>>>> +{
>>>> +	const struct meson_a1_pll_clks *data = clk_data;
>>>> +	unsigned int idx = clkspec->args[0];
>>>> +
>>>> +	if (idx >= data->hw_clk_num) {
>>>> +		pr_err("%s: invalid index %u\n", __func__, idx);
>>>> +		return ERR_PTR(-EINVAL);
>>>> +	}
>>>> +
>>>> +	return data->hw_clks[idx];
>>>> +}
>>>
>>> I'd prefer to have a single struct type and and single custom
>>> callback for the different SoC please.
>>
>> Sure, I've written a common code for that, but I have a hard time finding
>> a proper naming for it... so I choosed meson-clkc since it could have
>> more common helper code for duplicated code over the clk driver:
>>
>> ===================================><============================================================================
>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>> index 8ce846fdbe43..9070dcfd9e71 100644
>> --- a/drivers/clk/meson/Kconfig
>> +++ b/drivers/clk/meson/Kconfig
>> @@ -30,6 +30,9 @@ config COMMON_CLK_MESON_VID_PLL_DIV
>>   	tristate
>>   	select COMMON_CLK_MESON_REGMAP
>>
>> +config COMMON_CLK_MESON_CLKC
>> +	tristate
>> +
>>   config COMMON_CLK_MESON_AO_CLKC
>>   	tristate
>>   	select COMMON_CLK_MESON_REGMAP
>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>> index d5288662881d..13c6db466986 100644
>> --- a/drivers/clk/meson/Makefile
>> +++ b/drivers/clk/meson/Makefile
>> @@ -1,6 +1,7 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>>   # Amlogic clock drivers
>>
>> +obj-$(CONFIG_COMMON_CLK_MESON_CLKC) += meson-clkc.o
>>   obj-$(CONFIG_COMMON_CLK_MESON_AO_CLKC) += meson-aoclk.o
>>   obj-$(CONFIG_COMMON_CLK_MESON_CPU_DYNDIV) += clk-cpu-dyndiv.o
>>   obj-$(CONFIG_COMMON_CLK_MESON_DUALDIV) += clk-dualdiv.o
>> diff --git a/drivers/clk/meson/meson-clkc.c b/drivers/clk/meson/meson-clkc.c
>> new file mode 100644
>> index 000000000000..fa98b9d09011
>> --- /dev/null
>> +++ b/drivers/clk/meson/meson-clkc.c
>> @@ -0,0 +1,25 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (c) 2023 Neil Armstrong <neil.armstrong@linaro.org>
>> + */
>> +
>> +#include <linux/of_device.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/module.h>
>> +#include "meson-clkc.h"
>> +
>> +struct clk_hw *meson_clk_hw_get(struct of_phandle_args *clkspec, void *clk_hw_data)
>> +{
>> +	const struct meson_clk_hw_data *data = clk_hw_data;
>> +	unsigned int idx = clkspec->args[0];
>> +
>> +	if (idx >= data->num) {
>> +		pr_err("%s: invalid index %u\n", __func__, idx);
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	return data->hws[idx];
>> +}
>> +EXPORT_SYMBOL_GPL(meson_clk_hw_get);
>> +
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/clk/meson/meson-clkc.h b/drivers/clk/meson/meson-clkc.h
>> new file mode 100644
>> index 000000000000..e3bad2aa17eb
>> --- /dev/null
>> +++ b/drivers/clk/meson/meson-clkc.h
>> @@ -0,0 +1,19 @@
>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>> +/*
>> + * Copyright (c) 2023 Neil Armstrong <neil.armstrong@linaro.org>
>> + */
>> +
>> +#ifndef __MESON_HW_CLKC_H__
>> +#define __MESON_HW_CLKC_H__
>> +
>> +#include <linux/of_device.h>
>> +#include <linux/clk-provider.h>
>> +
>> +struct meson_clk_hw_data {
>> +	struct clk_hw	**hws;
>> +	unsigned int	num;
>> +};
>> +
>> +struct clk_hw *meson_clk_hw_get(struct of_phandle_args *clkspec, void *clk_hw_data);
>> +
>> +#endif
>> ===================================><============================================================================
>>
>> If it's ok I'll send a v2 using this.
>>
>> Thanks,
>> Neil
> 
> In addition, I propose consolidating the probe() routine of the a1
> clocks into a common part, which can be utilized for s4 and other
> similar clocks. This solution was presented in the early a1 review
> stages of this patch set.
> 
> https://lore.kernel.org/linux-amlogic/20221201225703.6507-7-ddrokosov@sberdevices.ru/
> 
> Maybe, it can be generalized for the all meson clock controller drivers.
> 

Sure, the goal of that is to get rid of NR_CLKS & private IDS only,
then yes afterwards any common clkc function will be welcome !

Neil
Dmitry Rokosov June 9, 2023, 12:57 p.m. UTC | #4 
On Fri, Jun 09, 2023 at 02:47:04PM +0200, Neil Armstrong wrote:
> Hi,
> 
> On 09/06/2023 13:48, Dmitry Rokosov wrote:
> > Hello Neil,
> > 
> > On Thu, Jun 08, 2023 at 02:53:50PM +0200, Neil Armstrong wrote:
> > > On 08/06/2023 14:45, Jerome Brunet wrote:
> > > > > +struct meson_a1_pll_clks {
> > > > > +	struct clk_hw **hw_clks;
> > > > > +	unsigned int hw_clk_num;
> > > > > +};
> > > > > +
> > > > > +static struct meson_a1_pll_clks a1_pll_clks = {
> > > > > +	.hw_clks = a1_pll_hw_clks,
> > > > > +	.hw_clk_num = ARRAY_SIZE(a1_pll_hw_clks),
> > > > > +};
> > > > > +
> > > > > +static struct clk_hw *meson_a1_pll_hw_get(struct of_phandle_args *clkspec, void *clk_data)
> > > > > +{
> > > > > +	const struct meson_a1_pll_clks *data = clk_data;
> > > > > +	unsigned int idx = clkspec->args[0];
> > > > > +
> > > > > +	if (idx >= data->hw_clk_num) {
> > > > > +		pr_err("%s: invalid index %u\n", __func__, idx);
> > > > > +		return ERR_PTR(-EINVAL);
> > > > > +	}
> > > > > +
> > > > > +	return data->hw_clks[idx];
> > > > > +}
> > > > 
> > > > I'd prefer to have a single struct type and and single custom
> > > > callback for the different SoC please.
> > > 
> > > Sure, I've written a common code for that, but I have a hard time finding
> > > a proper naming for it... so I choosed meson-clkc since it could have
> > > more common helper code for duplicated code over the clk driver:
> > > 
> > > ===================================><============================================================================
> > > diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> > > index 8ce846fdbe43..9070dcfd9e71 100644
> > > --- a/drivers/clk/meson/Kconfig
> > > +++ b/drivers/clk/meson/Kconfig
> > > @@ -30,6 +30,9 @@ config COMMON_CLK_MESON_VID_PLL_DIV
> > >   	tristate
> > >   	select COMMON_CLK_MESON_REGMAP
> > > 
> > > +config COMMON_CLK_MESON_CLKC
> > > +	tristate
> > > +
> > >   config COMMON_CLK_MESON_AO_CLKC
> > >   	tristate
> > >   	select COMMON_CLK_MESON_REGMAP
> > > diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> > > index d5288662881d..13c6db466986 100644
> > > --- a/drivers/clk/meson/Makefile
> > > +++ b/drivers/clk/meson/Makefile
> > > @@ -1,6 +1,7 @@
> > >   # SPDX-License-Identifier: GPL-2.0-only
> > >   # Amlogic clock drivers
> > > 
> > > +obj-$(CONFIG_COMMON_CLK_MESON_CLKC) += meson-clkc.o
> > >   obj-$(CONFIG_COMMON_CLK_MESON_AO_CLKC) += meson-aoclk.o
> > >   obj-$(CONFIG_COMMON_CLK_MESON_CPU_DYNDIV) += clk-cpu-dyndiv.o
> > >   obj-$(CONFIG_COMMON_CLK_MESON_DUALDIV) += clk-dualdiv.o
> > > diff --git a/drivers/clk/meson/meson-clkc.c b/drivers/clk/meson/meson-clkc.c
> > > new file mode 100644
> > > index 000000000000..fa98b9d09011
> > > --- /dev/null
> > > +++ b/drivers/clk/meson/meson-clkc.c
> > > @@ -0,0 +1,25 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright (c) 2023 Neil Armstrong <neil.armstrong@linaro.org>
> > > + */
> > > +
> > > +#include <linux/of_device.h>
> > > +#include <linux/clk-provider.h>
> > > +#include <linux/module.h>
> > > +#include "meson-clkc.h"
> > > +
> > > +struct clk_hw *meson_clk_hw_get(struct of_phandle_args *clkspec, void *clk_hw_data)
> > > +{
> > > +	const struct meson_clk_hw_data *data = clk_hw_data;
> > > +	unsigned int idx = clkspec->args[0];
> > > +
> > > +	if (idx >= data->num) {
> > > +		pr_err("%s: invalid index %u\n", __func__, idx);
> > > +		return ERR_PTR(-EINVAL);
> > > +	}
> > > +
> > > +	return data->hws[idx];
> > > +}
> > > +EXPORT_SYMBOL_GPL(meson_clk_hw_get);
> > > +
> > > +MODULE_LICENSE("GPL v2");
> > > diff --git a/drivers/clk/meson/meson-clkc.h b/drivers/clk/meson/meson-clkc.h
> > > new file mode 100644
> > > index 000000000000..e3bad2aa17eb
> > > --- /dev/null
> > > +++ b/drivers/clk/meson/meson-clkc.h
> > > @@ -0,0 +1,19 @@
> > > +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> > > +/*
> > > + * Copyright (c) 2023 Neil Armstrong <neil.armstrong@linaro.org>
> > > + */
> > > +
> > > +#ifndef __MESON_HW_CLKC_H__
> > > +#define __MESON_HW_CLKC_H__
> > > +
> > > +#include <linux/of_device.h>
> > > +#include <linux/clk-provider.h>
> > > +
> > > +struct meson_clk_hw_data {
> > > +	struct clk_hw	**hws;
> > > +	unsigned int	num;
> > > +};
> > > +
> > > +struct clk_hw *meson_clk_hw_get(struct of_phandle_args *clkspec, void *clk_hw_data);
> > > +
> > > +#endif
> > > ===================================><============================================================================
> > > 
> > > If it's ok I'll send a v2 using this.
> > > 
> > > Thanks,
> > > Neil
> > 
> > In addition, I propose consolidating the probe() routine of the a1
> > clocks into a common part, which can be utilized for s4 and other
> > similar clocks. This solution was presented in the early a1 review
> > stages of this patch set.
> > 
> > https://lore.kernel.org/linux-amlogic/20221201225703.6507-7-ddrokosov@sberdevices.ru/
> > 
> > Maybe, it can be generalized for the all meson clock controller drivers.
> > 
> 
> Sure, the goal of that is to get rid of NR_CLKS & private IDS only,
> then yes afterwards any common clkc function will be welcome !
> 
> Neil
> 

Understood, I fully endorse and support this approach!
Jerome Brunet June 9, 2023, 2:30 p.m. UTC | #5 
On Thu 08 Jun 2023 at 14:53, Neil Armstrong <neil.armstrong@linaro.org> wrote:

> On 08/06/2023 14:45, Jerome Brunet wrote:
>>>   +struct meson_a1_pll_clks {
>>> +	struct clk_hw **hw_clks;
>>> +	unsigned int hw_clk_num;
>>> +};
>>> +
>>> +static struct meson_a1_pll_clks a1_pll_clks = {
>>> +	.hw_clks = a1_pll_hw_clks,
>>> +	.hw_clk_num = ARRAY_SIZE(a1_pll_hw_clks),
>>> +};
>>> +
>>> +static struct clk_hw *meson_a1_pll_hw_get(struct of_phandle_args *clkspec, void *clk_data)
>>> +{
>>> +	const struct meson_a1_pll_clks *data = clk_data;
>>> +	unsigned int idx = clkspec->args[0];
>>> +
>>> +	if (idx >= data->hw_clk_num) {
>>> +		pr_err("%s: invalid index %u\n", __func__, idx);
>>> +		return ERR_PTR(-EINVAL);
>>> +	}
>>> +
>>> +	return data->hw_clks[idx];
>>> +}
>> I'd prefer to have a single struct type and and single custom
>> callback for the different SoC please.
>
> Sure, I've written a common code for that, but I have a hard time finding
> a proper naming for it... so I choosed meson-clkc since it could have
> more common helper code for duplicated code over the clk driver:

Agreed. meson-clkc-utils maybe ?

>
> ===================================><============================================================================
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index 8ce846fdbe43..9070dcfd9e71 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -30,6 +30,9 @@ config COMMON_CLK_MESON_VID_PLL_DIV
>  	tristate
>  	select COMMON_CLK_MESON_REGMAP
>
> +config COMMON_CLK_MESON_CLKC
> +	tristate
> +
>  config COMMON_CLK_MESON_AO_CLKC
>  	tristate
>  	select COMMON_CLK_MESON_REGMAP
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index d5288662881d..13c6db466986 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  # Amlogic clock drivers
>
> +obj-$(CONFIG_COMMON_CLK_MESON_CLKC) += meson-clkc.o
>  obj-$(CONFIG_COMMON_CLK_MESON_AO_CLKC) += meson-aoclk.o
>  obj-$(CONFIG_COMMON_CLK_MESON_CPU_DYNDIV) += clk-cpu-dyndiv.o
>  obj-$(CONFIG_COMMON_CLK_MESON_DUALDIV) += clk-dualdiv.o
> diff --git a/drivers/clk/meson/meson-clkc.c b/drivers/clk/meson/meson-clkc.c
> new file mode 100644
> index 000000000000..fa98b9d09011
> --- /dev/null
> +++ b/drivers/clk/meson/meson-clkc.c
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2023 Neil Armstrong <neil.armstrong@linaro.org>
> + */
> +
> +#include <linux/of_device.h>
> +#include <linux/clk-provider.h>
> +#include <linux/module.h>
> +#include "meson-clkc.h"
> +
> +struct clk_hw *meson_clk_hw_get(struct of_phandle_args *clkspec, void *clk_hw_data)
> +{
> +	const struct meson_clk_hw_data *data = clk_hw_data;
> +	unsigned int idx = clkspec->args[0];
> +
> +	if (idx >= data->num) {
> +		pr_err("%s: invalid index %u\n", __func__, idx);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	return data->hws[idx];
> +}
> +EXPORT_SYMBOL_GPL(meson_clk_hw_get);
> +
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/clk/meson/meson-clkc.h b/drivers/clk/meson/meson-clkc.h
> new file mode 100644
> index 000000000000..e3bad2aa17eb
> --- /dev/null
> +++ b/drivers/clk/meson/meson-clkc.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> +/*
> + * Copyright (c) 2023 Neil Armstrong <neil.armstrong@linaro.org>
> + */
> +
> +#ifndef __MESON_HW_CLKC_H__
> +#define __MESON_HW_CLKC_H__
> +
> +#include <linux/of_device.h>
> +#include <linux/clk-provider.h>
> +
> +struct meson_clk_hw_data {
> +	struct clk_hw	**hws;
> +	unsigned int	num;
> +};
> +
> +struct clk_hw *meson_clk_hw_get(struct of_phandle_args *clkspec, void *clk_hw_data);
> +
> +#endif
> ===================================><============================================================================
>
> If it's ok I'll send a v2 using this.
>
> Thanks,
> Neil
Neil Armstrong June 9, 2023, 3:32 p.m. UTC | #6 
On 09/06/2023 16:30, Jerome Brunet wrote:
> 
> On Thu 08 Jun 2023 at 14:53, Neil Armstrong <neil.armstrong@linaro.org> wrote:
> 
>> On 08/06/2023 14:45, Jerome Brunet wrote:
>>>>    +struct meson_a1_pll_clks {
>>>> +	struct clk_hw **hw_clks;
>>>> +	unsigned int hw_clk_num;
>>>> +};
>>>> +
>>>> +static struct meson_a1_pll_clks a1_pll_clks = {
>>>> +	.hw_clks = a1_pll_hw_clks,
>>>> +	.hw_clk_num = ARRAY_SIZE(a1_pll_hw_clks),
>>>> +};
>>>> +
>>>> +static struct clk_hw *meson_a1_pll_hw_get(struct of_phandle_args *clkspec, void *clk_data)
>>>> +{
>>>> +	const struct meson_a1_pll_clks *data = clk_data;
>>>> +	unsigned int idx = clkspec->args[0];
>>>> +
>>>> +	if (idx >= data->hw_clk_num) {
>>>> +		pr_err("%s: invalid index %u\n", __func__, idx);
>>>> +		return ERR_PTR(-EINVAL);
>>>> +	}
>>>> +
>>>> +	return data->hw_clks[idx];
>>>> +}
>>> I'd prefer to have a single struct type and and single custom
>>> callback for the different SoC please.
>>
>> Sure, I've written a common code for that, but I have a hard time finding
>> a proper naming for it... so I choosed meson-clkc since it could have
>> more common helper code for duplicated code over the clk driver:
> 
> Agreed. meson-clkc-utils maybe ?

Ack seems better

Thanks
Neil

> 
>>
>> ===================================><============================================================================
>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>> index 8ce846fdbe43..9070dcfd9e71 100644
>> --- a/drivers/clk/meson/Kconfig
>> +++ b/drivers/clk/meson/Kconfig
>> @@ -30,6 +30,9 @@ config COMMON_CLK_MESON_VID_PLL_DIV
>>   	tristate
>>   	select COMMON_CLK_MESON_REGMAP
>>
>> +config COMMON_CLK_MESON_CLKC
>> +	tristate
>> +
>>   config COMMON_CLK_MESON_AO_CLKC
>>   	tristate
>>   	select COMMON_CLK_MESON_REGMAP
>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>> index d5288662881d..13c6db466986 100644
>> --- a/drivers/clk/meson/Makefile
>> +++ b/drivers/clk/meson/Makefile
>> @@ -1,6 +1,7 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>>   # Amlogic clock drivers
>>
>> +obj-$(CONFIG_COMMON_CLK_MESON_CLKC) += meson-clkc.o
>>   obj-$(CONFIG_COMMON_CLK_MESON_AO_CLKC) += meson-aoclk.o
>>   obj-$(CONFIG_COMMON_CLK_MESON_CPU_DYNDIV) += clk-cpu-dyndiv.o
>>   obj-$(CONFIG_COMMON_CLK_MESON_DUALDIV) += clk-dualdiv.o
>> diff --git a/drivers/clk/meson/meson-clkc.c b/drivers/clk/meson/meson-clkc.c
>> new file mode 100644
>> index 000000000000..fa98b9d09011
>> --- /dev/null
>> +++ b/drivers/clk/meson/meson-clkc.c
>> @@ -0,0 +1,25 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (c) 2023 Neil Armstrong <neil.armstrong@linaro.org>
>> + */
>> +
>> +#include <linux/of_device.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/module.h>
>> +#include "meson-clkc.h"
>> +
>> +struct clk_hw *meson_clk_hw_get(struct of_phandle_args *clkspec, void *clk_hw_data)
>> +{
>> +	const struct meson_clk_hw_data *data = clk_hw_data;
>> +	unsigned int idx = clkspec->args[0];
>> +
>> +	if (idx >= data->num) {
>> +		pr_err("%s: invalid index %u\n", __func__, idx);
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	return data->hws[idx];
>> +}
>> +EXPORT_SYMBOL_GPL(meson_clk_hw_get);
>> +
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/clk/meson/meson-clkc.h b/drivers/clk/meson/meson-clkc.h
>> new file mode 100644
>> index 000000000000..e3bad2aa17eb
>> --- /dev/null
>> +++ b/drivers/clk/meson/meson-clkc.h
>> @@ -0,0 +1,19 @@
>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>> +/*
>> + * Copyright (c) 2023 Neil Armstrong <neil.armstrong@linaro.org>
>> + */
>> +
>> +#ifndef __MESON_HW_CLKC_H__
>> +#define __MESON_HW_CLKC_H__
>> +
>> +#include <linux/of_device.h>
>> +#include <linux/clk-provider.h>
>> +
>> +struct meson_clk_hw_data {
>> +	struct clk_hw	**hws;
>> +	unsigned int	num;
>> +};
>> +
>> +struct clk_hw *meson_clk_hw_get(struct of_phandle_args *clkspec, void *clk_hw_data);
>> +
>> +#endif
>> ===================================><============================================================================
>>
>> If it's ok I'll send a v2 using this.
>>
>> Thanks,
>> Neil
>