Up2/PCM512x w/ gpiochip to control LEDs and clocks #1945

plbossart · 2020-03-28T00:25:03Z

These patches provide support for LED control with a gpiochip as well as clock control for HifiBerry DAC+ PRO (codec master, APL slave)

This was tested on my Up2 board with the following configs:

BIOS 4.0
Adanced tab
HAT Configurations
I2S6 (Pin12) Config: -> Input (default is Output)
I2S6 (Pin35) Config: -> Input
make sure the BIOS settings are correct, I've seen the pins reset to Output sometimes.

Topology change:
thesofproject/sof#2671
sof-apl-pcm512x-master.tplg copied as sof-apl-pcm512x.tplg
This changes the SSP settings from codec_slave to codec_master. We may need additional changes for the media

ACPI initrd:
thesofproject/acpi-scripts#8
This adds the PCM512x-codec-master.asl file. You need to make sure this file is used ONLY when the BIOS settings are configured for inputs

Kconfig change:
thesofproject/kconfig#29
This adds support for CONFIG_OF needed to probe a platform driver using the .compatible string

Testers welcome, this could be a very nice addition to our CI tests where we don't have Slave mode support.

Success can be checked with the following message
[ 145.270589] sof_pcm512x sof_pcm512x: DAC+ PRO detected

And sound coming out :-)

Missing: Need to provide DT bindings before upstream
Open: is the CONFIG_AGENT compatible with the codec master mode? If not we have a disconnect between topology capabilities and firmware setup.

sound/soc/codecs/pcm512x.c

sound/soc/intel/boards/sof_pcm512x.c

kv2019i

Nothing major aside what @lyakh has raised.

sound/soc/codecs/pcm512x.c

drivers/clk/clk-hifiberry-dacpro.c

lyakh · 2020-04-01T07:43:08Z

sound/soc/intel/boards/sof_pcm512x.c

We have multiple HiFiBerry models, right? Maybe an enum?

no, we only have two

sound/soc/intel/boards/sof_pcm512x.c

singalsu · 2020-04-01T13:27:21Z

Good instructions, thank you!

I got the "DAC+ PRO detected" to dmesg but it sounds like the sample rate is off (flat). Playing 997 Hz sine sounds as 880 Hz and playback of 10s wav clip takes 11.5s. There's traces output with "ERROR validate(), ll drift detected, delta = 20781". The sound is continuous and no xruns, and other but rate good quality as far as I can tell with my home setup (cheapo powered travel speaker only).

I'm using your topology as such, no ASRC.

singalsu · 2020-04-01T14:50:18Z

I got the oscilloscope connected. The frame sync is about 42.2 kHz. The variation is 42.1 - 42.3 kHz:

I'm not familiar with the scope SW. I'll see if I can get better picture with faster sampling.

singalsu · 2020-04-01T15:07:00Z

Not faster sampling but learned to zoom. The BCLK is steady 2.049 MHz.

plbossart · 2020-04-01T15:38:58Z

@singalsu if the bclock is 2048kHz, it is based on 32kHz instead of 48kHz then, or we have a problem with S24 handling? Interesting.

But the 997->880 Hz change would mean a 42367kHz, that's not aligned with the bclk settings.

plbossart · 2020-04-01T20:18:52Z

@lyakh can you recheck the code, I added most of your suggested changes.

@singalsu the clock errors are corrected, and if you take in addition the update from the topology I just pushed we can support 44.1 kHz. There were two issues in my previous versions, I was taking the wrong clock and the bclk_ratio was 24 instead of 32. Note that it seems the BIOS configuration for inputs does not survive if you take the power off, I again had issues this morning.

sound/soc/codecs/pcm512x.c

drivers/clk/clk-hifiberry-dacpro.c

sound/soc/intel/boards/sof_pcm512x.c

singalsu · 2020-04-02T09:11:11Z

@plbossart Great work, here the playback is now perfect as far as I can tell with the setup. I don't have issue with BIOS settings, the settings remain. Maybe your flash or battery backup (?) has got broken.

Once your work is merged I'll update the UP2 test ASRC topology use this configuration.

