On Fri, May 12, 2023 at 09:52:21AM -0500, Pierre-Louis Bossart wrote:
+static int cs42l43_sdw_interrupt(struct sdw_slave *sdw,
struct sdw_slave_intr_status *status)+{
- /*
* There is only a single bit in GEN_INT_STAT_1 and it doesn't clear if* IRQs are still pending so doing a read/write here after handling the* IRQ is fine.*/- sdw_read_no_pm(sdw, CS42L43_GEN_INT_STAT_1);
- sdw_write_no_pm(sdw, CS42L43_GEN_INT_STAT_1, 1);
- return 0;
+}
not really getting the comment and code above. Where is the IRQ handled? In the 'other non-SoundWire part"?
Yeah in the actual IRQ handler, I will update the comment to make this more clear.
- ret = regmap_register_patch(cs42l43->regmap, cs42l43_reva_patch,
ARRAY_SIZE(cs42l43_reva_patch));- if (ret) {
dev_err(cs42l43->dev, "Failed to apply register patch: %d\n", ret);goto err;- }
- pm_runtime_mark_last_busy(cs42l43->dev);
- pm_runtime_put_autosuspend(cs42l43->dev);
any reason why the two pm_runtime routines are not placed last, just before the return?
Yeah that probably makes more sense I will update.
- ret = cs42l43_power_up(cs42l43);
- if (ret)
return ret;- pm_runtime_set_autosuspend_delay(cs42l43->dev, 250);
- pm_runtime_use_autosuspend(cs42l43->dev);
- pm_runtime_set_active(cs42l43->dev);
- pm_runtime_get_noresume(cs42l43->dev);
you probably want a comment to explain that the get_noresume() is intentional to prevent the device from suspending before the workqueue is handled.
Yeah will add one.
- /*
* Don't care about being resumed here, but we do want force_resume to* always trigger an actual resume, so that register state for the* MCU/GPIOs is returned as soon as possible after system resume*/- pm_runtime_get_noresume(dev);
- ret = pm_runtime_force_suspend(dev);
- if (ret) {
dev_err(cs42l43->dev, "Failed to force suspend: %d\n", ret);return ret;- }
- pm_runtime_put_noidle(dev);
Is the get_noresume/put_noidle useful here? What does it do?
The hope was the comment would explain this :-) Yeah it is a slightly surprising sequence. It is about ensuring force_resume runs a runtime resume, which it will only do if the reference count is resumed when we suspend.
I will add a little more to the comment to hopefully clarify why we are doing this.
And it seems wrong anyways, if pm_runtime_force_suspend() fails then the usage-count is not decreased.
Yeah that is bug, thanks for the spot I will fix that up.
+static int __maybe_unused cs42l43_runtime_resume(struct device *dev) +{
- struct cs42l43 *cs42l43 = dev_get_drvdata(dev);
- unsigned int reset_canary;
- int ret;
- dev_dbg(cs42l43->dev, "Runtime resume\n");
- ret = cs42l43_wait_for_attach(cs42l43);
is there a specific reason why the existing initialization_complete is not used?
Not massively, the driver does a fair amount of detaching and attaching during probe (has to soft reset a few times and they cause the device to fall off the bus). It was just slightly easier to keep track of all of them keeping the code internal to the driver and once we were doing it anyway it was less code to use the same mechanism on resume.
Thanks, Charles