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