Edit: I've tested both 44.1 kHz and 48 kHz topologies.

plbossart · 2020-04-02T11:01:34Z

@lyakh @singalsu update pushed, thanks for testing and reviews, much appreciated

The function connects an IRQ domain to a gpiochip and reuses gpiochip_to_irq() which is provided by gpiolib. gpiochip_irqchip_* and regmap_irq partially provide the same functionality. This function will help to connect just the minimal functionality of the gpiochip_irqchip which is needed to work together with regmap-irq. Signed-off-by: Michael Walle <michael@walle.cc>

There are quite a lot simple GPIO controller which are using regmap to access the hardware. This driver tries to be a base to unify existing code into one place. This won't cover everything but it should be a good starting point. It does not implement its own irq_chip because there is already a generic one for regmap based devices. Instead, the irq_chip will be instantiated in the parent driver and its irq domain will be associate to this driver. For now it consists of the usual registers, like set (and an optional clear) data register, an input register and direction registers. Out-of-the-box, it supports consecutive register mappings and mappings where the registers have gaps between them with a linear mapping between GPIO offset and bit position. For weirder mappings the user can register its own .xlate(). Signed-off-by: Michael Walle <michael@walle.cc>

Add myself as a reviewer for the gpio regmap. Signed-off-by: Michael Walle <michael@walle.cc>

plbossart · 2020-05-28T17:06:12Z

@singalsu can you try the update? it's the same functionality but implemented with a shiny new 'gpio-regmap' solution.

The GPIOs are used e.g. on HifiBerry DAC+ HATs to control the LED (GPIO3) and the choice of the 44.1 (GPIO6) or 48 (GPIO3) kHz oscillator (when present). Enable gpio_regmap to get/set values and get/set directions. Tested with GPIO_LIB from sys/class/gpio, the LED turns on/off as desired. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Using devm_clk_get() with a NULL string fails on ACPI platforms, use the "sclk" string as a fallback. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Remove direct regmap access, use gpios exposed by PCM512x codec Keep the codec_init function, this will be used in following patches The gpios handling is done with an explicit lookup table. We cannot use ACPI-based mappings since we don't have an ACPI device for the machine driver, and the gpiochip is created during the probe of the PCM512x driver. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Try to detect HifiBerry 44.1 and 48kHz oscillators on codec init Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The SCLK is resumed by the codec driver. In case the rate specified in hw_params does not match the current configuration, disable, set the new rate and restart the clock. There is no operation on hw_free, the codec suspend routine will disable/deprepare the clock. Note that we don't change the DAI configuration when the DAC+ PRO is detected. All changes for the codec master mode are handled in the topology file (DAI configuration change and scheduling change) Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

This configuration is needed to get the GPIO-controller clocks. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

This patch imports the clock code from the Raspberry v5.5-y tree. The ASoC machine driver initially present in this patch was dropped. The comments are also dropped but all sign-offs are kept below. The patch authorship was modified with explicit permission from Daniel Matuschek to make sure it matches the Signed-off tag. This patch generates a lot of checkpatch.pl warnings that are corrected in follow-up patches. Signed-off-by: DigitalDreamtime <clive.messer@digitaldreamtime.co.uk> Signed-off-by: Daniel Matuschek <daniel@hifiberry.com> Signed-off-by: Matthias Reichl <hias@horus.com> Signed-off-by: Hui Wang <hui.wang@canonical.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Reformat license information and add Intel copyright Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Lots of small issues, xmas style, alignment, wrong comments, memory leak Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Make sure OF is enabled, in case ACPI platforms use OF matching with PRP0001 and .compatible string Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

devm_clk_register() and of_clk_add_provider() are deprecated, use the recommended functions. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

On ACPI platforms the of_ functions are irrelevant, conditionally compile them out and add devm_clk_hw_register_clkdev() call instead. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

