| Message ID | 1532068715-2992-1-git-send-email-akshu.agrawal@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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
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);
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(+)