| Message ID | 20180722232822.34641-1-djkurtz@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] ASoC: da7219: Allow pdata to specify a VDDIO | expand |
On 23 July 2018 00:28, Daniel Kurtz wrote: > Some systems do not have software controllable regulators driving the > DA7219's supplies, nor can they use device tree to create "always-on fixed > regulators" to easily pretend like they do. > > On these systems the call to devm_regulator_bulk_get() just creates > a set of dummy registers. Calling regulator_get_voltage() on a dummy > regulator just returns -EINVAL, in which case the DA7219 is always set up > to use the default VDDIO voltage range of 2.5-3.6V. > > Provide a new device property to let such systems specify a different > VDDIO if needed (e.g., 1.8V). I'm not sure what the general view on this is. In the past it was suggested the regulator framework was the way to go to pass this kind of information, but obviously ACPI platforms don't tend to use it. Mark, what is your feeling on this? Would you be in favour of some kind of fixed voltage regulator representation, similar to the patch for the AMD platform (ASoC: AMD: Add a fix voltage regulator for DA7219 and ADAU7002), albeit tweaked to avoid asynchronous probe() issues, or is this a reasonable route? Personally in my mind, and in an ideal world, I'd prefer just one method for retrieving this data in the codec driver, but that may not be sensible. > > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org> > --- > Changes for v2: > - fix to use device_property_read_u32() > > include/sound/da7219.h | 2 ++ > sound/soc/codecs/da7219.c | 19 +++++++++++++------ > 2 files changed, 15 insertions(+), 6 deletions(-) > > diff --git a/include/sound/da7219.h b/include/sound/da7219.h > index 1bfcb16f2d10ab..16ab125ad4adbf 100644 > --- a/include/sound/da7219.h > +++ b/include/sound/da7219.h > @@ -38,6 +38,8 @@ struct da7219_pdata { > > const char *dai_clks_name; > > + u32 vddio; > + > /* Mic */ > enum da7219_micbias_voltage micbias_lvl; > enum da7219_mic_amp_in_sel mic_amp_in_sel; > diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c > index 980a6a8bf56d38..9893920b26f41f 100644 > --- a/sound/soc/codecs/da7219.c > +++ b/sound/soc/codecs/da7219.c > @@ -1634,6 +1634,9 @@ static struct da7219_pdata *da7219_fw_to_pdata(struct > snd_soc_component *compone > else > pdata->mic_amp_in_sel = DA7219_MIC_AMP_IN_SEL_DIFF; > > + if (device_property_read_u32(dev, "dlg,vddio", &of_val32) >= 0) > + pdata->vddio = of_val32; > + > return pdata; > } > > @@ -1717,8 +1720,12 @@ static int da7219_handle_supplies(struct > snd_soc_component *component) > /* Determine VDDIO voltage provided */ > vddio = da7219->supplies[DA7219_SUPPLY_VDDIO].consumer; > ret = regulator_get_voltage(vddio); > + /* If regulator_get_voltage() fails, try to use vddio from pdata. */ > + if (ret < 0 && da7219->pdata) > + ret = da7219->pdata->vddio; > if (ret < 1200000) > - dev_warn(component->dev, "Invalid VDDIO voltage\n"); > + dev_warn(component->dev, "Invalid VDDIO voltage: %d mV\n", > + ret); > else if (ret < 2800000) > io_voltage_lvl = DA7219_IO_VOLTAGE_LEVEL_1_2V_2_8V; > > @@ -1872,6 +1879,11 @@ static int da7219_probe(struct snd_soc_component > *component) > mutex_init(&da7219->ctrl_lock); > mutex_init(&da7219->pll_lock); > > + /* Handle DT/ACPI/Platform data */ > + da7219->pdata = dev_get_platdata(component->dev); > + if (!da7219->pdata) > + da7219->pdata = da7219_fw_to_pdata(component); > + > /* Regulator configuration */ > ret = da7219_handle_supplies(component); > if (ret) > @@ -1897,11 +1909,6 @@ static int da7219_probe(struct snd_soc_component > *component) > break; > } > > - /* Handle DT/ACPI/Platform data */ > - da7219->pdata = dev_get_platdata(component->dev); > - if (!da7219->pdata) > - da7219->pdata = da7219_fw_to_pdata(component); > - > da7219_handle_pdata(component); > > /* Check if MCLK provided */ > -- > 2.18.0.233.g985f88cf7e-goog
On Mon, Jul 23, 2018 at 02:41:26PM +0000, Adam Thomson wrote: > On 23 July 2018 00:28, Daniel Kurtz wrote: > > Provide a new device property to let such systems specify a different > > VDDIO if needed (e.g., 1.8V). > I'm not sure what the general view on this is. In the past it was suggested > the regulator framework was the way to go to pass this kind of information, > but obviously ACPI platforms don't tend to use it. > Mark, what is your feeling on this? Would you be in favour of some kind of > fixed voltage regulator representation, similar to the patch for the AMD > platform (ASoC: AMD: Add a fix voltage regulator for DA7219 and ADAU7002), > albeit tweaked to avoid asynchronous probe() issues, or is this a reasonable > route? Personally in my mind, and in an ideal world, I'd prefer just one method > for retrieving this data in the codec driver, but that may not be sensible. Yeah, keeping things consistent if we can seems like a definite win which points towards using regulators here. One other thing that concerns me with using device properties here is what exactly we'd be expecting to set them - I'd not expect system integrators to suddenly start adding such properties.
diff --git a/include/sound/da7219.h b/include/sound/da7219.h index 1bfcb16f2d10ab..16ab125ad4adbf 100644 --- a/include/sound/da7219.h +++ b/include/sound/da7219.h @@ -38,6 +38,8 @@ struct da7219_pdata { const char *dai_clks_name; + u32 vddio; + /* Mic */ enum da7219_micbias_voltage micbias_lvl; enum da7219_mic_amp_in_sel mic_amp_in_sel; diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c index 980a6a8bf56d38..9893920b26f41f 100644 --- a/sound/soc/codecs/da7219.c +++ b/sound/soc/codecs/da7219.c @@ -1634,6 +1634,9 @@ static struct da7219_pdata *da7219_fw_to_pdata(struct snd_soc_component *compone else pdata->mic_amp_in_sel = DA7219_MIC_AMP_IN_SEL_DIFF; + if (device_property_read_u32(dev, "dlg,vddio", &of_val32) >= 0) + pdata->vddio = of_val32; + return pdata; } @@ -1717,8 +1720,12 @@ static int da7219_handle_supplies(struct snd_soc_component *component) /* Determine VDDIO voltage provided */ vddio = da7219->supplies[DA7219_SUPPLY_VDDIO].consumer; ret = regulator_get_voltage(vddio); + /* If regulator_get_voltage() fails, try to use vddio from pdata. */ + if (ret < 0 && da7219->pdata) + ret = da7219->pdata->vddio; if (ret < 1200000) - dev_warn(component->dev, "Invalid VDDIO voltage\n"); + dev_warn(component->dev, "Invalid VDDIO voltage: %d mV\n", + ret); else if (ret < 2800000) io_voltage_lvl = DA7219_IO_VOLTAGE_LEVEL_1_2V_2_8V; @@ -1872,6 +1879,11 @@ static int da7219_probe(struct snd_soc_component *component) mutex_init(&da7219->ctrl_lock); mutex_init(&da7219->pll_lock); + /* Handle DT/ACPI/Platform data */ + da7219->pdata = dev_get_platdata(component->dev); + if (!da7219->pdata) + da7219->pdata = da7219_fw_to_pdata(component); + /* Regulator configuration */ ret = da7219_handle_supplies(component); if (ret) @@ -1897,11 +1909,6 @@ static int da7219_probe(struct snd_soc_component *component) break; } - /* Handle DT/ACPI/Platform data */ - da7219->pdata = dev_get_platdata(component->dev); - if (!da7219->pdata) - da7219->pdata = da7219_fw_to_pdata(component); - da7219_handle_pdata(component); /* Check if MCLK provided */
Some systems do not have software controllable regulators driving the DA7219's supplies, nor can they use device tree to create "always-on fixed regulators" to easily pretend like they do. On these systems the call to devm_regulator_bulk_get() just creates a set of dummy registers. Calling regulator_get_voltage() on a dummy regulator just returns -EINVAL, in which case the DA7219 is always set up to use the default VDDIO voltage range of 2.5-3.6V. Provide a new device property to let such systems specify a different VDDIO if needed (e.g., 1.8V). Signed-off-by: Daniel Kurtz <djkurtz@chromium.org> --- Changes for v2: - fix to use device_property_read_u32() include/sound/da7219.h | 2 ++ sound/soc/codecs/da7219.c | 19 +++++++++++++------ 2 files changed, 15 insertions(+), 6 deletions(-)