devm_clk_get() fails on ACPI platforms when a NULL string is used. Create a "sclk" lookup to make sure codec and machine drivers can get the clock. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Now that the PCM512x driver exposes GPIOs, we can set their values as needed in this clk driver (instead of doing nothing). This clk driver does not have access to the codec regmap, so it only toggles GPIOs. The user (typically a machine driver) should verify that the clocks are present by testing the PCM512x_RATE4_DET register (reports if the sclk is seen by the codec). Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Add a delay to make sure the PCM512x codec can detect SCLK presence. The initial code from the Raspberry tree used msleep(2), which can be up to 20ms. A delay of 5-10ms seems fine in practice. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The Hifiberry DAC+ PRO relies on two local audio oscillators exposed with the clock framework. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

andy-shev

Looks good to me now!

andy-shev · 2020-05-28T20:56:15Z

sound/soc/intel/boards/sof_pcm512x.c

+
+	ctx->gpio_4 = devm_gpiod_get(&pdev->dev, "PCM512x-GPIO4",
+				     GPIOD_OUT_LOW);
+


Extra blank line

andy-shev · 2020-05-28T20:56:53Z

sound/soc/intel/boards/sof_pcm512x.c

+	/* .dev_id set during probe */
+	.table = {
+		GPIO_LOOKUP("pcm512x-gpio", 3, "PCM512x-GPIO4", GPIO_ACTIVE_HIGH),
+		{ },


Usually we don't put comma after terminator line.

andy-shev · 2020-05-28T20:58:02Z

sound/soc/codecs/pcm512x.c

-		if (ret != 0) {
-			dev_err(dev, "Failed to enable SCLK: %d\n", ret);
+	for (i = 0; i < ARRAY_SIZE(clk_name); i++) {
+		pcm512x->sclk = devm_clk_get(dev, clk_name[i]);


Perhaps devm_clk_get_optional()?

andy-shev · 2020-05-28T20:59:02Z

sound/soc/codecs/pcm512x.c

+			break;
+		}
+
+		if (!clk_name[i])


Why not positive conditional?

plbossart · 2021-11-04T23:07:46Z

I don't have the time to complete this work, it's been 18 months, closing.

plbossart requested review from dbaluta, kv2019i, lgirdwood and ranj063 as code owners March 28, 2020 00:25

plbossart requested a review from lyakh March 28, 2020 00:25

lyakh requested changes Mar 30, 2020

View reviewed changes

kv2019i reviewed Mar 30, 2020

View reviewed changes

sound/soc/codecs/pcm512x.c Outdated Show resolved Hide resolved

plbossart changed the title ~~Up2/PCM512x LEDs w/ gpiochip~~ Up2/PCM512x w/ gpiochip to control LEDs and clocks Mar 31, 2020

plbossart force-pushed the fix/sof-pcm512x-leds branch from 062e787 to 8ed2d76 Compare March 31, 2020 20:56

plbossart requested review from kv2019i, lyakh and singalsu March 31, 2020 21:00

lyakh requested changes Apr 1, 2020

View reviewed changes

plbossart force-pushed the fix/sof-pcm512x-leds branch from 8ed2d76 to db1d01f Compare April 1, 2020 20:16

plbossart requested a review from lyakh April 1, 2020 20:19

lyakh requested changes Apr 2, 2020

View reviewed changes

singalsu previously approved these changes Apr 2, 2020

View reviewed changes

plbossart dismissed singalsu’s stale review via e5172b1 April 2, 2020 11:00

plbossart force-pushed the fix/sof-pcm512x-leds branch from db1d01f to e5172b1 Compare April 2, 2020 11:00

plbossart requested review from lyakh and singalsu April 2, 2020 11:00

plbossart force-pushed the fix/sof-pcm512x-leds branch 2 times, most recently from 167f5a7 to fb9fcc3 Compare April 2, 2020 15:09

mwalle added 3 commits May 28, 2020 10:12

MAINTAINERS: Add gpio regmap section

0e984fa

Add myself as a reviewer for the gpio regmap. Signed-off-by: Michael Walle <michael@walle.cc>

plbossart force-pushed the fix/sof-pcm512x-leds branch from d6cb9cd to fe8af5f Compare May 28, 2020 17:05

plbossart and others added 16 commits May 28, 2020 13:02

ASoC: pcm512x: use "sclk" string to retrieve clock

25462c9

Using devm_clk_get() with a NULL string fails on ACPI platforms, use the "sclk" string as a fallback. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: Intel: sof-pcm512x: detect Hifiberry DAC+ PRO

f970871

Try to detect HifiBerry 44.1 and 48kHz oscillators on codec init Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: Intel: sof-pcm512x: select HIFIBERRY_DACPRO clk

ef061d1

This configuration is needed to get the GPIO-controller clocks. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

clk: hifiberry-dacpro: update SDPX/copyright

daa2132

Reformat license information and add Intel copyright Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

clk: hifiberry-dacpro: style cleanups, use devm_

e3277c2

Lots of small issues, xmas style, alignment, wrong comments, memory leak Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

clk: hifiberry-dacpro: add OF dependency

abdc298

Make sure OF is enabled, in case ACPI platforms use OF matching with PRP0001 and .compatible string Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

clk: hifiberry-dacpro: transition to _hw functions

8623d27

devm_clk_register() and of_clk_add_provider() are deprecated, use the recommended functions. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

clk: hifiberry-dacpro: add ACPI support

5996969

On ACPI platforms the of_ functions are irrelevant, conditionally compile them out and add devm_clk_hw_register_clkdev() call instead. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

clk: hifiberry-dacpro: add "sclk" lookup

01ed837

devm_clk_get() fails on ACPI platforms when a NULL string is used. Create a "sclk" lookup to make sure codec and machine drivers can get the clock. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: dt-bindings: add document for Hifiberry DAC+ PRO clock

4424962

The Hifiberry DAC+ PRO relies on two local audio oscillators exposed with the clock framework. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart force-pushed the fix/sof-pcm512x-leds branch from fe8af5f to 4424962 Compare May 28, 2020 18:06

andy-shev approved these changes May 28, 2020

View reviewed changes

andy-shev reviewed May 28, 2020

View reviewed changes

sound/soc/intel/boards/sof_pcm512x.c

ctx->gpio_4 = devm_gpiod_get(&pdev->dev, "PCM512x-GPIO4",

GPIOD_OUT_LOW);

Copy link

andy-shev May 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra blank line

andy-shev reviewed May 28, 2020

View reviewed changes

sound/soc/codecs/pcm512x.c

break;

}

if (!clk_name[i])

Copy link

andy-shev May 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not positive conditional?

plbossart closed this Nov 4, 2021

plbossart mentioned this pull request Feb 3, 2022

ASoC: SOF: Intel: boards: fix GPIOLIB dependencies #3409

Merged


		ctx->gpio_4 = devm_gpiod_get(&pdev->dev, "PCM512x-GPIO4",
		GPIOD_OUT_LOW);

Up2/PCM512x w/ gpiochip to control LEDs and clocks #1945

Up2/PCM512x w/ gpiochip to control LEDs and clocks #1945

Uh oh!

Conversation

plbossart commented Mar 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lyakh Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

plbossart Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

singalsu commented Apr 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

singalsu commented Apr 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

singalsu commented Apr 1, 2020

Uh oh!

plbossart commented Apr 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart commented Apr 1, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

singalsu commented Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart commented Apr 2, 2020

Uh oh!

plbossart commented May 28, 2020

Uh oh!

andy-shev left a comment

Choose a reason for hiding this comment

Uh oh!

andy-shev May 28, 2020

Choose a reason for hiding this comment

Uh oh!

andy-shev May 28, 2020

Choose a reason for hiding this comment

Uh oh!

andy-shev May 28, 2020

Choose a reason for hiding this comment

Uh oh!

andy-shev May 28, 2020

Choose a reason for hiding this comment

Uh oh!

plbossart commented Nov 4, 2021

Uh oh!

plbossart commented Mar 28, 2020 •

edited

Loading

singalsu commented Apr 1, 2020 •

edited

Loading

singalsu commented Apr 1, 2020 •

edited

Loading

plbossart commented Apr 1, 2020 •

edited

Loading

singalsu commented Apr 2, 2020 •

edited

Loading