[PATCH v2] ASoC: da7219: Allow pdata to specify a VDDIO

[PATCH v2] ASoC: da7219: Allow pdata to specify a VDDIO

[Daniel Kurtz]

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(-)

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

[PATCH v2] ASoC: da7219: Allow pdata to specify a VDDIO

[Daniel Kurtz]

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(-)

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

RE: [PATCH v2] ASoC: da7219: Allow pdata to specify a VDDIO

[Adam Thomson]

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

Re: [PATCH v2] ASoC: da7219: Allow pdata to specify a VDDIO

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1223 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2] ASoC: da7219: Allow pdata to specify a VDDIO

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1223 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]