On 9/27/21 12:10 PM, Mark Brown wrote:
> On Mon, Sep 27, 2021 at 11:48:56AM -0500, Pierre-Louis Bossart wrote:
>> On 9/27/21 11:06 AM, Mark Brown wrote:
>
>>> More specifically what it does is make the invalidation of the register
>>> cache unconditional. It doesn't really matter if the invalidation is
>>> done on suspend or resume, so long as it happens before we attempt to
>>> resync - this could also be done by deleting the first_hw_init check.
>
>> Mark, that's exactly my point: if the amp rejoins the bus, we will
>> *always* mark the cache as dirty, before the resync is done in the
>> resume sequence.
>
> Ah, yes - I see.
>
>> I am really trying to figure out if we have a major flaw in the resume
>> sequence and why things are different in the case of the Maxim amp.
>
>> Instead of changing the suspend sequence, can we please try to modify
>> the max98373_io_init() routine to unconditionally flag the cache as
>> dirty, maybe this points to a problem with the management of the
>> max98373->first_hw_init flag.
>
> A quick survey of other drivers suggests that this pattern should be
> factored out into some helpers as it looks like there's several ways of
> implementing it that look very similar but not quite the same...
No disagreement here, we tried really hard to enforce a common pattern
for suspend-resume, but i just noticed that the maxim amp driver is
different on suspend (resume is consistent with the rest).
static int __maybe_unused rt711_dev_suspend(struct device *dev)
{
struct rt711_priv *rt711 = dev_get_drvdata(dev);
if (!rt711->hw_init)
return 0;
cancel_delayed_work_sync(&rt711->jack_detect_work);
cancel_delayed_work_sync(&rt711->jack_btn_check_work);
cancel_work_sync(&rt711->calibration_work);
regcache_cache_only(rt711->regmap, true);
return 0;
}
static int __maybe_unused rt1308_dev_suspend(struct device *dev)
{
struct rt1308_sdw_priv *rt1308 = dev_get_drvdata(dev);
if (!rt1308->hw_init)
return 0;
regcache_cache_only(rt1308->regmap, true);
return 0;
}
static __maybe_unused int max98373_suspend(struct device *dev)
{
struct max98373_priv *max98373 = dev_get_drvdata(dev);
int i;
<<<< missing test
/* cache feedback register values before suspend */
for (i = 0; i < max98373->cache_num; i++)
regmap_read(max98373->regmap, max98373->cache[i].reg,
&max98373->cache[i].val);
<<<< why is this needed???
regcache_cache_only(max98373->regmap, true);
return 0;
}
