ASoC: AMD: Add a fix voltage regulator for DA7219 and ADAU7002
diff mbox

Message ID 1532068715-2992-1-git-send-email-akshu.agrawal@amd.com
State New
Headers show

Commit Message

Agrawal, Akshu July 20, 2018, 6:38 a.m. UTC
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(+)

Comments

Mark Brown July 20, 2018, 12:18 p.m. UTC | #1
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.
Agrawal, Akshu July 23, 2018, 5:26 a.m. UTC | #2
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
Mark Brown July 24, 2018, 5:14 p.m. UTC | #3
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.
Agrawal, Akshu July 25, 2018, 9 a.m. UTC | #4
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

Patch
diff mbox

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