Enhances the DAC+ driver to control the optional headphone amplifier

[j-schambacher]

Probes on the I2C bus for TPA6130A2, if successful, requests the
tpa6130-module and sets DT-parameter 'status' from 'disabled' to 'okay'
using change_sets to enable the headphone control.

Signed-off-by: Joerg Schambacher [email protected]

[pelwell]

Although very clever, is it really necessary to have the hpamp node disabled by default? Why don't you leave it permanently enabled? I'm pretty sure the module won't be loaded unless a device is present at that address.

[j-schambacher]

I've tried that. It does (unfortunately) not work that easy way. I guess many people have tried around optional parts in DT definitions. Your are right, the module is not loaded but as it is in the DT-overlay defined the whole 'board' is considered not working/loaded properly. I think that's actually the reason behind the 'dynamic.c' portion of the OF concept.

[pelwell]

This line only works by accident, and only in 32-bit builds. "okay" is a const char *, which occupies 4 bytes - the same as the string length. In a 64-bit build the sizeof would have the value 8. Just write 4 + 1.

[j-schambacher]

This is a good one - agreed! Thanks! Will fix...

[popcornmix]

Actually, a string literal is an array of size N const chars, that decays to const char pointer in most uses.
But that doesn't include sizeof. sizeof("okay") is 5 (on 32 or 64-bit) - it will include the terminating null.
So correct fix is to remove the +1. (although '4+1' also works).

[pelwell]

I'm not in a position to try this myself, but as an experiment could you change the code as follows?:

	if (hb_hp_detect()) {
		card->aux_dev = hifiberry_dacplus_aux_devs;
		card->num_aux_devs =
				ARRAY_SIZE(hifiberry_dacplus_aux_devs);
		/* I don't think this module load is necessary, but leave it for now */
		ret = request_module("snd-soc-tpa6130a2");
		if (ret) {
			dev_err(&pdev->dev,
			"loading tpa6130a2 module failed: %d\n", ret);
			return -ENODEV;
		}
		/* Delete everything else */
	}

[j-schambacher]

... and I've tried your suggestions:
The request_module() indeed is obsolete - it's a remainder from my experiments earlier and I haven't thought about it later. Setting the status-property to 'okay' loads it.
The change_set stuff is necessary: if a component is set to 'disabled' the corresponding module is not loaded. If now in the 'card' definitions the aux_dev is declared, the whole 'card' failed to install, because the 'disabled' component is missing. It is not automatically loaded just because of the card-declaration. Otherwise, setting a component to 'disabled' in the DT-overlay would have never any effect. And setting it to 'okay' does not work either in the case the component is not present. What do you think?

[j-schambacher]

That would be the code:

/* probe for head phone amp */
if (hb_hp_detect()) {
	card->aux_dev = hifiberry_dacplus_aux_devs;
	card->num_aux_devs =
			ARRAY_SIZE(hifiberry_dacplus_aux_devs);
	tpa_node = of_find_compatible_node(NULL, NULL, "ti,tpa6130a2");
	tpa_prop = of_find_property(tpa_node, "status", &len);

	if (strcmp((char *)tpa_prop->value, "okay")) {
		/* and activate headphone using change_sets */
		dev_info(&pdev->dev, "activating headphone amplifier");
		of_changeset_init(&ocs);
		ret = of_changeset_update_property(&ocs, tpa_node,
						&tpa_enable_prop);
		if (ret) {
			dev_err(&pdev->dev,
			"cannot activate headphone amplifier\n");
			return -ENODEV;
		}
		ret = of_changeset_apply(&ocs);
		if (ret) {
			dev_err(&pdev->dev,
			"cannot activate headphone amplifier\n");
			return -ENODEV;
		}
	}
}

[pelwell]

I was expecting (although I didn't say so) you to test with the status set to okay because, without any reference to it from either the soundcard driver or the soundcard's part of the overlay. I can't see how enabling the node would prevent the soundcard from intialising correctly.

One of the effects of enabling the hpamp's DT node from the main driver is to force an initialisation order - main soundcard first, hpamp second - but I wouldn't expect that to make a positive difference.

This all feels unnecessary, but it's your driver and I don't have time to dig into it now.

[pelwell]

Leave the changeset code in, fix the sizeof error, then push it and ping me.

[j-schambacher]

Phil,

Also tried your suggestion with status 'okay' and that works, too. The drawback is the headphone driver gets loaded always. The question is now to treat a couple of lines of code vs the useless memory usage for an unnecessary driver. As we want to use that concept also for other card drivers in the future, I really want to agree with you on the best and smartest solution.

[pelwell]

I was expecting the address to be probed before the module was loaded, but thinking about it there is no real safe way to do that for all devices - it's better to let the probe function for the driver look for it, which is why the module gets loaded.

It gets neater if you don't use Device Tree and call i2c_new_device directly. Here's an example I found, but there are many to choose from: https://github.com/raspberrypi/linux/blob/rpi-5.4.y/drivers/gpu/drm/i2c/sil164_drv.c#L380

Have a read, then decide which you prefer.

[j-schambacher]

I've had a close look to the other i2c_new_*** approach and still, I think, the 'cleanest' way is to simply "switch" the status. Then everything is completely handled by the DT and OF infrastructure. I've just updated the commit/PR.

[pelwell]

OK - I give in. Just split the overlay change into a separate commit for easier squashing and I'll merge this PR.

[j-schambacher]

Squashed everything already into 1 commit. Should be OK now.