| Message ID | 20181013005504.46399-3-briannorris@chromium.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | bfe57a6ac75aec73aec43ca24942d7704100fc2c |
| 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: > > I rarely see drivers specify precise voltage requirements like this, but > if we really have to...let's at least give a little wiggle room. Board > designs (and accompanying device trees) may not provide exactly the > voltage listed here, and we shouldn't fail to probe just because of > this. > > Round these ranges down to the nearest volt, and provide a 0.05V margin. > The regulator should provide its own supported ranges, which will > helpfully intersect with these ranges. > > I would just as well remove these ranges entirely, but if I understand > correctly, there's some reason that QCOM SoC's like to set zero / > non-zero voltages. Yeah, I'll try to up-prioritize working on making that better (assuming others like my ideas in that area). > Signed-off-by: Brian Norris <briannorris@chromium.org> > --- > drivers/net/wireless/ath/ath10k/snoc.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c > index b63ae8b006b4..5a8e87339df2 100644 > --- a/drivers/net/wireless/ath/ath10k/snoc.c > +++ b/drivers/net/wireless/ath/ath10k/snoc.c > @@ -47,10 +47,10 @@ static char *const ce_name[] = { > }; > > static struct ath10k_vreg_info vreg_cfg[] = { > - {NULL, "vdd-0.8-cx-mx", 800000, 800000, 0, 0, false}, > - {NULL, "vdd-1.8-xo", 1800000, 1800000, 0, 0, false}, > - {NULL, "vdd-1.3-rfa", 1304000, 1304000, 0, 0, false}, > - {NULL, "vdd-3.3-ch0", 3312000, 3312000, 0, 0, false}, > + {NULL, "vdd-0.8-cx-mx", 800000, 850000, 0, 0, false}, > + {NULL, "vdd-1.8-xo", 1800000, 1850000, 0, 0, false}, > + {NULL, "vdd-1.3-rfa", 1300000, 1350000, 0, 0, false}, > + {NULL, "vdd-3.3-ch0", 3300000, 3350000, 0, 0, false}, These look fine to me. I find it really funny that this array has all those load values and they're all 0, but that's not new to your patch. Reviewed-by: Douglas Anderson <dianders@chromium.org>
On Thu, Oct 18, 2018 at 10:56 AM Doug Anderson <dianders@chromium.org> wrote: > On Fri, Oct 12, 2018 at 5:55 PM Brian Norris <briannorris@chromium.org> wrote: > > > > I rarely see drivers specify precise voltage requirements like this, but > > if we really have to...let's at least give a little wiggle room. Board > > designs (and accompanying device trees) may not provide exactly the > > voltage listed here, and we shouldn't fail to probe just because of > > this. > > > > Round these ranges down to the nearest volt, and provide a 0.05V margin. > > The regulator should provide its own supported ranges, which will > > helpfully intersect with these ranges. > > > > I would just as well remove these ranges entirely, but if I understand > > correctly, there's some reason that QCOM SoC's like to set zero / > > non-zero voltages. > > Yeah, I'll try to up-prioritize working on making that better > (assuming others like my ideas in that area). Ah, OK, so my understanding is correct? (I feel like I've bumped into this multiple times, but it probably didn't stick because it makes so little sense to me.) > > static struct ath10k_vreg_info vreg_cfg[] = { > > - {NULL, "vdd-0.8-cx-mx", 800000, 800000, 0, 0, false}, > > - {NULL, "vdd-1.8-xo", 1800000, 1800000, 0, 0, false}, > > - {NULL, "vdd-1.3-rfa", 1304000, 1304000, 0, 0, false}, > > - {NULL, "vdd-3.3-ch0", 3312000, 3312000, 0, 0, false}, > > + {NULL, "vdd-0.8-cx-mx", 800000, 850000, 0, 0, false}, > > + {NULL, "vdd-1.8-xo", 1800000, 1850000, 0, 0, false}, > > + {NULL, "vdd-1.3-rfa", 1300000, 1350000, 0, 0, false}, > > + {NULL, "vdd-3.3-ch0", 3300000, 3350000, 0, 0, false}, > > These look fine to me. I find it really funny that this array has all > those load values and they're all 0, but that's not new to your patch. Indeed, funny. It's also funny to have that 'required' field, which is all 'false' -- but that kinda goes to your binding review too: there's an overabundant use of "optional", to avoid defining real requirements on a per-IP basis. Brian
diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c index b63ae8b006b4..5a8e87339df2 100644 --- a/drivers/net/wireless/ath/ath10k/snoc.c +++ b/drivers/net/wireless/ath/ath10k/snoc.c @@ -47,10 +47,10 @@ static char *const ce_name[] = { }; static struct ath10k_vreg_info vreg_cfg[] = { - {NULL, "vdd-0.8-cx-mx", 800000, 800000, 0, 0, false}, - {NULL, "vdd-1.8-xo", 1800000, 1800000, 0, 0, false}, - {NULL, "vdd-1.3-rfa", 1304000, 1304000, 0, 0, false}, - {NULL, "vdd-3.3-ch0", 3312000, 3312000, 0, 0, false}, + {NULL, "vdd-0.8-cx-mx", 800000, 850000, 0, 0, false}, + {NULL, "vdd-1.8-xo", 1800000, 1850000, 0, 0, false}, + {NULL, "vdd-1.3-rfa", 1300000, 1350000, 0, 0, false}, + {NULL, "vdd-3.3-ch0", 3300000, 3350000, 0, 0, false}, }; static struct ath10k_clk_info clk_cfg[] = {
I rarely see drivers specify precise voltage requirements like this, but if we really have to...let's at least give a little wiggle room. Board designs (and accompanying device trees) may not provide exactly the voltage listed here, and we shouldn't fail to probe just because of this. Round these ranges down to the nearest volt, and provide a 0.05V margin. The regulator should provide its own supported ranges, which will helpfully intersect with these ranges. I would just as well remove these ranges entirely, but if I understand correctly, there's some reason that QCOM SoC's like to set zero / non-zero voltages. Signed-off-by: Brian Norris <briannorris@chromium.org> --- drivers/net/wireless/ath/ath10k/snoc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)