ASoC: Intel: boards: remove device properties

[plbossart]

Follow the recommendation to remove device properties in the driver
who added the properties, instead of relying on the device core to
clean-up. This will help with future API rename or removals.

This change was already added for SoundWire boards, add for legacy
boards as well. This patch should have no functional impact.

Suggested-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

[plbossart]

@andy-shev, is this what you had in mind?

[plbossart]

this needs more work, we also need to remove the properties when the card registration fails.

[lyakh]

2 questions to this version: looks fine, but maybe it can be improved a bit more.

[lyakh]

this is rather theoretical, but this is a platform driver .return() method. What happens if it returns an error code? rmmod fails and the module remains loaded? Is this what we want?

[plbossart]

I don't think it matters, the driver core ignores the remove() return value.

grep --color -nH --null -e 'drv->remove' *.c
dd.c\0541:		else if (drv->remove)
dd.c\0542:			drv->remove(dev);
dd.c\0569:	else if (drv->remove)
dd.c\0570:		drv->remove(dev);
dd.c\01107:		else if (drv->remove)
dd.c\01108:			drv->remove(dev);
driver.c\0159:	    (drv->bus->remove && drv->remove) ||
platform.c\0772:	if (drv->remove)
platform.c\0773:		ret = drv->remove(dev);

[lyakh]

how about sharing this function? Maybe even just making it an inline in a suitable header, or creating a new header for it in this directory.

[plbossart]

I couldn't find a header for this, so I left it as an optimization.

[kv2019i]

@plbossart Change itself looks good, but considering it is a cleanup/refactor patch, the addition of many duplicated remove_properties() seems out of place. How about just put it in hda_dsp_common.h?

[andy-shev]

@kv2019i

@plbossart Change itself looks good, but considering it is a cleanup/refactor patch, the addition of many duplicated remove_properties() seems out of place. How about just put it in hda_dsp_common.h?

I don't know the structure of ASoC drivers but the idea from software nodes perspective is that each driver / framework / subsystem decides when to create them and when to delete. They are not dependent to the struct device! That is the main point of removing device_add_properties() and accompanying device_remove_properties(). Inside ASoC you may create a helper that will add properties and remove them whenever ASoC considers it's a right time for it.

If I'm not mistaken Heikki (author of swnodes idea) has a proposal (don't remember if it went public) to move those helpers to swnode realm like software_node_attach_to_device() or something like that.

[plbossart]

@kv2019i

@plbossart Change itself looks good, but considering it is a cleanup/refactor patch, the addition of many duplicated remove_properties() seems out of place. How about just put it in hda_dsp_common.h?

I don't know the structure of ASoC drivers but the idea from software nodes perspective is that each driver / framework / subsystem decides when to create them and when to delete. They are not dependent to the struct device! That is the main point of removing device_add_properties() and accompanying device_remove_properties(). Inside ASoC you may create a helper that will add properties and remove them whenever ASoC considers it's a right time for it.

If I'm not mistaken Heikki (author of swnodes idea) has a proposal (don't remember if it went public) to move those helpers to swnode realm like software_node_attach_to_device() or something like that.

Re-reading this after a while.

The problem is really that this is not controlled by ASoC, e.g. for the audio codecs what we do is that we set properties because we don't have DT support to set those values, and ACPI also doesn't set these properties in commercial devices, e.g.

linux/sound/soc/codecs/rt5640.c

Line 2514 in 53b27a2

