[PATCH] ASoC: AMD: Add a fix voltage regulator for DA7219 and ADAU7002

[PATCH] ASoC: AMD: Add a fix voltage regulator for DA7219 and ADAU7002

[Akshu Agrawal]

DA7219 for our platform need to be configured for 1.8V.
Hence, we add a fixed volatge regulator with supplies
of 1.8V in the machine driver.

Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com>
---
 sound/soc/amd/Kconfig                |  2 ++
 sound/soc/amd/acp-da7219-max98357a.c | 45 ++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/sound/soc/amd/Kconfig b/sound/soc/amd/Kconfig
index 6cbf9cf..c447a51 100644
--- a/sound/soc/amd/Kconfig
+++ b/sound/soc/amd/Kconfig
@@ -8,6 +8,8 @@ config SND_SOC_AMD_CZ_DA7219MX98357_MACH
 	select SND_SOC_DA7219
 	select SND_SOC_MAX98357A
 	select SND_SOC_ADAU7002
+	select REGULATOR
+	select REGULATOR_FIXED_VOLTAGE
 	depends on SND_SOC_AMD_ACP && I2C
 	help
 	 This option enables machine driver for DA7219 and MAX9835.
diff --git a/sound/soc/amd/acp-da7219-max98357a.c b/sound/soc/amd/acp-da7219-max98357a.c
index f42606e..fdf8972 100644
--- a/sound/soc/amd/acp-da7219-max98357a.c
+++ b/sound/soc/amd/acp-da7219-max98357a.c
@@ -31,7 +31,10 @@
 #include <sound/jack.h>
 #include <linux/clk.h>
 #include <linux/gpio.h>
+#include <linux/platform_device.h>
 #include <linux/module.h>
+#include <linux/regulator/fixed.h>
+#include <linux/regulator/machine.h>
 #include <linux/i2c.h>
 #include <linux/input.h>
 #include <linux/acpi.h>
@@ -320,11 +323,53 @@ static void cz_dmic_shutdown(struct snd_pcm_substream *substream)
 	.num_controls = ARRAY_SIZE(cz_mc_controls),
 };
 
+static struct regulator_consumer_supply acp_da7219_supplies[] = {
+	REGULATOR_SUPPLY("VDD", "i2c-DLGS7219:00"),
+	REGULATOR_SUPPLY("VDDMIC", "i2c-DLGS7219:00"),
+	REGULATOR_SUPPLY("VDDIO", "i2c-DLGS7219:00"),
+	REGULATOR_SUPPLY("IOVDD", "ADAU7002:00"),
+};
+
+static struct regulator_init_data acp_da7219_data = {
+	.constraints = {
+		.always_on = 1,
+	},
+	.num_consumer_supplies = ARRAY_SIZE(acp_da7219_supplies),
+	.consumer_supplies = acp_da7219_supplies,
+};
+
+static struct fixed_voltage_config acp_da7219 = {
+	.supply_name = "reg-fixed-1.8V",
+	.microvolts = 1800000, /* 1.8V */
+	.gpio = -EINVAL,
+	.enabled_at_boot = 1,
+	.init_data = &acp_da7219_data,
+};
+
+static struct platform_device acp_da7219_regulator = {
+	.name = "reg-fixed-voltage",
+	.id = PLATFORM_DEVID_AUTO,
+	.dev = {
+		.platform_data = &acp_da7219,
+	},
+};
+
 static int cz_probe(struct platform_device *pdev)
 {
 	int ret;
 	struct snd_soc_card *card;
 	struct acp_platform_info *machine;
+	static bool regulators_registered;
+
+	if (!regulators_registered) {
+		ret = platform_device_register(&acp_da7219_regulator);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to register regulator: %d\n",
+				ret);
+			return ret;
+		}
+		regulators_registered = true;
+	}
 
 	machine = devm_kzalloc(&pdev->dev, sizeof(struct acp_platform_info),
 			       GFP_KERNEL);
-- 
1.9.1

Re: [PATCH] ASoC: AMD: Add a fix voltage regulator for DA7219 and ADAU7002

[Mark Brown]

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

On Fri, Jul 20, 2018 at 02:38:11PM +0800, Akshu Agrawal wrote:

>  static int cz_probe(struct platform_device *pdev)
>  {
>  	int ret;
>  	struct snd_soc_card *card;
>  	struct acp_platform_info *machine;
> +	static bool regulators_registered;
> +
> +	if (!regulators_registered) {
> +		ret = platform_device_register(&acp_da7219_regulator);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Failed to register regulator: %d\n",
> +				ret);
> +			return ret;
> +		}
> +		regulators_registered = true;
> +	}

