| Message ID | 20181013005504.46399-2-briannorris@chromium.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1a1a0d5ccefca6f3f7417b448793c753a610da0c |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [1/4] ath10k: snoc: remove 'wcn3990' from generic resource handling | expand |
Hi, On Fri, Oct 12, 2018 at 5:55 PM Brian Norris <briannorris@chromium.org> wrote: > + if (vreg_info->settle_delay) > + udelay(vreg_info->settle_delay); Not new to your patch, but this seems like a duplication of what the regulator framework is doing for you. There are plenty of regulator properties describing lots of different types delays and that would be the place to put it. Doing so makes it automatically easy for boards to specify a different delay if they have different ramp characteristics (like someone put a bigger capacitor on the line or somesuch). At the moment it would be easy for someone to submit a patch to kill the settle delay in this driver this since the entire vreg_cfg table has 0 for the settle delay. > +static int __ath10k_snoc_vreg_off(struct ath10k *ar, > + struct ath10k_vreg_info *vreg_info) > +{ > + int ret; > + > + ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc regulator %s being disabled\n", > + vreg_info->name); > + > + ret = regulator_disable(vreg_info->reg); > + if (ret) > + ath10k_err(ar, "failed to disable regulator %s\n", > + vreg_info->name); > + > + ret = regulator_set_load(vreg_info->reg, 0); > + if (ret < 0) > + ath10k_err(ar, "failed to set load %s\n", vreg_info->name); > + > + ret = regulator_set_voltage(vreg_info->reg, 0, vreg_info->max_v); > + if (ret) > + ath10k_err(ar, "failed to set voltage %s\n", vreg_info->name); Not new to your patch, but ick, forcing someone to manually set the load / voltage of a regulator that they've turned off is silly. It's only list to try to send a patch to remedy this situation. Let me try to put that higher on my plate. ...just like with the clock patch I suspect that some of this is duplicating the "regulator_bulk" APIs. ...though I guess maybe we can't use those too easily until we can avoid setting voltage and current so much... In any case, your patch overall looks good and an improvement. Reviewed-by: Douglas Anderson <dianders@chromium.org>
Hi, On Thu, Oct 18, 2018 at 10:54:39AM -0700, Doug Anderson wrote: > On Fri, Oct 12, 2018 at 5:55 PM Brian Norris <briannorris@chromium.org> wrote: > > + if (vreg_info->settle_delay) > > + udelay(vreg_info->settle_delay); > > Not new to your patch, but this seems like a duplication of what the > regulator framework is doing for you. There are plenty of regulator > properties describing lots of different types delays and that would be > the place to put it. Doing so makes it automatically easy for boards > to specify a different delay if they have different ramp > characteristics (like someone put a bigger capacitor on the line or > somesuch). > > At the moment it would be easy for someone to submit a patch to kill > the settle delay in this driver this since the entire vreg_cfg table > has 0 for the settle delay. Thanks! I'll probably take a stab at killing the excessive specifications in these tables, in a later patch. Would be nice to get the bugfixes landed first though. > > +static int __ath10k_snoc_vreg_off(struct ath10k *ar, > > + struct ath10k_vreg_info *vreg_info) > > +{ > > + int ret; > > + > > + ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc regulator %s being disabled\n", > > + vreg_info->name); > > + > > + ret = regulator_disable(vreg_info->reg); > > + if (ret) > > + ath10k_err(ar, "failed to disable regulator %s\n", > > + vreg_info->name); > > + > > + ret = regulator_set_load(vreg_info->reg, 0); > > + if (ret < 0) > > + ath10k_err(ar, "failed to set load %s\n", vreg_info->name); > > + > > + ret = regulator_set_voltage(vreg_info->reg, 0, vreg_info->max_v); > > + if (ret) > > + ath10k_err(ar, "failed to set voltage %s\n", vreg_info->name); > > Not new to your patch, but ick, forcing someone to manually set the > load / voltage of a regulator that they've turned off is silly. It's > only list to try to send a patch to remedy this situation. Let me try I'm guessing you meant: s/only/on my/ > to put that higher on my plate. > > > ...just like with the clock patch I suspect that some of this is > duplicating the "regulator_bulk" APIs. ...though I guess maybe we > can't use those too easily until we can avoid setting voltage and > current so much... In any case, your patch overall looks good and an > improvement. I'll admit I've never used the bulk APIs. But I might give them a try as a follow-up cleanup. (As before: would be nice to have the simpler bugfix first.) > Reviewed-by: Douglas Anderson <dianders@chromium.org> Thanks, Brian
diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c index c6254db17dab..b63ae8b006b4 100644 --- a/drivers/net/wireless/ath/ath10k/snoc.c +++ b/drivers/net/wireless/ath/ath10k/snoc.c @@ -1311,6 +1311,76 @@ static int ath10k_get_clk_info(struct ath10k *ar, struct device *dev, return ret; } +static int __ath10k_snoc_vreg_on(struct ath10k *ar, + struct ath10k_vreg_info *vreg_info) +{ + int ret; + + ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc regulator %s being enabled\n", + vreg_info->name); + + ret = regulator_set_voltage(vreg_info->reg, vreg_info->min_v, + vreg_info->max_v); + if (ret) { + ath10k_err(ar, + "failed to set regulator %s voltage-min: %d voltage-max: %d\n", + vreg_info->name, vreg_info->min_v, vreg_info->max_v); + return ret; + } + + if (vreg_info->load_ua) { + ret = regulator_set_load(vreg_info->reg, vreg_info->load_ua); + if (ret < 0) { + ath10k_err(ar, "failed to set regulator %s load: %d\n", + vreg_info->name, vreg_info->load_ua); + goto err_set_load; + } + } + + ret = regulator_enable(vreg_info->reg); + if (ret) { + ath10k_err(ar, "failed to enable regulator %s\n", + vreg_info->name); + goto err_enable; + } + + if (vreg_info->settle_delay) + udelay(vreg_info->settle_delay); + + return 0; + +err_enable: + regulator_set_load(vreg_info->reg, 0); +err_set_load: + regulator_set_voltage(vreg_info->reg, 0, vreg_info->max_v); + + return ret; +} + +static int __ath10k_snoc_vreg_off(struct ath10k *ar, + struct ath10k_vreg_info *vreg_info) +{ + int ret; + + ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc regulator %s being disabled\n", + vreg_info->name); + + ret = regulator_disable(vreg_info->reg); + if (ret) + ath10k_err(ar, "failed to disable regulator %s\n", + vreg_info->name); + + ret = regulator_set_load(vreg_info->reg, 0); + if (ret < 0) + ath10k_err(ar, "failed to set load %s\n", vreg_info->name); + + ret = regulator_set_voltage(vreg_info->reg, 0, vreg_info->max_v); + if (ret) + ath10k_err(ar, "failed to set voltage %s\n", vreg_info->name); + + return ret; +} + static int ath10k_snoc_vreg_on(struct ath10k *ar) { struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar); @@ -1324,53 +1394,21 @@ static int ath10k_snoc_vreg_on(struct ath10k *ar) if (!vreg_info->reg) continue; - ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc regulator %s being enabled\n", - vreg_info->name); - - ret = regulator_set_voltage(vreg_info->reg, vreg_info->min_v, - vreg_info->max_v); - if (ret) { - ath10k_err(ar, - "failed to set regulator %s voltage-min: %d voltage-max: %d\n", - vreg_info->name, vreg_info->min_v, vreg_info->max_v); - goto err_reg_config; - } - - if (vreg_info->load_ua) { - ret = regulator_set_load(vreg_info->reg, - vreg_info->load_ua); - if (ret < 0) { - ath10k_err(ar, - "failed to set regulator %s load: %d\n", - vreg_info->name, - vreg_info->load_ua); - goto err_reg_config; - } - } - - ret = regulator_enable(vreg_info->reg); - if (ret) { - ath10k_err(ar, "failed to enable regulator %s\n", - vreg_info->name); + ret = __ath10k_snoc_vreg_on(ar, vreg_info); + if (ret) goto err_reg_config; - } - - if (vreg_info->settle_delay) - udelay(vreg_info->settle_delay); } return 0; err_reg_config: - for (; i >= 0; i--) { + for (i = i - 1; i >= 0; i--) { vreg_info = &ar_snoc->vreg[i]; if (!vreg_info->reg) continue; - regulator_disable(vreg_info->reg); - regulator_set_load(vreg_info->reg, 0); - regulator_set_voltage(vreg_info->reg, 0, vreg_info->max_v); + __ath10k_snoc_vreg_off(ar, vreg_info); } return ret; @@ -1389,24 +1427,7 @@ static int ath10k_snoc_vreg_off(struct ath10k *ar) if (!vreg_info->reg) continue; - ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc regulator %s being disabled\n", - vreg_info->name); - - ret = regulator_disable(vreg_info->reg); - if (ret) - ath10k_err(ar, "failed to disable regulator %s\n", - vreg_info->name); - - ret = regulator_set_load(vreg_info->reg, 0); - if (ret < 0) - ath10k_err(ar, "failed to set load %s\n", - vreg_info->name); - - ret = regulator_set_voltage(vreg_info->reg, 0, - vreg_info->max_v); - if (ret) - ath10k_err(ar, "failed to set voltage %s\n", - vreg_info->name); + ret = __ath10k_snoc_vreg_off(ar, vreg_info); } return ret;
If a regulator fails to set its voltage, we end up with an unbalanced call to regulator_disable(), because the error path starts with the current regulator (which was never enabled). Factor out the "on" function to perform (and unwind if failed) a single regulator at a time, and then main loop (ath10k_snoc_vreg_on()) can just worry about unwinding the regulators that were already enabled. It also helps to factor out the "off" function, to avoid repeating some code here. Signed-off-by: Brian Norris <briannorris@chromium.org> --- drivers/net/wireless/ath/ath10k/snoc.c | 129 ++++++++++++++----------- 1 file changed, 75 insertions(+), 54 deletions(-)