lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 02 Dec 2022 12:36:50 +0100
From:   Jerome Brunet <jbrunet@...libre.com>
To:     Dmitry Rokosov <ddrokosov@...rdevices.ru>,
        neil.armstrong@...aro.org, mturquette@...libre.com,
        sboyd@...nel.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, khilman@...libre.com,
        martin.blumenstingl@...glemail.com
Cc:     jian.hu@...ogic.com, kernel@...rdevices.ru, rockosov@...il.com,
        linux-amlogic@...ts.infradead.org, linux-clk@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v8 06/11] clk: meson: introduce a1-clkc common driver
 for all A1 clock controllers


On Fri 02 Dec 2022 at 01:56, Dmitry Rokosov <ddrokosov@...rdevices.ru> wrote:

> Generally, A1 SoC has four clock controllers on the board: PLL,
> Peripherals, CPU, and Audio. The audio clock controller is different
> from others, but the rest are very similar from a functional and regmap
> point of view. So a it's good idea to generalize some routines for all
> of them. Exactly, meson-a1-clkc driver contains the common probe() flow.
>
> Signed-off-by: Dmitry Rokosov <ddrokosov@...rdevices.ru>

I think you should leave this out for the initial submission. It makes
harder to review.

In some case, these factorizations actually the maitenance harder.

There is also the s4 that is coming up. It looks similar to the A1 as
well.

Let's wait for both them to land, see how it goes and then we'll decide
if a factorization is appropriate.

> ---
>  drivers/clk/meson/Kconfig         |  4 ++
>  drivers/clk/meson/Makefile        |  1 +
>  drivers/clk/meson/meson-a1-clkc.c | 63 +++++++++++++++++++++++++++++++
>  drivers/clk/meson/meson-a1-clkc.h | 25 ++++++++++++
>  4 files changed, 93 insertions(+)
>  create mode 100644 drivers/clk/meson/meson-a1-clkc.c
>  create mode 100644 drivers/clk/meson/meson-a1-clkc.h
>
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index bd44ba47200e..1c885541c3a9 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -43,6 +43,10 @@ config COMMON_CLK_MESON_CPU_DYNDIV
>  	tristate
>  	select COMMON_CLK_MESON_REGMAP
>  
> +config COMMON_CLK_MESON_A1_CLKC
> +	tristate
> +	select COMMON_CLK_MESON_REGMAP
> +
>  config COMMON_CLK_MESON8B
>  	bool "Meson8 SoC Clock controller support"
>  	depends on ARM
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index 0e6f293c05d4..15136d861a65 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_PLL) += clk-pll.o
>  obj-$(CONFIG_COMMON_CLK_MESON_REGMAP) += clk-regmap.o
>  obj-$(CONFIG_COMMON_CLK_MESON_SCLK_DIV) += sclk-div.o
>  obj-$(CONFIG_COMMON_CLK_MESON_VID_PLL_DIV) += vid-pll-div.o
> +obj-$(CONFIG_COMMON_CLK_MESON_A1_CLKC) += meson-a1-clkc.o
>  
>  # Amlogic Clock controllers
>  
> diff --git a/drivers/clk/meson/meson-a1-clkc.c b/drivers/clk/meson/meson-a1-clkc.c
> new file mode 100644
> index 000000000000..2fe320a0e16e
> --- /dev/null
> +++ b/drivers/clk/meson/meson-a1-clkc.c
> @@ -0,0 +1,63 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Amlogic Meson-A1 Clock Controller Driver
> + *
> + * Copyright (c) 2022, SberDevices. All Rights Reserved.
> + * Author: Dmitry Rokosov <ddrokosov@...rdevices.ru>
> + */
> +
> +#include <linux/of_device.h>
> +#include "meson-a1-clkc.h"
> +
> +static struct regmap_config clkc_regmap_config = {
> +	.reg_bits   = 32,
> +	.val_bits   = 32,
> +	.reg_stride = 4,
> +};
> +
> +int meson_a1_clkc_probe(struct platform_device *pdev)
> +{
> +	struct meson_a1_clkc_data *clkc;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	void __iomem *base;
> +	struct regmap *map;
> +	int clkid, i, err;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return dev_err_probe(dev, -ENXIO, "can't get IO resource\n");
> +
> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return dev_err_probe(dev, PTR_ERR(base),
> +				     "can't ioremap resource %pr\n", res);
> +
> +	map = devm_regmap_init_mmio(dev, base, &clkc_regmap_config);
> +	if (IS_ERR(map))
> +		return dev_err_probe(dev, PTR_ERR(map),
> +				     "can't init regmap mmio region\n");
> +
> +	clkc = (struct meson_a1_clkc_data *)of_device_get_match_data(dev);
> +	if (!clkc)
> +		return dev_err_probe(dev, -ENODEV,
> +				     "can't get A1 clkc driver data\n");
> +
> +	/* Populate regmap for the regmap backed clocks */
> +	for (i = 0; i < clkc->num_regs; i++)
> +		clkc->regs[i]->map = map;
> +
> +	for (clkid = 0; clkid < clkc->hw->num; clkid++) {
> +		err = devm_clk_hw_register(dev, clkc->hw->hws[clkid]);
> +		if (err)
> +			return dev_err_probe(dev, err,
> +					     "clock registration failed\n");
> +	}
> +
> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> +					   (void *)clkc->hw);
> +}
> +EXPORT_SYMBOL_GPL(meson_a1_clkc_probe);
> +
> +MODULE_AUTHOR("Dmitry Rokosov <ddrokosov@...rdevices.ru>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/clk/meson/meson-a1-clkc.h b/drivers/clk/meson/meson-a1-clkc.h
> new file mode 100644
> index 000000000000..503eca0f6cb5
> --- /dev/null
> +++ b/drivers/clk/meson/meson-a1-clkc.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> +/*
> + * Amlogic Meson-A1 Clock Controller driver
> + *
> + * Copyright (c) 2022, SberDevices. All Rights Reserved.
> + * Author: Dmitry Rokosov <ddrokosov@...rdevices.ru>
> + */
> +
> +#ifndef __MESON_A1_CLKC_H__
> +#define __MESON_A1_CLKC_H__
> +
> +#include <linux/clk-provider.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include "clk-regmap.h"
> +
> +struct meson_a1_clkc_data {
> +	const struct clk_hw_onecell_data *hw;
> +	struct clk_regmap *const *regs;
> +	size_t num_regs;
> +};
> +
> +int meson_a1_clkc_probe(struct platform_device *pdev);
> +#endif

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