Re: [PATCH] mmc: sdhci-of-dwcmshc: add reset control support in suspend/resume

[Adrian Hunter <adrian.hunter@intel.com>]

On 17/04/2026 13:45, Artem Shimko wrote:
> Hi Andy,
> 
> Thank you for your review!
> 
> On Wed, Apr 15, 2026 at 5:52 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
>> No need to repeat the commit message in the comments. It does not add any value.
> 
> Got it =)
> 
>> This is serious behaviour change (might not be, one needs to understand what it
>> does in comparison to no reset (and in accordance with datasheet).
>>
>> Do you have any HW to test?
> 
> I made these changes so that our MMC controller (dwc) can handle the
> reset signal not only during the initialization function, but also
> during suspend and resume.
> Since the reset control was previously stored in vendor-specific
> private structures (rk35xx_priv and eic7700_priv), it was inaccessible
> from the common PM code.
> To make suspend/resume work for our platform, I had to move the reset
> pointer to the generic dwcmshc_priv structure, which required updating
> the Rockchip and EIC7700 variants accordingly.
> 
> 
> 
>> The driver seems can be refactored a lot (with no functional changes) by:
>> - replacing *sleep() with fsleep() API
>> - dropping unneeded checks like above, clk_disable_unprepare() is error
>> pointer-aware IIRC (or it can be made so it is either NULL or valid one)
>> - using 'return dev_err_probe()'
>>
>> This is just a side note in case you are interested.
> 
> Yes, sure, thank you!
> 
>> No need to add an extra blank line.
> okay =)
> 
> 
> Instead of moving the reset control directly into the common path,
> perhaps a cleaner solution would be to introduce a set of platform PM
> ops (e.g., suspend/resume callbacks in the variant data).
> These ops could be checked for NULL in the common
> dwcmshc_suspend/resume functions. For platforms that don't implement
> them, the behavior remains exactly as it was before (no reset
> assertion),
> ensuring zero risk of regression for Rockchip and EIC7700. Our
> platform would then simply implement these ops to handle the custom
> reset sequence.
> 
> What do you think about it?

Only dwcmshc_rk35xx_init() and sdhci_eic7700_reset_init() assign
dwc_priv->reset, so I am confused about what devices you intend
this patch for.

You definitely need to limit the change to devices that you know
for certain it will work.

> --
> Regards,
> Artem