Hifiberry DAC+ driver addition to support Hifiberry AMP100 #4102

j-schambacher · 2021-01-29T08:55:20Z

Adds new DT-overlay to control AMP100 and enhances the DAC+ driver with
GPIO handling for RESET and MUTE HW control of the AMP100.
Contains also a small fix for proper PLL control after changing sample rates.

Signed-off-by: Joerg Schambacher joerg@hifiberry.com

pelwell

I've highlighted a few issues in the PR. In addition, it would be helpful if you could split the overlay change into a separate commit. It would also be polite to give Clive a mention in your driver commit message.

arch/arm/boot/dts/overlays/README

pelwell · 2021-01-29T09:19:24Z

arch/arm/boot/dts/overlays/hifiberry-amp100-overlay.dts

+		leds_off = <&hifiberry_dacplus>,"hifiberry-dacplus,leds_off?";
+		mute_ext_ctl = <&hifiberry_dacplus>,"hifiberry-dacplus,mute_ext_ctl:0";
+		auto_mute = <&hifiberry_dacplus>,"hifiberry-dacplus,auto_mute?";
+		reset_ext_ctl = <&hifiberry_dacplus>,"hifiberry-dacplus,reset_ext_ctl?";


This parameter needs to be mentioned in the README.

reset_ext_ctl is actually not used. I will remove it.

arch/arm/boot/dts/overlays/README

sound/soc/bcm/hifiberry_dacplus.c

pelwell · 2021-01-29T09:37:52Z

sound/soc/bcm/hifiberry_dacplus.c

+
+		/* check for HW MUTE as defined in DT-overlay
+		   active high, therefore default to HIGH to MUTE  */
+		snd_mute_gpio =	devm_gpiod_get(&pdev->dev, "mute", GPIOD_OUT_HIGH);


Do you actually want this to be devm_gpiod_get_optional? That will return NULL if nothing is found, but an error if there is some other problem.

Ok, followed your proposal and modified the following if(..) to get the dev_info() correct about the selected GPIO, too. Otherwise, it will run into a NULL pointer exception.

pelwell · 2021-01-29T10:30:32Z

P.S. We will be switching to rpi-5.10.y in a matter of days - you might want to target that instead.

Adds new DT-overlay to control AMP100. Signed-off-by: Joerg Schambacher <joerg@hifiberry.com>

j-schambacher · 2021-01-29T15:23:07Z

Hopefully all is fixed as required. Also split into two commits.
Thanks for the support to Phil and Clive!

j-schambacher · 2021-01-29T15:25:08Z

@pelwell we have not yet done any testing with 5.10 therefore we keep it for now on 5.4. But I will probably start soon with some 5.10 testing.

pelwell

Error detection is still necessary with the optional GPIO, but otherwise I'm happy with this.

There are two typos/Germanic spellings in the driver commit message which I would have ignored ("mixeer" and "switchung"), but since you're redoing that commit...

pelwell · 2021-01-29T15:54:19Z

sound/soc/bcm/hifiberry_dacplus.c