/*

Somehow we need to make sure the properties are handled in the same way, whether they are set during enumeration or added manually. That's the part I don't get for now, this really can't be an ASoC specific change, can it?

[andy-shev]

@kv2019i

@plbossart Change itself looks good, but considering it is a cleanup/refactor patch, the addition of many duplicated remove_properties() seems out of place. How about just put it in hda_dsp_common.h?

I don't know the structure of ASoC drivers but the idea from software nodes perspective is that each driver / framework / subsystem decides when to create them and when to delete. They are not dependent to the struct device! That is the main point of removing device_add_properties() and accompanying device_remove_properties(). Inside ASoC you may create a helper that will add properties and remove them whenever ASoC considers it's a right time for it.
If I'm not mistaken Heikki (author of swnodes idea) has a proposal (don't remember if it went public) to move those helpers to swnode realm like software_node_attach_to_device() or something like that.

Re-reading this after a while.

The problem is really that this is not controlled by ASoC, e.g. for the audio codecs what we do is that we set properties because we don't have DT support to set those values, and ACPI also doesn't set these properties in commercial devices, e.g.

linux/sound/soc/codecs/rt5640.c

Line 2514 in 53b27a2

/*

Somehow we need to make sure the properties are handled in the same way, whether they are set during enumeration or added manually. That's the part I don't get for now, this really can't be an ASoC specific change, can it?

The nice thing about swnode API that is has no connection to the device object(s). So, in board file you create a set of swnodes (properties) and when it's a right time, you attach them to the codec device. Clean up in reversed order. I would like to help, but I don't understand a full picture here, I don't know what is the impediment(s) you are talking about.

[plbossart]

The nice thing about swnode API that is has no connection to the device object(s). So, in board file you create a set of swnodes (properties) and when it's a right time, you attach them to the codec device. Clean up in reversed order. I would like to help, but I don't understand a full picture here, I don't know what is the impediment(s) you are talking about.

well, I don't really get why we would do something like sw_node_property_add(), and the existing driver then does device_property_read().

The type of things we care about is jack detection, and we set the property from the machine driver, but it's still used as

if (device_property_read_u32(component->dev,
				     "realtek,jack-detect-source", &val) == 0) {

In short, I don't see how the property would NOT be connected to the device object when it practice the device is the parameter to access these properties.

Confused (and hungry, which might explain the former :-))

[andy-shev]

Okay, let's forget above. Can you explain in a summary how enumeration process in ASoC works (how board file affects the codec enumeration and so on)?

Meanwhile I'm looking now in the code and it seems to me that board file should rather try to reprobe the codec (if there is a problem with enumeration order), otherwise I do not understand why it can't be simple done the way that board file adds swnode (you may name it in the same way as codec instance since you have it already, though it's not a good idea in principle) and attach at some (suitable) point to the codec device node.

[andy-shev]

Here is how codec is being registered in component framework, right?
https://elixir.bootlin.com/linux/v5.9-rc7/source/sound/soc/codecs/rt5640.c#L2851.
So, you need to have the same mechanism to register swnode which will be attached to the device node (perhaps by component framework). Or maybe somewhere here https://elixir.bootlin.com/linux/v5.9-rc7/source/sound/soc/sof/core.c#L161.

[plbossart]

not

ASoC works with the concept of 'component'. The I2C codec driver will e.g. probe and register a component. The PCI driver will register a DSP component. Nothing will happen until the snd_soc_card_register() is called. At that point, the card creation will call all the component.probe and in this specific example of rt5640 will figure out what sort of jack detection is desired.

So what happens here in that in the machine driver which creates the card, we add a property after the initial I2C probe, but before the component probe. We don't really have a 'board file', it's like a quirk added to the codec (and this can be changed based on DMI info, etc).

Note that this design was suggested by @jwrdegoede, we've only piggy-backed on it for SoundWire. Maybe there's a better way of doing things, I don't really know.

Does this help?

[bardliao]

@plbossart @andy-shev IMHO, the issue is that somehow we can't get ACPI property by device_property_read_ functions. That's why we need to add property in machine driver. I think the device_property_read_ functions should work with both device tree and ACPI. Maybe the issue is that our BIOS lacks the codec's property?

[jwrdegoede]

@plbossart @andy-shev IMHO, the issue is that somehow we can't get ACPI property by device_property_read_ functions. That's why we need to add property in machine driver. I think the device_property_read_ functions should work with both device tree and ACPI. Maybe the issue is that our BIOS lacks the codec's property?

So the issue here is that the codec drivers need a bunch of board specific info, like how jack-detect is hooked up, but also e.g. microphone routing info.

This info is typically not available in the ACPI tables at all. We use a combination of trying to have defaults which hopefully match most devices + DMI string based identification of the exact board on which we are running to adjust the settings from the defaults where necessary.

So reading the info directly from ACPI inside the codec driver simply is not possible.

We need to pass this info in some way from the machine-driver to the codec-driver. I made the choice to use device-properties for this, so that other platforms (e.g. devicetree based platforms) can directly set those properties in their device-tree files without needing to fill in a codec-driver specific platform_data struct.

I hope this helps explain why this is done the way it is done.

[jwrdegoede]

About the actual changes suggested here.

All modified drivers get a new "remove_properties()" helper, which looks up the codec-device by name, similar to how it is done in probe() and then calls device_remove_properties() on the device.

This lookup happens on the same singleton codec_name string (we assume the machine-driver only binds once) as the lookup in probe.

So IMHO it would be better to:

1. Make the codec_dev variable a global variable, like we do for codec_name already.
2. Remove the put_device on codec_dev from probe()'s main path
3. On probe error and remove do:

	device_remove_properties(codec_dev);
	put_device(codec_dev);

This removes the need for the helper function + second lookup and seems like a cleaner approach to me.

[plbossart]

replaced by #2810