From: Doug Anderson <dianders@chromium.org> To: Brian Norris <briannorris@chromium.org> Cc: kvalo@qca.qualcomm.com, ath10k@lists.infradead.org, linux-wireless@vger.kernel.org, Govind Singh <govinds@codeaurora.org>, LKML <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 2/4] ath10k: snoc: fix unabalanced regulator error handling Date: Thu, 18 Oct 2018 10:54:39 -0700 [thread overview] Message-ID: <CAD=FV=U0uK25mL9_d1nVB7uBV+vR9YTqGm1iTQgzdNuZO5O5_A@mail.gmail.com> (raw) In-Reply-To: <20181013005504.46399-2-briannorris@chromium.org> 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>
WARNING: multiple messages have this Message-ID (diff)
From: Doug Anderson <dianders@chromium.org> To: Brian Norris <briannorris@chromium.org> Cc: kvalo@qca.qualcomm.com, Govind Singh <govinds@codeaurora.org>, linux-wireless@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>, ath10k@lists.infradead.org Subject: Re: [PATCH 2/4] ath10k: snoc: fix unabalanced regulator error handling Date: Thu, 18 Oct 2018 10:54:39 -0700 [thread overview] Message-ID: <CAD=FV=U0uK25mL9_d1nVB7uBV+vR9YTqGm1iTQgzdNuZO5O5_A@mail.gmail.com> (raw) In-Reply-To: <20181013005504.46399-2-briannorris@chromium.org> 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> _______________________________________________ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
next prev parent reply other threads:[~2018-10-18 17:54 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-10-13 0:55 [PATCH 1/4] ath10k: snoc: remove 'wcn3990' from generic resource handling Brian Norris 2018-10-13 0:55 ` Brian Norris 2018-10-13 0:55 ` [PATCH 2/4] ath10k: snoc: fix unabalanced regulator error handling Brian Norris 2018-10-13 0:55 ` Brian Norris 2018-10-18 17:54 ` Doug Anderson [this message] 2018-10-18 17:54 ` Doug Anderson 2018-10-24 22:10 ` Brian Norris 2018-10-24 22:10 ` Brian Norris 2018-10-13 0:55 ` [PATCH 3/4] ath10k: snoc: relax voltage requirements Brian Norris 2018-10-13 0:55 ` Brian Norris 2018-10-18 17:56 ` Doug Anderson 2018-10-18 17:56 ` Doug Anderson 2018-10-18 18:14 ` Brian Norris 2018-10-18 18:14 ` Brian Norris 2018-10-13 0:55 ` [PATCH 4/4] ath10k: snoc: fix unbalanced clock error handling Brian Norris 2018-10-13 0:55 ` Brian Norris 2018-10-16 23:53 ` Doug Anderson 2018-10-16 23:53 ` Doug Anderson 2018-11-06 16:14 ` Kalle Valo 2018-11-06 16:14 ` Kalle Valo 2018-10-16 23:43 ` [PATCH 1/4] ath10k: snoc: remove 'wcn3990' from generic resource handling Doug Anderson 2018-10-16 23:43 ` Doug Anderson 2018-10-16 23:47 ` Brian Norris 2018-10-16 23:47 ` Brian Norris 2018-11-05 13:04 ` Kalle Valo 2018-11-05 13:04 ` Kalle Valo [not found] ` <20181105130403.93B6560600@smtp.codeaurora.org> 2018-11-05 21:17 ` Brian Norris 2018-11-05 21:17 ` Brian Norris 2018-11-06 16:18 ` Kalle Valo 2018-11-06 16:18 ` Kalle Valo
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAD=FV=U0uK25mL9_d1nVB7uBV+vR9YTqGm1iTQgzdNuZO5O5_A@mail.gmail.com' \ --to=dianders@chromium.org \ --cc=ath10k@lists.infradead.org \ --cc=briannorris@chromium.org \ --cc=govinds@codeaurora.org \ --cc=kvalo@qca.qualcomm.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-wireless@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.