+		   active high, therefore default to HIGH to MUTE  */
+		snd_mute_gpio =	devm_gpiod_get_optional(&pdev->dev, "mute", GPIOD_OUT_HIGH);
+		if (snd_mute_gpio)
+			dev_info(&pdev->dev, "GPIO%i for HW-MUTE selected",


An error is still a possible outcome that should be reported here - an absent property results in a NULL, but some other problem will still cause an error (which is probably what you want).

j-schambacher · 2021-02-01T12:28:52Z

@pelwell
Just want to x-check before commiting the (last) changes. To avoid the useless messages from assigning the GPIOs (AMP100 case) before the complete card dirver can be successfully load, I'd like to move these into an if-statement as below. Please let me know if that's ok. I've also added the error-handling. Thanks.

`....

	/* check for HW RESET (AMP100) */
	snd_reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", GPIOD_OUT_HIGH);
	if (IS_ERR(snd_reset_gpio)) {
		dev_err(&pdev->dev, "Can't allocate GPIO (HW-RESET)");
		return PTR_ERR(snd_reset_gpio);
	}

}

ret = devm_snd_soc_register_card(&pdev->dev,
		&snd_rpi_hifiberry_dacplus);
if (ret && ret != -EPROBE_DEFER)
	dev_err(&pdev->dev,
		"snd_soc_register_card() failed: %d\n", ret);
if (!ret) {
	if (snd_mute_gpio)
		dev_info(&pdev->dev, "GPIO%i for HW-MUTE selected",
				gpio_chip_hwgpio(snd_mute_gpio));
	if (snd_reset_gpio)
		dev_info(&pdev->dev, "GPIO%i for HW-RESET selected",
				gpio_chip_hwgpio(snd_reset_gpio));
}
return ret;

}
`

pelwell · 2021-02-01T12:37:04Z

That change looks sensible to me, but I'll reserve final judgement until I've seen the updated commits.

pelwell · 2021-02-01T13:27:16Z

Sorry Jörg, but checkpatch is complaining about long lines and multi-line comment formatting:

WARNING: line over 80 characters
#29: FILE: sound/soc/bcm/hifiberry_dacplus.c:7:
+ *		Headphone/AMP100 added by Joerg Schambacher, <joerg@hifiberry.com>

WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.rst
#143: FILE: sound/soc/bcm/hifiberry_dacplus.c:230:
+		msleep(1);

WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.rst
#145: FILE: sound/soc/bcm/hifiberry_dacplus.c:232:
+		msleep(1);

WARNING: Block comments use * on subsequent lines
#193: FILE: sound/soc/bcm/hifiberry_dacplus.c:461:
+		/* check for HW MUTE as defined in DT-overlay
+		   active high, therefore default to HIGH to MUTE  */

WARNING: Block comments use a trailing */ on a separate line
#193: FILE: sound/soc/bcm/hifiberry_dacplus.c:461:
+		   active high, therefore default to HIGH to MUTE  */

WARNING: line over 80 characters
#194: FILE: sound/soc/bcm/hifiberry_dacplus.c:462:
+		snd_mute_gpio =	devm_gpiod_get_optional(&pdev->dev, "mute", GPIOD_OUT_HIGH);

WARNING: line over 80 characters
#201: FILE: sound/soc/bcm/hifiberry_dacplus.c:469:
+		pp = of_find_property(pdev->dev.of_node, "hifiberry-dacplus,mute_ext_ctl", &tmp);

WARNING: line over 80 characters
#204: FILE: sound/soc/bcm/hifiberry_dacplus.c:472:
+						"hifiberry-dacplus,mute_ext_ctl", &mute_ext)) {

WARNING: line over 80 characters
#205: FILE: sound/soc/bcm/hifiberry_dacplus.c:473:
+				mute_ext_ctl = 1; /* ALSA control will be used */

WARNING: line over 80 characters
#210: FILE: sound/soc/bcm/hifiberry_dacplus.c:478:
+		snd_reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", GPIOD_OUT_HIGH);

Don't worry about the msleep()s - I don't think timing is critical in this case.

j-schambacher · 2021-02-01T13:32:11Z

Ok, most of the style (long lines etc) have been already in there before - so I didn't care...is the policy now to fix all of these?
The msleep() is not critical.

pelwell · 2021-02-01T13:38:20Z

While there are some existing long lines, checkpatch is only complaining about the new ones. I'm not expecting you to fix existing problems, but to not make things worse.

j-schambacher · 2021-02-01T13:56:36Z

I hope it's fixed now. I usually run checkpatch on the whole file no to miss stuff I screwed some how. Will run it only on the commits from now on. Sorry.

pelwell · 2021-02-01T13:59:43Z

Take the comma out of your email address on line 7 then it will also fit.

I'm happy for you to check more than the minimum, but I don't expect you to.

Adds the necessary GPIO handling and ALSA mixer extensions. Also fixes a problem with the PLL/CLK control when switching sample rates. Thanks to Clive Messer for the support! Signed-off-by: Joerg Schambacher <joerg@hifiberry.com>

j-schambacher · 2021-02-01T14:06:52Z

...even without comma - too long. Now reduced to the minimum.

clivem · 2021-02-03T13:28:57Z

@j-schambacher Next time you have cause to edit your driver..... Noticed a few log lines without newline termination of text string.

--- hifiberry_dacplus.c~	2021-02-03 01:53:11.000000000 +0000
+++ hifiberry_dacplus.c	2021-02-03 13:23:05.662751656 +0000
@@ -414,7 +414,7 @@
 
 		if (strcmp((char *)tpa_prop->value, "okay")) {
 			/* and activate headphone using change_sets */
-			dev_info(&pdev->dev, "activating headphone amplifier");
+			dev_info(&pdev->dev, "activating headphone amplifier\n");
 			of_changeset_init(&ocs);
 			ret = of_changeset_update_property(&ocs, tpa_node,
 							&tpa_enable_prop);
@@ -464,7 +464,7 @@
 		snd_mute_gpio =	devm_gpiod_get_optional(&pdev->dev,
 						 "mute", GPIOD_OUT_HIGH);
 		if (IS_ERR(snd_mute_gpio)) {
-			dev_err(&pdev->dev, "Can't allocate GPIO (HW-MUTE)");
+			dev_err(&pdev->dev, "Can't allocate GPIO (HW-MUTE)\n");
 			return PTR_ERR(snd_mute_gpio);
 		}
 
@@ -483,7 +483,7 @@
 		snd_reset_gpio = devm_gpiod_get_optional(&pdev->dev,
 						"reset", GPIOD_OUT_HIGH);
 		if (IS_ERR(snd_reset_gpio)) {
-			dev_err(&pdev->dev, "Can't allocate GPIO (HW-RESET)");
+			dev_err(&pdev->dev, "Can't allocate GPIO (HW-RESET)\n");
 			return PTR_ERR(snd_reset_gpio);
 		}
 
@@ -496,10 +496,10 @@
 			"snd_soc_register_card() failed: %d\n", ret);
 	if (!ret) {
 		if (snd_mute_gpio)
-			dev_info(&pdev->dev, "GPIO%i for HW-MUTE selected",
+			dev_info(&pdev->dev, "GPIO%i for HW-MUTE selected\n",
 					gpio_chip_hwgpio(snd_mute_gpio));
 		if (snd_reset_gpio)
-			dev_info(&pdev->dev, "GPIO%i for HW-RESET selected",
+			dev_info(&pdev->dev, "GPIO%i for HW-RESET selected\n",
 					gpio_chip_hwgpio(snd_reset_gpio));
 	}
 	return ret;

j-schambacher · 2021-02-03T15:38:10Z

@clivem I've just x-checked on my 5.4 setup and at least the last dev_info() gives the desired output without the '\n'. The subsequent message starts on a new line. Same for the headphone message. It is necessary the end with '\n' ?

clivem · 2021-02-03T19:07:43Z

Is it strictly necessary, no. Is it still sensible to do it.....

ISTR, a big discussion about this back in 2017 when Linus decided to enforce using KERN_CONT with printk() if you wanted multiple logging calls concat'd together, otherwise the behaviour changed to starting every printk() on a newline. Something like that.... @pelwell for the definitive answer?

pelwell · 2021-02-03T19:32:47Z

I think you are referring to this: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4bcc595ccd80decb4245096e3d1258989c50ed41

Thanks - I wasn't aware of it. It's a shame there wasn't a followup commit to remove all of the unnecessary newlines. ;-)

clivem · 2021-02-03T20:27:41Z

@pelwell Yes, I think that was the commit that caused the grumbling from people who hadn't used KERN_CONT flag in the printk and just depended on their last (of many) calls having a "\n", where the behaviour changed, and each of their printk's ended up on separate lines, regardless of whether the individual printk instance was terminated with a newline or not.

kernel: configs: Enable CONFIG_MEDIA_CEC_RC See: raspberrypi/linux#4109 kernel: Hifiberry DAC+ driver addition to support Hifiberry AMP100 See: raspberrypi/linux#4102 kernel: ARM: dts: Declare Pi400 and CM4 have no audio pins See: https://www.raspberrypi.org/forums/viewtopic.php?f=98&t=301891

j-schambacher · 2021-02-09T16:47:49Z

@pelwell What is your final crecommendation? Shall I follow Clive's suggestion? I want to fix another little thing in this driver and can fix this, too..

pelwell · 2021-02-09T16:59:36Z

I for one have got used to seeing printk and friends with embedded newlines, so to avoid other people pointing out their absence I think you should add them. If at some point there is a global newline cull (think of all the memory saved) then we get rid of them here as well.

pelwell requested changes Jan 29, 2021

View reviewed changes

Adds the DT-overlays to support Hifiberry AMP100

c69fa36

Adds new DT-overlay to control AMP100. Signed-off-by: Joerg Schambacher <joerg@hifiberry.com>

pelwell reviewed Jan 29, 2021

View reviewed changes

pelwell merged commit cf14b27 into raspberrypi:rpi-5.4.y Feb 1, 2021

Hifiberry DAC+ driver addition to support Hifiberry AMP100 #4102

Hifiberry DAC+ driver addition to support Hifiberry AMP100 #4102

Uh oh!

Conversation

j-schambacher commented Jan 29, 2021

Uh oh!

pelwell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pelwell Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

j-schambacher Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pelwell Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

j-schambacher Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

pelwell commented Jan 29, 2021

Uh oh!

j-schambacher commented Jan 29, 2021

Uh oh!

j-schambacher commented Jan 29, 2021

Uh oh!

pelwell left a comment

Choose a reason for hiding this comment

Uh oh!

pelwell Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

j-schambacher commented Feb 1, 2021

Uh oh!

pelwell commented Feb 1, 2021

Uh oh!

pelwell commented Feb 1, 2021

Uh oh!

j-schambacher commented Feb 1, 2021

Uh oh!

pelwell commented Feb 1, 2021

Uh oh!

j-schambacher commented Feb 1, 2021

Uh oh!

pelwell commented Feb 1, 2021

Uh oh!

j-schambacher commented Feb 1, 2021

Uh oh!

clivem commented Feb 3, 2021

Uh oh!

j-schambacher commented Feb 3, 2021

Uh oh!

clivem commented Feb 3, 2021

Uh oh!

pelwell commented Feb 3, 2021

Uh oh!

clivem commented Feb 3, 2021

Uh oh!

j-schambacher commented Feb 9, 2021

Uh oh!

pelwell commented Feb 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants