Hi,
On 6/8/21 11:02 AM, Andy Shevchenko wrote:
On Tue, Jun 08, 2021 at 10:18:08AM +0200, Hans de Goede wrote:
On 6/8/21 12:35 AM, Pierre-Louis Bossart wrote:
From: Heikki Krogerus heikki.krogerus@linux.intel.com
The function device_add_properties() is going to be removed. Replacing it with software node API equivalents.
...
- device_remove_software_node(priv->codec_dev);
This is a problem, nothing guarantees codec_dev not going away before snd_byt_cht_es8316_mc_remove() runs. Although the only thing which I can come up with where this happens is unbinding the i2c-controller driver I still would like us to take this scenario into account.
I think it would be better to use device_create_managed_software_node() to tie the lifetime of the swnode to the lifetime of the device, this also removes the need for all the goto err cases (and introducing a remove function in the bytcr_rt5640.c case).
Which device? If you are telling about codec, the unload-load of the machine driver won't be successful or will produce a leak / warning / etc.
Yes I'm talking about the codec, and yes if the codec device goes away before the machine-driver is unbound things will likely already break. But the machine driver does not hold any explicit reference on the codec-device, so this might happen (I guess there might be a reference somewhere inside the ASoC code once devm_snd_soc_register_card() has returned successfully).
If you are telling about machine related device, it simply doesn't belong to it.
Am I got all this right?
This does mean that we could end up calling device_create_managed_software_node() on the same device twice, when the machine driver gets unbound + rebound, but that is an already existing issue with our current device_add_properties() usage.
Yep.
We could fix this (in a separate commit since it is an already existing issue) by adding a PROPERTY_ENTRY_BOOL("cht_es8316,swnode-created") property to the properties and checking for that being set with device_property_read_bool(codec, "cht_es8316,swnode-created")
Not sure it's a good idea, this sounds like a hack.
Right, which is why I also suggested that device_create_managed_software_node() could be modified to fail when called a second time on the same device, this is a check which probably would be good to add regardless. More specifically I guess that set_secondary_fwnode() could be made to return an error when replacing an existing secondary fwnode with a non NULL value, rather then just replacing it.
Or we could move the device_put(codec_dev) to snd_byt_cht_es8316_mc_remove().
This sounds better.
As I already mentioned I'm not a fan of all the goto err-s these patches introduce, this won't fix that.
Regards,
Hans