| 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
| ||
|
Message-id: <569D87E0.6020001@samsung.com>
Date: Tue, 19 Jan 2016 09:48:32 +0900
From: Krzysztof Kozlowski <k.kozlowski@...sung.com>
To: Yadwinder Singh Brar <yadi.brar01@...il.com>,
Andi Shyti <andi.shyti@...sung.com>
Cc: linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
Sangbeom Kim <sbkim73@...sung.com>,
Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...eaurora.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
linux-clk@...r.kernel.org, Andi Shyti <andi@...zian.org>
Subject: Re: [PATCH 2/2] clk: s2mps11: allocate only one structure for clock
init
On 19.01.2016 09:42, Yadwinder Singh Brar wrote:
> Hello Andi Shyti,
>
> On Mon, Jan 18, 2016 at 12:25 AM, Andi Shyti <andi.shyti@...sung.com
> <mailto:andi.shyti@...sung.com>> wrote:
>
> The driver allocates three structures for three different clock
> types. They are quite similar and in the clock init data they
> differ only by the name. Only one of these structure is used,
> while the others lie unused in the memory.
>
>
> If you are worried about memory, they can be made __initdata by
> creating a copy during probe.
Which would add small delay (need to allocate memory and copy the data)
during the boot.
>
>
>
> The clock's name, though, is not such a meaningful information
>
>
> I think it can be meaningful in debugging.
What kind of meaning? The name is still present... It is easy to find
the type of device (e.g. compatible).
>
>
> and by assigning the same name to the initial data we can avoid
> over allocation. The common name chosen will be s2mps11,
> coherently with the device driver name, instead of the clock
> device.
>
> Therefore, remove the structures associated to s2mps13 and
> s2mps14 and use only the one referred to s2mps11 for all kind of
> clocks.
>
>
> IMHO, with all these modifications, it will leave driver with some extra
> checks and reduced readability, perhaps will make it complex to add
> support for similar clocks but with different clk_ops, if next version or
> any similar mfd chip comes up in future.
There is no evidence that some future chip will require clk_ops. All of
them are so far exactly the same. Even the newly added S2MPS15 is the same.
This really should be simplified.
Best regards,
Krzysztof
>
>
> Regards,
> Yadwinder
>
>
>
> Signed-off-by: Andi Shyti <andi.shyti@...sung.com
> <mailto:andi.shyti@...sung.com>>
> Suggested-by: Krzysztof Kozlowski <k.kozlowski@...sung.com
> <mailto:k.kozlowski@...sung.com>>
> ---
> drivers/clk/clk-s2mps11.c | 48
> +++++++----------------------------------------
> 1 file changed, 7 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/clk/clk-s2mps11.c b/drivers/clk/clk-s2mps11.c
> index ccb12f1..dce9753 100644
> --- a/drivers/clk/clk-s2mps11.c
> +++ b/drivers/clk/clk-s2mps11.c
> @@ -99,6 +99,7 @@ static struct clk_ops s2mps11_clk_ops = {
> .recalc_rate = s2mps11_clk_recalc_rate,
> };
>
> +/* This ss2mps11_clks_init tructure is common to s2mps11, s2mps13
> and s2mps14 */
> static struct clk_init_data s2mps11_clks_init[S2MPS11_CLKS_NUM] = {
> [S2MPS11_CLK_AP] = {
> .name = "s2mps11_ap",
> @@ -117,37 +118,6 @@ static struct clk_init_data
> s2mps11_clks_init[S2MPS11_CLKS_NUM] = {
> },
> };
>
> -static struct clk_init_data s2mps13_clks_init[S2MPS11_CLKS_NUM] = {
> - [S2MPS11_CLK_AP] = {
> - .name = "s2mps13_ap",
> - .ops = &s2mps11_clk_ops,
> - .flags = CLK_IS_ROOT,
> - },
> - [S2MPS11_CLK_CP] = {
> - .name = "s2mps13_cp",
> - .ops = &s2mps11_clk_ops,
> - .flags = CLK_IS_ROOT,
> - },
> - [S2MPS11_CLK_BT] = {
> - .name = "s2mps13_bt",
> - .ops = &s2mps11_clk_ops,
> - .flags = CLK_IS_ROOT,
> - },
> -};
> -
> -static struct clk_init_data s2mps14_clks_init[S2MPS11_CLKS_NUM] = {
> - [S2MPS11_CLK_AP] = {
> - .name = "s2mps14_ap",
> - .ops = &s2mps11_clk_ops,
> - .flags = CLK_IS_ROOT,
> - },
> - [S2MPS11_CLK_BT] = {
> - .name = "s2mps14_bt",
> - .ops = &s2mps11_clk_ops,
> - .flags = CLK_IS_ROOT,
> - },
> -};
> -
> static struct device_node *s2mps11_clk_parse_dt(struct
> platform_device *pdev,
> struct clk_init_data *clks_init)
> {
> @@ -179,8 +149,8 @@ static int s2mps11_clk_probe(struct
> platform_device *pdev)
> struct sec_pmic_dev *iodev = dev_get_drvdata(pdev->dev.parent);
> struct s2mps11_clk *s2mps11_clks;
> unsigned int s2mps11_reg;
> - struct clk_init_data *clks_init;
> int i, ret = 0;
> + enum sec_device_type hwid =
> platform_get_device_id(pdev)->driver_data;
>
> s2mps11_clks = devm_kcalloc(&pdev->dev, S2MPS11_CLKS_NUM,
> sizeof(*s2mps11_clks), GFP_KERNEL);
> @@ -192,22 +162,18 @@ static int s2mps11_clk_probe(struct
> platform_device *pdev)
> if (!clk_table)
> return -ENOMEM;
>
> - switch(platform_get_device_id(pdev)->driver_data) {
> + switch (hwid) {
> case S2MPS11X:
> s2mps11_reg = S2MPS11_REG_RTC_CTRL;
> - clks_init = s2mps11_clks_init;
> break;
> case S2MPS13X:
> s2mps11_reg = S2MPS13_REG_RTCCTRL;
> - clks_init = s2mps13_clks_init;
> break;
> case S2MPS14X:
> s2mps11_reg = S2MPS14_REG_RTCCTRL;
> - clks_init = s2mps14_clks_init;
> break;
> case S5M8767X:
> s2mps11_reg = S5M8767_REG_CTRL1;
> - clks_init = s2mps11_clks_init;
> break;
> default:
> dev_err(&pdev->dev, "Invalid device type\n");
> @@ -215,15 +181,15 @@ static int s2mps11_clk_probe(struct
> platform_device *pdev)
> }
>
> /* Store clocks of_node in first element of s2mps11_clks
> array */
> - s2mps11_clks->clk_np = s2mps11_clk_parse_dt(pdev, clks_init);
> + s2mps11_clks->clk_np = s2mps11_clk_parse_dt(pdev,
> s2mps11_clks_init);
> if (IS_ERR(s2mps11_clks->clk_np))
> return PTR_ERR(s2mps11_clks->clk_np);
>
> for (i = 0; i < S2MPS11_CLKS_NUM; i++) {
> - if (!clks_init[i].name)
> + if (i == S2MPS11_CLK_CP && hwid == S2MPS14X)
> continue; /* Skip clocks not present in some
> devices */
> s2mps11_clks[i].iodev = iodev;
> - s2mps11_clks[i].hw.init = &clks_init[i];
> + s2mps11_clks[i].hw.init = &s2mps11_clks_init[i];
> s2mps11_clks[i].mask = 1 << i;
> s2mps11_clks[i].reg = s2mps11_reg;
>
> @@ -246,7 +212,7 @@ static int s2mps11_clk_probe(struct
> platform_device *pdev)
>
> for (i = 0; i < S2MPS11_CLKS_NUM; i++) {
> /* Skip clocks not present on S2MPS14 */
> - if (!clks_init[i].name)
> + if (i == S2MPS11_CLK_CP && hwid == S2MPS14X)
> continue;
> clk_table[i] = s2mps11_clks[i].clk;
> }
> --
> 2.6.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-samsung-soc" in
> the body of a message to majordomo@...r.kernel.org
> <mailto:majordomo@...r.kernel.org>
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
Powered by blists - more mailing lists