| Message ID | 1453105525-31506-3-git-send-email-andi.shyti@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18.01.2016 17:25, Andi Shyti 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, s/structure/structures/ > while the others lie unused in the memory. > > The clock's name, though, is not such a meaningful information > 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. > > Signed-off-by: Andi Shyti <andi.shyti@samsung.com> > Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.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 */ s/ss2mps11_clks_init/s2mps11_clks_init/ > 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) > { The check if (!clks_init[i].name) in this function is meaningless now. How will device with S2MPS14 behave? Best regards, Krzysztof > @@ -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; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@samsung.com > <mailto:andi.shyti@samsung.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@samsung.com > <mailto:andi.shyti@samsung.com>> > Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com > <mailto:k.kozlowski@samsung.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@vger.kernel.org > <mailto:majordomo@vger.kernel.org> > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> >> 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. > Can we guaranty about future chips? Even we didn't expect there will be one less clock in case of s2mps14. > This really should be simplified. > IMO, it is just reduction of LoC. Regards, Yadwinder -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Yadwinder, > 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. mmmhhh... allocating in boot time as much as we want and then copy what we need? It doesn't look that pretty to me. > The clock's name, though, is not such a meaningful information > > > I think it can be meaningful in debugging. Can you explain what's the use of the naming other than debugging? > 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. In that case, when the new chip will come, we would need to figure out something, but for sure I don't see it as a good idea to leave allocated unused structures. Thanks, Andi -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 18, 2016 at 6:43 PM, Andi Shyti <andi.shyti@samsung.com> wrote: > Hi Yadwinder, > >> 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. > > mmmhhh... allocating in boot time as much as we want and then copy > what we need? It doesn't look that pretty to me. > I think its not a new practice and I don't see any issue with it. >> The clock's name, though, is not such a meaningful information >> >> >> I think it can be meaningful in debugging. > > Can you explain what's the use of the naming other than > debugging? > Isn't debugging important enough ? :) I had misunderstood your below statement. Looking at code, it seems its still using different names for different clocks. >> 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. > > In that case, when the new chip will come, we would need to > figure out something, Different structures were introduced to handle such cases and keep driver simple and clean by keeping keep no. of if() checks as limited as possible. > but for sure I don't see it as a good idea > to leave allocated unused structures. > Even a single unused structure isn't a good idea, in case where this driver doesn't get probed. :) Regards, Yadwinder > Thanks, > Andi > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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. The clock's name, though, is not such a meaningful information 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. Signed-off-by: Andi Shyti <andi.shyti@samsung.com> Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> --- drivers/clk/clk-s2mps11.c | 48 +++++++---------------------------------------- 1 file changed, 7 insertions(+), 41 deletions(-)