diff for duplicates of <1541157.TohRuBEVWh@diego> diff --git a/a/1.txt b/N1/1.txt index a6363c8..87c7247 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,100 +1,80 @@ Am Montag, 28. September 2015, 11:46:40 schrieb Stephen Boyd: -> On 09/23, Heiko St=FCbner wrote: +> On 09/23, Heiko Stübner wrote: > > --- -> >=20 -> > While it may be nice to do the actual handling of the clock referen= -ces +> > +> > While it may be nice to do the actual handling of the clock references > > only in the calling code, in this current use case it would create > > a big additional overhead. -> >=20 -> > It looks like this so called synchronous reset on power-domain stat= -e- -> > changes, requiring device clocks to be turned on, is not that uncom= -mon +> > +> > It looks like this so called synchronous reset on power-domain state- +> > changes, requiring device clocks to be turned on, is not that uncommon > > or rockchip-specific. -> > For this Kevin requested that we read the clocks from the actual co= -nsumer +> > For this Kevin requested that we read the clocks from the actual consumer > > devices and not double-list them in the power-domain node as well. -> >=20 -> > So when expecting pm_clk_add_clk() to work, the current powerdomain= - code -> >=20 -> > can simply do when adding a device to a domain in=20 +> > +> > So when expecting pm_clk_add_clk() to work, the current powerdomain code +> > +> > can simply do when adding a device to a domain in rockchip_pd_attach_dev(): -> > while ((clk =3D of_clk_get(dev->of_node, i++)) && !IS_ERR(clk))= - { -> > =09 -> > =09dev_dbg(dev, "adding clock '%pC' to list of PM clocks\n", clk); -> > =09error =3D pm_clk_add_clk(dev, clk); -> > =09clk_put(clk); -> > =09 +> > while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) { +> > +> > dev_dbg(dev, "adding clock '%pC' to list of PM clocks\n", clk); +> > error = pm_clk_add_clk(dev, clk); +> > clk_put(clk); +> > > > } -> >=20 -> > The clock gets handed off to the generic pm clock handling and thus= - +> > +> > The clock gets handed off to the generic pm clock handling and thus > > clk_put in there. -> >=20 -> >=20 -> > On the other hand when only the rockchip power-domain code is expec= -ted -> > to get and put the clock, we would require a lot of new overhead, a= -s now +> > +> > +> > On the other hand when only the rockchip power-domain code is expected +> > to get and put the clock, we would require a lot of new overhead, as now > > the code would also need to track which devices got added to what -> > domain and also all clock-references until the device gets detached= - +> > domain and also all clock-references until the device gets detached > > again. So this would essentially duplicate a big part of what the -> > genpd-code does (per-domain device-list and per-device clock-list).= - -> >=20 +> > genpd-code does (per-domain device-list and per-device clock-list). +> > > > As this seems to be not uncommon, future powerdomain drivers -> > might need that too and would also need to duplicate that handling.= - -> >=20 +> > might need that too and would also need to duplicate that handling. +> > > > When allowing multiple __clk_get and __clk_put calls on the other -> > hand, the overhead for the regular case comes down to one atomic_in= -c, -> > atomic_sub_and_test and the function call to the new separate relea= -se +> > hand, the overhead for the regular case comes down to one atomic_inc, +> > atomic_sub_and_test and the function call to the new separate release > > function ;-) . ->=20 +> > Why are we doing of_clk_get(), pm_clk_add_clk(), and then > clk_put()? Just drop that clk_put() in the caller and remove the > __clk_get() inside pm_clk_add_clk() and everything works the > same. This patch does most of that, except it doesn't handle the > error path where we would need to throw a clk_put(). -I guess that was my try to keep all the gets and puts together, but als= -o an=20 -instance of not seeing the forest for the trees. Looking at your soluti= -on=20 +I guess that was my try to keep all the gets and puts together, but also an +instance of not seeing the forest for the trees. Looking at your solution below does look actually really cool. -Would still be nice if the genpd people would speak up on their prefere= -nce. In=20 +Would still be nice if the genpd people would speak up on their preference. In the worst case I'll try hunting them down at ELCE next week :-) -I guess for the error path, one could just set the notion that if=20 -pm_clk_add_clk returns sucessfully it has taken over the clock referenc= -e=20 +I guess for the error path, one could just set the notion that if +pm_clk_add_clk returns sucessfully it has taken over the clock reference completely and in the error case the caller should clean up? ->=20 +> > Really, that snippet of code that loops over a device's clocks > and adds them to a pm domain is an example of duplicate code that > should go into some common layer like the PM clock stuff. Then we > don't have any kind of situation where we're passing struct clk > pointers off to other layers of code and this "problem" doesn't > exist. ->=20 +> > ----8<---- -> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/cloc= -k_ops.c +> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c > index acef9f9f759a..529a03e8282c 100644 > --- a/drivers/base/power/clock_ops.c > +++ b/drivers/base/power/clock_ops.c -> @@ -95,7 +95,7 @@ static int __pm_clk_add(struct device *dev, const c= -har +> @@ -95,7 +95,7 @@ static int __pm_clk_add(struct device *dev, const char > *con_id, return -ENOMEM; > } > } else { @@ -103,16 +83,12 @@ har > kfree(ce); > return -ENOENT; > } -> @@ -129,7 +129,7 @@ int pm_clk_add(struct device *dev, const char *co= -n_id) +> @@ -129,7 +129,7 @@ int pm_clk_add(struct device *dev, const char *con_id) > * @clk: Clock pointer > * -> * Add the clock to the list of clocks used for the power management= - of -> @dev. - * It will increment refcount on clock pointer, use clk_put() = -on it -> when done. + * Callers should not call clk_put() on @clk after callin= -g this +> * Add the clock to the list of clocks used for the power management of +> @dev. - * It will increment refcount on clock pointer, use clk_put() on it +> when done. + * Callers should not call clk_put() on @clk after calling this > function. */ > int pm_clk_add_clk(struct device *dev, struct clk *clk) > { diff --git a/a/content_digest b/N1/content_digest index cafaec5..5273a30 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -15,102 +15,82 @@ "\00:1\0" "b\0" "Am Montag, 28. September 2015, 11:46:40 schrieb Stephen Boyd:\n" - "> On 09/23, Heiko St=FCbner wrote:\n" + "> On 09/23, Heiko St\303\274bner wrote:\n" "> > ---\n" - "> >=20\n" - "> > While it may be nice to do the actual handling of the clock referen=\n" - "ces\n" + "> > \n" + "> > While it may be nice to do the actual handling of the clock references\n" "> > only in the calling code, in this current use case it would create\n" "> > a big additional overhead.\n" - "> >=20\n" - "> > It looks like this so called synchronous reset on power-domain stat=\n" - "e-\n" - "> > changes, requiring device clocks to be turned on, is not that uncom=\n" - "mon\n" + "> > \n" + "> > It looks like this so called synchronous reset on power-domain state-\n" + "> > changes, requiring device clocks to be turned on, is not that uncommon\n" "> > or rockchip-specific.\n" - "> > For this Kevin requested that we read the clocks from the actual co=\n" - "nsumer\n" + "> > For this Kevin requested that we read the clocks from the actual consumer\n" "> > devices and not double-list them in the power-domain node as well.\n" - "> >=20\n" - "> > So when expecting pm_clk_add_clk() to work, the current powerdomain=\n" - " code\n" - "> >=20\n" - "> > can simply do when adding a device to a domain in=20\n" + "> > \n" + "> > So when expecting pm_clk_add_clk() to work, the current powerdomain code\n" + "> > \n" + "> > can simply do when adding a device to a domain in \n" "rockchip_pd_attach_dev():\n" - "> > while ((clk =3D of_clk_get(dev->of_node, i++)) && !IS_ERR(clk))=\n" - " {\n" - "> > =09\n" - "> > =09dev_dbg(dev, \"adding clock '%pC' to list of PM clocks\\n\", clk);\n" - "> > =09error =3D pm_clk_add_clk(dev, clk);\n" - "> > =09clk_put(clk);\n" - "> > =09\n" + "> > while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {\n" + "> > \t\n" + "> > \tdev_dbg(dev, \"adding clock '%pC' to list of PM clocks\\n\", clk);\n" + "> > \terror = pm_clk_add_clk(dev, clk);\n" + "> > \tclk_put(clk);\n" + "> > \t\n" "> > }\n" - "> >=20\n" - "> > The clock gets handed off to the generic pm clock handling and thus=\n" - "\n" + "> > \n" + "> > The clock gets handed off to the generic pm clock handling and thus\n" "> > clk_put in there.\n" - "> >=20\n" - "> >=20\n" - "> > On the other hand when only the rockchip power-domain code is expec=\n" - "ted\n" - "> > to get and put the clock, we would require a lot of new overhead, a=\n" - "s now\n" + "> > \n" + "> > \n" + "> > On the other hand when only the rockchip power-domain code is expected\n" + "> > to get and put the clock, we would require a lot of new overhead, as now\n" "> > the code would also need to track which devices got added to what\n" - "> > domain and also all clock-references until the device gets detached=\n" - "\n" + "> > domain and also all clock-references until the device gets detached\n" "> > again. So this would essentially duplicate a big part of what the\n" - "> > genpd-code does (per-domain device-list and per-device clock-list).=\n" - "\n" - "> >=20\n" + "> > genpd-code does (per-domain device-list and per-device clock-list).\n" + "> > \n" "> > As this seems to be not uncommon, future powerdomain drivers\n" - "> > might need that too and would also need to duplicate that handling.=\n" - "\n" - "> >=20\n" + "> > might need that too and would also need to duplicate that handling.\n" + "> > \n" "> > When allowing multiple __clk_get and __clk_put calls on the other\n" - "> > hand, the overhead for the regular case comes down to one atomic_in=\n" - "c,\n" - "> > atomic_sub_and_test and the function call to the new separate relea=\n" - "se\n" + "> > hand, the overhead for the regular case comes down to one atomic_inc,\n" + "> > atomic_sub_and_test and the function call to the new separate release\n" "> > function ;-) .\n" - ">=20\n" + "> \n" "> Why are we doing of_clk_get(), pm_clk_add_clk(), and then\n" "> clk_put()? Just drop that clk_put() in the caller and remove the\n" "> __clk_get() inside pm_clk_add_clk() and everything works the\n" "> same. This patch does most of that, except it doesn't handle the\n" "> error path where we would need to throw a clk_put().\n" "\n" - "I guess that was my try to keep all the gets and puts together, but als=\n" - "o an=20\n" - "instance of not seeing the forest for the trees. Looking at your soluti=\n" - "on=20\n" + "I guess that was my try to keep all the gets and puts together, but also an \n" + "instance of not seeing the forest for the trees. Looking at your solution \n" "below does look actually really cool.\n" "\n" - "Would still be nice if the genpd people would speak up on their prefere=\n" - "nce. In=20\n" + "Would still be nice if the genpd people would speak up on their preference. In \n" "the worst case I'll try hunting them down at ELCE next week :-)\n" "\n" - "I guess for the error path, one could just set the notion that if=20\n" - "pm_clk_add_clk returns sucessfully it has taken over the clock referenc=\n" - "e=20\n" + "I guess for the error path, one could just set the notion that if \n" + "pm_clk_add_clk returns sucessfully it has taken over the clock reference \n" "completely and in the error case the caller should clean up?\n" "\n" "\n" - ">=20\n" + "> \n" "> Really, that snippet of code that loops over a device's clocks\n" "> and adds them to a pm domain is an example of duplicate code that\n" "> should go into some common layer like the PM clock stuff. Then we\n" "> don't have any kind of situation where we're passing struct clk\n" "> pointers off to other layers of code and this \"problem\" doesn't\n" "> exist.\n" - ">=20\n" + "> \n" "> ----8<----\n" - "> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/cloc=\n" - "k_ops.c\n" + "> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c\n" "> index acef9f9f759a..529a03e8282c 100644\n" "> --- a/drivers/base/power/clock_ops.c\n" "> +++ b/drivers/base/power/clock_ops.c\n" - "> @@ -95,7 +95,7 @@ static int __pm_clk_add(struct device *dev, const c=\n" - "har\n" + "> @@ -95,7 +95,7 @@ static int __pm_clk_add(struct device *dev, const char\n" "> *con_id, return -ENOMEM;\n" "> }\n" "> } else {\n" @@ -119,18 +99,14 @@ "> kfree(ce);\n" "> return -ENOENT;\n" "> }\n" - "> @@ -129,7 +129,7 @@ int pm_clk_add(struct device *dev, const char *co=\n" - "n_id)\n" + "> @@ -129,7 +129,7 @@ int pm_clk_add(struct device *dev, const char *con_id)\n" "> * @clk: Clock pointer\n" "> *\n" - "> * Add the clock to the list of clocks used for the power management=\n" - " of\n" - "> @dev. - * It will increment refcount on clock pointer, use clk_put() =\n" - "on it\n" - "> when done. + * Callers should not call clk_put() on @clk after callin=\n" - "g this\n" + "> * Add the clock to the list of clocks used for the power management of\n" + "> @dev. - * It will increment refcount on clock pointer, use clk_put() on it\n" + "> when done. + * Callers should not call clk_put() on @clk after calling this\n" "> function. */\n" "> int pm_clk_add_clk(struct device *dev, struct clk *clk)\n" > { -6d8e48ef8e491cd888e1c7331505dda2fcf02707d17a92e798ba5db9cf7179f3 +22285b31de5a7947d7290097607b56f42fb5111852d273ee19b658c12fa356f7
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.