| Message ID | 20181204052119.806-5-rnayak@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Add power domain driver for corners on msm8996/sdm845 | expand |
Quoting Rajendra Nayak (2018-12-03 21:21:15) > @@ -221,6 +224,47 @@ static int rpmpd_power_off(struct generic_pm_domain *domain) > return ret; > } > > +static int rpmpd_set_performance(struct generic_pm_domain *domain, > + unsigned int state) > +{ > + int ret = 0; > + struct rpmpd *pd = domain_to_rpmpd(domain); > + > + mutex_lock(&rpmpd_lock); > + > + if (state > MAX_RPMPD_STATE) > + goto out; Does this need to be under the mutex lock? Doesn't look like it. > + > + pd->corner = state; > + > + if (!pd->enabled && (pd->key != KEY_FLOOR_CORNER)) Please drop useless parenthesis. > + goto out; > + > + ret = rpmpd_aggregate_corner(pd); > + > +out: > + mutex_unlock(&rpmpd_lock); > + > + return ret; > +} > + > +static unsigned int rpmpd_get_performance(struct generic_pm_domain *genpd, > + struct dev_pm_opp *opp) > +{ > + struct device_node *np; > + unsigned int corner = 0; > + > + np = dev_pm_opp_get_of_node(opp); > + if (of_property_read_u32(np, "qcom,level", &corner)) { > + pr_err("%s: missing 'qcom,level' property\n", __func__); We leak np reference here, assuming dev_pm_opp_get_of_node() returns an of_node_get() pointer to the caller. > + return 0; > + } > + > + of_node_put(np); This same code exists twice. Perhaps a helper needs to exist for qcom_rpm_get_performance() to pull the number out of the DT. > + > + return corner; > +}
On 12/5/2018 4:44 AM, Stephen Boyd wrote: > Quoting Rajendra Nayak (2018-12-03 21:21:15) >> @@ -221,6 +224,47 @@ static int rpmpd_power_off(struct generic_pm_domain *domain) >> return ret; >> } >> >> +static int rpmpd_set_performance(struct generic_pm_domain *domain, >> + unsigned int state) >> +{ >> + int ret = 0; >> + struct rpmpd *pd = domain_to_rpmpd(domain); >> + >> + mutex_lock(&rpmpd_lock); >> + >> + if (state > MAX_RPMPD_STATE) >> + goto out; > > Does this need to be under the mutex lock? Doesn't look like it. Nope, will move it out. > >> + >> + pd->corner = state; >> + >> + if (!pd->enabled && (pd->key != KEY_FLOOR_CORNER)) > > Please drop useless parenthesis. sure > >> + goto out; >> + >> + ret = rpmpd_aggregate_corner(pd); >> + >> +out: >> + mutex_unlock(&rpmpd_lock); >> + >> + return ret; >> +} >> + >> +static unsigned int rpmpd_get_performance(struct generic_pm_domain *genpd, >> + struct dev_pm_opp *opp) >> +{ >> + struct device_node *np; >> + unsigned int corner = 0; >> + >> + np = dev_pm_opp_get_of_node(opp); >> + if (of_property_read_u32(np, "qcom,level", &corner)) { >> + pr_err("%s: missing 'qcom,level' property\n", __func__); > > We leak np reference here, assuming dev_pm_opp_get_of_node() returns an > of_node_get() pointer to the caller. good catch, will fix. > >> + return 0; >> + } >> + >> + of_node_put(np); > > This same code exists twice. Perhaps a helper needs to exist for > qcom_rpm_get_performance() to pull the number out of the DT. Sure I can make both drivers use a common helper instead of duplicating it. > >> + >> + return corner; >> +}
On 12/5/2018 12:33 PM, Rajendra Nayak wrote: >> >>> + return 0; >>> + } >>> + >>> + of_node_put(np); >> >> This same code exists twice. Perhaps a helper needs to exist for >> qcom_rpm_get_performance() to pull the number out of the DT. > > Sure I can make both drivers use a common helper instead of duplicating it. which would mean I will need to create a new file just to define the common helper. Does that seem like an overkill?
Quoting Rajendra Nayak (2018-12-05 02:11:22) > > On 12/5/2018 12:33 PM, Rajendra Nayak wrote: > >> > >>> + return 0; > >>> + } > >>> + > >>> + of_node_put(np); > >> > >> This same code exists twice. Perhaps a helper needs to exist for > >> qcom_rpm_get_performance() to pull the number out of the DT. > > > > Sure I can make both drivers use a common helper instead of duplicating it. > > which would mean I will need to create a new file just to define the > common helper. Does that seem like an overkill? Maybe put it in the genpd code and let it take a const char *name argument that picks out the property that drivers want to look at? That way other OPP properties can be picked out with a simple call to the function but it's generic enough to be used in other places.
diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c index a0b9f122d793..eb1cfa6a03d6 100644 --- a/drivers/soc/qcom/rpmpd.c +++ b/drivers/soc/qcom/rpmpd.c @@ -12,6 +12,7 @@ #include <linux/of.h> #include <linux/of_device.h> #include <linux/platform_device.h> +#include <linux/pm_opp.h> #include <linux/soc/qcom/smd-rpm.h> #include <dt-bindings/mfd/qcom-rpm.h> @@ -28,6 +29,8 @@ #define KEY_ENABLE 0x6e657773 /* swen */ #define KEY_FLOOR_CORNER 0x636676 /* vfc */ +#define MAX_RPMPD_STATE 6 + #define DEFINE_RPMPD_CORN_SMPA(_platform, _name, _active, r_id) \ static struct rpmpd _platform##_##_active; \ static struct rpmpd _platform##_##_name = { \ @@ -221,6 +224,47 @@ static int rpmpd_power_off(struct generic_pm_domain *domain) return ret; } +static int rpmpd_set_performance(struct generic_pm_domain *domain, + unsigned int state) +{ + int ret = 0; + struct rpmpd *pd = domain_to_rpmpd(domain); + + mutex_lock(&rpmpd_lock); + + if (state > MAX_RPMPD_STATE) + goto out; + + pd->corner = state; + + if (!pd->enabled && (pd->key != KEY_FLOOR_CORNER)) + goto out; + + ret = rpmpd_aggregate_corner(pd); + +out: + mutex_unlock(&rpmpd_lock); + + return ret; +} + +static unsigned int rpmpd_get_performance(struct generic_pm_domain *genpd, + struct dev_pm_opp *opp) +{ + struct device_node *np; + unsigned int corner = 0; + + np = dev_pm_opp_get_of_node(opp); + if (of_property_read_u32(np, "qcom,level", &corner)) { + pr_err("%s: missing 'qcom,level' property\n", __func__); + return 0; + } + + of_node_put(np); + + return corner; +} + static int rpmpd_probe(struct platform_device *pdev) { int i; @@ -261,6 +305,8 @@ static int rpmpd_probe(struct platform_device *pdev) rpmpds[i]->rpm = rpm; rpmpds[i]->pd.power_off = rpmpd_power_off; rpmpds[i]->pd.power_on = rpmpd_power_on; + rpmpds[i]->pd.set_performance_state = rpmpd_set_performance; + rpmpds[i]->pd.opp_to_performance_state = rpmpd_get_performance; pm_genpd_init(&rpmpds[i]->pd, NULL, true); data->domains[i] = &rpmpds[i]->pd;