You should be unregistering the regulator in your remove function, not
doing this hack here.  I'd also expect to see the card made the parent
of the device that gets registered.

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

Re: [PATCH] ASoC: AMD: Add a fix voltage regulator for DA7219 and ADAU7002

[Agrawal, Akshu]

On 7/20/2018 5:48 PM, Mark Brown wrote:
> On Fri, Jul 20, 2018 at 02:38:11PM +0800, Akshu Agrawal wrote:
> 
>>  static int cz_probe(struct platform_device *pdev)
>>  {
>>  	int ret;
>>  	struct snd_soc_card *card;
>>  	struct acp_platform_info *machine;
>> +	static bool regulators_registered;
>> +
>> +	if (!regulators_registered) {
>> +		ret = platform_device_register(&acp_da7219_regulator);
>> +		if (ret) {
>> +			dev_err(&pdev->dev, "Failed to register regulator: %d\n",
>> +				ret);
>> +			return ret;
>> +		}
>> +		regulators_registered = true;
>> +	}
> 
> You should be unregistering the regulator in your remove function, not
> doing this hack here.  I'd also expect to see the card made the parent
> of the device that gets registered.
> 

This approach shows inconsistencies and in some boot cycles da7219 fails
to get regulator. Form the logs (below) it shows time gap between the
time we call “platform_device_register(&acp_da7219_regulator);” and when
the regulator actually gets registered.

[   12.594237] regulator registered
**print after calling “platform_device_register(&acp_da7219_regulator);”
...
[   13.583689] da7219 i2c-DLGS7219:00: i2c-DLGS7219:00 supply VDD not
found, using dummy regulator
[   13.593818] da7219 i2c-DLGS7219:00: i2c-DLGS7219:00 supply VDDMIC not
found, using dummy regulator
[   13.603242] da7219 i2c-DLGS7219:00: i2c-DLGS7219:00 supply VDDIO not
found, using dummy regulator
[   13.612626] da7219 i2c-DLGS7219:00: Invalid VDDIO voltage
**Above DA7219 gets probed and does not find the regulator**
...
[   13.750894] reg_fixed_voltage_probe: Supply -> reg-fixed-1.8V
[   13.766746] reg-fixed-1.8V supplying 1800000uV
**Regulator actually gets registered**

Alternate and consistent approach to this is pushed by Daniel here:
https://patchwork.kernel.org/patch/10539485/

Thanks,
Akshu

Re: [PATCH] ASoC: AMD: Add a fix voltage regulator for DA7219 and ADAU7002

[Mark Brown]

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

On Mon, Jul 23, 2018 at 10:56:44AM +0530, Agrawal, Akshu wrote:

> This approach shows inconsistencies and in some boot cycles da7219 fails
> to get regulator. Form the logs (below) it shows time gap between the
> time we call “platform_device_register(&acp_da7219_regulator);” and when
> the regulator actually gets registered.

This hack isn't going to help with that AFAICT, you're still registering
a platform device and relying on it appearing in time?  It just
registers less often.  I think what we need here is a way to register
the link between the devices independently of the regulator registering
or firmware, that way we won't get a dummy regulator.  Since AFAICT we
know the names of the devices that are being registered we can use their
dev_name()s which seems tractable - we need a function in
regulator/machine.h which lets you provide a set of struct
regulator_consumer_supply for a target dev_name.

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

Re: [PATCH] ASoC: AMD: Add a fix voltage regulator for DA7219 and ADAU7002

[Agrawal, Akshu]

On 7/24/2018 10:44 PM, Mark Brown wrote:
> On Mon, Jul 23, 2018 at 10:56:44AM +0530, Agrawal, Akshu wrote:
> 
>> This approach shows inconsistencies and in some boot cycles da7219 fails
>> to get regulator. Form the logs (below) it shows time gap between the
>> time we call “platform_device_register(&acp_da7219_regulator);” and when
>> the regulator actually gets registered.
> 
> This hack isn't going to help with that AFAICT, you're still registering
> a platform device and relying on it appearing in time?  It just
> registers less often.

Yes, I agree and by "this approach" I also meant registering platform
device and waiting for its probe to happen.

Instead of adding a platform device to the reg-fixed-voltage, we can
register a regulator and thus not wait for the probe to occur.
Pushing a v2 with this change which has consistent behavior.

Thanks,
Akshu