On Fri, 18 Aug 2023 18:30:42 +0200, Pierre-Louis Bossart wrote:
The code doesn't look too bad but needs a bit more work. There are quite a few error handling issues, pm_runtime needs to be revisited and ACPI/EFI as well.
+enum calib_data {
tas2781_calib_data?
Well, as long as it's a local stuff, a suffix isn't really needed. If it makes thing too confusing, it should be named properly, of course, though.
+static int tas2781_read_acpi(struct tasdevice_priv *p, const char *hid) +{
- struct acpi_device *adev;
- struct device *physdev;
- LIST_HEAD(resources);
- const char *sub;
- int ret;
- adev = acpi_dev_get_first_match_dev(hid, NULL, -1);
- if (!adev) {
dev_err(p->dev,"Failed to find an ACPI device for %s\n", hid);return -ENODEV;- }
[1] need to take care of a resource leak here
Right, and that's rather a typo at the end of the function...
+err:
- dev_err(p->dev, "read acpi error, ret: %d\n", ret);
- put_device(physdev);
... this must be put_device(adev) instead physdev.
+static void tas2781_hda_playback_hook(struct device *dev, int action) +{
- struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
- dev_dbg(tas_priv->dev, "%s: action = %d\n", __func__, action);
- switch (action) {
- case HDA_GEN_PCM_ACT_OPEN:
pm_runtime_get_sync(dev);test if this actually works?
To be fair, most of driver codes don't check it, including the HD-audio core. (Actually, over 900 of 1300 calls have no check in the whole tree.)
It implies that forcing the check in each place is moot; rather the helper needs to be coded not to fail, IMO.
+static int tasdevice_hda_clamp(int val, int max) +{
- if (val > max)
val = max;- if (val < 0)
val = 0;- return val;
+}
I've seen that macro in the TAS2783 code as well, that sounds like a good helper function to share?
There is already clamp() macro, and I guess it can be replaced with clamp(val, 0, max).
- comps->dev = dev;
- strscpy(comps->name, dev_name(dev), sizeof(comps->name));
- ret = tascodec_init(tas_priv, codec, tasdev_fw_ready);
- if (ret)
return ret;need to do a put_autosuspend below, this is leaking a refcount.
Right, that needs an obvious leak. Let's fix it.
+static int tas2781_system_suspend(struct device *dev) +{
- struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
- int ret;
- dev_dbg(tas_priv->dev, "System Suspend\n");
- ret = pm_runtime_force_suspend(dev);
- if (ret)
return ret;that's usually the other way around, for system suspend you either want the device to be pm_runtime active, or if it's already suspended do nothing.
This is very odd to me.
This is a normal procedure, as stated in pm_runtime_force_suspend() definition:
/** * pm_runtime_force_suspend - Force a device into suspend state if needed. .... * Typically this function may be invoked from a system suspend callback to make * sure the device is put into low power state and it should only be used during * system-wide PM transitions to sleep states. It assumes that the analogous * pm_runtime_force_resume() will be used to resume the device.
thanks,
Takashi